-
Notifications
You must be signed in to change notification settings - Fork 1.4k
NSIB nRF54H20 support - no partition manager #22274
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
Conversation
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: f528302c977793cebf2edc084c01993078750ec5 more detailssdk-nrf:
mcuboot:
Github labels
List of changed files detected by CI (55)
Outputs:ToolchainVersion: 4aa3467a6d Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
34d5709
to
5557ad0
Compare
You can find the documentation preview for this PR here. |
320daae
to
883abbb
Compare
Kconfig.nrf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go in the board file, not here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will accept, though need to get the upstream hex merging system reviewed and use that eventually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean this one: zephyrproject-rtos/zephyr#83085 ?
I am still unsure how would we handle the B0 case. Specifically
- the
build/app_provision.hex
file is not related to any image... There does not seem to be a mechanism to add this file to merged.hex - I am also signed_by_mcuboot_and_b0_mcuboot.hex and signed_by_mcuboot_and_b0_s1_image.hex - although these are connected to images they are generated by sysbuild, not by the the image cmake. But most of all, it seems that merging only supports files pointed to by one of the three variables:
CONFIG_KERNEL_BIN_NAME
,BYPRODUCT_KERNEL_SIGNED_HEX_NAME
andBYPRODUCT_KERNEL_SIGNED_CONFIRMED_HEX_NAME
- so there seemingly is not way using files generated for NSIB, there might be a need of a noup change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that provision file needs to go, it is awful, so it will not be a problem - see NCSDK-31918
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, just read the dts directly from an image and do not use generator expressions (so the whole file can go)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with removing generator expressions - done.
However with the file itself I'd like to leave most of it - I've now changed the name to bootloader_dts_utils.cmake
There are two things this file achieves:
-
I've now added functions
nsib_get_s0_address
,nsib_get_s1_address
andnsib_get_provision_address
. This, in case of S0/S1 allows to provide an abstraction layer, thanks to which other files does need to duplicate the code related to the fact that depending on the bootloader configuration (mcuboot present or not) the S0/S1 labels can be either boot_partition/boot_partition_slot1 or slot0_partition/slot1_partition -
It provides validation (I've now wrapped it in the
verify_bootloader_dts_configuration
) function, which allows to verify that the appropriate partitions are used - for example verifies that zephyr,code-partition for mcuboot is set toboot_partition
- and other similar checks. Almost all of our team members ran into an issue in which due to a wrong overlay the images were linked to wrong partitions which led to strange errors.
So we think that having verbose error messages in such cases will greatly help the user.
cmake/sysbuild/image_signing.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly, if not using partition manager, do not store this in the cache. The cache is needed for variable replacement, it's sticky/buggy, so let's avoid creating issues for users that are only present due to how PM works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed
cmake/sysbuild/provision_hex.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for nrf54h20 soc Kconfig instead then allow if set, otherwise throw error (nrf54l, nrf52, etc. should all show error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmake/sysbuild/provision_hex.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmake/sysbuild/sign.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limit this to when PM is disabled, same with below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? CONFIG_ROM_START_OFFSET
defaults to 0 if partition manager is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do pointless calculations which aren't clear from the code where they are used?
cmake/sysbuild/sign.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--start-offset ${${slot}_input_data_start_offset} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the suggestion, the --start-offset
argument is necessary, otherwise the hash would be calculated CONFIG_ROM_START_OFFSET
times 0x0 at the start of the image, which is later filled by imgtool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with PM this is 0, so like above it's doing something pointless for something that doesn't need it, considering if the value is not supplied to the script it will default to 0
7b3ff2e
to
05782b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A proof-of-concept PR to support NSIB nRF54H20 without relying on the partition manager.
- Introduces a new offset parameter for hashing and signature validation functions.
- Updates CMake and source code to remove partition manager dependencies and use NSIB-specific addresses/constants.
- Adjusts tests and CLI argument parsing accordingly.
Reviewed Changes
Copilot reviewed 19 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
scripts/bootloader/validation_data.py | Adds input_data_offset parameter and adjusts validation logic. |
scripts/bootloader/tests/validation_data_test.py | Updates tests to pass the new offset parameter. |
scripts/bootloader/hash.py | Propagates a new start_offset parameter in hash calculation. |
scripts/bootloader/do_sign.py | Updates signing functions to use a start_offset parameter. |
samples/bootloader/src/main.c | Updates partition constants and includes for NSIB usage. |
include/fw_info.h | Adjusts memory map offsets to reflect new NSIB configuration. |
include/bl_storage.h | Replaces PM-based storage addresses with NSIB-based ones. |
Files not reviewed (13)
- cmake/sysbuild/b0_mcuboot_signing.cmake: Language not supported
- cmake/sysbuild/bootloader_dts_utils.cmake: Language not supported
- cmake/sysbuild/image_signing.cmake: Language not supported
- cmake/sysbuild/provision_hex.cmake: Language not supported
- cmake/sysbuild/secureboot_merge.cmake: Language not supported
- cmake/sysbuild/sign.cmake: Language not supported
- samples/bootloader/boards/nrf54h20dk_nrf54h20_cpuapp_iron.conf: Language not supported
- samples/bootloader/boards/nrf54h20dk_nrf54h20_cpuapp_iron.overlay: Language not supported
- samples/bootloader/boards/nrf54h20dk_nrf54h20_cpuapp_iron_mcuboot_b0.conf: Language not supported
- samples/bootloader/boards/nrf54h20dk_nrf54h20_cpuapp_iron_mcuboot_b0.overlay: Language not supported
- samples/bootloader/sysbuild/mcuboot_s1_image.overlay: Language not supported
- samples/bootloader/sysbuild/s1_image.overlay: Language not supported
- subsys/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (3)
scripts/bootloader/validation_data.py:42
- [nitpick] Consider renaming the parameter from 'input_data_offset' to 'start_offset' to be consistent with similar parameters introduced in hash.py and do_sign.py.
def get_hash(self, input_hex: IntelHex, input_data_offset: int) -> bytes:
samples/bootloader/src/main.c:139
- Verify that the new NSIB_B0_ADDRESS and NSIB_B0_SIZE macros are correctly defined and consistently used across the codebase.
int err = fprotect_area(NSIB_B0_ADDRESS, NSIB_B0_SIZE);
include/bl_storage.h:182
- Ensure that NSIB_PROVISION_ADDRESS is consistently defined and that its usage correctly replaces the PM_PROVISION_ADDRESS in all related modules.
#define BL_STORAGE ((const volatile struct bl_storage_data *)(NSIB_PROVISION_ADDRESS))
05782b9
to
f58ba62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a JIRA ref here too?
samples/bootloader/boards/nrf54h20dk_nrf54h20_cpuapp_iron_mcuboot_b0.conf
Outdated
Show resolved
Hide resolved
f58ba62
to
05784c7
Compare
4c2034e
to
7b19d1a
Compare
b9d3bd4
to
ddda2df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors partition handling to remove Partition Manager dependencies, adds device-tree–based partition definitions, adjusts firmware info offset computation, and introduces nRF54H20-specific board support.
- Replace PM_* symbols with DT-based NSIB_* macros and add
bl_partitions.h
- Update
FW_INFO_VECTOR_OFFSET
logic to useCONFIG_ROM_START_OFFSET
when PM is disabled - Add new board YAML and variant entry for nRF54H20 DK with MCUBOOT B0 support
Reviewed Changes
Copilot reviewed 24 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
include/fw_info.h | Added branch for CONFIG_ROM_START_OFFSET when PM is disabled |
include/bl_storage.h | Swapped PM include for bl_partitions.h and updated BL_STORAGE |
include/bl_partitions.h | New DT- and PM-based NSIB_* partition address/size macros |
boards/nordic/nrf54h20dk/*.yaml | Added new board definition for MCUBOOT B0 configuration |
boards/nordic/nrf54h20dk/board.yml | Added mcuboot/b0 variant qualifier |
Files not reviewed (15)
- CODEOWNERS: Language not supported
- boards/nordic/nrf54h20dk/Kconfig.defconfig: Language not supported
- boards/nordic/nrf54h20dk/Kconfig.nrf54h20dk: Language not supported
- boards/nordic/nrf54h20dk/Kconfig.sysbuild: Language not supported
- boards/nordic/nrf54h20dk/board.cmake: Language not supported
- boards/nordic/nrf54h20dk/nrf54h20dk_nrf54h20-memory_map_iron_mcuboot_b0.dtsi: Language not supported
- boards/nordic/nrf54h20dk/nrf54h20dk_nrf54h20_cpuapp_iron_mcuboot_b0.dts: Language not supported
- boards/nordic/nrf54h20dk/nrf54h20dk_nrf54h20_cpuapp_iron_mcuboot_b0_defconfig: Language not supported
- cmake/sysbuild/b0_mcuboot_signing.cmake: Language not supported
- cmake/sysbuild/bootloader_dts_utils.cmake: Language not supported
- cmake/sysbuild/image_signing.cmake: Language not supported
- cmake/sysbuild/provision_hex.cmake: Language not supported
- cmake/sysbuild/secureboot_merge.cmake: Language not supported
- cmake/sysbuild/sign.cmake: Language not supported
- samples/bootloader/boards/nrf54h20dk_nrf54h20_cpuapp_iron.conf: Language not supported
Comments suppressed due to low confidence (2)
include/bl_partitions.h:31
- The macro
SOC_NV_FLASH_NODE_B0
is not defined here; consider usingDT_REG_SIZE(NSIB_B0_CONTAINER_NODE)
to consistently reference the container node.
#define NSIB_B0_CONTAINER_SIZE DT_REG_SIZE(SOC_NV_FLASH_NODE_B0)
include/bl_partitions.h:42
- In the DT-based branch, address macros for S0 and S1 (
NSIB_S0_ADDRESS
andNSIB_S1_ADDRESS
) are missing, which will lead to undefined references when used elsewhere.
#define NSIB_S0_SIZE DT_REG_SIZE(DT_NODELABEL(s0_partition))
include/fw_info.h
Outdated
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes OK but needs updating as per linked comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these all need changing #22462
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
15b2aca
to
3035b23
Compare
sysbuild/secureboot.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply the image configuration script to it e.g.
set_target_properties(${IMAGE} PROPERTIES
IMAGE_CONF_SCRIPT ${ZEPHYR_BASE}/share/sysbuild/image_configurations/BOOTLOADER_image_default.cmake
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fixed
boards/nordic/nrf54h20dk/board.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
include/bl_partitions.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes no sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be addressed by now
6ecff1e
to
ceee67b
Compare
This commit introduces the changes needed for running the NSIB on nRF54H - currently when running an application-core only application. Note: the NSIB is still experimental, as OTP memory is not handled properly - MRAM is used instead. Signed-off-by: Artur Hadasz <[email protected]>
Added as an extension to the nrf54h20dk board. Signed-off-by: Artur Hadasz <[email protected]>
Added support for the nrf54h20dk/nrf54h20/cpuapp/iron/b0/mcuboot in the nordic secure bootloader. Signed-off-by: Artur Hadasz <[email protected]>
The support for the sample with the nrf54h20dk/nrf54h20/cpuapp/iron/b0/mcuboot board was added. Signed-off-by: Artur Hadasz <[email protected]>
ceee67b
to
f528302
Compare
Closing as NSIB won't be supported on nRF54H20 |
The most of the changes introduced are related to getting rid of partition manager dependencies, however there are a couple of strictly nRF54H20 related changes.
The main changes introduced by this PR are:
In case of the NSIB + MCUBOOT configuration the labels are:
b0_partition - for placing the B0 code itself
provision_partition - for placing the "provisioning" information (app_provision.hex)
boot_partition - for placing the MCUBOOT S0 image
boot_partition_slot1 - for placing the MCUBOOT S1 image
slot0_partition - application primary slot
slot1_partition - mcuboot secondary slot
In case NSIB without MCUBOOT is used, slot0_partition and slot1_partition are used to specify the S0 and S1 images.
Ensuring devicetree consistency across all of the images. This is done in the
cmake/sysbuild/bootloader_vars_preparation.cmake
file. The code in this file checks that the all the needed partitions (with labels mentioned in (1)) point to the same nodes across all images. It also checks ifzephyr,code-partition
points to the appropriate partition for every image (for exampleboot_partition
for the mcuboot image and so on)PM_
symbols in CMake as well in the source code/header files are replaced by values taken from the device tree.Changes needed due to the fact that instead of using an additional
_pad
partition before every image (which was the case with partition manager), theCONFIG_ROM_START_OFFSET
is used to add MCUBOOT padding. This has several implications, such the need to calculate the NSIB signatures from a certain offset - asCONFIG_ROM_START_OFFSET
causes the start of the image to be filled with zeroes, which are filled byimgtool
after signing the image for NSIB.Workarounds for OTP memory for nRF54H20 - the LCS/Key management/Monotonic counters on nRF54H20 will be handled in a different manner than for older devices, probably through SSF services. However, these services are not available yet. Thus, to mimic the NSIB functionality without the needed services being present, MRAM instead of OTP is used.
Note: the nrf54h20dk/nrf54h20/cpuapp/iron/mcuboot/b0 board was added in zephyr. This might not be the final approach. Other ways of specifying the memory map might be used.