| Message ID | 20240313162324.2117909-3-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean up arm linker scripts | expand |
On 3/13/24 06:23, Ilias Apalodimas wrote: > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > @@ -63,18 +63,11 @@ SECTIONS > > _image_binary_end = .; > > - .bss_start (NOLOAD) : { > - . = ALIGN(8); > - KEEP(*(.__bss_start)); > - } >.sdram > - > - .bss (NOLOAD) : { > + .bss : { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } >.sdram > - > - .bss_end (NOLOAD) : { > - KEEP(*(.__bss_end)); > + . = ALIGN(8); > + __bss_end = .; Still missing the alignment on .bss, previously in .bss_start. With that fixed, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi Richard, On Wed, 13 Mar 2024 at 22:19, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/13/24 06:23, Ilias Apalodimas wrote: > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > @@ -63,18 +63,11 @@ SECTIONS > > > > _image_binary_end = .; > > > > - .bss_start (NOLOAD) : { > > - . = ALIGN(8); > > - KEEP(*(.__bss_start)); > > - } >.sdram > > - > > - .bss (NOLOAD) : { > > + .bss : { > > + __bss_start = .; > > *(.bss*) > > - . = ALIGN(8); > > - } >.sdram > > - > > - .bss_end (NOLOAD) : { > > - KEEP(*(.__bss_end)); > > + . = ALIGN(8); > > + __bss_end = .; > > Still missing the alignment on .bss, previously in .bss_start. Since this is emitted in .sdram memory I can't define it as .bss ALIGN(8) : {....} since the calculated memory will be outside the sdram-defined region I could define it as .bss : { . = ALIGN(8); __bss_start = .; ...... } But instead, I added an assert at the bottom which will break the linking if the __bss_start is not 8byte aligned. Looking at the output for xilinx_zynqmp_kria_defconfig (which is a v8 board & SPL) looks identical and correct since CONFIG_SPL_BSS_START_ADDR=0x1000 for that board. $~ readelf -sW spl/u-boot-spl | grep bss_start 1550: 0000000000001000 0 NOTYPE GLOBAL DEFAULT 6 __bss_start Isn't the assert enough? Or do you think adding the . = ALIGN(8) in the section definition is better? Thanks /Ilias > > With that fixed, > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > r~
On 3/13/24 11:43, Ilias Apalodimas wrote: > Hi Richard, > > On Wed, 13 Mar 2024 at 22:19, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 3/13/24 06:23, Ilias Apalodimas wrote: >>> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds >>> @@ -63,18 +63,11 @@ SECTIONS >>> >>> _image_binary_end = .; >>> >>> - .bss_start (NOLOAD) : { >>> - . = ALIGN(8); >>> - KEEP(*(.__bss_start)); >>> - } >.sdram >>> - >>> - .bss (NOLOAD) : { >>> + .bss : { >>> + __bss_start = .; >>> *(.bss*) >>> - . = ALIGN(8); >>> - } >.sdram >>> - >>> - .bss_end (NOLOAD) : { >>> - KEEP(*(.__bss_end)); >>> + . = ALIGN(8); >>> + __bss_end = .; >> >> Still missing the alignment on .bss, previously in .bss_start. > > Since this is emitted in .sdram memory I can't define it as > .bss ALIGN(8) : {....} since the calculated memory will be outside the > sdram-defined region > > I could define it as > .bss : { > . = ALIGN(8); > __bss_start = .; > ...... > } > > But instead, I added an assert at the bottom which will break the > linking if the __bss_start is not 8byte aligned. I think it would be clearer to assert on __bss_start address rather than CONFIG_SPL_BSS_START_ADDR, if that's possible. If not, then the constant will have to do. r~
On 3/13/24 10:23, Ilias Apalodimas wrote: > commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated") > and > commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts") > were moving the bss_start/end on c generated variables that were > injected in their own sections. The reason was that we needed relative > relocations for position independent code and linker bugs back then > prevented us from doing so [0]. > > However, the linker documentation pages states that symbols that are > defined within a section definition will create a relocatable type with > the value being a fixed offset from the base of a section [1]. > So let's start cleaning this up starting with the bss_start and bss_end > variables. Convert them into symbols within the .bss section definition. > > [0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object") > [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html > > Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845 > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/arm/cpu/armv8/u-boot-spl.lds | 18 +++++++----------- > arch/arm/cpu/armv8/u-boot.lds | 16 ++++------------ > arch/arm/cpu/u-boot.lds | 22 +++++++--------------- > arch/arm/lib/sections.c | 2 -- > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 15 ++++----------- > arch/arm/mach-zynq/u-boot.lds | 22 +++++++--------------- > 6 files changed, 29 insertions(+), 66 deletions(-) > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds > index 7cb9d731246d..692414fe46fb 100644 > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > @@ -63,18 +63,11 @@ SECTIONS > > _image_binary_end = .; > > - .bss_start (NOLOAD) : { > - . = ALIGN(8); > - KEEP(*(.__bss_start)); > - } >.sdram > - > - .bss (NOLOAD) : { > + .bss : { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } >.sdram > - > - .bss_end (NOLOAD) : { > - KEEP(*(.__bss_end)); > + . = ALIGN(8); > + __bss_end = .; > } >.sdram > > /DISCARD/ : { *(.rela*) } > @@ -89,3 +82,6 @@ SECTIONS > #include "linux-kernel-image-header-vars.h" > #endif > } > +ASSERT(CONFIG_SPL_BSS_START_ADDR % 8 == 0, \ > + "CONFIG_SPL_BSS_START_ADDR must be 8-byte aligned"); > + Git complains about this added blank line at the end of the file. (My personal preference would be a blank line before the ASSERT, if the ASSERT is truly necessary.) But beyond that: Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical Still really excited for this to land! I'm going to have to blow the dust off of my Clang/LLD support series here soon. :) Best, Sam > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds > index fb6a30c922f7..9640cc7a04b8 100644 > --- a/arch/arm/cpu/armv8/u-boot.lds > +++ b/arch/arm/cpu/armv8/u-boot.lds > @@ -149,19 +149,11 @@ SECTIONS > > _end = .; > > - . = ALIGN(8); > - > - .bss_start : { > - KEEP(*(.__bss_start)); > - } > - > - .bss : { > + .bss ALIGN(8): { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } > - > - .bss_end : { > - KEEP(*(.__bss_end)); > + . = ALIGN(8); > + __bss_end = .; > } > > /DISCARD/ : { *(.dynsym) } > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > index 7724c9332c3b..0dfe5f633b16 100644 > --- a/arch/arm/cpu/u-boot.lds > +++ b/arch/arm/cpu/u-boot.lds > @@ -207,23 +207,15 @@ SECTIONS > } > > /* > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > - * __bss_base and __bss_limit are for linker only (overlay ordering) > + * These sections occupy the same memory, but their lifetimes do > + * not overlap: U-Boot initializes .bss only after applying dynamic > + * relocations and therefore after it doesn't need .rel.dyn any more. > */ > - > - .bss_start __rel_dyn_start (OVERLAY) : { > - KEEP(*(.__bss_start)); > - __bss_base = .; > - } > - > - .bss __bss_base (OVERLAY) : { > + .bss ADDR(.rel.dyn) (OVERLAY): { > + __bss_start = .; > *(.bss*) > - . = ALIGN(4); > - __bss_limit = .; > - } > - > - .bss_end __bss_limit (OVERLAY) : { > - KEEP(*(.__bss_end)); > + . = ALIGN(4); > + __bss_end = .; > } > > .dynsym _image_binary_end : { *(.dynsym) } > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c > index 857879711c6a..8e8bd5797e16 100644 > --- a/arch/arm/lib/sections.c > +++ b/arch/arm/lib/sections.c > @@ -19,8 +19,6 @@ > * aliasing warnings. > */ > > -char __bss_start[0] __section(".__bss_start"); > -char __bss_end[0] __section(".__bss_end"); > char __image_copy_start[0] __section(".__image_copy_start"); > char __image_copy_end[0] __section(".__image_copy_end"); > char __rel_dyn_start[0] __section(".__rel_dyn_start"); > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > index 74618eba591b..712c485d4d0b 100644 > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > @@ -56,18 +56,11 @@ SECTIONS > > _image_binary_end = .; > > - .bss_start (NOLOAD) : { > - . = ALIGN(8); > - KEEP(*(.__bss_start)); > - } > - > - .bss (NOLOAD) : { > + .bss ALIGN(8) : { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } > - > - .bss_end (NOLOAD) : { > - KEEP(*(.__bss_end)); > + . = ALIGN(8); > + __bss_end = .; > } > > /DISCARD/ : { *(.dynsym) } > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds > index 3b7c9d515f8b..3c5008b57392 100644 > --- a/arch/arm/mach-zynq/u-boot.lds > +++ b/arch/arm/mach-zynq/u-boot.lds > @@ -103,23 +103,15 @@ SECTIONS > _image_binary_end = .; > > /* > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > - * __bss_base and __bss_limit are for linker only (overlay ordering) > + * These sections occupy the same memory, but their lifetimes do > + * not overlap: U-Boot initializes .bss only after applying dynamic > + * relocations and therefore after it doesn't need .rel.dyn any more. > */ > - > - .bss_start __rel_dyn_start (OVERLAY) : { > - KEEP(*(.__bss_start)); > - __bss_base = .; > - } > - > - .bss __bss_base (OVERLAY) : { > + .bss ADDR(.rel.dyn) (OVERLAY): { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - __bss_limit = .; > - } > - > - .bss_end __bss_limit (OVERLAY) : { > - KEEP(*(.__bss_end)); > + . = ALIGN(8); > + __bss_end = .; > } > > /*
On Wed, Mar 13, 2024 at 04:57:57PM -0600, Sam Edwards wrote: [snip] > Still really excited for this to land! I'm going to have to blow the dust > off of my Clang/LLD support series here soon. :) Please tell me that includes updates to the Clang support in general copied over from the Linux kernel :)
Hi Richard, On Thu, 14 Mar 2024 at 00:43, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/13/24 11:43, Ilias Apalodimas wrote: > > Hi Richard, > > > > On Wed, 13 Mar 2024 at 22:19, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> On 3/13/24 06:23, Ilias Apalodimas wrote: > >>> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > >>> @@ -63,18 +63,11 @@ SECTIONS > >>> > >>> _image_binary_end = .; > >>> > >>> - .bss_start (NOLOAD) : { > >>> - . = ALIGN(8); > >>> - KEEP(*(.__bss_start)); > >>> - } >.sdram > >>> - > >>> - .bss (NOLOAD) : { > >>> + .bss : { > >>> + __bss_start = .; > >>> *(.bss*) > >>> - . = ALIGN(8); > >>> - } >.sdram > >>> - > >>> - .bss_end (NOLOAD) : { > >>> - KEEP(*(.__bss_end)); > >>> + . = ALIGN(8); > >>> + __bss_end = .; > >> > >> Still missing the alignment on .bss, previously in .bss_start. > > > > Since this is emitted in .sdram memory I can't define it as > > .bss ALIGN(8) : {....} since the calculated memory will be outside the > > sdram-defined region > > > > I could define it as > > .bss : { > > . = ALIGN(8); > > __bss_start = .; > > ...... > > } > > > > But instead, I added an assert at the bottom which will break the > > linking if the __bss_start is not 8byte aligned. > > I think it would be clearer to assert on __bss_start address rather than > CONFIG_SPL_BSS_START_ADDR, if that's possible. If not, then the constant will have to do. Fair enough and it is possible. I can convert that assertion to ASSERT(ADDR(.bss) % 8 == 0, ".bss must be 8-byte aligned"); Keep in mind that .bss, due to the linker aligning the section to the strictest input section requirement will end up aligning that to 8b regardless. IOW compiling with CONFIG_SPL_BSS_START_ADDR=0x1001, won't break the linking since .bss will end up at 0x1008. Testing the assertion with a section definition which looks like this will trigger the assert. .bss 0x1001 : { __bss_start = .; *(.bss*) . = ALIGN(8); __bss_end = .; } >.sdram I don't have a really strong opinion here. I think the assertion above is ok. What we could also do is .bss : { . = ALIGN(8); __bss_start = .; *(.bss*) . = ALIGN(8); __bss_end = .; } >.sdram which would also fix the __bss_start alignment regardless of the .bss output address. For example with the linker entry below __bss_start will end up at 0x1008 .bss 0x1001 : { . = ALIGN(8); __bss_start = .; *(.bss*) . = ALIGN(8); __bss_end = .; } >.sdram As you already mentioned, the bss_end - bss_start % 8 == 0 can be fixed by using memset. So since I plan to continue working on this to get RO, RW^x etc mappings I think I'll just go for the assertion for now -- but test the .bss address instead of the config option. Anyone objects? Thanks /Ilias > > > r~
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 7cb9d731246d..692414fe46fb 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -63,18 +63,11 @@ SECTIONS _image_binary_end = .; - .bss_start (NOLOAD) : { - . = ALIGN(8); - KEEP(*(.__bss_start)); - } >.sdram - - .bss (NOLOAD) : { + .bss : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } >.sdram - - .bss_end (NOLOAD) : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; } >.sdram /DISCARD/ : { *(.rela*) } @@ -89,3 +82,6 @@ SECTIONS #include "linux-kernel-image-header-vars.h" #endif } +ASSERT(CONFIG_SPL_BSS_START_ADDR % 8 == 0, \ + "CONFIG_SPL_BSS_START_ADDR must be 8-byte aligned"); + diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index fb6a30c922f7..9640cc7a04b8 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -149,19 +149,11 @@ SECTIONS _end = .; - . = ALIGN(8); - - .bss_start : { - KEEP(*(.__bss_start)); - } - - .bss : { + .bss ALIGN(8): { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } - - .bss_end : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; } /DISCARD/ : { *(.dynsym) } diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 7724c9332c3b..0dfe5f633b16 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -207,23 +207,15 @@ SECTIONS } /* - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c - * __bss_base and __bss_limit are for linker only (overlay ordering) + * These sections occupy the same memory, but their lifetimes do + * not overlap: U-Boot initializes .bss only after applying dynamic + * relocations and therefore after it doesn't need .rel.dyn any more. */ - - .bss_start __rel_dyn_start (OVERLAY) : { - KEEP(*(.__bss_start)); - __bss_base = .; - } - - .bss __bss_base (OVERLAY) : { + .bss ADDR(.rel.dyn) (OVERLAY): { + __bss_start = .; *(.bss*) - . = ALIGN(4); - __bss_limit = .; - } - - .bss_end __bss_limit (OVERLAY) : { - KEEP(*(.__bss_end)); + . = ALIGN(4); + __bss_end = .; } .dynsym _image_binary_end : { *(.dynsym) } diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index 857879711c6a..8e8bd5797e16 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -19,8 +19,6 @@ * aliasing warnings. */ -char __bss_start[0] __section(".__bss_start"); -char __bss_end[0] __section(".__bss_end"); char __image_copy_start[0] __section(".__image_copy_start"); char __image_copy_end[0] __section(".__image_copy_end"); char __rel_dyn_start[0] __section(".__rel_dyn_start"); diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index 74618eba591b..712c485d4d0b 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -56,18 +56,11 @@ SECTIONS _image_binary_end = .; - .bss_start (NOLOAD) : { - . = ALIGN(8); - KEEP(*(.__bss_start)); - } - - .bss (NOLOAD) : { + .bss ALIGN(8) : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } - - .bss_end (NOLOAD) : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; } /DISCARD/ : { *(.dynsym) } diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index 3b7c9d515f8b..3c5008b57392 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -103,23 +103,15 @@ SECTIONS _image_binary_end = .; /* - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c - * __bss_base and __bss_limit are for linker only (overlay ordering) + * These sections occupy the same memory, but their lifetimes do + * not overlap: U-Boot initializes .bss only after applying dynamic + * relocations and therefore after it doesn't need .rel.dyn any more. */ - - .bss_start __rel_dyn_start (OVERLAY) : { - KEEP(*(.__bss_start)); - __bss_base = .; - } - - .bss __bss_base (OVERLAY) : { + .bss ADDR(.rel.dyn) (OVERLAY): { + __bss_start = .; *(.bss*) - . = ALIGN(8); - __bss_limit = .; - } - - .bss_end __bss_limit (OVERLAY) : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; } /*
commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated") and commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts") were moving the bss_start/end on c generated variables that were injected in their own sections. The reason was that we needed relative relocations for position independent code and linker bugs back then prevented us from doing so [0]. However, the linker documentation pages states that symbols that are defined within a section definition will create a relocatable type with the value being a fixed offset from the base of a section [1]. So let's start cleaning this up starting with the bss_start and bss_end variables. Convert them into symbols within the .bss section definition. [0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object") [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845 Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- arch/arm/cpu/armv8/u-boot-spl.lds | 18 +++++++----------- arch/arm/cpu/armv8/u-boot.lds | 16 ++++------------ arch/arm/cpu/u-boot.lds | 22 +++++++--------------- arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 15 ++++----------- arch/arm/mach-zynq/u-boot.lds | 22 +++++++--------------- 6 files changed, 29 insertions(+), 66 deletions(-)