On 3/4/24 02:01, 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.

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. This have been fixed since 2016 [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] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Tested-by: Caleb Connolly caleb.connolly@linaro.org # Qualcomm sdm845

---

arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++----------- arch/arm/cpu/armv8/u-boot.lds | 15 +++------------ arch/arm/cpu/u-boot.lds | 22 ++++------------------ arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++----------- arch/arm/mach-zynq/u-boot.lds | 21 +++------------------ 6 files changed, 16 insertions(+), 72 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 7cb9d731246d..16fddb46e9cb 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -63,18 +63,10 @@ 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));
  

• __bss_end = .;
  

  } >.sdram

  /DISCARD/ : { *(.rela*) }

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index fb6a30c922f7..c4ee10ebc3ff 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -149,19 +149,10 @@ SECTIONS

_end = .;

• . = ALIGN(8);
• .bss_start : {

• KEEP(*(.__bss_start));
  

• }
• .bss : {

• .bss ALIGN(8): {

• __bss_start = .;
  

  *(.bss*)

•  . = ALIGN(8);
  

• }
• .bss_end : {

• KEEP(*(.__bss_end));
  

• __bss_end = .;
  

  }

  /DISCARD/ : { *(.dynsym) }

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 7724c9332c3b..90d329b1ebe0 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -206,27 +206,13 @@ SECTIONS *(.mmutable) }

-/*

• • Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
• • __bss_base and __bss_limit are for linker only (overlay ordering)
• */
• .bss_start __rel_dyn_start (OVERLAY) : {

• KEEP(*(.__bss_start));
  

• __bss_base = .;
  

• }
• .bss __bss_base (OVERLAY) : {

• .bss ALIGN(4): {

Hi Ilias,

I have been reviewing this patchset by diffing output binaries and linker maps between successive patches. Most of the patches in this series result in no change to the output binary whatsoever (which also means, no regressions!) but this one does have a change: .bss is being moved after .mmutable. You should be able to see this yourself by comparing `u-boot.map` after a successful build, before and after applying this patch.

Since the current u-boot.lds puts .bss right after __image_copy_end (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I guess makes sense, if U-Boot zero-initializes .bss after applying relocations), perhaps this patch should be moving the .bss section up there in the .lds, putting (OVERLAY) back, and adding a comment to the effect of "These sections occupy the same memory, but their lifetimes do not overlap: U-Boot initializes .bss only after applying relocations and therefore after it doesn't need .rel.dyn any more."

We might also decide that the overlay memory-saving trick isn't actually all that important anymore and that .bss should have a new home. Someone far more seasoned than I should be the one to make that decision though.

The rest of the patchset looks great! I'll add my tags to those in a bit.

Cheers, Sam

• __bss_start = .;
  

  *(.bss*)

•  . = ALIGN(4);
  

•  __bss_limit = .;
  

• }
• .bss_end __bss_limit (OVERLAY) : {

• KEEP(*(.__bss_end));
  

• __bss_end = .;
  

  }

• .dynsym _image_binary_end : { *(.dynsym) }

• .dynsym : { *(.dynsym) } .dynbss : { *(.dynbss) } .dynstr : { *(.dynstr*) } .dynamic : { *(.dynamic*) }

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..b7887194026e 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -56,18 +56,10 @@ 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));
  

• __bss_end = .;
  

  }

  /DISCARD/ : { *(.dynsym) }

diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index 3b7c9d515f8b..8d3259821719 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -102,26 +102,11 @@ 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)
• */
• .bss_start __rel_dyn_start (OVERLAY) : {

• KEEP(*(.__bss_start));
  

• __bss_base = .;
  

• }
• .bss __bss_base (OVERLAY) : {

• .bss ALIGN(4): {

• __bss_start = .;
  

  *(.bss*)

•  . = ALIGN(8);
  

•  __bss_limit = .;
  

• }
• .bss_end __bss_limit (OVERLAY) : {

• KEEP(*(.__bss_end));
  

• __bss_end = .;
  

  }

• /*
  • Zynq needs to discard these sections because the user
  • is expected to pass this image on to tools for boot.bin

On Wed, 6 Mar 2024 at 11:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Sam,

First of all, thanks for taking the time to review this.

On Wed, 6 Mar 2024 at 09:32, Sam Edwards cfsworks@gmail.com wrote:

...

On 3/4/24 02:01, 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.

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. This have been fixed since 2016 [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] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Tested-by: Caleb Connolly caleb.connolly@linaro.org # Qualcomm sdm845

---

arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++----------- arch/arm/cpu/armv8/u-boot.lds | 15 +++------------ arch/arm/cpu/u-boot.lds | 22 ++++------------------ arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++----------- arch/arm/mach-zynq/u-boot.lds | 21 +++------------------ 6 files changed, 16 insertions(+), 72 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 7cb9d731246d..16fddb46e9cb 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -63,18 +63,10 @@ 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));
  

•         __bss_end = .;
  } >.sdram
  
  /DISCARD/ : { *(.rela*) }
  

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index fb6a30c922f7..c4ee10ebc3ff 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -149,19 +149,10 @@ SECTIONS

  _end = .;

• . = ALIGN(8);
  

• .bss_start : {
  

•         KEEP(*(.__bss_start));
  

• }
  

• .bss : {
  

• .bss ALIGN(8): {
  

•         __bss_start = .;
          *(.bss*)
  

•          . = ALIGN(8);
  

• }
  

• .bss_end : {
  

•         KEEP(*(.__bss_end));
  

•         __bss_end = .;
  }
  
  /DISCARD/ : { *(.dynsym) }
  

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 7724c9332c3b..90d329b1ebe0 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -206,27 +206,13 @@ SECTIONS *(.mmutable) }

-/*

• • Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
• • __bss_base and __bss_limit are for linker only (overlay ordering)
• */

• .bss_start __rel_dyn_start (OVERLAY) : {
  

•         KEEP(*(.__bss_start));
  

•         __bss_base = .;
  

• }
  

• .bss __bss_base (OVERLAY) : {
  

• .bss ALIGN(4): {
  

Hi Ilias,

I have been reviewing this patchset by diffing output binaries and linker maps between successive patches. Most of the patches in this series result in no change to the output binary whatsoever (which also means, no regressions!) but this one does have a change: .bss is being moved after .mmutable. You should be able to see this yourself by comparing `u-boot.map` after a successful build, before and after applying this patch.

Yes it does

...

Since the current u-boot.lds puts .bss right after __image_copy_end (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I guess makes sense, if U-Boot zero-initializes .bss after applying relocations), perhaps this patch should be moving the .bss section up there in the .lds, putting (OVERLAY) back, and adding a comment to the effect of "These sections occupy the same memory, but their lifetimes do not overlap: U-Boot initializes .bss only after applying relocations and therefore after it doesn't need .rel.dyn any more."

We could do that, I honestly don't have a strong opinion on that.

replying to myself here, but there was a relevant discussion here as well https://lore.kernel.org/u-boot/ZcJF2uGIMj-jxT3M@hera/

...

We might also decide that the overlay memory-saving trick isn't actually all that important anymore and that .bss should have a new home. Someone far more seasoned than I should be the one to make that decision though.

I am not sure tbh. I imagine that the overlay saving trick between .rel_dyn and .bss would only matter in U-Boot SPL where memory is limited. The v8 version of SPL doesn't have the overlay but the v7 does and I left it untouched on purpose. On top of that the overlay on v7 for SPL is defined after the image_binary_end. Perhaps Tom remembers a bit more of the lds history than I do. Tom thoughts?

Thanks! /Ilias

...

The rest of the patchset looks great! I'll add my tags to those in a bit.

Cheers, Sam

...

•         __bss_start = .;
          *(.bss*)
  

•          . = ALIGN(4);
  

•          __bss_limit = .;
  

• }
  

• .bss_end __bss_limit (OVERLAY) : {
  

•         KEEP(*(.__bss_end));
  

•         __bss_end = .;
  }
  
  

• .dynsym _image_binary_end : { *(.dynsym) }
  

• .dynsym  : { *(.dynsym) }
  .dynbss : { *(.dynbss) }
  .dynstr : { *(.dynstr*) }
  .dynamic : { *(.dynamic*) }
  

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..b7887194026e 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -56,18 +56,10 @@ 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));
  

•         __bss_end = .;
  }
  
  /DISCARD/ : { *(.dynsym) }
  

diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index 3b7c9d515f8b..8d3259821719 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -102,26 +102,11 @@ 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)
• */

• .bss_start __rel_dyn_start (OVERLAY) : {
  

•         KEEP(*(.__bss_start));
  

•         __bss_base = .;
  

• }
  

• .bss __bss_base (OVERLAY) : {
  

• .bss ALIGN(4): {
  

•         __bss_start = .;
          *(.bss*)
  

•          . = ALIGN(8);
  

•          __bss_limit = .;
  

• }
  

• .bss_end __bss_limit (OVERLAY) : {
  

•         KEEP(*(.__bss_end));
  

•         __bss_end = .;
  }
  

• /*
   * Zynq needs to discard these sections because the user
   * is expected to pass this image on to tools for boot.bin
  

On 3/4/24 02:01, 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. 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

Reviewed-by: Sam Edwards CFSworks@gmail.com Tested-by: Sam Edwards CFSworks@gmail.com # Binary output identical

Thanks for the cleanup, Sam

---

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(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index c4ee10ebc3ff..eccb116d3cfa 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -115,9 +115,7 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); }

• . = ALIGN(8);
• .efi_runtime_rel : {

• .efi_runtime_rel ALIGN(8) : { __efi_runtime_rel_start = .; *(.rel*.efi_runtime) *(.rel*.efi_runtime.*)

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 90d329b1ebe0..70e78ce46672 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -152,21 +152,11 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); }

• . = ALIGN(4);
• .efi_runtime_rel_start :
• {

• *(.__efi_runtime_rel_start)
  

• }
• .efi_runtime_rel : {

• .efi_runtime_rel ALIGN(4) : {

• __efi_runtime_rel_start = .;
  

  *(.rel*.efi_runtime) *(.rel*.efi_runtime.*)

• }
• .efi_runtime_rel_stop :
• {

• *(.__efi_runtime_rel_stop)
  

• __efi_runtime_rel_stop = .;
  

  }

  . = ALIGN(4);

diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index 8e8bd5797e16..ddfde52163fc 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -29,6 +29,4 @@ 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 __efi_runtime_rel_start[0] __section(".__efi_runtime_rel_start"); -char __efi_runtime_rel_stop[0] __section(".__efi_runtime_rel_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 8d3259821719..66a9e37f9198 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -58,21 +58,11 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); }

• . = ALIGN(4);
• .efi_runtime_rel_start :
• {

• *(.__efi_runtime_rel_start)
  

• }
• .efi_runtime_rel : {

• .efi_runtime_rel ALIGN(4) : {

• __efi_runtime_rel_start = .;
  

  *(.rel*.efi_runtime) *(.rel*.efi_runtime.*)

• }
• .efi_runtime_rel_stop :
• {

• *(.__efi_runtime_rel_stop)
  

• __efi_runtime_rel_stop = .;
  

  }

  . = ALIGN(8);

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index 1e1657a01673..60949200dd93 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -34,6 +34,8 @@ extern char __priv_data_start[], __priv_data_end[]; /* Start and end of .ctors section - used for constructor calls. */ extern char __ctors_start[], __ctors_end[];

+extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];

• /* function descriptor handling (if any). Override
  • in asm/sections.h */
  #ifndef dereference_function_descriptor

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 18da6892e796..9185f1894c47 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -15,6 +15,7 @@ #include <rtc.h> #include <asm/global_data.h> #include <u-boot/crc.h> +#include <asm/sections.h>

/* For manual relocation support */ DECLARE_GLOBAL_DATA_PTR;

On 3/4/24 02:01, 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.

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 [0]. This have been fixed since 2016 [1]

[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Reviewed-by: Sam Edwards CFSworks@gmail.com Tested-by: Sam Edwards CFSworks@gmail.com # Binary output identical

Thanks for the cleanup, Sam

---

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(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index eccb116d3cfa..e737de761a9d 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -129,20 +129,10 @@ SECTIONS *(.__image_copy_end) }

• . = ALIGN(8);
• .rel_dyn_start :
• {

• *(.__rel_dyn_start)
  

• }
• .rela.dyn : {

• .rela.dyn ALIGN(8) : {

• __rel_dyn_start = .;
  

  *(.rela*)

• }
• .rel_dyn_end :
• {

• *(.__rel_dyn_end)
  

• __rel_dyn_end = .;
  

  }

  _end = .;

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 70e78ce46672..7c6e7891d360 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -166,18 +166,10 @@ SECTIONS *(.__image_copy_end) }

• .rel_dyn_start :
• {

• *(.__rel_dyn_start)
  

• }
• .rel.dyn : {

• .rel.dyn ALIGN(4) : {

• __rel_dyn_start = .;
  

  *(.rel*)

• }
• .rel_dyn_end :
• {

• *(.__rel_dyn_end)
  

• __rel_dyn_end = .;
  

  }

  .end :

diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index ddfde52163fc..1ee3dd3667ba 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -21,8 +21,6 @@

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"); -char __rel_dyn_end[0] __section(".__rel_dyn_end"); char __secure_start[0] __section(".__secure_start"); char __secure_end[0] __section(".__secure_end"); char __secure_stack_start[0] __section(".__secure_stack_start"); diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index 66a9e37f9198..71dea4a1f60a 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -71,18 +71,10 @@ SECTIONS *(.__image_copy_end) }

• .rel_dyn_start :
• {

• *(.__rel_dyn_start)
  

• }
• .rel.dyn : {

• .rel.dyn ALIGN(8) : {

• __rel_dyn_start = .;
  

  *(.rel*)

• }
• .rel_dyn_end :
• {

• *(.__rel_dyn_end)
  

• __rel_dyn_end = .;
  

  }

  .end :

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

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@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@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.

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

Hi Sam,

On Thu, 7 Mar 2024 at 08:50, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Thu, 7 Mar 2024 at 00:19, Sam Edwards cfsworks@gmail.com wrote:

...

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@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@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.

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.

Yep will do

...

Reviewed-by: Sam Edwards CFSworks@gmail.com

Thanks. However, I noticed the alignment rule was wrong it needs to be '.efi_runtime : ALIGN(4)' instead of '.efi_runtime ALIGN(4) :'. The latter will define the start address instead of specifying the alignment. Due to what you mentioned above this (that we don't really need the ALIGN(4) the resulting binary is unaffected.

Thanks /Ilias

Thanks!

...

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

On Fri, 8 Mar 2024 at 16:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Fri, 8 Mar 2024 at 15:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Sam,

On Thu, 7 Mar 2024 at 08:50, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

On Thu, 7 Mar 2024 at 00:19, Sam Edwards cfsworks@gmail.com wrote:

...

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@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@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.

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.

Yep will do

...

Reviewed-by: Sam Edwards CFSworks@gmail.com

Thanks. However, I noticed the alignment rule was wrong it needs to be '.efi_runtime : ALIGN(4)' instead of '.efi_runtime ALIGN(4) :'. The latter will define the start address instead of specifying the alignment. Due to what you mentioned above this (that we don't really need the ALIGN(4) the resulting binary is unaffected.

To be exact, it will work because ALIGN returns the current location counter aligned upward to the specified value.

But thinking about it a bit more, I'll just get rid of the redundant ALIGN as you suggested. I don't think it makes sense to keep or force the output section alignment just because in the future we want to fix page permissions. We can always re-introduce it when stricter (4k) alignment is required for that

Cheers /Ilias

...

Thanks /Ilias

...

Thanks!

...

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

On 3/4/24 02:01, 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.

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.

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Tested-by: Sam Edwards CFSworks@gmail.com # Binary output identical

Since the __image_copy_* symbols are marking boundaries in the whole image as opposed to a single section, I'd find it clearer if they were loose in the SECTIONS { ... } block, so as not to imply that they are coupled to any particular section. Thoughts?

Thanks for the cleanup, Sam

---

arch/arm/cpu/armv8/u-boot.lds | 10 ++-------- arch/arm/cpu/u-boot.lds | 10 ++-------- arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------ arch/arm/mach-zynq/u-boot.lds | 9 ++------- 5 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index e737de761a9d..340acf5c043b 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -23,7 +23,7 @@ SECTIONS . = ALIGN(8); .text : {

• *(.__image_copy_start)
  

• __image_copy_start = .;
  

  CPUDIR/start.o (.text*) }

@@ -120,13 +120,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
• . = ALIGN(8);
• .image_copy_end :
• {

• *(.__image_copy_end)
  

• __image_copy_end = .;
  

  }

  .rela.dyn ALIGN(8) : {

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index df55bb716e35..7c4f9c2d4dd3 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -37,7 +37,7 @@ SECTIONS . = ALIGN(4); .text : {

• *(.__image_copy_start)
  

• __image_copy_start = .;
  

  *(.vectors) CPUDIR/start.o (.text*) }

@@ -151,13 +151,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
• . = ALIGN(4);
• .image_copy_end :
• {

• *(.__image_copy_end)
  

• __image_copy_end = .;
  

  }

  .rel.dyn ALIGN(4) : {

diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index a4d4202e99f5..db5463b2bbbc 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -19,8 +19,6 @@

• aliasing warnings.

*/

-char __image_copy_start[0] __section(".__image_copy_start"); -char __image_copy_end[0] __section(".__image_copy_end"); char __secure_start[0] __section(".__secure_start"); char __secure_end[0] __section(".__secure_end"); char __secure_stack_start[0] __section(".__secure_stack_start"); diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index b7887194026e..4b480ebb0c4d 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -24,7 +24,7 @@ SECTIONS

.text : { . = ALIGN(8);

• *(.__image_copy_start)
  

• __image_copy_start = .;
  

  CPUDIR/start.o (.text*) *(.text*) }

@@ -42,11 +42,7 @@ SECTIONS __u_boot_list : { . = ALIGN(8); KEEP(*(SORT(__u_boot_list*)));

• }
• .image_copy_end : {

• . = ALIGN(8);
  

• *(.__image_copy_end)
  

• __image_copy_end = .;
  

  }

  .end : {

diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index fcd0f42a7106..553bcf182272 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -16,7 +16,7 @@ SECTIONS . = ALIGN(4); .text : {

• *(.__image_copy_start)
  

• __image_copy_start = .;
  

  *(.vectors) CPUDIR/start.o (.text*) }

@@ -57,12 +57,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
• . = ALIGN(8);
• .image_copy_end :
• {

• *(.__image_copy_end)
  

• __image_copy_end = .;
  

  }

  .rel.dyn ALIGN(8) : {

On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Sam,

On Wed, 6 Mar 2024 at 10:22, Sam Edwards cfsworks@gmail.com wrote:

...

On 3/4/24 02:01, 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.

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.

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Tested-by: Sam Edwards CFSworks@gmail.com # Binary output identical

Since the __image_copy_* symbols are marking boundaries in the whole image as opposed to a single section, I'd find it clearer if they were loose in the SECTIONS { ... } block, so as not to imply that they are coupled to any particular section. Thoughts?

The reason I included it within a section is my understanding of the ld (way older) manual [0]. Copy-pasting from that:

" Assignment statements may appear:

• as commands in their own right in an ld script; or
• as independent statements within a SECTIONS command; or
• as part of the contents of a section definition in a SECTIONS command.

The first two cases are equivalent in effect--both define a symbol with an absolute address. The last case defines a symbol whose address is relative to a particular section." So, since we need relocatable entries, I included it within a section.

Looking at the latest ld [1] the wording has changed a bit it says "Expressions appearing outside an output section definition treat all numbers as absolute addresses" and it also has the following example SECTIONS { . = 0x100; __executable_start = 0x100; .data : { . = 0x10; __data_start = 0x10; *(.data) } … } both . and __executable_start are set to the absolute address 0x100 in the first two assignments, then both . and __data_start are set to 0x10 relative to the .data section in the second two assignments. So I assume the same behavior persists?

I just tested moving image_binary_end outside the section definition and it's still emitted properly. I'll try to figure this out and on v3 I'll move both image_copy_start/end outside the sections. I also noticed that I forgot to change arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.

Thanks /Ilias

[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html

Thanks /Ilias

...

Thanks for the cleanup, Sam

...

---

arch/arm/cpu/armv8/u-boot.lds | 10 ++-------- arch/arm/cpu/u-boot.lds | 10 ++-------- arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------ arch/arm/mach-zynq/u-boot.lds | 9 ++------- 5 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index e737de761a9d..340acf5c043b 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -23,7 +23,7 @@ SECTIONS . = ALIGN(8); .text : {

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
          CPUDIR/start.o (.text*)
  }
  
  

@@ -120,13 +120,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
  

• . = ALIGN(8);
  

• .image_copy_end :
  

• {
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
  }
  
  .rela.dyn ALIGN(8) : {
  

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index df55bb716e35..7c4f9c2d4dd3 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -37,7 +37,7 @@ SECTIONS . = ALIGN(4); .text : {

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
          *(.vectors)
          CPUDIR/start.o (.text*)
  }
  

@@ -151,13 +151,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
  

• . = ALIGN(4);
  

• .image_copy_end :
  

• {
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
  }
  
  .rel.dyn ALIGN(4) : {
  

diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index a4d4202e99f5..db5463b2bbbc 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -19,8 +19,6 @@

• aliasing warnings.

*/

-char __image_copy_start[0] __section(".__image_copy_start"); -char __image_copy_end[0] __section(".__image_copy_end"); char __secure_start[0] __section(".__secure_start"); char __secure_end[0] __section(".__secure_end"); char __secure_stack_start[0] __section(".__secure_stack_start"); diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index b7887194026e..4b480ebb0c4d 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -24,7 +24,7 @@ SECTIONS

  .text : {
          . = ALIGN(8);

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
          CPUDIR/start.o (.text*)
          *(.text*)
  }
  

@@ -42,11 +42,7 @@ SECTIONS __u_boot_list : { . = ALIGN(8); KEEP(*(SORT(__u_boot_list*)));

• }
  

• .image_copy_end : {
  

•         . = ALIGN(8);
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
  }
  
  .end : {
  

diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index fcd0f42a7106..553bcf182272 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -16,7 +16,7 @@ SECTIONS . = ALIGN(4); .text : {

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
          *(.vectors)
          CPUDIR/start.o (.text*)
  }
  

@@ -57,12 +57,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
  

• . = ALIGN(8);
  

• .image_copy_end :
  

• {
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
  }
  
  .rel.dyn ALIGN(8) : {
  

On 3/6/24 06:23, Ilias Apalodimas wrote:

On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Sam,

On Wed, 6 Mar 2024 at 10:22, Sam Edwards cfsworks@gmail.com wrote:

...

On 3/4/24 02:01, 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.

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.

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Tested-by: Sam Edwards CFSworks@gmail.com # Binary output identical

Since the __image_copy_* symbols are marking boundaries in the whole image as opposed to a single section, I'd find it clearer if they were loose in the SECTIONS { ... } block, so as not to imply that they are coupled to any particular section. Thoughts?

The reason I included it within a section is my understanding of the ld (way older) manual [0]. Copy-pasting from that:

" Assignment statements may appear:

• as commands in their own right in an ld script; or
• as independent statements within a SECTIONS command; or
• as part of the contents of a section definition in a SECTIONS command.

The first two cases are equivalent in effect--both define a symbol with an absolute address. The last case defines a symbol whose address is relative to a particular section." So, since we need relocatable entries, I included it within a section.

Looking at the latest ld [1] the wording has changed a bit it says "Expressions appearing outside an output section definition treat all numbers as absolute addresses" and it also has the following example SECTIONS { . = 0x100; __executable_start = 0x100; .data : { . = 0x10; __data_start = 0x10; *(.data) } … } both . and __executable_start are set to the absolute address 0x100 in the first two assignments, then both . and __data_start are set to 0x10 relative to the .data section in the second two assignments. So I assume the same behavior persists?

I just tested moving image_binary_end outside the section definition and it's still emitted properly. I'll try to figure this out and on v3 I'll move both image_copy_start/end outside the sections. I also noticed that I forgot to change arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.

Reading the manual again, the symbols will be emitted as relocatable entries regardless of their placement after that LD bug you mentioned is fixed. The only thing that will change when moving it outside the section definition is that an absolute value will be set, instead of a relative one to the start of the section. But that won't change the final binary placement in our case.

As far as I know, the linker is smart enough to understand that *any* symbol defined in terms of a memory location (e.g. `.` or `_other_symname`, ...) cannot be absolute when linking a PIE. The [1] documentation excerpt you cited seems to be saying that the exception to this rule is when a linker symbol is assigned to a constant value loose in the SECTIONS block -- and that apparently within the context of a `.section : {...}`, number literals are treated as offsets from the section's beginning, not absolute addresses: so we get relative symbols once again. This seems to be what your [0] documentation excerpt is also trying to say: "numbers in a section definition are relative to the section," not "relative symbols must be assigned in a section definition."

I played around a bit and removed the .image_copy_end section from the SPL in favor of a symbol. Compiling for xilinx_zynqmp_kria_defconfig which builds u-boot & SPL (note that I am building natively on an arm64 box)

# Before -- image_copy_end is .efi_runtime_rel and spl still has a section for .image_copy_end $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop 5: 00000000fffe0dc8 0 SECTION LOCAL DEFAULT 5 .image_copy_end 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start

# After -- image_copy_end moved outside .efi_runtime_rel and .image_copy_end converted to a linker symbol $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop 1349: 00000000fffe0dc8 0 NOTYPE GLOBAL DEFAULT 4 __image_copy_end 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start

Nothing changes in offsets and sizes.

The only thing I won't do right now is move image_copy_start outside the text region since that accounts for the CONFIG_SPL_TEXT_BASE and CONFIG_SYS_LOAD_ADDR. Keeping the __image_copy_start in there has an obvious caveat -- __image_copy_start will end up in .text and __image_copy_end in .rodata, but since we always had that it's fine for now.

I see what you're saying: the .text address is specified by -Ttext from Makefile.spl, so we don't know it in the linker script, and we can't rely on a `__image_copy_start = .;` assignment right before .text because the linker isn't going to place .text contiguously.

For what it's worth, `__image_copy_start = ADDR(.text);` *does* work (and produces a relative relocation). It might also be preferable to call attention to the fact that the .text section's start address is not determined/known by the linker script.

Up to you though! I'm fine with either approach, I just want to make sure that they're both considered. :)

Cheers, Sam

Thanks /Ilias

...

Thanks /Ilias

...

[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html

Thanks /Ilias

...

Thanks for the cleanup, Sam

...

---

arch/arm/cpu/armv8/u-boot.lds | 10 ++-------- arch/arm/cpu/u-boot.lds | 10 ++-------- arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------ arch/arm/mach-zynq/u-boot.lds | 9 ++------- 5 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index e737de761a9d..340acf5c043b 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -23,7 +23,7 @@ SECTIONS . = ALIGN(8); .text : {

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
           CPUDIR/start.o (.text*)
   }
  
  

@@ -120,13 +120,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
  

• . = ALIGN(8);
  

• .image_copy_end :
  

• {
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
   }
  
   .rela.dyn ALIGN(8) : {
  

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index df55bb716e35..7c4f9c2d4dd3 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -37,7 +37,7 @@ SECTIONS . = ALIGN(4); .text : {

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
           *(.vectors)
           CPUDIR/start.o (.text*)
   }
  

@@ -151,13 +151,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
  

• . = ALIGN(4);
  

• .image_copy_end :
  

• {
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
   }
  
   .rel.dyn ALIGN(4) : {
  

diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index a4d4202e99f5..db5463b2bbbc 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -19,8 +19,6 @@ * aliasing warnings. */

-char __image_copy_start[0] __section(".__image_copy_start"); -char __image_copy_end[0] __section(".__image_copy_end"); char __secure_start[0] __section(".__secure_start"); char __secure_end[0] __section(".__secure_end"); char __secure_stack_start[0] __section(".__secure_stack_start"); diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index b7887194026e..4b480ebb0c4d 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -24,7 +24,7 @@ SECTIONS

   .text : {
           . = ALIGN(8);

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
           CPUDIR/start.o (.text*)
           *(.text*)
   }
  

@@ -42,11 +42,7 @@ SECTIONS __u_boot_list : { . = ALIGN(8); KEEP(*(SORT(__u_boot_list*)));

• }
  

• .image_copy_end : {
  

•         . = ALIGN(8);
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
   }
  
   .end : {
  

diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index fcd0f42a7106..553bcf182272 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -16,7 +16,7 @@ SECTIONS . = ALIGN(4); .text : {

•         *(.__image_copy_start)
  

•         __image_copy_start = .;
           *(.vectors)
           CPUDIR/start.o (.text*)
   }
  

@@ -57,12 +57,7 @@ SECTIONS *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) __efi_runtime_rel_stop = .;

• }
  

• . = ALIGN(8);
  

• .image_copy_end :
  

• {
  

•         *(.__image_copy_end)
  

•         __image_copy_end = .;
   }
  
   .rel.dyn ALIGN(8) : {
  

On Thu, 7 Mar 2024 at 08:55, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Thu, 7 Mar 2024 at 01:08, Sam Edwards cfsworks@gmail.com wrote:

...

On 3/6/24 06:23, Ilias Apalodimas wrote:

...

On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Sam,

On Wed, 6 Mar 2024 at 10:22, Sam Edwards cfsworks@gmail.com wrote:

...

On 3/4/24 02:01, 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. > > 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. > > Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Tested-by: Sam Edwards CFSworks@gmail.com # Binary output identical

Since the __image_copy_* symbols are marking boundaries in the whole image as opposed to a single section, I'd find it clearer if they were loose in the SECTIONS { ... } block, so as not to imply that they are coupled to any particular section. Thoughts?

The reason I included it within a section is my understanding of the ld (way older) manual [0]. Copy-pasting from that:

" Assignment statements may appear:

• as commands in their own right in an ld script; or
• as independent statements within a SECTIONS command; or
• as part of the contents of a section definition in a SECTIONS command.

The first two cases are equivalent in effect--both define a symbol with an absolute address. The last case defines a symbol whose address is relative to a particular section." So, since we need relocatable entries, I included it within a section.

Looking at the latest ld [1] the wording has changed a bit it says "Expressions appearing outside an output section definition treat all numbers as absolute addresses" and it also has the following example SECTIONS { . = 0x100; __executable_start = 0x100; .data : { . = 0x10; __data_start = 0x10; *(.data) } … } both . and __executable_start are set to the absolute address 0x100 in the first two assignments, then both . and __data_start are set to 0x10 relative to the .data section in the second two assignments. So I assume the same behavior persists?

I just tested moving image_binary_end outside the section definition and it's still emitted properly. I'll try to figure this out and on v3 I'll move both image_copy_start/end outside the sections. I also noticed that I forgot to change arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.

Reading the manual again, the symbols will be emitted as relocatable entries regardless of their placement after that LD bug you mentioned is fixed. The only thing that will change when moving it outside the section definition is that an absolute value will be set, instead of a relative one to the start of the section. But that won't change the final binary placement in our case.

As far as I know, the linker is smart enough to understand that *any* symbol defined in terms of a memory location (e.g. `.` or `_other_symname`, ...) cannot be absolute when linking a PIE. The [1] documentation excerpt you cited seems to be saying that the exception to this rule is when a linker symbol is assigned to a constant value loose in the SECTIONS block -- and that apparently within the context of a `.section : {...}`, number literals are treated as offsets from the section's beginning, not absolute addresses: so we get relative symbols once again. This seems to be what your [0] documentation excerpt is also trying to say: "numbers in a section definition are relative to the section," not "relative symbols must be assigned in a section definition."

Exactly. I somehow misread that at first and assumed that 'absolute address' loose in the SECTIONS block also implied absolute relocations. But that's not the case so we can move it outside the section definition

...

...

I played around a bit and removed the .image_copy_end section from the SPL in favor of a symbol. Compiling for xilinx_zynqmp_kria_defconfig which builds u-boot & SPL (note that I am building natively on an arm64 box)

# Before -- image_copy_end is .efi_runtime_rel and spl still has a section for .image_copy_end $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop 5: 00000000fffe0dc8 0 SECTION LOCAL DEFAULT 5 .image_copy_end 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start

# After -- image_copy_end moved outside .efi_runtime_rel and .image_copy_end converted to a linker symbol $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop 1349: 00000000fffe0dc8 0 NOTYPE GLOBAL DEFAULT 4 __image_copy_end 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start

Nothing changes in offsets and sizes.

The only thing I won't do right now is move image_copy_start outside the text region since that accounts for the CONFIG_SPL_TEXT_BASE and CONFIG_SYS_LOAD_ADDR. Keeping the __image_copy_start in there has an obvious caveat -- __image_copy_start will end up in .text and __image_copy_end in .rodata, but since we always had that it's fine for now.

I see what you're saying: the .text address is specified by -Ttext from Makefile.spl, so we don't know it in the linker script, and we can't rely on a `__image_copy_start = .;` assignment right before .text because the linker isn't going to place .text contiguously.

Exactly and apart from that that __image_copy_start will probably end up @ 0x0 instead of the actual start address

...

For what it's worth, `__image_copy_start = ADDR(.text);` *does* work (and produces a relative relocation). It might also be preferable to call attention to the fact that the .text section's start address is not determined/known by the linker script.

Up to you though! I'm fine with either approach, I just want to make sure that they're both considered. :)

Ah good point, I completely forgot the ADDR directive. I'll switch to that since I also think having those loose in the SECTIONS block is far more intuitive

FWIW, I completely forgot your RFC trying to clean the same mess. I'll add your Suggested-by on v2

[0] https://lore.kernel.org/u-boot/20230520205547.1009254-11-CFSworks@gmail.com/

Cheers /Ilias

...

Cheers, Sam

...

Thanks /Ilias

...

Thanks /Ilias

...

[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html

Thanks /Ilias

...

Thanks for the cleanup, Sam

> --- > arch/arm/cpu/armv8/u-boot.lds | 10 ++-------- > arch/arm/cpu/u-boot.lds | 10 ++-------- > arch/arm/lib/sections.c | 2 -- > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------ > arch/arm/mach-zynq/u-boot.lds | 9 ++------- > 5 files changed, 8 insertions(+), 31 deletions(-) > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds > index e737de761a9d..340acf5c043b 100644 > --- a/arch/arm/cpu/armv8/u-boot.lds > +++ b/arch/arm/cpu/armv8/u-boot.lds > @@ -23,7 +23,7 @@ SECTIONS > . = ALIGN(8); > .text : > { > - *(.__image_copy_start) > + __image_copy_start = .; > CPUDIR/start.o (.text*) > } > > @@ -120,13 +120,7 @@ SECTIONS > *(.rel*.efi_runtime) > *(.rel*.efi_runtime.*) > __efi_runtime_rel_stop = .; > - } > - > - . = ALIGN(8); > - > - .image_copy_end : > - { > - *(.__image_copy_end) > + __image_copy_end = .; > } > > .rela.dyn ALIGN(8) : { > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > index df55bb716e35..7c4f9c2d4dd3 100644 > --- a/arch/arm/cpu/u-boot.lds > +++ b/arch/arm/cpu/u-boot.lds > @@ -37,7 +37,7 @@ SECTIONS > . = ALIGN(4); > .text : > { > - *(.__image_copy_start) > + __image_copy_start = .; > *(.vectors) > CPUDIR/start.o (.text*) > } > @@ -151,13 +151,7 @@ SECTIONS > *(.rel*.efi_runtime) > *(.rel*.efi_runtime.*) > __efi_runtime_rel_stop = .; > - } > - > - . = ALIGN(4); > - > - .image_copy_end : > - { > - *(.__image_copy_end) > + __image_copy_end = .; > } > > .rel.dyn ALIGN(4) : { > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c > index a4d4202e99f5..db5463b2bbbc 100644 > --- a/arch/arm/lib/sections.c > +++ b/arch/arm/lib/sections.c > @@ -19,8 +19,6 @@ > * aliasing warnings. > */ > > -char __image_copy_start[0] __section(".__image_copy_start"); > -char __image_copy_end[0] __section(".__image_copy_end"); > char __secure_start[0] __section(".__secure_start"); > char __secure_end[0] __section(".__secure_end"); > char __secure_stack_start[0] __section(".__secure_stack_start"); > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > index b7887194026e..4b480ebb0c4d 100644 > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > @@ -24,7 +24,7 @@ SECTIONS > > .text : { > . = ALIGN(8); > - *(.__image_copy_start) > + __image_copy_start = .; > CPUDIR/start.o (.text*) > *(.text*) > } > @@ -42,11 +42,7 @@ SECTIONS > __u_boot_list : { > . = ALIGN(8); > KEEP(*(SORT(__u_boot_list*))); > - } > - > - .image_copy_end : { > - . = ALIGN(8); > - *(.__image_copy_end) > + __image_copy_end = .; > } > > .end : { > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds > index fcd0f42a7106..553bcf182272 100644 > --- a/arch/arm/mach-zynq/u-boot.lds > +++ b/arch/arm/mach-zynq/u-boot.lds > @@ -16,7 +16,7 @@ SECTIONS > . = ALIGN(4); > .text : > { > - *(.__image_copy_start) > + __image_copy_start = .; > *(.vectors) > CPUDIR/start.o (.text*) > } > @@ -57,12 +57,7 @@ SECTIONS > *(.rel*.efi_runtime) > *(.rel*.efi_runtime.*) > __efi_runtime_rel_stop = .; > - } > - > - . = ALIGN(8); > - .image_copy_end : > - { > - *(.__image_copy_end) > + __image_copy_end = .; > } > > .rel.dyn ALIGN(8) : {