Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Support for choosing image to boot in runtime #2044

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

edersondisouza
Copy link
Contributor

MCUboot assumes that the images being booted live on a flash (or will be loaded to it, in case of serial support) or something that pretends to be a flash. It will usually check version of available images on flash partition slots, and choose the newer one to perform an update, with support to fallback to a previous working image in case the new one fails.

Another use cause for MCUboot, however, is to be able to load an image from an arbitrary source, in fact, not caring about the update facilities. In that case, the application will usually check some hardware straps to decide from which source to get the image to boot.

This RFC PR adds support for such scenarios: it uses single loader to load an image from a set of "sources". The single loader loops through available sources and the first one to succeed signature/validation boots. To access the images, weak functions flash_map_id_get_next() and flash_map_id_get_current() are used. Default implementation keeps current behaviour for single loader, i.e. just loads from FLASH_AREA_IMAGE_PRIMARY(0).

It is expected applications will reimplement these functions, allowing them to define a priority of different sources. As these different storage media may be ready only, MCUboot won't attempt to update them to record last source to succeed or so, it's application responsibility to define the correct priority of sources on every boot.

This PR also moves RAM loading code to its own file, so it's available for single loaders, as well as provide one example of such approach, using two slots on a FRDM-K64F.

While some of the source devices can not be flash devices at all (for instance, image is made available via I2C or eSPI bus), feedback on #2031 supports simply implementing the Flash API for them at Zephyr level, hence this new RFC PR.

Please review - comments and suggestions on how to better achieve this are highly appreciated =D

This is a follow on for #2031.

@edersondisouza
Copy link
Contributor Author

Any comments? @d3zd3z?

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 4, 2024

Having a quick read of this description (have not looked at code), I do not think this belongs in MCUboot, it does not deal with firmware updates and deals with selecting which image to boot from a bunch of devices, which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device by manipulating the end product e.g. removing one pin from an external flash device.

I suggest that you create your own bootloader using the MCUboot files by bringing MCUboot in as a module - TF-M does this so can be used as an inspiration for how to do this, or just fork MCUboot into your own bootloader and make changes in that, but I do not envisage this being in MCUboot itself as it really is a separate system you want that does not tie in with MCUboot's fundamentals

@butok
Copy link
Contributor

butok commented Sep 4, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

@butok
Copy link
Contributor

butok commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

OK. So an user can choose full security or the ability to unbrick the device or reset it to factory settings.

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

OK. So an user can choose full security or the ability to unbrick the device or reset it to factory settings.

That's what swap using move or directXIP with revert is designed for, you update the application from the application, and check it works before confirming it should stay, the bootloader is a dumb bootloader and just deals with verifying the image, you reduce the attack vector by removing features that are not essential to that and use hardware to enforce those settings, which is what TF-M does with it's MCUboot (called bl2 there) build

@butok
Copy link
Contributor

butok commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

OK. So an user can choose full security or the ability to unbrick the device or reset it to factory settings.

That's what swap using move or directXIP with revert is designed for, you update the application from the application, and check it works before confirming it should stay, the bootloader is a dumb bootloader and just deals with verifying the image, you reduce the attack vector by removing features that are not essential to that and use hardware to enforce those settings, which is what TF-M does with it's MCUboot (called bl2 there) build

Can we merge TFM bl2 features back to MCUBoot? Of cause, make them configurable,

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

OK. So an user can choose full security or the ability to unbrick the device or reset it to factory settings.

That's what swap using move or directXIP with revert is designed for, you update the application from the application, and check it works before confirming it should stay, the bootloader is a dumb bootloader and just deals with verifying the image, you reduce the attack vector by removing features that are not essential to that and use hardware to enforce those settings, which is what TF-M does with it's MCUboot (called bl2 there) build

Can we merge TFM bl2 features back to MCUBoot? Of cause, make them configurable,

Not sure what you mean, TF-M's bl2 is MCUboot

@butok
Copy link
Contributor

butok commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

OK. So an user can choose full security or the ability to unbrick the device or reset it to factory settings.

That's what swap using move or directXIP with revert is designed for, you update the application from the application, and check it works before confirming it should stay, the bootloader is a dumb bootloader and just deals with verifying the image, you reduce the attack vector by removing features that are not essential to that and use hardware to enforce those settings, which is what TF-M does with it's MCUboot (called bl2 there) build

Can we merge TFM bl2 features back to MCUBoot? Of cause, make them configurable,

Not sure what you mean, TF-M's bl2 is MCUboot

Not fully, it has own variant: https://github.com/zephyrproject-rtos/trusted-firmware-m/tree/main/bl2

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 5, 2024

which then adds security issues because downgrades are implicitly allowed and an attacker can therefore boot from a possibly vulnerable device

But MCUBoot has a recovery, with the same security issue ;)

It does, and in no end device to be considered secure should it ever be used

OK. So an user can choose full security or the ability to unbrick the device or reset it to factory settings.

That's what swap using move or directXIP with revert is designed for, you update the application from the application, and check it works before confirming it should stay, the bootloader is a dumb bootloader and just deals with verifying the image, you reduce the attack vector by removing features that are not essential to that and use hardware to enforce those settings, which is what TF-M does with it's MCUboot (called bl2 there) build

Can we merge TFM bl2 features back to MCUBoot? Of cause, make them configurable,

Not sure what you mean, TF-M's bl2 is MCUboot

Not fully, it has own variant: https://github.com/zephyrproject-rtos/trusted-firmware-m/tree/main/bl2

Those are the interface files, which is no different then the boot/zephyr folder in MCUboot, it's the shim layer

@nashif
Copy link
Contributor

nashif commented Sep 6, 2024

I do not think this belongs in MCUboot, it does not deal with firmware updates

mcuboot is a bootloader, no? why using mcuboot does mean you have to do updates? Bootloader can be used for doing many other things, beside updates.

@d3zd3z
Copy link
Member

d3zd3z commented Sep 6, 2024

So, to clarify a bit about the purpose of mcuboot. MCUboot really does two things: 1. it validates that the code running in flash has been properly signed, and 2. it performs the last step of an update, the atomic swap of the two images.

The problem is that we need some initial bootloader that is immutable. Because of this, we want it to have as little functionality as possible. It is not, and should never be responsible for getting images from some other location.

So, the question comes down to: what is this change trying to recover from.

MCUboot won't swap or load images that haven't been properly signed. At least some level of testing should be preventing most of the cases of needing any kind of recovery. The revert operation should cover the cases when an image is deployed but is discoverd that it doesn't work on some hardware.

Any case of say power failure during the swap operation causing the device to not properly finish when power is restored should be considered bugs that need to be fixed.

Is there some other scenario where recovery is needed?

Aside from this, additional functionality would be the realm of a second stage bootloader, where this second bootloader would be the image upgraded by mcuboot.

@d3zd3z
Copy link
Member

d3zd3z commented Sep 7, 2024

That said, I don't actually find the arguments about security of this to be a concern. Allowing multiple sources of boot images isn't going to be less secure. We already make the assumption that the upgrade slot can be written with arbitrary data, and that is the case with these other slots. The images still have to be signed in order to be run.

One possibly security concern is if we are XIP-ing the code, could the flash be changed after the signature verification. This actually applies even with loading to RAM. Fixing that would require changing the way ram loading works, where it would have to very the signature of the image after it has been loaded into RAM. That might be worth fixing anyway. Ram loading in MCUboot doesn't get a lot of love, as there still are a lot of devices that XIP out of flash.

I've enabled the CI run, which so far is showing a failure in mynewt.

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 9, 2024

I do not think this belongs in MCUboot, it does not deal with firmware updates

mcuboot is a bootloader, no? why using mcuboot does mean you have to do updates? Bootloader can be used for doing many other things, beside updates.

Using mcuboot without firmware updates is pointless and makes no sense, you would just skip mcuboot entirely and load your application direct to the start of flash.

@butok
Copy link
Contributor

butok commented Sep 9, 2024

The problem is that we need some initial bootloader that is immutable. Because of this, we want it to have as little functionality as possible. It is not, and should never be responsible for getting images from some other location.

If it is configurable. It can have any amount of functionality.

@edersondisouza
Copy link
Contributor Author

[...]
I've enabled the CI run, which so far is showing a failure in mynewt.

Thanks for that! I've fixed those!

v2:

  • Fix RAM load code separation issues on Zephyr, mynewt and simulator.
  • Rebased.

@edersondisouza
Copy link
Contributor Author

So, to clarify a bit about the purpose of mcuboot. MCUboot really does two things: 1. it validates that the code running in flash has been properly signed, and 2. it performs the last step of an update, the atomic swap of the two images.
[...]

That is already not the case when some one uses "single application slot"/"single loader", right? In this case, each MCUboot port (at least Zephyr and mynewt, AFAICT) simply verifies the image and then loads/run it, no swap. This PR simply adds some hooks to Zephyr single loader that make it easier for a MCUboot based bootloader to choose, based on some runtime verification, from which source (a flash partition, as any kind of underlying storage will just pretend it's flash) to load the image to run.

That said, I don't actually find the arguments about security of this to be a concern. Allowing multiple sources of boot images isn't going to be less secure. We already make the assumption that the upgrade slot can be written with arbitrary data, and that is the case with these other slots. The images still have to be signed in order to be run.

One possibly security concern is if we are XIP-ing the code, could the flash be changed after the signature verification. This actually applies even with loading to RAM. Fixing that would require changing the way ram loading works, where it would have to very the signature of the image after it has been loaded into RAM. That might be worth fixing anyway. Ram loading in MCUboot doesn't get a lot of love, as there still are a lot of devices that XIP out of flash.
[...]

About the RAM loading, verification is actually done on RAM after loading is done, so this case is already covered. Indeed, the other, and actually bigger, change in this PR is to split the RAM loading code, so that it can be used by single loader (RAM loading code is currently behind the non single loader code path). There's even a part of me that wants to avoid the - possibly - long discussion here, and have another PR just with the RAM loading code split.

Any thoughts, @d3zd3z and @nordicjm?

@nashif
Copy link
Contributor

nashif commented Sep 12, 2024

@nordicjm

Using mcuboot without firmware updates is pointless and makes no sense, you would just skip mcuboot entirely and load your application direct to the start of flash.

This is your view of the world and what bootloader should do. mcuboot is defined as a bootloader with update capabilities, does not mean you have to update anything to use mcuboot. There are different usecases that go beyond your own, so please be inclusive here.
If mcuboot is only about updates, then it should not be called a bootloader in the context of zephyr, maybe mcu-updater? in that case we will figure a way to introduce a proper bootloader into zephyr that does more than just updates.

@nordicjm
Copy link
Collaborator

@nordicjm

Using mcuboot without firmware updates is pointless and makes no sense, you would just skip mcuboot entirely and load your application direct to the start of flash.

This is your view of the world and what bootloader should do. mcuboot is defined as a bootloader with update capabilities, does not mean you have to update anything to use mcuboot. There are different usecases that go beyond your own, so please be inclusive here. If mcuboot is only about updates, then it should not be called a bootloader in the context of zephyr, maybe mcu-updater? in that case we will figure a way to introduce a proper bootloader into zephyr that does more than just updates.

I have been using MCUboot as a user since 2018, I have been a MCUboot developer since 2021, so yes I have a very good idea of what this project is, how it works, what the goal of it is and what features it needs to support. Many people use MCUboot and do their own thing with it, the concept introduced here is in my opinion not a good fit for the project, that's not to say it can't be done, it just means you should have it as a fork or patch in your project, as you say when people post modules to zephyr "as an external module", and people do that with MCUboot. We do that in nordic for the nRF connect SDK, people have their own forks where they do things like have the second image as a file on a filesystem (which, again, is not a good fit for a feature to include in MCUboot but there is no reason someone can't take MCUboot, apply the changes they want for it and use that, then pull in fixes from MCUboot to their fork)

@butok
Copy link
Contributor

butok commented Sep 16, 2024

what the goal of it is and what features it needs to support. Many people use MCUboot and do their own thing with it, the concept introduced here is in my opinion not a good fit for the project, that's not to say it can't be done, it just means you should have it as a fork or patch in your project, as you say when people post modules to zephyr "as an external module", and people do that with MCUboot. We do that in nordic for the nRF connect SDK, people have their own forks where they do things like have the second image as a file on a filesystem (which, again, is not a good fit for a feature to include in MCUboot but there is no reason someone can't take MCUboot, apply the changes they want for it and use that, then pull in fixes from MCUboot to their fork)

It is pity that others have to spend resources to create and maintain their own forks of MCUBoot, such as the nRD SDK.
Where can we find the mentioned goals of the MCUBoot project?

@edersondisouza edersondisouza force-pushed the runtime-source branch 2 times, most recently from ee1be0d to 0ffcf4f Compare September 18, 2024 18:39
@teburd
Copy link

teburd commented Sep 24, 2024

is it really that uncommon to say, have a jumper that lets you load an image from sd, onboard flash, qspi flash, or some other source? It's a fairly common boot rom feature for example on NXP parts, this would enable mimicking that behavior in mcuboot which would be nice and obviously useful for a case we have at Intel

@nordicjm
Copy link
Collaborator

is it really that uncommon to say, have a jumper that lets you load an image from sd, onboard flash, qspi flash, or some other source? It's a fairly common boot rom feature for example on NXP parts, this would enable mimicking that behavior in mcuboot which would be nice and obviously useful for a case we have at Intel

You have that, build in firmware loader mode and use a GPIO for selecting if you boot the primary or secondary image. Multiple boot sources are not and should not be supported (in tree)

@butok
Copy link
Contributor

butok commented Sep 25, 2024

Multiple boot sources are not and should not be supported (in tree)

If multiple-boot is configurable, it should be posible.
For example, the NXP ROM Secure Bootloaders have this possibility (if it's enabled).

@nordicjm
Copy link
Collaborator

Multiple boot sources are not and should not be supported (in tree)

If multiple-boot is configurable, it should be posible. For example, the NXP ROM Secure Bootloaders have this possibility (if it's enabled).

We're talking about MCUboot, not <insert_name_of_completely_unrelated_project_here>, MCUboot is designed around 1 (single slot mode) or 2 (dual slot mode) slots

@d3zd3z
Copy link
Member

d3zd3z commented Sep 25, 2024

So, I don't have a particular problem with the concept of this change, especially if the feature is optional. As long as we document things meaningful, I don't think the security rollback concerns are an issue. We need to document that it is possible, and that if rollback protection is needed, hardware rollback protection will be needed.

But, the change is also fairly large, so I will need to review it.

@nvlsianpu
Copy link
Collaborator

I do not think this belongs in MCUboot, it does not deal with firmware updates

mcuboot is a bootloader, no? why using mcuboot does mean you have to do updates? Bootloader can be used for doing many other things, beside updates.

Using mcuboot without firmware updates is pointless and makes no sense, you would just skip mcuboot entirely and load your application direct to the start of flash.

Actually I encountered serious request for MCUboot which is only able to verify and run the given content, It was why single loader was added (yes - it was meant to be used without any MCUboot's recovery, FWU was planed to be done by direct memory programing). That's mean that it was requirement for enable only secure-boot by the MCUboot.

FIH_DECLARE(fih_rc, FIH_FAILURE);

rc = flash_area_open(FLASH_AREA_IMAGE_PRIMARY(0), &_fa_p);
assert(rc == 0);
while (flash_map_id_get_next(&flash_id, reset)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid looping when feature not enabled.

@d3zd3z
Copy link
Member

d3zd3z commented Nov 13, 2024

Actually I encountered serious request for MCUboot which is only able to verify and run the given content, It was why single loader was added (yes - it was meant to be used without any MCUboot's recovery, FWU was planed to be done by direct memory programing). That's mean that it was requirement for enable only secure-boot by the MCUboot.

This is a quite reasonable request.

The main security issue I see with this prompting for image to run has to do with rollback protection. At least some security requirements require fairly strict adherence to non-rollback, which for mcuboot means HW rollback counters. In that case, selecting an older image to run would not actually allow it to run.

That said, we don't require rollback protection to be enabled (in contrast to SUIT, which I'm not sure if it requires it, does require the counter to be in every image), and this makes sense with out that.

Instead of normal MCUboot flow, that checks flash slots to find update
images, this patch enables an MCUboot application to chose from which
available slots to load an image.

This allows an application, for instance, to check hardware
configuration at runtime (by checking hardware straps, state of GPIOs,
etc) and decide from where to load an image to boot.

MCUboot will basically loop through provided image sources (flash slots)
and boot the first one that succed signature/validation. To provide the
image sources, the application need to provide strong implementation for
functions flash_map_id_get_next() and flash_map_id_get_current(). The
default, weak, implementations just keep current behaviour for single
loader, i.e. just load from FLASH_AREA_IMAGE_PRIMARY(0).

Note that in this case, MCUboot won't try to record if image succeeded
boot, it will only boot the image provided.

Signed-off-by: Ederson de Souza <[email protected]>
Signed-off-by: Tom Burdick <[email protected]>
A sample for runtime chose image on FRDM K64F. It provides implementation
for Zephyr flash_area_open_custom(), so the right flash map
implementation is used, and MCUboot flash_map_id_get_next() and
flash_map_id_get_current() to prioritize sources. It should show what is
expected from an application to be able to use non-flash sources for
images.

In this sample, one can influence from which slot image will be loaded
by pressing a button on the board.

For more details on how to build and test the samples, check the
provided README.md.

Signed-off-by: Ederson de Souza <[email protected]>
Signed-off-by: Tom Burdick <[email protected]>
@teburd
Copy link

teburd commented Nov 18, 2024

Rebased to use the #2062 now merged into the tree

Please take a look again if you can @nordicjm @nvlsianpu @d3zd3z

Thank you!

@teburd
Copy link

teburd commented Nov 18, 2024

Actually I encountered serious request for MCUboot which is only able to verify and run the given content, It was why single loader was added (yes - it was meant to be used without any MCUboot's recovery, FWU was planed to be done by direct memory programing). That's mean that it was requirement for enable only secure-boot by the MCUboot.

This is a quite reasonable request.

The main security issue I see with this prompting for image to run has to do with rollback protection. At least some security requirements require fairly strict adherence to non-rollback, which for mcuboot means HW rollback counters. In that case, selecting an older image to run would not actually allow it to run.

That said, we don't require rollback protection to be enabled (in contrast to SUIT, which I'm not sure if it requires it, does require the counter to be in every image), and this makes sense with out that.

As I understand it, the sample here is only meant to show one possible usage. Some bootloaders, like the ROM bootloader builtin to imxrt for example, allow selecting the image source using gpio pins.

This isn't about running images with rollbacks and such. This is about having multiple sources of images and being able to select which one to boot from.

@nordicjm
Copy link
Collaborator

nordicjm commented Nov 19, 2024

This does not belong in MCUboot to me, this should be implemented as a custom hook in an application or module, I cannot approve it.

@teburd
Copy link

teburd commented Nov 19, 2024

This does not belong in MCUboot to me, this should be implemented as a custom hook in an application or module, I cannot approve it.

Can you please give some more details here as to why, its a tiny feature add that allows "flash" source selection and a sample showing its usage. Again this is a fairly common feature of boot roms/loaders. We are not talking about upgrading the images at all here. We do want the image signing and validation still.

@nordicjm
Copy link
Collaborator

nordicjm commented Nov 20, 2024

This does not belong in MCUboot to me, this should be implemented as a custom hook in an application or module, I cannot approve it.

Can you please give some more details here as to why, its a tiny feature add that allows "flash" source selection and a sample showing its usage. Again this is a fairly common feature of boot roms/loaders. We are not talking about upgrading the images at all here. We do want the image signing and validation still.

MCUboot is designed for images with 2 slots, it can be used for different configurations but the base bootloader is designed around that concept, masquerading additional hidden slots that can't actually be accessed or used at all from MCUboot/application as these 2 slots does not fit with the design of this bootloader. That's not to say you can't or shouldn't do it, as said a similar thing is done by nordic out of tree, using hooks to transfer an image to another CPU, which is what I advise should be done here, and image signing/validation would still be present on such a system. Whilst you don't need the firmware updating portion of the code the vast majority of users using MCUboot do, so adding a feature that completely breaks another feature which arguably is the most important part of why people use this bootloader if this new option is enabled does not seem like a good thing to have included.

@teburd
Copy link

teburd commented Nov 20, 2024

This does not belong in MCUboot to me, this should be implemented as a custom hook in an application or module, I cannot approve it.

Can you please give some more details here as to why, its a tiny feature add that allows "flash" source selection and a sample showing its usage. Again this is a fairly common feature of boot roms/loaders. We are not talking about upgrading the images at all here. We do want the image signing and validation still.

MCUboot is designed for images with 2 slots, it can be used for different configurations but the base bootloader is designed around that concept, masquerading additional hidden slots that can't actually be accessed or used at all from MCUboot/application as these 2 slots does not fit with the design of this bootloader. That's not to say you can't or shouldn't do it, as said a similar thing is done by nordic out of tree, using hooks to transfer an image to another CPU, which is what I advise should be done here, and image signing/validation would still be present on such a system. Whilst you don't need the firmware updating portion of the code the vast majority of users using MCUboot do, so adding a feature that completely breaks another feature which arguably is the most important part of why people use this bootloader if this new option is enabled does not seem like a good thing to have included.

So to clarify, in your opinion here we should fork mcuboot as mcuboot only caters to particular use cases? I'd note that this typically has downsides as opposed to keeping things under one roof, particularly over a 150 line patch. Despite Nordic also doing something similar?

Is that the consensus of the mcuboot maintainers? If so that's fine, really. I don't want to waste anymore time here trying to get patches in if that's the case.

@nordicjm
Copy link
Collaborator

nordicjm commented Nov 20, 2024

You don't need to fork anything, hooks do not require any changes to the repo, they can be added by a module, there is Kconfig to enable it. And the above is my view, I'm not going to block this I'm just not going to approve it, if @d3zd3z or others think different then they can approve/merge/comment. See here for example: https://github.com/nrfconnect/sdk-nrf/tree/main/modules/mcuboot/hooks it is not in the mcuboot repository

@teburd
Copy link

teburd commented Nov 20, 2024

You don't need to fork anything, hooks do not require any changes to the repo, they can be added by a module, there is Kconfig to enable it. And the above is my view, I'm not going to block this I'm just not going to approve it, if @d3zd3z or others think different then they can approve/merge/comment. See here for example: https://github.com/nrfconnect/sdk-nrf/tree/main/modules/mcuboot/hooks it is not in the mcuboot repository

Got it! Thank you for the clarification.

@edersondisouza
Copy link
Contributor Author

You don't need to fork anything, hooks do not require any changes to the repo, they can be added by a module, there is Kconfig to enable it. And the above is my view, I'm not going to block this I'm just not going to approve it, if @d3zd3z or others think different then they can approve/merge/comment. See here for example: https://github.com/nrfconnect/sdk-nrf/tree/main/modules/mcuboot/hooks it is not in the mcuboot repository

I like the idea of using external modules and hooks, thanks for the insight. But it seems no useful hooks are available here - I'd say at least boot_go should get some boot_go_hook so that image selection mechanism can be overridden. Is that a reasonable direction in your view, @nordicjm? If so, I'm happy to send a new PR creating this new hook and supporting it at least on zephyr/single_loader.c.

@nordicjm
Copy link
Collaborator

You don't need to fork anything, hooks do not require any changes to the repo, they can be added by a module, there is Kconfig to enable it. And the above is my view, I'm not going to block this I'm just not going to approve it, if @d3zd3z or others think different then they can approve/merge/comment. See here for example: https://github.com/nrfconnect/sdk-nrf/tree/main/modules/mcuboot/hooks it is not in the mcuboot repository

I like the idea of using external modules and hooks, thanks for the insight. But it seems no useful hooks are available here - I'd say at least boot_go should get some boot_go_hook so that image selection mechanism can be overridden. Is that a reasonable direction in your view, @nordicjm? If so, I'm happy to send a new PR creating this new hook and supporting it at least on zephyr/single_loader.c.

Had a quick look and seems there is a bit of a disjoint between loader.c and single image/firmware updater loader.c files, the normal loader.c file uses flash_area_id_from_multi_image_slot to look up the slots to open but the single image/firmware updater loader.c file directly uses FLASH_AREA_IMAGE_PRIMARY(0) directly, even though support for single image loader is in the flash_area_id_from_multi_image_slot function, so it could either be through making flash_area_id_from_multi_image_slot, flash_area_id_to_multi_image_slot and flash_area_id_from_direct_image extensible with hooks which would leave the common boot code (which I assume is OK as it is?) or could be done by making boot_go extensible. Thoughts @de-nordic ? Would need a Kconfig for enabling it separately to the other hooks or without a hook but falling back on the default if the hook one is not defined

@de-nordic
Copy link
Collaborator

Had a quick look and seems there is a bit of a disjoint between loader.c and single image/firmware updater loader.c files, the normal loader.c file uses flash_area_id_from_multi_image_slot to look up the slots to open but the single image/firmware updater loader.c file directly uses FLASH_AREA_IMAGE_PRIMARY(0) directly, even though support for single image loader is in the flash_area_id_from_multi_image_slot function, so it could either be through making flash_area_id_from_multi_image_slot, flash_area_id_to_multi_image_slot and flash_area_id_from_direct_image extensible with hooks which would leave the common boot code (which I assume is OK as it is?) or could be done by making boot_go extensible. Thoughts @de-nordic ? Would need a Kconfig for enabling it separately to the other hooks or without a hook but falling back on the default if the hook one is not defined

I would rather go with hook and preferably operate on flash area objects directly rather than flash area id.
Note that hook and evaluation of what will be boot next should happen before we do any moves, because it makes no sense to override swap that has already happened.

Generally I would like to remove any reliance on area id, or whatever that is called, from the MCUboot internals and directly operate on objects representing potential boot source, currently flash area objects, and drop mapping, of however the sources/slots/DTS partitions are defined in these systems, to backends for that systems.

Preferably I would like to have image being represented as chain of potential boot sources, that the MCUboot will iterate over to find the one will be next for the given image, this also drops reliance on slot ids, because position in chain would be defined by how system maps own structures to objects in chain - here usr hook could be used, at startup, to add additional boot sources to image chain. This would also benefits us with ability to define more than two slots per image, and we do not have to care map it to any slot/area.

The current/next functions kinda seem to fit into the idea, but in reality are just gluing into existing arch.

Some time ago I have started work on moving flash_area_open/flash_area_close to a single location to reduce potential error paths and move initialization of objects into one place (#1569), unfortunately I do not have time to do that now, but I have been able to add support to Zephyr to instantiate Flash Area object directly from DTS, without reliance on flash map. Both of these things are an attempt to remove reliance on Flash Area ID in Zephyr, and in MCUboot. Adding more reliance on IDs just digs us in.

@de-nordic
Copy link
Collaborator

You don't need to fork anything, hooks do not require any changes to the repo, they can be added by a module, there is Kconfig to enable it. And the above is my view, I'm not going to block this I'm just not going to approve it, if @d3zd3z or others think different then they can approve/merge/comment. See here for example: https://github.com/nrfconnect/sdk-nrf/tree/main/modules/mcuboot/hooks it is not in the mcuboot repository

I like the idea of using external modules and hooks, thanks for the insight. But it seems no useful hooks are available here - I'd say at least boot_go should get some boot_go_hook so that image selection mechanism can be overridden. Is that a reasonable direction in your view, @nordicjm? If so, I'm happy to send a new PR creating this new hook and supporting it at least on zephyr/single_loader.c.

Please propose hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants