[PATCH v2] arm: Add OVERLAY command to BSS section on ARM64

[PATCH v2] arm: Add OVERLAY command to BSS section on ARM64

[Marek Vasut]

Avoid allocating and loading the BSS section.

$ aarch64-linux-gnu-objdump -Sh u-boot

Before:
  10 .bss_start    00000000  00000000000f21d8  00000000000f21d8  001021d8  2**0
                   CONTENTS, ALLOC, LOAD, DATA
  11 .bss          000068f8  00000000000f2200  00000000000f2200  001021d8  2**6
                   ALLOC
  12 .bss_end      00000000  00000000000f8af8  00000000000f8af8  00108af8  2**0
                   CONTENTS, ALLOC, LOAD, DATA

After:
  10 .bss_start    00000000  00000000000bf990  00000000000bf990  001021e0  2**0
                   CONTENTS
  11 .bss          000068e8  00000000000bf990  00000000000bf990  001021e0  2**4
                   CONTENTS
  12 .bss_end      00000000  00000000000c6278  00000000000c6278  00108ac8  2**0
                   CONTENTS

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
V2: Replicate arch/arm/cpu/u-boot.lds BSS part verbatim
---
 arch/arm/cpu/armv8/u-boot.lds | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index fb6a30c922f..ebdc079552d 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -151,16 +151,18 @@ SECTIONS
 
 	. = ALIGN(8);
 
-	.bss_start : {
+	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		__bss_base = .;
 	}
 
-	.bss : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(8);
+		 __bss_limit = .;
 	}
 
-	.bss_end : {
+	.bss_end __bss_limit (OVERLAY) : {
 		KEEP(*(.__bss_end));
 	}
 
-- 
2.43.0

Re: [PATCH v2] arm: Add OVERLAY command to BSS section on ARM64

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

On Sun, Dec 17, 2023 at 01:33:39AM +0100, Marek Vasut wrote:
> Avoid allocating and loading the BSS section.
> 
> $ aarch64-linux-gnu-objdump -Sh u-boot
> 
> Before:
>   10 .bss_start    00000000  00000000000f21d8  00000000000f21d8  001021d8  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>   11 .bss          000068f8  00000000000f2200  00000000000f2200  001021d8  2**6
>                    ALLOC
>   12 .bss_end      00000000  00000000000f8af8  00000000000f8af8  00108af8  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
> 
> After:
>   10 .bss_start    00000000  00000000000bf990  00000000000bf990  001021e0  2**0
>                    CONTENTS
>   11 .bss          000068e8  00000000000bf990  00000000000bf990  001021e0  2**4
>                    CONTENTS
>   12 .bss_end      00000000  00000000000c6278  00000000000c6278  00108ac8  2**0
>                    CONTENTS
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> V2: Replicate arch/arm/cpu/u-boot.lds BSS part verbatim
> ---
>  arch/arm/cpu/armv8/u-boot.lds | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

The point I was trying to make before was that I wasn't 100% confident
that we need the linker scripts to match in that most of the relevant
code is in foo_64.S and not foo.S. But from a very quick grep it does
look similar enough that perhaps it was ported over directly enough that
we should be doing this with the linker scripts here too. Hopefully
throwing this in to -next soon enough will get any problems to fall out
quickly if we're wrong.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] arm: Add OVERLAY command to BSS section on ARM64

[Ilias Apalodimas]

Hi Marek,

On Sun, Dec 17, 2023 at 01:33:39AM +0100, Marek Vasut wrote:
> Avoid allocating and loading the BSS section.
>

Can we elaborate a bit more on why we need this? AFAICT there's no code
loading those segments in memory and swapping them, so why do we need the
OVERLAY? On top of that the ALLOC flag seems to be missing? The .bss
section doesn't need to be loaded indeed, since we can memset it to 0, but
it does need proper backing memory.

another thing I noticed is the bss_start and end are defined as sections of
their own and a bit of git history led me to 3ebd1cbc49f00050. But the
linker script will emit absolute addresses only if the symbol is defined
outside a section. IOW applying this makes the value expressed as a fixed
offset from the base of the section and the bss_start/end sections go away

--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -151,17 +151,11 @@ SECTIONS

        . = ALIGN(8);

-       .bss_start : {
-               KEEP(*(.__bss_start));
-       }
-
        .bss : {
+               .bss_start = .;
                *(.bss*)
                 . = ALIGN(8);
-       }
-
-       .bss_end : {
-               KEEP(*(.__bss_end));
+               .bss_end = .;
        }

        /DISCARD/ : { *(.dynsym) }


leads to this:

 11 .bss          000090b0  0000000000102b00  0000000000102b00  00112aa0  2**7
                  ALLOC

P.S: I am playing around with rewriting the linker script and mapping
u-boot with proper permissions at least for armv8. If this patch is neeeded
*now* can someone explain why? Otherwise I'll clean it up once I test my
patches enough

Thanks
/Ilias

> $ aarch64-linux-gnu-objdump -Sh u-boot
>
> Before:
>   10 .bss_start    00000000  00000000000f21d8  00000000000f21d8  001021d8  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>   11 .bss          000068f8  00000000000f2200  00000000000f2200  001021d8  2**6
>                    ALLOC
>   12 .bss_end      00000000  00000000000f8af8  00000000000f8af8  00108af8  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>
> After:
>   10 .bss_start    00000000  00000000000bf990  00000000000bf990  001021e0  2**0
>                    CONTENTS
>   11 .bss          000068e8  00000000000bf990  00000000000bf990  001021e0  2**4
>                    CONTENTS
>   12 .bss_end      00000000  00000000000c6278  00000000000c6278  00108ac8  2**0
>                    CONTENTS
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> V2: Replicate arch/arm/cpu/u-boot.lds BSS part verbatim
> ---
>  arch/arm/cpu/armv8/u-boot.lds | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index fb6a30c922f..ebdc079552d 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -151,16 +151,18 @@ SECTIONS
>
>  	. = ALIGN(8);
>
> -	.bss_start : {
> +	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		__bss_base = .;
>  	}
>
> -	.bss : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(8);
> +		 __bss_limit = .;
>  	}
>
> -	.bss_end : {
> +	.bss_end __bss_limit (OVERLAY) : {
>  		KEEP(*(.__bss_end));
>  	}
>
> --
> 2.43.0
>

Re: [PATCH v2] arm: Add OVERLAY command to BSS section on ARM64

[Ilias Apalodimas]

On Tue, 6 Feb 2024 at 16:44, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Marek,
>
> On Sun, Dec 17, 2023 at 01:33:39AM +0100, Marek Vasut wrote:
> > Avoid allocating and loading the BSS section.
> >
>
> Can we elaborate a bit more on why we need this? AFAICT there's no code
> loading those segments in memory and swapping them, so why do we need the
> OVERLAY? On top of that the ALLOC flag seems to be missing? The .bss
> section doesn't need to be loaded indeed, since we can memset it to 0, but
> it does need proper backing memory.
>
> another thing I noticed is the bss_start and end are defined as sections of
> their own and a bit of git history led me to 3ebd1cbc49f00050. But the
> linker script will emit absolute addresses only if the symbol is defined
> outside a section. IOW applying this makes the value expressed as a fixed
> offset from the base of the section and the bss_start/end sections go away
>
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -151,17 +151,11 @@ SECTIONS
>
>         . = ALIGN(8);
>
> -       .bss_start : {
> -               KEEP(*(.__bss_start));
> -       }
> -
>         .bss : {
> +               .bss_start = .;
>                 *(.bss*)
>                  . = ALIGN(8);
> -       }
> -
> -       .bss_end : {
> -               KEEP(*(.__bss_end));
> +               .bss_end = .;
>         }
>
>         /DISCARD/ : { *(.dynsym) }

Apologies the patch above was wrong. The . in bss_start/end isn't
needed and I was missing a __ before the symbol definition.

The diff below though should produce the same binary without the
bss_start/end sections and the .bss section marked as ALLOC only

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index fb6a30c922f7..8105cfd122af 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -151,17 +151,11 @@ SECTIONS

        . = ALIGN(8);

-       .bss_start : {
-               KEEP(*(.__bss_start));
-       }
-
        .bss : {
+               __bss_start = .;
                *(.bss*)
                 . = ALIGN(8);
-       }
-
-       .bss_end : {
-               KEEP(*(.__bss_end));
+               __bss_end = .;
        }

        /DISCARD/ : { *(.dynsym) }
diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index 857879711c6a..59dfc53851a8 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -19,8 +19,8 @@
  * aliasing warnings.
  */

-char __bss_start[0] __section(".__bss_start");
-char __bss_end[0] __section(".__bss_end");
+//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");


>
>
> leads to this:
>
>  11 .bss          000090b0  0000000000102b00  0000000000102b00  00112aa0  2**7
>                   ALLOC
>
> P.S: I am playing around with rewriting the linker script and mapping
> u-boot with proper permissions at least for armv8. If this patch is neeeded
> *now* can someone explain why? Otherwise I'll clean it up once I test my
> patches enough
>
> Thanks
> /Ilias
>
> > $ aarch64-linux-gnu-objdump -Sh u-boot
> >
> > Before:
> >   10 .bss_start    00000000  00000000000f21d8  00000000000f21d8  001021d8  2**0
> >                    CONTENTS, ALLOC, LOAD, DATA
> >   11 .bss          000068f8  00000000000f2200  00000000000f2200  001021d8  2**6
> >                    ALLOC
> >   12 .bss_end      00000000  00000000000f8af8  00000000000f8af8  00108af8  2**0
> >                    CONTENTS, ALLOC, LOAD, DATA
> >
> > After:
> >   10 .bss_start    00000000  00000000000bf990  00000000000bf990  001021e0  2**0
> >                    CONTENTS
> >   11 .bss          000068e8  00000000000bf990  00000000000bf990  001021e0  2**4
> >                    CONTENTS
> >   12 .bss_end      00000000  00000000000c6278  00000000000c6278  00108ac8  2**0
> >                    CONTENTS
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > ---
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Tom Rini <trini@konsulko.com>
> > ---
> > V2: Replicate arch/arm/cpu/u-boot.lds BSS part verbatim
> > ---
> >  arch/arm/cpu/armv8/u-boot.lds | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > index fb6a30c922f..ebdc079552d 100644
> > --- a/arch/arm/cpu/armv8/u-boot.lds
> > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > @@ -151,16 +151,18 @@ SECTIONS
> >
> >       . = ALIGN(8);
> >
> > -     .bss_start : {
> > +     .bss_start __rel_dyn_start (OVERLAY) : {
> >               KEEP(*(.__bss_start));
> > +             __bss_base = .;
> >       }
> >
> > -     .bss : {
> > +     .bss __bss_base (OVERLAY) : {
> >               *(.bss*)
> >                . = ALIGN(8);
> > +              __bss_limit = .;
> >       }
> >
> > -     .bss_end : {
> > +     .bss_end __bss_limit (OVERLAY) : {
> >               KEEP(*(.__bss_end));
> >       }
> >
> > --
> > 2.43.0
> >