sysflash: Correct ALL_AVAILABLE_SLOTS when MCUBOOT is not image 2 #444
Conversation
de-nordic
left a comment
de-nordic
left a comment
There was a problem hiding this comment.
Commit message missing [nrf noup] tagging.
ball-hayden
force-pushed
the
pm-sysflash-netcore
branch
from
dbf1826 to
5458fb4
Compare
|
nordicjm
left a comment
nordicjm
left a comment
There was a problem hiding this comment.
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
|
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, This is because the conditions on line 34 hold ( |
I cannot replicate this, taking so as can be seen, all 3 images are present and set |
nordicjm
left a comment
nordicjm
left a comment
There was a problem hiding this comment.
Change does not make sense
|
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 In the application image, Separately, If we expand We get: #if (2 == 1) || (2 == 2 && 2 != -1)
This falls through to
This leaves us with 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 |
Using Please try with 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. |
|
It looks like our main disagreement is over the value of forces this to
If I set this to 3, as you suggest, I get the following compilation error: Compiler ErrorThis 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 This means some of the functions from |
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. |
|
@ball-hayden this looks like an issue on DFU application side - as reverse of staging image for update made the difference. |
|
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. |
|
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. |
|
@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. |
|
We will take action for fixing the bug. |
…nto one place Make enc_key_public.h single point of definitions for key sizes, TLV indexes and so on. Upstream PR #: 2327 Signed-off-by: Dominik Ermel <[email protected]> (cherry picked from commit 2d93958)
Use bootutil_macros.h instead. Upstream PR #: 2327 Signed-off-by: Dominik Ermel <[email protected]> (cherry picked from commit c1bb3a3)
…tions Cleanup. Upstream PR #: 2327 Signed-off-by: Dominik Ermel <[email protected]> (cherry picked from commit f4a5081)
Incorrect range check fix. Upstream PR #: 2337 Signed-off-by: Dominik Ermel <[email protected]> (cherry picked from commit fa17bc9)
This fixes issues when trying to compress images with no header padding requested. Upstream PR #: 2334 Signed-off-by: Michal Kozikowski <[email protected]> (cherry picked from commit 9e0bebc)
The hfwinfo returns bitmask, not single values. Upstream PR #: 2342 Signed-off-by: Tomasz Chyrowicz <[email protected]> (cherry picked from commit 18e3bc8)
FLASH_DEVICE_ID was incorrectly set to spi related flash id instead of SoC related. Upstream PR #: 2414 Signed-off-by: Michal Kozikowski <[email protected]>
Modified code to correctly generate the TLV for AES256 Signed-off-by: Artur Hadasz <[email protected]> (cherry picked from commit a5c48f3)
This commit adds the parts in the tooling allowing AES256 to work with MCUBoot in zephyr. Currently only in combination PSA + ED25519 Signed-off-by: Artur Hadasz <[email protected]> (cherry picked from commit 268968f)
nrf-squash! [nrf noup] zephyr: Clean up non-secure RAM if enabled This leads to stack corruption. Signed-off-by: Mateusz Michalek <[email protected]>
nrf-squash! [nrf noup] boot: zephyr: Add bm firmware loader code Delays checking IO button state by 5us after pull-up has been applied to allow time for it to be applied Signed-off-by: Jamie McCrae <[email protected]>
This reverts commit 8d31ad7. Signed-off-by: Jamie McCrae <[email protected]>
This reverts commit 50e1caa. Signed-off-by: Jamie McCrae <[email protected]>
Adds a boot banner which shows as MCUboot Signed-off-by: Jamie McCrae <[email protected]> (cherry picked from commit 4b3d6ab)
Allows GPIO entrance mode when bare metal is used, this is needed because the zephyr GPIO drivers are not used, therefore the Kconfig will not be enabled Signed-off-by: Jamie McCrae <[email protected]>
Disables read write and execute on mcuboots NVM at the end of execution. Signed-off-by: Mateusz Michalek <[email protected]>
adding DK default configuration and fixing PDK configuration. Signed-off-by: Mateusz Michalek <[email protected]>
Add a bootloader hook to alter the logic of the active slot selection in Direct XIP modes. Signed-off-by: Tomasz Chyrowicz <[email protected]> (cherry picked from commit 7c4ec9a)
Add a Kconfig option to enable a bootloader hook to alter the logic of the active slot selection in Direct XIP modes. Signed-off-by: Tomasz Chyrowicz <[email protected]> (cherry picked from commit d5f84b4)
Add a capability inside the Zephyr bootloader to handle memory-based bootloader requests to: - Boot recovery firmware - Boot firmware loader - Confirm an image - Set the slot preference Ref: NCSDK-34429 Signed-off-by: Tomasz Chyrowicz <[email protected]>
memset was given incorrectly pointer size, instead of object size. Signed-off-by: Dominik Ermel <[email protected]> (cherry picked from commit aa22913)
|
@rlubos not sure what's gone on with this - I'll push this back tomorrow to how I think it should be unless you tell me otherwise. |
This reverts commit 54f2129. nrf-squash! [nrf noup] zephyr: add 'minimal' configuration files The minimal configuration is now deprecated: - it is not minimal configuration anymore - built image cannot fit into flash memory - smp_svr_mini_boot sample is a new reference for minimal config Signed-off-by: Adam Szczygieł <[email protected]>
|
@ball-hayden There was a periodic history cleanup in |
ball-hayden
force-pushed
the
pm-sysflash-netcore
branch
from
5458fb4 to
4e48cbd
Compare
|
none Note: This comment is automatically posted and updated by the Contribs GitHub Action. |
…image 2 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.
ball-hayden
force-pushed
the
pm-sysflash-netcore
branch
from
1ee4f0f to
a7f9f77
Compare
|
|
@nvlsianpu do you have an update to share on fixing the bug please? I'm still proposing this as a fix, given there doesn't appear to be any other movement. |
|
@nvlsianpu still after a fix for this, please, if this isn't the correct way forward. |
|
Hello, Is there is another solution being proposed by Nordic besides the one proposed in this PR?
Depending on what application you are using, it might be referencing the manifest.json if you are confirming after an upload when setting flags rather than reading the actual image data from the application. For my use case, and where I noticed this issue, I have a custom C++ app that interfaces with the SMP CBOR stream directly over BLE and can confirm when executing a mcumgr image read command, the image data (including slot info) expected for net core returned is actually for app core. Therefore I cannot use the slot info returned by image read to then set flags on the network core image because it would write to the app image secondary. |
|
I'm afraid Nordic don't seem to be interested @anthony289478 - they maintain that the current behaviour is correct, despite our independent reports of incorrect behaviour and proposed fixes. @nordicjm please can we have a proper fix if neither of our proposed solutions are valid? |
30 participants
Consider an nRF5340 application with the following sysbuild config:
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 = 2as the net core image has index 1.In this case, we should fall through to the definition of
ALL_AVAILABLE_SLOTSthat caters for 2 updatable images.