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

Make RAM loading available for single loaders #2062

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

edersondisouza
Copy link
Contributor

@edersondisouza edersondisouza commented Sep 12, 2024

Code for loading the image to RAM is currently under bootutil/loader.c. However, single loaders (used when one sets MCUBOOT_SINGLE_APPLICATION_SLOT) cannot use this file - a single loader in fact re-implements some of the methods of bootutil/loader.c.
This PR splits the bits of code related to RAM loading to another file inside bootutil (ram_load.c) and make them available on bootutil_priv.h, so that single loaders can simply reuse this code.

Note: these are basically the first four patches from #2044 - it seems that the multiple source bits of that PR are a bit controversial, so this PR focus on the RAM loading availability, which is hopefully less controversial.

Comment on lines 264 to 268
config BOOT_NO_UPGRADE
bool "No upgrade"
help
No upgrade is performed - usually, this option will be used in
conjunction with BOOT_RAM_LOAD.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what you're doing here but no, this Kconfig is not needed and put the ram load Kconfig back where it was

@nordicjm
Copy link
Collaborator

Code for loading the image to RAM is currently under bootutil/loader.c. However, single loaders (used when one sets MCUBOOT_SINGLE_APPLICATION_SLOT) cannot use this file - a single loader in fact re-implements some of the methods of bootutil/loader.c.

Because single application slot has nothing to do with RAM load mode, they are two completely different modes, RAM load is it's own dual-slot system, single application image is a single flash image. If you want to add a single RAM slot mode then that's something that can be added as a distinct mode, not as modifying single application slot

@edersondisouza
Copy link
Contributor Author

v2:

  • Have a separate Kconfig for RAM loading on single application slot

@edersondisouza
Copy link
Contributor Author

Code for loading the image to RAM is currently under bootutil/loader.c. However, single loaders (used when one sets MCUBOOT_SINGLE_APPLICATION_SLOT) cannot use this file - a single loader in fact re-implements some of the methods of bootutil/loader.c.

Because single application slot has nothing to do with RAM load mode, they are two completely different modes, RAM load is it's own dual-slot system, single application image is a single flash image. If you want to add a single RAM slot mode then that's something that can be added as a distinct mode, not as modifying single application slot

Ok - using a separate Kconfig instead of reusing the choice under !SINGLE_APPLICATION_SLOT.

Comment on lines 310 to 320
if SINGLE_APPLICATION_SLOT

config SINGLE_APPLICATION_SLOT_RAM_LOAD
bool "RAM load for single application slot"
help
If y, the image is loaded to RAM and executed from there. For this reason,
the image has to be linked to be executed from RAM. The address that the
image is copied to is specified using the load-addr argument to the
imgtool.py script which writes it to the image header.

endif # SINGLE_APPLICATION_SLOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be a distinct mode, not one that uses SINGLE_APPLICATION_SLOT, that Kconfig is as I said in the previous review for single flash application only, there needs to be a new Kconfig, one that does not use an existing Kconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - moved the Kconfig outside if SINGLE_APPLICATION_SLOT, so one doesn't need something like:

CONFIG_SINGLE_APPLICATION_SLOT=y
CONFIG_SINGLE_APPLICATION_SLOT_RAM_LOAD=y

Only CONFIG_SINGLE_APPLICATION_SLOT_RAM_LOAD=y is needed. Implementation-wise though, it will select SINGLE_APPLICATION_SLOT as the code path is basically the same.

@edersondisouza
Copy link
Contributor Author

v3:

  • CONFIG_SINGLE_APPLICATION_SLOT_RAM_LOAD is not behind CONFIG_SINGLE_APPLICATION_SLOT.

@@ -320,6 +307,28 @@ config BOOT_SWAP_SAVE_ENCTLV

endif # !SINGLE_APPLICATION_SLOT

config SINGLE_APPLICATION_SLOT_RAM_LOAD
bool "RAM load for single application slot"
select SINGLE_APPLICATION_SLOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
select SINGLE_APPLICATION_SLOT

This needs to be a distinct mode, it can use the same .c file and check the ifdefs but this Kconfig needs to be its own Kconfig and not do anything with other MCUboot mode Kconfigs

@@ -108,6 +108,12 @@

#endif /* CONFIG_SINGLE_APPLICATION_SLOT */

#ifdef CONFIG_SINGLE_APPLICATION_SLOT_RAM_LOAD
#define MCUBOOT_RAM_LOAD 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here, add a new define for this mode and add it to the base .c files that needs it to enable the required functions

@edersondisouza
Copy link
Contributor Author

v4:

  • CONFIG_SINGLE_APPLICATION_SLOT_RAM_LOAD doesn't select CONFIG_SINGLE_APPLICATION_SLOT anymore. This means that there a bit more widespread changes, though.

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Minor nit., changes are OK

boot/bootutil/src/boot_record.c Outdated Show resolved Hide resolved
@nordicjm
Copy link
Collaborator

@teburd Needs a signed off line from you

@teburd
Copy link

teburd commented Sep 24, 2024

@teburd Needs a signed off line from you

Done

@nordicjm
Copy link
Collaborator

@teburd CI still failing on sign off

Copy link
Collaborator

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

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

CI issues need to be fixed

@teburd
Copy link

teburd commented Oct 1, 2024

@nordicjm @nvlsianpu all commits with signed-off now (as the rebase I guess has changed the committer, which makes sense), CI passing, please re-review thanks!

@de-nordic de-nordic requested a review from nvlsianpu October 1, 2024 13:39
@teburd
Copy link

teburd commented Oct 15, 2024

@nvlsianpu any updates? Seems I'll need a rebase now but would like feedback that this is good to go

@teburd teburd force-pushed the ram-load branch 2 times, most recently from 36f710e to 025235f Compare October 23, 2024 22:34
RAM loading code is currently under bootutil/loader.c, and it's not
accessible for different loaders, such as the single loaders. Future
patches will make use of the RAM loading code outside the
bootutil/loader.c context, and this patch prepares for that by making it
standalone on boot/bootutil/src/ram_load.c

Signed-off-by: Ederson de Souza <[email protected]>
Signed-off-by: Tom Burdick <[email protected]>
Following the split of RAM code, these definitions will help use it with
single slot applications.

Signed-off-by: Ederson de Souza <[email protected]>
Signed-off-by: Tom Burdick <[email protected]>
This option basically enables MCUBOOT_RAM_LOAD in a single
slot configuration, meaning the image on slot0 will be loaded into RAM.

Signed-off-by: Ederson de Souza <[email protected]>
Signed-off-by: Tom Burdick <[email protected]>
Now that's possible to load image to RAM on single loaders, add support
on Zephyr port for that.

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

teburd commented Oct 23, 2024

Latest rebase should fix the conflict that was introduced, the boot_enc_decrypt rename cause the rebase to show a conflict and that was fixed.

@teburd
Copy link

teburd commented Oct 24, 2024

CI issues need to be fixed

They’ve been fixed again, please do review again when you can

@nvlsianpu nvlsianpu merged commit 4893193 into mcu-tools:main Oct 25, 2024
58 checks passed
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.

4 participants