[PATCH 6/6] arm: move image_copy_start/end to linker symbols
Ilias Apalodimas
ilias.apalodimas at linaro.org
Thu Mar 7 17:45:05 CET 2024
On Thu, 7 Mar 2024 at 08:55, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Thu, 7 Mar 2024 at 01:08, Sam Edwards <cfsworks at gmail.com> wrote:
> >
> >
> >
> > On 3/6/24 06:23, Ilias Apalodimas wrote:
> > > 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.
> >
> > 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) : {
More information about the U-Boot
mailing list