-
Notifications
You must be signed in to change notification settings - Fork 1.4k
NSIB: Cleanup RAM fixes and tests - nRF54L #25136
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
NSIB: Cleanup RAM fixes and tests - nRF54L #25136
Conversation
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: d7a355a7aab148a8eb0025b8db176ef037324851 more detailssdk-nrf:
Github labels
List of changed files detected by CI (20)Outputs:ToolchainVersion: cfa6b06338 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
|
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsdoc.z6.web.core.windows.net/PR-25136/nrf/libraries/security/bootloader/fw_info.html |
This comment was marked as outdated.
This comment was marked as outdated.
acda681 to
96aa097
Compare
96aa097 to
27b6e00
Compare
27b6e00 to
388413f
Compare
| bool "Perform RAM cleanup" | ||
| default y | ||
| depends on !FW_INFO_PROVIDE_ENABLE |
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.
Has the exact same issue as mcu-tools/mcuboot#2451 has including destroying any data sharing capabilities because it doesn't even clear the correct RAM range
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.
It doesn't, as it has the dependency on FW_INFO_PROVIDE which is automatically selected by EXT_API-s. Perhaps instead FW_INFO_PROVIDE_ENABLE we should rather use a more generic Kconfig, like DATA_SHARING_ENABLED, but I do not see a point of adding it until other means of data sharing apart from EXT_API-s are added.
Note that for nRF52/nRF53 platforms this will be 'n' due to FW_INFO_PROVIDE
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.
EXT_API is not panned to be used on nRF54l devices.
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.
By data sharing, I mean data sharing in general, unrelated to ext APIs. E.g. a user can no longer have any memory retained between boots, nor can they share any information from application to bootloader or vice versa - and this is a feature customers and even FAEs are using
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.
FAO @vidarbe
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 is also the retention subsystem, but the current code won't erase the retention memory - I've added a build assert to ensure that: https://github.com/nrfconnect/sdk-nrf/pull/25136/files#diff-a58c0467d91771dc35ca94bb2297b3de0cd6f6330192c6924ca6f56ed973a4f3R26-R44
Also, I've added a test in this PR to verify that indeed the retention memory is not erased.
By the way, considering the boot loops in MCUBOOT, please see: mcu-tools/mcuboot#2451 (comment) - this issue is not present in NSIB, as we have CONFIG_MPU_STACK_GUARD=n - for now, I added a "depends on" so that we do not run into the issue.
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.
PM does not support retention, it would be non-init RAM using a PM static file, e.g. https://github.com/CanvasDM/bt610_firmware/blob/main/pm_static.sb.bt610.yml#L114
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 see, I will in fact remove the default y.
I just keep it for the time being to see which tests fail
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. Removed the default y - it causes to much issues at the moment.
ae55a0c to
119cc0d
Compare
c390b34 to
d97a84c
Compare
d97a84c to
3c4fa42
Compare
3c4fa42 to
f94d72b
Compare
f94d72b to
495650b
Compare
495650b to
f8c4da6
Compare
subsys/bootloader/bl_boot/bl_boot.c
Outdated
| #define REGIONS_OVERLAP(addr1, size1, addr2, size2) \ | ||
| ((addr1) < ((addr2) + (size2)) && (addr2) < ((addr1) + (size1))) | ||
|
|
||
| #define RETAINED_RAM_ADDR(ret_ram_node) DT_REG_ADDR(DT_PARENT(ret_ram_node)) | ||
| #define RETAINED_RAM_SIZE(ret_ram_node) DT_REG_SIZE(DT_PARENT(ret_ram_node)) | ||
|
|
||
|
|
||
| #define CHECK_RETAINED_RAM_NO_SRAM_OVERLAP(ret_ram_node) \ | ||
| BUILD_ASSERT(!REGIONS_OVERLAP(CONFIG_SRAM_BASE_ADDRESS, CONFIG_SRAM_SIZE * 1024, \ | ||
| RETAINED_RAM_ADDR(ret_ram_node), RETAINED_RAM_SIZE(ret_ram_node)), \ | ||
| "Retained RAM region overlaps with defined SRAM, cannot cleanup RAM") | ||
|
|
||
| /* Ensure that the retained RAM region does not overlap with the defined SRAM region. | ||
| * Otherwise, the RAM cleanup would overwrite the retained RAM region. | ||
| */ | ||
| DT_FOREACH_STATUS_OKAY(zephyr_retained_ram, CHECK_RETAINED_RAM_NO_SRAM_OVERLAP); |
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.
note that this check is not really worth keeping since anyone that modifies the driver and changes the name will now get a false negative on this, likewise if they only define it in their application app.overlay file, let's not try to guess how things will work that fail basic sniff tests
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, removed
| @@ -0,0 +1,10 @@ | |||
| # | |||
| # Copyright (c) 2025 Nordic Semiconductor | |||
| # | |||
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 is this in a zephyr sub folder?
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.
By mistake, moved
tests/subsys/bootloader/b0_ram_cleanup/nrf54l15dk_nrf54l15_cpuapp_common.dtsi
Show resolved
Hide resolved
tests/subsys/bootloader/b0_ram_cleanup/nrf54l15dk_nrf54l15_cpuapp_common.dtsi
Show resolved
Hide resolved
f8c4da6 to
99d39d1
Compare
99d39d1 to
fa79724
Compare
| cmake_minimum_required(VERSION 3.20.0) | ||
|
|
||
| find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) | ||
| project(NONE) |
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.
project name is missing i.e. b0_ram_cleanup
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, seems to be a general issue in test/subsys/bootloader - that is why it was also here, I copied the test.
I will create a task to fix that throughout the whole directory
The SB_CLEANUP_RAM was being blocked by the EXT_APIs, which were enabled in the NSIB-s prj.conf There are currently no use cases for EXT_APIs for nRF54L and cleanup up RAM after NSIB finishes its work is an important feature, so the part enabling the EXT_APIs needed to be removed from the NSIB-s prj.conf However, backwards compatibility with applications working on nRF52 and nRF53 needed to be mantained, so the EXT_APIs are now enabled by default in KConfings for the NSIB image for these platforms. Note this change aligns with the EXT_APIs documentation - in the documentation it is not described how to enable the EXT_APIs in NSIB when adding a new custom EXT_API. Now, the template will ensure it is enabled in NSIB. Signed-off-by: Artur Hadasz <[email protected]>
This commit adds tests to verify if the NSIB RAM cleanup functionality works as expected. Signed-off-by: Artur Hadasz <[email protected]>
fa79724 to
d7a355a
Compare
This PR enables the CONFIG_SB_CLEANUP_RAM option by default for nRF54L. To achieve that, it disables the conflicting EXT_API-s functionality for nRF54L, where it is not used and wasn't functional. Backwards compatibility for nRF52 and nRF53 is ensured.
Also, tests are added to verify the functionality works as expected.