[PATCH 6/6] arm: move image_copy_start/end to linker symbols
Sam Edwards
cfsworks at gmail.com
Thu Mar 7 00:08:03 CET 2024
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."
>
> 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) : {
More information about the U-Boot
mailing list