[RFC] Hooks to support choosing image to boot in runtime #2161

edersondisouza · 2024-12-24T01:17:55Z

[An alternative to https://github.com//pull/2044]

In order to allow an MCUBoot user to have more control on where to get image to boot - eventually bypassing all of MCUBoot compare and swap mechanism - but still preserving integrity (and other) checks provided by MCUBoot, this PR introduces a few hooks that an application can implement to achieve such control.

The main hooks introduced allow one to modify boot_go - the method that effectively selects an image to boot and some methods related to Flash Area ID/Device ID - so that applications are no constrained to statically defined flash partitions, but have some runtime leeway - enabling, for instance, images to decide which flash partition is going to be used based on GPIO state.

New configurations are used to enable these hooks, so they can be enabled independently from currently available hooks.

This PR also provides a sample of how those hooks could be used.

Finally, there are some patches exposing "RAM loading" functions so they can be used by an external module - while not strictly needed on this PR, they are used in the sample. If deemed better, I can extract those patches to another PR.

As always, comments and suggestions on how to better achieve this are highly appreciated =D

nordicjm

Some nits. in the sample with styling, other than that looks good

nordicjm · 2024-12-30T07:39:21Z

samples/runtime-source/zephyr/app/CMakeLists.txt

+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+project(non_flash_backend_app)
+
+if (NOT DEFINED FROM_WHO)


Suggested change

if (NOT DEFINED FROM_WHO)

if(NOT DEFINED FROM_WHO)

nordicjm · 2024-12-30T07:39:36Z

samples/runtime-source/zephyr/app/boards/frdm_k64f.overlay

+    chosen {
+        zephyr,code-partition = &slot1_partition;
+    };


tab indents for dts

nordicjm · 2024-12-30T07:39:52Z

samples/runtime-source/zephyr/app/prj.conf

+
+CONFIG_MCUBOOT_SIGNATURE_KEY_FILE="./bootloader/mcuboot/root-rsa-2048.pem"


this will be the default so can be dropped

Hmmm... got this when removed this line:

CMake Warning at /home/ederson/work/zephyr/zephyr-west/zephyr/cmake/mcuboot.cmake:28 (message): Neither CONFIG_MCUBOOT_GENERATE_UNSIGNED_IMAGE or CONFIG_MCUBOOT_SIGNATURE_KEY_FILE are set, the generated build will not be bootable by MCUboot unless it is signed manually/externally.

So I kept it.

nordicjm · 2024-12-30T07:40:14Z

samples/runtime-source/zephyr/app/src/main.c

+{
+	printk("Hello World from %s on %s, slot %s!\n",
+	       MCUBOOT_HELLO_WORLD_FROM, CONFIG_BOARD,
+           DT_PROP(DT_CHOSEN(zephyr_code_partition), label));


tab for main indent then spaces to align

nordicjm · 2024-12-30T07:40:24Z

samples/runtime-source/zephyr/app/src/main.c

@@ -0,0 +1,15 @@
+/*
+ * Copyright (c) 2017 Linaro, Ltd.


might want to update copyright

nordicjm · 2024-12-30T07:41:53Z

samples/runtime-source/zephyr/hooks/hooks.c

+
+fih_ret boot_go_hook(struct boot_rsp *rsp)
+{
+    int rc;


nordicjm · 2024-12-30T07:42:13Z

samples/runtime-source/zephyr/hooks/hooks.c

+}
+
+int flash_area_id_from_multi_image_slot_hook(int image_index, int slot,
+        int *area_id)


align to int image_index

nordicjm · 2024-12-30T07:42:29Z

samples/runtime-source/zephyr/sample.conf

@@ -0,0 +1,8 @@
+CONFIG_FLASH_RUNTIME_SOURCES=y
+CONFIG_SINGLE_APPLICATION_SLOT=y
+CONFIG_BOOT_SIGNATURE_TYPE_RSA=y


needs a sample.yml file so that twister will build this

would also need adding here https://github.com/mcu-tools/mcuboot/blob/main/.github/workflows/zephyr_build.yaml#L86

nordicjm · 2024-12-30T07:43:05Z

rebase will fix the CI error too

Add `boot_get_loader_state()` to allow one have a reference to current bootloader state. While this state is internal - and struct is opaque - it will be important to be able to "pass it around" when using hooks that will be introduced in future patches. While at it, make Zephyr single loader also expose it. Signed-off-by: Ederson de Souza <[email protected]>

RAM loading methods are currently private, which prevents external modules with hooks from using them. This patch makes them public. An interesting note is that a new method, `boot_load_image_from_flash_to_sram()`, is added so that code can actually set the image to be loaded - currently, that is inferred from current boot loader state, but external code can't directly access members of relevant `struct boot_loader_state`. Signed-off-by: Ederson de Souza <[email protected]>

This new hook allows external modules to override the choice of which image to boot - it can be used, for instance, to load an image based on some GPIO state. If the hook returns FIH_BOOT_HOOK_REGULAR, then normal platform boot_go() will be run to choose image to boot. This hook is shielded by a different configuration than other hooks - MCUBOOT_BOOT_GO_HOOKS, so that it can be enabled independently from other available hooks. Support for this new hook was added to Zephyr port. Signed-off-by: Ederson de Souza <[email protected]>

For users that customise (for instance, via boot_go() hook) from where images are being loaded from, having those hardcoded to `slot_partitionX` (when using Zephyr) can be problematic. New hooks, `flash_area_id_from_multi_image_slot_hook` and `flash_area_get_device_id_hook` allow some customisation around these processes. Support for these hooks is shielded by a different configuration than other hooks - MCUBOOT_FLASH_AREA_HOOKS, so that it can be enabled independently from other available hooks. Support for this new hook was added to the Zephyr port. Signed-off-by: Ederson de Souza <[email protected]>

edersondisouza · 2025-01-07T00:37:28Z

v2:

Fixed style issues;
Made sample build-able by twister.

de-nordic

Looks ok, some minor problem.
Just one note though: I am working on removing usage of flash area and flash area id within MCUboot, so in the long term the hooks will be affected, although there will be compatibility mote. Just letting you know.

The replacement will actually require systems to provide own implementation of open function that will have to map from MCUboot ids to system specific, which means that proposed here hook flash_area_id_from_multi_image_slot_hook will loose its importance, but you will be able to write your open function to override ids however you want.

This will take a moment to get completed so I think that for now we are good with the flash_area_id_from_multi_image_slot_hook,

de-nordic · 2025-01-12T15:55:48Z

samples/runtime-source/zephyr/hooks/hooks.c

+
+int flash_area_get_device_id_hook(const struct flash_area *fa, uint8_t *dev_id)
+{
+	*dev_id = fa->fa_id;


Avoid direct access to object fields, use getter for that, and I do not think this is actually correct either. The fa_id is Flash Area ID (or fixed partition id), the device id is supposed to identify flash device, which would be either internal soc flash or external, for example SPI, flash.

Hmmm... the value returned by flash_area_get_device_id ends up being used (at least for Zephyr) as parameter for flash_area_open or flash_device_base. For the later, yes, it is the flash device. However, for the former, it ends up being the fixed partition id - kinda circular, but IIUC, that's how it goes.

Indeed, this is the same pattern found at https://github.com/mcu-tools/mcuboot/blob/main/boot/zephyr/flash_map_extended.c#L145 for ARM. In this sample, one could even ask if there's a point for the flash_area_get_device_id_hook, as it's for ARM so it would just get fa_id anyway, but I'm also looking at other platforms (that may or may not need more patches in the future).

I need to check that, but as far as I can tell the fa_id is used with flash_area_open but the flash_area_get_device_id is not. The getter for fa_id should be flash_area_get_id. The function is here provided because Zephyr does not have one, does not have dev id l.
There might be a bug here as I you can actually see in the code, that does the ifdef on arm, that the id for non-arm devices would be hardcoded to single value; additionally Zephyr devices are identified by flash API by device pointer, and the id is only for the partition.

The current implementation of flash_area_get_device_id has inconsistent behavior here between architectures: why would ARM have these ids but others not? That is not even described in api.

Give me a few moments to take another look at that.

Hi @de-nordic! Any news here?

OK, sorry for lagging. We have a bug. Here is only function, Zephyr specific implementation, that takes device id:

mcuboot/boot/zephyr/flash_map_extended.c

Lines 43 to 52 in 0674798

int flash_device_base(uint8_t fd_id, uintptr_t *ret)

{

if (fd_id != FLASH_DEVICE_ID) {

BOOT_LOG_ERR("invalid flash ID %d; expected %d",

fd_id, FLASH_DEVICE_ID);

return -EINVAL;

}

*ret = FLASH_DEVICE_BASE;

return 0;

}

It is supposed to get device base == where flash address space starts.
As you can see it is hardcoded to be able to process only internal flash (at this point).
The device id is not present in Zephyr variant of flash_area structure, but is provided by cypress, embed and otherr flash area backends provided as part of support for these systems.
If you look at any of these you will notice, that this is set to a device id not flash area id:

mcuboot/boot/cypress/cy_flash_pal/cy_flash_map.c

Lines 76 to 82 in 0674798

static struct flash_area bootloader =

{

.fa_id = FLASH_AREA_BOOTLOADER,

.fa_device_id = FLASH_DEVICE_INTERNAL_FLASH,

.fa_off = CY_BOOTLOADER_START_ADDRESS,

.fa_size = CY_BOOT_BOOTLOADER_SIZE

};

This can not be flash area device flash_device_base would not work properly for Zephyr. Frankly the only usage, I can see, is in do_boot that is for RISC V and therefore the CONFIG_ARM ifdef in the flash_area_get_device_id does not break anything.

de-nordic · 2025-01-12T16:09:26Z

samples/runtime-source/zephyr/hooks/hooks.c

+
+#define BOOT_TMPBUF_SZ 256
+
+static const struct flash_area *_fa_p;


why is this not a function local variable?

A sample for runtime chose image on FRDM K64F. It provides implementation for boot_go and flash area id related hooks, showing how an external module implementing hooks can influence which images end up being booted on the platform. 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]>

This sample has two parts: hooks for an mcuboot instance and a target application. For the first, a new entry at boot/zephyr/sample.yaml is added as well as a corresponding change at boot/zephyr/CMakeLists.txt to include the hook as a module. For the second, added a sample.yaml as well as invoke it from the github workflow. Signed-off-by: Ederson de Souza <[email protected]>

edersondisouza · 2025-01-14T06:51:58Z

v3:

Make some variables local;
Use getter to access object fields.

nordicjm · 2025-01-15T13:54:34Z

boot/bootutil/src/loader.c

+struct boot_loader_state *boot_get_loader_state(void)
+{
+    return &boot_data;
+}


actually after having a talk with David about other issues I don't think this will work, @d3zd3z

Hmm... why? My goal here is to have some state that can be prepared and passed to boot_load_image_to_sram without needing to expose struct boot_loader_state itself. I went with getting a static one instead of creating a new one to avoid allocation. Note that the one that matters to me is the boot_get_loader_state at boot/zephyr/single_loader.c - which is there just to be shared - instead of the one here at bootutil, which I only added for completeness.

edersondisouza requested review from nordicjm, de-nordic and davidvincze as code owners December 24, 2024 01:17

edersondisouza mentioned this pull request Dec 24, 2024

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

Open

de-nordic added area: core Affects core functionality area: zephyr Affects the Zephyr port labels Dec 25, 2024

nordicjm approved these changes Dec 30, 2024

View reviewed changes

nordicjm requested a review from d3zd3z December 30, 2024 07:43

edersondisouza added 4 commits January 6, 2025 10:03

edersondisouza force-pushed the bootgo-hook branch from a6a911c to 15c0273 Compare January 7, 2025 00:35

edersondisouza requested a review from nordicjm January 7, 2025 00:38

de-nordic reviewed Jan 12, 2025

View reviewed changes

edersondisouza added 2 commits January 13, 2025 15:00

edersondisouza force-pushed the bootgo-hook branch from 15c0273 to 78d3509 Compare January 14, 2025 06:49

edersondisouza requested a review from de-nordic January 14, 2025 06:52

nordicjm reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Hooks to support choosing image to boot in runtime #2161

[RFC] Hooks to support choosing image to boot in runtime #2161

edersondisouza commented Dec 24, 2024

nordicjm left a comment

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

edersondisouza Jan 7, 2025

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

nordicjm Dec 30, 2024

nordicjm commented Dec 30, 2024

edersondisouza commented Jan 7, 2025

de-nordic left a comment

de-nordic Jan 12, 2025

edersondisouza Jan 13, 2025

de-nordic Jan 14, 2025

edersondisouza Jan 22, 2025

de-nordic Jan 23, 2025

de-nordic Jan 12, 2025

edersondisouza commented Jan 14, 2025

nordicjm Jan 15, 2025

edersondisouza Jan 22, 2025


		CONFIG_MCUBOOT_SIGNATURE_KEY_FILE="./bootloader/mcuboot/root-rsa-2048.pem"

	int flash_device_base(uint8_t fd_id, uintptr_t *ret)
	{
	if (fd_id != FLASH_DEVICE_ID) {
	BOOT_LOG_ERR("invalid flash ID %d; expected %d",
	fd_id, FLASH_DEVICE_ID);
	return -EINVAL;
	}
	*ret = FLASH_DEVICE_BASE;
	return 0;
	}

	static struct flash_area bootloader =
	{
	.fa_id = FLASH_AREA_BOOTLOADER,
	.fa_device_id = FLASH_DEVICE_INTERNAL_FLASH,
	.fa_off = CY_BOOTLOADER_START_ADDRESS,
	.fa_size = CY_BOOT_BOOTLOADER_SIZE
	};


		#define BOOT_TMPBUF_SZ 256

		static const struct flash_area *_fa_p;

[RFC] Hooks to support choosing image to boot in runtime #2161

Are you sure you want to change the base?

[RFC] Hooks to support choosing image to boot in runtime #2161

Conversation

edersondisouza commented Dec 24, 2024

nordicjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordicjm commented Dec 30, 2024

edersondisouza commented Jan 7, 2025

de-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edersondisouza commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment