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

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

Merged
merged 1 commit into from Oct 24, 2019

Conversation

@kjbracey-arm
Copy link
Contributor

kjbracey-arm commented Oct 16, 2019

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 #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

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:armstack branch from b41fb09 to c91b928 Oct 16, 2019
@ciarmcom ciarmcom requested review from andrewc-arm, ashok-rao, bentcooke, bulislaw, JuhPuur, maclobdell and ARMmbed/mbed-os-maintainers Oct 16, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 16, 2019

@andrewc-arm

This comment has been minimized.

Copy link
Contributor

andrewc-arm commented Oct 18, 2019

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

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

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-arm

This comment has been minimized.

Copy link
Contributor Author

kjbracey-arm commented Oct 21, 2019

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-arm

This comment has been minimized.

Copy link
Contributor Author

kjbracey-arm commented Oct 21, 2019

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

This comment has been minimized.

Copy link

mbed-ci commented Oct 21, 2019

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
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:armstack branch from c91b928 to ef3452e Oct 21, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 21, 2019

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

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 21, 2019

CI restarted

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

kjbracey-arm commented Oct 21, 2019

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

This comment has been minimized.

Copy link

mbed-ci commented Oct 21, 2019

Test run: SUCCESS

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

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:armstack branch from ef3452e to 4bdb09e Oct 21, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

kjbracey-arm commented Oct 21, 2019

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 0xc0170 requested review from ARMmbed/team-silabs Oct 21, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 21, 2019

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.

Copy link
Contributor

bentcooke left a comment

This looks good as long as it gets documented.

@@ -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

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Oct 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 22, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 22, 2019

Author Contributor

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...

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Oct 22, 2019

Contributor

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.

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Oct 22, 2019

Contributor

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 23, 2019

Author Contributor

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 #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.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:armstack branch from 4bdb09e to fb6aa3e Oct 23, 2019
Copy link
Contributor

mmahadevan108 left a comment

Thank you

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 23, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 23, 2019

Test run: SUCCESS

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

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 24, 2019

@kjbracey-arm Ready for integration?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

kjbracey-arm commented Oct 24, 2019

Ready for integration?

I guess so.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 24, 2019
@0xc0170 0xc0170 merged commit 8637069 into ARMmbed:master Oct 24, 2019
27 checks passed
27 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
jenkins-ci/wisun-mesh-test Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8676 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 24, 2019

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? 👀

kjbracey-arm added a commit to kjbracey-arm/mbed-os-5-docs that referenced this pull request Oct 24, 2019
Add some notes corresponding to clean-up in
ARMmbed/mbed-os#11698.
@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:armstack branch Oct 24, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

kjbracey-arm commented Oct 24, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.