Clean up ARM toolchain heap+stack setup in targets #11698

kjbracey · 2019-10-16T13:36:56Z

Description

ARM Compiler 6.13 testing revealed linker errors pointing out conflicting use of __user_setup_stackheap and __user_initial_stackheap in some targets. Remove the unwanted __user_initial_stackheap from the targets - the setup is centralised in the common platform code.

Looking into this, a number of other issues were highlighted

Almost all targets had __initial_sp hardcoded in assembler, rather than getting it from the scatter file. This was behind issue Move RAM partition with ARM compiler #11313. Fix this generally.
A few targets' __initial_sp values did not match the scatter file layout, in some cases meaning they were overlapping heap space. They now all use the area reserved in the scatter file. If any problems are seen, then there is an error in the scatter file.
A number of targets were reserving unneeded space for heap and stack in their startup assembler, on top of the space reserved in the scatter file, so wasting a few K. A couple were using that space for the stack, rather than the space in the scatter file.

To clarify expected behaviour:

Each scatter file contains empty regions ARM_LIB_HEAP and ARM_LIB_STACK to reserve space. ARM_LIB_STACK is sized by the macro MBED_BOOT_STACK_SIZE, which is set by the tools. ARM_LIB_HEAP is generally the space left over after static data and stack.
The address of the end of ARM_LIB_STACK is written into the vector table and on reset the CPU sets MSP to that address.
The common platform code in Mbed OS provides __user_setup_stackheap for the ARM library. The ARM library calls this during startup, and it calls __mbed_user_setup_stackheap.
The default weak definition of __mbed_user_setup_stackheap does not modify SP, so we remain on the boot stack, and the heap is set to the region described by ARM_LIB_HEAP. If ARM_LIB_HEAP doesn't exist, then the heap is the space from the end of the used data in RW_IRAM1 to the start of ARM_LIB_STACK.
Targets can override __mbed_user_setup_stackheap if they want. Currently only Renesas (ARMv7-A class) devices do.
If microlib is in use, then it doesn't call __user_setup_stackheap. Instead it just finds and uses ARM_LIB_STACK and ARM_LIB_HEAP itself.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@JuhPuur, @bulislaw

ciarmcom · 2019-10-16T15:00:36Z

@kjbracey-arm, thank you for your changes.
@bentcooke @maclobdell @ashok-rao @bulislaw @andrewc-arm @JuhPuur @ARMmbed/mbed-os-maintainers please review.

andrewc-arm · 2019-10-18T06:46:10Z

Hi, @kjbracey-arm
I want to make sure I understand the code change correctly. Instead of using the hard configured __initial_sp, we want to use the output of |Image$$ARM_LIB_STACK$$ZI$$Limit|. Right?
Any chance you can provide us on how to reproduce the problem if this is not done?

0xc0170 · 2019-10-18T09:19:06Z

I want to make sure I understand the code change correctly. Instead of using the hard configured __initial_sp, we want to use the output of |Image$$ARM_LIB_STACK$$ZI$$Limit|. Right?

Is this worth documenting anywhere? Files changed here are coming from vendors and most likely will contain initial_sp used and defined __user_initial_stackheap . For any new target coming our way we need to check if t his is properly defined as in this PR.

kjbracey · 2019-10-21T06:32:45Z

These changes are self-contained within the targets themselves. Nothing is being changed in the "target -> Mbed OS" interface.

It's just that almost all the targets have copy-and-pasted the same basic pattern - reserving a region for the stack in their scatter file, but setting the initial stack pointer manually with a separate define in their startup assembler file.

This allows inconsistencies, and confusion, as we saw in #11313. So all these targets are being changed (internally) so their assembler file gets the initial stack pointer for the vector table from their scatter file.

This is what GCC and IAR versions for each target (and a couple of ARM targets) are already doing.

I've no idea why the ARM versions were never doing this - possibly someone struggled with the || escaping or the need for the IMPORT directive or the need for the EMPTY in the scatter file.

I don't think the previous behaviour with __initial_sp was documented, unless there's an example or reference port somewhere out of tree?

kjbracey · 2019-10-21T06:39:43Z

Is this worth documenting anywhere? Files changed here are coming from vendors and most likely will contain initial_sp used and defined __user_initial_stackheap . For any new target coming our way we need to check if t his is properly defined as in this PR.

There is a chance we get old-style imports for a bit, if they've been working on an out-of-tree port for a while. If they have __user_initial_stackheap (rare), ARM C 6.13 will complain.

For __initial_sp, it's just a quality issue - hard-coding the initial SP isn't wrong it's just ugly. We should just keep an eye out for the pattern. I think that pattern is just coming from cut-and-paste from existing targets, but if you can see any documentation references, we need to fix them.

mbed-ci · 2019-10-21T07:20:23Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM

0xc0170 · 2019-10-21T08:13:35Z

Understood. I recall from porting targets, these files are "copy-paste" from cmsis-packs or they are included in 3rd party drivers (they include cmsis startup files,etc). The expected behavior from above should be captured somewhere. Related symbols are here : https://os.mbed.com/docs/mbed-os/v5.14/porting/porting-bootstrap.html , can we add this expected behavior there?

0xc0170 · 2019-10-21T08:14:07Z

CI restarted

kjbracey · 2019-10-21T08:22:53Z

Okay, I can see the ARM template in http://www.keil.com/pack/doc/CMSIS/Core/html/startup_s_pg.html - there's no GCC/IAR version there, so maybe ARM has ended up a bit wonky because they're taking a CMSIS template for that, and an Mbed OS template for the others.

I guess we could add a paragraph about the layout to the "Startup" section in https://os.mbed.com/docs/mbed-os/v5.14/porting/porting-bootstrap.html. The following "Linker script" section looks good and accurate, but the startup should note that the stack and heap info should come from the linker script, rather than being built-in to the assembler like the linked CMSIS example.

mbed-ci · 2019-10-21T09:25:30Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 2
Build artifacts

kjbracey · 2019-10-21T10:40:02Z

Had to make a couple of tweaks - one target missed due to failure to save in the editor, and there was a new startup file (startup_psoc6_02_cm4.S) that needed doing.

Thanks for that passed CI run - seems like it's basically sound. Should get a round of PR review from all the vendors...

0xc0170 · 2019-10-21T11:27:11Z

Thanks for that passed CI run - seems like it's basically sound. Should get a round of PR review from all the vendors...

Teams requested for review.

bentcooke

This looks good as long as it gets documented.

mmahadevan108 · 2019-10-21T19:04:02Z

...resso_MCUS/TARGET_LPC55S69/TARGET_M33_S/device/TOOLCHAIN_ARMC6/startup_LPC55S69_cm33_core0.S

@@ -157,9 +134,6 @@ Reset_Handler   PROC
                ;MSR     MSPLIM, R0
                LDR     R0, =SystemInit
                BLX     R0
-                MRS     R0, control    ; Get control value
-                ORR     R0, R0, #2     ; Select switch to PSP, which will be set by __user_initial_stackheap
-                MSR     control, R0


Why is this code deleted. From the documentation this bit selects the stack pointer that is used.

It's not clear to me what it was trying to do.

I was baulking at that operation being illegal - ARMv7-M says setting the SPSEL bit in Handler mode is reserved. But I see ARMv8-M says it's just ignored.

Still, I don't see what you're trying to achieve here. Any switch to Thread Mode + PSP will occur in the RTOS, and it will be dealing with SPSEL. I would expect all target startup code to remain in Handler mode + MSP.

Is there something I'm missing about the startup code for the secure side here?

Okay, been staring at TF-M main repo for a while. I can see those lines are in its startup assembler files. A couple of them follow it by an attempt to set SP to the top of ARM_LIB_STACK, which seems erroneous, as we're in Handler mode, so we are not switching to PSP now.

I can't find CONTROL_S.SPSEL being manipulated anywhere else in the TF-M source, so I think the effect of those lines may be to make the secure side run with separate Process stack, when eventually a Thread Mode secure->non-secure gateway call happens. The PSP itself is actually set up elsewhere (tfm_spm_db_init?)

Not clear why this is placed in target-specific startup, rather than that tfm_spm_db_init, as it doesn't do anything immediate - maybe it's a remnant from that old failed attempt to initialise PSP in target startup code?

I can put it back, but I'd like to understand a bit better. That reference to __user_initial_stackheap is wrong...

I have received this code from NXP TFM team. I checked with the TFM team, they said this implementation is based on the reference received for the MUSCA platform.

We can change the comment to "/* Select switch to PSP */", I agree that is wrong and it has been corrected in the updated version of this file

I've put the code back, adjusting just the comment.

Issue raised over on TF-M to query what's going on here:

https://developer.trustedfirmware.org/T565

ARM Compiler 6.13 testing revealed linker errors pointing out conflicting use of `__user_setup_stackheap` and `__user_initial_stackheap` in some targets. Remove the unwanted `__user_initial_stackheap` from the targets - the setup is centralised in the common platform code. Looking into this, a number of other issues were highlighted * Almost all targets had `__initial_sp` hardcoded in assembler, rather than getting it from the scatter file. This was behind issue ARMmbed#11313. Fix this generally. * A few targets' `__initial_sp` values did not match the scatter file layout, in some cases meaning they were overlapping heap space. They now all use the area reserved in the scatter file. If any problems are seen, then there is an error in the scatter file. * A number of targets were reserving unneeded space for heap and stack in their startup assembler, on top of the space reserved in the scatter file, so wasting a few K. A couple were using that space for the stack, rather than the space in the scatter file. To clarify expected behaviour: * Each scatter file contains empty regions `ARM_LIB_HEAP` and `ARM_LIB_STACK` to reserve space. `ARM_LIB_STACK` is sized by the macro `MBED_BOOT_STACK_SIZE`, which is set by the tools. `ARM_LIB_HEAP` is generally the space left over after static RAM and stack. * The address of the end of `ARM_LIB_STACK` is written into the vector table and on reset the CPU sets MSP to that address. * The common platform code in Mbed OS provides `__user_setup_stackheap` for the ARM library. The ARM library calls this during startup, and it calls `__mbed_user_setup_stackheap`. * The default weak definition of `__mbed_user_setup_stackheap` does not modify SP, so we remain on the boot stack, and the heap is set to the region described by `ARM_LIB_HEAP`. If `ARM_LIB_HEAP` doesn't exist, then the heap is the space from the end of the used data in `RW_IRAM1` to the start of `ARM_LIB_STACK`. * Targets can override `__mbed_user_setup_stackheap` if they want. Currently only Renesas (ARMv7-A class) devices do. * If microlib is in use, then it doesn't call `__user_setup_stackheap`. Instead it just finds and uses `ARM_LIB_STACK` and `ARM_LIB_HEAP` itself.

mmahadevan108

Thank you

0xc0170 · 2019-10-23T12:59:14Z

CI started

mbed-ci · 2019-10-23T16:47:57Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 3
Build artifacts

0xc0170 · 2019-10-24T07:24:42Z

@kjbracey-arm Ready for integration?

kjbracey · 2019-10-24T09:10:52Z

Ready for integration?

I guess so.

0xc0170 · 2019-10-24T09:38:30Z

Okay, I can see the ARM template in http://www.keil.com/pack/doc/CMSIS/Core/html/startup_s_pg.html - there's no GCC/IAR version there, so maybe ARM has ended up a bit wonky because they're taking a CMSIS template for that, and an Mbed OS template for the others.

I guess we could add a paragraph about the layout to the "Startup" section in https://os.mbed.com/docs/mbed-os/v5.14/porting/porting-bootstrap.html. The following "Linker script" section looks good and accurate, but the startup should note that the stack and heap info should come from the linker script, rather than being built-in to the assembler like the linked CMSIS example.

@kjbracey-arm This one has been resolved? I can't find the PR in docs? 👀

Add some notes corresponding to clean-up in ARMmbed/mbed-os#11698.

kjbracey · 2019-10-24T12:27:16Z

Created a docs PR: ARMmbed/mbed-os-5-docs#1157

kjbracey force-pushed the armstack branch from b41fb09 to c91b928 Compare October 16, 2019 14:03

ciarmcom requested review from andrewc-arm, ashok-rao, bentcooke, bulislaw, JuhPuur, maclobdell and a team October 16, 2019 15:00

ciarmcom added the needs: review label Oct 16, 2019

kjbracey force-pushed the armstack branch from c91b928 to ef3452e Compare October 21, 2019 07:34

0xc0170 approved these changes Oct 21, 2019

View reviewed changes

kjbracey force-pushed the armstack branch from ef3452e to 4bdb09e Compare October 21, 2019 10:37

0xc0170 requested review from a team October 21, 2019 11:26

jeromecoutant approved these changes Oct 21, 2019

View reviewed changes

bentcooke approved these changes Oct 21, 2019

View reviewed changes

morser499 approved these changes Oct 21, 2019

View reviewed changes

mmahadevan108 reviewed Oct 21, 2019

View reviewed changes

kjbracey force-pushed the armstack branch from 4bdb09e to fb6aa3e Compare October 23, 2019 11:54

mmahadevan108 approved these changes Oct 23, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.14.2 and removed needs: review labels Oct 23, 2019

andrewc-arm approved these changes Oct 24, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Oct 24, 2019

0xc0170 merged commit 8637069 into ARMmbed:master Oct 24, 2019

0xc0170 removed the ready for merge label Oct 24, 2019

kjbracey added a commit to kjbracey/mbed-os-5-docs that referenced this pull request Oct 24, 2019

Porting bootstrap heap/stack notes

671438c

Add some notes corresponding to clean-up in ARMmbed/mbed-os#11698.

kjbracey mentioned this pull request Oct 24, 2019

Porting bootstrap heap/stack notes ARMmbed/mbed-os-5-docs#1157

Merged

kjbracey deleted the armstack branch October 24, 2019 12:27

Clean up ARM toolchain heap+stack setup in targets #11698

Clean up ARM toolchain heap+stack setup in targets #11698

Uh oh!

Conversation

kjbracey commented Oct 16, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Oct 16, 2019

Uh oh!

andrewc-arm commented Oct 18, 2019

Uh oh!

0xc0170 commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Oct 21, 2019

Uh oh!

mbed-ci commented Oct 21, 2019

Uh oh!

0xc0170 commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

kjbracey commented Oct 21, 2019

Uh oh!

mbed-ci commented Oct 21, 2019

Uh oh!

kjbracey commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

bentcooke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 23, 2019

Uh oh!

mbed-ci commented Oct 23, 2019

Uh oh!

0xc0170 commented Oct 24, 2019

Uh oh!

kjbracey commented Oct 24, 2019

Uh oh!

0xc0170 commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Oct 24, 2019

Uh oh!

Uh oh!

0xc0170 commented Oct 18, 2019 •

edited

Loading

kjbracey commented Oct 21, 2019 •

edited

Loading

0xc0170 commented Oct 21, 2019 •

edited

Loading

kjbracey commented Oct 21, 2019 •

edited

Loading

0xc0170 commented Oct 24, 2019 •

edited

Loading