sysflash: Correct ALL_AVAILABLE_SLOTS when MCUBOOT is not image 2

[ball-hayden]

Consider an nRF5340 application with the following sysbuild config:

SB_CONFIG_BOOTLOADER_MCUBOOT=y
SB_CONFIG_BOOT_SIGNATURE_TYPE_ECDSA_P256=y

SB_CONFIG_PM_EXTERNAL_FLASH_MCUBOOT_SECONDARY=y

SB_CONFIG_SECURE_BOOT_APPCORE=y
SB_CONFIG_SECURE_BOOT_NETCORE=y

SB_CONFIG_NETCORE_APP_UPDATE=y
SB_CONFIG_MCUBOOT_NRF53_MULTI_IMAGE_UPDATE=y
SB_CONFIG_MCUBOOT_MODE_OVERWRITE_ONLY=y

In this case, we have a total of 3 updatable images (app, net, mcuboot).

MCUBOOT_IMAGE_NUMBER = 2 (i.e. CONFIG_UPDATEABLE_IMAGE_NUMBER = 2), as the app secondary partition is reused for MCUBoot secondary.

CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER = 2 as the net core image has index 1.

In this case, we should fall through to the definition of ALL_AVAILABLE_SLOTS that caters for 2 updatable images.

[de-nordic]

Commit message missing [nrf noup] tagging.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[nordicjm]

not sure what this is trying to fix but it does not look valid, there are 2 images which the application sees, that is application and network core, for MCUboot itself it has it's own update slot, and that is already handled by this code in the same file:

#if CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER != -1
#ifdef CONFIG_NCS_IS_VARIANT_IMAGE
#define MCUBOOT_S0_S1_SLOTS PM_S0_ID, PM_MCUBOOT_SECONDARY_ID,
#else
#define MCUBOOT_S0_S1_SLOTS PM_S1_ID, PM_MCUBOOT_SECONDARY_ID,
#endif
#else
#define MCUBOOT_S0_S1_SLOTS
#endif

...

static inline uint32_t __flash_area_ids_for_slot(int img, int slot)
{
    static const int all_slots[] = {
        ALL_AVAILABLE_SLOTS
        MCUBOOT_S0_S1_SLOTS
    };
    return all_slots[img * 2 + slot];
};

therefore the change here does not make sense

[ball-hayden]

Thank you @nordicjm.

Please see https://devzone.nordicsemi.com/f/nordic-q-a/113100/nrf5340-mcuboot-issue-with-multi-image-update-when-sb_config_secure_boot_appcore-is-enabled/537541 for reproduction steps for the issue this is trying to solve.

Without the change, ALL_AVAILABLE_SLOTS ends up being just the app core image slots and does not include the net core image slots.

This is because the conditions on line 34 hold (MCUBOOT_IMAGE_NUMBER == 2 && CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER != -1), so we end up ignoring FLASH_AREA_IMAGE_1_SLOTS.

[nordicjm]

In this case, we have a total of 3 updatable images (app, net, mcuboot).

MCUBOOT_IMAGE_NUMBER = 2 (i.e. CONFIG_UPDATEABLE_IMAGE_NUMBER = 2), as the app secondary partition is reused for MCUBoot secondary.

I cannot replicate this, taking zephyr/samples/subsys/mgmt/mcumgr/smp_svr and configuring for nrf5340dk/nrf5340/cpuapp with -DSB_CONFIG_NETCORE_EMPTY=y -DSB_CONFIG_NETCORE_APP_UPDATE=y -DSB_CONFIG_SECURE_BOOT_APPCORE=y -DSB_CONFIG_SECURE_BOOT_NETCORE=y -DSB_CONFIG_MCUBOOT_NRF53_MULTI_IMAGE_UPDATE=y yields SB_CONFIG_MCUBOOT_UPDATEABLE_IMAGES=2 and SB_CONFIG_MCUBOOT_ADDITIONAL_UPDATEABLE_IMAGES=1 which means MCUboot gets CONFIG_UPDATEABLE_IMAGE_NUMBER=3, CONFIG_MCUBOOT_APPLICATION_IMAGE_NUMBER=0, CONFIG_MCUBOOT_NETWORK_CORE_IMAGE_NUMBER=1 and CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER=2 which means in the .h file it would follow this:

...
#if CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER != -1
#ifdef CONFIG_NCS_IS_VARIANT_IMAGE
#else
#define MCUBOOT_S0_S1_SLOTS PM_S1_ID, PM_MCUBOOT_SECONDARY_ID,
...
#elif (MCUBOOT_IMAGE_NUMBER == 2) || (MCUBOOT_IMAGE_NUMBER == 3 && CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER != -1)
#define ALL_AVAILABLE_SLOTS FLASH_AREA_IMAGE_0_SLOTS \
                            FLASH_AREA_IMAGE_1_SLOTS  
...
static inline uint32_t __flash_area_ids_for_slot(int img, int slot)
{
    static const int all_slots[] = {
        ALL_AVAILABLE_SLOTS
        MCUBOOT_S0_S1_SLOTS
    };
    return all_slots[img * 2 + slot];
};

so as can be seen, all 3 images are present and set

[nordicjm]

Change does not make sense

[ball-hayden]

Let me expand some macros to demonstrate.

Our desired end result is the following, as we have 2 images to update with MCUboot - the app image (image number 0) and the net image (image number 1) - and the MCUboot image itself (image number 2):

static inline uint32_t __flash_area_ids_for_slot(int img, int slot)
{
    static const int all_slots[] = {
	PM_MCUBOOT_PRIMARY_ID, PM_MCUBOOT_SECONDARY_ID,
        PM_MCUBOOT_PRIMARY_1_ID, PM_MCUBOOT_SECONDARY_1_ID,
	PM_S0_ID, PM_MCUBOOT_SECONDARY_ID,
    };
    return all_slots[img * 2 + slot];
};

From

sdk-mcuboot/boot/zephyr/include/mcuboot_config/mcuboot_config.h

Line 116 in 1916314

#define MCUBOOT_IMAGE_NUMBER CONFIG_UPDATEABLE_IMAGE_NUMBER

In the application image, MCUBOOT_IMAGE_NUMBER expands to CONFIG_UPDATEABLE_IMAGE_NUMBER, which expands to 2.

Separately, CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER also expands to 2.

If we expand

sdk-mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h

Line 34 in 1916314

#if (MCUBOOT_IMAGE_NUMBER == 1) || (MCUBOOT_IMAGE_NUMBER == 2 && CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER != -1)

We get:

#if (2 == 1) || (2 == 2 && 2 != -1)

This falls through to

sdk-mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h

Line 35 in 1916314

#define ALL_AVAILABLE_SLOTS FLASH_AREA_IMAGE_0_SLOTS

FLASH_AREA_IMAGE_0_SLOTS expands to PM_MCUBOOT_PRIMARY_ID, PM_MCUBOOT_SECONDARY_ID.

This leaves us with __flash_area_ids_for_slot expanding to:

static inline uint32_t __flash_area_ids_for_slot(int img, int slot)
{
    static const int all_slots[] = {
	PM_MCUBOOT_PRIMARY_ID, PM_MCUBOOT_SECONDARY_ID,
	PM_S0_ID, PM_MCUBOOT_SECONDARY_ID,
    };
    return all_slots[img * 2 + slot];
};

This results in MBUboot looking at the MCUBoot secondary slot while deciding whether it should update the net core, rather than looking at PM_MCUBOOT_SECONDARY_1_ID.

[ball-hayden]

I cannot replicate this, taking zephyr/samples/subsys/mgmt/mcumgr/smp_svr and configuring for nrf5340dk/nrf5340/cpuapp with -DSB_CONFIG_NETCORE_EMPTY=y -DSB_CONFIG_NETCORE_APP_UPDATE=y -DSB_CONFIG_SECURE_BOOT_APPCORE=y -DSB_CONFIG_SECURE_BOOT_NETCORE=y -DSB_CONFIG_MCUBOOT_NRF53_MULTI_IMAGE_UPDATE=y

Using NETCORE_EMPTY isn't really a fair test - the failure case is that the net core image doesn't update, so you wouldn't see this with an empty netcore image.

Please try with ipc_radio as the net core image, adding a VERSION file so you can see the version number change in the logs.

You should see that the process looks like it succeeds, but if you observe the MCUBoot logs you will see that image 1 (the net core image) did not swap when it should have, and the version printed by the net core will not have changed.

[ball-hayden]

It looks like our main disagreement is over the value of CONFIG_UPDATEABLE_IMAGE_NUMBER.

https://github.com/nrfconnect/sdk-nrf/blob/8854d47ddf22b08e9a0b564fa56f6ffb1579bbf0/sysbuild/CMakeLists.txt#L172-L174

forces this to SB_CONFIG_MCUBOOT_UPDATEABLE_IMAGES.

SB_CONFIG_MCUBOOT_UPDATEABLE_IMAGES defaults to 2 (https://github.com/nrfconnect/sdk-nrf/blob/8854d47ddf22b08e9a0b564fa56f6ffb1579bbf0/sysbuild/Kconfig.mcuboot#L98)

If I set this to 3, as you suggest, I get the following compilation error:

Compiler Error

In file included from /Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/sysflash.h:10,
                 from /Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/bootutil/src/bootutil_priv.h:33,
                 from /Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/bootutil/src/loader.c:42:
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h: In function '__flash_area_ids_for_slot':
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:21:37: error: 'PM_MCUBOOT_PRIMARY_2_ID' undeclared (first use in this function); did you mean 'PM_MCUBOOT_PRIMARY_1_ID'?
   21 | #define FLASH_AREA_IMAGE_2_SLOTS    PM_MCUBOOT_PRIMARY_2_ID, PM_MCUBOOT_SECONDARY_2_ID,
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:46:29: note: in expansion of macro 'FLASH_AREA_IMAGE_2_SLOTS'
   46 |                             FLASH_AREA_IMAGE_2_SLOTS \
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:55:9: note: in expansion of macro 'ALL_AVAILABLE_SLOTS'
   55 |         ALL_AVAILABLE_SLOTS
      |         ^~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:21:37: note: each undeclared identifier is reported only once for each function it appears in
   21 | #define FLASH_AREA_IMAGE_2_SLOTS    PM_MCUBOOT_PRIMARY_2_ID, PM_MCUBOOT_SECONDARY_2_ID,
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:46:29: note: in expansion of macro 'FLASH_AREA_IMAGE_2_SLOTS'
   46 |                             FLASH_AREA_IMAGE_2_SLOTS \
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:55:9: note: in expansion of macro 'ALL_AVAILABLE_SLOTS'
   55 |         ALL_AVAILABLE_SLOTS
      |         ^~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:21:62: error: 'PM_MCUBOOT_SECONDARY_2_ID' undeclared (first use in this function); did you mean 'PM_MCUBOOT_SECONDARY_1_ID'?
   21 | #define FLASH_AREA_IMAGE_2_SLOTS    PM_MCUBOOT_PRIMARY_2_ID, PM_MCUBOOT_SECONDARY_2_ID,
      |                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:46:29: note: in expansion of macro 'FLASH_AREA_IMAGE_2_SLOTS'
   46 |                             FLASH_AREA_IMAGE_2_SLOTS \
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:55:9: note: in expansion of macro 'ALL_AVAILABLE_SLOTS'
   55 |         ALL_AVAILABLE_SLOTS
      |         ^~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:22:37: error: 'PM_MCUBOOT_PRIMARY_3_ID' undeclared (first use in this function); did you mean 'PM_MCUBOOT_PRIMARY_1_ID'?
   22 | #define FLASH_AREA_IMAGE_3_SLOTS    PM_MCUBOOT_PRIMARY_3_ID, PM_MCUBOOT_SECONDARY_3_ID,
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:47:29: note: in expansion of macro 'FLASH_AREA_IMAGE_3_SLOTS'
   47 |                             FLASH_AREA_IMAGE_3_SLOTS
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:55:9: note: in expansion of macro 'ALL_AVAILABLE_SLOTS'
   55 |         ALL_AVAILABLE_SLOTS
      |         ^~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:22:62: error: 'PM_MCUBOOT_SECONDARY_3_ID' undeclared (first use in this function); did you mean 'PM_MCUBOOT_SECONDARY_1_ID'?
   22 | #define FLASH_AREA_IMAGE_3_SLOTS    PM_MCUBOOT_PRIMARY_3_ID, PM_MCUBOOT_SECONDARY_3_ID,
      |                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:47:29: note: in expansion of macro 'FLASH_AREA_IMAGE_3_SLOTS'
   47 |                             FLASH_AREA_IMAGE_3_SLOTS
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/hayden/development/PlayerData/live_gateway/bootloader/mcuboot/boot/zephyr/include/sysflash/pm_sysflash.h:55:9: note: in expansion of macro 'ALL_AVAILABLE_SLOTS'
   55 |         ALL_AVAILABLE_SLOTS
      |         ^~~~~~~~~~~~~~~~~~~

This makes sense, as there are no flash slots for image 2 - image 2 is MCUboot, so should be sharing the other partitions, as you've said earlier.

It's worth calling out that CONFIG_UPDATEABLE_IMAGE_NUMBER is different between the app image and mcuboot.

This means some of the functions from bootutil_public.h behave differently to how we might expect.

[ball-hayden]

Change does not make sense

Yes. The whole thing does not make sense. I was very confused. I'm still very confused. The indirection is massive, and it took me several days to come to the proposed solution.

[nvlsianpu]

@ball-hayden this looks like an issue on DFU application side - as reverse of staging image for update made the difference.
The activation of each image (i do believe this is image test or image confirm command) set dedicated flag(s) in image trailer (mcuboot's image control data) in FLASH. These fields are read by MCUboot just after the reboot. Looks like somewhy these fields values are different when image activation order is changed (which is a bug).

[ball-hayden]

I'm glad you've been able to see the issue @nvlsianpu.

I still feel like from the application image the images are being incorrectly reported, but I'm happy to accept suggestions for different approaches here.

[ball-hayden]

For context, I've used both the nRF Connect app (on iOS and Android) and https://pypi.org/project/smpclient/ while debugging this.

The higher-level MCUMgr Image Group definitely thinks it has set both image 0 and 1 to the test slot, but when MCUBoot is reading the image trailers it finds that image 0 is set to test, but image 1 is not.

[nvlsianpu]

@ball-hayden Issue is somewhere in libraries used by the application. I confirmed this with Nordic verification team. Known workaround is sequence of image activation you already discovered.

[nvlsianpu]

We will take action for fixing the bug.

[ball-hayden]

@nvlsianpu do you have an update to share on fixing the bug please?

[ball-hayden]

@rlubos I think the diff has got confused here - was that intentional?