| Message ID | 20240312140832.1968215-8-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up arm linker scripts | expand |
On 3/12/24 04:08, Ilias Apalodimas wrote: > index 33f4624b561d..ccdd1966cfbc 100644 > --- a/arch/arm/cpu/armv8/u-boot.lds > +++ b/arch/arm/cpu/armv8/u-boot.lds > @@ -132,7 +132,7 @@ SECTIONS > > _end = .; > > - .bss ALIGN(8): { > + .bss : { > __bss_start = .; > *(.bss*) > __bss_end = .; The code in arch/arm/lib/crt0_64.S assumes __bss_end - __bss_start is a multiple of 8. But that could probably be replaced by a proper call to memset fairly easily. > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > index b6b19a4174fe..a9fcbbf22e96 100644 > --- a/arch/arm/cpu/u-boot.lds > +++ b/arch/arm/cpu/u-boot.lds > @@ -155,7 +155,7 @@ SECTIONS > > __image_copy_end = .; > > - .rel.dyn ALIGN(4) : { > + .rel.dyn : { > __rel_dyn_start = .; > *(.rel*) > __rel_dyn_end = .; Because of the overlay, this affects .bss too. The code in arch/arm/lib/crt0.S may or may not be configured to use memset. When it isn't, it requires __bss_end - __bss_start to be a multiple of 4. Why does this not always use memset? r~
Hi Richard, Pasting some of the discussions we had over IRC for completeness. On Tue, 12 Mar 2024 at 19:00, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/12/24 04:08, Ilias Apalodimas wrote: > > index 33f4624b561d..ccdd1966cfbc 100644 > > --- a/arch/arm/cpu/armv8/u-boot.lds > > +++ b/arch/arm/cpu/armv8/u-boot.lds > > @@ -132,7 +132,7 @@ SECTIONS > > > > _end = .; > > > > - .bss ALIGN(8): { > > + .bss : { > > __bss_start = .; > > *(.bss*) > > __bss_end = .; > > The code in arch/arm/lib/crt0_64.S assumes __bss_end - __bss_start is a multiple of 8. > But that could probably be replaced by a proper call to memset fairly easily. > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > > index b6b19a4174fe..a9fcbbf22e96 100644 > > --- a/arch/arm/cpu/u-boot.lds > > +++ b/arch/arm/cpu/u-boot.lds > > @@ -155,7 +155,7 @@ SECTIONS > > > > __image_copy_end = .; > > > > - .rel.dyn ALIGN(4) : { > > + .rel.dyn : { > > __rel_dyn_start = .; > > *(.rel*) > > __rel_dyn_end = .; > > Because of the overlay, this affects .bss too. > > The code in arch/arm/lib/crt0.S may or may not be configured to use memset. When it > isn't, it requires __bss_end - __bss_start to be a multiple of 4. Why does this not > always use memset? I think it's due to size restrictions in SPL. Since I can't test all affected platforms, I prefer leaving it as is at least for now. I'll explicitly align sections to 4/8 bytes in v3. Once we get this merged we can try and clean those up as you suggested on a followup patchset Thanks /Ilias > > > r~
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index 33f4624b561d..ccdd1966cfbc 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -115,7 +115,7 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); } - .efi_runtime_rel ALIGN(8) : { + .efi_runtime_rel : { __efi_runtime_rel_start = .; *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) @@ -124,7 +124,7 @@ SECTIONS __image_copy_end = .; - .rela.dyn ALIGN(8) : { + .rela.dyn : { __rel_dyn_start = .; *(.rela*) __rel_dyn_end = .; @@ -132,7 +132,7 @@ SECTIONS _end = .; - .bss ALIGN(8): { + .bss : { __bss_start = .; *(.bss*) __bss_end = .; diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index b6b19a4174fe..a9fcbbf22e96 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -43,7 +43,7 @@ SECTIONS } /* This needs to come before *(.text*) */ - .efi_runtime ALIGN(4) : { + .efi_runtime : { __efi_runtime_start = .; *(.text.efi_runtime*) *(.rodata.efi_runtime*) @@ -146,7 +146,7 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); } - .efi_runtime_rel ALIGN(4) : { + .efi_runtime_rel : { __efi_runtime_rel_start = .; *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) @@ -155,7 +155,7 @@ SECTIONS __image_copy_end = .; - .rel.dyn ALIGN(4) : { + .rel.dyn : { __rel_dyn_start = .; *(.rel*) __rel_dyn_end = .; diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index 8ff35b5c17cf..e67416c09c1c 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -53,7 +53,7 @@ SECTIONS _image_binary_end = .; - .bss ALIGN(8) : { + .bss : { __bss_start = .; *(.bss*) __bss_end = .; diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index f739d1cfce85..111410215072 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -22,7 +22,7 @@ SECTIONS } /* This needs to come before *(.text*) */ - .efi_runtime ALIGN(4) : { + .efi_runtime : { __efi_runtime_start = .; *(.text.efi_runtime*) *(.rodata.efi_runtime*) @@ -52,7 +52,7 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); } - .efi_runtime_rel ALIGN(4) : { + .efi_runtime_rel : { __efi_runtime_rel_start = .; *(.rel*.efi_runtime) *(.rel*.efi_runtime.*)
Previous patches cleaning up linker symbols, also merged any explicit . = ALIGN(x); into section definitions -- e.g '.bss ALIGN(x) :' instead of '. - ALIGN(x); bss : {...}' However, if the output address is not specified then one will be chosen for the section. This address will be adjusted to fit the alignment requirement of the output section following the strictest alignment of any input section contained within the output section. So let's get rid of the redundant ALIGN directives. It's worth noting that the only platform the alignment is preserved on .rel.dyn is mach-zynq, which was explicitly aligning that section on an 8b boundary instead of the automatically chosen 4b alignment. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- arch/arm/cpu/armv8/u-boot.lds | 6 +++--- arch/arm/cpu/u-boot.lds | 6 +++--- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 2 +- arch/arm/mach-zynq/u-boot.lds | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) -- 2.37.2