[PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Mar 6 11:11:12 CET 2024
On Wed, 6 Mar 2024 at 11:08, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Sam,
>
> First of all, thanks for taking the time to review this.
>
> On Wed, 6 Mar 2024 at 09:32, Sam Edwards <cfsworks at gmail.com> wrote:
> >
> > On 3/4/24 02:01, 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.
> > >
> > > 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.
> > > This have been fixed since 2016 [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] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> > > [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > Tested-by: Caleb Connolly <caleb.connolly at linaro.org> # Qualcomm sdm845
> > > ---
> > > arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++-----------
> > > arch/arm/cpu/armv8/u-boot.lds | 15 +++------------
> > > arch/arm/cpu/u-boot.lds | 22 ++++------------------
> > > arch/arm/lib/sections.c | 2 --
> > > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++-----------
> > > arch/arm/mach-zynq/u-boot.lds | 21 +++------------------
> > > 6 files changed, 16 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
> > > index 7cb9d731246d..16fddb46e9cb 100644
> > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds
> > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds
> > > @@ -63,18 +63,10 @@ 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));
> > > + __bss_end = .;
> > > } >.sdram
> > >
> > > /DISCARD/ : { *(.rela*) }
> > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > > index fb6a30c922f7..c4ee10ebc3ff 100644
> > > --- a/arch/arm/cpu/armv8/u-boot.lds
> > > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > > @@ -149,19 +149,10 @@ SECTIONS
> > >
> > > _end = .;
> > >
> > > - . = ALIGN(8);
> > > -
> > > - .bss_start : {
> > > - KEEP(*(.__bss_start));
> > > - }
> > > -
> > > - .bss : {
> > > + .bss ALIGN(8): {
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(8);
> > > - }
> > > -
> > > - .bss_end : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > >
> > > /DISCARD/ : { *(.dynsym) }
> > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > index 7724c9332c3b..90d329b1ebe0 100644
> > > --- a/arch/arm/cpu/u-boot.lds
> > > +++ b/arch/arm/cpu/u-boot.lds
> > > @@ -206,27 +206,13 @@ SECTIONS
> > > *(.mmutable)
> > > }
> > >
> > > -/*
> > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > - * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > - */
> > > -
> > > - .bss_start __rel_dyn_start (OVERLAY) : {
> > > - KEEP(*(.__bss_start));
> > > - __bss_base = .;
> > > - }
> > > -
> > > - .bss __bss_base (OVERLAY) : {
> > > + .bss ALIGN(4): {
> >
> > Hi Ilias,
> >
> > I have been reviewing this patchset by diffing output binaries and
> > linker maps between successive patches. Most of the patches in this
> > series result in no change to the output binary whatsoever (which also
> > means, no regressions!) but this one does have a change: .bss is being
> > moved after .mmutable. You should be able to see this yourself by
> > comparing `u-boot.map` after a successful build, before and after
> > applying this patch.
>
> Yes it does
>
> >
> > Since the current u-boot.lds puts .bss right after __image_copy_end
> > (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I
> > guess makes sense, if U-Boot zero-initializes .bss after applying
> > relocations), perhaps this patch should be moving the .bss section up
> > there in the .lds, putting (OVERLAY) back, and adding a comment to the
> > effect of "These sections occupy the same memory, but their lifetimes do
> > not overlap: U-Boot initializes .bss only after applying relocations and
> > therefore after it doesn't need .rel.dyn any more."
>
> We could do that, I honestly don't have a strong opinion on that.
replying to myself here, but there was a relevant discussion here as well
https://lore.kernel.org/u-boot/ZcJF2uGIMj-jxT3M@hera/
>
> >
> > We might also decide that the overlay memory-saving trick isn't actually
> > all that important anymore and that .bss should have a new home. Someone
> > far more seasoned than I should be the one to make that decision though.
>
> I am not sure tbh. I imagine that the overlay saving trick between
> .rel_dyn and .bss would only matter in U-Boot SPL where memory is
> limited. The v8 version of SPL doesn't have the overlay but the v7
> does and I left it untouched on purpose. On top of that the overlay on
> v7 for SPL is defined after the image_binary_end.
> Perhaps Tom remembers a bit more of the lds history than I do. Tom thoughts?
>
> Thanks!
> /Ilias
>
> >
> > The rest of the patchset looks great! I'll add my tags to those in a bit.
> >
> > Cheers,
> > Sam
> >
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(4);
> > > - __bss_limit = .;
> > > - }
> > > -
> > > - .bss_end __bss_limit (OVERLAY) : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > >
> > > - .dynsym _image_binary_end : { *(.dynsym) }
> > > + .dynsym : { *(.dynsym) }
> > > .dynbss : { *(.dynbss) }
> > > .dynstr : { *(.dynstr*) }
> > > .dynamic : { *(.dynamic*) }
> > > 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..b7887194026e 100644
> > > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > @@ -56,18 +56,10 @@ 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));
> > > + __bss_end = .;
> > > }
> > >
> > > /DISCARD/ : { *(.dynsym) }
> > > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > > index 3b7c9d515f8b..8d3259821719 100644
> > > --- a/arch/arm/mach-zynq/u-boot.lds
> > > +++ b/arch/arm/mach-zynq/u-boot.lds
> > > @@ -102,26 +102,11 @@ 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)
> > > - */
> > > -
> > > - .bss_start __rel_dyn_start (OVERLAY) : {
> > > - KEEP(*(.__bss_start));
> > > - __bss_base = .;
> > > - }
> > > -
> > > - .bss __bss_base (OVERLAY) : {
> > > + .bss ALIGN(4): {
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(8);
> > > - __bss_limit = .;
> > > - }
> > > -
> > > - .bss_end __bss_limit (OVERLAY) : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > > -
> > > /*
> > > * Zynq needs to discard these sections because the user
> > > * is expected to pass this image on to tools for boot.bin
More information about the U-Boot
mailing list