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

[gramsay0]

This results in a smaller binary with CONFIG_XIP=n as these NOLOAD sections can be excluded

bss/noinit have been included in CONFIG_XIP=n binaries by default since c107827. This also occurs if using iterable sections or adding code/data to ITCM/DTCM sections. Moving bss/noinit to the end of the linker file should ensure they are always excluded

Partial fix for #72307

[MaureenHelm]

@ithinuel please take a look

[ithinuel]

@MaureenHelm
This seems oddly related to #76651

TBH there’s so much things going on in those files I can’t tell if something is right in all circumnstances or not.
Do we have any linker & Zephyr experts in the community who could help review this?

[pdgendt]

Added @tejlmand as the expert

[gramsay0]

A similar issue was also recently fixed for x86: #73044

It could do with a test; a big static storage variable (e.g. uint8_t some_bss[KB(64)]) and a twister or cmake post build assert on the size of the .bin file would help

[tejlmand]

bss/noinit have been included in CONFIG_XIP=n binaries by default since c107827.

actually it was not c107827 which introduced the .last_section for proper rom usage calculation, the referenced commit only fixed an issue whereby the location counter and actual usage could get out-of-sync.

The .last_section was introduced in d85efe0.

these NOLOAD sections can be excluded

they should normally be excluded from loading by the linker if the linker script is properly written.
I wonder if the bss section for the target on which you see the has mistakenly been placed in rom instead of ram ?

One guess could be the internal macro definition of ROM_REGION vs RAM_REGION which could be wrongly handled when XIP=y.

Taking a look at the issue reported to reproduce this locally before approving a linker script adjustment, even though placing bss later should not impact rom usage on xip targets. I have a hunch that something else has been overlooked.

[gramsay0]

actually it was not c107827 which introduced the .last_section for proper rom usage calculation, the referenced commit only fixed an issue whereby the location counter and actual usage could get out-of-sync.

c107827 removed NOLOAD from .last_section, before that commit it was not included in the binary

-SECTION_PROLOGUE(.last_section,(NOLOAD),)
+SECTION_PROLOGUE(.last_section,,)

(P.S. sorry about the email spam, I accidentally clicked an emoji button on my mobile email client...)

[gramsay0]

I went through the ARM Cortex-M linker.ld file, extracted some section of interest (ignoring userspace) and tried to make an image of what an XIP=y and XIP=n image might look like in the physical memory regions:

With config XIP=y, everything outside of FLASH is either NOLOAD (green) or VMA (virtual memory address) (purple).

With XIP=n:

• ITCM (LMA), DTCM DATA (LMA), Sections snippets and .last_section are "LOAD" sections that can occur after BSS/NOINIT "NOLOAD" sections
• .last_section "LOAD" section is after DT_SECTIONS "NOLOAD" sections
• ITCM and DTCM still use VMA/LMA but the code to copy from LMA to VMA (kernel/xip.c void z_data_copy(void)) is excluded if XIP=n (seems like a separate bug)

---

Flattened memory map of the above for an STM32H7B0:

If you create a ".bin" file it will be from the first to last blue box.
i.e. if you used ITCM code relocation with XIP=n you would have a ".bin" file of at least 576MB on this SoC.

I've gone a bit off topic, but hopefully visualising it helps make it clearer what is going on

[tejlmand]

c107827 removed NOLOAD from .last_section, before that commit it was not included in the binary

Correct, but as you have also noticed, the ITCM, DTCM and the inclusion of linker section-snippets also comes after bss.
But as those are not included by every board / SoC then removing the NOLOAD for the .last_section we went from this happening in certain cases to always happen when XIP=n.

But I would still argue that the original cause is that .last_section was not carefully enough placed in linker.ld by the original commit which introduce it.

ITCM and DTCM still use VMA/LMA but the code to copy from LMA to VMA (kernel/xip.c void z_data_copy(void)) is excluded if XIP=n (seems like a separate bug)

yep, or perhaps more a feature which has not been taken all the way , that is XIP=n was not considered when doing the ITCM / DTCM support.

[tejlmand]

Thanks for both finding, investigating and providing a fix for this issue 👍 .

I have found no side-effects so changes lgtm, but I have a couple additional wishes before a +1:

• can you please take userspace support into consideration in this fix.
  CONFIG_XIP=n CONFIG_USERSPACE=y still suffers this problem after this PR, and it's not nice just waiting for someone else hitting this issue again later.
  

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

  Lines 283 to 292 in 7773fe8

  	#if defined(CONFIG_USERSPACE)
  	#define APP_SHARED_ALIGN . = ALIGN(_region_min_align);
  	#define SMEM_PARTITION_ALIGN MPU_ALIGN
  	#include <app_smem.ld>
  	_app_smem_size = _app_smem_end - _app_smem_start;
  	_app_smem_rom_start = LOADADDR(_APP_SMEM_SECTION_NAME);
  	SECTION_DATA_PROLOGUE(_BSS_SECTION_NAME,(NOLOAD),)

• Can you please take a look at similar linker.ld files.
  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.
  Regarding the SoC specific linker.ld files, then but feel free to ping the SoC maintainers and let them evaluate if the same changes should be done there.

[gramsay0]

@tejlmand thanks! I will check userspace and other linker files soon when I find time

[gramsay0]

Small update for Cortex-A/R. There was an attempt to move bss/noinit toward the end of the linker file #60368. This ended up getting reverted due to CI failures. The CI failures were later discovered to be unrelated to this change (there was some undefined behavior in one of the tests that just happened to start failing). This PR was never re-applied

Interestingly, Cortex-A/R does not put bss/noinit in different places depending on CONFIG_USERSPACE (as Cortex-M does). I'll try to catch up with the original reasons for this #21747

[tejlmand]

@gramsay0 I consider updating / aligning with cortex_a_r ld template scripts addressed by @Dat-NguyenDuy in #79502

[Dat-NguyenDuy]

Small update for Cortex-A/R. There was an attempt to move bss/noinit toward the end of the linker file #60368. This ended up getting reverted due to CI failures. The CI failures were later discovered to be unrelated to this change (there was some undefined behavior in one of the tests that just happened to start failing). This PR was never re-applied

Interestingly, Cortex-A/R does not put bss/noinit in different places depending on CONFIG_USERSPACE (as Cortex-M does). I'll try to catch up with the original reasons for this #21747

There was a discussion regarding this topic for cortex-M, but the issue was closed without any actions. I have just re-opened it #24619

[Dat-NguyenDuy]

Hi @gramsay0, have you had time to look into the USERSPACE issue on cortex-M? I have tried with your commits and always put bss/noinit sections to the end of the linker script. Run tests\kernel, tests\lib on an cortex-M7 based board, i see no issue.
Look like the problem describe here: #21747 (comment) is no longer exist.

[gramsay0]

@Dat-NguyenDuy yes I had done some testing and also found no issues.

I think this patch fixed the need for bss to be before the kobject_data section with userspace enabled: #33687

See #21747 (comment) (which I believe is what the above patch does):

The appropriate method may be to reserve space for kobject_data during prelink, but the related process is relatively unfamiliar to me, so it is difficult to modify.

@tejlmand do you agree, and if so should I make that change here in in a follow-up PR?

[tejlmand]

I think this patch fixed the need for bss to be before the kobject_data section with userspace enabled: #33687

yes, that does seem to be the case.

@tejlmand do you agree, and if so should I make that change here in in a follow-up PR?

it does seem that #33687 provides the needed steps to allow us to move bss to the end.
A follow-up PR will be nice and allow us to review the parts individually.

[Dat-NguyenDuy]

Hi @tejlmand and @gramsay0, i'm not sure if there is still work to be done for this PR since i don't see an approve from @tejlmand.

[kartben]

@ithinuel could you please review and potentially be the second +1 here?