link.x.in: put most __[se] symbols back into sections #323
Conversation
This puts most start/end address symbols back into the sections. Only `__ebss` and `__edata` are kept outside their sections so that potential user code with external libraries can inject stuff using `INSERT AFTER .bss/.data` and profit from the .bss/.data zeroing/loading mechanism. This also leads to the `__sbss` and `__veneer_base` symbols having the right section type (B not D in nm). Also the trust zone start and end address are aligned to 32 bytes as per the requirements. That section does cost up to 28 byte of FLASH due to that alignment even if empty. The .rodata start is kep free for the linker to alocate it after .text. This enables users to inject sections between .text and .rodata and removes the chance to get overlapping address errors. With this the linker will by default place .rodata after .text as before. This commit also adds and exposes a few more stable address start/end symbols (__[se]uninit, __stext, __srodata) that are usefull for debugging and hooking into. See rust-embedded#287 (comment) for discussion of the issues and description of this compromise solution.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonas-schievink (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Could we place the trustzone section after .data to prevent this extra flash usage? |
That would still effectively pad the flash usage to 32 byte which sound benign. |
|
@mattico / @Dirbaio I think that your use cases should still be supported with these changes, right?
Yea, fair point. I think these came from @hug-dev in #297, and looking back on it I also saw they said "I will update the PR as discussed but would still like to keep the __veneer_limit symbol unaligned. It could be used as a way to count the number of entry functions declared in all the dependency graph as well", which I think this would inadvertently be undoing. Has anyone used this section? Could it just be added by TrustZone users in their memory.x as required, with INSERT AFTER .data and including whatever alignment is required, without it needing to be in our main link.x? |
|
Very annoying to test this when the crates-io version is ahead of the git version... But yes, this does still work with the project that originally motivated my changes. |
I actually removed that in the end in the original PR because this was not working for some reason. So it would be fine that way!
I have a local TrustZone-M example I am happy to try with this PR by injecting the veneers sections manually. I guess I would do INSERT AFTER .text though? So that the veneers are in a read-only section by default? As far as I know, it is common for the veneers to be in flash but it ultimately depends on the Implementation Defined Attribution Unit (IDAU) on the particular platform to know where they can be put. See here for other details. |
From what I read (in the links above) they should indeed be in flash, not in the "default" flash alias region (which is S), but in the NSC alias region. |
|
I tried on my example and that PR works without any modification. I then removed the veneers section from it: diff --git a/link.x.in b/link.x.in
index 92004b7..95a8a14 100644
--- a/link.x.in
+++ b/link.x.in
@@ -132,19 +132,6 @@ SECTIONS
/* LMA of .data */
__sidata = LOADADDR(.data);
- /* ### .gnu.sgstubs
- This section contains the TrustZone-M veneers put there by the Arm GNU linker. */
- /* Security Attribution Unit blocks must be 32 bytes aligned. */
- /* Note that this pads the FLASH usage to 32 byte alignment. */
- .gnu.sgstubs : ALIGN(32)
- {
- . = ALIGN(32);
- __veneer_base = .;
- *(.gnu.sgstubs*)
- . = ALIGN(32);
- __veneer_limit = .;
- } > FLASH
-
/* ### .bss */
.bss (NOLOAD) : ALIGN(4)
{and added in but I have the error when compiling: Same error if I do |
In fact, that error even happens with an empty n.b. The |
Huh, that's no good. I'm fairly sure this previously worked... we should add some CI tests for the various What do you think is the best course of action given this, then? Go ahead with this PR as-is? One related thought I've had (which needn't block this PR) is we can use build.rs to customise the default linker script -- for example only adding the secure veneers on v8 targets (or even just including a different link.x.in for v8 targets). That would let us get around the extra flash usage if it's a problem. |
This was tested in three cases now (I think). Maybe try to get at least two more testers on board? But then yes, I'd propose merging. CI for several specific memory organization cases and more dynamic link.x.in would be excellent next steps after this one. |
Agree! Maybe it's also possible to put the veneer sections (or others) in a As far as I am concerned, I would be ok to merge this since it works on Armv8-M 👌 |
323: link.x.in: put most __[se] symbols back into sections r=adamgreig a=jordens This puts most start/end address symbols back into the sections. Only `__ebss` and `__edata` are kept outside their sections so that potential user code with external libraries can inject stuff using `INSERT AFTER .bss/.data` and profit from the .bss/.data zeroing/loading mechanism. This also leads to the `__sbss` and `__veneer_base` symbols having the right section type (bss is B not D in nm). Also the trust zone start and end address are aligned to 32 bytes as per the requirements. That section does cost up to 28 byte of padding at the end of FLASH due to that alignment even if empty. But since flash is typically a multiple of 32 bytes and since the padding is at the end, there is no downside. The .rodata start is kept free for the linker to allocate it after .text. This enables users to inject sections between .text and .rodata and removes the chance to get overlapping address errors. With this the linker will by default place .rodata after .text as before. This commit also adds and exposes a few more stable address start/end symbols (__[se]uninit, __stext, __srodata) that are usefull for debugging and hooking into. See rust-embedded/cortex-m-rt#287 (comment) for discussion of the issues and description of this compromise solution. Tested: * [x] [stm32h7 ITCM](quartiq/stabilizer#322) * [x] [sgstubs](rust-embedded/cortex-m-rt#323 (comment)) * [x] `INSERT AFTER` for bss/data still works (rust-embedded/cortex-m-rt#323 (comment)) Topics: * [x] `sgstubs` moved to be the last section in FLASH to minimize the impact of the 32 byte alignment. (Padding flash to 32 byte is considered benign.) * [ ] `INSERT AFTER` with binutils ld doesn't work. But that's independent of these changes. This is the `sgstubs`-in `memory.x` use case. Currently the `sgstubs` section is kept in `link.x`. Co-authored-by: Robert Jördens <rj@quartiq.de>
This puts most start/end address symbols back into the sections.
Only
__ebssand__edataare kept outside their sections so thatpotential user code with external libraries can inject stuff using
INSERT AFTER .bss/.dataand profit from the .bss/.data zeroing/loadingmechanism. This also leads to the
__sbssand__veneer_basesymbolshaving the right section type (bss is B not D in nm).
Also the trust zone start and end address are aligned to 32 bytes as per
the requirements. That section does cost up to 28 byte of padding at the end of FLASH due to
that alignment even if empty. But since flash is typically a multiple of 32 bytes and since the padding is at the end, there is no downside.
The .rodata start is kept free for the linker to allocate it after .text.
This enables users to inject sections between .text and .rodata and
removes the chance to get overlapping address errors. With this the
linker will by default place .rodata after .text as before.
This commit also adds and exposes a few more stable address start/end symbols
(__[se]uninit, __stext, __srodata) that are usefull for debugging and hooking
into.
See
#287 (comment)
for discussion of the issues and description of this compromise solution.
Tested:
INSERT AFTERfor bss/data still works (link.x.in: put most __[se] symbols back into sections #323 (comment))Topics:
sgstubsmoved to be the last section in FLASH to minimize the impact of the 32 byte alignment. (Padding flash to 32 byte is considered benign.)INSERT AFTERwith binutils ld doesn't work. But that's independent of these changes. This is thesgstubs-inmemory.xuse case. Currently thesgstubssection is kept inlink.x.