[PATCH 0/4] ArmVirtPkg: use PE/COFF metadata for self relocation

[PATCH 0/4] ArmVirtPkg: use PE/COFF metadata for self relocation

[Ard Biesheuvel]

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

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
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>

Ard Biesheuvel (4):
  ArmVirtPkg: add FDF rule for self-relocating PrePi
  BaseTools/Scripts/GccBase.lds: export image base symbol
  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                            | 21 +++++++++
 ArmVirtPkg/ArmVirtRules.fdf.inc                     |  5 ++
 ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S         | 49 +++++---------------
 ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S             | 47 +++++--------------
 ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds              | 41 ----------------
 BaseTools/Scripts/GccBase.lds                       |  2 +
 12 files changed, 63 insertions(+), 152 deletions(-)
 delete mode 100644 ArmVirtPkg/Include/Platform/Hidden.h
 delete mode 100644 ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds

-- 
2.26.2

[PATCH 1/4] ArmVirtPkg: add FDF rule for self-relocating PrePi

[Ard Biesheuvel]

In preparation for making the self-relocating PrePi use the ordinary
BasePeCoffLib routines for relocating the image in place in memory
at start, add a special FDF rule that builds SEC modules as PE32
images with the relocation metadata preserved.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmVirtPkg/ArmVirtQemuKernel.fdf | 2 +-
 ArmVirtPkg/ArmVirtXen.fdf        | 2 +-
 ArmVirtPkg/ArmVirtRules.fdf.inc  | 5 +++++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf
index 72fc8dd698f8..55e33aba0d55 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.fdf
+++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf
@@ -136,7 +136,7 @@ [FV.FVMAIN_COMPACT]
 READ_LOCK_CAP      = TRUE
 READ_LOCK_STATUS   = TRUE
 
-  INF ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
+  INF RuleOverride = SELF_RELOC ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
 
   FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
     SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
index 6a97bceeacbc..f708878f4965 100644
--- a/ArmVirtPkg/ArmVirtXen.fdf
+++ b/ArmVirtPkg/ArmVirtXen.fdf
@@ -233,7 +233,7 @@ [FV.FVMAIN_COMPACT]
 READ_LOCK_CAP      = TRUE
 READ_LOCK_STATUS   = TRUE
 
-  INF ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
+  INF RuleOverride = SELF_RELOC ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
 
   FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
     SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
diff --git a/ArmVirtPkg/ArmVirtRules.fdf.inc b/ArmVirtPkg/ArmVirtRules.fdf.inc
index 63de26abe056..8496767c718e 100644
--- a/ArmVirtPkg/ArmVirtRules.fdf.inc
+++ b/ArmVirtPkg/ArmVirtRules.fdf.inc
@@ -39,6 +39,11 @@ [Rule.Common.SEC]
     TE  TE Align = Auto                 $(INF_OUTPUT)/$(MODULE_NAME).efi
   }
 
+[Rule.Common.SEC.SELF_RELOC]
+  FILE SEC = $(NAMED_GUID) {
+    PE32  PE32 Align = Auto             $(INF_OUTPUT)/$(MODULE_NAME).efi
+  }
+
 [Rule.Common.PEI_CORE]
   FILE PEI_CORE = $(NAMED_GUID) FIXED {
     TE     TE Align = Auto              $(INF_OUTPUT)/$(MODULE_NAME).efi
-- 
2.26.2

Re: [edk2-devel] [PATCH 1/4] ArmVirtPkg: add FDF rule for self-relocating PrePi

[Laszlo Ersek]

On 06/08/20 19:34, Ard Biesheuvel wrote:
> In preparation for making the self-relocating PrePi use the ordinary
> BasePeCoffLib routines for relocating the image in place in memory
> at start, add a special FDF rule that builds SEC modules as PE32
> images with the relocation metadata preserved.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.fdf | 2 +-
>  ArmVirtPkg/ArmVirtXen.fdf        | 2 +-
>  ArmVirtPkg/ArmVirtRules.fdf.inc  | 5 +++++
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf
> index 72fc8dd698f8..55e33aba0d55 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.fdf
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf
> @@ -136,7 +136,7 @@ [FV.FVMAIN_COMPACT]
>  READ_LOCK_CAP      = TRUE
>  READ_LOCK_STATUS   = TRUE
>  
> -  INF ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +  INF RuleOverride = SELF_RELOC ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
>  
>    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>      SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
> diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
> index 6a97bceeacbc..f708878f4965 100644
> --- a/ArmVirtPkg/ArmVirtXen.fdf
> +++ b/ArmVirtPkg/ArmVirtXen.fdf
> @@ -233,7 +233,7 @@ [FV.FVMAIN_COMPACT]
>  READ_LOCK_CAP      = TRUE
>  READ_LOCK_STATUS   = TRUE
>  
> -  INF ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +  INF RuleOverride = SELF_RELOC ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
>  
>    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>      SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
> diff --git a/ArmVirtPkg/ArmVirtRules.fdf.inc b/ArmVirtPkg/ArmVirtRules.fdf.inc
> index 63de26abe056..8496767c718e 100644
> --- a/ArmVirtPkg/ArmVirtRules.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtRules.fdf.inc
> @@ -39,6 +39,11 @@ [Rule.Common.SEC]
>      TE  TE Align = Auto                 $(INF_OUTPUT)/$(MODULE_NAME).efi
>    }
>  
> +[Rule.Common.SEC.SELF_RELOC]
> +  FILE SEC = $(NAMED_GUID) {
> +    PE32  PE32 Align = Auto             $(INF_OUTPUT)/$(MODULE_NAME).efi
> +  }
> +
>  [Rule.Common.PEI_CORE]
>    FILE PEI_CORE = $(NAMED_GUID) FIXED {
>      TE     TE Align = Auto              $(INF_OUTPUT)/$(MODULE_NAME).efi
> 

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

[PATCH 2/4] BaseTools/Scripts/GccBase.lds: export image base symbol

[Ard Biesheuvel]

To help converted ELF images perform self-relocation, export a symbol
'ElfImageBase' that can be used in the code to discover the start of
the image in memory.

Note the use of PROVIDE() - this ensures that the symbol is only emitted
if a reference to it exists in the code being linked, but no definition.
This means the symbol is never emitted in a way that can conflict with
existing code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 BaseTools/Scripts/GccBase.lds | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
index a9dd2138d4b5..e73c1206a2e2 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -12,6 +12,8 @@
 
 SECTIONS {
 
+  PROVIDE(ElfImageBase = .);
+
   /*
    * The PE/COFF binary consists of DOS and PE/COFF headers, and a sequence of
    * section headers adding up to PECOFF_HEADER_SIZE bytes (which differs
-- 
2.26.2

Re: [PATCH 2/4] BaseTools/Scripts/GccBase.lds: export image base symbol

[Ard Biesheuvel]

On 6/8/20 7:34 PM, Ard Biesheuvel wrote:
> To help converted ELF images perform self-relocation, export a symbol
> 'ElfImageBase' that can be used in the code to discover the start of
> the image in memory.
> 
> Note the use of PROVIDE() - this ensures that the symbol is only emitted
> if a reference to it exists in the code being linked, but no definition.
> This means the symbol is never emitted in a way that can conflict with
> existing code.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Bob, Liming, any objections?

> ---
>   BaseTools/Scripts/GccBase.lds | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
> index a9dd2138d4b5..e73c1206a2e2 100644
> --- a/BaseTools/Scripts/GccBase.lds
> +++ b/BaseTools/Scripts/GccBase.lds
> @@ -12,6 +12,8 @@
>   
>   SECTIONS {
>   
> +  PROVIDE(ElfImageBase = .);
> +
>     /*
>      * The PE/COFF binary consists of DOS and PE/COFF headers, and a sequence of
>      * section headers adding up to PECOFF_HEADER_SIZE bytes (which differs
> 

Re: [edk2-devel] [PATCH 2/4] BaseTools/Scripts/GccBase.lds: export image base symbol

[Bob Feng]

Reviewed-by: Bob Feng<bob.c.feng@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
Sent: Tuesday, June 9, 2020 1:34 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Ilias Apalodimas <ilias.apalodimas@linaro.org>; Julien Grall <julien@xen.org>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: [edk2-devel] [PATCH 2/4] BaseTools/Scripts/GccBase.lds: export image base symbol

To help converted ELF images perform self-relocation, export a symbol 'ElfImageBase' that can be used in the code to discover the start of the image in memory.

Note the use of PROVIDE() - this ensures that the symbol is only emitted if a reference to it exists in the code being linked, but no definition.
This means the symbol is never emitted in a way that can conflict with existing code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 BaseTools/Scripts/GccBase.lds | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds index a9dd2138d4b5..e73c1206a2e2 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -12,6 +12,8 @@
  SECTIONS { +  PROVIDE(ElfImageBase = .);+   /*    * The PE/COFF binary consists of DOS and PE/COFF headers, and a sequence of    * section headers adding up to PECOFF_HEADER_SIZE bytes (which differs-- 
2.26.2


-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#60893): https://edk2.groups.io/g/devel/message/60893
Mute This Topic: https://groups.io/mt/74757210/1768742
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [bob.c.feng@intel.com] -=-=-=-=-=-=

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

[Ard Biesheuvel]

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
-- 
2.26.2

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

[Laszlo Ersek]

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>

[PATCH 4/4] ArmVirtPkg: remove unused files

[Ard Biesheuvel]

We no longer use ELF PIE executables to implement the self-relocating
PrePi so drop the custom linker script and visibility override header
file.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmVirtPkg/Include/Platform/Hidden.h   | 22 -----------
 ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds | 41 --------------------
 2 files changed, 63 deletions(-)

diff --git a/ArmVirtPkg/Include/Platform/Hidden.h b/ArmVirtPkg/Include/Platform/Hidden.h
deleted file mode 100644
index 7a7bdb42b8bd..000000000000
--- a/ArmVirtPkg/Include/Platform/Hidden.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/** @file
-
-  Copyright (c) 2018, Linaro Limited. All rights reserved.
-
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#ifndef __PLATFORM_HIDDEN_H
-#define __PLATFORM_HIDDEN_H
-
-//
-// Setting the GCC -fvisibility=hidden command line option is not quite the same
-// as setting the pragma below: the former only affects definitions, whereas the
-// pragma affects extern declarations as well. So if we want to ensure that no
-// GOT indirected symbol references are emitted, we need to use the pragma, or
-// GOT based cross object references could be emitted, e.g., in libraries, and
-// these cannot be relaxed to ordinary symbol references at link time.
-//
-#pragma GCC visibility push (hidden)
-
-#endif
diff --git a/ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds b/ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds
deleted file mode 100644
index c9a15ca3493a..000000000000
--- a/ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds
+++ /dev/null
@@ -1,41 +0,0 @@
-/** @file
-
-  Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
-
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-SECTIONS
-{
-  PROVIDE(__reloc_base = .);
-
-  . = PECOFF_HEADER_SIZE;
-  .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
-    *(.text .text*)
-    *(.got .got*)
-    *(.rodata .rodata*)
-    *(.data .data*)
-    *(.bss .bss*)
-
-    . = ALIGN(0x20);
-    PROVIDE(__reloc_start = .);
-    *(.rel .rel.*)
-    *(.rela .rela.*)
-    PROVIDE(__reloc_end = .);
-  }
-
-  .note (INFO) : { *(.note.gnu.build-id) }
-
-  /DISCARD/ : {
-    *(.note.GNU-stack)
-    *(.gnu.hash)
-    *(.gnu_debuglink)
-    *(.interp)
-    *(.dynamic)
-    *(.dynsym)
-    *(.dynstr)
-    *(.hash)
-    *(.comment)
-  }
-}
-- 
2.26.2

Re: [edk2-devel] [PATCH 4/4] ArmVirtPkg: remove unused files

[Laszlo Ersek]

On 06/08/20 19:34, Ard Biesheuvel wrote:
> We no longer use ELF PIE executables to implement the self-relocating
> PrePi so drop the custom linker script and visibility override header
> file.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmVirtPkg/Include/Platform/Hidden.h   | 22 -----------
>  ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds | 41 --------------------
>  2 files changed, 63 deletions(-)
> 
> diff --git a/ArmVirtPkg/Include/Platform/Hidden.h b/ArmVirtPkg/Include/Platform/Hidden.h
> deleted file mode 100644
> index 7a7bdb42b8bd..000000000000
> --- a/ArmVirtPkg/Include/Platform/Hidden.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -/** @file
> -
> -  Copyright (c) 2018, Linaro Limited. All rights reserved.
> -
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#ifndef __PLATFORM_HIDDEN_H
> -#define __PLATFORM_HIDDEN_H
> -
> -//
> -// Setting the GCC -fvisibility=hidden command line option is not quite the same
> -// as setting the pragma below: the former only affects definitions, whereas the
> -// pragma affects extern declarations as well. So if we want to ensure that no
> -// GOT indirected symbol references are emitted, we need to use the pragma, or
> -// GOT based cross object references could be emitted, e.g., in libraries, and
> -// these cannot be relaxed to ordinary symbol references at link time.
> -//
> -#pragma GCC visibility push (hidden)
> -
> -#endif
> diff --git a/ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds b/ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds
> deleted file mode 100644
> index c9a15ca3493a..000000000000
> --- a/ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/** @file
> -
> -  Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
> -
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -SECTIONS
> -{
> -  PROVIDE(__reloc_base = .);
> -
> -  . = PECOFF_HEADER_SIZE;
> -  .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> -    *(.text .text*)
> -    *(.got .got*)
> -    *(.rodata .rodata*)
> -    *(.data .data*)
> -    *(.bss .bss*)
> -
> -    . = ALIGN(0x20);
> -    PROVIDE(__reloc_start = .);
> -    *(.rel .rel.*)
> -    *(.rela .rela.*)
> -    PROVIDE(__reloc_end = .);
> -  }
> -
> -  .note (INFO) : { *(.note.gnu.build-id) }
> -
> -  /DISCARD/ : {
> -    *(.note.GNU-stack)
> -    *(.gnu.hash)
> -    *(.gnu_debuglink)
> -    *(.interp)
> -    *(.dynamic)
> -    *(.dynsym)
> -    *(.dynstr)
> -    *(.hash)
> -    *(.comment)
> -  }
> -}
> 

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

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

[Yao, Jiewen]

Thank you Ard.
Good catch on jump-table stuff.

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Tuesday, June 9, 2020 1:34 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Ilias Apalodimas
> <ilias.apalodimas@linaro.org>; Julien Grall <julien@xen.org>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: [PATCH 0/4] ArmVirtPkg: use PE/COFF metadata for self relocation
> 
> 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
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> 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>
> 
> Ard Biesheuvel (4):
>   ArmVirtPkg: add FDF rule for self-relocating PrePi
>   BaseTools/Scripts/GccBase.lds: export image base symbol
>   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                            | 21 +++++++++
>  ArmVirtPkg/ArmVirtRules.fdf.inc                     |  5 ++
>  ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S         | 49 +++++---------------
>  ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S             | 47 +++++--------------
>  ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds              | 41 ----------------
>  BaseTools/Scripts/GccBase.lds                       |  2 +
>  12 files changed, 63 insertions(+), 152 deletions(-)
>  delete mode 100644 ArmVirtPkg/Include/Platform/Hidden.h
>  delete mode 100644 ArmVirtPkg/PrePi/Scripts/PrePi-PIE.lds
> 
> --
> 2.26.2

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

[Julien Grall]

Hi Ard,

On 08/06/2020 18:34, 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.

I have tested the change in a Xen guest. No issues reported.

Tested-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

Re: [edk2-devel] [PATCH 0/4] ArmVirtPkg: use PE/COFF metadata for self relocation

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

Hi Ard,

I have made corresponding changes to the Kvmtool port patch series (that I am working on) and can confirm this patchset works.
Not sure if Tested-by would be correct in the context that the Kvmtool port is not yet merged. In either case.

Tested-by: Sami Mujawar <Sami.Mujawar@arm.com>
Acked-by: Sami Mujawar <Sami.Mujawar@arm.com>

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 428 bytes --]