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

[Sam Edwards]

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.

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

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 */