[RFC PATCH 0/6] Clean up arm linker scripts

Fri Mar 1 14:14:01 CET 2024

Thanks Sam

On Thu, 29 Feb 2024 at 23:07, Sam Edwards <cfsworks at gmail.com> wrote:
>
>
>
> On 2/28/24 04:15, Ilias Apalodimas wrote:
> > On Wed, 28 Feb 2024 at 13:11, Peter Robinson <pbrobinson at gmail.com> wrote:
> >>
> >> On Wed, 28 Feb 2024 at 10:58, Ilias Apalodimas
> >> <ilias.apalodimas at linaro.org> wrote:
> >>>
> >>> 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].
> >>> This has led to confusion over the years, ending up with mixed section
> >>> definitions. Some sections being 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.
> >>>
> >>> I am not sure if this used to be a compiler bug, but 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
> >>
> >> Should we be setting/upping the minimum version of the linker version
> >> as part of this?
> >
> > The patch that added those as C-defined variables, was in 2013. So any
> > linker that's not ancient history should emit those correctly.
> >
>
> GNU ld fixed the problem on 2016-02-23, with this entry in the bfd
> changelog:
>
>         * elflink.c (bfd_elf_record_link_assignment): Check for shared
>         library, instead of PIC, and don't check PDE when making linker
>         assigned symbol dynamic.
>
> It looks like the change was first included in binutils 2.27, released
> 2016-08-03. I don't know where the minimum linker version requirement is
> memorialized but there's a good chance that U-Boot already requires a
> version more recent than that. (Someone who knows where that is will
> have to check.)

Ok, this is useful. Thanks for digging it up. I'll update the commit
messages on the next version.

>
> In either case, I strongly agree: sections.c is an unnecessary
> workaround with any reasonably recent linker version, it's
> fickle/incompatible with non-GNU linkers such as LLD, and the marker
> symbols belong in the linker script. This patchset is a big step in the
> right direction!

Yes and the only reason I haven't removed secure_start/end etc, is
that I don't have a platform to test.
So I'd rather land this first and then clean up the remaining 5
entries of sections.c (and the file itself)

Cheers
/Ilias
>
> >
> >>
> >>> 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")
> >>>
> >>> 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/qualcomm/dragonboard820c/u-boot.lds |  41 ++-----
> >>>   board/vscom/baltos/u-boot.lds             | 128 ----------------------
> >>>   include/asm-generic/sections.h            |   3 +
> >>>   lib/efi_loader/efi_runtime.c              |   1 +
> >>>   10 files changed, 58 insertions(+), 352 deletions(-)
> >>>   delete mode 100644 board/vscom/baltos/u-boot.lds
> >>>
> >>> --
> >>> 2.37.2
> >>>