Use picolibc instead of newlib-nano

[keith-packard]

 1. Add picolibc bits to linker scripts. Picolibc uses thread local storage
    for unshared data, and the picolibc startup code relies on carefully
    constructed linker script that allocates one TLS block in RAM for
    applications that don't have explicit TLS support. The picolibc
    startup code also uses different symbols than the tf-m startup code.

 2. Support picolibc stdio. There was already picolibc support code
    present in the library for the ARM LLVM toolchain. The
    conditionals which selected it have been changed to use
    __PICOLIBC__ instead of __clang_major__.

 3. Add _exit stub. Code using assert or abort end up calling _exit
    through the picolibc signal handling code.

 4. Switch to picolibc.specs. This is needed for toolchains which
    don't use picolibc by default, and can also be used without
    trouble in toolchains where picolibc is the default.

[keith-packard]

These are (finally) passing Zephyr tests using both SDK 0.17 and SDK 0.18-alpha4 (although each requires some additional changes which are wending their way upstream). I think they're ready for review and help getting them upstream.

[tomi-font]

How did the linker get confused exactly? Is this really needed?

[keith-packard]

. got set to LOADADDR instead of ADDR when building with binutils 2.43. I'm afraid that after spending several hours chasing this down, I didn't dig into the linker code to figure out precisely why that happened.

[keith-packard]

I've re-added this patch as it is necessary even with SDK 0.17.4.

[tomi-font]

Had a chat with upstream about these (picolibc) changes. Let's keep them only in this fork until they're not needed anymore (TF-M is moving away from C library usage), in the next release or the one following it.
But it would be good to have someone who understands this a bit to review the changes. Don't know who that could be. cc @frkv @d3zd3z @ceolin @dleach02 @Vge0rge

[keith-packard]

Had a chat with upstream about these (picolibc) changes. Let's keep them only in this fork until they're not needed anymore (TF-M is moving away from C library usage), in the next release or the one following it. But it would be good to have someone who understands this a bit to review the changes. Don't know who that could be. cc @frkv @d3zd3z @ceolin @dleach02 @Vge0rge

Sounds good. I'll keep running tests as this series moves forward. This is not buildable using SDK 0.17.2 because of a last-minute oops in that release, but it should be buildable with the upcoming SDK 0.17.3.

[frkv]

I recommend @wearyzen and @adeaarm give it a quick look from TF-M's perspective.

The Clang/LLVM toolchain for TF-M was added with no dependency on libc and/or picolib in the core TF-M code, but you still see references to nano.specs in the GNUARM toolchain

Zephyr, making this choice for GNUARM toolchain (this change), will take any penalty related to support, any security advisories etc... But the choice to go to picolibc is an active choice in the Zephyr community, and in extension it will impact any vendor who makes use of standard c library function in PSA crypto and PSA crypto driver build as well as in bootloaders...

With regards to these commits, I think we should consider them to be marked as commits emitted from zephyr integration, unless they are possible to add upstream in the TF-M project... If they are out-of-tree patches required for Zephyr integration, they would need to be maintained across updated versions of TF-M in our fork

[tomi-font]

With regards to these commits, I think we should consider them to be marked as commits emitted from zephyr integration

Yeah this is something we've agreed on and I think just needs to be implemented (documenting the practice basically). In the meantime I don't think we can require that of anyone yet.

[Anton-TF]

True, TF-M plans to remove libc from all secure binaries (BL1, BL2, tfm_s) for Clang/LLVM and GCC toolchains at least.
As part of this effort, we also intend to clean up and streamline the linker scripts.

Additionally, you might want to consider these recent fixes in the scripts:
https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/40014
https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/40068

[keith-packard]

Sounds like any libc-specific changes will be transient then, which will be nice.

[wearyzen]

While trying to build the non-secure MPS4 board with this PR and Zephyr SDK 0.18.0-alpha4 we see below error:

west build -p -b mps4/corstone320/fvp/ns samples/hello_world/ -t run
...
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x30): undefined reference to `__data_source'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x34): undefined reference to `__data_start'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x38): undefined reference to `__bss_size'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x3c): undefined reference to `__bss_start'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x40): undefined reference to `__tls_base'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: /home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/lib/thumb/v8-m.main/nofp/space/libc.a(libc_picolib_machine_arm_set_tls.c.o): in function `_set_tls':
set_tls.c:(.text._set_tls+0xc): undefined reference to `__arm32_tls_tcb_offset'

Referring to the changes in this PR, I tried below patch and it seems to fix the issue:

git diff
diff --git a/bl1/bl1_2/bl1_dummy_rotpk.prv b/bl1/bl1_2/bl1_dummy_rotpk.prv
index bf011cd4d..b6bf9b600 100644
Binary files a/bl1/bl1_2/bl1_dummy_rotpk.prv and b/bl1/bl1_2/bl1_dummy_rotpk.prv differ
diff --git a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld
index 44f04021a..7fad80960 100644
--- a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld
+++ b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld
@@ -160,6 +160,25 @@ SECTIONS
     } > RAM

     bss_size = __bss_end__ - __bss_start__;
+    /* --- add after the .bss section --- */
+    PROVIDE( __bss_start = __bss_start__ );
+    PROVIDE( __bss_end   = __bss_end__ );
+    PROVIDE( __bss_size  = __bss_end - __bss_start );
+
+    PROVIDE( __data_start  = ADDR(.data) );
+    PROVIDE( __data_end    = __data_end__ );
+    PROVIDE( __data_source = LOADADDR(.data) );
+    PROVIDE( __data_size   = __data_end - __data_start );
+
+    /* BL1_1 doesn’t use TLS, but picolibc’s _set_tls needs these present */
+    PROVIDE( __tls_base = 0 );
+    PROVIDE( __tls_align = 8 );
+    PROVIDE( __arm32_tls_tcb_offset = 8 );
+
+    /* --- add after the .heap section (where __HeapBase/Limit are set) --- */
+    PROVIDE( __heap_start = __HeapBase );
+    PROVIDE( __heap_end   = __HeapLimit );
+

 #if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
     .msp_stack (NOLOAD) : ALIGN(32)
diff --git a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld
index 83aaf144f..9151ff3a0 100644
--- a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld
+++ b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld
@@ -162,6 +162,25 @@ SECTIONS

     bss_size = __bss_end__ - __bss_start__;

+    /* --- add after the .bss section --- */
+    PROVIDE( __bss_start = __bss_start__ );
+    PROVIDE( __bss_end   = __bss_end__ );
+    PROVIDE( __bss_size  = __bss_end - __bss_start );
+
+    PROVIDE( __data_start  = ADDR(.data) );
+    PROVIDE( __data_end    = __data_end__ );
+    PROVIDE( __data_source = LOADADDR(.data) );
+    PROVIDE( __data_size   = __data_end - __data_start );
+
+    /* BL1_1 doesn’t use TLS, but picolibc’s _set_tls needs these present */
+    PROVIDE( __tls_base = 0 );
+    PROVIDE( __tls_align = 8 );
+    PROVIDE( __arm32_tls_tcb_offset = 8 );
+
+    /* --- add after the .heap section (where __HeapBase/Limit are set) --- */
+    PROVIDE( __heap_start = __HeapBase );
+    PROVIDE( __heap_end   = __HeapLimit );
+
 #if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
     .msp_stack (NOLOAD) : ALIGN(32)
     {
(END)

I am not a linker script expert but if above change looks good then please let me know if you would like to include it in this PR.
Thanks!

[tomi-font]

@wearyzen could you review this?

[tejlmand]

this will work for Zephyr SDK, but Zephyr SDK is not the only supported toolchain in Zephyr.

This will not work for users using GNU Arm embedded toolchain with newlib enabled.

Shouldn't we be supporting both options ?

[keith-packard]

We could support both options with some configuration mechanism to select between them, but that seems like a lot of added complexity. For users with the GNU Arm embedded toolchain, we could direct them to download the picolibc add-in and use that for this library?

[Anton-TF]

If it helps: TF-M plans to avoid using any standard C library (libc) on the secure side when building with GNU Arm toolchains, similar to what’s already done with Clang. However, BL2 may still require a libc for certain functionality.

[wearyzen]

@wearyzen could you review this?

I don't have expertise on toolchain or linker so hard for me to say if there is an issue with this but I remember from my previous comment this change is needed to use with Zephyr SDK 0.18.0-alpha4.

[tejlmand]

@Vge0rge ptal

[Vge0rge]

I am no linker script expert either here.

But I did cherry picked these changes and tried to build nrfconnect sdk with zehpyr SDK 17.4.0. I tried to build the TF-M PSA architecture tests on nrf5340 and nrf54l15. Even though both could build ok none of them could boot unfortunately. So from Nordic side this needs investigation before it can go in.

[Vge0rge]

NACKing until the issues with the Nordic devices are solved

[keith-packard]

NACKing until the issues with the Nordic devices are solved

How can we fix this? We're running out of time to get SDK 1.0 ready for Zephyr 4.3, and this is the only blocker left.

[Vge0rge]

NACKing until the issues with the Nordic devices are solved

How can we fix this? We're running out of time to get SDK 1.0 ready for Zephyr 4.3, and this is the only blocker left.

@keith-packard I am trying to investigate how to resolve this, I will send updates here by the end of the week.

[etienne-lms]

Sorry for this late feedback. I face some issues testing these changes on the few STM32 Zephyr platforms providing a 'ns' variant that embed TF-M, mainly b_u585i_iot02a and stm32wba65i_dk1 (and almost equiv. nucleo_wba65ri, these STM32WBA65xx based board where recently integrated in Zephyr: zephyrproject-rtos/zephyr#95722):

1. the U585 board fails to boot Zephyr images. TF-M runtime firmware inits fails with message:
   Partition initialization FAILED in 0xc0618af.
2. tfm_regression_test fails to build on all ST platforms: missing changes in the ST linker script:
   platform/ext/target/stm/common/hal/template/gcc/appli_ns.ld
3. the WBA65 boards fails to build: missing changes in its related STM GCC linker script
   ./platform/ext/target/stm/common/stm32wbaxx/Device/Source/gcc/tfm_common_s.ld*

To address point 2, I quickly ported changes based on your equiv. changes. I was able to build and run tfm_regression_test for the U585 boards: it boots them fine and run them successfully.

To address point 3, I also quiclky ported what seem the required changes. I manage to build Zephyr images as well as tfm_regression_test but non boots: TF-M initialization fail at what seem partition initialization. It seems to me it looks like the same issue as for point 1. To be confirmed.

I'll try to understand where the issue for point 1 comes from.

[Vge0rge]

Hello, I spend some time investigating this and I noticed one problem that doesn't make a lot of sense to me.

So I noticed that when I build TF-M with this PR the initialization values of the ITS/PS static objects here:
https://git.trustedfirmware.org/plugins/gitiles/TF-M/trusted-firmware-m.git/+/refs/heads/main/secure_fw/partitions/internal_trusted_storage/tfm_internal_trusted_storage.c#58

Are basically undefined, I am getting the value 0 for some of the fields. I debugged the memory address which holds the invalid fields with a watchpoint and I don't see it getting written by any other code.

I added a workaround in the tfm_its_init function where I set these fields manually:

  fs_cfg_its.flash_dev = &ITS_FLASH_DEV;
  fs_cfg_its.program_unit = ITS_FLASH_ALIGNMENT;
  fs_cfg_its.max_file_size = ITS_UTILS_ALIGN(ITS_MAX_ASSET_SIZE, ITS_FLASH_ALIGNMENT);
  fs_cfg_its.max_num_files = ITS_NUM_ASSETS + 1; /* Extra file for atomic replacement */

#ifdef TFM_PARTITION_PROTECTED_STORAGE
  fs_cfg_ps.flash_dev = &PS_FLASH_DEV;
  fs_cfg_ps.program_unit = PS_FLASH_ALIGNMENT;
  fs_cfg_ps.max_file_size = ITS_UTILS_ALIGN(PS_MAX_OBJECT_SIZE, PS_FLASH_ALIGNMENT);
  fs_cfg_ps.max_num_files = PS_MAX_NUM_OBJECTS;
#endif

And this allowed me to at least boot the PSA arch tests and run some crypto test cases.
I am investigating more at the moment but in the mean time @keith-packard do you have any ideas how the picolibc can affect something like this?

[etienne-lms]

Thanks @Vge0rge, indeed the trick works around the issue I'm facing. Looking at the generated map file, .TFM_PSA_ROT_LINKER_DATA section is between the .TFM_APP_ROT_LINKER_BSS and .TFM_PSA_ROT_LINKER_BSS sections, likely erased at BSS early initialization I guess. This issue is in the GCC linker file. I Moved the former section to between .TFM_DATA and .TFM_TDATA (a bit hacky I agree) to solve the initial issue.

(edited) Second thought, no, my description above does not explain the issue. GCC linker files already located .TFM_APP_ROT_LINKER_DATA between 2 ZI sections and there were no issues. Those ZI sections are properly listed in the __zero_table_start__/__zero_table_end__ area. Root cause is elsewhere...

[keith-packard]

And this allowed me to at least boot the PSA arch tests and run some crypto test cases. I am investigating more at the moment but in the mean time @keith-packard do you have any ideas how the picolibc can affect something like this?

It could be thread local storage (TLS) stuff. The picolibc changes attempt to stuff a static TLS segment (both .tdata and .tbss) in between the usual data-initialized memory area (.data) and zero-initialized memory area (.bss). If any of the symbols that define how those segments are arranged in memory get messed up, memory will not be correctly initialized.

I admit that this is not how thread local storage is usually managed; it's a "clever hack" which avoids needing any additional code to perform that initialization, aside from setting the TLS base pointer.

It could also be other symbols used by picolibc's crt0 for memory initialization aren't getting set correctly.

If you have a linker map file from a failing build, please post it and we can look at the various symbol values and see what's wrong.

If it's the TLS stuff, I'm wondering if we can just assert that TF-M doesn't use any C library functions which need thread local storage -- the most obvious use is for errno, but there are a few others used by APIs which are required to be re-entrant and which are also required to hold persistent data.

[Vge0rge]

@keith-packard I attached here the linker for when building the TF-M regression tests for nRF54L15.

nrf54l_tfm_s.txt

The symbols which I confirmed that they are not initialized correctly are:

• fs_cfg_its
• fs_cfg_ps
• nv_increment_status

There are probably more, I just found these until now. But these are normal global static variables which are placed in .data as expected. So maybe it is indeed crt related and not TLS after all?

EDIT I did some more digging and it seems that the problem is not the linker script at all. The problem is simpler, the function picolibc_startup is never called for me. So the __copy_table_t which is initialized in this function and sets the values of the global initialized objects is never actually copied.

I confirmed that by manually implementing the function inside the ResetHandler and then I could run and pass the TF-M regression tests.

This was introduced with this PR because the cmsis header changes the __PROGRAM_START define based on the picolibc:
https://github.com/zephyrproject-rtos/CMSIS_6/blob/30a859f44ef8ab4dc8f84b03ed586fd16ccf9d74/CMSIS/Core/Include/m-profile/cmsis_gcc_m.h#L33

So before this PR the define was pointing to the cmsis_start in the same file which copies the table but now that it is not used the global data are not initialized.

EDIT2 I am wondering if this is because the TF-M main function belongs to a separate target (tfm_spm) compared to the picolib_startup source file. So since there is no main function in the target that the picolib_startup belongs to the constructor attribute does nothing?

I did more checks with 2 Nordic devices, I run the TF-M tests and some TF-M applications and all work fine as long as I include the logic of the picolib_startup in the reset handler. So Keith please give a proposal on how to resolve the issue and we can move forward with this from Nordics side.