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

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

Conversation

edersondisouza
Copy link

@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
Author

v2:

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

@edersondisouza
Copy link
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
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
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
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
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 <ederson.desouza@intel.com>
@nordicjm
Copy link
Collaborator

@teburd Needs a signed off line from you

Following the split of RAM code, these definitions will help use it with
single slot applications.

Signed-off-by: Ederson de Souza <ederson.desouza@intel.com>
Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
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 <ederson.desouza@intel.com>
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 <ederson.desouza@intel.com>
@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

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