[PATCH 00/17] Various toolchain compatibility fixes/improvements

[PATCH 00/17] Various toolchain compatibility fixes/improvements

[Sam Edwards]

Long time no see, U-Boot folks!

This patchset consists of various bug fixes and correctness improvements that
I discovered while attempting to add first-class LLVM support to the build
system. These patches are NOT related to LLVM support directly; rather, they
address existing issues that should be resolved regardless of future changes.
For the most part, the patches are mutually independent and can be reviewed and
applied separately. If any patch is not suitable for merging now, feel free to
skip it: I will incorporate feedback and revisit those changes as part of the
upcoming LLVM support patchset. I'd like this patchset to be evaluated on its
own merits, based on the current state of the code, without consideration for
future LLVM support.

Note that the issues addressed in this patchset do not occur when U-Boot is
built using the GCC/GNU toolchain. These bugs seem to be specific to builds
using other toolchains, like LLVM, and do not appear to affect users relying on
GCC/GNU. Therefore, I see no need to rush these changes into the stable branch.

Again, these patches are mostly independent/reorderable...
...except that: "arm: Add aligned-memory aliases to eabi_compat"
...depends on: "arm: Add __aeabi_memclr in eabi_compat"

Warm regards,
Sam

Sam Edwards (17):
  arm: Remove stray .mmutable reference in linker script
  arm: Exclude eabi_compat from LTO
  arm: Add __aeabi_memclr in eabi_compat
  arm: Add aligned-memory aliases to eabi_compat
  arm: Discard unwanted sections in linker script
  arm: Use -mstrict-align when the MMU is off
  arm: Replace 'adrl' in EFI crt0
  x86: Fix call64's section flags
  spl: riscv: opensbi: Error on misaligned FDT
  spl: Align FDT load address
  makefile: Fix symbol typo in binary_size_check
  makefile: Avoid objcopy --gap-fill for .hex/.srec
  makefile: Add `norelro` linker option
  makefile: Add READELF command variable
  efi_loader: Remove ENTRY(_start) from linker script
  efi_loader: Move .dynamic out of .text in EFI
  scripts/Makefile.lib: efi: Preserve the .dynstr section as well

 Makefile                    | 16 +++++++++-------
 arch/arm/config.mk          |  4 ++++
 arch/arm/cpu/u-boot.lds     | 29 ++++++++++-------------------
 arch/arm/lib/Makefile       |  1 +
 arch/arm/lib/crt0_arm_efi.S |  3 ++-
 arch/arm/lib/eabi_compat.c  | 17 +++++++++++++++++
 arch/x86/cpu/i386/call64.S  |  2 +-
 common/spl/spl_fit.c        |  2 +-
 common/spl/spl_opensbi.c    |  5 +++++
 lib/efi_loader/elf_efi.ldsi |  7 +++----
 scripts/Makefile.lib        |  4 ++--
 11 files changed, 55 insertions(+), 35 deletions(-)

-- 
2.45.2

[PATCH 01/17] arm: Remove stray .mmutable reference in linker script

[Sam Edwards]

The .mmutable section was deprecated in 2012 [1] and finally removed
entirely from U-Boot in 2022 [2], so this special handling is no longer
necessary. Remove it to tidy up the linker script.

[1]: dde3b70dcf3d ("arm: add a common .lds link script")
[2]: 3135ba642f9a ("arm: pxa: Remove CONFIG_CPU_PXA25X")

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/cpu/u-boot.lds | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 2f50087f57a..63e82a09fad 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -169,15 +169,6 @@ SECTIONS
 	_end = .;
 	_image_binary_end = .;
 
-	/*
-	 * Deprecated: this MMU section is used by pxa at present but
-	 * should not be used by new boards/CPUs.
-	 */
-	. = ALIGN(4096);
-	.mmutable : {
-		*(.mmutable)
-	}
-
 /*
  * These sections occupy the same memory, but their lifetimes do
  * not overlap: U-Boot initializes .bss only after applying dynamic
-- 
2.45.2

Re: [PATCH 01/17] arm: Remove stray .mmutable reference in linker script

[Ilias Apalodimas]

On Mon, 24 Feb 2025 at 07:55, Sam Edwards <cfsworks@gmail.com> wrote:
>
> The .mmutable section was deprecated in 2012 [1] and finally removed
> entirely from U-Boot in 2022 [2], so this special handling is no longer
> necessary. Remove it to tidy up the linker script.
>
> [1]: dde3b70dcf3d ("arm: add a common .lds link script")
> [2]: 3135ba642f9a ("arm: pxa: Remove CONFIG_CPU_PXA25X")
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/cpu/u-boot.lds | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 2f50087f57a..63e82a09fad 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -169,15 +169,6 @@ SECTIONS
>         _end = .;
>         _image_binary_end = .;
>
> -       /*
> -        * Deprecated: this MMU section is used by pxa at present but
> -        * should not be used by new boards/CPUs.
> -        */
> -       . = ALIGN(4096);
> -       .mmutable : {
> -               *(.mmutable)
> -       }
> -
>  /*
>   * These sections occupy the same memory, but their lifetimes do
>   * not overlap: U-Boot initializes .bss only after applying dynamic
> --
> 2.45.2
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 02/17] arm: Exclude eabi_compat from LTO

[Sam Edwards]

These symbols need to survive the IR-level dead function elimination pass,
since nothing at the IR level is referencing them (calls to these are inserted
later, at codegen time).

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/lib/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 1c95dd6fed2..74cd5051552 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi_table.o
 # For EABI conformant tool chains, provide eabi_compat()
 ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS)))
 extra-y	+= eabi_compat.o
+CFLAGS_REMOVE_eabi_compat.o := $(LTO_CFLAGS)
 endif
 
 # some files can only build in ARM or THUMB2, not THUMB1
-- 
2.45.2

Re: [PATCH 02/17] arm: Exclude eabi_compat from LTO

[Ilias Apalodimas]

On Mon, 24 Feb 2025 at 07:55, Sam Edwards <cfsworks@gmail.com> wrote:
>
> These symbols need to survive the IR-level dead function elimination pass,
> since nothing at the IR level is referencing them (calls to these are inserted
> later, at codegen time).
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/lib/Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 1c95dd6fed2..74cd5051552 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi_table.o
>  # For EABI conformant tool chains, provide eabi_compat()
>  ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS)))
>  extra-y        += eabi_compat.o
> +CFLAGS_REMOVE_eabi_compat.o := $(LTO_CFLAGS)
>  endif
>
>  # some files can only build in ARM or THUMB2, not THUMB1
> --
> 2.45.2
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 03/17] arm: Add __aeabi_memclr in eabi_compat

[Sam Edwards]

LLVM's code generator will sometimes emit calls to __aeabi_memclr. Add an
implementation of this for LLVM compatibility.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/lib/eabi_compat.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/lib/eabi_compat.c b/arch/arm/lib/eabi_compat.c
index 602efe04c04..e4190c049a3 100644
--- a/arch/arm/lib/eabi_compat.c
+++ b/arch/arm/lib/eabi_compat.c
@@ -37,3 +37,8 @@ void __aeabi_memset(void *dest, size_t n, int c)
 {
 	(void) memset(dest, c, n);
 }
+
+void __aeabi_memclr(void *dest, size_t n)
+{
+	(void) memset(dest, 0, n);
+}
-- 
2.45.2

Re: [PATCH 03/17] arm: Add __aeabi_memclr in eabi_compat

[Ilias Apalodimas]

On Mon, 24 Feb 2025 at 07:56, Sam Edwards <cfsworks@gmail.com> wrote:
>
> LLVM's code generator will sometimes emit calls to __aeabi_memclr. Add an
> implementation of this for LLVM compatibility.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/lib/eabi_compat.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/lib/eabi_compat.c b/arch/arm/lib/eabi_compat.c
> index 602efe04c04..e4190c049a3 100644
> --- a/arch/arm/lib/eabi_compat.c
> +++ b/arch/arm/lib/eabi_compat.c
> @@ -37,3 +37,8 @@ void __aeabi_memset(void *dest, size_t n, int c)
>  {
>         (void) memset(dest, c, n);
>  }
> +
> +void __aeabi_memclr(void *dest, size_t n)
> +{
> +       (void) memset(dest, 0, n);
> +}
> --
> 2.45.2
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 04/17] arm: Add aligned-memory aliases to eabi_compat

[Sam Edwards]

These are sometimes used by LLVM's code-generator, when it can guarantee that
the memory buffer being passed is aligned on a (4- or 8-byte) boundary. They
can safely be aliased to the unaligned versions.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/lib/eabi_compat.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/lib/eabi_compat.c b/arch/arm/lib/eabi_compat.c
index e4190c049a3..e6cafcc5f2b 100644
--- a/arch/arm/lib/eabi_compat.c
+++ b/arch/arm/lib/eabi_compat.c
@@ -33,12 +33,24 @@ void __aeabi_memcpy(void *dest, const void *src, size_t n)
 	(void) memcpy(dest, src, n);
 }
 
+void __aeabi_memcpy4(void *dest, const void *src, size_t n) __alias(__aeabi_memcpy);
+
+void __aeabi_memcpy8(void *dest, const void *src, size_t n) __alias(__aeabi_memcpy);
+
 void __aeabi_memset(void *dest, size_t n, int c)
 {
 	(void) memset(dest, c, n);
 }
 
+void __aeabi_memset4(void *dest, size_t n, int c) __alias(__aeabi_memset);
+
+void __aeabi_memset8(void *dest, size_t n, int c) __alias(__aeabi_memset);
+
 void __aeabi_memclr(void *dest, size_t n)
 {
 	(void) memset(dest, 0, n);
 }
+
+void __aeabi_memclr4(void *dest, size_t n) __alias(__aeabi_memclr);
+
+void __aeabi_memclr8(void *dest, size_t n) __alias(__aeabi_memclr);
-- 
2.45.2

Re: [PATCH 04/17] arm: Add aligned-memory aliases to eabi_compat

[Ilias Apalodimas]

On Mon, 24 Feb 2025 at 07:56, Sam Edwards <cfsworks@gmail.com> wrote:
>
> These are sometimes used by LLVM's code-generator, when it can guarantee that
> the memory buffer being passed is aligned on a (4- or 8-byte) boundary. They
> can safely be aliased to the unaligned versions.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/lib/eabi_compat.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/lib/eabi_compat.c b/arch/arm/lib/eabi_compat.c
> index e4190c049a3..e6cafcc5f2b 100644
> --- a/arch/arm/lib/eabi_compat.c
> +++ b/arch/arm/lib/eabi_compat.c
> @@ -33,12 +33,24 @@ void __aeabi_memcpy(void *dest, const void *src, size_t n)
>         (void) memcpy(dest, src, n);
>  }
>
> +void __aeabi_memcpy4(void *dest, const void *src, size_t n) __alias(__aeabi_memcpy);
> +
> +void __aeabi_memcpy8(void *dest, const void *src, size_t n) __alias(__aeabi_memcpy);
> +
>  void __aeabi_memset(void *dest, size_t n, int c)
>  {
>         (void) memset(dest, c, n);
>  }
>
> +void __aeabi_memset4(void *dest, size_t n, int c) __alias(__aeabi_memset);
> +
> +void __aeabi_memset8(void *dest, size_t n, int c) __alias(__aeabi_memset);
> +
>  void __aeabi_memclr(void *dest, size_t n)
>  {
>         (void) memset(dest, 0, n);
>  }
> +
> +void __aeabi_memclr4(void *dest, size_t n) __alias(__aeabi_memclr);
> +
> +void __aeabi_memclr8(void *dest, size_t n) __alias(__aeabi_memclr);
> --
> 2.45.2
>


Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 05/17] arm: Discard unwanted sections in linker script

[Sam Edwards]

There are a handful of sections that are not useful in the U-Boot output
binary. At present, the linker script moves these to the end of the
binary, after the _image_binary_end marker symbol, so that they don't
get loaded.

The linker script syntax supports discarding sections that shouldn't be
included in the output. Switch to this instead, to make the intention
clearer and reduce the ELF sections that have to be handled later in the
build. This is also consistent with the other architectures' linker
scripts.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/cpu/u-boot.lds | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 63e82a09fad..817e7a983ae 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -181,14 +181,14 @@ SECTIONS
 		__bss_end = .;
 	}
 
-	.dynsym _image_binary_end : { *(.dynsym) }
-	.dynbss : { *(.dynbss) }
-	.dynstr : { *(.dynstr*) }
-	.dynamic : { *(.dynamic*) }
-	.plt : { *(.plt*) }
-	.interp : { *(.interp*) }
-	.gnu.hash : { *(.gnu.hash) }
-	.gnu : { *(.gnu*) }
-	.ARM.exidx : { *(.ARM.exidx*) }
-	.gnu.linkonce.armexidx : { *(.gnu.linkonce.armexidx.*) }
+	/DISCARD/ : { *(.dynsym) }
+	/DISCARD/ : { *(.dynbss) }
+	/DISCARD/ : { *(.dynstr*) }
+	/DISCARD/ : { *(.dynamic*) }
+	/DISCARD/ : { *(.plt*) }
+	/DISCARD/ : { *(.interp*) }
+	/DISCARD/ : { *(.gnu.hash) }
+	/DISCARD/ : { *(.gnu*) }
+	/DISCARD/ : { *(.ARM.exidx*) }
+	/DISCARD/ : { *(.gnu.linkonce.armexidx.*) }
 }
-- 
2.45.2

[PATCH 06/17] arm: Use -mstrict-align when the MMU is off

[Sam Edwards]

On ARM, enabling the MMU is a prerequisite to enabling caching, which
allows unaligned memory accesses by consolidating them into aligned
accesses. When the MMU is disabled, however, all accesses must strictly
adhere to the alignment rules.

Some compilers (e.g. Clang) contain optimizations that may break these
rules on the assumption that misaligned accesses merely incur a
performance penalty. We do not necessarily want to prevent the compiler
from applying these optimizations where it believes there may be net
gains, but we do need to tell it to respect the alignment requirements
when the MMU is disabled.

Therefore, add the `-mstrict-align` compiler flag when SYS_ARM_MMU is
not set.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/config.mk | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/config.mk b/arch/arm/config.mk
index e0045e22271..98237b251f1 100644
--- a/arch/arm/config.mk
+++ b/arch/arm/config.mk
@@ -33,6 +33,10 @@ PLATFORM_RELFLAGS	+= $(LLVM_RELFLAGS)
 
 PLATFORM_CPPFLAGS += -D__ARM__
 
+ifneq ($(CONFIG_SYS_ARM_MMU),y)
+PLATFORM_RELFLAGS += -mstrict-align
+endif
+
 ifdef CONFIG_ARM64
 PLATFORM_ELFFLAGS += -B aarch64 -O elf64-littleaarch64
 else
-- 
2.45.2

Re: [PATCH 06/17] arm: Use -mstrict-align when the MMU is off

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> On ARM, enabling the MMU is a prerequisite to enabling caching, which
> allows unaligned memory accesses by consolidating them into aligned
> accesses. When the MMU is disabled, however, all accesses must strictly
> adhere to the alignment rules.
>
> Some compilers (e.g. Clang) contain optimizations that may break these
> rules on the assumption that misaligned accesses merely incur a
> performance penalty. We do not necessarily want to prevent the compiler
> from applying these optimizations where it believes there may be net
> gains, but we do need to tell it to respect the alignment requirements
> when the MMU is disabled.
>
> Therefore, add the `-mstrict-align` compiler flag when SYS_ARM_MMU is
> not set.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   arch/arm/config.mk | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/config.mk b/arch/arm/config.mk
> index e0045e22271..98237b251f1 100644
> --- a/arch/arm/config.mk
> +++ b/arch/arm/config.mk
> @@ -33,6 +33,10 @@ PLATFORM_RELFLAGS	+= $(LLVM_RELFLAGS)
>
>   PLATFORM_CPPFLAGS += -D__ARM__
>
> +ifneq ($(CONFIG_SYS_ARM_MMU),y)
> +PLATFORM_RELFLAGS += -mstrict-align

We don't set CONFIG_SYS_ARM_MMU on armv8 though we initialize the MMU.

The flag -mstrict-align seems only to exist for 64bit ARM and is already
set in arch/arm/cpu/armv8/config.mk.

On 32-bit ARM there is a flag -mno-unaligned-access which we set in
arch/arm/cpu/armv7/config.mk and arch/arm/cpu/armv7m/config.mk.

Here are some places we forbid unaligned access irrespective of
CONFIG_SYS_ARM_MMU:

arch/arm/cpu/arm1136/start.S:80:        orr     r0, r0, #0x00000002
@ set bit 1 (A) Align
arch/arm/cpu/arm1176/start.S:82:        orr     r0, r0, #0x00000002
@ set bit 1 (A) Align
arch/arm/cpu/arm920t/start.S:81:        orr     r0, r0, #0x00000002
@ set bit 1 (A) Align
arch/arm/cpu/arm926ejs/start.S:102:     orr     r0, r0, #0x00000002
/* set bit 1 (A) Align */
arch/arm/cpu/arm946es/start.S:88:       orr     r0, r0, #0x00000002
/* set bit 1 (A) Align */
arch/arm/cpu/armv7/start.S:229: orr     r0, r0, #0x00000002     @ set
bit 1 (--A-) Align

Best regards

Heinrich

> +endif
> +
>   ifdef CONFIG_ARM64
>   PLATFORM_ELFFLAGS += -B aarch64 -O elf64-littleaarch64
>   else

Re: [PATCH 06/17] arm: Use -mstrict-align when the MMU is off

[Mark Kettenis]

> From: Sam Edwards <cfsworks@gmail.com>
> Date: Sun, 23 Feb 2025 21:55:13 -0800

Hi Sam,

> On ARM, enabling the MMU is a prerequisite to enabling caching, which
> allows unaligned memory accesses by consolidating them into aligned
> accesses. When the MMU is disabled, however, all accesses must strictly
> adhere to the alignment rules.
> 
> Some compilers (e.g. Clang) contain optimizations that may break these
> rules on the assumption that misaligned accesses merely incur a
> performance penalty. We do not necessarily want to prevent the compiler
> from applying these optimizations where it believes there may be net
> gains, but we do need to tell it to respect the alignment requirements
> when the MMU is disabled.
> 
> Therefore, add the `-mstrict-align` compiler flag when SYS_ARM_MMU is
> not set.

The default for this option changed somewhat recently in clang for
non-Linux targets, which may explain why this wasn't an issue in the past.

One question though.  I believe there is some C code in U-Boot that
runs before the MMU gets turned on, even if SYS_ARM_MMU is set.  That
code should always be compiled with -mstrict-align.  And what about
SYS_ICACHE_OFF and SYS_DCACHE_OFF?

So I wonder if -mstrict-align should always be used for arm.  I doubt
the optimizations the compiler is doing in absence of that option
result in any measurable speedup.

> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/config.mk | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/config.mk b/arch/arm/config.mk
> index e0045e22271..98237b251f1 100644
> --- a/arch/arm/config.mk
> +++ b/arch/arm/config.mk
> @@ -33,6 +33,10 @@ PLATFORM_RELFLAGS	+= $(LLVM_RELFLAGS)
>  
>  PLATFORM_CPPFLAGS += -D__ARM__
>  
> +ifneq ($(CONFIG_SYS_ARM_MMU),y)
> +PLATFORM_RELFLAGS += -mstrict-align
> +endif
> +
>  ifdef CONFIG_ARM64
>  PLATFORM_ELFFLAGS += -B aarch64 -O elf64-littleaarch64
>  else
> -- 
> 2.45.2
> 
> 

[PATCH 07/17] arm: Replace 'adrl' in EFI crt0

[Sam Edwards]

LLVM's IAS does not (and cannot easily) support the 'adrl'
pseudoinstruction, and ARM developers generally do not consider it
portable across assembler implementations either.

Instead, expand it into the two subtract instructions it would emit
anyway. An explanation of the math follows:

The .+8 and .+4 refer to the same memory location; this is because the
.+4 expression occurs in a subsequent instruction, 4 bytes after the
first. This memory location is the value of the PC register when it is
read by the first sub instruction. Thus, both inner parenthesized
expressions evaluate to the same result: PC's offset relative to
image_base. The subtract instructions then remove one byte each
(low, then high) of the total offset, thereby getting the absolute
address of image_base loaded in r0.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/lib/crt0_arm_efi.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S
index 91b0fe12c51..235b3a0c48f 100644
--- a/arch/arm/lib/crt0_arm_efi.S
+++ b/arch/arm/lib/crt0_arm_efi.S
@@ -149,7 +149,8 @@ _start:
 	adr		r1, .L_DYNAMIC
 	ldr		r0, [r1]
 	add		r1, r0, r1
-	adrl		r0, image_base
+	sub		r0, pc, #((.+8-image_base) & 0xff)
+	sub		r0, r0, #((.+4-image_base) & 0xff00)
 	bl		_relocate
 	teq		r0, #0
 	bne		0f
-- 
2.45.2

Re: [PATCH 07/17] arm: Replace 'adrl' in EFI crt0

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> LLVM's IAS does not (and cannot easily) support the 'adrl'
> pseudoinstruction, and ARM developers generally do not consider it
> portable across assembler implementations either.
>
> Instead, expand it into the two subtract instructions it would emit
> anyway. An explanation of the math follows:
>
> The .+8 and .+4 refer to the same memory location; this is because the

Are we sure that we never generate thumb instructions here?

> .+4 expression occurs in a subsequent instruction, 4 bytes after the
> first. This memory location is the value of the PC register when it is
> read by the first sub instruction. Thus, both inner parenthesized
> expressions evaluate to the same result: PC's offset relative to
> image_base. The subtract instructions then remove one byte each
> (low, then high) of the total offset, thereby getting the absolute
> address of image_base loaded in r0.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   arch/arm/lib/crt0_arm_efi.S | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S
> index 91b0fe12c51..235b3a0c48f 100644
> --- a/arch/arm/lib/crt0_arm_efi.S
> +++ b/arch/arm/lib/crt0_arm_efi.S
> @@ -149,7 +149,8 @@ _start:
>   	adr		r1, .L_DYNAMIC
>   	ldr		r0, [r1]
>   	add		r1, r0, r1
> -	adrl		r0, image_base
> +	sub		r0, pc, #((.+8-image_base) & 0xff)
> +	sub		r0, r0, #((.+4-image_base) & 0xff00)

These are the instructions resulting from building qemu_arm_defconfig:

sub     r0, pc, #24
sub     r0, r0, #16, 24 @ 0x1000

The last instruction subtracts 0x1000000.

I can't see how your instructions would be doing the same.

Best regards

Heinrich

>   	bl		_relocate
>   	teq		r0, #0
>   	bne		0f

Re: [PATCH 07/17] arm: Replace 'adrl' in EFI crt0

[Sam Edwards]

On Mon, Feb 24, 2025 at 12:42 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/24/25 06:55, Sam Edwards wrote:
> > LLVM's IAS does not (and cannot easily) support the 'adrl'
> > pseudoinstruction, and ARM developers generally do not consider it
> > portable across assembler implementations either.
> >
> > Instead, expand it into the two subtract instructions it would emit
> > anyway. An explanation of the math follows:
> >
> > The .+8 and .+4 refer to the same memory location; this is because the
>

Hey there Heinrich,

> Are we sure that we never generate thumb instructions here?

Completely positive; the stmfd/ldmfd (at least) instructions do not
have THUMB encodings, so this file can only assemble for ARM.

>
> > .+4 expression occurs in a subsequent instruction, 4 bytes after the
> > first. This memory location is the value of the PC register when it is
> > read by the first sub instruction. Thus, both inner parenthesized
> > expressions evaluate to the same result: PC's offset relative to
> > image_base. The subtract instructions then remove one byte each
> > (low, then high) of the total offset, thereby getting the absolute
> > address of image_base loaded in r0.
> >
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >   arch/arm/lib/crt0_arm_efi.S | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S
> > index 91b0fe12c51..235b3a0c48f 100644
> > --- a/arch/arm/lib/crt0_arm_efi.S
> > +++ b/arch/arm/lib/crt0_arm_efi.S
> > @@ -149,7 +149,8 @@ _start:
> >       adr             r1, .L_DYNAMIC
> >       ldr             r0, [r1]
> >       add             r1, r0, r1
> > -     adrl            r0, image_base
> > +     sub             r0, pc, #((.+8-image_base) & 0xff)
> > +     sub             r0, r0, #((.+4-image_base) & 0xff00)
>
> These are the instructions resulting from building qemu_arm_defconfig:
>
> sub     r0, pc, #24
> sub     r0, r0, #16, 24 @ 0x1000
>
> The last instruction subtracts 0x1000000.

I'd have to see the machine code to be sure, but it looks like that's
the 12-bit immediate value encoding of:
(16 >>> 24)
...which would equal 0x1000, as suggested by the disassembler comment.

Cheers,
Sam

>
> I can't see how your instructions would be doing the same.
>
> Best regards
>
> Heinrich
>
> >       bl              _relocate
> >       teq             r0, #0
> >       bne             0f
>

[PATCH 08/17] x86: Fix call64's section flags

[Sam Edwards]

When a section is not flagged with SHF_ALLOC, LLD's --gc-sections
algorithm fails to visit the sections that it references. As a result of
this, LLD was dropping the call64.o(.data) section, which is itself only
referenced by .text_call64.

This appears to be a bug in LLD, but the .section directive for
.text_call64 should really have the correct flags either way.

Add `"ax"` to mark the section as ALLOC ("supposed to be loaded") and
CODE ("supposed to be executed").

Fixes: 7dc82591d68e2a ("x86: Move call64 into its own section")
Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/x86/cpu/i386/call64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/cpu/i386/call64.S b/arch/x86/cpu/i386/call64.S
index d81bcc6f8f4..424732fa3fa 100644
--- a/arch/x86/cpu/i386/call64.S
+++ b/arch/x86/cpu/i386/call64.S
@@ -10,7 +10,7 @@
 #include <asm/processor-flags.h>
 
 .code32
-.section .text_call64
+.section .text_call64, "ax"
 .globl cpu_call64
 cpu_call64:
 	/*
-- 
2.45.2

[PATCH 09/17] spl: riscv: opensbi: Error on misaligned FDT

[Sam Edwards]

libfdt 1.6.1+ requires the FDT to be 8-byte aligned and returns an error
if not. OpenSBI 1.0+ includes this version of libfdt and will also
reject misaligned FDTs.

However, OpenSBI cannot indicate the error to the user: since it cannot
access the serial console, it can only silently hang. This can be very
difficult to diagnose without proper debugging facilities. Therefore,
give the U-Boot SPL, which *can* print error messages, an additional
check for proper FDT alignment.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 common/spl/spl_opensbi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
index 5a26d7c31a4..0ed6afeacc6 100644
--- a/common/spl/spl_opensbi.c
+++ b/common/spl/spl_opensbi.c
@@ -57,6 +57,11 @@ void __noreturn spl_invoke_opensbi(struct spl_image_info *spl_image)
 		hang();
 	}
 
+	if (!IS_ALIGNED((uintptr_t)spl_image->fdt_addr, 8)) {
+		pr_err("SPL image loaded an improperly-aligned device tree\n");
+		hang();
+	}
+
 	/*
 	 * Originally, u-boot-spl will place DTB directly after the kernel,
 	 * but the size of the kernel did not include the BSS section, which
-- 
2.45.2

Re: [PATCH 09/17] spl: riscv: opensbi: Error on misaligned FDT

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> libfdt 1.6.1+ requires the FDT to be 8-byte aligned and returns an error
> if not. OpenSBI 1.0+ includes this version of libfdt and will also
> reject misaligned FDTs.
>
> However, OpenSBI cannot indicate the error to the user: since it cannot
> access the serial console, it can only silently hang. This can be very
> difficult to diagnose without proper debugging facilities. Therefore,
> give the U-Boot SPL, which *can* print error messages, an additional
> check for proper FDT alignment.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   common/spl/spl_opensbi.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> index 5a26d7c31a4..0ed6afeacc6 100644
> --- a/common/spl/spl_opensbi.c
> +++ b/common/spl/spl_opensbi.c
> @@ -57,6 +57,11 @@ void __noreturn spl_invoke_opensbi(struct spl_image_info *spl_image)
>   		hang();
>   	}
>
> +	if (!IS_ALIGNED((uintptr_t)spl_image->fdt_addr, 8)) {
> +		pr_err("SPL image loaded an improperly-aligned device tree\n");

We only use pr_err() in drivers when we copy code from Linux. Otherwise
use log_err().

As this code is for RISC-V, this patch should have been sent to the
RISC-V maintainers.

cc: Leo, Rick

SPL size is very restricted on boards where it is responsible for
initializing DRAM and therefore has to fit into cache. We should only
add code that is strictly needed to SPL.

What makes you think that this problem can realistically occur?

Best regards

Heinrich

> +		hang();
> +	}
> +
>   	/*
>   	 * Originally, u-boot-spl will place DTB directly after the kernel,
>   	 * but the size of the kernel did not include the BSS section, which

Re: [PATCH 09/17] spl: riscv: opensbi: Error on misaligned FDT

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Mon, Feb 24, 2025 at 09:54:56AM +0100, Heinrich Schuchardt wrote:
> On 2/24/25 06:55, Sam Edwards wrote:
> > libfdt 1.6.1+ requires the FDT to be 8-byte aligned and returns an error
> > if not. OpenSBI 1.0+ includes this version of libfdt and will also
> > reject misaligned FDTs.
> > 
> > However, OpenSBI cannot indicate the error to the user: since it cannot
> > access the serial console, it can only silently hang. This can be very
> > difficult to diagnose without proper debugging facilities. Therefore,
> > give the U-Boot SPL, which *can* print error messages, an additional
> > check for proper FDT alignment.
> > 
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >   common/spl/spl_opensbi.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> > index 5a26d7c31a4..0ed6afeacc6 100644
> > --- a/common/spl/spl_opensbi.c
> > +++ b/common/spl/spl_opensbi.c
> > @@ -57,6 +57,11 @@ void __noreturn spl_invoke_opensbi(struct spl_image_info *spl_image)
> >   		hang();
> >   	}
> > 
> > +	if (!IS_ALIGNED((uintptr_t)spl_image->fdt_addr, 8)) {
> > +		pr_err("SPL image loaded an improperly-aligned device tree\n");
> 
> We only use pr_err() in drivers when we copy code from Linux. Otherwise
> use log_err().
> 
> As this code is for RISC-V, this patch should have been sent to the
> RISC-V maintainers.
> 
> cc: Leo, Rick
> 
> SPL size is very restricted on boards where it is responsible for
> initializing DRAM and therefore has to fit into cache. We should only
> add code that is strictly needed to SPL.
> 
> What makes you think that this problem can realistically occur?

I would like to know if Sam hit this in practice as well. But we have
had more than one place where because we don't ensure 8-byte alignment
and have use-in-place, that we've broken things. Picking up one of the
patches that would address this within U-Boot is on my list now that
we've had confirmation of some padding command that's sufficiently
portable.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 09/17] spl: riscv: opensbi: Error on misaligned FDT

[Sam Edwards]

On Mon, Feb 24, 2025 at 6:54 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Feb 24, 2025 at 09:54:56AM +0100, Heinrich Schuchardt wrote:
> > On 2/24/25 06:55, Sam Edwards wrote:
> > > libfdt 1.6.1+ requires the FDT to be 8-byte aligned and returns an error
> > > if not. OpenSBI 1.0+ includes this version of libfdt and will also
> > > reject misaligned FDTs.
> > >
> > > However, OpenSBI cannot indicate the error to the user: since it cannot
> > > access the serial console, it can only silently hang. This can be very
> > > difficult to diagnose without proper debugging facilities. Therefore,
> > > give the U-Boot SPL, which *can* print error messages, an additional
> > > check for proper FDT alignment.
> > >
> > > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > > ---
> > >   common/spl/spl_opensbi.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> > > index 5a26d7c31a4..0ed6afeacc6 100644
> > > --- a/common/spl/spl_opensbi.c
> > > +++ b/common/spl/spl_opensbi.c
> > > @@ -57,6 +57,11 @@ void __noreturn spl_invoke_opensbi(struct spl_image_info *spl_image)
> > >             hang();
> > >     }
> > >
> > > +   if (!IS_ALIGNED((uintptr_t)spl_image->fdt_addr, 8)) {
> > > +           pr_err("SPL image loaded an improperly-aligned device tree\n");
> >
> > We only use pr_err() in drivers when we copy code from Linux. Otherwise
> > use log_err().
> >
> > As this code is for RISC-V, this patch should have been sent to the
> > RISC-V maintainers.
> >
> > cc: Leo, Rick
> >
> > SPL size is very restricted on boards where it is responsible for
> > initializing DRAM and therefore has to fit into cache. We should only
> > add code that is strictly needed to SPL.
> >
> > What makes you think that this problem can realistically occur?
>
> I would like to know if Sam hit this in practice as well. But we have
> had more than one place where because we don't ensure 8-byte alignment
> and have use-in-place, that we've broken things. Picking up one of the
> patches that would address this within U-Boot is on my list now that
> we've had confirmation of some padding command that's sufficiently
> portable.

Hi gents,

I have indeed hit this in practice; I will elaborate how in my reply
to patch 10/17. But this whole series is sanding down various sharp
corners that snagged me while trying to build with LLVM. :)

I agree with the aim of conserving space in the SPL, but since I spent
a fair amount of time debugging why OpenSBI was hanging, I believe the
need for a sanity check on the assumption of 8-byte alignment *before*
passing control to OpenSBI is the greater concern. I would be more
favorable to putting an assert in the common SPL code; my only aim
here is to catch this problem with a diagnosable error message, as I
suspect we have not seen the last of it.

Cheers,
Sam

>
> --
> Tom

[PATCH 10/17] spl: Align FDT load address

[Sam Edwards]

While the image size is generally a multiple of 8 bytes, this is not
actually guaranteed; some linkers (like LLD) may shave a few bytes off
of the end of output sections if there are no content bytes there. Since
libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
explicit about the alignment when loading the FDT.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 common/spl/spl_fit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 49b4df60560..86506d6905c 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	 * Use the address following the image as target address for the
 	 * device tree.
 	 */
-	image_info.load_addr = spl_image->load_addr + spl_image->size;
+	image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
 
 	/* Figure out which device tree the board wants to use */
 	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
-- 
2.45.2

Re: [PATCH 10/17] spl: Align FDT load address

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> While the image size is generally a multiple of 8 bytes, this is not
> actually guaranteed; some linkers (like LLD) may shave a few bytes off
> of the end of output sections if there are no content bytes there. Since
> libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
> explicit about the alignment when loading the FDT.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   common/spl/spl_fit.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 49b4df60560..86506d6905c 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>   	 * Use the address following the image as target address for the
>   	 * device tree.
>   	 */
> -	image_info.load_addr = spl_image->load_addr + spl_image->size;
> +	image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);

We want to keep the SPL code size as small as possible as on many
platforms it is restricted to the cache size.

Can't we fix this linker issue in the linker script by properly aligning
the SPL image end address?

Best regards

Heinrich

>
>   	/* Figure out which device tree the board wants to use */
>   	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);

Re: [PATCH 10/17] spl: Align FDT load address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On Mon, Feb 24, 2025 at 09:59:42AM +0100, Heinrich Schuchardt wrote:
> On 2/24/25 06:55, Sam Edwards wrote:
> > While the image size is generally a multiple of 8 bytes, this is not
> > actually guaranteed; some linkers (like LLD) may shave a few bytes off
> > of the end of output sections if there are no content bytes there. Since
> > libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
> > explicit about the alignment when loading the FDT.
> > 
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >   common/spl/spl_fit.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > index 49b4df60560..86506d6905c 100644
> > --- a/common/spl/spl_fit.c
> > +++ b/common/spl/spl_fit.c
> > @@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
> >   	 * Use the address following the image as target address for the
> >   	 * device tree.
> >   	 */
> > -	image_info.load_addr = spl_image->load_addr + spl_image->size;
> > +	image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
> 
> We want to keep the SPL code size as small as possible as on many
> platforms it is restricted to the cache size.
> 
> Can't we fix this linker issue in the linker script by properly aligning
> the SPL image end address?

Size growth is always something to watch for, but not at the expense of
correctness and saving a few bytes. We really do need to fix the places
where U-Boot could but doesn't ensure the device tree is correctly
aligned in memory.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 10/17] spl: Align FDT load address

[Heinrich Schuchardt]

On 24.02.25 15:56, Tom Rini wrote:
> On Mon, Feb 24, 2025 at 09:59:42AM +0100, Heinrich Schuchardt wrote:
>> On 2/24/25 06:55, Sam Edwards wrote:
>>> While the image size is generally a multiple of 8 bytes, this is not
>>> actually guaranteed; some linkers (like LLD) may shave a few bytes off
>>> of the end of output sections if there are no content bytes there. Since
>>> libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
>>> explicit about the alignment when loading the FDT.
>>>
>>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
>>> ---
>>>    common/spl/spl_fit.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index 49b4df60560..86506d6905c 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>>>    	 * Use the address following the image as target address for the
>>>    	 * device tree.
>>>    	 */
>>> -	image_info.load_addr = spl_image->load_addr + spl_image->size;
>>> +	image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
>>
>> We want to keep the SPL code size as small as possible as on many
>> platforms it is restricted to the cache size.
>>
>> Can't we fix this linker issue in the linker script by properly aligning
>> the SPL image end address?
>
> Size growth is always something to watch for, but not at the expense of
> correctness and saving a few bytes. We really do need to fix the places
> where U-Boot could but doesn't ensure the device tree is correctly
> aligned in memory.
>

Hello Tom,

spl_image->load_addr is always a multiple of 8.

Adding

    . = ALIGN(8)"

in arch/riscv/cpu/u-boot-spl.lds before

     _end = .;
     _image_binary_end = .;

is all it takes.

Best regards

Heinrich

Re: [PATCH 10/17] spl: Align FDT load address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]

On Mon, Feb 24, 2025 at 04:54:09PM +0100, Heinrich Schuchardt wrote:
> On 24.02.25 15:56, Tom Rini wrote:
> > On Mon, Feb 24, 2025 at 09:59:42AM +0100, Heinrich Schuchardt wrote:
> > > On 2/24/25 06:55, Sam Edwards wrote:
> > > > While the image size is generally a multiple of 8 bytes, this is not
> > > > actually guaranteed; some linkers (like LLD) may shave a few bytes off
> > > > of the end of output sections if there are no content bytes there. Since
> > > > libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
> > > > explicit about the alignment when loading the FDT.
> > > > 
> > > > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > > > ---
> > > >    common/spl/spl_fit.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > > > index 49b4df60560..86506d6905c 100644
> > > > --- a/common/spl/spl_fit.c
> > > > +++ b/common/spl/spl_fit.c
> > > > @@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
> > > >    	 * Use the address following the image as target address for the
> > > >    	 * device tree.
> > > >    	 */
> > > > -	image_info.load_addr = spl_image->load_addr + spl_image->size;
> > > > +	image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
> > > 
> > > We want to keep the SPL code size as small as possible as on many
> > > platforms it is restricted to the cache size.
> > > 
> > > Can't we fix this linker issue in the linker script by properly aligning
> > > the SPL image end address?
> > 
> > Size growth is always something to watch for, but not at the expense of
> > correctness and saving a few bytes. We really do need to fix the places
> > where U-Boot could but doesn't ensure the device tree is correctly
> > aligned in memory.
> > 
> 
> Hello Tom,
> 
> spl_image->load_addr is always a multiple of 8.
> 
> Adding
> 
>    . = ALIGN(8)"
> 
> in arch/riscv/cpu/u-boot-spl.lds before
> 
>     _end = .;
>     _image_binary_end = .;
> 
> is all it takes.

But this is generic code and I don't see how we know that in every case
every way we could be reading the device tree that it will be at an 8
byte aligned location. There's a number of ways today where it's not,
which is what Patrice found as part of updating our own libfdt to one
that enforces the alignment check.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 10/17] spl: Align FDT load address

[Sam Edwards]

On Mon, Feb 24, 2025 at 8:03 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Feb 24, 2025 at 04:54:09PM +0100, Heinrich Schuchardt wrote:
> > On 24.02.25 15:56, Tom Rini wrote:
> > > On Mon, Feb 24, 2025 at 09:59:42AM +0100, Heinrich Schuchardt wrote:
> > > > On 2/24/25 06:55, Sam Edwards wrote:
> > > > > While the image size is generally a multiple of 8 bytes, this is not
> > > > > actually guaranteed; some linkers (like LLD) may shave a few bytes off
> > > > > of the end of output sections if there are no content bytes there. Since
> > > > > libfdt imposes a hard rule of 8-byte alignment, make the SPL also be
> > > > > explicit about the alignment when loading the FDT.
> > > > >
> > > > > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > > > > ---
> > > > >    common/spl/spl_fit.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > > > > index 49b4df60560..86506d6905c 100644
> > > > > --- a/common/spl/spl_fit.c
> > > > > +++ b/common/spl/spl_fit.c
> > > > > @@ -397,7 +397,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
> > > > >          * Use the address following the image as target address for the
> > > > >          * device tree.
> > > > >          */
> > > > > -       image_info.load_addr = spl_image->load_addr + spl_image->size;
> > > > > +       image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
> > > >
> > > > We want to keep the SPL code size as small as possible as on many
> > > > platforms it is restricted to the cache size.
> > > >
> > > > Can't we fix this linker issue in the linker script by properly aligning
> > > > the SPL image end address?
> > >
> > > Size growth is always something to watch for, but not at the expense of
> > > correctness and saving a few bytes. We really do need to fix the places
> > > where U-Boot could but doesn't ensure the device tree is correctly
> > > aligned in memory.
> > >
> >
> > Hello Tom,
> >

Hi gents,

> > spl_image->load_addr is always a multiple of 8.

So while spl_image->load_addr is 8-aligned, spl_image->size appears to
have no such guarantees.

When loaded from FIT, the size is simply the data length of the U-Boot
image, which is itself the filesize of `u-boot-nodtb.bin`. That ends
up being determined by the whims of the toolchain (ld+objcopy).

> >
> > Adding
> >
> >    . = ALIGN(8)"
> >
> > in arch/riscv/cpu/u-boot-spl.lds before
> >
> >     _end = .;
> >     _image_binary_end = .;
> >
> > is all it takes.

While that guarantees that `_end` and `_image_binary_end` are aligned
on an 8-byte boundary, that does not necessarily mean that sufficient
padding bytes will be emitted, so the `u-boot-notdb.bin` file may
still end immediately after the last included content byte.

If there is a hard rule that the size must be a multiple of 8, we
should probably be enforcing/padding it when the FIT is generated.
(And then, a comment here in spl_fit.c explaining that it is
guaranteeing the alignment by depending on load_addr/size assumptions
would go a long way toward keeping those assumptions valid.) But if
that rule does not exist, and it's actually valid for the size to be
whatever odd length, then this patch is necessary for correctness.

> But this is generic code and I don't see how we know that in every case
> every way we could be reading the device tree that it will be at an 8
> byte aligned location. There's a number of ways today where it's not,
> which is what Patrice found as part of updating our own libfdt to one
> that enforces the alignment check.

I mentioned in my reply to patch 9/17 of this series that I would also
be fine with a common assert that catches any misaligned FDTs before
they are passed to third-party components for consumption. I suspect
the main reason that many of these cases are slipping through is
because some of those components are tolerant of misaligned FDTs, and
if a developer is only testing against those then they won't notice
the misalignment. Putting something in the unconditional code path of
the SPL that enforces the FDT alignment ought to put a stop to that.

Thoughts?

Cheers,
Sam

>
> --
> Tom

[PATCH 11/17] makefile: Fix symbol typo in binary_size_check

[Sam Edwards]

The start-of-image marker symbol is `__image_copy_start`; by searching
for `_image_copy_start` instead, this check can accidentally match
`_image_copy_start_ofs`.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b32606b69f5..a9194b46ae1 100644
--- a/Makefile
+++ b/Makefile
@@ -1314,7 +1314,7 @@ binary_size_check: u-boot-nodtb.bin FORCE
 	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{ print $$1 }') ; \
 	map_size=$(shell cat u-boot.map | \
 		awk ' \
-			/_image_copy_start/ { start = $$1 } \
+			/__image_copy_start/ { start = $$1 } \
 			/_image_binary_end/ { end = $$1 } \
 			END { \
 				if (start != "" && end != "") \
-- 
2.45.2

Re: [PATCH 11/17] makefile: Fix symbol typo in binary_size_check

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> The start-of-image marker symbol is `__image_copy_start`; by searching
> for `_image_copy_start` instead, this check can accidentally match
> `_image_copy_start_ofs`.

_image_copy_start_ofs seems to be the only match for
git grep -n '[^_]_image_copy_start'.

Can _image_copy_start_ofs seems ever occur before __image_copy_start?

>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index b32606b69f5..a9194b46ae1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1314,7 +1314,7 @@ binary_size_check: u-boot-nodtb.bin FORCE
>   	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{ print $$1 }') ; \
>   	map_size=$(shell cat u-boot.map | \
>   		awk ' \
> -			/_image_copy_start/ { start = $$1 } \
> +			/__image_copy_start/ { start = $$1 } \

Most architectures don't define __image_copy_start so this code looks
broken. Please, provide a solution that works on all architectures.

Best regards

Heinrich

>   			/_image_binary_end/ { end = $$1 } \
>   			END { \
>   				if (start != "" && end != "") \

Re: [PATCH 11/17] makefile: Fix symbol typo in binary_size_check

[Simon Glass]

Hi Sam,

On Mon, 24 Feb 2025 at 02:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/24/25 06:55, Sam Edwards wrote:
> > The start-of-image marker symbol is `__image_copy_start`; by searching
> > for `_image_copy_start` instead, this check can accidentally match
> > `_image_copy_start_ofs`.
>
> _image_copy_start_ofs seems to be the only match for
> git grep -n '[^_]_image_copy_start'.
>
> Can _image_copy_start_ofs seems ever occur before __image_copy_start?
>
> >
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >   Makefile | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index b32606b69f5..a9194b46ae1 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1314,7 +1314,7 @@ binary_size_check: u-boot-nodtb.bin FORCE
> >       @file_size=$(shell wc -c u-boot-nodtb.bin | awk '{ print $$1 }') ; \
> >       map_size=$(shell cat u-boot.map | \
> >               awk ' \
> > -                     /_image_copy_start/ { start = $$1 } \
> > +                     /__image_copy_start/ { start = $$1 } \
>
> Most architectures don't define __image_copy_start so this code looks
> broken. Please, provide a solution that works on all architectures.

This is existing code, so there is no requirement to rewrite it just
to get this patch in. Please review the patch rather than the original
code.

Sam, if you wish you take on some yak-shaving then the .lds symbols
across all architectures could be unified.

>
> >                       /_image_binary_end/ { end = $$1 } \
> >                       END { \
> >                               if (start != "" && end != "") \
>

Regards,
Simon

[PATCH 12/17] makefile: Avoid objcopy --gap-fill for .hex/.srec

[Sam Edwards]

This flag only makes sense for `binary` output, because .hex/.srec are
sparse formats and represent gaps without filler. While the GNU binutils
version of objcopy does not seem to mind the extra flag being passed,
llvm-objcopy considers this a fatal error.

There is already a version of the objcopy command template in the
Makefile that doesn't use --gap-fill, which is provided for EFI. So use
this other version for all .hex/.srec outputs as well.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index a9194b46ae1..87b07d8989a 100644
--- a/Makefile
+++ b/Makefile
@@ -1067,7 +1067,7 @@ quiet_cmd_objcopy = OBJCOPY $@
 cmd_objcopy = $(OBJCOPY) --gap-fill=0xff $(OBJCOPYFLAGS) \
 	$(OBJCOPYFLAGS_$(@F)) $< $@
 
-# Provide a version which does not do this, for use by EFI
+# Provide a version which does not do this, for use by EFI and hex/srec
 quiet_cmd_zobjcopy = OBJCOPY $@
 cmd_zobjcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $(OBJCOPYFLAGS_$(@F)) $< $@
 
@@ -1282,7 +1282,7 @@ OBJCOPYFLAGS_u-boot.hex := -O ihex
 OBJCOPYFLAGS_u-boot.srec := -O srec
 
 u-boot.hex u-boot.srec: u-boot FORCE
-	$(call if_changed,objcopy)
+	$(call if_changed,zobjcopy)
 
 OBJCOPYFLAGS_u-boot-elf.srec := $(OBJCOPYFLAGS_u-boot.srec)
 
@@ -1296,12 +1296,12 @@ OBJCOPYFLAGS_u-boot-elf.srec += --change-addresses=0x50000000
 endif
 
 u-boot-elf.srec: u-boot.elf FORCE
-	$(call if_changed,objcopy)
+	$(call if_changed,zobjcopy)
 
 OBJCOPYFLAGS_u-boot-spl.srec = $(OBJCOPYFLAGS_u-boot.srec)
 
 spl/u-boot-spl.srec: spl/u-boot-spl FORCE
-	$(call if_changed,objcopy)
+	$(call if_changed,zobjcopy)
 
 %.scif: %.srec
 	$(Q)$(MAKE) $(build)=arch/arm/mach-renesas $@
@@ -1436,7 +1436,7 @@ OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex
 OBJCOPYFLAGS_u-boot.ldr.srec := -I binary -O srec
 
 u-boot.ldr.hex u-boot.ldr.srec: u-boot.ldr FORCE
-	$(call if_changed,objcopy)
+	$(call if_changed,zobjcopy)
 
 ifdef CONFIG_SPL_LOAD_FIT
 MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
-- 
2.45.2

[PATCH 13/17] makefile: Add `norelro` linker option

[Sam Edwards]

RELRO is an instruction to a dynamic loader to make a memory range
read-only after relocations are applied, for added security. Some
linkers (e.g. LLD) require that all sections covered by the RELRO are
contiguous, so that only a single RELRO is needed. U-Boot at present
neither satisfies this requirement (e.g. x86_64 linker script currently
puts .dynamic too far from .got) nor preserves the RELRO when converting
away from ELF, therefore add `-z norelro` to global linker options.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 87b07d8989a..c869b5c55fa 100644
--- a/Makefile
+++ b/Makefile
@@ -820,6 +820,7 @@ KBUILD_AFLAGS += $(KAFLAGS)
 KBUILD_CFLAGS += $(KCFLAGS)
 
 KBUILD_LDFLAGS  += -z noexecstack
+KBUILD_LDFLAGS  += -z norelro
 KBUILD_LDFLAGS  += $(call ld-option,--no-warn-rwx-segments)
 
 KBUILD_HOSTCFLAGS += $(if $(CONFIG_TOOLS_DEBUG),-g)
-- 
2.45.2

Re: [PATCH 13/17] makefile: Add `norelro` linker option

[Ilias Apalodimas]

On Mon, 24 Feb 2025 at 07:56, Sam Edwards <cfsworks@gmail.com> wrote:
>
> RELRO is an instruction to a dynamic loader to make a memory range
> read-only after relocations are applied, for added security. Some
> linkers (e.g. LLD) require that all sections covered by the RELRO are
> contiguous, so that only a single RELRO is needed. U-Boot at present
> neither satisfies this requirement (e.g. x86_64 linker script currently
> puts .dynamic too far from .got) nor preserves the RELRO when converting
> away from ELF, therefore add `-z norelro` to global linker options.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index 87b07d8989a..c869b5c55fa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -820,6 +820,7 @@ KBUILD_AFLAGS += $(KAFLAGS)
>  KBUILD_CFLAGS += $(KCFLAGS)
>
>  KBUILD_LDFLAGS  += -z noexecstack
> +KBUILD_LDFLAGS  += -z norelro
>  KBUILD_LDFLAGS  += $(call ld-option,--no-warn-rwx-segments)
>
>  KBUILD_HOSTCFLAGS += $(if $(CONFIG_TOOLS_DEBUG),-g)
> --
> 2.45.2
>

I guess we can bring it back eventually when we clean up the linkler
scripts more

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 14/17] makefile: Add READELF command variable

[Sam Edwards]

This allows setting READELF=llvm-readelf in order to use the LLVM
version of the readelf utility. It also aligns with the practice of not
using $(CROSS_COMPILE) in any build recipes directly, reducing the
number of places where $(CROSS_COMPILE) is used.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c869b5c55fa..c1749e78d6f 100644
--- a/Makefile
+++ b/Makefile
@@ -406,6 +406,7 @@ LDR		= $(CROSS_COMPILE)ldr
 STRIP		= $(CROSS_COMPILE)strip
 OBJCOPY		= $(CROSS_COMPILE)objcopy
 OBJDUMP		= $(CROSS_COMPILE)objdump
+READELF		= $(CROSS_COMPILE)readelf
 LEX		= flex
 YACC		= bison
 AWK		= awk
@@ -2177,7 +2178,7 @@ System.map:	u-boot
 # ARM relocations should all be R_ARM_RELATIVE (32-bit) or
 # R_AARCH64_RELATIVE (64-bit).
 checkarmreloc: u-boot
-	@RELOC="`$(CROSS_COMPILE)readelf -r -W $< | cut -d ' ' -f 4 | \
+	@RELOC="`$(READELF) -r -W $< | cut -d ' ' -f 4 | \
 		grep R_A | sort -u`"; \
 	if test "$$RELOC" != "R_ARM_RELATIVE" -a \
 		 "$$RELOC" != "R_AARCH64_RELATIVE"; then \
-- 
2.45.2

Re: [PATCH 14/17] makefile: Add READELF command variable

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> This allows setting READELF=llvm-readelf in order to use the LLVM
> version of the readelf utility. It also aligns with the practice of not
> using $(CROSS_COMPILE) in any build recipes directly, reducing the
> number of places where $(CROSS_COMPILE) is used.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   Makefile | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index c869b5c55fa..c1749e78d6f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -406,6 +406,7 @@ LDR		= $(CROSS_COMPILE)ldr
>   STRIP		= $(CROSS_COMPILE)strip
>   OBJCOPY		= $(CROSS_COMPILE)objcopy
>   OBJDUMP		= $(CROSS_COMPILE)objdump
> +READELF		= $(CROSS_COMPILE)readelf
>   LEX		= flex
>   YACC		= bison
>   AWK		= awk
> @@ -2177,7 +2178,7 @@ System.map:	u-boot
>   # ARM relocations should all be R_ARM_RELATIVE (32-bit) or
>   # R_AARCH64_RELATIVE (64-bit).
>   checkarmreloc: u-boot
> -	@RELOC="`$(CROSS_COMPILE)readelf -r -W $< | cut -d ' ' -f 4 | \
> +	@RELOC="`$(READELF) -r -W $< | cut -d ' ' -f 4 | \
>   		grep R_A | sort -u`"; \
>   	if test "$$RELOC" != "R_ARM_RELATIVE" -a \
>   		 "$$RELOC" != "R_AARCH64_RELATIVE"; then \

Re: [PATCH 14/17] makefile: Add READELF command variable

[Ilias Apalodimas]

On Mon, 24 Feb 2025 at 07:56, Sam Edwards <cfsworks@gmail.com> wrote:
>
> This allows setting READELF=llvm-readelf in order to use the LLVM
> version of the readelf utility. It also aligns with the practice of not
> using $(CROSS_COMPILE) in any build recipes directly, reducing the
> number of places where $(CROSS_COMPILE) is used.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index c869b5c55fa..c1749e78d6f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -406,6 +406,7 @@ LDR         = $(CROSS_COMPILE)ldr
>  STRIP          = $(CROSS_COMPILE)strip
>  OBJCOPY                = $(CROSS_COMPILE)objcopy
>  OBJDUMP                = $(CROSS_COMPILE)objdump
> +READELF                = $(CROSS_COMPILE)readelf
>  LEX            = flex
>  YACC           = bison
>  AWK            = awk
> @@ -2177,7 +2178,7 @@ System.map:       u-boot
>  # ARM relocations should all be R_ARM_RELATIVE (32-bit) or
>  # R_AARCH64_RELATIVE (64-bit).
>  checkarmreloc: u-boot
> -       @RELOC="`$(CROSS_COMPILE)readelf -r -W $< | cut -d ' ' -f 4 | \
> +       @RELOC="`$(READELF) -r -W $< | cut -d ' ' -f 4 | \
>                 grep R_A | sort -u`"; \
>         if test "$$RELOC" != "R_ARM_RELATIVE" -a \
>                  "$$RELOC" != "R_AARCH64_RELATIVE"; then \
> --
> 2.45.2
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 15/17] efi_loader: Remove ENTRY(_start) from linker script

[Sam Edwards]

The EFI apps are built using ELF only as an intermediate "linker output"
format, with the final PE header crafted within the ELF to take effect
after the objcopy conversion to raw binary. As such, we really don't
care what the ELF header indicates as the entry point.

Because nothing that uses this linker script even defines a _start
symbol, this directive has no effect. On GNU ld, it is silently ignored.
On certain linkers (e.g. LLVM lld), a warning is logged. But in any
case, no entry point is set.

Therefore, remove it to silence warnings and avoid confusion.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 lib/efi_loader/elf_efi.ldsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/efi_loader/elf_efi.ldsi b/lib/efi_loader/elf_efi.ldsi
index 190a88fb69e..dc4d1a3660e 100644
--- a/lib/efi_loader/elf_efi.ldsi
+++ b/lib/efi_loader/elf_efi.ldsi
@@ -10,7 +10,6 @@ PHDRS
 	data PT_LOAD FLAGS(3); /* SHF_WRITE | SHF_ALLOC */
 }
 
-ENTRY(_start)
 SECTIONS
 {
 	.text 0x0 : {
-- 
2.45.2

Re: [PATCH 15/17] efi_loader: Remove ENTRY(_start) from linker script

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> The EFI apps are built using ELF only as an intermediate "linker output"
> format, with the final PE header crafted within the ELF to take effect
> after the objcopy conversion to raw binary. As such, we really don't
> care what the ELF header indicates as the entry point.
>
> Because nothing that uses this linker script even defines a _start
> symbol, this directive has no effect. On GNU ld, it is silently ignored.
> On certain linkers (e.g. LLVM lld), a warning is logged. But in any
> case, no entry point is set.

I can understand the first part of your reasoning. But the statement
about _start is wrong. _start is defined on all EFI architectures, e.g.

arch/arm/lib/crt0_aarch64_efi.S:147:
_start

arch/riscv/lib/crt0_riscv_efi.S:182:
_start:

Why does lld complain? Is it because we lack

	.globl _start

on ARM and RISC-V?

Best regards

Heinrich

>
> Therefore, remove it to silence warnings and avoid confusion.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   lib/efi_loader/elf_efi.ldsi | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/lib/efi_loader/elf_efi.ldsi b/lib/efi_loader/elf_efi.ldsi
> index 190a88fb69e..dc4d1a3660e 100644
> --- a/lib/efi_loader/elf_efi.ldsi
> +++ b/lib/efi_loader/elf_efi.ldsi
> @@ -10,7 +10,6 @@ PHDRS
>   	data PT_LOAD FLAGS(3); /* SHF_WRITE | SHF_ALLOC */
>   }
>
> -ENTRY(_start)
>   SECTIONS
>   {
>   	.text 0x0 : {

Re: [PATCH 15/17] efi_loader: Remove ENTRY(_start) from linker script

[Sam Edwards]

On Mon, Feb 24, 2025 at 1:37 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/24/25 06:55, Sam Edwards wrote:
> > The EFI apps are built using ELF only as an intermediate "linker output"
> > format, with the final PE header crafted within the ELF to take effect
> > after the objcopy conversion to raw binary. As such, we really don't
> > care what the ELF header indicates as the entry point.
> >
> > Because nothing that uses this linker script even defines a _start
> > symbol, this directive has no effect. On GNU ld, it is silently ignored.
> > On certain linkers (e.g. LLVM lld), a warning is logged. But in any
> > case, no entry point is set.
>
> I can understand the first part of your reasoning. But the statement
> about _start is wrong. _start is defined on all EFI architectures, e.g.
>
> arch/arm/lib/crt0_aarch64_efi.S:147:
> _start
>
> arch/riscv/lib/crt0_riscv_efi.S:182:
> _start:
>
> Why does lld complain? Is it because we lack
>
>         .globl _start
>
> on ARM and RISC-V?

Hi Heinrich,

Yeah, pretty much. The .globl _start directive is required to turn
_start into a global symbol, rather than just a local label (which
might as well not even exist, as far as the linker is concerned).

I admit I only grepped in lib/efi* at first, but after looking in
arch/ it doesn't seem like anything creates this entry point for that
linker script to use (x86/64 do, but they have their own lds).

Cheers,
Sam

>
> Best regards
>
> Heinrich
>
> >
> > Therefore, remove it to silence warnings and avoid confusion.
> >
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >   lib/efi_loader/elf_efi.ldsi | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/elf_efi.ldsi b/lib/efi_loader/elf_efi.ldsi
> > index 190a88fb69e..dc4d1a3660e 100644
> > --- a/lib/efi_loader/elf_efi.ldsi
> > +++ b/lib/efi_loader/elf_efi.ldsi
> > @@ -10,7 +10,6 @@ PHDRS
> >       data PT_LOAD FLAGS(3); /* SHF_WRITE | SHF_ALLOC */
> >   }
> >
> > -ENTRY(_start)
> >   SECTIONS
> >   {
> >       .text 0x0 : {
>

[PATCH 16/17] efi_loader: Move .dynamic out of .text in EFI

[Sam Edwards]

This was not proper: A .text section is SHT_PROGBITS, while the .dynamic
section is SHT_DYNAMIC. Attempting to combine them like this creates a
section type mismatch.

It seems that while GNU ld does not complain, LLVM's lld considers this
an error.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/efi_loader/elf_efi.ldsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/elf_efi.ldsi b/lib/efi_loader/elf_efi.ldsi
index dc4d1a3660e..acf02eba4d6 100644
--- a/lib/efi_loader/elf_efi.ldsi
+++ b/lib/efi_loader/elf_efi.ldsi
@@ -20,10 +20,10 @@ SECTIONS
 		*(.gnu.linkonce.t.*)
 		*(.srodata)
 		*(.rodata*)
-		. = ALIGN(16);
-		*(.dynamic);
-		. = ALIGN(512);
 	}
+	. = ALIGN(16);
+	.dynamic : { *(.dynamic) }
+	. = ALIGN(512);
 	.rela.dyn : { *(.rela.dyn) }
 	.rela.plt : { *(.rela.plt) }
 	.rela.got : { *(.rela.got) }
-- 
2.45.2

Re: [PATCH 16/17] efi_loader: Move .dynamic out of .text in EFI

[Heinrich Schuchardt]

Sam Edwards <cfsworks@gmail.com> schrieb am Mo., 24. Feb. 2025, 06:56:

> This was not proper: A .text section is SHT_PROGBITS, while the .dynamic
> section is SHT_DYNAMIC. Attempting to combine them like this creates a
> section type mismatch.
>
> It seems that while GNU ld does not complain, LLVM's lld considers this
> an error.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_loader/elf_efi.ldsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_loader/elf_efi.ldsi b/lib/efi_loader/elf_efi.ldsi
> index dc4d1a3660e..acf02eba4d6 100644
> --- a/lib/efi_loader/elf_efi.ldsi
> +++ b/lib/efi_loader/elf_efi.ldsi
> @@ -20,10 +20,10 @@ SECTIONS
>                 *(.gnu.linkonce.t.*)
>                 *(.srodata)
>                 *(.rodata*)
> -               . = ALIGN(16);
> -               *(.dynamic);
> -               . = ALIGN(512);
>         }
> +       . = ALIGN(16);
> +       .dynamic : { *(.dynamic) }
>

.dynamic is related to dynamic linking of libraries.

Under which circumstances would we have content in .dynamic? Why would it
be needed for executing an EFI application?

Best regards

Heinrich


+       . = ALIGN(512);
>         .rela.dyn : { *(.rela.dyn) }
>         .rela.plt : { *(.rela.plt) }
>         .rela.got : { *(.rela.got) }
> --
> 2.45.2
>
>

Re: [PATCH 16/17] efi_loader: Move .dynamic out of .text in EFI

[Sam Edwards]

On Sun, Feb 23, 2025 at 11:34 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> Sam Edwards <cfsworks@gmail.com> schrieb am Mo., 24. Feb. 2025, 06:56:
>>
>> This was not proper: A .text section is SHT_PROGBITS, while the .dynamic
>> section is SHT_DYNAMIC. Attempting to combine them like this creates a
>> section type mismatch.
>>
>> It seems that while GNU ld does not complain, LLVM's lld considers this
>> an error.
>>
>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>  lib/efi_loader/elf_efi.ldsi | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/efi_loader/elf_efi.ldsi b/lib/efi_loader/elf_efi.ldsi
>> index dc4d1a3660e..acf02eba4d6 100644
>> --- a/lib/efi_loader/elf_efi.ldsi
>> +++ b/lib/efi_loader/elf_efi.ldsi
>> @@ -20,10 +20,10 @@ SECTIONS
>>                 *(.gnu.linkonce.t.*)
>>                 *(.srodata)
>>                 *(.rodata*)
>> -               . = ALIGN(16);
>> -               *(.dynamic);
>> -               . = ALIGN(512);
>>         }
>> +       . = ALIGN(16);
>> +       .dynamic : { *(.dynamic) }
>
>
> .dynamic is related to dynamic linking of libraries.
>
> Under which circumstances would we have content in .dynamic? Why would it be needed for executing an EFI application?

Hey Heinrich,

Apologies for the late response; I didn't have sufficient time/tuits
to get to this until now.

While EFI applications are indeed statically linked, they need to be
relocatable. The .dynamic section contains the ELF dynamic table
(exposed via the _DYNAMIC symbol), which is passed to _relocate by the
crt0_[arch]_efi.S stubs. _relocate finds the relocations table by
querying the DT_RELA/_RELASZ/_RELAENT entries in the dynamic table.
Normally, the PE loader handles relocation of EFI applications before
transferring control by applying the PE relocations in .reloc, but
generating PE-format relocations would require some toolchain
trickery. Hence the ELF dynamic information (and relocations) in a PE
file. :)

Happy Monday,
Sam

>
> Best regards
>
> Heinrich
>
>
>> +       . = ALIGN(512);
>>         .rela.dyn : { *(.rela.dyn) }
>>         .rela.plt : { *(.rela.plt) }
>>         .rela.got : { *(.rela.got) }
>> --
>> 2.45.2
>>

[PATCH 17/17] scripts/Makefile.lib: efi: Preserve the .dynstr section as well

[Sam Edwards]

This section is required by .dynamic and llvm-objcopy will exit with a
fatal error if it is not also preserved in the output.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 scripts/Makefile.lib | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 18993435eae..275c308154b 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -513,8 +513,8 @@ $(obj)/%_efi.S: $(obj)/%.efi
 	$(call cmd,S_efi)
 
 quiet_cmd_efi_objcopy = OBJCOPY $@
-cmd_efi_objcopy = $(OBJCOPY) -j .header -j .text -j .sdata -j .data -j \
-		.dynamic -j .dynsym  -j .rel* -j .rela* -j .reloc \
+cmd_efi_objcopy = $(OBJCOPY) -j .header -j .text -j .sdata -j .data \
+		-j .dynamic -j .dynstr  -j .dynsym -j .rel* -j .reloc \
 		$(if $(EFI_TARGET),$(EFI_TARGET),-O binary) $^ $@
 
 $(obj)/%.efi: $(obj)/%_efi.so
-- 
2.45.2

Re: [PATCH 17/17] scripts/Makefile.lib: efi: Preserve the .dynstr section as well

[Heinrich Schuchardt]

On 2/24/25 06:55, Sam Edwards wrote:
> This section is required by .dynamic and llvm-objcopy will exit with a
> fatal error if it is not also preserved in the output.
>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   scripts/Makefile.lib | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 18993435eae..275c308154b 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -513,8 +513,8 @@ $(obj)/%_efi.S: $(obj)/%.efi
>   	$(call cmd,S_efi)
>
>   quiet_cmd_efi_objcopy = OBJCOPY $@
> -cmd_efi_objcopy = $(OBJCOPY) -j .header -j .text -j .sdata -j .data -j \
> -		.dynamic -j .dynsym  -j .rel* -j .rela* -j .reloc \
> +cmd_efi_objcopy = $(OBJCOPY) -j .header -j .text -j .sdata -j .data \
> +		-j .dynamic -j .dynstr  -j .dynsym -j .rel* -j .reloc \
>   		$(if $(EFI_TARGET),$(EFI_TARGET),-O binary) $^ $@
>
>   $(obj)/%.efi: $(obj)/%_efi.so

I see sections .dynsym and .dynstr in intermediate *.so files but no
.dynamic section.

Why would we need a .dynamic section for a static EFI application?

Best regards

Heinrich

Re: [PATCH 00/17] Various toolchain compatibility fixes/improvements

[Sam Edwards]

On Sun, Feb 23, 2025 at 9:55 PM Sam Edwards <cfsworks@gmail.com> wrote:
>
> Long time no see, U-Boot folks!
>
> This patchset consists of various bug fixes and correctness improvements that
> I discovered while attempting to add first-class LLVM support to the build
> system. These patches are NOT related to LLVM support directly; rather, they
> address existing issues that should be resolved regardless of future changes.
> For the most part, the patches are mutually independent and can be reviewed and
> applied separately. If any patch is not suitable for merging now, feel free to
> skip it: I will incorporate feedback and revisit those changes as part of the
> upcoming LLVM support patchset. I'd like this patchset to be evaluated on its
> own merits, based on the current state of the code, without consideration for
> future LLVM support.
>
> Note that the issues addressed in this patchset do not occur when U-Boot is
> built using the GCC/GNU toolchain. These bugs seem to be specific to builds
> using other toolchains, like LLVM, and do not appear to affect users relying on
> GCC/GNU. Therefore, I see no need to rush these changes into the stable branch.
>
> Again, these patches are mostly independent/reorderable...
> ...except that: "arm: Add aligned-memory aliases to eabi_compat"
> ...depends on: "arm: Add __aeabi_memclr in eabi_compat"
>
> Warm regards,
> Sam

Hi Tom,

I noticed that all patches in this series have been marked 'Changes
Requested' on Patchwork. While some patches do need changes, this
series was intended as a set of independent submissions: each patch
can be accepted, rejected, or reordered without affecting the others.
Would it be possible to reconsider the remaining patches for review
without resending the series?

I'd like to withdraw the following patches:

- [06/17] arm: Use -mstrict-align when the MMU is off (Incorrect approach)
- [11/17] makefile: Fix symbol typo in binary_size_check (Will follow
Simon's suggestion for a more comprehensive fix across architectures
in a future submission)

The feedback I've received so far was mostly requests for
clarification, which I believe I've addressed in my replies. Please
let me know if anything remains unclear or if further adjustments are
needed.

Thank you so much for your time!

Cheers,
Sam

>
> Sam Edwards (17):
>   arm: Remove stray .mmutable reference in linker script
>   arm: Exclude eabi_compat from LTO
>   arm: Add __aeabi_memclr in eabi_compat
>   arm: Add aligned-memory aliases to eabi_compat
>   arm: Discard unwanted sections in linker script
>   arm: Use -mstrict-align when the MMU is off
>   arm: Replace 'adrl' in EFI crt0
>   x86: Fix call64's section flags
>   spl: riscv: opensbi: Error on misaligned FDT
>   spl: Align FDT load address
>   makefile: Fix symbol typo in binary_size_check
>   makefile: Avoid objcopy --gap-fill for .hex/.srec
>   makefile: Add `norelro` linker option
>   makefile: Add READELF command variable
>   efi_loader: Remove ENTRY(_start) from linker script
>   efi_loader: Move .dynamic out of .text in EFI
>   scripts/Makefile.lib: efi: Preserve the .dynstr section as well
>
>  Makefile                    | 16 +++++++++-------
>  arch/arm/config.mk          |  4 ++++
>  arch/arm/cpu/u-boot.lds     | 29 ++++++++++-------------------
>  arch/arm/lib/Makefile       |  1 +
>  arch/arm/lib/crt0_arm_efi.S |  3 ++-
>  arch/arm/lib/eabi_compat.c  | 17 +++++++++++++++++
>  arch/x86/cpu/i386/call64.S  |  2 +-
>  common/spl/spl_fit.c        |  2 +-
>  common/spl/spl_opensbi.c    |  5 +++++
>  lib/efi_loader/elf_efi.ldsi |  7 +++----
>  scripts/Makefile.lib        |  4 ++--
>  11 files changed, 55 insertions(+), 35 deletions(-)
>
> --
> 2.45.2
>

Re: [PATCH 00/17] Various toolchain compatibility fixes/improvements

[Ilias Apalodimas]

Hi Sam

On Thu, 6 Mar 2025 at 03:47, Sam Edwards <cfsworks@gmail.com> wrote:
>
> On Sun, Feb 23, 2025 at 9:55 PM Sam Edwards <cfsworks@gmail.com> wrote:
> >
> > Long time no see, U-Boot folks!
> >
> > This patchset consists of various bug fixes and correctness improvements that
> > I discovered while attempting to add first-class LLVM support to the build
> > system. These patches are NOT related to LLVM support directly; rather, they
> > address existing issues that should be resolved regardless of future changes.
> > For the most part, the patches are mutually independent and can be reviewed and
> > applied separately. If any patch is not suitable for merging now, feel free to
> > skip it: I will incorporate feedback and revisit those changes as part of the
> > upcoming LLVM support patchset. I'd like this patchset to be evaluated on its
> > own merits, based on the current state of the code, without consideration for
> > future LLVM support.
> >
> > Note that the issues addressed in this patchset do not occur when U-Boot is
> > built using the GCC/GNU toolchain. These bugs seem to be specific to builds
> > using other toolchains, like LLVM, and do not appear to affect users relying on
> > GCC/GNU. Therefore, I see no need to rush these changes into the stable branch.
> >
> > Again, these patches are mostly independent/reorderable...
> > ...except that: "arm: Add aligned-memory aliases to eabi_compat"
> > ...depends on: "arm: Add __aeabi_memclr in eabi_compat"
> >
> > Warm regards,
> > Sam
>
> Hi Tom,
>
> I noticed that all patches in this series have been marked 'Changes
> Requested' on Patchwork. While some patches do need changes, this
> series was intended as a set of independent submissions: each patch
> can be accepted, rejected, or reordered without affecting the others.
> Would it be possible to reconsider the remaining patches for review
> without resending the series?
>
> I'd like to withdraw the following patches:
>
> - [06/17] arm: Use -mstrict-align when the MMU is off (Incorrect approach)
> - [11/17] makefile: Fix symbol typo in binary_size_check (Will follow
> Simon's suggestion for a more comprehensive fix across architectures
> in a future submission)
>
> The feedback I've received so far was mostly requests for
> clarification, which I believe I've addressed in my replies. Please
> let me know if anything remains unclear or if further adjustments are
> needed.
>
> Thank you so much for your time!

I am currently on a trip. I had a quick look at the patches and looked
reasonable. I'll have a closer look next week

Cheers
/Ilias
>
> Cheers,
> Sam
>
> >
> > Sam Edwards (17):
> >   arm: Remove stray .mmutable reference in linker script
> >   arm: Exclude eabi_compat from LTO
> >   arm: Add __aeabi_memclr in eabi_compat
> >   arm: Add aligned-memory aliases to eabi_compat
> >   arm: Discard unwanted sections in linker script
> >   arm: Use -mstrict-align when the MMU is off
> >   arm: Replace 'adrl' in EFI crt0
> >   x86: Fix call64's section flags
> >   spl: riscv: opensbi: Error on misaligned FDT
> >   spl: Align FDT load address
> >   makefile: Fix symbol typo in binary_size_check
> >   makefile: Avoid objcopy --gap-fill for .hex/.srec
> >   makefile: Add `norelro` linker option
> >   makefile: Add READELF command variable
> >   efi_loader: Remove ENTRY(_start) from linker script
> >   efi_loader: Move .dynamic out of .text in EFI
> >   scripts/Makefile.lib: efi: Preserve the .dynstr section as well
> >
> >  Makefile                    | 16 +++++++++-------
> >  arch/arm/config.mk          |  4 ++++
> >  arch/arm/cpu/u-boot.lds     | 29 ++++++++++-------------------
> >  arch/arm/lib/Makefile       |  1 +
> >  arch/arm/lib/crt0_arm_efi.S |  3 ++-
> >  arch/arm/lib/eabi_compat.c  | 17 +++++++++++++++++
> >  arch/x86/cpu/i386/call64.S  |  2 +-
> >  common/spl/spl_fit.c        |  2 +-
> >  common/spl/spl_opensbi.c    |  5 +++++
> >  lib/efi_loader/elf_efi.ldsi |  7 +++----
> >  scripts/Makefile.lib        |  4 ++--
> >  11 files changed, 55 insertions(+), 35 deletions(-)
> >
> > --
> > 2.45.2
> >

Re: [PATCH 00/17] Various toolchain compatibility fixes/improvements

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2912 bytes --]

On Wed, Mar 05, 2025 at 07:46:56PM -0800, Sam Edwards wrote:
> On Sun, Feb 23, 2025 at 9:55 PM Sam Edwards <cfsworks@gmail.com> wrote:
> >
> > Long time no see, U-Boot folks!
> >
> > This patchset consists of various bug fixes and correctness improvements that
> > I discovered while attempting to add first-class LLVM support to the build
> > system. These patches are NOT related to LLVM support directly; rather, they
> > address existing issues that should be resolved regardless of future changes.
> > For the most part, the patches are mutually independent and can be reviewed and
> > applied separately. If any patch is not suitable for merging now, feel free to
> > skip it: I will incorporate feedback and revisit those changes as part of the
> > upcoming LLVM support patchset. I'd like this patchset to be evaluated on its
> > own merits, based on the current state of the code, without consideration for
> > future LLVM support.
> >
> > Note that the issues addressed in this patchset do not occur when U-Boot is
> > built using the GCC/GNU toolchain. These bugs seem to be specific to builds
> > using other toolchains, like LLVM, and do not appear to affect users relying on
> > GCC/GNU. Therefore, I see no need to rush these changes into the stable branch.
> >
> > Again, these patches are mostly independent/reorderable...
> > ...except that: "arm: Add aligned-memory aliases to eabi_compat"
> > ...depends on: "arm: Add __aeabi_memclr in eabi_compat"
> >
> > Warm regards,
> > Sam
> 
> Hi Tom,
> 
> I noticed that all patches in this series have been marked 'Changes
> Requested' on Patchwork. While some patches do need changes, this
> series was intended as a set of independent submissions: each patch
> can be accepted, rejected, or reordered without affecting the others.
> Would it be possible to reconsider the remaining patches for review
> without resending the series?
> 
> I'd like to withdraw the following patches:
> 
> - [06/17] arm: Use -mstrict-align when the MMU is off (Incorrect approach)
> - [11/17] makefile: Fix symbol typo in binary_size_check (Will follow
> Simon's suggestion for a more comprehensive fix across architectures
> in a future submission)
> 
> The feedback I've received so far was mostly requests for
> clarification, which I believe I've addressed in my replies. Please
> let me know if anything remains unclear or if further adjustments are
> needed.
> 
> Thank you so much for your time!

It's *really* hard to track parts of a series in that way. If they
aren't intended to be applied all in one go, please post them
individually as v2s. The clarifications likely mean a bit more rewording
of the commit messages are in order.

If it's really hard on your end to resend things, I can go and poke
through the series (once Ilias has had time to do the reviews I see he
promised).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/17] Various toolchain compatibility fixes/improvements

[Sam Edwards]

On Thu, Mar 6, 2025 at 7:47 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 05, 2025 at 07:46:56PM -0800, Sam Edwards wrote:
> > On Sun, Feb 23, 2025 at 9:55 PM Sam Edwards <cfsworks@gmail.com> wrote:
> > >
> > > Long time no see, U-Boot folks!
> > >
> > > This patchset consists of various bug fixes and correctness improvements that
> > > I discovered while attempting to add first-class LLVM support to the build
> > > system. These patches are NOT related to LLVM support directly; rather, they
> > > address existing issues that should be resolved regardless of future changes.
> > > For the most part, the patches are mutually independent and can be reviewed and
> > > applied separately. If any patch is not suitable for merging now, feel free to
> > > skip it: I will incorporate feedback and revisit those changes as part of the
> > > upcoming LLVM support patchset. I'd like this patchset to be evaluated on its
> > > own merits, based on the current state of the code, without consideration for
> > > future LLVM support.
> > >
> > > Note that the issues addressed in this patchset do not occur when U-Boot is
> > > built using the GCC/GNU toolchain. These bugs seem to be specific to builds
> > > using other toolchains, like LLVM, and do not appear to affect users relying on
> > > GCC/GNU. Therefore, I see no need to rush these changes into the stable branch.
> > >
> > > Again, these patches are mostly independent/reorderable...
> > > ...except that: "arm: Add aligned-memory aliases to eabi_compat"
> > > ...depends on: "arm: Add __aeabi_memclr in eabi_compat"
> > >
> > > Warm regards,
> > > Sam
> >
> > Hi Tom,
> >
> > I noticed that all patches in this series have been marked 'Changes
> > Requested' on Patchwork. While some patches do need changes, this
> > series was intended as a set of independent submissions: each patch
> > can be accepted, rejected, or reordered without affecting the others.
> > Would it be possible to reconsider the remaining patches for review
> > without resending the series?
> >
> > I'd like to withdraw the following patches:
> >
> > - [06/17] arm: Use -mstrict-align when the MMU is off (Incorrect approach)
> > - [11/17] makefile: Fix symbol typo in binary_size_check (Will follow
> > Simon's suggestion for a more comprehensive fix across architectures
> > in a future submission)
> >
> > The feedback I've received so far was mostly requests for
> > clarification, which I believe I've addressed in my replies. Please
> > let me know if anything remains unclear or if further adjustments are
> > needed.
> >
> > Thank you so much for your time!
>
> It's *really* hard to track parts of a series in that way. If they
> aren't intended to be applied all in one go, please post them
> individually as v2s. The clarifications likely mean a bit more rewording
> of the commit messages are in order.
>
> If it's really hard on your end to resend things, I can go and poke
> through the series (once Ilias has had time to do the reviews I see he
> promised).

Hi Tom,

Well... that's "hard" but not "really hard" for me, so I think what
I'll pitch is this: let's let Ilias (and others if so inclined) have a
week to pick through the series (thanks Ilias!) and offer feedback;
I'll resend any patches that get no/positive feedback as a v2 series
(where the "just skip any patch deemed objectionable" option will
still apply), and any patches that need reworking can be part of my
upcoming LLVM support series. Would that work?

Cheers,
Sam

>
> --
> Tom

Re: [PATCH 00/17] Various toolchain compatibility fixes/improvements

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]

On Fri, Mar 07, 2025 at 10:48:11PM -0800, Sam Edwards wrote:
> On Thu, Mar 6, 2025 at 7:47 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 05, 2025 at 07:46:56PM -0800, Sam Edwards wrote:
> > > On Sun, Feb 23, 2025 at 9:55 PM Sam Edwards <cfsworks@gmail.com> wrote:
> > > >
> > > > Long time no see, U-Boot folks!
> > > >
> > > > This patchset consists of various bug fixes and correctness improvements that
> > > > I discovered while attempting to add first-class LLVM support to the build
> > > > system. These patches are NOT related to LLVM support directly; rather, they
> > > > address existing issues that should be resolved regardless of future changes.
> > > > For the most part, the patches are mutually independent and can be reviewed and
> > > > applied separately. If any patch is not suitable for merging now, feel free to
> > > > skip it: I will incorporate feedback and revisit those changes as part of the
> > > > upcoming LLVM support patchset. I'd like this patchset to be evaluated on its
> > > > own merits, based on the current state of the code, without consideration for
> > > > future LLVM support.
> > > >
> > > > Note that the issues addressed in this patchset do not occur when U-Boot is
> > > > built using the GCC/GNU toolchain. These bugs seem to be specific to builds
> > > > using other toolchains, like LLVM, and do not appear to affect users relying on
> > > > GCC/GNU. Therefore, I see no need to rush these changes into the stable branch.
> > > >
> > > > Again, these patches are mostly independent/reorderable...
> > > > ...except that: "arm: Add aligned-memory aliases to eabi_compat"
> > > > ...depends on: "arm: Add __aeabi_memclr in eabi_compat"
> > > >
> > > > Warm regards,
> > > > Sam
> > >
> > > Hi Tom,
> > >
> > > I noticed that all patches in this series have been marked 'Changes
> > > Requested' on Patchwork. While some patches do need changes, this
> > > series was intended as a set of independent submissions: each patch
> > > can be accepted, rejected, or reordered without affecting the others.
> > > Would it be possible to reconsider the remaining patches for review
> > > without resending the series?
> > >
> > > I'd like to withdraw the following patches:
> > >
> > > - [06/17] arm: Use -mstrict-align when the MMU is off (Incorrect approach)
> > > - [11/17] makefile: Fix symbol typo in binary_size_check (Will follow
> > > Simon's suggestion for a more comprehensive fix across architectures
> > > in a future submission)
> > >
> > > The feedback I've received so far was mostly requests for
> > > clarification, which I believe I've addressed in my replies. Please
> > > let me know if anything remains unclear or if further adjustments are
> > > needed.
> > >
> > > Thank you so much for your time!
> >
> > It's *really* hard to track parts of a series in that way. If they
> > aren't intended to be applied all in one go, please post them
> > individually as v2s. The clarifications likely mean a bit more rewording
> > of the commit messages are in order.
> >
> > If it's really hard on your end to resend things, I can go and poke
> > through the series (once Ilias has had time to do the reviews I see he
> > promised).
> 
> Hi Tom,
> 
> Well... that's "hard" but not "really hard" for me, so I think what
> I'll pitch is this: let's let Ilias (and others if so inclined) have a
> week to pick through the series (thanks Ilias!) and offer feedback;
> I'll resend any patches that get no/positive feedback as a v2 series
> (where the "just skip any patch deemed objectionable" option will
> still apply), and any patches that need reworking can be part of my
> upcoming LLVM support series. Would that work?

Yes, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]