arch: arm: arm-v8 a/r: move bss/noinit sections to the end of linker script #79502

Dat-NguyenDuy · 2024-10-07T12:49:39Z

This PR helps to reduces size of zephyr.bin on non-XIP by moving bss/noinit sections to the end of linker script.
As a result, moving __kernel_ram_start to another place to ensure it still include bss + data as before.

Dat-NguyenDuy · 2024-11-08T02:54:25Z

Hi @nashif , @dcpleung could you help review this PR. Thanks

Dat-NguyenDuy · 2024-11-18T02:55:02Z

Hello, @ithinuel could you help to take a look, thanks.

ithinuel · 2024-11-25T14:41:05Z

This seems related to #74042 and more specifically #60368 which was reverted in #61595.

@gramsay0 & @tejlmand, you have a much better understanding of linker scripts than I have.
Could you have a look at this change please?

gramsay0 · 2024-11-26T20:16:51Z

@ithinuel #60368 was reverted because of unrelated, undefined behavior in a test, but was never reapplied after the test was fixed (see the last few comments in #61572).

AFAICT this looks good

tejlmand

Changes in themselves looks reasonable, but I would like this work to be aligned with the work done in #74042, so that we can have similar structure / layout on the cortex_m and cortex_a_r, to the extent possible of course;

I made some comments here: #74042 (review)
Please take a look, especially:

Seems like the arm64, cortex_a_r suffers the same issue and for the above reason but also because it's nice to have the linker.ld files somewhat aligned in how they look, then it would be nice to have those aligned as well.

#74042 covers cortex_m, and this PR covers cortex_a_r, however they define the bss section differently in the two files.
For #74042 (cortex_m) the bss section is moved all the way to the end of the linker script, and this PR just moves it a little bit.

Please get the location in the ld templates aligned between the two PRs.
It's hard enough already when looking into the ld template files, and having same sections ordered differently when there is no obvious reason just make things harder to understand and update.

Perhaps going for the at-the-end approach used in #74042, as that also seems to cover the last_section counter when XIP is used, except for the ITCM case, see #74042 (comment).

as for:

and disabling Kconfig LINKER_LAST_SECTION_ID by default if non-XIP (it is seems only needed for XIP)

then I wonder from where this impression stems ?
The _flash_used might be used by images themselves to know their size or by bootloaders / validation code to know the size of image. Thus independent to XIP / non-XIP.
I believe moving the BSS section description to the end will have positive impact on the _flash_used calculation.

Dat-NguyenDuy · 2025-01-23T09:29:14Z

Hi @tejlmand, could you pls revisit this PR. Thanks a lot.

wearyzen · 2025-02-05T19:02:39Z

include/zephyr/arch/arm/cortex_a_r/scripts/linker.ld

+         */
+        . = ALIGN(4);
+        __bss_start = .;
+        __kernel_ram_start = .;


This essentially reduces __kernel_ram_start from bss+data to just bss and doesn't seem to be inline with its definition here.

This also changes the dynamic region configured by mpu here

And if there is an attempt to change __kernel_ram_start to something other than __bss_start just to comply with this PR then there is also this code in linker which assumes __kernel_ram_start is same as __bss_start.

I am not an expert in linker scripts but wanted to highlight the above issue for other experts reviewing this.

@wearyzen thanks for those observations.

This essentially reduces __kernel_ram_start from bss+data to just bss and doesn't seem to be inline with its definition here.

Nice catch 👍 .
afaict, and what you also observed, is that the __kernel_start_ram symbol should have been moved so that it still covers the same memory region as before, even though the sections within that region is reorganized.

Seems this should also be addressed as followup from this PR: #74042
FYI @gramsay0

@wearyzen thanks for finding that!

It looks like there was a similar issue and fix for aarch64 here:
7473aa9#diff-197a7777652a0ad3f659a00cf6e1840c8e1e0c0612857064a4d1bb21f56beea6

Seems this should also be addressed as followup from this PR: #74042
FYI @gramsay0

@tejlmand Yep, will do 👍

@wearyzen, @tejlmand I have addressed the comment. Pls take a look. Thanks

tejlmand

Please take a look at the observation from @wearyzen to ensure the region defined as the kernel ram region is preserved even if the internal sections are relocated.

tejlmand · 2025-02-06T09:46:22Z

include/zephyr/arch/arm/cortex_a_r/scripts/linker.ld

+         */
+        . = ALIGN(4);
+        __bss_start = .;
+        __kernel_ram_start = .;


@wearyzen thanks for those observations.

This essentially reduces __kernel_ram_start from bss+data to just bss and doesn't seem to be inline with its definition here.

Nice catch 👍 .
afaict, and what you also observed, is that the __kernel_start_ram symbol should have been moved so that it still covers the same memory region as before, even though the sections within that region is reorganized.

Seems this should also be addressed as followup from this PR: #74042
FYI @gramsay0

Dat-NguyenDuy · 2025-02-07T04:06:41Z

Yep, sure. I will address @wearyzen's comment.

ibirnbaum · 2025-02-17T08:54:52Z

Hi all,

I'm the author of #60368, as was mentioned before, this PR was first merged but then reverted due to what later turned out to be a bug in a logging-related test. At that time, I didn't get around to getting that PR started again, and things went dormant for quite some time, as my colleagues and I eventually just used the linker command file from the PR out-of-tree, as we had to get rid of the potential massive .bss/.noinit bloat in our flat .bin images one way or another. As far as I can tell, my version of the linker command file can still be merged with relatively little effort, not too much has changed in the cortex_a_r linker command file since then so we just stuck to this solution.

So thanks to @Dat-NguyenDuy for picking up the issue again and making a fix available to everyone!

I had a look at my old PR and two things I can comment on from the sideline are the following (referring to: https://github.com/ibirnbaum/zephyr/blob/d8545f42e2bfceb51dd89b990c632401374be70f/include/zephyr/arch/arm/aarch32/cortex_a_r/scripts/linker.ld):

It seemed by the time that I submitted my PR nobody had really tried building for XIP on Cortex-A, and when building for qemu_cortex_a9 or actual Zynq-based hardware, there was a weird syntax error hidden behind the SECTION_DATA_PROLOGUE macro. That's why I defined SECTION_ALIGN in dependency of XIP mode. Does that problem still exist, or did someone change something about SECTION_DATA_PROLOGUE in the meantime?
There was an issue revolving around the placement of z_mapped_start, which was documented in aarch32 excn vector not pinned in mmu causing newlib heap overlap #51024. This would have been fixed if my PR had been (re-)merged. If z_mapped_start does not cover the first 4k of the image, which contains the exception vectors to be copied to 0x0 (plus the required padding up to the size of a full MMU page), the first call that maps memory at run-time via the MMU will assign the single page where the vectors used to be in the image followed by subsequent pages which are above the Zephyr image. So any first memory mapping at run-time which is > 4k will work with memory that is not physically contiguous. In the end, during my testing, it eventually turned out that the position of z_mapped_start already used for XIP was also applicable for non-XIP and solved the issue of the memory mapper thinking that this single 4k page below all the Zephyr code/data was suitable for a user mapping at run-time.

If it's helpful, you can ping me for verification testing on the Zynq once the modifications of the linker command file are considered final.

mathieuchopstm · 2025-04-01T15:31:54Z

as for:

and disabling Kconfig LINKER_LAST_SECTION_ID by default if non-XIP (it is seems only needed for XIP)

then I wonder from where this impression stems ? The _flash_used might be used by images themselves to know their size or by bootloaders / validation code to know the size of image. Thus independent to XIP / non-XIP. I believe moving the BSS section description to the end will have positive impact on the _flash_used calculation.

I agree with @Dat-NguyenDuy 's analysis about this point: LINKER_LAST_SECTION_ID should be disabled when !XIP, otherwise the entire noinit is emitted (filled with 0xFF padding) in zephyr.bin, followed by the LINKER_LAST_SECTION_ID_PATTERN word.

Not blocking, but a follow-up PR or issue should be opened to address aforementionned point after this PR is merged. (A solution might just be to have a different formula for calculating _flash_used depending on whether CONFIG_XIP is enabled?)

wearyzen · 2025-04-04T09:47:07Z

@Dat-NguyenDuy could you please rebase and resolve conflicts in this PR?

Dat-NguyenDuy · 2025-04-04T16:47:58Z

@Dat-NguyenDuy could you please rebase and resolve conflicts in this PR?
I've fixed the conflict. I see you have changed this recently:

#if ROM_ADDR != RAM_ADDR
    . = RAM_ADDR;
#endif

so my update (used #if CONFIG_XIP) is not needed anymore.

Edit: Hi @wearyzen, looking into your recently merged PR: https://github.com/zephyrproject-rtos/zephyr/pull/87965/files. After the PR merged, the text region is overlapping with RAM in non-XIP again (that was addressed by this PR: #77291).

IMO, for non-XIP, the RAM_ADDR and ROM_ADDR should be the same (SRAM), at least for ARM. CONFIG_FLASH_<> should not be used, because the default linker script does not use Flash at all, no input sections are placed in there: https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/cortex_a_r/scripts/linker.ld#L22-L26, https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/cortex_m/scripts/linker.ld#L22-L26.
-> default linker script should be updated instead.

Also by default, if non-XIP, CONFIG_FLASH_SIZE=0, https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/Kconfig#L238. Some SoCs override this value for specific purposes, such as configuring the MPU for Flash, which is not necessary. I think this can be cleaned up in the future.

Dat-NguyenDuy · 2025-04-04T16:50:50Z

as for:

and disabling Kconfig LINKER_LAST_SECTION_ID by default if non-XIP (it is seems only needed for XIP)

then I wonder from where this impression stems ? The _flash_used might be used by images themselves to know their size or by bootloaders / validation code to know the size of image. Thus independent to XIP / non-XIP. I believe moving the BSS section description to the end will have positive impact on the _flash_used calculation.

I agree with @Dat-NguyenDuy 's analysis about this point: LINKER_LAST_SECTION_ID should be disabled when !XIP, otherwise the entire noinit is emitted (filled with 0xFF padding) in zephyr.bin, followed by the LINKER_LAST_SECTION_ID_PATTERN word.

Not blocking, but a follow-up PR or issue should be opened to address aforementionned point after this PR is merged. (A solution might just be to have a different formula for calculating _flash_used depending on whether CONFIG_XIP is enabled?)

Yeah, i was wondering the same, but since Tejlmand mentioned that the scenario is difficult to reproduce, so we decided to leave it as it is.

wearyzen · 2025-04-07T15:20:00Z

Edit: Hi @wearyzen, looking into your recently merged PR: https://github.com/zephyrproject-rtos/zephyr/pull/87965/files. After the PR merged, the text region is overlapping with RAM in non-XIP again (that was addressed by this PR: #77291).

Hi @Dat-NguyenDuy, so in 87965, I reverted to what was initially proposed in 77291 (ref: #77291 (comment)) so now I am bit confused and wondering if 77291 fixed any problem at all in the initial version😕 .
1 thing is for sure that, neither of the 2 solutions in the above PRs is completely right. With #if CONFIG_XIP the ci fails as reported here and with 87965 you still have the ram overlapping with text.
Update: we can use CONFIG_XIP to avoid the overlap issue however, we will also have to add entries in MMU config to avoid ci failures like #77291 (comment). Since this looks like a topic of its own, we can do this as part of another PR and get this one merged first. Actually, I can push a draft PR once this gets merged to fix the overlap issue and we can discuss it over there if you would like.

IMO, for non-XIP, the RAM_ADDR and ROM_ADDR should be the same (SRAM), at least for ARM. CONFIG_FLASH_<> should not be used, because the default linker script does not use Flash at all, no input sections are placed in there: https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/cortex_a_r/scripts/linker.ld#L22-L26, https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/cortex_m/scripts/linker.ld#L22-L26. -> default linker script should be updated instead.

Also by default, if non-XIP, CONFIG_FLASH_SIZE=0, https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/Kconfig#L238. Some SoCs override this value for specific purposes, such as configuring the MPU for Flash, which is not necessary. I think this can be cleaned up in the future.

I don't have much experience on linker scripts as you do but I agree that it needs a cleanup since there is a lot of confusion.
I would appreciate if you or some linker expert would help on this.

include/zephyr/arch/arm/cortex_a_r/scripts/linker.ld

By moving data section between rodata and bss, and disabling Kconfig LINKER_LAST_SECTION_ID by default if non-XIP (it is seems only needed for XIP), the size of zephyr.bin can be reduced significantly on non-XIP system. As a result, moving __kernel_ram_start to another place to ensure it still include bss + data as before. Signed-off-by: Dat Nguyen Duy <dat.nguyenduy@nxp.com>

wearyzen · 2025-04-07T17:59:40Z

@tejlmand could you please have a look at this PR as well? there are few new follow ups raised but it would be good to get this one merged first.

Dat-NguyenDuy · 2025-04-16T10:37:34Z

Hi @tejlmand, could you take another look, you left one blocker for this PR. Thanks

zephyrbot added area: Kconfig area: ARM ARM (32-bit) Architecture area: Linker Scripts area: Architectures labels Oct 7, 2024

zephyrbot requested review from bbolen, carlocaione, dcpleung, galak, ithinuel, MaureenHelm, microbuilder, nashif, stephanosio and tejlmand October 7, 2024 12:50

zephyrbot assigned dcpleung and nashif Oct 7, 2024

Dat-NguyenDuy force-pushed the armv8-a/r-linker-improvement branch 3 times, most recently from 7f3d81d to 2e82da9 Compare October 12, 2024 09:21

stephanosio assigned ithinuel and unassigned nashif Nov 8, 2024

dcpleung previously approved these changes Nov 18, 2024

View reviewed changes

dcpleung removed their assignment Nov 18, 2024

gramsay0 previously approved these changes Nov 26, 2024

View reviewed changes

tejlmand requested changes Nov 27, 2024

View reviewed changes

tejlmand mentioned this pull request Nov 27, 2024

arch: arm: cortex_m: Move bss/noinit sections to the end of linker script #74042

Merged

ithinuel removed their assignment Jan 17, 2025

Dat-NguyenDuy requested a review from tejlmand February 5, 2025 03:11

wearyzen reviewed Feb 5, 2025

View reviewed changes

wearyzen mentioned this pull request Feb 5, 2025

arch: arm: cortex_m: Consolidate bss/noinit sections for kernel/user mode #83721

Merged

tejlmand requested changes Feb 6, 2025

View reviewed changes

Dat-NguyenDuy dismissed gramsay0’s stale review via 316ff5b February 13, 2025 16:54

Dat-NguyenDuy force-pushed the armv8-a/r-linker-improvement branch from 8334eba to 316ff5b Compare February 13, 2025 16:54

wearyzen previously approved these changes Mar 14, 2025

View reviewed changes

wearyzen requested review from tejlmand, dcpleung and gramsay0 March 31, 2025 11:29

Dat-NguyenDuy dismissed wearyzen’s stale review via 9233ba7 April 4, 2025 16:42

Dat-NguyenDuy force-pushed the armv8-a/r-linker-improvement branch from 316ff5b to 9233ba7 Compare April 4, 2025 16:42

wearyzen reviewed Apr 7, 2025

View reviewed changes

include/zephyr/arch/arm/cortex_a_r/scripts/linker.ld Outdated Show resolved Hide resolved

Dat-NguyenDuy force-pushed the armv8-a/r-linker-improvement branch from 9233ba7 to a189bf3 Compare April 7, 2025 16:43

wearyzen approved these changes Apr 7, 2025

View reviewed changes

tejlmand approved these changes May 1, 2025

View reviewed changes

kartben merged commit eb7ac1c into zephyrproject-rtos:main May 1, 2025
24 checks passed

manuargue deleted the armv8-a/r-linker-improvement branch May 4, 2025 09:38

arch: arm: arm-v8 a/r: move bss/noinit sections to the end of linker script #79502

arch: arm: arm-v8 a/r: move bss/noinit sections to the end of linker script #79502

Uh oh!

Conversation

Dat-NguyenDuy commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dat-NguyenDuy commented Nov 8, 2024

Uh oh!

Dat-NguyenDuy commented Nov 18, 2024

Uh oh!

ithinuel commented Nov 25, 2024

Uh oh!

gramsay0 commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

Dat-NguyenDuy commented Jan 23, 2025

Uh oh!

wearyzen Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

gramsay0 Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dat-NguyenDuy Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

Dat-NguyenDuy commented Feb 7, 2025

Uh oh!

ibirnbaum commented Feb 17, 2025

Uh oh!

mathieuchopstm commented Apr 1, 2025

Uh oh!

wearyzen commented Apr 4, 2025

Uh oh!

Dat-NguyenDuy commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dat-NguyenDuy commented Apr 4, 2025

Uh oh!

wearyzen commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wearyzen commented Apr 7, 2025

Uh oh!

Dat-NguyenDuy commented Apr 16, 2025

Uh oh!

Uh oh!

Uh oh!

Dat-NguyenDuy commented Oct 7, 2024 •

edited

Loading

gramsay0 commented Nov 26, 2024 •

edited

Loading

wearyzen Feb 5, 2025 •

edited

Loading

gramsay0 Feb 6, 2025 •

edited

Loading

Dat-NguyenDuy commented Apr 4, 2025 •

edited

Loading

wearyzen commented Apr 7, 2025 •

edited

Loading