* [PATCH 0/6] Clean up arm linker scripts
@ 2024-03-04 9:01 Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 1/6] arm: baltos: remove custom linker script Ilias Apalodimas
` (5 more replies)
0 siblings, 6 replies; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
The arm linker scripts had a mix of symbols and C defined variables in an
effort to emit relative references instead of absolute ones e.g [0]. A
linker bug prevented us from doing so [1] -- fixed since 2016.
This has led to confusion over the years, ending up with mixed section
definitions. Some sections are defined with overlays and different
definitions between v7 and v8 architectures.
For example __efi_runtime_rel_start/end is defined as a linker symbol for
armv8 and a C variable in armv7.
Linker scripts nowadays can emit relative references, as long as the symbol
definition is contained within the section definition. So let's switch most
of the C defined variables and clean up the arm sections.c file.
There's still a few symbols remaining -- __secure_start/end,
__secure_stack_start/end and __end which can be cleaned up
in a followup series.
The resulting binary (tested in QEMU v7/v8) had no size differences apart from
the emited sections and object types of those variables. I've also added prints
throughout the U-Boot init sequence. The offsets and delta for 'end - start'
section sizes is unchanged.
For example on QEMU v8:
$~ ./scripts/bloat-o-meter u-boot u-boot.new
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Function old new delta
Total: Before=798861, After=798861, chg +0.00%
$~ readelf -sW u-boot | grep bss_s
12: 00000000001029b8 0 SECTION LOCAL DEFAULT 12 .bss_start
8088: 0000000000000018 0 NOTYPE GLOBAL DEFAULT 1 _bss_start_ofs
8376: 00000000001029b8 0 OBJECT GLOBAL DEFAULT 12 __bss_start
$~ readelf -sW u-boot.new | grep bss_s
8085: 0000000000000018 0 NOTYPE GLOBAL DEFAULT 1 _bss_start_ofs
8373: 00000000001029b8 0 NOTYPE GLOBAL DEFAULT 12 __bss_start
For QEMU v7 the differences are a bit bigger but only affect the variables
placed in the .bss section because that was defined as an OVERLAY in the
existing linker script.
For example:
$~ nm u-boot | grep tftp_prev_block
000ca0dc ? tftp_prev_block
$~ nm u-boot.new | grep tftp_prev_block
000e0a5c b tftp_prev_block -----> The symbol is now placed into .bss
It's worth noting that since the efi regions are affected by the change, booting
with EFI is preferable while testing. Booting the kernel only should be enough
since the efi stub and the kernel proper do request boottime and runtime
services respectively.
Something along the lines of
> virtio scan && load virtio 0 $kernel_addr_r Image && bootefi $kernel_addr_r
will work for QEMU aarch64.
Tested platforms:
- QEMU aarch64
- Xilinx kv260 kria starter kit & zynq
- QEMU armv7
- STM32MP157C-DK2
[0] commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
[1] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
Ilias Apalodimas (6):
arm: baltos: remove custom linker script
arm: clean up v7 and v8 linker scripts for bss_start/end
arm: fix __efi_runtime_rel_start/end definitions
arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end
arm: fix __efi_runtime_start/end definitions
arm: move image_copy_start/end to linker symbols
arch/arm/cpu/armv8/u-boot-spl.lds | 14 +--
arch/arm/cpu/armv8/u-boot.lds | 45 ++------
arch/arm/cpu/u-boot.lds | 74 +++----------
arch/arm/lib/sections.c | 10 --
arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 22 +---
arch/arm/mach-zynq/u-boot.lds | 72 +++----------
board/vscom/baltos/u-boot.lds | 128 -----------------------
include/asm-generic/sections.h | 3 +
lib/efi_loader/efi_runtime.c | 1 +
9 files changed, 50 insertions(+), 319 deletions(-)
delete mode 100644 board/vscom/baltos/u-boot.lds
--
Changes since RFC:
- Rebase on top of -next and get rid of the dragonboard linker script changes.
Caleb removed that file completely
- Rewrite some commit messages to include the binutils bug details (thanks Sam)
2.37.2
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/6] arm: baltos: remove custom linker script
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
@ 2024-03-04 9:01 ` Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end Ilias Apalodimas
` (4 subsequent siblings)
5 siblings, 0 replies; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
commit 3d74a0977f514 ("ti: am335x: Remove unused linker script") removed
the linker script for the TI variant. This linker script doesn't seem to
do anything special and on top of that, has no definitions for the EFI
runtime sections.
So let's get rid of it and use the generic linker script which defines
those correctly
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
board/vscom/baltos/u-boot.lds | 128 ----------------------------------
1 file changed, 128 deletions(-)
delete mode 100644 board/vscom/baltos/u-boot.lds
diff --git a/board/vscom/baltos/u-boot.lds b/board/vscom/baltos/u-boot.lds
deleted file mode 100644
index cb2ee6769753..000000000000
--- a/board/vscom/baltos/u-boot.lds
+++ /dev/null
@@ -1,128 +0,0 @@
-/*
- * Copyright (c) 2004-2008 Texas Instruments
- *
- * (C) Copyright 2002
- * Gary Jennejohn, DENX Software Engineering, <garyj@denx.de>
- *
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of
- * the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- * MA 02111-1307 USA
- */
-
-OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
-OUTPUT_ARCH(arm)
-ENTRY(_start)
-SECTIONS
-{
- . = 0x00000000;
-
- . = ALIGN(4);
- .text :
- {
- *(.__image_copy_start)
- *(.vectors)
- CPUDIR/start.o (.text*)
- board/vscom/baltos/built-in.o (.text*)
- *(.text*)
- }
-
- . = ALIGN(4);
- .rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
-
- . = ALIGN(4);
- .data : {
- *(.data*)
- }
-
- . = ALIGN(4);
-
- . = .;
-
- . = ALIGN(4);
- __u_boot_list : {
- KEEP(*(SORT(__u_boot_list*)));
- }
-
- . = ALIGN(4);
-
- .image_copy_end :
- {
- *(.__image_copy_end)
- }
-
- .rel_dyn_start :
- {
- *(.__rel_dyn_start)
- }
-
- .rel.dyn : {
- *(.rel*)
- }
-
- .rel_dyn_end :
- {
- *(.__rel_dyn_end)
- }
-
- .hash : { *(.hash*) }
-
- .end :
- {
- *(.__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)
- }
-
-/*
- * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
- * __bss_base and __bss_limit are for linker only (overlay ordering)
- */
-
- .bss_start __rel_dyn_start (OVERLAY) : {
- KEEP(*(.__bss_start));
- __bss_base = .;
- }
-
- .bss __bss_base (OVERLAY) : {
- *(.bss*)
- . = ALIGN(4);
- __bss_limit = .;
- }
-
- .bss_end __bss_limit (OVERLAY) : {
- KEEP(*(.__bss_end));
- }
-
- .dynsym _image_binary_end : { *(.dynsym) }
- .dynbss : { *(.dynbss) }
- .dynstr : { *(.dynstr*) }
- .dynamic : { *(.dynamic*) }
- .gnu.hash : { *(.gnu.hash) }
- .plt : { *(.plt*) }
- .interp : { *(.interp*) }
- .gnu : { *(.gnu*) }
- .ARM.exidx : { *(.ARM.exidx*) }
-}
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 1/6] arm: baltos: remove custom linker script Ilias Apalodimas
@ 2024-03-04 9:01 ` Ilias Apalodimas
2024-03-06 7:32 ` Sam Edwards
2024-03-04 9:01 ` [PATCH 3/6] arm: fix __efi_runtime_rel_start/end definitions Ilias Apalodimas
` (3 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
and
commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts")
were moving the bss_start/end on c generated variables that were
injected in their own sections. The reason was that we needed relative
relocations for position independent code and linker bugs back then
prevented us from doing so.
However, the linker documentation pages states that symbols that are
defined within a section definition will create a relocatable
type with the value being a fixed offset from the base of a section.
This have been fixed since 2016 [1]. So let's start cleaning this up
starting with the bss_start and bss_end variables. Convert them into
symbols within the .bss section definition.
[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
[1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845
---
arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++-----------
arch/arm/cpu/armv8/u-boot.lds | 15 +++------------
arch/arm/cpu/u-boot.lds | 22 ++++------------------
arch/arm/lib/sections.c | 2 --
arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++-----------
arch/arm/mach-zynq/u-boot.lds | 21 +++------------------
6 files changed, 16 insertions(+), 72 deletions(-)
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
index 7cb9d731246d..16fddb46e9cb 100644
--- a/arch/arm/cpu/armv8/u-boot-spl.lds
+++ b/arch/arm/cpu/armv8/u-boot-spl.lds
@@ -63,18 +63,10 @@ SECTIONS
_image_binary_end = .;
- .bss_start (NOLOAD) : {
- . = ALIGN(8);
- KEEP(*(.__bss_start));
- } >.sdram
-
- .bss (NOLOAD) : {
+ .bss : {
+ __bss_start = .;
*(.bss*)
- . = ALIGN(8);
- } >.sdram
-
- .bss_end (NOLOAD) : {
- KEEP(*(.__bss_end));
+ __bss_end = .;
} >.sdram
/DISCARD/ : { *(.rela*) }
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index fb6a30c922f7..c4ee10ebc3ff 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -149,19 +149,10 @@ SECTIONS
_end = .;
- . = ALIGN(8);
-
- .bss_start : {
- KEEP(*(.__bss_start));
- }
-
- .bss : {
+ .bss ALIGN(8): {
+ __bss_start = .;
*(.bss*)
- . = ALIGN(8);
- }
-
- .bss_end : {
- KEEP(*(.__bss_end));
+ __bss_end = .;
}
/DISCARD/ : { *(.dynsym) }
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 7724c9332c3b..90d329b1ebe0 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -206,27 +206,13 @@ SECTIONS
*(.mmutable)
}
-/*
- * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
- * __bss_base and __bss_limit are for linker only (overlay ordering)
- */
-
- .bss_start __rel_dyn_start (OVERLAY) : {
- KEEP(*(.__bss_start));
- __bss_base = .;
- }
-
- .bss __bss_base (OVERLAY) : {
+ .bss ALIGN(4): {
+ __bss_start = .;
*(.bss*)
- . = ALIGN(4);
- __bss_limit = .;
- }
-
- .bss_end __bss_limit (OVERLAY) : {
- KEEP(*(.__bss_end));
+ __bss_end = .;
}
- .dynsym _image_binary_end : { *(.dynsym) }
+ .dynsym : { *(.dynsym) }
.dynbss : { *(.dynbss) }
.dynstr : { *(.dynstr*) }
.dynamic : { *(.dynamic*) }
diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index 857879711c6a..8e8bd5797e16 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -19,8 +19,6 @@
* aliasing warnings.
*/
-char __bss_start[0] __section(".__bss_start");
-char __bss_end[0] __section(".__bss_end");
char __image_copy_start[0] __section(".__image_copy_start");
char __image_copy_end[0] __section(".__image_copy_end");
char __rel_dyn_start[0] __section(".__rel_dyn_start");
diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
index 74618eba591b..b7887194026e 100644
--- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
+++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
@@ -56,18 +56,10 @@ SECTIONS
_image_binary_end = .;
- .bss_start (NOLOAD) : {
- . = ALIGN(8);
- KEEP(*(.__bss_start));
- }
-
- .bss (NOLOAD) : {
+ .bss ALIGN(8) : {
+ __bss_start = .;
*(.bss*)
- . = ALIGN(8);
- }
-
- .bss_end (NOLOAD) : {
- KEEP(*(.__bss_end));
+ __bss_end = .;
}
/DISCARD/ : { *(.dynsym) }
diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
index 3b7c9d515f8b..8d3259821719 100644
--- a/arch/arm/mach-zynq/u-boot.lds
+++ b/arch/arm/mach-zynq/u-boot.lds
@@ -102,26 +102,11 @@ SECTIONS
_image_binary_end = .;
-/*
- * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
- * __bss_base and __bss_limit are for linker only (overlay ordering)
- */
-
- .bss_start __rel_dyn_start (OVERLAY) : {
- KEEP(*(.__bss_start));
- __bss_base = .;
- }
-
- .bss __bss_base (OVERLAY) : {
+ .bss ALIGN(4): {
+ __bss_start = .;
*(.bss*)
- . = ALIGN(8);
- __bss_limit = .;
- }
-
- .bss_end __bss_limit (OVERLAY) : {
- KEEP(*(.__bss_end));
+ __bss_end = .;
}
-
/*
* Zynq needs to discard these sections because the user
* is expected to pass this image on to tools for boot.bin
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/6] arm: fix __efi_runtime_rel_start/end definitions
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 1/6] arm: baltos: remove custom linker script Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end Ilias Apalodimas
@ 2024-03-04 9:01 ` Ilias Apalodimas
2024-03-06 7:35 ` Sam Edwards
2024-03-04 9:01 ` [PATCH 4/6] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end Ilias Apalodimas
` (2 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
__efi_runtime_rel_start/end are defined as c variables for arm7 only in
order to force the compiler emit relative references. However, defining
those within a section definition will do the same thing. On top of that
the v8 linker scripts define it as a symbol.
So let's remove the special sections from the linker scripts, the
variable definitions from sections.c and define them as a symbols within
the correct section.
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
arch/arm/cpu/armv8/u-boot.lds | 4 +---
arch/arm/cpu/u-boot.lds | 16 +++-------------
arch/arm/lib/sections.c | 2 --
arch/arm/mach-zynq/u-boot.lds | 16 +++-------------
include/asm-generic/sections.h | 2 ++
lib/efi_loader/efi_runtime.c | 1 +
6 files changed, 10 insertions(+), 31 deletions(-)
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index c4ee10ebc3ff..eccb116d3cfa 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -115,9 +115,7 @@ SECTIONS
KEEP(*(SORT(__u_boot_list*)));
}
- . = ALIGN(8);
-
- .efi_runtime_rel : {
+ .efi_runtime_rel ALIGN(8) : {
__efi_runtime_rel_start = .;
*(.rel*.efi_runtime)
*(.rel*.efi_runtime.*)
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 90d329b1ebe0..70e78ce46672 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -152,21 +152,11 @@ SECTIONS
KEEP(*(SORT(__u_boot_list*)));
}
- . = ALIGN(4);
-
- .efi_runtime_rel_start :
- {
- *(.__efi_runtime_rel_start)
- }
-
- .efi_runtime_rel : {
+ .efi_runtime_rel ALIGN(4) : {
+ __efi_runtime_rel_start = .;
*(.rel*.efi_runtime)
*(.rel*.efi_runtime.*)
- }
-
- .efi_runtime_rel_stop :
- {
- *(.__efi_runtime_rel_stop)
+ __efi_runtime_rel_stop = .;
}
. = ALIGN(4);
diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index 8e8bd5797e16..ddfde52163fc 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -29,6 +29,4 @@ char __secure_stack_start[0] __section(".__secure_stack_start");
char __secure_stack_end[0] __section(".__secure_stack_end");
char __efi_runtime_start[0] __section(".__efi_runtime_start");
char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
-char __efi_runtime_rel_start[0] __section(".__efi_runtime_rel_start");
-char __efi_runtime_rel_stop[0] __section(".__efi_runtime_rel_stop");
char _end[0] __section(".__end");
diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
index 8d3259821719..66a9e37f9198 100644
--- a/arch/arm/mach-zynq/u-boot.lds
+++ b/arch/arm/mach-zynq/u-boot.lds
@@ -58,21 +58,11 @@ SECTIONS
KEEP(*(SORT(__u_boot_list*)));
}
- . = ALIGN(4);
-
- .efi_runtime_rel_start :
- {
- *(.__efi_runtime_rel_start)
- }
-
- .efi_runtime_rel : {
+ .efi_runtime_rel ALIGN(4) : {
+ __efi_runtime_rel_start = .;
*(.rel*.efi_runtime)
*(.rel*.efi_runtime.*)
- }
-
- .efi_runtime_rel_stop :
- {
- *(.__efi_runtime_rel_stop)
+ __efi_runtime_rel_stop = .;
}
. = ALIGN(8);
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 1e1657a01673..60949200dd93 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -34,6 +34,8 @@ extern char __priv_data_start[], __priv_data_end[];
/* Start and end of .ctors section - used for constructor calls. */
extern char __ctors_start[], __ctors_end[];
+extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
+
/* function descriptor handling (if any). Override
* in asm/sections.h */
#ifndef dereference_function_descriptor
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 18da6892e796..9185f1894c47 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -15,6 +15,7 @@
#include <rtc.h>
#include <asm/global_data.h>
#include <u-boot/crc.h>
+#include <asm/sections.h>
/* For manual relocation support */
DECLARE_GLOBAL_DATA_PTR;
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/6] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
` (2 preceding siblings ...)
2024-03-04 9:01 ` [PATCH 3/6] arm: fix __efi_runtime_rel_start/end definitions Ilias Apalodimas
@ 2024-03-04 9:01 ` Ilias Apalodimas
2024-03-06 7:35 ` Sam Edwards
2024-03-04 9:01 ` [PATCH 5/6] arm: fix __efi_runtime_start/end definitions Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 6/6] arm: move image_copy_start/end to linker symbols Ilias Apalodimas
5 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
commit 47bd65ef057f ("arm: make __rel_dyn_{start, end} compiler-generated")
were moving the __rel_dyn_start/end on c generated variables that were
injected in their own sections. The reason was that we needed relative
relocations for position independent code and linker bugs back then
prevented us from doing so.
However, the linker documentation pages states that symbols that are
defined within a section definition will create a relocatable
type with the value being a fixed offset from the base of a section [0].
This have been fixed since 2016 [1]
[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
[1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
arch/arm/cpu/armv8/u-boot.lds | 16 +++-------------
arch/arm/cpu/u-boot.lds | 14 +++-----------
arch/arm/lib/sections.c | 2 --
arch/arm/mach-zynq/u-boot.lds | 14 +++-----------
4 files changed, 9 insertions(+), 37 deletions(-)
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index eccb116d3cfa..e737de761a9d 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -129,20 +129,10 @@ SECTIONS
*(.__image_copy_end)
}
- . = ALIGN(8);
-
- .rel_dyn_start :
- {
- *(.__rel_dyn_start)
- }
-
- .rela.dyn : {
+ .rela.dyn ALIGN(8) : {
+ __rel_dyn_start = .;
*(.rela*)
- }
-
- .rel_dyn_end :
- {
- *(.__rel_dyn_end)
+ __rel_dyn_end = .;
}
_end = .;
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 70e78ce46672..7c6e7891d360 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -166,18 +166,10 @@ SECTIONS
*(.__image_copy_end)
}
- .rel_dyn_start :
- {
- *(.__rel_dyn_start)
- }
-
- .rel.dyn : {
+ .rel.dyn ALIGN(4) : {
+ __rel_dyn_start = .;
*(.rel*)
- }
-
- .rel_dyn_end :
- {
- *(.__rel_dyn_end)
+ __rel_dyn_end = .;
}
.end :
diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index ddfde52163fc..1ee3dd3667ba 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -21,8 +21,6 @@
char __image_copy_start[0] __section(".__image_copy_start");
char __image_copy_end[0] __section(".__image_copy_end");
-char __rel_dyn_start[0] __section(".__rel_dyn_start");
-char __rel_dyn_end[0] __section(".__rel_dyn_end");
char __secure_start[0] __section(".__secure_start");
char __secure_end[0] __section(".__secure_end");
char __secure_stack_start[0] __section(".__secure_stack_start");
diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
index 66a9e37f9198..71dea4a1f60a 100644
--- a/arch/arm/mach-zynq/u-boot.lds
+++ b/arch/arm/mach-zynq/u-boot.lds
@@ -71,18 +71,10 @@ SECTIONS
*(.__image_copy_end)
}
- .rel_dyn_start :
- {
- *(.__rel_dyn_start)
- }
-
- .rel.dyn : {
+ .rel.dyn ALIGN(8) : {
+ __rel_dyn_start = .;
*(.rel*)
- }
-
- .rel_dyn_end :
- {
- *(.__rel_dyn_end)
+ __rel_dyn_end = .;
}
.end :
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
` (3 preceding siblings ...)
2024-03-04 9:01 ` [PATCH 4/6] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end Ilias Apalodimas
@ 2024-03-04 9:01 ` Ilias Apalodimas
2024-03-06 8:14 ` Sam Edwards
2024-03-04 9:01 ` [PATCH 6/6] arm: move image_copy_start/end to linker symbols Ilias Apalodimas
5 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
__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>
---
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) : {
+ __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) : {
+ __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 */
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
` (4 preceding siblings ...)
2024-03-04 9:01 ` [PATCH 5/6] arm: fix __efi_runtime_start/end definitions Ilias Apalodimas
@ 2024-03-04 9:01 ` Ilias Apalodimas
2024-03-06 8:22 ` Sam Edwards
5 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-04 9:01 UTC (permalink / raw)
To: u-boot, trini, cfsworks
Cc: caleb.connolly, sumit.garg, Ilias Apalodimas, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
image_copy_start/end are defined as c variables in order to force the compiler
emit relative references. However, defining those within a section definition
will do the same thing.
So let's remove the special sections from the linker scripts, the
variable definitions from sections.c and define them as a symbols within
a section.
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
arch/arm/cpu/u-boot.lds | 10 ++--------
arch/arm/lib/sections.c | 2 --
arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
arch/arm/mach-zynq/u-boot.lds | 9 ++-------
5 files changed, 8 insertions(+), 31 deletions(-)
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index e737de761a9d..340acf5c043b 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -23,7 +23,7 @@ SECTIONS
. = ALIGN(8);
.text :
{
- *(.__image_copy_start)
+ __image_copy_start = .;
CPUDIR/start.o (.text*)
}
@@ -120,13 +120,7 @@ SECTIONS
*(.rel*.efi_runtime)
*(.rel*.efi_runtime.*)
__efi_runtime_rel_stop = .;
- }
-
- . = ALIGN(8);
-
- .image_copy_end :
- {
- *(.__image_copy_end)
+ __image_copy_end = .;
}
.rela.dyn ALIGN(8) : {
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index df55bb716e35..7c4f9c2d4dd3 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -37,7 +37,7 @@ SECTIONS
. = ALIGN(4);
.text :
{
- *(.__image_copy_start)
+ __image_copy_start = .;
*(.vectors)
CPUDIR/start.o (.text*)
}
@@ -151,13 +151,7 @@ SECTIONS
*(.rel*.efi_runtime)
*(.rel*.efi_runtime.*)
__efi_runtime_rel_stop = .;
- }
-
- . = ALIGN(4);
-
- .image_copy_end :
- {
- *(.__image_copy_end)
+ __image_copy_end = .;
}
.rel.dyn ALIGN(4) : {
diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index a4d4202e99f5..db5463b2bbbc 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -19,8 +19,6 @@
* aliasing warnings.
*/
-char __image_copy_start[0] __section(".__image_copy_start");
-char __image_copy_end[0] __section(".__image_copy_end");
char __secure_start[0] __section(".__secure_start");
char __secure_end[0] __section(".__secure_end");
char __secure_stack_start[0] __section(".__secure_stack_start");
diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
index b7887194026e..4b480ebb0c4d 100644
--- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
+++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
@@ -24,7 +24,7 @@ SECTIONS
.text : {
. = ALIGN(8);
- *(.__image_copy_start)
+ __image_copy_start = .;
CPUDIR/start.o (.text*)
*(.text*)
}
@@ -42,11 +42,7 @@ SECTIONS
__u_boot_list : {
. = ALIGN(8);
KEEP(*(SORT(__u_boot_list*)));
- }
-
- .image_copy_end : {
- . = ALIGN(8);
- *(.__image_copy_end)
+ __image_copy_end = .;
}
.end : {
diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
index fcd0f42a7106..553bcf182272 100644
--- a/arch/arm/mach-zynq/u-boot.lds
+++ b/arch/arm/mach-zynq/u-boot.lds
@@ -16,7 +16,7 @@ SECTIONS
. = ALIGN(4);
.text :
{
- *(.__image_copy_start)
+ __image_copy_start = .;
*(.vectors)
CPUDIR/start.o (.text*)
}
@@ -57,12 +57,7 @@ SECTIONS
*(.rel*.efi_runtime)
*(.rel*.efi_runtime.*)
__efi_runtime_rel_stop = .;
- }
-
- . = ALIGN(8);
- .image_copy_end :
- {
- *(.__image_copy_end)
+ __image_copy_end = .;
}
.rel.dyn ALIGN(8) : {
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end
2024-03-04 9:01 ` [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end Ilias Apalodimas
@ 2024-03-06 7:32 ` Sam Edwards
2024-03-06 9:08 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 7:32 UTC (permalink / raw)
To: Ilias Apalodimas, u-boot, trini
Cc: caleb.connolly, sumit.garg, Simon Glass, Philipp Tomsich,
Kever Yang, Michal Simek, Yegor Yefremov, Heinrich Schuchardt,
Shiji Yang, Bin Meng
On 3/4/24 02:01, Ilias Apalodimas wrote:
> commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
> and
> commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts")
> were moving the bss_start/end on c generated variables that were
> injected in their own sections. The reason was that we needed relative
> relocations for position independent code and linker bugs back then
> prevented us from doing so.
>
> However, the linker documentation pages states that symbols that are
> defined within a section definition will create a relocatable
> type with the value being a fixed offset from the base of a section.
> This have been fixed since 2016 [1]. So let's start cleaning this up
> starting with the bss_start and bss_end variables. Convert them into
> symbols within the .bss section definition.
>
> [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845
> ---
> arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++-----------
> arch/arm/cpu/armv8/u-boot.lds | 15 +++------------
> arch/arm/cpu/u-boot.lds | 22 ++++------------------
> arch/arm/lib/sections.c | 2 --
> arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++-----------
> arch/arm/mach-zynq/u-boot.lds | 21 +++------------------
> 6 files changed, 16 insertions(+), 72 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
> index 7cb9d731246d..16fddb46e9cb 100644
> --- a/arch/arm/cpu/armv8/u-boot-spl.lds
> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds
> @@ -63,18 +63,10 @@ SECTIONS
>
> _image_binary_end = .;
>
> - .bss_start (NOLOAD) : {
> - . = ALIGN(8);
> - KEEP(*(.__bss_start));
> - } >.sdram
> -
> - .bss (NOLOAD) : {
> + .bss : {
> + __bss_start = .;
> *(.bss*)
> - . = ALIGN(8);
> - } >.sdram
> -
> - .bss_end (NOLOAD) : {
> - KEEP(*(.__bss_end));
> + __bss_end = .;
> } >.sdram
>
> /DISCARD/ : { *(.rela*) }
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index fb6a30c922f7..c4ee10ebc3ff 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -149,19 +149,10 @@ SECTIONS
>
> _end = .;
>
> - . = ALIGN(8);
> -
> - .bss_start : {
> - KEEP(*(.__bss_start));
> - }
> -
> - .bss : {
> + .bss ALIGN(8): {
> + __bss_start = .;
> *(.bss*)
> - . = ALIGN(8);
> - }
> -
> - .bss_end : {
> - KEEP(*(.__bss_end));
> + __bss_end = .;
> }
>
> /DISCARD/ : { *(.dynsym) }
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 7724c9332c3b..90d329b1ebe0 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -206,27 +206,13 @@ SECTIONS
> *(.mmutable)
> }
>
> -/*
> - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> - * __bss_base and __bss_limit are for linker only (overlay ordering)
> - */
> -
> - .bss_start __rel_dyn_start (OVERLAY) : {
> - KEEP(*(.__bss_start));
> - __bss_base = .;
> - }
> -
> - .bss __bss_base (OVERLAY) : {
> + .bss ALIGN(4): {
Hi Ilias,
I have been reviewing this patchset by diffing output binaries and
linker maps between successive patches. Most of the patches in this
series result in no change to the output binary whatsoever (which also
means, no regressions!) but this one does have a change: .bss is being
moved after .mmutable. You should be able to see this yourself by
comparing `u-boot.map` after a successful build, before and after
applying this patch.
Since the current u-boot.lds puts .bss right after __image_copy_end
(which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I
guess makes sense, if U-Boot zero-initializes .bss after applying
relocations), perhaps this patch should be moving the .bss section up
there in the .lds, putting (OVERLAY) back, and adding a comment to the
effect of "These sections occupy the same memory, but their lifetimes do
not overlap: U-Boot initializes .bss only after applying relocations and
therefore after it doesn't need .rel.dyn any more."
We might also decide that the overlay memory-saving trick isn't actually
all that important anymore and that .bss should have a new home. Someone
far more seasoned than I should be the one to make that decision though.
The rest of the patchset looks great! I'll add my tags to those in a bit.
Cheers,
Sam
> + __bss_start = .;
> *(.bss*)
> - . = ALIGN(4);
> - __bss_limit = .;
> - }
> -
> - .bss_end __bss_limit (OVERLAY) : {
> - KEEP(*(.__bss_end));
> + __bss_end = .;
> }
>
> - .dynsym _image_binary_end : { *(.dynsym) }
> + .dynsym : { *(.dynsym) }
> .dynbss : { *(.dynbss) }
> .dynstr : { *(.dynstr*) }
> .dynamic : { *(.dynamic*) }
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index 857879711c6a..8e8bd5797e16 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -19,8 +19,6 @@
> * aliasing warnings.
> */
>
> -char __bss_start[0] __section(".__bss_start");
> -char __bss_end[0] __section(".__bss_end");
> char __image_copy_start[0] __section(".__image_copy_start");
> char __image_copy_end[0] __section(".__image_copy_end");
> char __rel_dyn_start[0] __section(".__rel_dyn_start");
> diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> index 74618eba591b..b7887194026e 100644
> --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> @@ -56,18 +56,10 @@ SECTIONS
>
> _image_binary_end = .;
>
> - .bss_start (NOLOAD) : {
> - . = ALIGN(8);
> - KEEP(*(.__bss_start));
> - }
> -
> - .bss (NOLOAD) : {
> + .bss ALIGN(8) : {
> + __bss_start = .;
> *(.bss*)
> - . = ALIGN(8);
> - }
> -
> - .bss_end (NOLOAD) : {
> - KEEP(*(.__bss_end));
> + __bss_end = .;
> }
>
> /DISCARD/ : { *(.dynsym) }
> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> index 3b7c9d515f8b..8d3259821719 100644
> --- a/arch/arm/mach-zynq/u-boot.lds
> +++ b/arch/arm/mach-zynq/u-boot.lds
> @@ -102,26 +102,11 @@ SECTIONS
>
> _image_binary_end = .;
>
> -/*
> - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> - * __bss_base and __bss_limit are for linker only (overlay ordering)
> - */
> -
> - .bss_start __rel_dyn_start (OVERLAY) : {
> - KEEP(*(.__bss_start));
> - __bss_base = .;
> - }
> -
> - .bss __bss_base (OVERLAY) : {
> + .bss ALIGN(4): {
> + __bss_start = .;
> *(.bss*)
> - . = ALIGN(8);
> - __bss_limit = .;
> - }
> -
> - .bss_end __bss_limit (OVERLAY) : {
> - KEEP(*(.__bss_end));
> + __bss_end = .;
> }
> -
> /*
> * Zynq needs to discard these sections because the user
> * is expected to pass this image on to tools for boot.bin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] arm: fix __efi_runtime_rel_start/end definitions
2024-03-04 9:01 ` [PATCH 3/6] arm: fix __efi_runtime_rel_start/end definitions Ilias Apalodimas
@ 2024-03-06 7:35 ` Sam Edwards
0 siblings, 0 replies; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 7:35 UTC (permalink / raw)
To: Ilias Apalodimas, u-boot, trini
Cc: caleb.connolly, sumit.garg, Simon Glass, Philipp Tomsich,
Kever Yang, Michal Simek, Yegor Yefremov, Heinrich Schuchardt,
Shiji Yang, Bin Meng
On 3/4/24 02:01, Ilias Apalodimas wrote:
> __efi_runtime_rel_start/end are defined as c variables for arm7 only in
> order to force the compiler emit relative references. However, defining
> those within a section definition will do the same thing. On top of that
> the v8 linker scripts define it as a symbol.
>
> So let's remove the special sections from the linker scripts, the
> variable definitions from sections.c and define them as a symbols within
> the correct section.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Sam Edwards <CFSworks@gmail.com>
Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
Thanks for the cleanup,
Sam
> ---
> arch/arm/cpu/armv8/u-boot.lds | 4 +---
> arch/arm/cpu/u-boot.lds | 16 +++-------------
> arch/arm/lib/sections.c | 2 --
> arch/arm/mach-zynq/u-boot.lds | 16 +++-------------
> include/asm-generic/sections.h | 2 ++
> lib/efi_loader/efi_runtime.c | 1 +
> 6 files changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index c4ee10ebc3ff..eccb116d3cfa 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -115,9 +115,7 @@ SECTIONS
> KEEP(*(SORT(__u_boot_list*)));
> }
>
> - . = ALIGN(8);
> -
> - .efi_runtime_rel : {
> + .efi_runtime_rel ALIGN(8) : {
> __efi_runtime_rel_start = .;
> *(.rel*.efi_runtime)
> *(.rel*.efi_runtime.*)
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 90d329b1ebe0..70e78ce46672 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -152,21 +152,11 @@ SECTIONS
> KEEP(*(SORT(__u_boot_list*)));
> }
>
> - . = ALIGN(4);
> -
> - .efi_runtime_rel_start :
> - {
> - *(.__efi_runtime_rel_start)
> - }
> -
> - .efi_runtime_rel : {
> + .efi_runtime_rel ALIGN(4) : {
> + __efi_runtime_rel_start = .;
> *(.rel*.efi_runtime)
> *(.rel*.efi_runtime.*)
> - }
> -
> - .efi_runtime_rel_stop :
> - {
> - *(.__efi_runtime_rel_stop)
> + __efi_runtime_rel_stop = .;
> }
>
> . = ALIGN(4);
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index 8e8bd5797e16..ddfde52163fc 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -29,6 +29,4 @@ char __secure_stack_start[0] __section(".__secure_stack_start");
> char __secure_stack_end[0] __section(".__secure_stack_end");
> char __efi_runtime_start[0] __section(".__efi_runtime_start");
> char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
> -char __efi_runtime_rel_start[0] __section(".__efi_runtime_rel_start");
> -char __efi_runtime_rel_stop[0] __section(".__efi_runtime_rel_stop");
> char _end[0] __section(".__end");
> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> index 8d3259821719..66a9e37f9198 100644
> --- a/arch/arm/mach-zynq/u-boot.lds
> +++ b/arch/arm/mach-zynq/u-boot.lds
> @@ -58,21 +58,11 @@ SECTIONS
> KEEP(*(SORT(__u_boot_list*)));
> }
>
> - . = ALIGN(4);
> -
> - .efi_runtime_rel_start :
> - {
> - *(.__efi_runtime_rel_start)
> - }
> -
> - .efi_runtime_rel : {
> + .efi_runtime_rel ALIGN(4) : {
> + __efi_runtime_rel_start = .;
> *(.rel*.efi_runtime)
> *(.rel*.efi_runtime.*)
> - }
> -
> - .efi_runtime_rel_stop :
> - {
> - *(.__efi_runtime_rel_stop)
> + __efi_runtime_rel_stop = .;
> }
>
> . = ALIGN(8);
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 1e1657a01673..60949200dd93 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -34,6 +34,8 @@ extern char __priv_data_start[], __priv_data_end[];
> /* Start and end of .ctors section - used for constructor calls. */
> extern char __ctors_start[], __ctors_end[];
>
> +extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
> +
> /* function descriptor handling (if any). Override
> * in asm/sections.h */
> #ifndef dereference_function_descriptor
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 18da6892e796..9185f1894c47 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -15,6 +15,7 @@
> #include <rtc.h>
> #include <asm/global_data.h>
> #include <u-boot/crc.h>
> +#include <asm/sections.h>
>
> /* For manual relocation support */
> DECLARE_GLOBAL_DATA_PTR;
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/6] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end
2024-03-04 9:01 ` [PATCH 4/6] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end Ilias Apalodimas
@ 2024-03-06 7:35 ` Sam Edwards
0 siblings, 0 replies; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 7:35 UTC (permalink / raw)
To: Ilias Apalodimas, u-boot, trini
Cc: caleb.connolly, sumit.garg, Simon Glass, Philipp Tomsich,
Kever Yang, Michal Simek, Yegor Yefremov, Heinrich Schuchardt,
Shiji Yang, Bin Meng
On 3/4/24 02:01, Ilias Apalodimas wrote:
> commit 47bd65ef057f ("arm: make __rel_dyn_{start, end} compiler-generated")
> were moving the __rel_dyn_start/end on c generated variables that were
> injected in their own sections. The reason was that we needed relative
> relocations for position independent code and linker bugs back then
> prevented us from doing so.
>
> However, the linker documentation pages states that symbols that are
> defined within a section definition will create a relocatable
> type with the value being a fixed offset from the base of a section [0].
> This have been fixed since 2016 [1]
>
> [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Sam Edwards <CFSworks@gmail.com>
Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
Thanks for the cleanup,
Sam
> ---
> arch/arm/cpu/armv8/u-boot.lds | 16 +++-------------
> arch/arm/cpu/u-boot.lds | 14 +++-----------
> arch/arm/lib/sections.c | 2 --
> arch/arm/mach-zynq/u-boot.lds | 14 +++-----------
> 4 files changed, 9 insertions(+), 37 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index eccb116d3cfa..e737de761a9d 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -129,20 +129,10 @@ SECTIONS
> *(.__image_copy_end)
> }
>
> - . = ALIGN(8);
> -
> - .rel_dyn_start :
> - {
> - *(.__rel_dyn_start)
> - }
> -
> - .rela.dyn : {
> + .rela.dyn ALIGN(8) : {
> + __rel_dyn_start = .;
> *(.rela*)
> - }
> -
> - .rel_dyn_end :
> - {
> - *(.__rel_dyn_end)
> + __rel_dyn_end = .;
> }
>
> _end = .;
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 70e78ce46672..7c6e7891d360 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -166,18 +166,10 @@ SECTIONS
> *(.__image_copy_end)
> }
>
> - .rel_dyn_start :
> - {
> - *(.__rel_dyn_start)
> - }
> -
> - .rel.dyn : {
> + .rel.dyn ALIGN(4) : {
> + __rel_dyn_start = .;
> *(.rel*)
> - }
> -
> - .rel_dyn_end :
> - {
> - *(.__rel_dyn_end)
> + __rel_dyn_end = .;
> }
>
> .end :
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index ddfde52163fc..1ee3dd3667ba 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -21,8 +21,6 @@
>
> char __image_copy_start[0] __section(".__image_copy_start");
> char __image_copy_end[0] __section(".__image_copy_end");
> -char __rel_dyn_start[0] __section(".__rel_dyn_start");
> -char __rel_dyn_end[0] __section(".__rel_dyn_end");
> char __secure_start[0] __section(".__secure_start");
> char __secure_end[0] __section(".__secure_end");
> char __secure_stack_start[0] __section(".__secure_stack_start");
> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> index 66a9e37f9198..71dea4a1f60a 100644
> --- a/arch/arm/mach-zynq/u-boot.lds
> +++ b/arch/arm/mach-zynq/u-boot.lds
> @@ -71,18 +71,10 @@ SECTIONS
> *(.__image_copy_end)
> }
>
> - .rel_dyn_start :
> - {
> - *(.__rel_dyn_start)
> - }
> -
> - .rel.dyn : {
> + .rel.dyn ALIGN(8) : {
> + __rel_dyn_start = .;
> *(.rel*)
> - }
> -
> - .rel_dyn_end :
> - {
> - *(.__rel_dyn_end)
> + __rel_dyn_end = .;
> }
>
> .end :
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-04 9:01 ` [PATCH 5/6] arm: fix __efi_runtime_start/end definitions Ilias Apalodimas
@ 2024-03-06 8:14 ` Sam Edwards
2024-03-06 9:13 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 8:14 UTC (permalink / raw)
To: Ilias Apalodimas, u-boot, trini
Cc: caleb.connolly, sumit.garg, Simon Glass, Philipp Tomsich,
Kever Yang, Michal Simek, Yegor Yefremov, Heinrich Schuchardt,
Shiji Yang, Bin Meng
On 3/4/24 02:01, Ilias Apalodimas wrote:
> __efi_runtime_start/end are defined as c variables for arm7 only in
> order to force the compiler emit relative references. However, defining
> those within a section definition will do the same thing. On top of that
> the v8 linker scripts define it as a symbol.
>
> So let's remove the special sections from the linker scripts, the
> variable definitions from sections.c and define them as a symbols within
> the correct section.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
Thanks for the cleanup,
Sam
> ---
> arch/arm/cpu/u-boot.lds | 12 +++---------
> arch/arm/lib/sections.c | 2 --
> arch/arm/mach-zynq/u-boot.lds | 12 +++---------
> include/asm-generic/sections.h | 1 +
> 4 files changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 7c6e7891d360..df55bb716e35 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -43,18 +43,12 @@ SECTIONS
> }
>
> /* This needs to come before *(.text*) */
> - .__efi_runtime_start : {
> - *(.__efi_runtime_start)
> - }
> -
> - .efi_runtime : {
> + .efi_runtime ALIGN(4) : {
Do we truly require the ALIGN(4)? If I understand correctly, by default,
the linker calculates the alignment of an output section as the least
common multiple of the input sections' alignment requirements -- meaning
most (perhaps all) of our ALIGN()s today are redundant. For the time
being, I'm in favor of merging existing `. = ALIGN(x)` into each
following section for clarity and to avoid the testing overhead of
removing them in the same patch as other changes. However, in the near
future (perhaps even as "near" as v2 of this series?), I'd also like to
see a patch that eliminates unnecessary ALIGN()s altogether. Introducing
additional ALIGN()s where we already know they aren't needed may be a
step away from that goal.
> + __efi_runtime_start = .;
> *(.text.efi_runtime*)
> *(.rodata.efi_runtime*)
> *(.data.efi_runtime*)
> - }
> -
> - .__efi_runtime_stop : {
> - *(.__efi_runtime_stop)
> + __efi_runtime_stop = .;
> }
>
> .text_rest :
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index 1ee3dd3667ba..a4d4202e99f5 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -25,6 +25,4 @@ char __secure_start[0] __section(".__secure_start");
> char __secure_end[0] __section(".__secure_end");
> char __secure_stack_start[0] __section(".__secure_stack_start");
> char __secure_stack_end[0] __section(".__secure_stack_end");
> -char __efi_runtime_start[0] __section(".__efi_runtime_start");
> -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
> char _end[0] __section(".__end");
> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> index 71dea4a1f60a..fcd0f42a7106 100644
> --- a/arch/arm/mach-zynq/u-boot.lds
> +++ b/arch/arm/mach-zynq/u-boot.lds
> @@ -22,18 +22,12 @@ SECTIONS
> }
>
> /* This needs to come before *(.text*) */
> - .__efi_runtime_start : {
> - *(.__efi_runtime_start)
> - }
> -
> - .efi_runtime : {
> + .efi_runtime ALIGN(4) : {
Ditto above
> + __efi_runtime_start = .;
> *(.text.efi_runtime*)
> *(.rodata.efi_runtime*)
> *(.data.efi_runtime*)
> - }
> -
> - .__efi_runtime_stop : {
> - *(.__efi_runtime_stop)
> + __efi_runtime_stop = .;
> }
>
> .text_rest :
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 60949200dd93..b6bca53db10d 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
> extern char __ctors_start[], __ctors_end[];
>
> extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
> +extern char __efi_runtime_start[], __efi_runtime_stop[];
>
> /* function descriptor handling (if any). Override
> * in asm/sections.h */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-04 9:01 ` [PATCH 6/6] arm: move image_copy_start/end to linker symbols Ilias Apalodimas
@ 2024-03-06 8:22 ` Sam Edwards
2024-03-06 9:35 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 8:22 UTC (permalink / raw)
To: Ilias Apalodimas, u-boot, trini
Cc: caleb.connolly, sumit.garg, Simon Glass, Philipp Tomsich,
Kever Yang, Michal Simek, Yegor Yefremov, Heinrich Schuchardt,
Shiji Yang, Bin Meng
On 3/4/24 02:01, Ilias Apalodimas wrote:
> image_copy_start/end are defined as c variables in order to force the compiler
> emit relative references. However, defining those within a section definition
> will do the same thing.
>
> So let's remove the special sections from the linker scripts, the
> variable definitions from sections.c and define them as a symbols within
> a section.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
Since the __image_copy_* symbols are marking boundaries in the whole
image as opposed to a single section, I'd find it clearer if they were
loose in the SECTIONS { ... } block, so as not to imply that they are
coupled to any particular section. Thoughts?
Thanks for the cleanup,
Sam
> ---
> arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
> arch/arm/cpu/u-boot.lds | 10 ++--------
> arch/arm/lib/sections.c | 2 --
> arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
> arch/arm/mach-zynq/u-boot.lds | 9 ++-------
> 5 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index e737de761a9d..340acf5c043b 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -23,7 +23,7 @@ SECTIONS
> . = ALIGN(8);
> .text :
> {
> - *(.__image_copy_start)
> + __image_copy_start = .;
> CPUDIR/start.o (.text*)
> }
>
> @@ -120,13 +120,7 @@ SECTIONS
> *(.rel*.efi_runtime)
> *(.rel*.efi_runtime.*)
> __efi_runtime_rel_stop = .;
> - }
> -
> - . = ALIGN(8);
> -
> - .image_copy_end :
> - {
> - *(.__image_copy_end)
> + __image_copy_end = .;
> }
>
> .rela.dyn ALIGN(8) : {
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index df55bb716e35..7c4f9c2d4dd3 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -37,7 +37,7 @@ SECTIONS
> . = ALIGN(4);
> .text :
> {
> - *(.__image_copy_start)
> + __image_copy_start = .;
> *(.vectors)
> CPUDIR/start.o (.text*)
> }
> @@ -151,13 +151,7 @@ SECTIONS
> *(.rel*.efi_runtime)
> *(.rel*.efi_runtime.*)
> __efi_runtime_rel_stop = .;
> - }
> -
> - . = ALIGN(4);
> -
> - .image_copy_end :
> - {
> - *(.__image_copy_end)
> + __image_copy_end = .;
> }
>
> .rel.dyn ALIGN(4) : {
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index a4d4202e99f5..db5463b2bbbc 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -19,8 +19,6 @@
> * aliasing warnings.
> */
>
> -char __image_copy_start[0] __section(".__image_copy_start");
> -char __image_copy_end[0] __section(".__image_copy_end");
> char __secure_start[0] __section(".__secure_start");
> char __secure_end[0] __section(".__secure_end");
> char __secure_stack_start[0] __section(".__secure_stack_start");
> diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> index b7887194026e..4b480ebb0c4d 100644
> --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> @@ -24,7 +24,7 @@ SECTIONS
>
> .text : {
> . = ALIGN(8);
> - *(.__image_copy_start)
> + __image_copy_start = .;
> CPUDIR/start.o (.text*)
> *(.text*)
> }
> @@ -42,11 +42,7 @@ SECTIONS
> __u_boot_list : {
> . = ALIGN(8);
> KEEP(*(SORT(__u_boot_list*)));
> - }
> -
> - .image_copy_end : {
> - . = ALIGN(8);
> - *(.__image_copy_end)
> + __image_copy_end = .;
> }
>
> .end : {
> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> index fcd0f42a7106..553bcf182272 100644
> --- a/arch/arm/mach-zynq/u-boot.lds
> +++ b/arch/arm/mach-zynq/u-boot.lds
> @@ -16,7 +16,7 @@ SECTIONS
> . = ALIGN(4);
> .text :
> {
> - *(.__image_copy_start)
> + __image_copy_start = .;
> *(.vectors)
> CPUDIR/start.o (.text*)
> }
> @@ -57,12 +57,7 @@ SECTIONS
> *(.rel*.efi_runtime)
> *(.rel*.efi_runtime.*)
> __efi_runtime_rel_stop = .;
> - }
> -
> - . = ALIGN(8);
> - .image_copy_end :
> - {
> - *(.__image_copy_end)
> + __image_copy_end = .;
> }
>
> .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end
2024-03-06 7:32 ` Sam Edwards
@ 2024-03-06 9:08 ` Ilias Apalodimas
2024-03-06 10:11 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-06 9:08 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
Hi Sam,
First of all, thanks for taking the time to review this.
On Wed, 6 Mar 2024 at 09:32, Sam Edwards <cfsworks@gmail.com> wrote:
>
> On 3/4/24 02:01, Ilias Apalodimas wrote:
> > commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
> > and
> > commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts")
> > were moving the bss_start/end on c generated variables that were
> > injected in their own sections. The reason was that we needed relative
> > relocations for position independent code and linker bugs back then
> > prevented us from doing so.
> >
> > However, the linker documentation pages states that symbols that are
> > defined within a section definition will create a relocatable
> > type with the value being a fixed offset from the base of a section.
> > This have been fixed since 2016 [1]. So let's start cleaning this up
> > starting with the bss_start and bss_end variables. Convert them into
> > symbols within the .bss section definition.
> >
> > [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> > [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845
> > ---
> > arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++-----------
> > arch/arm/cpu/armv8/u-boot.lds | 15 +++------------
> > arch/arm/cpu/u-boot.lds | 22 ++++------------------
> > arch/arm/lib/sections.c | 2 --
> > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++-----------
> > arch/arm/mach-zynq/u-boot.lds | 21 +++------------------
> > 6 files changed, 16 insertions(+), 72 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
> > index 7cb9d731246d..16fddb46e9cb 100644
> > --- a/arch/arm/cpu/armv8/u-boot-spl.lds
> > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds
> > @@ -63,18 +63,10 @@ SECTIONS
> >
> > _image_binary_end = .;
> >
> > - .bss_start (NOLOAD) : {
> > - . = ALIGN(8);
> > - KEEP(*(.__bss_start));
> > - } >.sdram
> > -
> > - .bss (NOLOAD) : {
> > + .bss : {
> > + __bss_start = .;
> > *(.bss*)
> > - . = ALIGN(8);
> > - } >.sdram
> > -
> > - .bss_end (NOLOAD) : {
> > - KEEP(*(.__bss_end));
> > + __bss_end = .;
> > } >.sdram
> >
> > /DISCARD/ : { *(.rela*) }
> > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > index fb6a30c922f7..c4ee10ebc3ff 100644
> > --- a/arch/arm/cpu/armv8/u-boot.lds
> > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > @@ -149,19 +149,10 @@ SECTIONS
> >
> > _end = .;
> >
> > - . = ALIGN(8);
> > -
> > - .bss_start : {
> > - KEEP(*(.__bss_start));
> > - }
> > -
> > - .bss : {
> > + .bss ALIGN(8): {
> > + __bss_start = .;
> > *(.bss*)
> > - . = ALIGN(8);
> > - }
> > -
> > - .bss_end : {
> > - KEEP(*(.__bss_end));
> > + __bss_end = .;
> > }
> >
> > /DISCARD/ : { *(.dynsym) }
> > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > index 7724c9332c3b..90d329b1ebe0 100644
> > --- a/arch/arm/cpu/u-boot.lds
> > +++ b/arch/arm/cpu/u-boot.lds
> > @@ -206,27 +206,13 @@ SECTIONS
> > *(.mmutable)
> > }
> >
> > -/*
> > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > - * __bss_base and __bss_limit are for linker only (overlay ordering)
> > - */
> > -
> > - .bss_start __rel_dyn_start (OVERLAY) : {
> > - KEEP(*(.__bss_start));
> > - __bss_base = .;
> > - }
> > -
> > - .bss __bss_base (OVERLAY) : {
> > + .bss ALIGN(4): {
>
> Hi Ilias,
>
> I have been reviewing this patchset by diffing output binaries and
> linker maps between successive patches. Most of the patches in this
> series result in no change to the output binary whatsoever (which also
> means, no regressions!) but this one does have a change: .bss is being
> moved after .mmutable. You should be able to see this yourself by
> comparing `u-boot.map` after a successful build, before and after
> applying this patch.
Yes it does
>
> Since the current u-boot.lds puts .bss right after __image_copy_end
> (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I
> guess makes sense, if U-Boot zero-initializes .bss after applying
> relocations), perhaps this patch should be moving the .bss section up
> there in the .lds, putting (OVERLAY) back, and adding a comment to the
> effect of "These sections occupy the same memory, but their lifetimes do
> not overlap: U-Boot initializes .bss only after applying relocations and
> therefore after it doesn't need .rel.dyn any more."
We could do that, I honestly don't have a strong opinion on that.
>
> We might also decide that the overlay memory-saving trick isn't actually
> all that important anymore and that .bss should have a new home. Someone
> far more seasoned than I should be the one to make that decision though.
I am not sure tbh. I imagine that the overlay saving trick between
.rel_dyn and .bss would only matter in U-Boot SPL where memory is
limited. The v8 version of SPL doesn't have the overlay but the v7
does and I left it untouched on purpose. On top of that the overlay on
v7 for SPL is defined after the image_binary_end.
Perhaps Tom remembers a bit more of the lds history than I do. Tom thoughts?
Thanks!
/Ilias
>
> The rest of the patchset looks great! I'll add my tags to those in a bit.
>
> Cheers,
> Sam
>
> > + __bss_start = .;
> > *(.bss*)
> > - . = ALIGN(4);
> > - __bss_limit = .;
> > - }
> > -
> > - .bss_end __bss_limit (OVERLAY) : {
> > - KEEP(*(.__bss_end));
> > + __bss_end = .;
> > }
> >
> > - .dynsym _image_binary_end : { *(.dynsym) }
> > + .dynsym : { *(.dynsym) }
> > .dynbss : { *(.dynbss) }
> > .dynstr : { *(.dynstr*) }
> > .dynamic : { *(.dynamic*) }
> > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > index 857879711c6a..8e8bd5797e16 100644
> > --- a/arch/arm/lib/sections.c
> > +++ b/arch/arm/lib/sections.c
> > @@ -19,8 +19,6 @@
> > * aliasing warnings.
> > */
> >
> > -char __bss_start[0] __section(".__bss_start");
> > -char __bss_end[0] __section(".__bss_end");
> > char __image_copy_start[0] __section(".__image_copy_start");
> > char __image_copy_end[0] __section(".__image_copy_end");
> > char __rel_dyn_start[0] __section(".__rel_dyn_start");
> > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > index 74618eba591b..b7887194026e 100644
> > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > @@ -56,18 +56,10 @@ SECTIONS
> >
> > _image_binary_end = .;
> >
> > - .bss_start (NOLOAD) : {
> > - . = ALIGN(8);
> > - KEEP(*(.__bss_start));
> > - }
> > -
> > - .bss (NOLOAD) : {
> > + .bss ALIGN(8) : {
> > + __bss_start = .;
> > *(.bss*)
> > - . = ALIGN(8);
> > - }
> > -
> > - .bss_end (NOLOAD) : {
> > - KEEP(*(.__bss_end));
> > + __bss_end = .;
> > }
> >
> > /DISCARD/ : { *(.dynsym) }
> > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > index 3b7c9d515f8b..8d3259821719 100644
> > --- a/arch/arm/mach-zynq/u-boot.lds
> > +++ b/arch/arm/mach-zynq/u-boot.lds
> > @@ -102,26 +102,11 @@ SECTIONS
> >
> > _image_binary_end = .;
> >
> > -/*
> > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > - * __bss_base and __bss_limit are for linker only (overlay ordering)
> > - */
> > -
> > - .bss_start __rel_dyn_start (OVERLAY) : {
> > - KEEP(*(.__bss_start));
> > - __bss_base = .;
> > - }
> > -
> > - .bss __bss_base (OVERLAY) : {
> > + .bss ALIGN(4): {
> > + __bss_start = .;
> > *(.bss*)
> > - . = ALIGN(8);
> > - __bss_limit = .;
> > - }
> > -
> > - .bss_end __bss_limit (OVERLAY) : {
> > - KEEP(*(.__bss_end));
> > + __bss_end = .;
> > }
> > -
> > /*
> > * Zynq needs to discard these sections because the user
> > * is expected to pass this image on to tools for boot.bin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-06 8:14 ` Sam Edwards
@ 2024-03-06 9:13 ` Ilias Apalodimas
2024-03-06 22:19 ` Sam Edwards
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-06 9:13 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
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?
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 */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-06 8:22 ` Sam Edwards
@ 2024-03-06 9:35 ` Ilias Apalodimas
2024-03-06 10:37 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-06 9:35 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
Hi Sam,
On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks@gmail.com> wrote:
>
> On 3/4/24 02:01, Ilias Apalodimas wrote:
> > image_copy_start/end are defined as c variables in order to force the compiler
> > emit relative references. However, defining those within a section definition
> > will do the same thing.
> >
> > So let's remove the special sections from the linker scripts, the
> > variable definitions from sections.c and define them as a symbols within
> > a section.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
>
> Since the __image_copy_* symbols are marking boundaries in the whole
> image as opposed to a single section, I'd find it clearer if they were
> loose in the SECTIONS { ... } block, so as not to imply that they are
> coupled to any particular section. Thoughts?
The reason I included it within a section is my understanding of the
ld (way older) manual [0].
Copy-pasting from that:
" Assignment statements may appear:
- as commands in their own right in an ld script; or
- as independent statements within a SECTIONS command; or
- as part of the contents of a section definition in a SECTIONS command.
The first two cases are equivalent in effect--both define a symbol
with an absolute address. The last case defines a symbol whose address
is relative to a particular section."
So, since we need relocatable entries, I included it within a section.
Looking at the latest ld [1] the wording has changed a bit it says
"Expressions appearing outside an output section definition treat all
numbers as absolute addresses" and it also has the following example
SECTIONS
{
. = 0x100;
__executable_start = 0x100;
.data :
{
. = 0x10;
__data_start = 0x10;
*(.data)
}
…
}
both . and __executable_start are set to the absolute address 0x100 in
the first two assignments, then both . and __data_start are set to
0x10 relative to the .data section in the second two assignments.
So I assume the same behavior persists?
[0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
[1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
Thanks
/Ilias
>
> Thanks for the cleanup,
> Sam
>
> > ---
> > arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
> > arch/arm/cpu/u-boot.lds | 10 ++--------
> > arch/arm/lib/sections.c | 2 --
> > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
> > arch/arm/mach-zynq/u-boot.lds | 9 ++-------
> > 5 files changed, 8 insertions(+), 31 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > index e737de761a9d..340acf5c043b 100644
> > --- a/arch/arm/cpu/armv8/u-boot.lds
> > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > @@ -23,7 +23,7 @@ SECTIONS
> > . = ALIGN(8);
> > .text :
> > {
> > - *(.__image_copy_start)
> > + __image_copy_start = .;
> > CPUDIR/start.o (.text*)
> > }
> >
> > @@ -120,13 +120,7 @@ SECTIONS
> > *(.rel*.efi_runtime)
> > *(.rel*.efi_runtime.*)
> > __efi_runtime_rel_stop = .;
> > - }
> > -
> > - . = ALIGN(8);
> > -
> > - .image_copy_end :
> > - {
> > - *(.__image_copy_end)
> > + __image_copy_end = .;
> > }
> >
> > .rela.dyn ALIGN(8) : {
> > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > index df55bb716e35..7c4f9c2d4dd3 100644
> > --- a/arch/arm/cpu/u-boot.lds
> > +++ b/arch/arm/cpu/u-boot.lds
> > @@ -37,7 +37,7 @@ SECTIONS
> > . = ALIGN(4);
> > .text :
> > {
> > - *(.__image_copy_start)
> > + __image_copy_start = .;
> > *(.vectors)
> > CPUDIR/start.o (.text*)
> > }
> > @@ -151,13 +151,7 @@ SECTIONS
> > *(.rel*.efi_runtime)
> > *(.rel*.efi_runtime.*)
> > __efi_runtime_rel_stop = .;
> > - }
> > -
> > - . = ALIGN(4);
> > -
> > - .image_copy_end :
> > - {
> > - *(.__image_copy_end)
> > + __image_copy_end = .;
> > }
> >
> > .rel.dyn ALIGN(4) : {
> > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > index a4d4202e99f5..db5463b2bbbc 100644
> > --- a/arch/arm/lib/sections.c
> > +++ b/arch/arm/lib/sections.c
> > @@ -19,8 +19,6 @@
> > * aliasing warnings.
> > */
> >
> > -char __image_copy_start[0] __section(".__image_copy_start");
> > -char __image_copy_end[0] __section(".__image_copy_end");
> > char __secure_start[0] __section(".__secure_start");
> > char __secure_end[0] __section(".__secure_end");
> > char __secure_stack_start[0] __section(".__secure_stack_start");
> > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > index b7887194026e..4b480ebb0c4d 100644
> > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > @@ -24,7 +24,7 @@ SECTIONS
> >
> > .text : {
> > . = ALIGN(8);
> > - *(.__image_copy_start)
> > + __image_copy_start = .;
> > CPUDIR/start.o (.text*)
> > *(.text*)
> > }
> > @@ -42,11 +42,7 @@ SECTIONS
> > __u_boot_list : {
> > . = ALIGN(8);
> > KEEP(*(SORT(__u_boot_list*)));
> > - }
> > -
> > - .image_copy_end : {
> > - . = ALIGN(8);
> > - *(.__image_copy_end)
> > + __image_copy_end = .;
> > }
> >
> > .end : {
> > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > index fcd0f42a7106..553bcf182272 100644
> > --- a/arch/arm/mach-zynq/u-boot.lds
> > +++ b/arch/arm/mach-zynq/u-boot.lds
> > @@ -16,7 +16,7 @@ SECTIONS
> > . = ALIGN(4);
> > .text :
> > {
> > - *(.__image_copy_start)
> > + __image_copy_start = .;
> > *(.vectors)
> > CPUDIR/start.o (.text*)
> > }
> > @@ -57,12 +57,7 @@ SECTIONS
> > *(.rel*.efi_runtime)
> > *(.rel*.efi_runtime.*)
> > __efi_runtime_rel_stop = .;
> > - }
> > -
> > - . = ALIGN(8);
> > - .image_copy_end :
> > - {
> > - *(.__image_copy_end)
> > + __image_copy_end = .;
> > }
> >
> > .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end
2024-03-06 9:08 ` Ilias Apalodimas
@ 2024-03-06 10:11 ` Ilias Apalodimas
0 siblings, 0 replies; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-06 10:11 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Wed, 6 Mar 2024 at 11:08, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sam,
>
> First of all, thanks for taking the time to review this.
>
> On Wed, 6 Mar 2024 at 09:32, Sam Edwards <cfsworks@gmail.com> wrote:
> >
> > On 3/4/24 02:01, Ilias Apalodimas wrote:
> > > commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
> > > and
> > > commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts")
> > > were moving the bss_start/end on c generated variables that were
> > > injected in their own sections. The reason was that we needed relative
> > > relocations for position independent code and linker bugs back then
> > > prevented us from doing so.
> > >
> > > However, the linker documentation pages states that symbols that are
> > > defined within a section definition will create a relocatable
> > > type with the value being a fixed offset from the base of a section.
> > > This have been fixed since 2016 [1]. So let's start cleaning this up
> > > starting with the bss_start and bss_end variables. Convert them into
> > > symbols within the .bss section definition.
> > >
> > > [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> > > [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845
> > > ---
> > > arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++-----------
> > > arch/arm/cpu/armv8/u-boot.lds | 15 +++------------
> > > arch/arm/cpu/u-boot.lds | 22 ++++------------------
> > > arch/arm/lib/sections.c | 2 --
> > > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++-----------
> > > arch/arm/mach-zynq/u-boot.lds | 21 +++------------------
> > > 6 files changed, 16 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
> > > index 7cb9d731246d..16fddb46e9cb 100644
> > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds
> > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds
> > > @@ -63,18 +63,10 @@ SECTIONS
> > >
> > > _image_binary_end = .;
> > >
> > > - .bss_start (NOLOAD) : {
> > > - . = ALIGN(8);
> > > - KEEP(*(.__bss_start));
> > > - } >.sdram
> > > -
> > > - .bss (NOLOAD) : {
> > > + .bss : {
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(8);
> > > - } >.sdram
> > > -
> > > - .bss_end (NOLOAD) : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > } >.sdram
> > >
> > > /DISCARD/ : { *(.rela*) }
> > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > > index fb6a30c922f7..c4ee10ebc3ff 100644
> > > --- a/arch/arm/cpu/armv8/u-boot.lds
> > > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > > @@ -149,19 +149,10 @@ SECTIONS
> > >
> > > _end = .;
> > >
> > > - . = ALIGN(8);
> > > -
> > > - .bss_start : {
> > > - KEEP(*(.__bss_start));
> > > - }
> > > -
> > > - .bss : {
> > > + .bss ALIGN(8): {
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(8);
> > > - }
> > > -
> > > - .bss_end : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > >
> > > /DISCARD/ : { *(.dynsym) }
> > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > index 7724c9332c3b..90d329b1ebe0 100644
> > > --- a/arch/arm/cpu/u-boot.lds
> > > +++ b/arch/arm/cpu/u-boot.lds
> > > @@ -206,27 +206,13 @@ SECTIONS
> > > *(.mmutable)
> > > }
> > >
> > > -/*
> > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > - * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > - */
> > > -
> > > - .bss_start __rel_dyn_start (OVERLAY) : {
> > > - KEEP(*(.__bss_start));
> > > - __bss_base = .;
> > > - }
> > > -
> > > - .bss __bss_base (OVERLAY) : {
> > > + .bss ALIGN(4): {
> >
> > Hi Ilias,
> >
> > I have been reviewing this patchset by diffing output binaries and
> > linker maps between successive patches. Most of the patches in this
> > series result in no change to the output binary whatsoever (which also
> > means, no regressions!) but this one does have a change: .bss is being
> > moved after .mmutable. You should be able to see this yourself by
> > comparing `u-boot.map` after a successful build, before and after
> > applying this patch.
>
> Yes it does
>
> >
> > Since the current u-boot.lds puts .bss right after __image_copy_end
> > (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I
> > guess makes sense, if U-Boot zero-initializes .bss after applying
> > relocations), perhaps this patch should be moving the .bss section up
> > there in the .lds, putting (OVERLAY) back, and adding a comment to the
> > effect of "These sections occupy the same memory, but their lifetimes do
> > not overlap: U-Boot initializes .bss only after applying relocations and
> > therefore after it doesn't need .rel.dyn any more."
>
> We could do that, I honestly don't have a strong opinion on that.
replying to myself here, but there was a relevant discussion here as well
https://lore.kernel.org/u-boot/ZcJF2uGIMj-jxT3M@hera/
>
> >
> > We might also decide that the overlay memory-saving trick isn't actually
> > all that important anymore and that .bss should have a new home. Someone
> > far more seasoned than I should be the one to make that decision though.
>
> I am not sure tbh. I imagine that the overlay saving trick between
> .rel_dyn and .bss would only matter in U-Boot SPL where memory is
> limited. The v8 version of SPL doesn't have the overlay but the v7
> does and I left it untouched on purpose. On top of that the overlay on
> v7 for SPL is defined after the image_binary_end.
> Perhaps Tom remembers a bit more of the lds history than I do. Tom thoughts?
>
> Thanks!
> /Ilias
>
> >
> > The rest of the patchset looks great! I'll add my tags to those in a bit.
> >
> > Cheers,
> > Sam
> >
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(4);
> > > - __bss_limit = .;
> > > - }
> > > -
> > > - .bss_end __bss_limit (OVERLAY) : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > >
> > > - .dynsym _image_binary_end : { *(.dynsym) }
> > > + .dynsym : { *(.dynsym) }
> > > .dynbss : { *(.dynbss) }
> > > .dynstr : { *(.dynstr*) }
> > > .dynamic : { *(.dynamic*) }
> > > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > > index 857879711c6a..8e8bd5797e16 100644
> > > --- a/arch/arm/lib/sections.c
> > > +++ b/arch/arm/lib/sections.c
> > > @@ -19,8 +19,6 @@
> > > * aliasing warnings.
> > > */
> > >
> > > -char __bss_start[0] __section(".__bss_start");
> > > -char __bss_end[0] __section(".__bss_end");
> > > char __image_copy_start[0] __section(".__image_copy_start");
> > > char __image_copy_end[0] __section(".__image_copy_end");
> > > char __rel_dyn_start[0] __section(".__rel_dyn_start");
> > > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > index 74618eba591b..b7887194026e 100644
> > > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > @@ -56,18 +56,10 @@ SECTIONS
> > >
> > > _image_binary_end = .;
> > >
> > > - .bss_start (NOLOAD) : {
> > > - . = ALIGN(8);
> > > - KEEP(*(.__bss_start));
> > > - }
> > > -
> > > - .bss (NOLOAD) : {
> > > + .bss ALIGN(8) : {
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(8);
> > > - }
> > > -
> > > - .bss_end (NOLOAD) : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > >
> > > /DISCARD/ : { *(.dynsym) }
> > > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > > index 3b7c9d515f8b..8d3259821719 100644
> > > --- a/arch/arm/mach-zynq/u-boot.lds
> > > +++ b/arch/arm/mach-zynq/u-boot.lds
> > > @@ -102,26 +102,11 @@ SECTIONS
> > >
> > > _image_binary_end = .;
> > >
> > > -/*
> > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > - * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > - */
> > > -
> > > - .bss_start __rel_dyn_start (OVERLAY) : {
> > > - KEEP(*(.__bss_start));
> > > - __bss_base = .;
> > > - }
> > > -
> > > - .bss __bss_base (OVERLAY) : {
> > > + .bss ALIGN(4): {
> > > + __bss_start = .;
> > > *(.bss*)
> > > - . = ALIGN(8);
> > > - __bss_limit = .;
> > > - }
> > > -
> > > - .bss_end __bss_limit (OVERLAY) : {
> > > - KEEP(*(.__bss_end));
> > > + __bss_end = .;
> > > }
> > > -
> > > /*
> > > * Zynq needs to discard these sections because the user
> > > * is expected to pass this image on to tools for boot.bin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-06 9:35 ` Ilias Apalodimas
@ 2024-03-06 10:37 ` Ilias Apalodimas
2024-03-06 13:23 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-06 10:37 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sam,
>
>
> On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks@gmail.com> wrote:
> >
> > On 3/4/24 02:01, Ilias Apalodimas wrote:
> > > image_copy_start/end are defined as c variables in order to force the compiler
> > > emit relative references. However, defining those within a section definition
> > > will do the same thing.
> > >
> > > So let's remove the special sections from the linker scripts, the
> > > variable definitions from sections.c and define them as a symbols within
> > > a section.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
> >
> > Since the __image_copy_* symbols are marking boundaries in the whole
> > image as opposed to a single section, I'd find it clearer if they were
> > loose in the SECTIONS { ... } block, so as not to imply that they are
> > coupled to any particular section. Thoughts?
>
> The reason I included it within a section is my understanding of the
> ld (way older) manual [0].
> Copy-pasting from that:
>
> " Assignment statements may appear:
> - as commands in their own right in an ld script; or
> - as independent statements within a SECTIONS command; or
> - as part of the contents of a section definition in a SECTIONS command.
> The first two cases are equivalent in effect--both define a symbol
> with an absolute address. The last case defines a symbol whose address
> is relative to a particular section."
> So, since we need relocatable entries, I included it within a section.
>
> Looking at the latest ld [1] the wording has changed a bit it says
> "Expressions appearing outside an output section definition treat all
> numbers as absolute addresses" and it also has the following example
> SECTIONS
> {
> . = 0x100;
> __executable_start = 0x100;
> .data :
> {
> . = 0x10;
> __data_start = 0x10;
> *(.data)
> }
> …
> }
> both . and __executable_start are set to the absolute address 0x100 in
> the first two assignments, then both . and __data_start are set to
> 0x10 relative to the .data section in the second two assignments.
> So I assume the same behavior persists?
I just tested moving image_binary_end outside the section definition
and it's still emitted properly. I'll try to figure this out and on v3
I'll move both image_copy_start/end outside the sections.
I also noticed that I forgot to change
arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.
Thanks
/Ilias
>
> [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
>
> Thanks
> /Ilias
>
>
> >
> > Thanks for the cleanup,
> > Sam
> >
> > > ---
> > > arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
> > > arch/arm/cpu/u-boot.lds | 10 ++--------
> > > arch/arm/lib/sections.c | 2 --
> > > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
> > > arch/arm/mach-zynq/u-boot.lds | 9 ++-------
> > > 5 files changed, 8 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > > index e737de761a9d..340acf5c043b 100644
> > > --- a/arch/arm/cpu/armv8/u-boot.lds
> > > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > > @@ -23,7 +23,7 @@ SECTIONS
> > > . = ALIGN(8);
> > > .text :
> > > {
> > > - *(.__image_copy_start)
> > > + __image_copy_start = .;
> > > CPUDIR/start.o (.text*)
> > > }
> > >
> > > @@ -120,13 +120,7 @@ SECTIONS
> > > *(.rel*.efi_runtime)
> > > *(.rel*.efi_runtime.*)
> > > __efi_runtime_rel_stop = .;
> > > - }
> > > -
> > > - . = ALIGN(8);
> > > -
> > > - .image_copy_end :
> > > - {
> > > - *(.__image_copy_end)
> > > + __image_copy_end = .;
> > > }
> > >
> > > .rela.dyn ALIGN(8) : {
> > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > index df55bb716e35..7c4f9c2d4dd3 100644
> > > --- a/arch/arm/cpu/u-boot.lds
> > > +++ b/arch/arm/cpu/u-boot.lds
> > > @@ -37,7 +37,7 @@ SECTIONS
> > > . = ALIGN(4);
> > > .text :
> > > {
> > > - *(.__image_copy_start)
> > > + __image_copy_start = .;
> > > *(.vectors)
> > > CPUDIR/start.o (.text*)
> > > }
> > > @@ -151,13 +151,7 @@ SECTIONS
> > > *(.rel*.efi_runtime)
> > > *(.rel*.efi_runtime.*)
> > > __efi_runtime_rel_stop = .;
> > > - }
> > > -
> > > - . = ALIGN(4);
> > > -
> > > - .image_copy_end :
> > > - {
> > > - *(.__image_copy_end)
> > > + __image_copy_end = .;
> > > }
> > >
> > > .rel.dyn ALIGN(4) : {
> > > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > > index a4d4202e99f5..db5463b2bbbc 100644
> > > --- a/arch/arm/lib/sections.c
> > > +++ b/arch/arm/lib/sections.c
> > > @@ -19,8 +19,6 @@
> > > * aliasing warnings.
> > > */
> > >
> > > -char __image_copy_start[0] __section(".__image_copy_start");
> > > -char __image_copy_end[0] __section(".__image_copy_end");
> > > char __secure_start[0] __section(".__secure_start");
> > > char __secure_end[0] __section(".__secure_end");
> > > char __secure_stack_start[0] __section(".__secure_stack_start");
> > > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > index b7887194026e..4b480ebb0c4d 100644
> > > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > @@ -24,7 +24,7 @@ SECTIONS
> > >
> > > .text : {
> > > . = ALIGN(8);
> > > - *(.__image_copy_start)
> > > + __image_copy_start = .;
> > > CPUDIR/start.o (.text*)
> > > *(.text*)
> > > }
> > > @@ -42,11 +42,7 @@ SECTIONS
> > > __u_boot_list : {
> > > . = ALIGN(8);
> > > KEEP(*(SORT(__u_boot_list*)));
> > > - }
> > > -
> > > - .image_copy_end : {
> > > - . = ALIGN(8);
> > > - *(.__image_copy_end)
> > > + __image_copy_end = .;
> > > }
> > >
> > > .end : {
> > > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > > index fcd0f42a7106..553bcf182272 100644
> > > --- a/arch/arm/mach-zynq/u-boot.lds
> > > +++ b/arch/arm/mach-zynq/u-boot.lds
> > > @@ -16,7 +16,7 @@ SECTIONS
> > > . = ALIGN(4);
> > > .text :
> > > {
> > > - *(.__image_copy_start)
> > > + __image_copy_start = .;
> > > *(.vectors)
> > > CPUDIR/start.o (.text*)
> > > }
> > > @@ -57,12 +57,7 @@ SECTIONS
> > > *(.rel*.efi_runtime)
> > > *(.rel*.efi_runtime.*)
> > > __efi_runtime_rel_stop = .;
> > > - }
> > > -
> > > - . = ALIGN(8);
> > > - .image_copy_end :
> > > - {
> > > - *(.__image_copy_end)
> > > + __image_copy_end = .;
> > > }
> > >
> > > .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-06 10:37 ` Ilias Apalodimas
@ 2024-03-06 13:23 ` Ilias Apalodimas
2024-03-06 23:08 ` Sam Edwards
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-06 13:23 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Sam,
> >
> >
> > On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks@gmail.com> wrote:
> > >
> > > On 3/4/24 02:01, Ilias Apalodimas wrote:
> > > > image_copy_start/end are defined as c variables in order to force the compiler
> > > > emit relative references. However, defining those within a section definition
> > > > will do the same thing.
> > > >
> > > > So let's remove the special sections from the linker scripts, the
> > > > variable definitions from sections.c and define them as a symbols within
> > > > a section.
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
> > >
> > > Since the __image_copy_* symbols are marking boundaries in the whole
> > > image as opposed to a single section, I'd find it clearer if they were
> > > loose in the SECTIONS { ... } block, so as not to imply that they are
> > > coupled to any particular section. Thoughts?
> >
> > The reason I included it within a section is my understanding of the
> > ld (way older) manual [0].
> > Copy-pasting from that:
> >
> > " Assignment statements may appear:
> > - as commands in their own right in an ld script; or
> > - as independent statements within a SECTIONS command; or
> > - as part of the contents of a section definition in a SECTIONS command.
> > The first two cases are equivalent in effect--both define a symbol
> > with an absolute address. The last case defines a symbol whose address
> > is relative to a particular section."
> > So, since we need relocatable entries, I included it within a section.
> >
> > Looking at the latest ld [1] the wording has changed a bit it says
> > "Expressions appearing outside an output section definition treat all
> > numbers as absolute addresses" and it also has the following example
> > SECTIONS
> > {
> > . = 0x100;
> > __executable_start = 0x100;
> > .data :
> > {
> > . = 0x10;
> > __data_start = 0x10;
> > *(.data)
> > }
> > …
> > }
> > both . and __executable_start are set to the absolute address 0x100 in
> > the first two assignments, then both . and __data_start are set to
> > 0x10 relative to the .data section in the second two assignments.
> > So I assume the same behavior persists?
>
> I just tested moving image_binary_end outside the section definition
> and it's still emitted properly. I'll try to figure this out and on v3
> I'll move both image_copy_start/end outside the sections.
> I also noticed that I forgot to change
> arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.
Reading the manual again, the symbols will be emitted as relocatable
entries regardless of their placement after that LD bug you mentioned
is fixed.
The only thing that will change when moving it outside the section
definition is that an absolute value will be set, instead of a
relative one to the start of the section. But that won't change the
final binary placement in our case.
I played around a bit and removed the .image_copy_end section from the
SPL in favor of a symbol.
Compiling for xilinx_zynqmp_kria_defconfig which builds u-boot & SPL
(note that I am building natively on an arm64 box)
# Before -- image_copy_end is .efi_runtime_rel and spl still has a
section for .image_copy_end
$:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
$:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
5: 00000000fffe0dc8 0 SECTION LOCAL DEFAULT 5 .image_copy_end
1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
# After -- image_copy_end moved outside .efi_runtime_rel and
.image_copy_end converted to a linker symbol
$:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
$:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
1349: 00000000fffe0dc8 0 NOTYPE GLOBAL DEFAULT 4 __image_copy_end
1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
Nothing changes in offsets and sizes.
The only thing I won't do right now is move image_copy_start outside
the text region since that accounts for the CONFIG_SPL_TEXT_BASE and
CONFIG_SYS_LOAD_ADDR. Keeping the __image_copy_start in there has an
obvious caveat -- __image_copy_start will end up in .text and
__image_copy_end in .rodata, but since we always had that it's fine
for now.
Thanks
/Ilias
>
> Thanks
> /Ilias
>
>
> >
> > [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> > [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
> >
> > Thanks
> > /Ilias
> >
> >
> > >
> > > Thanks for the cleanup,
> > > Sam
> > >
> > > > ---
> > > > arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
> > > > arch/arm/cpu/u-boot.lds | 10 ++--------
> > > > arch/arm/lib/sections.c | 2 --
> > > > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
> > > > arch/arm/mach-zynq/u-boot.lds | 9 ++-------
> > > > 5 files changed, 8 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > > > index e737de761a9d..340acf5c043b 100644
> > > > --- a/arch/arm/cpu/armv8/u-boot.lds
> > > > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > > > @@ -23,7 +23,7 @@ SECTIONS
> > > > . = ALIGN(8);
> > > > .text :
> > > > {
> > > > - *(.__image_copy_start)
> > > > + __image_copy_start = .;
> > > > CPUDIR/start.o (.text*)
> > > > }
> > > >
> > > > @@ -120,13 +120,7 @@ SECTIONS
> > > > *(.rel*.efi_runtime)
> > > > *(.rel*.efi_runtime.*)
> > > > __efi_runtime_rel_stop = .;
> > > > - }
> > > > -
> > > > - . = ALIGN(8);
> > > > -
> > > > - .image_copy_end :
> > > > - {
> > > > - *(.__image_copy_end)
> > > > + __image_copy_end = .;
> > > > }
> > > >
> > > > .rela.dyn ALIGN(8) : {
> > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > index df55bb716e35..7c4f9c2d4dd3 100644
> > > > --- a/arch/arm/cpu/u-boot.lds
> > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > @@ -37,7 +37,7 @@ SECTIONS
> > > > . = ALIGN(4);
> > > > .text :
> > > > {
> > > > - *(.__image_copy_start)
> > > > + __image_copy_start = .;
> > > > *(.vectors)
> > > > CPUDIR/start.o (.text*)
> > > > }
> > > > @@ -151,13 +151,7 @@ SECTIONS
> > > > *(.rel*.efi_runtime)
> > > > *(.rel*.efi_runtime.*)
> > > > __efi_runtime_rel_stop = .;
> > > > - }
> > > > -
> > > > - . = ALIGN(4);
> > > > -
> > > > - .image_copy_end :
> > > > - {
> > > > - *(.__image_copy_end)
> > > > + __image_copy_end = .;
> > > > }
> > > >
> > > > .rel.dyn ALIGN(4) : {
> > > > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > > > index a4d4202e99f5..db5463b2bbbc 100644
> > > > --- a/arch/arm/lib/sections.c
> > > > +++ b/arch/arm/lib/sections.c
> > > > @@ -19,8 +19,6 @@
> > > > * aliasing warnings.
> > > > */
> > > >
> > > > -char __image_copy_start[0] __section(".__image_copy_start");
> > > > -char __image_copy_end[0] __section(".__image_copy_end");
> > > > char __secure_start[0] __section(".__secure_start");
> > > > char __secure_end[0] __section(".__secure_end");
> > > > char __secure_stack_start[0] __section(".__secure_stack_start");
> > > > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > > index b7887194026e..4b480ebb0c4d 100644
> > > > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > > > @@ -24,7 +24,7 @@ SECTIONS
> > > >
> > > > .text : {
> > > > . = ALIGN(8);
> > > > - *(.__image_copy_start)
> > > > + __image_copy_start = .;
> > > > CPUDIR/start.o (.text*)
> > > > *(.text*)
> > > > }
> > > > @@ -42,11 +42,7 @@ SECTIONS
> > > > __u_boot_list : {
> > > > . = ALIGN(8);
> > > > KEEP(*(SORT(__u_boot_list*)));
> > > > - }
> > > > -
> > > > - .image_copy_end : {
> > > > - . = ALIGN(8);
> > > > - *(.__image_copy_end)
> > > > + __image_copy_end = .;
> > > > }
> > > >
> > > > .end : {
> > > > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > > > index fcd0f42a7106..553bcf182272 100644
> > > > --- a/arch/arm/mach-zynq/u-boot.lds
> > > > +++ b/arch/arm/mach-zynq/u-boot.lds
> > > > @@ -16,7 +16,7 @@ SECTIONS
> > > > . = ALIGN(4);
> > > > .text :
> > > > {
> > > > - *(.__image_copy_start)
> > > > + __image_copy_start = .;
> > > > *(.vectors)
> > > > CPUDIR/start.o (.text*)
> > > > }
> > > > @@ -57,12 +57,7 @@ SECTIONS
> > > > *(.rel*.efi_runtime)
> > > > *(.rel*.efi_runtime.*)
> > > > __efi_runtime_rel_stop = .;
> > > > - }
> > > > -
> > > > - . = ALIGN(8);
> > > > - .image_copy_end :
> > > > - {
> > > > - *(.__image_copy_end)
> > > > + __image_copy_end = .;
> > > > }
> > > >
> > > > .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-06 9:13 ` Ilias Apalodimas
@ 2024-03-06 22:19 ` Sam Edwards
2024-03-07 6:50 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 22:19 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On 3/6/24 02:13, Ilias Apalodimas wrote:
> Hi Sam,
>
> Again thank you for the elaborate review. This really helps a lot.
>
> On Wed, 6 Mar 2024 at 10:14, Sam Edwards <cfsworks@gmail.com> wrote:
>>
>>
>>
>> On 3/4/24 02:01, Ilias Apalodimas wrote:
>>> __efi_runtime_start/end are defined as c variables for arm7 only in
>>> order to force the compiler emit relative references. However, defining
>>> those within a section definition will do the same thing. On top of that
>>> the v8 linker scripts define it as a symbol.
>>>
>>> So let's remove the special sections from the linker scripts, the
>>> variable definitions from sections.c and define them as a symbols within
>>> the correct section.
>>>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
>>
>> Thanks for the cleanup,
>> Sam
>>
>>> ---
>>> arch/arm/cpu/u-boot.lds | 12 +++---------
>>> arch/arm/lib/sections.c | 2 --
>>> arch/arm/mach-zynq/u-boot.lds | 12 +++---------
>>> include/asm-generic/sections.h | 1 +
>>> 4 files changed, 7 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>>> index 7c6e7891d360..df55bb716e35 100644
>>> --- a/arch/arm/cpu/u-boot.lds
>>> +++ b/arch/arm/cpu/u-boot.lds
>>> @@ -43,18 +43,12 @@ SECTIONS
>>> }
>>>
>>> /* This needs to come before *(.text*) */
>>> - .__efi_runtime_start : {
>>> - *(.__efi_runtime_start)
>>> - }
>>> -
>>> - .efi_runtime : {
>>> + .efi_runtime ALIGN(4) : {
>>
>> Do we truly require the ALIGN(4)? If I understand correctly, by default,
>> the linker calculates the alignment of an output section as the least
>> common multiple of the input sections' alignment requirements -- meaning
>> most (perhaps all) of our ALIGN()s today are redundant.
>
> I don't think we do. But I preserved those for a few reasons.
>
>> For the time
>> being, I'm in favor of merging existing `. = ALIGN(x)` into each
>> following section for clarity and to avoid the testing overhead of
>> removing them in the same patch as other changes. However, in the near
>> future (perhaps even as "near" as v2 of this series?), I'd also like to
>> see a patch that eliminates unnecessary ALIGN()s altogether. Introducing
>> additional ALIGN()s where we already know they aren't needed may be a
>> step away from that goal.
>
> So as you already mentioned the reason I preserved this is:
> - Reduce the testing overhead by preserving the same layout for now
> - In the future, I am playing around with the idea of mapping U-Boot
> (not SPL but the relocated full U-Boot) in sections with proper memory
> permissions (R), RW^X etc). In that case, we will need a 4k section
> alignment and we can repurpose the ALIGN(4/8) to
> ALIGN(CONSTANT(COMMONPAGESIZE)).
>
> Thoughts?
Ah, right: I had forgotten for a moment that the ultimate goal was to
clean up the memory protection bit situation.
Okay, as long as that future cleanup will also remove all unnecessary
uses of ALIGN() (i.e. any that don't result in an actual change in
memory layout) then that sounds great to me.
Reviewed-by: Sam Edwards <CFSworks@gmail.com>
Cheers,
Sam
>
> Thanks
> /Ilias
>
>>
>>> + __efi_runtime_start = .;
>>> *(.text.efi_runtime*)
>>> *(.rodata.efi_runtime*)
>>> *(.data.efi_runtime*)
>>> - }
>>> -
>>> - .__efi_runtime_stop : {
>>> - *(.__efi_runtime_stop)
>>> + __efi_runtime_stop = .;
>>> }
>>>
>>> .text_rest :
>>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
>>> index 1ee3dd3667ba..a4d4202e99f5 100644
>>> --- a/arch/arm/lib/sections.c
>>> +++ b/arch/arm/lib/sections.c
>>> @@ -25,6 +25,4 @@ char __secure_start[0] __section(".__secure_start");
>>> char __secure_end[0] __section(".__secure_end");
>>> char __secure_stack_start[0] __section(".__secure_stack_start");
>>> char __secure_stack_end[0] __section(".__secure_stack_end");
>>> -char __efi_runtime_start[0] __section(".__efi_runtime_start");
>>> -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
>>> char _end[0] __section(".__end");
>>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
>>> index 71dea4a1f60a..fcd0f42a7106 100644
>>> --- a/arch/arm/mach-zynq/u-boot.lds
>>> +++ b/arch/arm/mach-zynq/u-boot.lds
>>> @@ -22,18 +22,12 @@ SECTIONS
>>> }
>>>
>>> /* This needs to come before *(.text*) */
>>> - .__efi_runtime_start : {
>>> - *(.__efi_runtime_start)
>>> - }
>>> -
>>> - .efi_runtime : {
>>> + .efi_runtime ALIGN(4) : {
>>
>> Ditto above
>>
>>> + __efi_runtime_start = .;
>>> *(.text.efi_runtime*)
>>> *(.rodata.efi_runtime*)
>>> *(.data.efi_runtime*)
>>> - }
>>> -
>>> - .__efi_runtime_stop : {
>>> - *(.__efi_runtime_stop)
>>> + __efi_runtime_stop = .;
>>> }
>>>
>>> .text_rest :
>>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>>> index 60949200dd93..b6bca53db10d 100644
>>> --- a/include/asm-generic/sections.h
>>> +++ b/include/asm-generic/sections.h
>>> @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
>>> extern char __ctors_start[], __ctors_end[];
>>>
>>> extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
>>> +extern char __efi_runtime_start[], __efi_runtime_stop[];
>>>
>>> /* function descriptor handling (if any). Override
>>> * in asm/sections.h */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-06 13:23 ` Ilias Apalodimas
@ 2024-03-06 23:08 ` Sam Edwards
2024-03-07 6:55 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Sam Edwards @ 2024-03-06 23:08 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On 3/6/24 06:23, Ilias Apalodimas wrote:
> On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>>
>>> Hi Sam,
>>>
>>>
>>> On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks@gmail.com> wrote:
>>>>
>>>> On 3/4/24 02:01, Ilias Apalodimas wrote:
>>>>> image_copy_start/end are defined as c variables in order to force the compiler
>>>>> emit relative references. However, defining those within a section definition
>>>>> will do the same thing.
>>>>>
>>>>> So let's remove the special sections from the linker scripts, the
>>>>> variable definitions from sections.c and define them as a symbols within
>>>>> a section.
>>>>>
>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
>>>>
>>>> Since the __image_copy_* symbols are marking boundaries in the whole
>>>> image as opposed to a single section, I'd find it clearer if they were
>>>> loose in the SECTIONS { ... } block, so as not to imply that they are
>>>> coupled to any particular section. Thoughts?
>>>
>>> The reason I included it within a section is my understanding of the
>>> ld (way older) manual [0].
>>> Copy-pasting from that:
>>>
>>> " Assignment statements may appear:
>>> - as commands in their own right in an ld script; or
>>> - as independent statements within a SECTIONS command; or
>>> - as part of the contents of a section definition in a SECTIONS command.
>>> The first two cases are equivalent in effect--both define a symbol
>>> with an absolute address. The last case defines a symbol whose address
>>> is relative to a particular section."
>>> So, since we need relocatable entries, I included it within a section.
>>>
>>> Looking at the latest ld [1] the wording has changed a bit it says
>>> "Expressions appearing outside an output section definition treat all
>>> numbers as absolute addresses" and it also has the following example
>>> SECTIONS
>>> {
>>> . = 0x100;
>>> __executable_start = 0x100;
>>> .data :
>>> {
>>> . = 0x10;
>>> __data_start = 0x10;
>>> *(.data)
>>> }
>>> …
>>> }
>>> both . and __executable_start are set to the absolute address 0x100 in
>>> the first two assignments, then both . and __data_start are set to
>>> 0x10 relative to the .data section in the second two assignments.
>>> So I assume the same behavior persists?
>>
>> I just tested moving image_binary_end outside the section definition
>> and it's still emitted properly. I'll try to figure this out and on v3
>> I'll move both image_copy_start/end outside the sections.
>> I also noticed that I forgot to change
>> arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.
>
> Reading the manual again, the symbols will be emitted as relocatable
> entries regardless of their placement after that LD bug you mentioned
> is fixed.
> The only thing that will change when moving it outside the section
> definition is that an absolute value will be set, instead of a
> relative one to the start of the section. But that won't change the
> final binary placement in our case.
As far as I know, the linker is smart enough to understand that *any*
symbol defined in terms of a memory location (e.g. `.` or
`_other_symname`, ...) cannot be absolute when linking a PIE. The [1]
documentation excerpt you cited seems to be saying that the exception to
this rule is when a linker symbol is assigned to a constant value loose
in the SECTIONS block -- and that apparently within the context of a
`.section : {...}`, number literals are treated as offsets from the
section's beginning, not absolute addresses: so we get relative symbols
once again. This seems to be what your [0] documentation excerpt is also
trying to say: "numbers in a section definition are relative to the
section," not "relative symbols must be assigned in a section definition."
>
> I played around a bit and removed the .image_copy_end section from the
> SPL in favor of a symbol.
> Compiling for xilinx_zynqmp_kria_defconfig which builds u-boot & SPL
> (note that I am building natively on an arm64 box)
>
> # Before -- image_copy_end is .efi_runtime_rel and spl still has a
> section for .image_copy_end
> $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
> 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
> 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
> 5: 00000000fffe0dc8 0 SECTION LOCAL DEFAULT 5 .image_copy_end
> 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
>
> # After -- image_copy_end moved outside .efi_runtime_rel and
> .image_copy_end converted to a linker symbol
> $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
> 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
> 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
> 1349: 00000000fffe0dc8 0 NOTYPE GLOBAL DEFAULT 4 __image_copy_end
> 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
>
> Nothing changes in offsets and sizes.
>
> The only thing I won't do right now is move image_copy_start outside
> the text region since that accounts for the CONFIG_SPL_TEXT_BASE and
> CONFIG_SYS_LOAD_ADDR. Keeping the __image_copy_start in there has an
> obvious caveat -- __image_copy_start will end up in .text and
> __image_copy_end in .rodata, but since we always had that it's fine
> for now.
I see what you're saying: the .text address is specified by -Ttext from
Makefile.spl, so we don't know it in the linker script, and we can't
rely on a `__image_copy_start = .;` assignment right before .text
because the linker isn't going to place .text contiguously.
For what it's worth, `__image_copy_start = ADDR(.text);` *does* work
(and produces a relative relocation). It might also be preferable to
call attention to the fact that the .text section's start address is not
determined/known by the linker script.
Up to you though! I'm fine with either approach, I just want to make
sure that they're both considered. :)
Cheers,
Sam
>
> Thanks
> /Ilias
>
>
>
>
>
>
>>
>> Thanks
>> /Ilias
>>
>>
>>>
>>> [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
>>> [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
>>>
>>> Thanks
>>> /Ilias
>>>
>>>
>>>>
>>>> Thanks for the cleanup,
>>>> Sam
>>>>
>>>>> ---
>>>>> arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
>>>>> arch/arm/cpu/u-boot.lds | 10 ++--------
>>>>> arch/arm/lib/sections.c | 2 --
>>>>> arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
>>>>> arch/arm/mach-zynq/u-boot.lds | 9 ++-------
>>>>> 5 files changed, 8 insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
>>>>> index e737de761a9d..340acf5c043b 100644
>>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
>>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
>>>>> @@ -23,7 +23,7 @@ SECTIONS
>>>>> . = ALIGN(8);
>>>>> .text :
>>>>> {
>>>>> - *(.__image_copy_start)
>>>>> + __image_copy_start = .;
>>>>> CPUDIR/start.o (.text*)
>>>>> }
>>>>>
>>>>> @@ -120,13 +120,7 @@ SECTIONS
>>>>> *(.rel*.efi_runtime)
>>>>> *(.rel*.efi_runtime.*)
>>>>> __efi_runtime_rel_stop = .;
>>>>> - }
>>>>> -
>>>>> - . = ALIGN(8);
>>>>> -
>>>>> - .image_copy_end :
>>>>> - {
>>>>> - *(.__image_copy_end)
>>>>> + __image_copy_end = .;
>>>>> }
>>>>>
>>>>> .rela.dyn ALIGN(8) : {
>>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>>>>> index df55bb716e35..7c4f9c2d4dd3 100644
>>>>> --- a/arch/arm/cpu/u-boot.lds
>>>>> +++ b/arch/arm/cpu/u-boot.lds
>>>>> @@ -37,7 +37,7 @@ SECTIONS
>>>>> . = ALIGN(4);
>>>>> .text :
>>>>> {
>>>>> - *(.__image_copy_start)
>>>>> + __image_copy_start = .;
>>>>> *(.vectors)
>>>>> CPUDIR/start.o (.text*)
>>>>> }
>>>>> @@ -151,13 +151,7 @@ SECTIONS
>>>>> *(.rel*.efi_runtime)
>>>>> *(.rel*.efi_runtime.*)
>>>>> __efi_runtime_rel_stop = .;
>>>>> - }
>>>>> -
>>>>> - . = ALIGN(4);
>>>>> -
>>>>> - .image_copy_end :
>>>>> - {
>>>>> - *(.__image_copy_end)
>>>>> + __image_copy_end = .;
>>>>> }
>>>>>
>>>>> .rel.dyn ALIGN(4) : {
>>>>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
>>>>> index a4d4202e99f5..db5463b2bbbc 100644
>>>>> --- a/arch/arm/lib/sections.c
>>>>> +++ b/arch/arm/lib/sections.c
>>>>> @@ -19,8 +19,6 @@
>>>>> * aliasing warnings.
>>>>> */
>>>>>
>>>>> -char __image_copy_start[0] __section(".__image_copy_start");
>>>>> -char __image_copy_end[0] __section(".__image_copy_end");
>>>>> char __secure_start[0] __section(".__secure_start");
>>>>> char __secure_end[0] __section(".__secure_end");
>>>>> char __secure_stack_start[0] __section(".__secure_stack_start");
>>>>> diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
>>>>> index b7887194026e..4b480ebb0c4d 100644
>>>>> --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
>>>>> +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
>>>>> @@ -24,7 +24,7 @@ SECTIONS
>>>>>
>>>>> .text : {
>>>>> . = ALIGN(8);
>>>>> - *(.__image_copy_start)
>>>>> + __image_copy_start = .;
>>>>> CPUDIR/start.o (.text*)
>>>>> *(.text*)
>>>>> }
>>>>> @@ -42,11 +42,7 @@ SECTIONS
>>>>> __u_boot_list : {
>>>>> . = ALIGN(8);
>>>>> KEEP(*(SORT(__u_boot_list*)));
>>>>> - }
>>>>> -
>>>>> - .image_copy_end : {
>>>>> - . = ALIGN(8);
>>>>> - *(.__image_copy_end)
>>>>> + __image_copy_end = .;
>>>>> }
>>>>>
>>>>> .end : {
>>>>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
>>>>> index fcd0f42a7106..553bcf182272 100644
>>>>> --- a/arch/arm/mach-zynq/u-boot.lds
>>>>> +++ b/arch/arm/mach-zynq/u-boot.lds
>>>>> @@ -16,7 +16,7 @@ SECTIONS
>>>>> . = ALIGN(4);
>>>>> .text :
>>>>> {
>>>>> - *(.__image_copy_start)
>>>>> + __image_copy_start = .;
>>>>> *(.vectors)
>>>>> CPUDIR/start.o (.text*)
>>>>> }
>>>>> @@ -57,12 +57,7 @@ SECTIONS
>>>>> *(.rel*.efi_runtime)
>>>>> *(.rel*.efi_runtime.*)
>>>>> __efi_runtime_rel_stop = .;
>>>>> - }
>>>>> -
>>>>> - . = ALIGN(8);
>>>>> - .image_copy_end :
>>>>> - {
>>>>> - *(.__image_copy_end)
>>>>> + __image_copy_end = .;
>>>>> }
>>>>>
>>>>> .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-06 22:19 ` Sam Edwards
@ 2024-03-07 6:50 ` Ilias Apalodimas
2024-03-08 13:22 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-07 6:50 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
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 */
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-06 23:08 ` Sam Edwards
@ 2024-03-07 6:55 ` Ilias Apalodimas
2024-03-07 16:45 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-07 6:55 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Thu, 7 Mar 2024 at 01:08, Sam Edwards <cfsworks@gmail.com> wrote:
>
>
>
> On 3/6/24 06:23, Ilias Apalodimas wrote:
> > On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >>
> >> On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas
> >> <ilias.apalodimas@linaro.org> wrote:
> >>>
> >>> Hi Sam,
> >>>
> >>>
> >>> On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks@gmail.com> wrote:
> >>>>
> >>>> On 3/4/24 02:01, Ilias Apalodimas wrote:
> >>>>> image_copy_start/end are defined as c variables in order to force the compiler
> >>>>> emit relative references. However, defining those within a section definition
> >>>>> will do the same thing.
> >>>>>
> >>>>> So let's remove the special sections from the linker scripts, the
> >>>>> variable definitions from sections.c and define them as a symbols within
> >>>>> a section.
> >>>>>
> >>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
> >>>>
> >>>> Since the __image_copy_* symbols are marking boundaries in the whole
> >>>> image as opposed to a single section, I'd find it clearer if they were
> >>>> loose in the SECTIONS { ... } block, so as not to imply that they are
> >>>> coupled to any particular section. Thoughts?
> >>>
> >>> The reason I included it within a section is my understanding of the
> >>> ld (way older) manual [0].
> >>> Copy-pasting from that:
> >>>
> >>> " Assignment statements may appear:
> >>> - as commands in their own right in an ld script; or
> >>> - as independent statements within a SECTIONS command; or
> >>> - as part of the contents of a section definition in a SECTIONS command.
> >>> The first two cases are equivalent in effect--both define a symbol
> >>> with an absolute address. The last case defines a symbol whose address
> >>> is relative to a particular section."
> >>> So, since we need relocatable entries, I included it within a section.
> >>>
> >>> Looking at the latest ld [1] the wording has changed a bit it says
> >>> "Expressions appearing outside an output section definition treat all
> >>> numbers as absolute addresses" and it also has the following example
> >>> SECTIONS
> >>> {
> >>> . = 0x100;
> >>> __executable_start = 0x100;
> >>> .data :
> >>> {
> >>> . = 0x10;
> >>> __data_start = 0x10;
> >>> *(.data)
> >>> }
> >>> …
> >>> }
> >>> both . and __executable_start are set to the absolute address 0x100 in
> >>> the first two assignments, then both . and __data_start are set to
> >>> 0x10 relative to the .data section in the second two assignments.
> >>> So I assume the same behavior persists?
> >>
> >> I just tested moving image_binary_end outside the section definition
> >> and it's still emitted properly. I'll try to figure this out and on v3
> >> I'll move both image_copy_start/end outside the sections.
> >> I also noticed that I forgot to change
> >> arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.
> >
> > Reading the manual again, the symbols will be emitted as relocatable
> > entries regardless of their placement after that LD bug you mentioned
> > is fixed.
> > The only thing that will change when moving it outside the section
> > definition is that an absolute value will be set, instead of a
> > relative one to the start of the section. But that won't change the
> > final binary placement in our case.
>
> As far as I know, the linker is smart enough to understand that *any*
> symbol defined in terms of a memory location (e.g. `.` or
> `_other_symname`, ...) cannot be absolute when linking a PIE. The [1]
> documentation excerpt you cited seems to be saying that the exception to
> this rule is when a linker symbol is assigned to a constant value loose
> in the SECTIONS block -- and that apparently within the context of a
> `.section : {...}`, number literals are treated as offsets from the
> section's beginning, not absolute addresses: so we get relative symbols
> once again. This seems to be what your [0] documentation excerpt is also
> trying to say: "numbers in a section definition are relative to the
> section," not "relative symbols must be assigned in a section definition."
Exactly. I somehow misread that at first and assumed that 'absolute
address' loose in the SECTIONS block also implied absolute
relocations. But that's not the case so we can move it outside the
section definition
>
> >
> > I played around a bit and removed the .image_copy_end section from the
> > SPL in favor of a symbol.
> > Compiling for xilinx_zynqmp_kria_defconfig which builds u-boot & SPL
> > (note that I am building natively on an arm64 box)
> >
> > # Before -- image_copy_end is .efi_runtime_rel and spl still has a
> > section for .image_copy_end
> > $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
> > 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
> > 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> > $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
> > 5: 00000000fffe0dc8 0 SECTION LOCAL DEFAULT 5 .image_copy_end
> > 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> >
> > # After -- image_copy_end moved outside .efi_runtime_rel and
> > .image_copy_end converted to a linker symbol
> > $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
> > 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
> > 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> > $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
> > 1349: 00000000fffe0dc8 0 NOTYPE GLOBAL DEFAULT 4 __image_copy_end
> > 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> >
> > Nothing changes in offsets and sizes.
> >
> > The only thing I won't do right now is move image_copy_start outside
> > the text region since that accounts for the CONFIG_SPL_TEXT_BASE and
> > CONFIG_SYS_LOAD_ADDR. Keeping the __image_copy_start in there has an
> > obvious caveat -- __image_copy_start will end up in .text and
> > __image_copy_end in .rodata, but since we always had that it's fine
> > for now.
>
> I see what you're saying: the .text address is specified by -Ttext from
> Makefile.spl, so we don't know it in the linker script, and we can't
> rely on a `__image_copy_start = .;` assignment right before .text
> because the linker isn't going to place .text contiguously.
Exactly and apart from that that __image_copy_start will probably end
up @ 0x0 instead of the actual start address
>
> For what it's worth, `__image_copy_start = ADDR(.text);` *does* work
> (and produces a relative relocation). It might also be preferable to
> call attention to the fact that the .text section's start address is not
> determined/known by the linker script.
>
> Up to you though! I'm fine with either approach, I just want to make
> sure that they're both considered. :)
Ah good point, I completely forgot the ADDR directive. I'll switch to
that since I also think having those loose in the SECTIONS block is
far more intuitive
>
> Cheers,
> Sam
>
> >
> > Thanks
> > /Ilias
> >
> >
> >
> >
> >
> >
> >>
> >> Thanks
> >> /Ilias
> >>
> >>
> >>>
> >>> [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> >>> [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
> >>>
> >>> Thanks
> >>> /Ilias
> >>>
> >>>
> >>>>
> >>>> Thanks for the cleanup,
> >>>> Sam
> >>>>
> >>>>> ---
> >>>>> arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
> >>>>> arch/arm/cpu/u-boot.lds | 10 ++--------
> >>>>> arch/arm/lib/sections.c | 2 --
> >>>>> arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
> >>>>> arch/arm/mach-zynq/u-boot.lds | 9 ++-------
> >>>>> 5 files changed, 8 insertions(+), 31 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> >>>>> index e737de761a9d..340acf5c043b 100644
> >>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
> >>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
> >>>>> @@ -23,7 +23,7 @@ SECTIONS
> >>>>> . = ALIGN(8);
> >>>>> .text :
> >>>>> {
> >>>>> - *(.__image_copy_start)
> >>>>> + __image_copy_start = .;
> >>>>> CPUDIR/start.o (.text*)
> >>>>> }
> >>>>>
> >>>>> @@ -120,13 +120,7 @@ SECTIONS
> >>>>> *(.rel*.efi_runtime)
> >>>>> *(.rel*.efi_runtime.*)
> >>>>> __efi_runtime_rel_stop = .;
> >>>>> - }
> >>>>> -
> >>>>> - . = ALIGN(8);
> >>>>> -
> >>>>> - .image_copy_end :
> >>>>> - {
> >>>>> - *(.__image_copy_end)
> >>>>> + __image_copy_end = .;
> >>>>> }
> >>>>>
> >>>>> .rela.dyn ALIGN(8) : {
> >>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> >>>>> index df55bb716e35..7c4f9c2d4dd3 100644
> >>>>> --- a/arch/arm/cpu/u-boot.lds
> >>>>> +++ b/arch/arm/cpu/u-boot.lds
> >>>>> @@ -37,7 +37,7 @@ SECTIONS
> >>>>> . = ALIGN(4);
> >>>>> .text :
> >>>>> {
> >>>>> - *(.__image_copy_start)
> >>>>> + __image_copy_start = .;
> >>>>> *(.vectors)
> >>>>> CPUDIR/start.o (.text*)
> >>>>> }
> >>>>> @@ -151,13 +151,7 @@ SECTIONS
> >>>>> *(.rel*.efi_runtime)
> >>>>> *(.rel*.efi_runtime.*)
> >>>>> __efi_runtime_rel_stop = .;
> >>>>> - }
> >>>>> -
> >>>>> - . = ALIGN(4);
> >>>>> -
> >>>>> - .image_copy_end :
> >>>>> - {
> >>>>> - *(.__image_copy_end)
> >>>>> + __image_copy_end = .;
> >>>>> }
> >>>>>
> >>>>> .rel.dyn ALIGN(4) : {
> >>>>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> >>>>> index a4d4202e99f5..db5463b2bbbc 100644
> >>>>> --- a/arch/arm/lib/sections.c
> >>>>> +++ b/arch/arm/lib/sections.c
> >>>>> @@ -19,8 +19,6 @@
> >>>>> * aliasing warnings.
> >>>>> */
> >>>>>
> >>>>> -char __image_copy_start[0] __section(".__image_copy_start");
> >>>>> -char __image_copy_end[0] __section(".__image_copy_end");
> >>>>> char __secure_start[0] __section(".__secure_start");
> >>>>> char __secure_end[0] __section(".__secure_end");
> >>>>> char __secure_stack_start[0] __section(".__secure_stack_start");
> >>>>> diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> >>>>> index b7887194026e..4b480ebb0c4d 100644
> >>>>> --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> >>>>> +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> >>>>> @@ -24,7 +24,7 @@ SECTIONS
> >>>>>
> >>>>> .text : {
> >>>>> . = ALIGN(8);
> >>>>> - *(.__image_copy_start)
> >>>>> + __image_copy_start = .;
> >>>>> CPUDIR/start.o (.text*)
> >>>>> *(.text*)
> >>>>> }
> >>>>> @@ -42,11 +42,7 @@ SECTIONS
> >>>>> __u_boot_list : {
> >>>>> . = ALIGN(8);
> >>>>> KEEP(*(SORT(__u_boot_list*)));
> >>>>> - }
> >>>>> -
> >>>>> - .image_copy_end : {
> >>>>> - . = ALIGN(8);
> >>>>> - *(.__image_copy_end)
> >>>>> + __image_copy_end = .;
> >>>>> }
> >>>>>
> >>>>> .end : {
> >>>>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> >>>>> index fcd0f42a7106..553bcf182272 100644
> >>>>> --- a/arch/arm/mach-zynq/u-boot.lds
> >>>>> +++ b/arch/arm/mach-zynq/u-boot.lds
> >>>>> @@ -16,7 +16,7 @@ SECTIONS
> >>>>> . = ALIGN(4);
> >>>>> .text :
> >>>>> {
> >>>>> - *(.__image_copy_start)
> >>>>> + __image_copy_start = .;
> >>>>> *(.vectors)
> >>>>> CPUDIR/start.o (.text*)
> >>>>> }
> >>>>> @@ -57,12 +57,7 @@ SECTIONS
> >>>>> *(.rel*.efi_runtime)
> >>>>> *(.rel*.efi_runtime.*)
> >>>>> __efi_runtime_rel_stop = .;
> >>>>> - }
> >>>>> -
> >>>>> - . = ALIGN(8);
> >>>>> - .image_copy_end :
> >>>>> - {
> >>>>> - *(.__image_copy_end)
> >>>>> + __image_copy_end = .;
> >>>>> }
> >>>>>
> >>>>> .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols
2024-03-07 6:55 ` Ilias Apalodimas
@ 2024-03-07 16:45 ` Ilias Apalodimas
0 siblings, 0 replies; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-07 16:45 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Thu, 7 Mar 2024 at 08:55, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 7 Mar 2024 at 01:08, Sam Edwards <cfsworks@gmail.com> wrote:
> >
> >
> >
> > On 3/6/24 06:23, Ilias Apalodimas wrote:
> > > On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > >>
> > >> On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas
> > >> <ilias.apalodimas@linaro.org> wrote:
> > >>>
> > >>> Hi Sam,
> > >>>
> > >>>
> > >>> On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks@gmail.com> wrote:
> > >>>>
> > >>>> On 3/4/24 02:01, Ilias Apalodimas wrote:
> > >>>>> image_copy_start/end are defined as c variables in order to force the compiler
> > >>>>> emit relative references. However, defining those within a section definition
> > >>>>> will do the same thing.
> > >>>>>
> > >>>>> So let's remove the special sections from the linker scripts, the
> > >>>>> variable definitions from sections.c and define them as a symbols within
> > >>>>> a section.
> > >>>>>
> > >>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > >>>> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
> > >>>>
> > >>>> Since the __image_copy_* symbols are marking boundaries in the whole
> > >>>> image as opposed to a single section, I'd find it clearer if they were
> > >>>> loose in the SECTIONS { ... } block, so as not to imply that they are
> > >>>> coupled to any particular section. Thoughts?
> > >>>
> > >>> The reason I included it within a section is my understanding of the
> > >>> ld (way older) manual [0].
> > >>> Copy-pasting from that:
> > >>>
> > >>> " Assignment statements may appear:
> > >>> - as commands in their own right in an ld script; or
> > >>> - as independent statements within a SECTIONS command; or
> > >>> - as part of the contents of a section definition in a SECTIONS command.
> > >>> The first two cases are equivalent in effect--both define a symbol
> > >>> with an absolute address. The last case defines a symbol whose address
> > >>> is relative to a particular section."
> > >>> So, since we need relocatable entries, I included it within a section.
> > >>>
> > >>> Looking at the latest ld [1] the wording has changed a bit it says
> > >>> "Expressions appearing outside an output section definition treat all
> > >>> numbers as absolute addresses" and it also has the following example
> > >>> SECTIONS
> > >>> {
> > >>> . = 0x100;
> > >>> __executable_start = 0x100;
> > >>> .data :
> > >>> {
> > >>> . = 0x10;
> > >>> __data_start = 0x10;
> > >>> *(.data)
> > >>> }
> > >>> …
> > >>> }
> > >>> both . and __executable_start are set to the absolute address 0x100 in
> > >>> the first two assignments, then both . and __data_start are set to
> > >>> 0x10 relative to the .data section in the second two assignments.
> > >>> So I assume the same behavior persists?
> > >>
> > >> I just tested moving image_binary_end outside the section definition
> > >> and it's still emitted properly. I'll try to figure this out and on v3
> > >> I'll move both image_copy_start/end outside the sections.
> > >> I also noticed that I forgot to change
> > >> arch/arm/cpu/armv8/u-boot-spl.lds and get rid of the .image_copy_end.
> > >
> > > Reading the manual again, the symbols will be emitted as relocatable
> > > entries regardless of their placement after that LD bug you mentioned
> > > is fixed.
> > > The only thing that will change when moving it outside the section
> > > definition is that an absolute value will be set, instead of a
> > > relative one to the start of the section. But that won't change the
> > > final binary placement in our case.
> >
> > As far as I know, the linker is smart enough to understand that *any*
> > symbol defined in terms of a memory location (e.g. `.` or
> > `_other_symname`, ...) cannot be absolute when linking a PIE. The [1]
> > documentation excerpt you cited seems to be saying that the exception to
> > this rule is when a linker symbol is assigned to a constant value loose
> > in the SECTIONS block -- and that apparently within the context of a
> > `.section : {...}`, number literals are treated as offsets from the
> > section's beginning, not absolute addresses: so we get relative symbols
> > once again. This seems to be what your [0] documentation excerpt is also
> > trying to say: "numbers in a section definition are relative to the
> > section," not "relative symbols must be assigned in a section definition."
>
>
> Exactly. I somehow misread that at first and assumed that 'absolute
> address' loose in the SECTIONS block also implied absolute
> relocations. But that's not the case so we can move it outside the
> section definition
>
> >
> > >
> > > I played around a bit and removed the .image_copy_end section from the
> > > SPL in favor of a symbol.
> > > Compiling for xilinx_zynqmp_kria_defconfig which builds u-boot & SPL
> > > (note that I am building natively on an arm64 box)
> > >
> > > # Before -- image_copy_end is .efi_runtime_rel and spl still has a
> > > section for .image_copy_end
> > > $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
> > > 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
> > > 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> > > $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
> > > 5: 00000000fffe0dc8 0 SECTION LOCAL DEFAULT 5 .image_copy_end
> > > 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> > >
> > > # After -- image_copy_end moved outside .efi_runtime_rel and
> > > .image_copy_end converted to a linker symbol
> > > $:~/work/u-boot-tpm>readelf -s u-boot | grep image_cop
> > > 9339: 000000000811f550 0 NOTYPE GLOBAL DEFAULT 10 __image_copy_end
> > > 10614: 0000000008000000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> > > $:~/work/u-boot-tpm>readelf -s spl/u-boot-spl | grep image_cop
> > > 1349: 00000000fffe0dc8 0 NOTYPE GLOBAL DEFAULT 4 __image_copy_end
> > > 1705: 00000000fffc0000 0 NOTYPE GLOBAL DEFAULT 1 __image_copy_start
> > >
> > > Nothing changes in offsets and sizes.
> > >
> > > The only thing I won't do right now is move image_copy_start outside
> > > the text region since that accounts for the CONFIG_SPL_TEXT_BASE and
> > > CONFIG_SYS_LOAD_ADDR. Keeping the __image_copy_start in there has an
> > > obvious caveat -- __image_copy_start will end up in .text and
> > > __image_copy_end in .rodata, but since we always had that it's fine
> > > for now.
> >
> > I see what you're saying: the .text address is specified by -Ttext from
> > Makefile.spl, so we don't know it in the linker script, and we can't
> > rely on a `__image_copy_start = .;` assignment right before .text
> > because the linker isn't going to place .text contiguously.
>
> Exactly and apart from that that __image_copy_start will probably end
> up @ 0x0 instead of the actual start address
>
> >
> > For what it's worth, `__image_copy_start = ADDR(.text);` *does* work
> > (and produces a relative relocation). It might also be preferable to
> > call attention to the fact that the .text section's start address is not
> > determined/known by the linker script.
> >
> > Up to you though! I'm fine with either approach, I just want to make
> > sure that they're both considered. :)
>
> Ah good point, I completely forgot the ADDR directive. I'll switch to
> that since I also think having those loose in the SECTIONS block is
> far more intuitive
FWIW, I completely forgot your RFC trying to clean the same mess.
I'll add your Suggested-by on v2
[0] https://lore.kernel.org/u-boot/20230520205547.1009254-11-CFSworks@gmail.com/
Cheers
/Ilias
>
> >
> > Cheers,
> > Sam
> >
> > >
> > > Thanks
> > > /Ilias
> > >
> > >
> > >
> > >
> > >
> > >
> > >>
> > >> Thanks
> > >> /Ilias
> > >>
> > >>
> > >>>
> > >>> [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13
> > >>> [1] https://sourceware.org/binutils/docs/ld/Expression-Section.html
> > >>>
> > >>> Thanks
> > >>> /Ilias
> > >>>
> > >>>
> > >>>>
> > >>>> Thanks for the cleanup,
> > >>>> Sam
> > >>>>
> > >>>>> ---
> > >>>>> arch/arm/cpu/armv8/u-boot.lds | 10 ++--------
> > >>>>> arch/arm/cpu/u-boot.lds | 10 ++--------
> > >>>>> arch/arm/lib/sections.c | 2 --
> > >>>>> arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 8 ++------
> > >>>>> arch/arm/mach-zynq/u-boot.lds | 9 ++-------
> > >>>>> 5 files changed, 8 insertions(+), 31 deletions(-)
> > >>>>>
> > >>>>> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > >>>>> index e737de761a9d..340acf5c043b 100644
> > >>>>> --- a/arch/arm/cpu/armv8/u-boot.lds
> > >>>>> +++ b/arch/arm/cpu/armv8/u-boot.lds
> > >>>>> @@ -23,7 +23,7 @@ SECTIONS
> > >>>>> . = ALIGN(8);
> > >>>>> .text :
> > >>>>> {
> > >>>>> - *(.__image_copy_start)
> > >>>>> + __image_copy_start = .;
> > >>>>> CPUDIR/start.o (.text*)
> > >>>>> }
> > >>>>>
> > >>>>> @@ -120,13 +120,7 @@ SECTIONS
> > >>>>> *(.rel*.efi_runtime)
> > >>>>> *(.rel*.efi_runtime.*)
> > >>>>> __efi_runtime_rel_stop = .;
> > >>>>> - }
> > >>>>> -
> > >>>>> - . = ALIGN(8);
> > >>>>> -
> > >>>>> - .image_copy_end :
> > >>>>> - {
> > >>>>> - *(.__image_copy_end)
> > >>>>> + __image_copy_end = .;
> > >>>>> }
> > >>>>>
> > >>>>> .rela.dyn ALIGN(8) : {
> > >>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > >>>>> index df55bb716e35..7c4f9c2d4dd3 100644
> > >>>>> --- a/arch/arm/cpu/u-boot.lds
> > >>>>> +++ b/arch/arm/cpu/u-boot.lds
> > >>>>> @@ -37,7 +37,7 @@ SECTIONS
> > >>>>> . = ALIGN(4);
> > >>>>> .text :
> > >>>>> {
> > >>>>> - *(.__image_copy_start)
> > >>>>> + __image_copy_start = .;
> > >>>>> *(.vectors)
> > >>>>> CPUDIR/start.o (.text*)
> > >>>>> }
> > >>>>> @@ -151,13 +151,7 @@ SECTIONS
> > >>>>> *(.rel*.efi_runtime)
> > >>>>> *(.rel*.efi_runtime.*)
> > >>>>> __efi_runtime_rel_stop = .;
> > >>>>> - }
> > >>>>> -
> > >>>>> - . = ALIGN(4);
> > >>>>> -
> > >>>>> - .image_copy_end :
> > >>>>> - {
> > >>>>> - *(.__image_copy_end)
> > >>>>> + __image_copy_end = .;
> > >>>>> }
> > >>>>>
> > >>>>> .rel.dyn ALIGN(4) : {
> > >>>>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > >>>>> index a4d4202e99f5..db5463b2bbbc 100644
> > >>>>> --- a/arch/arm/lib/sections.c
> > >>>>> +++ b/arch/arm/lib/sections.c
> > >>>>> @@ -19,8 +19,6 @@
> > >>>>> * aliasing warnings.
> > >>>>> */
> > >>>>>
> > >>>>> -char __image_copy_start[0] __section(".__image_copy_start");
> > >>>>> -char __image_copy_end[0] __section(".__image_copy_end");
> > >>>>> char __secure_start[0] __section(".__secure_start");
> > >>>>> char __secure_end[0] __section(".__secure_end");
> > >>>>> char __secure_stack_start[0] __section(".__secure_stack_start");
> > >>>>> diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > >>>>> index b7887194026e..4b480ebb0c4d 100644
> > >>>>> --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > >>>>> +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
> > >>>>> @@ -24,7 +24,7 @@ SECTIONS
> > >>>>>
> > >>>>> .text : {
> > >>>>> . = ALIGN(8);
> > >>>>> - *(.__image_copy_start)
> > >>>>> + __image_copy_start = .;
> > >>>>> CPUDIR/start.o (.text*)
> > >>>>> *(.text*)
> > >>>>> }
> > >>>>> @@ -42,11 +42,7 @@ SECTIONS
> > >>>>> __u_boot_list : {
> > >>>>> . = ALIGN(8);
> > >>>>> KEEP(*(SORT(__u_boot_list*)));
> > >>>>> - }
> > >>>>> -
> > >>>>> - .image_copy_end : {
> > >>>>> - . = ALIGN(8);
> > >>>>> - *(.__image_copy_end)
> > >>>>> + __image_copy_end = .;
> > >>>>> }
> > >>>>>
> > >>>>> .end : {
> > >>>>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > >>>>> index fcd0f42a7106..553bcf182272 100644
> > >>>>> --- a/arch/arm/mach-zynq/u-boot.lds
> > >>>>> +++ b/arch/arm/mach-zynq/u-boot.lds
> > >>>>> @@ -16,7 +16,7 @@ SECTIONS
> > >>>>> . = ALIGN(4);
> > >>>>> .text :
> > >>>>> {
> > >>>>> - *(.__image_copy_start)
> > >>>>> + __image_copy_start = .;
> > >>>>> *(.vectors)
> > >>>>> CPUDIR/start.o (.text*)
> > >>>>> }
> > >>>>> @@ -57,12 +57,7 @@ SECTIONS
> > >>>>> *(.rel*.efi_runtime)
> > >>>>> *(.rel*.efi_runtime.*)
> > >>>>> __efi_runtime_rel_stop = .;
> > >>>>> - }
> > >>>>> -
> > >>>>> - . = ALIGN(8);
> > >>>>> - .image_copy_end :
> > >>>>> - {
> > >>>>> - *(.__image_copy_end)
> > >>>>> + __image_copy_end = .;
> > >>>>> }
> > >>>>>
> > >>>>> .rel.dyn ALIGN(8) : {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-07 6:50 ` Ilias Apalodimas
@ 2024-03-08 13:22 ` Ilias Apalodimas
2024-03-08 14:14 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-08 13:22 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
Hi Sam,
On Thu, 7 Mar 2024 at 08:50, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
>
>
> On Thu, 7 Mar 2024 at 00:19, Sam Edwards <cfsworks@gmail.com> wrote:
>>
>>
>>
>> On 3/6/24 02:13, Ilias Apalodimas wrote:
>> > Hi Sam,
>> >
>> > Again thank you for the elaborate review. This really helps a lot.
>> >
>> > On Wed, 6 Mar 2024 at 10:14, Sam Edwards <cfsworks@gmail.com> wrote:
>> >>
>> >>
>> >>
>> >> On 3/4/24 02:01, Ilias Apalodimas wrote:
>> >>> __efi_runtime_start/end are defined as c variables for arm7 only in
>> >>> order to force the compiler emit relative references. However, defining
>> >>> those within a section definition will do the same thing. On top of that
>> >>> the v8 linker scripts define it as a symbol.
>> >>>
>> >>> So let's remove the special sections from the linker scripts, the
>> >>> variable definitions from sections.c and define them as a symbols within
>> >>> the correct section.
>> >>>
>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> >> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
>> >>
>> >> Thanks for the cleanup,
>> >> Sam
>> >>
>> >>> ---
>> >>> arch/arm/cpu/u-boot.lds | 12 +++---------
>> >>> arch/arm/lib/sections.c | 2 --
>> >>> arch/arm/mach-zynq/u-boot.lds | 12 +++---------
>> >>> include/asm-generic/sections.h | 1 +
>> >>> 4 files changed, 7 insertions(+), 20 deletions(-)
>> >>>
>> >>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>> >>> index 7c6e7891d360..df55bb716e35 100644
>> >>> --- a/arch/arm/cpu/u-boot.lds
>> >>> +++ b/arch/arm/cpu/u-boot.lds
>> >>> @@ -43,18 +43,12 @@ SECTIONS
>> >>> }
>> >>>
>> >>> /* This needs to come before *(.text*) */
>> >>> - .__efi_runtime_start : {
>> >>> - *(.__efi_runtime_start)
>> >>> - }
>> >>> -
>> >>> - .efi_runtime : {
>> >>> + .efi_runtime ALIGN(4) : {
>> >>
>> >> Do we truly require the ALIGN(4)? If I understand correctly, by default,
>> >> the linker calculates the alignment of an output section as the least
>> >> common multiple of the input sections' alignment requirements -- meaning
>> >> most (perhaps all) of our ALIGN()s today are redundant.
>> >
>> > I don't think we do. But I preserved those for a few reasons.
>> >
>> >> For the time
>> >> being, I'm in favor of merging existing `. = ALIGN(x)` into each
>> >> following section for clarity and to avoid the testing overhead of
>> >> removing them in the same patch as other changes. However, in the near
>> >> future (perhaps even as "near" as v2 of this series?), I'd also like to
>> >> see a patch that eliminates unnecessary ALIGN()s altogether. Introducing
>> >> additional ALIGN()s where we already know they aren't needed may be a
>> >> step away from that goal.
>> >
>> > So as you already mentioned the reason I preserved this is:
>> > - Reduce the testing overhead by preserving the same layout for now
>> > - In the future, I am playing around with the idea of mapping U-Boot
>> > (not SPL but the relocated full U-Boot) in sections with proper memory
>> > permissions (R), RW^X etc). In that case, we will need a 4k section
>> > alignment and we can repurpose the ALIGN(4/8) to
>> > ALIGN(CONSTANT(COMMONPAGESIZE)).
>> >
>> > Thoughts?
>>
>> Ah, right: I had forgotten for a moment that the ultimate goal was to
>> clean up the memory protection bit situation.
>>
>> Okay, as long as that future cleanup will also remove all unnecessary
>> uses of ALIGN() (i.e. any that don't result in an actual change in
>> memory layout) then that sounds great to me.
>
>
> Yep will do
>
>>
>> Reviewed-by: Sam Edwards <CFSworks@gmail.com>
Thanks. However, I noticed the alignment rule was wrong
it needs to be '.efi_runtime : ALIGN(4)' instead of '.efi_runtime ALIGN(4) :'.
The latter will define the start address instead of specifying the alignment.
Due to what you mentioned above this (that we don't really need the
ALIGN(4) the resulting binary is unaffected.
Thanks
/Ilias
>
>
>
>
> Thanks!
>
>>
>> Cheers,
>> Sam
>>
>> >
>> > Thanks
>> > /Ilias
>> >
>> >>
>> >>> + __efi_runtime_start = .;
>> >>> *(.text.efi_runtime*)
>> >>> *(.rodata.efi_runtime*)
>> >>> *(.data.efi_runtime*)
>> >>> - }
>> >>> -
>> >>> - .__efi_runtime_stop : {
>> >>> - *(.__efi_runtime_stop)
>> >>> + __efi_runtime_stop = .;
>> >>> }
>> >>>
>> >>> .text_rest :
>> >>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
>> >>> index 1ee3dd3667ba..a4d4202e99f5 100644
>> >>> --- a/arch/arm/lib/sections.c
>> >>> +++ b/arch/arm/lib/sections.c
>> >>> @@ -25,6 +25,4 @@ char __secure_start[0] __section(".__secure_start");
>> >>> char __secure_end[0] __section(".__secure_end");
>> >>> char __secure_stack_start[0] __section(".__secure_stack_start");
>> >>> char __secure_stack_end[0] __section(".__secure_stack_end");
>> >>> -char __efi_runtime_start[0] __section(".__efi_runtime_start");
>> >>> -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
>> >>> char _end[0] __section(".__end");
>> >>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
>> >>> index 71dea4a1f60a..fcd0f42a7106 100644
>> >>> --- a/arch/arm/mach-zynq/u-boot.lds
>> >>> +++ b/arch/arm/mach-zynq/u-boot.lds
>> >>> @@ -22,18 +22,12 @@ SECTIONS
>> >>> }
>> >>>
>> >>> /* This needs to come before *(.text*) */
>> >>> - .__efi_runtime_start : {
>> >>> - *(.__efi_runtime_start)
>> >>> - }
>> >>> -
>> >>> - .efi_runtime : {
>> >>> + .efi_runtime ALIGN(4) : {
>> >>
>> >> Ditto above
>> >>
>> >>> + __efi_runtime_start = .;
>> >>> *(.text.efi_runtime*)
>> >>> *(.rodata.efi_runtime*)
>> >>> *(.data.efi_runtime*)
>> >>> - }
>> >>> -
>> >>> - .__efi_runtime_stop : {
>> >>> - *(.__efi_runtime_stop)
>> >>> + __efi_runtime_stop = .;
>> >>> }
>> >>>
>> >>> .text_rest :
>> >>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> >>> index 60949200dd93..b6bca53db10d 100644
>> >>> --- a/include/asm-generic/sections.h
>> >>> +++ b/include/asm-generic/sections.h
>> >>> @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
>> >>> extern char __ctors_start[], __ctors_end[];
>> >>>
>> >>> extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
>> >>> +extern char __efi_runtime_start[], __efi_runtime_stop[];
>> >>>
>> >>> /* function descriptor handling (if any). Override
>> >>> * in asm/sections.h */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-08 13:22 ` Ilias Apalodimas
@ 2024-03-08 14:14 ` Ilias Apalodimas
2024-03-08 15:10 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-08 14:14 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Fri, 8 Mar 2024 at 15:22, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sam,
>
> On Thu, 7 Mar 2024 at 08:50, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> >
> >
> > On Thu, 7 Mar 2024 at 00:19, Sam Edwards <cfsworks@gmail.com> wrote:
> >>
> >>
> >>
> >> On 3/6/24 02:13, Ilias Apalodimas wrote:
> >> > Hi Sam,
> >> >
> >> > Again thank you for the elaborate review. This really helps a lot.
> >> >
> >> > On Wed, 6 Mar 2024 at 10:14, Sam Edwards <cfsworks@gmail.com> wrote:
> >> >>
> >> >>
> >> >>
> >> >> On 3/4/24 02:01, Ilias Apalodimas wrote:
> >> >>> __efi_runtime_start/end are defined as c variables for arm7 only in
> >> >>> order to force the compiler emit relative references. However, defining
> >> >>> those within a section definition will do the same thing. On top of that
> >> >>> the v8 linker scripts define it as a symbol.
> >> >>>
> >> >>> So let's remove the special sections from the linker scripts, the
> >> >>> variable definitions from sections.c and define them as a symbols within
> >> >>> the correct section.
> >> >>>
> >> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> >> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
> >> >>
> >> >> Thanks for the cleanup,
> >> >> Sam
> >> >>
> >> >>> ---
> >> >>> arch/arm/cpu/u-boot.lds | 12 +++---------
> >> >>> arch/arm/lib/sections.c | 2 --
> >> >>> arch/arm/mach-zynq/u-boot.lds | 12 +++---------
> >> >>> include/asm-generic/sections.h | 1 +
> >> >>> 4 files changed, 7 insertions(+), 20 deletions(-)
> >> >>>
> >> >>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> >> >>> index 7c6e7891d360..df55bb716e35 100644
> >> >>> --- a/arch/arm/cpu/u-boot.lds
> >> >>> +++ b/arch/arm/cpu/u-boot.lds
> >> >>> @@ -43,18 +43,12 @@ SECTIONS
> >> >>> }
> >> >>>
> >> >>> /* This needs to come before *(.text*) */
> >> >>> - .__efi_runtime_start : {
> >> >>> - *(.__efi_runtime_start)
> >> >>> - }
> >> >>> -
> >> >>> - .efi_runtime : {
> >> >>> + .efi_runtime ALIGN(4) : {
> >> >>
> >> >> Do we truly require the ALIGN(4)? If I understand correctly, by default,
> >> >> the linker calculates the alignment of an output section as the least
> >> >> common multiple of the input sections' alignment requirements -- meaning
> >> >> most (perhaps all) of our ALIGN()s today are redundant.
> >> >
> >> > I don't think we do. But I preserved those for a few reasons.
> >> >
> >> >> For the time
> >> >> being, I'm in favor of merging existing `. = ALIGN(x)` into each
> >> >> following section for clarity and to avoid the testing overhead of
> >> >> removing them in the same patch as other changes. However, in the near
> >> >> future (perhaps even as "near" as v2 of this series?), I'd also like to
> >> >> see a patch that eliminates unnecessary ALIGN()s altogether. Introducing
> >> >> additional ALIGN()s where we already know they aren't needed may be a
> >> >> step away from that goal.
> >> >
> >> > So as you already mentioned the reason I preserved this is:
> >> > - Reduce the testing overhead by preserving the same layout for now
> >> > - In the future, I am playing around with the idea of mapping U-Boot
> >> > (not SPL but the relocated full U-Boot) in sections with proper memory
> >> > permissions (R), RW^X etc). In that case, we will need a 4k section
> >> > alignment and we can repurpose the ALIGN(4/8) to
> >> > ALIGN(CONSTANT(COMMONPAGESIZE)).
> >> >
> >> > Thoughts?
> >>
> >> Ah, right: I had forgotten for a moment that the ultimate goal was to
> >> clean up the memory protection bit situation.
> >>
> >> Okay, as long as that future cleanup will also remove all unnecessary
> >> uses of ALIGN() (i.e. any that don't result in an actual change in
> >> memory layout) then that sounds great to me.
> >
> >
> > Yep will do
> >
> >>
> >> Reviewed-by: Sam Edwards <CFSworks@gmail.com>
>
> Thanks. However, I noticed the alignment rule was wrong
> it needs to be '.efi_runtime : ALIGN(4)' instead of '.efi_runtime ALIGN(4) :'.
> The latter will define the start address instead of specifying the alignment.
> Due to what you mentioned above this (that we don't really need the
> ALIGN(4) the resulting binary is unaffected.
To be exact, it will work because ALIGN returns the current location
counter aligned upward to the specified value.
>
> Thanks
> /Ilias
>
> >
> >
> >
> >
> > Thanks!
> >
> >>
> >> Cheers,
> >> Sam
> >>
> >> >
> >> > Thanks
> >> > /Ilias
> >> >
> >> >>
> >> >>> + __efi_runtime_start = .;
> >> >>> *(.text.efi_runtime*)
> >> >>> *(.rodata.efi_runtime*)
> >> >>> *(.data.efi_runtime*)
> >> >>> - }
> >> >>> -
> >> >>> - .__efi_runtime_stop : {
> >> >>> - *(.__efi_runtime_stop)
> >> >>> + __efi_runtime_stop = .;
> >> >>> }
> >> >>>
> >> >>> .text_rest :
> >> >>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> >> >>> index 1ee3dd3667ba..a4d4202e99f5 100644
> >> >>> --- a/arch/arm/lib/sections.c
> >> >>> +++ b/arch/arm/lib/sections.c
> >> >>> @@ -25,6 +25,4 @@ char __secure_start[0] __section(".__secure_start");
> >> >>> char __secure_end[0] __section(".__secure_end");
> >> >>> char __secure_stack_start[0] __section(".__secure_stack_start");
> >> >>> char __secure_stack_end[0] __section(".__secure_stack_end");
> >> >>> -char __efi_runtime_start[0] __section(".__efi_runtime_start");
> >> >>> -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
> >> >>> char _end[0] __section(".__end");
> >> >>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> >> >>> index 71dea4a1f60a..fcd0f42a7106 100644
> >> >>> --- a/arch/arm/mach-zynq/u-boot.lds
> >> >>> +++ b/arch/arm/mach-zynq/u-boot.lds
> >> >>> @@ -22,18 +22,12 @@ SECTIONS
> >> >>> }
> >> >>>
> >> >>> /* This needs to come before *(.text*) */
> >> >>> - .__efi_runtime_start : {
> >> >>> - *(.__efi_runtime_start)
> >> >>> - }
> >> >>> -
> >> >>> - .efi_runtime : {
> >> >>> + .efi_runtime ALIGN(4) : {
> >> >>
> >> >> Ditto above
> >> >>
> >> >>> + __efi_runtime_start = .;
> >> >>> *(.text.efi_runtime*)
> >> >>> *(.rodata.efi_runtime*)
> >> >>> *(.data.efi_runtime*)
> >> >>> - }
> >> >>> -
> >> >>> - .__efi_runtime_stop : {
> >> >>> - *(.__efi_runtime_stop)
> >> >>> + __efi_runtime_stop = .;
> >> >>> }
> >> >>>
> >> >>> .text_rest :
> >> >>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> >> >>> index 60949200dd93..b6bca53db10d 100644
> >> >>> --- a/include/asm-generic/sections.h
> >> >>> +++ b/include/asm-generic/sections.h
> >> >>> @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
> >> >>> extern char __ctors_start[], __ctors_end[];
> >> >>>
> >> >>> extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
> >> >>> +extern char __efi_runtime_start[], __efi_runtime_stop[];
> >> >>>
> >> >>> /* function descriptor handling (if any). Override
> >> >>> * in asm/sections.h */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] arm: fix __efi_runtime_start/end definitions
2024-03-08 14:14 ` Ilias Apalodimas
@ 2024-03-08 15:10 ` Ilias Apalodimas
0 siblings, 0 replies; 26+ messages in thread
From: Ilias Apalodimas @ 2024-03-08 15:10 UTC (permalink / raw)
To: Sam Edwards
Cc: u-boot, trini, caleb.connolly, sumit.garg, Simon Glass,
Philipp Tomsich, Kever Yang, Michal Simek, Yegor Yefremov,
Heinrich Schuchardt, Shiji Yang, Bin Meng
On Fri, 8 Mar 2024 at 16:14, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, 8 Mar 2024 at 15:22, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Sam,
> >
> > On Thu, 7 Mar 2024 at 08:50, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > >
> > >
> > > On Thu, 7 Mar 2024 at 00:19, Sam Edwards <cfsworks@gmail.com> wrote:
> > >>
> > >>
> > >>
> > >> On 3/6/24 02:13, Ilias Apalodimas wrote:
> > >> > Hi Sam,
> > >> >
> > >> > Again thank you for the elaborate review. This really helps a lot.
> > >> >
> > >> > On Wed, 6 Mar 2024 at 10:14, Sam Edwards <cfsworks@gmail.com> wrote:
> > >> >>
> > >> >>
> > >> >>
> > >> >> On 3/4/24 02:01, Ilias Apalodimas wrote:
> > >> >>> __efi_runtime_start/end are defined as c variables for arm7 only in
> > >> >>> order to force the compiler emit relative references. However, defining
> > >> >>> those within a section definition will do the same thing. On top of that
> > >> >>> the v8 linker scripts define it as a symbol.
> > >> >>>
> > >> >>> So let's remove the special sections from the linker scripts, the
> > >> >>> variable definitions from sections.c and define them as a symbols within
> > >> >>> the correct section.
> > >> >>>
> > >> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > >> >> Tested-by: Sam Edwards <CFSworks@gmail.com> # Binary output identical
> > >> >>
> > >> >> Thanks for the cleanup,
> > >> >> Sam
> > >> >>
> > >> >>> ---
> > >> >>> arch/arm/cpu/u-boot.lds | 12 +++---------
> > >> >>> arch/arm/lib/sections.c | 2 --
> > >> >>> arch/arm/mach-zynq/u-boot.lds | 12 +++---------
> > >> >>> include/asm-generic/sections.h | 1 +
> > >> >>> 4 files changed, 7 insertions(+), 20 deletions(-)
> > >> >>>
> > >> >>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > >> >>> index 7c6e7891d360..df55bb716e35 100644
> > >> >>> --- a/arch/arm/cpu/u-boot.lds
> > >> >>> +++ b/arch/arm/cpu/u-boot.lds
> > >> >>> @@ -43,18 +43,12 @@ SECTIONS
> > >> >>> }
> > >> >>>
> > >> >>> /* This needs to come before *(.text*) */
> > >> >>> - .__efi_runtime_start : {
> > >> >>> - *(.__efi_runtime_start)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .efi_runtime : {
> > >> >>> + .efi_runtime ALIGN(4) : {
> > >> >>
> > >> >> Do we truly require the ALIGN(4)? If I understand correctly, by default,
> > >> >> the linker calculates the alignment of an output section as the least
> > >> >> common multiple of the input sections' alignment requirements -- meaning
> > >> >> most (perhaps all) of our ALIGN()s today are redundant.
> > >> >
> > >> > I don't think we do. But I preserved those for a few reasons.
> > >> >
> > >> >> For the time
> > >> >> being, I'm in favor of merging existing `. = ALIGN(x)` into each
> > >> >> following section for clarity and to avoid the testing overhead of
> > >> >> removing them in the same patch as other changes. However, in the near
> > >> >> future (perhaps even as "near" as v2 of this series?), I'd also like to
> > >> >> see a patch that eliminates unnecessary ALIGN()s altogether. Introducing
> > >> >> additional ALIGN()s where we already know they aren't needed may be a
> > >> >> step away from that goal.
> > >> >
> > >> > So as you already mentioned the reason I preserved this is:
> > >> > - Reduce the testing overhead by preserving the same layout for now
> > >> > - In the future, I am playing around with the idea of mapping U-Boot
> > >> > (not SPL but the relocated full U-Boot) in sections with proper memory
> > >> > permissions (R), RW^X etc). In that case, we will need a 4k section
> > >> > alignment and we can repurpose the ALIGN(4/8) to
> > >> > ALIGN(CONSTANT(COMMONPAGESIZE)).
> > >> >
> > >> > Thoughts?
> > >>
> > >> Ah, right: I had forgotten for a moment that the ultimate goal was to
> > >> clean up the memory protection bit situation.
> > >>
> > >> Okay, as long as that future cleanup will also remove all unnecessary
> > >> uses of ALIGN() (i.e. any that don't result in an actual change in
> > >> memory layout) then that sounds great to me.
> > >
> > >
> > > Yep will do
> > >
> > >>
> > >> Reviewed-by: Sam Edwards <CFSworks@gmail.com>
> >
> > Thanks. However, I noticed the alignment rule was wrong
> > it needs to be '.efi_runtime : ALIGN(4)' instead of '.efi_runtime ALIGN(4) :'.
> > The latter will define the start address instead of specifying the alignment.
> > Due to what you mentioned above this (that we don't really need the
> > ALIGN(4) the resulting binary is unaffected.
>
> To be exact, it will work because ALIGN returns the current location
> counter aligned upward to the specified value.
But thinking about it a bit more, I'll just get rid of the redundant
ALIGN as you suggested. I don't think it makes sense to keep or force
the output section alignment just because in the future we want to fix
page permissions.
We can always re-introduce it when stricter (4k) alignment is required for that
Cheers
/Ilias
>
>
> >
> > Thanks
> > /Ilias
> >
> > >
> > >
> > >
> > >
> > > Thanks!
> > >
> > >>
> > >> Cheers,
> > >> Sam
> > >>
> > >> >
> > >> > Thanks
> > >> > /Ilias
> > >> >
> > >> >>
> > >> >>> + __efi_runtime_start = .;
> > >> >>> *(.text.efi_runtime*)
> > >> >>> *(.rodata.efi_runtime*)
> > >> >>> *(.data.efi_runtime*)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .__efi_runtime_stop : {
> > >> >>> - *(.__efi_runtime_stop)
> > >> >>> + __efi_runtime_stop = .;
> > >> >>> }
> > >> >>>
> > >> >>> .text_rest :
> > >> >>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > >> >>> index 1ee3dd3667ba..a4d4202e99f5 100644
> > >> >>> --- a/arch/arm/lib/sections.c
> > >> >>> +++ b/arch/arm/lib/sections.c
> > >> >>> @@ -25,6 +25,4 @@ char __secure_start[0] __section(".__secure_start");
> > >> >>> char __secure_end[0] __section(".__secure_end");
> > >> >>> char __secure_stack_start[0] __section(".__secure_stack_start");
> > >> >>> char __secure_stack_end[0] __section(".__secure_stack_end");
> > >> >>> -char __efi_runtime_start[0] __section(".__efi_runtime_start");
> > >> >>> -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
> > >> >>> char _end[0] __section(".__end");
> > >> >>> diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
> > >> >>> index 71dea4a1f60a..fcd0f42a7106 100644
> > >> >>> --- a/arch/arm/mach-zynq/u-boot.lds
> > >> >>> +++ b/arch/arm/mach-zynq/u-boot.lds
> > >> >>> @@ -22,18 +22,12 @@ SECTIONS
> > >> >>> }
> > >> >>>
> > >> >>> /* This needs to come before *(.text*) */
> > >> >>> - .__efi_runtime_start : {
> > >> >>> - *(.__efi_runtime_start)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .efi_runtime : {
> > >> >>> + .efi_runtime ALIGN(4) : {
> > >> >>
> > >> >> Ditto above
> > >> >>
> > >> >>> + __efi_runtime_start = .;
> > >> >>> *(.text.efi_runtime*)
> > >> >>> *(.rodata.efi_runtime*)
> > >> >>> *(.data.efi_runtime*)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .__efi_runtime_stop : {
> > >> >>> - *(.__efi_runtime_stop)
> > >> >>> + __efi_runtime_stop = .;
> > >> >>> }
> > >> >>>
> > >> >>> .text_rest :
> > >> >>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > >> >>> index 60949200dd93..b6bca53db10d 100644
> > >> >>> --- a/include/asm-generic/sections.h
> > >> >>> +++ b/include/asm-generic/sections.h
> > >> >>> @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
> > >> >>> extern char __ctors_start[], __ctors_end[];
> > >> >>>
> > >> >>> extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
> > >> >>> +extern char __efi_runtime_start[], __efi_runtime_stop[];
> > >> >>>
> > >> >>> /* function descriptor handling (if any). Override
> > >> >>> * in asm/sections.h */
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2024-03-08 15:11 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-04 9:01 [PATCH 0/6] Clean up arm linker scripts Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 1/6] arm: baltos: remove custom linker script Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end Ilias Apalodimas
2024-03-06 7:32 ` Sam Edwards
2024-03-06 9:08 ` Ilias Apalodimas
2024-03-06 10:11 ` Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 3/6] arm: fix __efi_runtime_rel_start/end definitions Ilias Apalodimas
2024-03-06 7:35 ` Sam Edwards
2024-03-04 9:01 ` [PATCH 4/6] arm: clean up v7 and v8 linker scripts for __rel_dyn_start/end Ilias Apalodimas
2024-03-06 7:35 ` Sam Edwards
2024-03-04 9:01 ` [PATCH 5/6] arm: fix __efi_runtime_start/end definitions Ilias Apalodimas
2024-03-06 8:14 ` Sam Edwards
2024-03-06 9:13 ` Ilias Apalodimas
2024-03-06 22:19 ` Sam Edwards
2024-03-07 6:50 ` Ilias Apalodimas
2024-03-08 13:22 ` Ilias Apalodimas
2024-03-08 14:14 ` Ilias Apalodimas
2024-03-08 15:10 ` Ilias Apalodimas
2024-03-04 9:01 ` [PATCH 6/6] arm: move image_copy_start/end to linker symbols Ilias Apalodimas
2024-03-06 8:22 ` Sam Edwards
2024-03-06 9:35 ` Ilias Apalodimas
2024-03-06 10:37 ` Ilias Apalodimas
2024-03-06 13:23 ` Ilias Apalodimas
2024-03-06 23:08 ` Sam Edwards
2024-03-07 6:55 ` Ilias Apalodimas
2024-03-07 16:45 ` Ilias Apalodimas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.