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

[Sam Edwards <cfsworks@gmail.com>]

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@linaro.org>
Tested-by: Sam Edwards <CFSworks@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. 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.

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