[PATCH 5/6] arm: fix __efi_runtime_start/end definitions

[Ilias Apalodimas]

Hi Sam,

Again thank you for the elaborate review. This really helps a lot.

On Wed, 6 Mar 2024 at 10:14, Sam Edwards <cfsworks at gmail.com> wrote:
>
>
>
> On 3/4/24 02:01, Ilias Apalodimas wrote:
> > __efi_runtime_start/end are defined as c variables for arm7 only in
> > order to force the compiler emit relative references. However, defining
> > those within a section definition will do the same thing. On top of that
> > the v8 linker scripts define it as a symbol.
> >
> > So let's remove the special sections from the linker scripts, the
> > variable definitions from sections.c and define them as a symbols within
> > the correct section.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Tested-by: Sam Edwards <CFSworks at gmail.com> # Binary output identical
>
> Thanks for the cleanup,
> Sam
>
> > ---
> >   arch/arm/cpu/u-boot.lds        | 12 +++---------
> >   arch/arm/lib/sections.c        |  2 --
> >   arch/arm/mach-zynq/u-boot.lds  | 12 +++---------
> >   include/asm-generic/sections.h |  1 +
> >   4 files changed, 7 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > index 7c6e7891d360..df55bb716e35 100644
> > --- a/arch/arm/cpu/u-boot.lds
> > +++ b/arch/arm/cpu/u-boot.lds
> > @@ -43,18 +43,12 @@ SECTIONS
> >       }
> >
> >       /* This needs to come before *(.text*) */
> > -     .__efi_runtime_start : {
> > -             *(.__efi_runtime_start)
> > -     }
> > -
> > -     .efi_runtime : {
> > +     .efi_runtime ALIGN(4) : {
>
> Do we truly require the ALIGN(4)? If I understand correctly, by default,
> the linker calculates the alignment of an output section as the least
> common multiple of the input sections' alignment requirements -- meaning
> most (perhaps all) of our ALIGN()s today are redundant.

I don't think we do. But I preserved those for a few reasons.

>  For the time
> being, I'm in favor of merging existing `. = ALIGN(x)` into each
> following section for clarity and to avoid the testing overhead of
> removing them in the same patch as other changes. However, in the near
> future (perhaps even as "near" as v2 of this series?), I'd also like to
> see a patch that eliminates unnecessary ALIGN()s altogether. Introducing
> additional ALIGN()s where we already know they aren't needed may be a
> step away from that goal.

So as you already mentioned the reason I preserved this is:
- Reduce the testing overhead by preserving the same layout for now
- In the future, I am playing around with the idea of mapping U-Boot
(not SPL but the relocated full U-Boot) in sections with proper memory
permissions (R), RW^X etc). In that case, we will need a 4k section
alignment and we can repurpose the ALIGN(4/8) to
ALIGN(CONSTANT(COMMONPAGESIZE)).

Thoughts?

Thanks
/Ilias

>
> > +             __efi_runtime_start = .;
> >               *(.text.efi_runtime*)
> >               *(.rodata.efi_runtime*)
> >               *(.data.efi_runtime*)
> > -     }
> > -
> > -     .__efi_runtime_stop : {
> > -             *(.__efi_runtime_stop)
> > +             __efi_runtime_stop = .;
> >       }
> >
> >       .text_rest :
> > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > index 1ee3dd3667ba..a4d4202e99f5 100644
> > --- a/arch/arm/lib/sections.c
> > +++ b/arch/arm/lib/sections.c
> > @@ -25,6 +25,4 @@ char __secure_start[0] __section(".__secure_start");
> >   char __secure_end[0] __section(".__secure_end");
> >   char __secure_stack_start[0] __section(".__secure_stack_start");
> >   char __secure_stack_end[0] __section(".__secure_stack_end");
> > -char __efi_runtime_start[0] __section(".__efi_runtime_start");
> > -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
> >   char _end[0] __section(".__end");
> > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > index 71dea4a1f60a..fcd0f42a7106 100644
> > --- a/arch/arm/mach-zynq/u-boot.lds
> > +++ b/arch/arm/mach-zynq/u-boot.lds
> > @@ -22,18 +22,12 @@ SECTIONS
> >       }
> >
> >       /* This needs to come before *(.text*) */
> > -     .__efi_runtime_start : {
> > -             *(.__efi_runtime_start)
> > -     }
> > -
> > -     .efi_runtime : {
> > +     .efi_runtime ALIGN(4) : {
>
> Ditto above
>
> > +             __efi_runtime_start = .;
> >               *(.text.efi_runtime*)
> >               *(.rodata.efi_runtime*)
> >               *(.data.efi_runtime*)
> > -     }
> > -
> > -     .__efi_runtime_stop : {
> > -             *(.__efi_runtime_stop)
> > +             __efi_runtime_stop = .;
> >       }
> >
> >       .text_rest :
> > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > index 60949200dd93..b6bca53db10d 100644
> > --- a/include/asm-generic/sections.h
> > +++ b/include/asm-generic/sections.h
> > @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
> >   extern char __ctors_start[], __ctors_end[];
> >
> >   extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
> > +extern char __efi_runtime_start[], __efi_runtime_stop[];
> >
> >   /* function descriptor handling (if any).  Override
> >    * in asm/sections.h */