-
Notifications
You must be signed in to change notification settings - Fork 827
Feature: images in image support #2465
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
base: main
Are you sure you want to change the base?
Feature: images in image support #2465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really not at all how MCUboot is designed to operate, if you want this then you should use the hooks MCUboot has to run your functions for doing this in your own application or module
@nordicjm thanks for the feedback. While adding this feature we have not change behavior of mcuboot. It is just extended. |
I find this feature useful for bundling multiple images into single envelope so that it can decrease the authentication burden. We may have a discussion on how to infuse it better for the architecture. Yet, adding a hook for tailoring is not the main purpose. The main purpose is to add capability of bundling multiple packs into single image for verification which enhances performance in MCUBoot in general. |
You have taken a mode and completely made up an entirely new container format without any discussion or thought about other MCUboot modes, security, previous version compatibility or design queries or how other supported OS's would implement such a feature notwithstanding a complete lack of documentation. Not to say that such a feature cannot be added, but this is really not at all the correct way to do it |
@nordicjm We tried to add it without changing existing feature as possible as, There might be some items that we missed. We are eager to refactor it as per of your and maintainers feedback and direction to handle it in appropriate way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the inline comments, before this can be merged, we will need to add simulator support to test this. The added test doesn't need to test upgrade robustness, as that should be handled the same as a regular single image case. But, it should test the subimages loading (providing buffers), and either be an extension of the existing ram loading tests, or an additional test.
In addition, please enhance the documentation to cover this feature. Likely add a mention in design.md that references a new documentation file describing how subimages work.
Beyond this. I do think this makes sense to include in mcuboot in a general sense, as long as the issues here are addressed. Going forward, I forsee ramloading becoming more common, and this addresses a few concerns with the current multiimage handling:
- The not-quite-working mutual dependency issues won't be a concern as this subimage always updates all images together.
- Future support for quantum cryptography, where signatures are often quite large, will benefit from only having a single signature.
boot/bootutil/src/loader.c
Outdated
boot_state_clear(NULL); | ||
|
||
FIH_CALL(context_boot_go, fih_rc, &boot_data, rsp); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality should be integrated into context_boot_go
. boot_go
is a wrapper for on-target vs simulated usage, and any functionality here would be skipped in the simulator.
However, I don't think we should have the existing ram load load this into ram first, and then have process_sub_images split that out, but we should enhance the ram loading process to load the subimages if that is defined and used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality should be integrated into context_boot_go. boot_go is a wrapper for on-target vs simulated usage, and any functionality here would be skipped in the simulator.
There are two context_boot_go ( 1 , 2 ) function, the feature moved under the one that execute for (MCUBOOT_DIRECT_XIP || MCUBOOT_RAM_LOAD). The executed rust tests as it is seems works, not applied any change yet.

However, I don't think we should have the existing ram load load this into ram first, and then have process_sub_images split that out, but we should enhance the ram loading process to load the subimages if that is defined and used.
For RAM mode MCUboot first load image into RAM than validate it in RAM.
By the feature that propose in this PR main image (Image0) need to be validated first before processing sub images then indirectly sub-image can be moved to target address. So that It seems not applicable it be moved under ram loading function.
Please let me know if I misinterpreted the existing code.
boot/bootutil/src/loader.c
Outdated
boot_state_clear(NULL); | ||
|
||
FIH_CALL(context_boot_go, fih_rc, &boot_data, rsp); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality should be integrated into context_boot_go
. boot_go
is a wrapper for on-target vs simulated usage, and any functionality here would be skipped in the simulator.
However, I don't think we should have the existing ram load load this into ram first, and then have process_sub_images split that out, but we should enhance the ram loading process to load the subimages if that is defined and used.
#endif /* MCUBOOT_DIRECT_XIP || MCUBOOT_RAM_LOAD */ | ||
|
||
|
||
#if (BOOT_IMAGE_NUMBER == 1) && defined(MCUBOOT_RAM_LOAD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should get it's own feature defined (maybe MCUBOOT_SUBIMAGES
) instead of just assuming that if we have a single image that is ram loaded subimages would be wanted.
In addition, we should define a new flag in the header to indicate when subimages are present in the image itself. Functionality wise:
- If
MCUBOOT_SUBIMAGES
it not defined, but the flag is present, fail. - if
MCUBOOT_SUBIMAGES
is defined, the flag will indicate whether a single image is loaded, or multiple images.
In other words, separate the implementation of the feature from the flags indicate whether it is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for feedback, checked it, as per of my check it seems new flags may not be fit for this feature due to for this feature MCUBOOT_IMAGE_NUMBER should be 1, unless you do not want it supports "multi image X multi-subimages".
And for Zephyr port, MCUBOOT_IMAGE_NUMBER set by zephyr Kconfig parameter, if we add MCUBOOT_SUBIMAGES that need to be set by each port of MCUboot.
Additionally if we define a new flag in mcuboot header, I am not clear how it should be set, please see existing options: https://github.com/mcu-tools/mcuboot/blob/main/scripts/imgtool/image.py#L804 The header flag set indirectly depend on some parameters specified.
So I think "#if (BOOT_IMAGE_NUMBER == 1) && defined(MCUBOOT_RAM_LOAD)" might be better,
Please let me know you would like to new flag be defined?
I do think this is a useful/valuable enough of a feature to have the discussion now and figure out how to implement this properly. One thing is that I don't think this should be a "fixup" that runs after the existing ramload runs, but that the ramload should be extended to support subimages. See my earlier comment, and please add any other concerns you have. My proposal is mainly to add a header flag to indicate the subimages, to make sure this functionality is behind a feature, but that the implementation within the feature is separate from the header flag (e.g., if the feature is present, but subimages is indicated in the header, reject the image entirely and fail to boot). |
I'm going to let CI run. If this passes that indicates we actually have a missing test coverage, as the case of a single image that is ramloaded should definitely be tested in the simulator, and this change, as it is, appears to break that. |
I don't think this should be added to RAM load at all, if you want sub-images, sure, made a system whereby it works for all modes, extending one mode without supporting the others is just the pinnacle of bad/lack of design |
And something else I would say is this can be done using multiple image slots already, so why we need a duplicate secluded feature for what is already (and has been for years) supported today? |
@nordicjm sorry i might missed it but not see it is supported yet. As an example it has been handled by TF-M with a custom solution, by adding a wrapper.py file and it is called with a Cmake command to concatenate images and execute one time signature check. Thanks for the feedback. |
TF-M does it's own things, I wouldn't trust that for a source of information on how to use MCUboot. Use this diff:
Then build |
I am not sure i get the trick in here, there are 4 slots and and you specify 2 of them are updatable, isn't it means at least there will be two times signature check? Additionally the proposal in this PR is just an extension to increase content of the project not changes existing MCUboot behaviour. By contributing this project we are trying to make it better without breaking/changing existing features. |
Yes? That is no different than if you are verifying mode data, the time taken is going to increase if you are hashing/verifying the signature of a larger data size
But as shown above this feature is 100% supported as-is today, the same way it is supported in other MCUboot modes, so what is improved by adding duplicate functionality that is going to have to be maintained by me, @de-nordic and @d3zd3z and is a starch difference for one specific mode of operation? |
Also note that your changes here completely break things like MCUmgr because you're hiding data of multiple images in one image, you no longer know what gets loaded where |
I doubt that will help / be possible. Hashing everything individually per partition or doing it all in one go will add up to take the same amount of time. It's the signature verification that is costly, so the more of them you do, the longer it'll take. |
ae6d9cc
to
91722bc
Compare
MCUBoot multi image support mode requires each image be procedded individually that requires multiple signature check during boot. If there be 4 image it will require 4 times signature validatation. This feature increase boot time and depend on the project long boot time may not be expected. To provide a solution for this case we propose to - Generate each image as regular - Concatanate them and re generate a main mcuboot image. ------------------------- | Header | ------------------------- | SubImage (optional) | | (Header+Data+Footer) | ------------------------- | SubImage (optional) | | (Header+Data+Footer) | ------------------------- | ..... | ------------------------- | Footer | ------------------------- During boot time if top level image be validated sub image can be just copied to the target location without to recheck them. To provide this feature there will be two commit 1- Provide a script that combine images 2- Update mcuboot source code to process subimages This commit is for mcu-tools#1, in this commit we are adding a script which called as combine_images.py. The usage of the script is: python combine_images.py --config combine_images.yaml --imgtool imgtool --output <outfolder> combine_images.yaml file is added as reference file it need to be updated as per of the project need. Signed-off-by: Sadik Ozer <[email protected]> Signed-off-by: Michael Eskowitz <[email protected]>
This is source code update to provide images in images feature support. As mentioned in prev commit: MCUBoot multi image support mode requires each image be procedded individually that requires multiple signature check during boot. If there be 4 image it will require 4 times signature validatation. This feature increase boot time and depend on the project long boot time may not be expected. In this commit the loader.c file update to search subimages and copy them in the related load_address. By this solution: - Image update will be handled as requlary - Signature check will be executed for combined image - Boot time will be decreased by eliminating multiple signature check This featue only support for (BOOT_IMAGE_NUMBER == 1 && MCUBOOT_RAM_LOAD) case. Signed-off-by: Sadik Ozer <[email protected]>
91722bc
to
cb63910
Compare
This PR introduce an proposal method that can be used to handle multi image in a single image pack to decrease number of signature check during bootup. By this change multiple images can be loaded/updated on device with one signature check.
Depend on the product this feature might requires to decrease boot up time.
This PR includes:
The feature only support for if (BOOT_IMAGE_NUMBER == 1) and MCUBOOT_RAM_LOAD defined case.
And assume on multicore heterogeneous system the host core which execute MCUBoot FW able to access sub core ITCM/DTCM by just memcpy operation.
The MCUBoot FW after copy them to the target address is going to execute first bootable image.
As an example 3 images can be concatenated as

Simply tested with Zephyr on MAX32690EVKIT

Performance test output for 3 images:

As summary this method clearly decrease boot time. Please see: