Skip to content

Conversation

@nordicjm
Copy link
Contributor

@nordicjm nordicjm commented Oct 1, 2024

No description provided.

@nordicjm nordicjm marked this pull request as draft October 1, 2024 07:47
@nordicjm nordicjm force-pushed the imagenumberpr branch 13 times, most recently from 7223ba7 to 6c2d4b0 Compare October 8, 2024 09:26
@nordicjm nordicjm force-pushed the imagenumberpr branch 7 times, most recently from 9dd5358 to 08730ee Compare October 15, 2024 10:57
@nordicjm nordicjm force-pushed the imagenumberpr branch 3 times, most recently from bea59cd to 3a100a5 Compare October 17, 2024 08:17
@nordicjm nordicjm marked this pull request as ready for review October 17, 2024 08:44
@nordicjm nordicjm added this to the ncs-2.8.0 milestone Oct 17, 2024
de-nordic
de-nordic previously approved these changes Oct 22, 2024
@de-nordic de-nordic self-requested a review October 22, 2024 14:09
@de-nordic de-nordic dismissed their stale review October 22, 2024 14:11

Clicked wrong review.


#if (MCUBOOT_IMAGE_NUMBER == 2) && defined(PM_B0_ADDRESS) && \
!defined(CONFIG_NRF53_MULTI_IMAGE_UPDATE)
/* This configuration is peculiar - the one physical secondary slot is
Copy link
Contributor

Choose a reason for hiding this comment

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

how it is resolved now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has it's own dedicated image number (present in MCUboot only, not present in the application)

static inline void sec_slot_touch(struct boot_loader_state *state)
{
uint8_t idx = (SEC_SLOT_PHYSICAL_CNT == 1) ? 0 : BOOT_CURR_IMG(state);
#if CONFIG_MCUBOOT_MCUBOOT_IMAGE_NUMBER != -1
Copy link
Contributor

Choose a reason for hiding this comment

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

now sec_slot_assignment can be marked twice. I do not understand that.

Copy link
Contributor Author

@nordicjm nordicjm Oct 23, 2024

Choose a reason for hiding this comment

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

This feature is completely broken in child/parent image as I've said many time, depending on what you have, you can get MCUboot to mark an image for upgrade, after which the cleanup code will run, then it will upgrade the primary for 4 bytes, and bricks the module. And brick meaning if access port protection is enabled, the device is irrecoverably dead, likely out in the field. This code here actually works by checking the slots properly and marking both as used if it is a shared slot. Note that all of these test cases are checked as part of sdk-mcuboot CI and pass.

The original code was added at the complete wrong point, it should have been added after the update, not before the update runs, because essentially you have the equivalent of use-after-free-pointer

*/
return BOOT_SWAP_TYPE_NONE;
}
#if 0 && defined(CONFIG_SOC_NRF5340_CPUAPP) && defined(CONFIG_NRF53_MULTI_IMAGE_UPDATE)
Copy link
Contributor

Choose a reason for hiding this comment

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

#if 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Adds support for image IDs that are assigned by sysbuild, which
allows for dynamically supporting different configurations without
needing dummy images to support different modes. Also fixes
multiple deficiencies with the previous code where things were not
properly accounted for e.g. using the swap algorithm including all
swap status parts when updating s0/s1 MCUboot image which could
overwrite and corrupt the image data in the other slot

Signed-off-by: Jamie McCrae <[email protected]>
fixup! [nrf noup] treewide: add NCS partition manager support

Fixes an issue whereby the end address of MCUboot had no
alignment requirements what-so-ever and would produce completely
invalid configurations for NSIB that prevented it from being
upgradeable

Signed-off-by: Jamie McCrae <[email protected]>
Adds a check that will also check the s0/s1 package version of the
currently running MCUboot against a MCUboot update image to ensure
that an older version of MCUboot isn't loaded to the opposite slot

Signed-off-by: Jamie McCrae <[email protected]>
Adds support for child and parent images back, this commit will
be reverted after the NCS 2.8 release when child/parent support
is dropped

Signed-off-by: Jamie McCrae <[email protected]>
Copy link
Contributor

@gchwier gchwier left a comment

Choose a reason for hiding this comment

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

test_sdk_mcuboot aligned on branch sdk-nrf-pr-17567.
CI is green, but this PR includes a significant number of changes. Our tests do not cover every configuration that has been modified, so please be aware that merging this PR carries some risk.

@nordicjm nordicjm merged commit 20ee337 into nrfconnect:main Oct 24, 2024
@nordicjm nordicjm deleted the imagenumberpr branch November 6, 2024 07:50
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.

5 participants