Skip to content
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

arm-v8 a/r: linker: move data section between rodata and bss #79502

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Dat-NguyenDuy
Copy link
Contributor

This PR helps to reduces size of zephyr.bin on non-XIP by moving data section between rodata and bss.
Also move to use proper macro helpers when declaring no-init section.

@Dat-NguyenDuy
Copy link
Contributor Author

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

@stephanosio stephanosio assigned ithinuel and unassigned nashif Nov 8, 2024
@Dat-NguyenDuy
Copy link
Contributor Author

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

dcpleung
dcpleung previously approved these changes Nov 18, 2024
@dcpleung dcpleung removed their assignment Nov 18, 2024
@ithinuel
Copy link
Collaborator

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
Copy link
Contributor

gramsay0 commented Nov 26, 2024

@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

gramsay0
gramsay0 previously approved these changes Nov 26, 2024
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Dat-NguyenDuy commented Nov 29, 2024

Hi @tejlmand, i agree with you that we should align the work for linker script, to have a similar layout for Arm platform. I will take a look into gramsay0's PR.

However, for this and disabling Kconfig LINKER_LAST_SECTION_ID by default if non-XIP (it is seems only needed for XIP).
At first, i cannot reproduce the issue you mentioned here #50711 (even XIP/non-XIP, at least on arm cortex_a_r). So i would really appreciate if you can point me how to replicate the issue to better understanding?

The purpose of _flash_used for non-XIP is not clear to me. From what i understand the _flash_used is size of LMA and size of LMA == VMA on non-XIP device. And there is last_ram_section already https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/linker/ram-end.ld. People normally uses image_ram_end for this case.

@gramsay0
Copy link
Contributor

gramsay0 commented Dec 5, 2024

However, for this and disabling Kconfig LINKER_LAST_SECTION_ID by default if non-XIP (it is seems only needed for XIP). At first, i cannot reproduce the issue you mentioned here #50711 (even XIP/non-XIP, at least on arm cortex_a_r). So i would really appreciate if you can point me how to replicate the issue to better understanding?

I also tried to recreate this. I think the issue was that rom_report was giving different results to the build footprint i.e. these both produce 8094 bytes:

west build -b qemu_cortex_m3 zephyr/samples/hello_world -t rom_report
...
Memory region         Used Size  Region Size  %age Used
           FLASH:        8094 B       256 KB      3.09%
...
Root   8094 100.00%  -

Then I added a test section in the linker script before .last_section:

     /* Sections generated from 'zephyr,memory-region' nodes */
     LINKER_DT_SECTIONS()
 
+SECTION_PROLOGUE(.test_section,,ALIGN(32))
+{
+  BYTE(1)
+} GROUP_LINK_IN(ROMABLE_REGION)
+
 /* Must be last in romable region */
 SECTION_PROLOGUE(.last_section,,)
 {

But the rom_report differed from the build footprint (8101 vs 8095 bytes):

west build -b qemu_cortex_m3 zephyr/samples/hello_world -t rom_report
...
Memory region         Used Size  Region Size  %age Used
           FLASH:        8101 B       256 KB      3.09%
...
Root   8095 100.00%  -

@tejlmand is this a bug, or am I misunderstanding what .last_section is intended for?

@tejlmand
Copy link
Collaborator

At first, i cannot reproduce the issue you mentioned here #50711

The issue is very hard to reproduce, and when I investigated the original cause I was not able to directly reproduce it in the Zephyr tree, only through some extra hacks which I had to use to verify that I had really solved the issue.

Besides the linked PR which briefly describes what actually happened, then triggering this issue was a combination of multiple things which seems to throw of the linker. I'm not fully sure if there could also be a linker issue involved.

The issue was originally triggered by use of zephyr_linker_sources() with RAM_SECTIONS (and also RWDATA iirc) which are provided for Zephyr downstream users to inject custom linker ld snippets into final linker script.

A combination of this feature together with use of KEEP and NOLOAD it those snippets seemed to be able to throw of the last_section calculation, if the symbol _flash_used was actually used in code.
In the exact case, this symbol was used for meta data information for firmware update, like here:
https://github.com/nrfconnect/sdk-nrf/blob/c4a02e0a9f9dc7ef3d14c13f655faf6143817e68/subsys/fw_info/fw_info.c#L24
https://github.com/nrfconnect/sdk-nrf/blob/c4a02e0a9f9dc7ef3d14c13f655faf6143817e68/subsys/fw_info/fw_info.c#L38

Now I don't remember the exact construct which can trigger this, but it was not as simple as tried here: #79502 (comment)

But the final investigation before #50711 proved that if a symbol was written into the section, instead of NOLOAD, then the location counter and flash usage would sync up.

Due to the fact that this indeed is hard to reproduce, then I want to play this safe, because having to once again trace down a bug like this can be time-consuming. A second problem is that in the case where someone uses this symbol for meta data, for example as part of a firmware update process, then this is the kind of error you don't want to experience on a target.

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.

Signed-off-by: Dat Nguyen Duy <[email protected]>
…ions

If a section is marked as noload, it will not be loaded
into memory by the program loader. So move to use:

- SECTION_PROLOGUE: because SECTION_DATA_PROLOGUE has
  ALIGN_WITH_INPUT attribute (on XIP) which seems is
  not needed for this sections

- GROUP_NOLOAD_LINK_IN: as it's supposed to be used for
  this sections. The ROM region will be consumed if using
  GROUP_DATA_LINK_IN and LMA != VMA

Signed-off-by: Dat Nguyen Duy <[email protected]>
@Dat-NguyenDuy Dat-NguyenDuy dismissed stale reviews from gramsay0 and dcpleung via 867b6c7 January 11, 2025 09:02
@Dat-NguyenDuy Dat-NguyenDuy force-pushed the armv8-a/r-linker-improvement branch from 2e82da9 to 867b6c7 Compare January 11, 2025 09:02
@zephyrbot zephyrbot requested a review from wearyzen January 11, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants