Re: [PATCH v4 2/7] arm: clean up v7 and v8 linker scripts for bss_start/end
On 3/14/24 20:43, 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 # Qualcomm sdm845
Tested-by: Sam Edwards # Binary output identical
Signed-off-by: Ilias Apalodimas
---
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(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH v3 2/7] arm: clean up v7 and v8 linker scripts for bss_start/end
On 3/13/24 11:43, Ilias Apalodimas wrote:
Hi Richard,
On Wed, 13 Mar 2024 at 22:19, Richard Henderson
wrote:
On 3/13/24 06:23, Ilias Apalodimas wrote:
+++ 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 = .;
Still missing the alignment on .bss, previously in .bss_start.
Since this is emitted in .sdram memory I can't define it as
.bss ALIGN(8) : {} since the calculated memory will be outside the
sdram-defined region
I could define it as
.bss : {
. = ALIGN(8);
__bss_start = .;
..
}
But instead, I added an assert at the bottom which will break the
linking if the __bss_start is not 8byte aligned.
I think it would be clearer to assert on __bss_start address rather than
CONFIG_SPL_BSS_START_ADDR, if that's possible. If not, then the constant will have to do.
r~
Re: [PATCH v3 7/7] arm: remove redundant section alignments
On 3/13/24 06:23, Ilias Apalodimas wrote:
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 when they are not needed.
While at add comments for the alignment of __bss_start/end since our
C runtime setup assembly assumes that __bss_start - __bss_end will be
a multiple of 4/8 for armv7 and armv8 respectively.
It's worth noting that the alignment is preserved on .rel.dyn for
mach-zynq which was explicitly aligning that section on an 8b
boundary instead of 4b one.
Signed-off-by: Ilias Apalodimas
---
arch/arm/cpu/armv8/u-boot.lds | 9 ++---
arch/arm/cpu/u-boot.lds | 8 ++--
arch/arm/mach-zynq/u-boot.lds | 4 ++--
3 files changed, 14 insertions(+), 7 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH v3 2/7] arm: clean up v7 and v8 linker scripts for bss_start/end
On 3/13/24 06:23, Ilias Apalodimas wrote:
+++ 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 = .;
Still missing the alignment on .bss, previously in .bss_start.
With that fixed,
Reviewed-by: Richard Henderson
r~
Re: [PATCH 7/7 v2] arm: remove redundant section alignments
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~
Re: [PATCH 6/7 v2] arm: move image_copy_start/end to linker symbols
On 3/12/24 04:08, Ilias Apalodimas wrote:
image_copy_start/end are defined as c variables in order to force the compiler
emit relative references. However, defining those within a section definition
will do the same thing since [0].
So let's remove the special sections from the linker scripts, the
variable definitions from sections.c and define them as a symbols within
a section.
[0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for
shared object")
Suggested-by: Sam Edwards
Signed-off-by: Ilias Apalodimas
Tested-by: Sam Edwards # Binary output identical
---
arch/arm/cpu/armv8/u-boot-spl.lds | 7 ++-
arch/arm/cpu/armv8/u-boot.lds | 9 ++---
arch/arm/cpu/u-boot-spl.lds | 2 +-
arch/arm/cpu/u-boot.lds | 9 ++---
arch/arm/lib/sections.c | 2 --
arch/arm/mach-aspeed/ast2600/u-boot-spl.lds | 2 +-
arch/arm/mach-rockchip/u-boot-tpl-v8.lds| 7 ++-
arch/arm/mach-zynq/u-boot-spl.lds | 2 +-
arch/arm/mach-zynq/u-boot.lds | 8 ++--
9 files changed, 13 insertions(+), 35 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH 5/7 v2] arm: fix __efi_runtime_start/end definitions
On 3/12/24 04:08, 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 since [0].
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.
[0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for
shared object")
Suggested-by: Sam Edwards
Signed-off-by: Ilias Apalodimas
Reviewed-by: Sam Edwards
Tested-by: Sam Edwards # Binary output identical
---
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(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH 4/7 v2] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end
On 3/12/24 04:08, Ilias Apalodimas wrote:
commit 47bd65ef057f ("arm: make __rel_dyn_{start, end} compiler-generated")
were moving the __rel_dyn_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].
[0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for
shared object")
[1]https://sourceware.org/binutils/docs/ld/Expression-Section.html
Suggested-by: Sam Edwards
Signed-off-by: Ilias Apalodimas
Reviewed-by: Sam Edwards
Tested-by: Sam Edwards # Binary output identical
---
arch/arm/cpu/armv8/u-boot.lds | 16 +++-
arch/arm/cpu/u-boot.lds | 14 +++---
arch/arm/lib/sections.c | 2 --
arch/arm/mach-zynq/u-boot.lds | 14 +++---
4 files changed, 9 insertions(+), 37 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH 3/7 v2] arm: fix __efi_runtime_rel_start/end definitions
On 3/12/24 04:08, Ilias Apalodimas wrote:
__efi_runtime_rel_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 since [0].
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.
[0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for
shared object")
Suggested-by: Sam Edwards
Signed-off-by: Ilias Apalodimas
Reviewed-by: Sam Edwards
Tested-by: Sam Edwards # Binary output identical
---
arch/arm/cpu/armv8/u-boot.lds | 4 +---
arch/arm/cpu/u-boot.lds| 16 +++-
arch/arm/lib/sections.c| 2 --
arch/arm/mach-zynq/u-boot.lds | 16 +++-
include/asm-generic/sections.h | 2 ++
lib/efi_loader/efi_runtime.c | 1 +
6 files changed, 10 insertions(+), 31 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH 2/7 v2] arm: clean up v7 and v8 linker scripts for bss_start/end
On 3/12/24 04:08, Ilias Apalodimas wrote:
- .bss_start (NOLOAD) : {
- . = ALIGN(8);
This alignment got lost.
- KEEP(*(.__bss_start));
- } >.sdram
-
- .bss (NOLOAD) : {
+ .bss : {
If it is required, the best replacement would be here on the output section
definition.
- . = ALIGN(8);
-
- .bss_start : {
- KEEP(*(.__bss_start));
- }
-
- .bss : {
+ .bss ALIGN(8): {
Like you did here.
r~
