Re: [PATCH v2 0/3] ArmVirtPkg: use PE/COFF metadata for self relocation

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 6/11/20 7:50 PM, Laszlo Ersek wrote:
> On 06/11/20 14:52, Ard Biesheuvel wrote:
>> As suggested by Jiewen in response to Ilias RFC [0], it is better to use
>> the PE/COFF metadata for self-relocating executables than to rely on ELF
>> metadata, given how the latter is only available when using ELF based
>> toolchains. Also, we have had some maintenance issues with this code in
>> the past, as PIE linking of non-position independent objects is not a well
>> tested code path in toolchains in general.
>>
>> So implement this for the self-relocating PrePi in ArmVirtPkg first.
>>
>> First, we need to ensure that the module in question is emitted with its
>> PE/COFF relocation metadata preserved, by creating a special FDF rule.
>>
>> We also need to provide a way for the code to refer to the start of the
>> image directly, by adding it to the linker script.
>>
>> Then, it is simply a matter of swapping out the two assembly routines,
>> and adding the C code that serves the same purpose but based on PE/COFF
>> base relocations.
>>
>> Note that PE/COFF relocations are considerably more compact than ELF RELA
>> relocations, so this does not impact the memory footprint of the resulting
>> image adversely.
>>
>> [0] https://edk2.groups.io/g/devel/message/60835
>>
>> Changes since v1:
>> - Drop change to linker script, and instead, use the existing FV parsing code
>>    (which is already incorporated into PrePi to load other modules), to find
>>    the start address of the image before relocation. This way, we can support
>>    TE images as well as PE32 images naturally, and not rely on GCC/binutils
>>    specific artifacts that make porting to a native PE/COFF toolchain more
>>    difficult
>> - Switch to TE format in the SELF_RELOC FDF rule - this is not terribly
>>    likely to matter in practice, but since PrePi is the only module that
>>    is incorporated in uncompressed form, and given that we used TE format
>>    before these changes, it is a more appropriate default.
> 
> Right, I noticed that when I compared the new rule in v1 against the
> pre-existent SEC rule. I'm happy to see my feedback tags carried forward.
> 
> Thanks
> Laszlo
> 

Merged as #692

Thanks all.

>> - Add acks from Jiewen, Laszlo and Sami. Note that I have dropped the
>>    Tested-bys - apologies for wasting anyone's time, but they could not
>>    be carried over due to the changes.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>>
>> Ard Biesheuvel (3):
>>    ArmVirtPkg: add FDF rule for self-relocating PrePi
>>    ArmVirtPkg/PrePi: use standard PeCoff routines for self-relocation
>>    ArmVirtPkg: remove unused files
>>
>>   ArmVirtPkg/ArmVirtQemuKernel.dsc                    | 10 ++--
>>   ArmVirtPkg/ArmVirtXen.dsc                           | 10 ++--
>>   ArmVirtPkg/ArmVirtQemuKernel.fdf                    |  2 +-
>>   ArmVirtPkg/ArmVirtXen.fdf                           |  2 +-
>>   ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf |  4 +-
>>   ArmVirtPkg/Include/Platform/Hidden.h                | 22 ---------
>>   ArmVirtPkg/PrePi/PrePi.c                            | 35 ++++++++++++++
>>   ArmVirtPkg/ArmVirtRules.fdf.inc                     |  5 ++
>>   ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S         | 49 +++++---------------
>>   ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S             | 47 +++++--------------
>>   ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds              | 41 ----------------
>>   11 files changed, 75 insertions(+), 152 deletions(-)
>>   delete mode 100644 ArmVirtPkg/Include/Platform/Hidden.h
>>   delete mode 100644 ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds
>>
>