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