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

Ilias Apalodimas ilias.apalodimas at linaro.org
Thu Mar 7 07:50:20 CET 2024


On Thu, 7 Mar 2024 at 00:19, Sam Edwards <cfsworks at gmail.com> wrote:

>
>
> On 3/6/24 02:13, Ilias Apalodimas wrote:
> > 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?
>
> Ah, right: I had forgotten for a moment that the ultimate goal was to
> clean up the memory protection bit situation.
>
> Okay, as long as that future cleanup will also remove all unnecessary
> uses of ALIGN() (i.e. any that don't result in an actual change in
> memory layout) then that sounds great to me.
>

Yep will do


> Reviewed-by: Sam Edwards <CFSworks at gmail.com>
>

Thanks!


> Cheers,
> Sam
>
> >
> > 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 */
>


More information about the U-Boot mailing list