[PATCH 4/4] efi_loader: use include in ARM EFI linker script

[Heinrich Schuchardt]

On 15.01.25 10:34, Ilias Apalodimas wrote:
> On Wed, 15 Jan 2025 at 10:53, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> On 15.01.25 09:06, Ilias Apalodimas wrote:
>>> Hi Heinrich
>>>
>>>
>>> On Tue, 14 Jan 2025 at 12:30, Heinrich Schuchardt
>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>
>>>> Use the same include as arm64 for the linker script.
>>>>
>>>> Adjust the 32-bit ARM PE-COFF header accordingly and harmonize it with the
>>>> 64-bit ARM header.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>> ---
>>>>    arch/arm/lib/crt0_arm_efi.S  | 37 ++++++++++---------
>>>>    arch/arm/lib/elf_arm_efi.lds | 71 +-----------------------------------
>>>>    2 files changed, 20 insertions(+), 88 deletions(-)
>>>>
>>>> diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S
>>>> index b5dfd4e3819..590fcf515da 100644
>>>> --- a/arch/arm/lib/crt0_arm_efi.S
>>>> +++ b/arch/arm/lib/crt0_arm_efi.S
>>>> @@ -14,11 +14,11 @@
>>>>           /*
>>>>            * Magic "MZ" signature for PE/COFF
>>>>            */
>>>> -       .globl  image_base
>>>> -image_base:
>>>> +       .globl  ImageBase
>>>> +ImageBase:
>>>>           .short  IMAGE_DOS_SIGNATURE             /* 'MZ' */
>>>>           .skip   58                              /* 'MZ' + pad + offset == 64 */
>>>> -       .long   pe_header - image_base          /* Offset to the PE header */
>>>> +       .long   pe_header - ImageBase           /* Offset to the PE header */
>>>>    pe_header:
>>>>           .long   IMAGE_NT_SIGNATURE              /* 'PE' */
>>>>    coff_header:
>>>> @@ -38,16 +38,16 @@ optional_header:
>>>>           .short  IMAGE_NT_OPTIONAL_HDR32_MAGIC   /* PE32 format */
>>>>           .byte   0x02                            /* MajorLinkerVersion */
>>>>           .byte   0x14                            /* MinorLinkerVersion */
>>>> -       .long   _edata - _start                 /* SizeOfCode */
>>>> +       .long   _etext - _start                 /* SizeOfCode */
>>>
>>> Was that an error all along? Or the boundaries changed by using the
>>> include file?
>>
>> According to https://learn.microsoft.com/en-us/windows/win32/debug/pe-format
>>
>> SizeOfCode:
>> "The size of the code (text) section, or the sum of all code sections if
>> there are multiple sections."
>>
>> We only have one code section.
>>
>> Looking at EDK2's Shell.efi SizeOfCode equals the VirtualSize of the
>> .text section. It does not include the .data section.
>>
> 
> Hmm that's interesting. gnu-efi includes .data AFAICT.
> Looking at the pe/coff spec there's SizeOfCode and
> SizeOfInitializedData (for .data) and SizeOfUninitializedData (for
> .bss).
> But in both gnu-efi and U-Boot we define those two as 0 though. So I
> think we should either keep SizeOfData as is, or define it as you
> propose and also define SizeOfInitializedData and
> SizeOfUninitializedData correctly -- and I don't know if the latter
> will have any implication on loaders.

Arm64 and RISC-V are already using SizeOfCode = _etext - _start. This 
patch only followed up on 32-bit ARM.

We should follow the example of EDK II and fill SizeOfInitializedData.

We are putting the _bss sections into .data. So SizeOfUninitializedData 
should be kept at 0.

Best regards

Heinrich

> 
> Cheers
> /Ilias
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>           .long   0                               /* SizeOfInitializedData */
>>>>           .long   0                               /* SizeOfUninitializedData */
>>>> -       .long   _start - image_base             /* AddressOfEntryPoint */
>>>> -       .long   _start - image_base             /* BaseOfCode */
>>>> +       .long   _start - ImageBase              /* AddressOfEntryPoint */
>>>> +       .long   _start - ImageBase              /* BaseOfCode */
>>>>           .long   0                               /* BaseOfData */
>>> [...]
>>>
>>> Other than that it looks ok
>>>
>>> Thanks for cleaning this up!
>>> /Ilias
>>