Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PrePi: use standard PeCoff routines for self-relocation

["Laszlo Ersek" <lersek@redhat.com>]

On 06/08/20 19:34, Ard Biesheuvel wrote:
> Instead of having a GCC specific routine to perform self-relocation
> based on ELF metadata, use the PE/COFF metadata and the existing
> PeCoff library routines. This reduces the amount of bespoke assembler
> code that is a burden to maintain, and is not portable across the set
> of toolchains we support.
> 
> This does require some special care, as we have no control over how
> the C code references global symbols, so we need to emit these
> references from the calling assembler code. Otherwise, they may be
> emitted as absolute references, in which case they need to be fixed
> up themselves, leading to a circular dependency.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                    | 10 ++--
>  ArmVirtPkg/ArmVirtXen.dsc                           | 10 ++--
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf |  4 +-
>  ArmVirtPkg/PrePi/PrePi.c                            | 21 +++++++++
>  ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S         | 49 +++++---------------
>  ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S             | 47 +++++--------------
>  6 files changed, 54 insertions(+), 87 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 2a6fd6bc06be..9449a01d6e40 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -83,14 +83,12 @@ [LibraryClasses.common.DXE_DRIVER]
>  [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>  
> -[BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
> +[BuildOptions]
>    #
> -  # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
> -  # on emitting GOT based symbol references when running in shared mode, unless
> -  # we override visibility to 'hidden' in all modules that make up the PrePi
> -  # build.
> +  # We need to avoid jump tables in SEC modules, so that the PE/COFF
> +  # self-relocation code itself is guaranteed to be position independent.
>    #
> -  GCC:*_CLANG38_*_CC_FLAGS = -include $(WORKSPACE)/ArmVirtPkg/Include/Platform/Hidden.h
> +  GCC:*_*_*_CC_FLAGS = -fno-jump-tables
>  
>  ################################################################################
>  #
> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> index 8a489b253684..278f5d3828ce 100644
> --- a/ArmVirtPkg/ArmVirtXen.dsc
> +++ b/ArmVirtPkg/ArmVirtXen.dsc
> @@ -52,14 +52,12 @@ [LibraryClasses]
>  [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>  
> -[BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
> +[BuildOptions]
>    #
> -  # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
> -  # on emitting GOT based symbol references when running in shared mode, unless
> -  # we override visibility to 'hidden' in all modules that make up the PrePi
> -  # build.
> +  # We need to avoid jump tables in SEC modules, so that the PE/COFF
> +  # self-relocation code itself is guaranteed to be position independent.
>    #
> -  GCC:*_CLANG38_*_CC_FLAGS = -include $(WORKSPACE)/ArmVirtPkg/Include/Platform/Hidden.h
> +  GCC:*_*_*_CC_FLAGS = -fno-jump-tables
>  
>  ################################################################################
>  #
> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> index 9e58e56fce09..7edf5018089d 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -46,6 +46,7 @@ [LibraryClasses]
>    SerialPortLib
>    ExtractGuidedSectionLib
>    LzmaDecompressLib
> +  PeCoffLib
>    PrePiLib
>    MemoryAllocationLib
>    HobLib
> @@ -95,6 +96,3 @@ [Pcd]
>    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
>    gArmTokenSpaceGuid.PcdFdBaseAddress
>    gArmTokenSpaceGuid.PcdFvBaseAddress
> -
> -[BuildOptions]
> -  GCC:*_*_*_DLINK_FLAGS = -Wl,-Bsymbolic,-pie,-T,$(MODULE_DIR)/Scripts/PrePi-PIE.lds
> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> index 72e35028c5bb..0cb064419759 100755
> --- a/ArmVirtPkg/PrePi/PrePi.c
> +++ b/ArmVirtPkg/PrePi/PrePi.c
> @@ -9,6 +9,7 @@
>  #include <PiPei.h>
>  #include <Pi/PiBootMode.h>
>  
> +#include <Library/PeCoffLib.h>
>  #include <Library/PrePiLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/PrePiHobListPointerLib.h>
> @@ -128,3 +129,23 @@ CEntryPoint (
>    // DXE Core should always load and never return
>    ASSERT (FALSE);
>  }
> +
> +VOID
> +RelocatePeCoffImage (
> +  IN  UINTN                     ImageBase,
> +  IN  PE_COFF_LOADER_READ_FILE  ImageRead
> +  )
> +{
> +  PE_COFF_LOADER_IMAGE_CONTEXT  ImageContext;
> +
> +  ZeroMem (&ImageContext, sizeof ImageContext);
> +
> +  ImageContext.Handle       = (EFI_HANDLE)ImageBase;
> +  ImageContext.ImageRead    = ImageRead;
> +  PeCoffLoaderGetImageInfo (&ImageContext);
> +
> +  if (ImageContext.ImageAddress != ImageBase) {
> +    ImageContext.ImageAddress = ImageBase;
> +    PeCoffLoaderRelocateImage (&ImageContext);
> +  }
> +}
> diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> index 34d6abecb0ac..7c5592b11a46 100644
> --- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> @@ -9,40 +9,6 @@
>  #include <AsmMacroIoLibV8.h>
>  
>  ASM_FUNC(_ModuleEntryPoint)
> -  //
> -  // We are built as a ET_DYN PIE executable, so we need to process all
> -  // relative relocations regardless of whether or not we are executing from
> -  // the same offset we were linked at. This is only possible if we are
> -  // running from RAM.
> -  //
> -  adr   x8, __reloc_base
> -  adr   x9, __reloc_start
> -  adr   x10, __reloc_end
> -
> -.Lreloc_loop:
> -  cmp   x9, x10
> -  bhs   .Lreloc_done
> -
> -  //
> -  // AArch64 uses the ELF64 RELA format, which means each entry in the
> -  // relocation table consists of
> -  //
> -  //   UINT64 offset          : the relative offset of the value that needs to
> -  //                            be relocated
> -  //   UINT64 info            : relocation type and symbol index (the latter is
> -  //                            not used for R_AARCH64_RELATIVE relocations)
> -  //   UINT64 addend          : value to be added to the value being relocated
> -  //
> -  ldp   x11, x12, [x9], #24   // read offset into x11 and info into x12
> -  cmp   x12, #0x403           // check info == R_AARCH64_RELATIVE?
> -  bne   .Lreloc_loop          // not a relative relocation? then skip
> -
> -  ldr   x12, [x9, #-8]        // read addend into x12
> -  add   x12, x12, x8          // add reloc base to addend to get relocated value
> -  str   x12, [x11, x8]        // write relocated value at offset
> -  b     .Lreloc_loop
> -.Lreloc_done:
> -
>    bl    ASM_PFX(DiscoverDramFromDt)
>  
>    // Get ID of this CPU in Multicore system
> @@ -170,15 +136,24 @@ ASM_PFX(DiscoverDramFromDt):
>    str   x1, [x8]
>    str   x7, [x9]
>  
> +  //
> +  // The runtime address may be different from the link time address so fix
> +  // up the PE/COFF relocations. Since we are calling a C function, use the
> +  // window at the beginning of the FD image as a temp stack.
> +  //
> +  adr   x0, ElfImageBase
> +  adr   x1, PeCoffLoaderImageReadFromMemory
> +  mov   sp, x7
> +  bl    RelocatePeCoffImage
> +
>    //
>    // Discover the memory size and offset from the DTB, and record in the
>    // respective PCDs. This will also return false if a corrupt DTB is
> -  // encountered. Since we are calling a C function, use the window at the
> -  // beginning of the FD image as a temp stack.
> +  // encountered.
>    //
> +  mov   x0, x28
>    adr   x1, PcdGet64 (PcdSystemMemoryBase)
>    adr   x2, PcdGet64 (PcdSystemMemorySize)
> -  mov   sp, x7
>    bl    FindMemnode
>    cbz   x0, .Lout
>  
> diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> index 72d756fdb571..bf39de86e7d0 100644
> --- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> @@ -9,38 +9,6 @@
>  #include <AsmMacroIoLib.h>
>  
>  ASM_FUNC(_ModuleEntryPoint)
> -  //
> -  // We are built as a ET_DYN PIE executable, so we need to process all
> -  // relative relocations if we are executing from a different offset than we
> -  // were linked at. This is only possible if we are running from RAM.
> -  //
> -  ADRL  (r4, __reloc_base)
> -  ADRL  (r5, __reloc_start)
> -  ADRL  (r6, __reloc_end)
> -
> -.Lreloc_loop:
> -  cmp   r5, r6
> -  bhs   .Lreloc_done
> -
> -  //
> -  // AArch32 uses the ELF32 REL format, which means each entry in the
> -  // relocation table consists of
> -  //
> -  //   UINT32 offset          : the relative offset of the value that needs to
> -  //                            be relocated
> -  //   UINT32 info            : relocation type and symbol index (the latter is
> -  //                            not used for R_ARM_RELATIVE relocations)
> -  //
> -  ldrd  r8, r9, [r5], #8      // read offset into r8 and info into r9
> -  cmp   r9, #23               // check info == R_ARM_RELATIVE?
> -  bne   .Lreloc_loop          // not a relative relocation? then skip
> -
> -  ldr   r9, [r8, r4]          // read addend into r9
> -  add   r9, r9, r1            // add image base to addend to get relocated value
> -  str   r9, [r8, r4]          // write relocated value at offset
> -  b     .Lreloc_loop
> -.Lreloc_done:
> -
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>  
> @@ -172,15 +140,24 @@ ASM_PFX(ArmPlatformPeiBootAction):
>    str   r1, [r8]
>    str   r5, [r7]
>  
> +  //
> +  // The runtime address may be different from the link time address so fix
> +  // up the PE/COFF relocations. Since we are calling a C function, use the
> +  // window at the beginning of the FD image as a temp stack.
> +  //
> +  ADRL  (r0, ElfImageBase)
> +  ADRL  (r1, PeCoffLoaderImageReadFromMemory)
> +  mov   sp, r5
> +  bl    RelocatePeCoffImage
> +
>    //
>    // Discover the memory size and offset from the DTB, and record in the
>    // respective PCDs. This will also return false if a corrupt DTB is
> -  // encountered. Since we are calling a C function, use the window at the
> -  // beginning of the FD image as a temp stack.
> +  // encountered.
>    //
> +  mov   r0, r10
>    ADRL  (r1, PcdGet64 (PcdSystemMemoryBase))
>    ADRL  (r2, PcdGet64 (PcdSystemMemorySize))
> -  mov   sp, r5
>    bl    FindMemnode
>    teq   r0, #0
>    beq   .Lout
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>