Re: [PATCH v3 2/7] arm: clean up v7 and v8 linker scripts for bss_start/end

[Sam Edwards <cfsworks@gmail.com>]

On 3/13/24 10:23, Ilias Apalodimas wrote:
> commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
> and
> commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts")
> were moving the bss_start/end on c generated variables that were
> injected in their own sections. The reason was that we needed relative
> relocations for position independent code and linker bugs back then
> prevented us from doing so [0].
> 
> However, the linker documentation pages states that symbols that are
> defined within a section definition will create a relocatable type with
> the value being a fixed offset from the base of a section [1].
> So let's start cleaning this up starting with the bss_start and bss_end
> variables. Convert them into symbols within the .bss section definition.
> 
> [0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
> [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
> 
> Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   arch/arm/cpu/armv8/u-boot-spl.lds        | 18 +++++++-----------
>   arch/arm/cpu/armv8/u-boot.lds            | 16 ++++------------
>   arch/arm/cpu/u-boot.lds                  | 22 +++++++---------------
>   arch/arm/lib/sections.c                  |  2 --
>   arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 15 ++++-----------
>   arch/arm/mach-zynq/u-boot.lds            | 22 +++++++---------------
>   6 files changed, 29 insertions(+), 66 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
> index 7cb9d731246d..692414fe46fb 100644
> --- a/arch/arm/cpu/armv8/u-boot-spl.lds
> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds
> @@ -63,18 +63,11 @@ SECTIONS
>   
>   	_image_binary_end = .;
>   
> -	.bss_start (NOLOAD) : {
> -		. = ALIGN(8);
> -		KEEP(*(.__bss_start));
> -	} >.sdram
> -
> -	.bss (NOLOAD) : {
> +	.bss : {
> +		__bss_start = .;
>   		*(.bss*)
> -		 . = ALIGN(8);
> -	} >.sdram
> -
> -	.bss_end (NOLOAD) : {
> -		KEEP(*(.__bss_end));
> +		. = ALIGN(8);
> +		__bss_end = .;
>   	} >.sdram
>   
>   	/DISCARD/ : { *(.rela*) }
> @@ -89,3 +82,6 @@ SECTIONS
>   #include "linux-kernel-image-header-vars.h"
>   #endif
>   }
> +ASSERT(CONFIG_SPL_BSS_START_ADDR % 8 == 0, \
> +       "CONFIG_SPL_BSS_START_ADDR must be 8-byte aligned");
> +

Git complains about this added blank line at the end of the file. (My 
personal preference would be a blank line before the ASSERT, if the 
ASSERT is truly necessary.)

But beyond that:
Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical

Still really excited for this to land! I'm going to have to blow the 
dust off of my Clang/LLD support series here soon. :)

Best,
Sam

> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index fb6a30c922f7..9640cc7a04b8 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -149,19 +149,11 @@ SECTIONS
>   
>   	_end = .;
>   
> -	. = ALIGN(8);
> -
> -	.bss_start : {
> -		KEEP(*(.__bss_start));
> -	}
> -
> -	.bss : {
> +	.bss ALIGN(8): {
> +		__bss_start = .;
>   		*(.bss*)
> -		 . = ALIGN(8);
> -	}
> -
> -	.bss_end : {
> -		KEEP(*(.__bss_end));
> +		. = ALIGN(8);
> +		__bss_end = .;
>   	}
>   
>   	/DISCARD/ : { *(.dynsym) }
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 7724c9332c3b..0dfe5f633b16 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -207,23 +207,15 @@ SECTIONS
>   	}
>   
>   /*
> - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> - * __bss_base and __bss_limit are for linker only (overlay ordering)
> + * These sections occupy the same memory, but their lifetimes do
> + * not overlap: U-Boot initializes .bss only after applying dynamic
> + * relocations and therefore after it doesn't need .rel.dyn any more.
>    */
> -
> -	.bss_start __rel_dyn_start (OVERLAY) : {
> -		KEEP(*(.__bss_start));
> -		__bss_base = .;
> -	}
> -
> -	.bss __bss_base (OVERLAY) : {
> +	.bss ADDR(.rel.dyn) (OVERLAY): {
> +		__bss_start = .;
>   		*(.bss*)
> -		 . = ALIGN(4);
> -		 __bss_limit = .;
> -	}
> -
> -	.bss_end __bss_limit (OVERLAY) : {
> -		KEEP(*(.__bss_end));
> +		. = ALIGN(4);
> +		__bss_end = .;
>   	}
>   
>   	.dynsym _image_binary_end : { *(.dynsym) }
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index 857879711c6a..8e8bd5797e16 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -19,8 +19,6 @@
>    * aliasing warnings.
>    */
>   
> -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");
> diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> index 74618eba591b..712c485d4d0b 100644
> --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> @@ -56,18 +56,11 @@ SECTIONS
>   
>   	_image_binary_end = .;
>   
> -	.bss_start (NOLOAD) : {
> -		. = ALIGN(8);
> -		KEEP(*(.__bss_start));
> -	}
> -
> -	.bss (NOLOAD) : {
> +	.bss ALIGN(8) : {
> +		__bss_start = .;
>   		*(.bss*)
> -		 . = ALIGN(8);
> -	}
> -
> -	.bss_end (NOLOAD) : {
> -		KEEP(*(.__bss_end));
> +		. = ALIGN(8);
> +		__bss_end = .;
>   	}
>   
>   	/DISCARD/ : { *(.dynsym) }
> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> index 3b7c9d515f8b..3c5008b57392 100644
> --- a/arch/arm/mach-zynq/u-boot.lds
> +++ b/arch/arm/mach-zynq/u-boot.lds
> @@ -103,23 +103,15 @@ SECTIONS
>   	_image_binary_end = .;
>   
>   /*
> - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> - * __bss_base and __bss_limit are for linker only (overlay ordering)
> + * These sections occupy the same memory, but their lifetimes do
> + * not overlap: U-Boot initializes .bss only after applying dynamic
> + * relocations and therefore after it doesn't need .rel.dyn any more.
>    */
> -
> -	.bss_start __rel_dyn_start (OVERLAY) : {
> -		KEEP(*(.__bss_start));
> -		__bss_base = .;
> -	}
> -
> -	.bss __bss_base (OVERLAY) : {
> +	.bss ADDR(.rel.dyn) (OVERLAY): {
> +		__bss_start = .;
>   		*(.bss*)
> -		 . = ALIGN(8);
> -		 __bss_limit = .;
> -	}
> -
> -	.bss_end __bss_limit (OVERLAY) : {
> -		KEEP(*(.__bss_end));
> +		. = ALIGN(8);
> +		__bss_end = .;
>   	}
>   
>   	/*