Re: [PATCH 6/6] arm: move image_copy_start/end to linker symbols

[Sam Edwards <cfsworks@gmail.com>]

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) : {