[PATCH 6/6] arm: move image_copy_start/end to linker symbols
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Mar 6 14:23:28 CET 2024
On Wed, 6 Mar 2024 at 12:37, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, 6 Mar 2024 at 11:35, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Sam,
> >
> >
> > On Wed, 6 Mar 2024 at 10:22, Sam Edwards <cfsworks at 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 at linaro.org>
> > > Tested-by: Sam Edwards <CFSworks at 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) : {
More information about the U-Boot
mailing list