Before handing over to the DXE core, iterate over all known FFS files
and find the ones that can execute in place. These files are then
relocated in place and mapped with restricted permissions, allowing the
DXE core to dispatch them without the need to perform any manipulation
of the file contents or the page table permissions.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf |   1 +
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c  | 196 ++++++++++++++++++++
 2 files changed, 197 insertions(+)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index f1990eac77607854..60112100df78b396 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -65,6 +65,7 @@ [LibraryClasses]
   PeimEntryPoint
   DebugLib
   DebugAgentLib
+  PeCoffLib
   PeiServicesTablePointerLib
   PerformanceLib
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 2c19f1a507baf34a..1f20db1faffbd1d2 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -10,6 +10,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "DxeIpl.h"
 
+#include <Library/PeCoffLib.h>
+#include <Ppi/MemoryAttribute.h>
+
 //
 // Module Globals used in the DXE to PEI hand off
 // These must be module globals, so the stack can be switched
@@ -228,6 +231,197 @@ ValidateMemoryTypeInfoVariable (
   return TRUE;
 }
 
+/**
+  Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
+  The function is used for XIP code to have optimized memory copy.
+
+  @param FileHandle      The handle to the PE/COFF file
+  @param FileOffset      The offset, in bytes, into the file to read
+  @param ReadSize        The number of bytes to read from the file starting at FileOffset
+  @param Buffer          A pointer to the buffer to read the data into.
+
+  @return EFI_SUCCESS    ReadSize bytes of data were read into Buffer from the
+                         PE/COFF file starting at FileOffset
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+PeiImageRead (
+  IN     VOID   *FileHandle,
+  IN     UINTN  FileOffset,
+  IN     UINTN  *ReadSize,
+  OUT    VOID   *Buffer
+  )
+{
+  CHAR8  *Destination8;
+  CHAR8  *Source8;
+
+  Destination8 = Buffer;
+  Source8      = (CHAR8 *)((UINTN)FileHandle + FileOffset);
+  if (Destination8 != Source8) {
+    CopyMem (Destination8, Source8, *ReadSize);
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+VOID
+RemapImage (
+  IN  EDKII_MEMORY_ATTRIBUTE_PPI          *MemoryPpi,
+  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
+  )
+{
+  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
+  EFI_IMAGE_SECTION_HEADER             *Section;
+  PHYSICAL_ADDRESS                     SectionAddress;
+  EFI_STATUS                           Status;
+  UINT64                               Permissions;
+  UINTN                                Index;
+
+  if (MemoryPpi == NULL) {
+    return;
+  }
+
+  Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext->Handle +
+                                                  ImageContext->PeCoffHeaderOffset);
+  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+  Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) +
+                                         sizeof (EFI_IMAGE_FILE_HEADER) +
+                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
+                                         );
+
+  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
+    SectionAddress = ImageContext->ImageAddress + Section[Index].VirtualAddress;
+    Permissions    = 0;
+
+    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) {
+      Permissions |= EFI_MEMORY_RO;
+    }
+
+    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
+      Permissions |= EFI_MEMORY_XP;
+    }
+
+    Status = MemoryPpi->SetPermissions (
+                          MemoryPpi,
+                          SectionAddress,
+                          Section[Index].Misc.VirtualSize,
+                          Permissions,
+                          Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP
+                          );
+    ASSERT_EFI_ERROR (Status);
+  }
+}
+
+STATIC
+VOID
+RelocateAndRemapDriversInPlace (
+  VOID
+  )
+{
+  EFI_STATUS                    Status;
+  UINTN                         Instance;
+  EFI_PEI_FV_HANDLE             VolumeHandle;
+  EFI_PEI_FILE_HANDLE           FileHandle;
+  PE_COFF_LOADER_IMAGE_CONTEXT  ImageContext;
+  UINT32                        AuthenticationState;
+  EDKII_MEMORY_ATTRIBUTE_PPI    *MemoryPpi;
+
+  MemoryPpi = NULL;
+  PeiServicesLocatePpi (&gEdkiiMemoryAttributePpiGuid, 0, NULL, (VOID **)&MemoryPpi);
+
+  Instance = 0;
+  do {
+    //
+    // Traverse all firmware volume instances
+    //
+    Status = PeiServicesFfsFindNextVolume (Instance, &VolumeHandle);
+    if (Status == EFI_NOT_FOUND) {
+      return;
+    }
+
+    ASSERT_EFI_ERROR (Status);
+
+    FileHandle = NULL;
+    do {
+      Status = PeiServicesFfsFindNextFile (
+                 EFI_FV_FILETYPE_DRIVER,
+                 VolumeHandle,
+                 &FileHandle);
+      if (Status == EFI_NOT_FOUND) {
+        break;
+      }
+
+      ASSERT_EFI_ERROR (Status);
+
+      ZeroMem (&ImageContext, sizeof (ImageContext));
+
+      Status = PeiServicesFfsFindSectionData3 (
+                 EFI_SECTION_PE32,
+                 0,
+                 FileHandle,
+                 &ImageContext.Handle,
+                 &AuthenticationState
+                 );
+      if (Status == EFI_NOT_FOUND) {
+        continue;
+      }
+
+      ASSERT_EFI_ERROR (Status);
+
+      ImageContext.ImageRead = PeiImageRead;
+      Status                 = PeCoffLoaderGetImageInfo (&ImageContext);
+      if (EFI_ERROR (Status)) {
+        ASSERT_EFI_ERROR (Status);
+        continue;
+      }
+
+      ImageContext.ImageAddress = (PHYSICAL_ADDRESS)(UINTN)ImageContext.Handle;
+      if ((ImageContext.ImageAddress & (ImageContext.SectionAlignment - 1)) != 0) {
+        DEBUG ((DEBUG_VERBOSE, "%a: skip PE image at %p\n", __func__, ImageContext.Handle));
+        continue;
+      }
+
+      DEBUG ((
+        DEBUG_INFO,
+        "%a: relocate PE image at %p for execution in place\n",
+        __func__,
+        ImageContext.Handle
+        ));
+
+      //
+      // 'Load' the image in-place - this just performs a sanity check on
+      // the PE metadata but does not actually move any data
+      //
+      Status = PeCoffLoaderLoadImage (&ImageContext);
+      if (EFI_ERROR (Status)) {
+        ASSERT_EFI_ERROR (Status);
+        continue;
+      }
+
+      //
+      // Relocate this driver in place
+      //
+      Status = PeCoffLoaderRelocateImage (&ImageContext);
+      if (EFI_ERROR (Status)) {
+        ASSERT_EFI_ERROR (Status);
+        continue;
+      }
+
+      //
+      // Apply section permissions to the page tables
+      //
+      RemapImage (MemoryPpi, &ImageContext);
+
+    } while (TRUE);
+
+    Instance++;
+  } while (TRUE);
+}
+
 /**
    Main entry point to last PEIM.
 
@@ -436,6 +630,8 @@ DxeLoadCore (
     DxeCoreEntryPoint
     );
 
+  RelocateAndRemapDriversInPlace ();
+
   //
   // Report Status Code EFI_SW_PEI_PC_HANDOFF_TO_NEXT
   //
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105370): https://edk2.groups.io/g/devel/message/105370
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

1. is it possible that a PE image sits in the right location but the SectionAlignment is larger than FileAlignment so each section still needs to be copied to the aligned memory location?

2. PeCoffLoaderRelocateImage() might not be called for XIP 

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Monday, May 29, 2023 6:17 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> Subject: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and
> remap XIP capable DXE drivers
> 
> Before handing over to the DXE core, iterate over all known FFS files
> and find the ones that can execute in place. These files are then
> relocated in place and mapped with restricted permissions, allowing the
> DXE core to dispatch them without the need to perform any manipulation
> of the file contents or the page table permissions.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf |   1 +
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c  | 196 ++++++++++++++++++++
>  2 files changed, 197 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index f1990eac77607854..60112100df78b396 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -65,6 +65,7 @@ [LibraryClasses]
>    PeimEntryPoint
> 
>    DebugLib
> 
>    DebugAgentLib
> 
> +  PeCoffLib
> 
>    PeiServicesTablePointerLib
> 
>    PerformanceLib
> 
> 
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> index 2c19f1a507baf34a..1f20db1faffbd1d2 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -10,6 +10,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
>  #include "DxeIpl.h"
> 
> 
> 
> +#include <Library/PeCoffLib.h>
> 
> +#include <Ppi/MemoryAttribute.h>
> 
> +
> 
>  //
> 
>  // Module Globals used in the DXE to PEI hand off
> 
>  // These must be module globals, so the stack can be switched
> 
> @@ -228,6 +231,197 @@ ValidateMemoryTypeInfoVariable (
>    return TRUE;
> 
>  }
> 
> 
> 
> +/**
> 
> +  Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
> 
> +  The function is used for XIP code to have optimized memory copy.
> 
> +
> 
> +  @param FileHandle      The handle to the PE/COFF file
> 
> +  @param FileOffset      The offset, in bytes, into the file to read
> 
> +  @param ReadSize        The number of bytes to read from the file starting at
> FileOffset
> 
> +  @param Buffer          A pointer to the buffer to read the data into.
> 
> +
> 
> +  @return EFI_SUCCESS    ReadSize bytes of data were read into Buffer from the
> 
> +                         PE/COFF file starting at FileOffset
> 
> +
> 
> +**/
> 
> +STATIC
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +PeiImageRead (
> 
> +  IN     VOID   *FileHandle,
> 
> +  IN     UINTN  FileOffset,
> 
> +  IN     UINTN  *ReadSize,
> 
> +  OUT    VOID   *Buffer
> 
> +  )
> 
> +{
> 
> +  CHAR8  *Destination8;
> 
> +  CHAR8  *Source8;
> 
> +
> 
> +  Destination8 = Buffer;
> 
> +  Source8      = (CHAR8 *)((UINTN)FileHandle + FileOffset);
> 
> +  if (Destination8 != Source8) {
> 
> +    CopyMem (Destination8, Source8, *ReadSize);
> 
> +  }
> 
> +
> 
> +  return EFI_SUCCESS;
> 
> +}
> 
> +
> 
> +STATIC
> 
> +VOID
> 
> +RemapImage (
> 
> +  IN  EDKII_MEMORY_ATTRIBUTE_PPI          *MemoryPpi,
> 
> +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
> 
> +  )
> 
> +{
> 
> +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
> 
> +  EFI_IMAGE_SECTION_HEADER             *Section;
> 
> +  PHYSICAL_ADDRESS                     SectionAddress;
> 
> +  EFI_STATUS                           Status;
> 
> +  UINT64                               Permissions;
> 
> +  UINTN                                Index;
> 
> +
> 
> +  if (MemoryPpi == NULL) {
> 
> +    return;
> 
> +  }
> 
> +
> 
> +  Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8
> *)ImageContext->Handle +
> 
> +                                                  ImageContext->PeCoffHeaderOffset);
> 
> +  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
> 
> +
> 
> +  Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof
> (UINT32) +
> 
> +                                         sizeof (EFI_IMAGE_FILE_HEADER) +
> 
> +                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
> 
> +                                         );
> 
> +
> 
> +  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
> 
> +    SectionAddress = ImageContext->ImageAddress +
> Section[Index].VirtualAddress;
> 
> +    Permissions    = 0;
> 
> +
> 
> +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) {
> 
> +      Permissions |= EFI_MEMORY_RO;
> 
> +    }
> 
> +
> 
> +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
> 
> +      Permissions |= EFI_MEMORY_XP;
> 
> +    }
> 
> +
> 
> +    Status = MemoryPpi->SetPermissions (
> 
> +                          MemoryPpi,
> 
> +                          SectionAddress,
> 
> +                          Section[Index].Misc.VirtualSize,
> 
> +                          Permissions,
> 
> +                          Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP
> 
> +                          );
> 
> +    ASSERT_EFI_ERROR (Status);
> 
> +  }
> 
> +}
> 
> +
> 
> +STATIC
> 
> +VOID
> 
> +RelocateAndRemapDriversInPlace (
> 
> +  VOID
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS                    Status;
> 
> +  UINTN                         Instance;
> 
> +  EFI_PEI_FV_HANDLE             VolumeHandle;
> 
> +  EFI_PEI_FILE_HANDLE           FileHandle;
> 
> +  PE_COFF_LOADER_IMAGE_CONTEXT  ImageContext;
> 
> +  UINT32                        AuthenticationState;
> 
> +  EDKII_MEMORY_ATTRIBUTE_PPI    *MemoryPpi;
> 
> +
> 
> +  MemoryPpi = NULL;
> 
> +  PeiServicesLocatePpi (&gEdkiiMemoryAttributePpiGuid, 0, NULL, (VOID
> **)&MemoryPpi);
> 
> +
> 
> +  Instance = 0;
> 
> +  do {
> 
> +    //
> 
> +    // Traverse all firmware volume instances
> 
> +    //
> 
> +    Status = PeiServicesFfsFindNextVolume (Instance, &VolumeHandle);
> 
> +    if (Status == EFI_NOT_FOUND) {
> 
> +      return;
> 
> +    }
> 
> +
> 
> +    ASSERT_EFI_ERROR (Status);
> 
> +
> 
> +    FileHandle = NULL;
> 
> +    do {
> 
> +      Status = PeiServicesFfsFindNextFile (
> 
> +                 EFI_FV_FILETYPE_DRIVER,
> 
> +                 VolumeHandle,
> 
> +                 &FileHandle);
> 
> +      if (Status == EFI_NOT_FOUND) {
> 
> +        break;
> 
> +      }
> 
> +
> 
> +      ASSERT_EFI_ERROR (Status);
> 
> +
> 
> +      ZeroMem (&ImageContext, sizeof (ImageContext));
> 
> +
> 
> +      Status = PeiServicesFfsFindSectionData3 (
> 
> +                 EFI_SECTION_PE32,
> 
> +                 0,
> 
> +                 FileHandle,
> 
> +                 &ImageContext.Handle,
> 
> +                 &AuthenticationState
> 
> +                 );
> 
> +      if (Status == EFI_NOT_FOUND) {
> 
> +        continue;
> 
> +      }
> 
> +
> 
> +      ASSERT_EFI_ERROR (Status);
> 
> +
> 
> +      ImageContext.ImageRead = PeiImageRead;
> 
> +      Status                 = PeCoffLoaderGetImageInfo (&ImageContext);
> 
> +      if (EFI_ERROR (Status)) {
> 
> +        ASSERT_EFI_ERROR (Status);
> 
> +        continue;
> 
> +      }
> 
> +
> 
> +      ImageContext.ImageAddress =
> (PHYSICAL_ADDRESS)(UINTN)ImageContext.Handle;
> 
> +      if ((ImageContext.ImageAddress & (ImageContext.SectionAlignment - 1)) !=
> 0) {
> 
> +        DEBUG ((DEBUG_VERBOSE, "%a: skip PE image at %p\n", __func__,
> ImageContext.Handle));
> 
> +        continue;
> 
> +      }
> 
> +
> 
> +      DEBUG ((
> 
> +        DEBUG_INFO,
> 
> +        "%a: relocate PE image at %p for execution in place\n",
> 
> +        __func__,
> 
> +        ImageContext.Handle
> 
> +        ));
> 
> +
> 
> +      //
> 
> +      // 'Load' the image in-place - this just performs a sanity check on
> 
> +      // the PE metadata but does not actually move any data
> 
> +      //
> 
> +      Status = PeCoffLoaderLoadImage (&ImageContext);
> 
> +      if (EFI_ERROR (Status)) {
> 
> +        ASSERT_EFI_ERROR (Status);
> 
> +        continue;
> 
> +      }
> 
> +
> 
> +      //
> 
> +      // Relocate this driver in place
> 
> +      //
> 
> +      Status = PeCoffLoaderRelocateImage (&ImageContext);
> 
> +      if (EFI_ERROR (Status)) {
> 
> +        ASSERT_EFI_ERROR (Status);
> 
> +        continue;
> 
> +      }
> 
> +
> 
> +      //
> 
> +      // Apply section permissions to the page tables
> 
> +      //
> 
> +      RemapImage (MemoryPpi, &ImageContext);
> 
> +
> 
> +    } while (TRUE);
> 
> +
> 
> +    Instance++;
> 
> +  } while (TRUE);
> 
> +}
> 
> +
> 
>  /**
> 
>     Main entry point to last PEIM.
> 
> 
> 
> @@ -436,6 +630,8 @@ DxeLoadCore (
>      DxeCoreEntryPoint
> 
>      );
> 
> 
> 
> +  RelocateAndRemapDriversInPlace ();
> 
> +
> 
>    //
> 
>    // Report Status Code EFI_SW_PEI_PC_HANDOFF_TO_NEXT
> 
>    //
> 
> --
> 2.39.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#105370): https://edk2.groups.io/g/devel/message/105370
> Mute This Topic: https://groups.io/mt/99197142/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105414): https://edk2.groups.io/g/devel/message/105414
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 30 May 2023 at 08:45, Ni, Ray <ray.ni@intel.com> wrote:
>
> 1. is it possible that a PE image sits in the right location but the SectionAlignment is larger than FileAlignment so each section still needs to be copied to the aligned memory location?
>

That is a good question. Currently, the ELF toolchains rely on GenFw
to construct the PE images in a way that permits execution in place,
but I have no idea how this works with native PE/COFF toolchains.

> 2. PeCoffLoaderRelocateImage() might not be called for XIP
>

Are you saying it is not permitted? Or that it may not happen?

In any case, relocating the image in place is exactly what the
BaseTools do for XIP PEIMs, so I think applying the same logic here is
reasonable.

> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > Biesheuvel
> > Sent: Monday, May 29, 2023 6:17 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> > <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > Subject: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and
> > remap XIP capable DXE drivers
> >
> > Before handing over to the DXE core, iterate over all known FFS files
> > and find the ones that can execute in place. These files are then
> > relocated in place and mapped with restricted permissions, allowing the
> > DXE core to dispatch them without the need to perform any manipulation
> > of the file contents or the page table permissions.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf |   1 +
> >  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c  | 196 ++++++++++++++++++++
> >  2 files changed, 197 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > index f1990eac77607854..60112100df78b396 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > @@ -65,6 +65,7 @@ [LibraryClasses]
> >    PeimEntryPoint
> >
> >    DebugLib
> >
> >    DebugAgentLib
> >
> > +  PeCoffLib
> >
> >    PeiServicesTablePointerLib
> >
> >    PerformanceLib
> >
> >
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > index 2c19f1a507baf34a..1f20db1faffbd1d2 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > @@ -10,6 +10,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >  #include "DxeIpl.h"
> >
> >
> >
> > +#include <Library/PeCoffLib.h>
> >
> > +#include <Ppi/MemoryAttribute.h>
> >
> > +
> >
> >  //
> >
> >  // Module Globals used in the DXE to PEI hand off
> >
> >  // These must be module globals, so the stack can be switched
> >
> > @@ -228,6 +231,197 @@ ValidateMemoryTypeInfoVariable (
> >    return TRUE;
> >
> >  }
> >
> >
> >
> > +/**
> >
> > +  Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
> >
> > +  The function is used for XIP code to have optimized memory copy.
> >
> > +
> >
> > +  @param FileHandle      The handle to the PE/COFF file
> >
> > +  @param FileOffset      The offset, in bytes, into the file to read
> >
> > +  @param ReadSize        The number of bytes to read from the file starting at
> > FileOffset
> >
> > +  @param Buffer          A pointer to the buffer to read the data into.
> >
> > +
> >
> > +  @return EFI_SUCCESS    ReadSize bytes of data were read into Buffer from the
> >
> > +                         PE/COFF file starting at FileOffset
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> > +PeiImageRead (
> >
> > +  IN     VOID   *FileHandle,
> >
> > +  IN     UINTN  FileOffset,
> >
> > +  IN     UINTN  *ReadSize,
> >
> > +  OUT    VOID   *Buffer
> >
> > +  )
> >
> > +{
> >
> > +  CHAR8  *Destination8;
> >
> > +  CHAR8  *Source8;
> >
> > +
> >
> > +  Destination8 = Buffer;
> >
> > +  Source8      = (CHAR8 *)((UINTN)FileHandle + FileOffset);
> >
> > +  if (Destination8 != Source8) {
> >
> > +    CopyMem (Destination8, Source8, *ReadSize);
> >
> > +  }
> >
> > +
> >
> > +  return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +STATIC
> >
> > +VOID
> >
> > +RemapImage (
> >
> > +  IN  EDKII_MEMORY_ATTRIBUTE_PPI          *MemoryPpi,
> >
> > +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
> >
> > +  )
> >
> > +{
> >
> > +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
> >
> > +  EFI_IMAGE_SECTION_HEADER             *Section;
> >
> > +  PHYSICAL_ADDRESS                     SectionAddress;
> >
> > +  EFI_STATUS                           Status;
> >
> > +  UINT64                               Permissions;
> >
> > +  UINTN                                Index;
> >
> > +
> >
> > +  if (MemoryPpi == NULL) {
> >
> > +    return;
> >
> > +  }
> >
> > +
> >
> > +  Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8
> > *)ImageContext->Handle +
> >
> > +                                                  ImageContext->PeCoffHeaderOffset);
> >
> > +  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
> >
> > +
> >
> > +  Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof
> > (UINT32) +
> >
> > +                                         sizeof (EFI_IMAGE_FILE_HEADER) +
> >
> > +                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
> >
> > +                                         );
> >
> > +
> >
> > +  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
> >
> > +    SectionAddress = ImageContext->ImageAddress +
> > Section[Index].VirtualAddress;
> >
> > +    Permissions    = 0;
> >
> > +
> >
> > +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) {
> >
> > +      Permissions |= EFI_MEMORY_RO;
> >
> > +    }
> >
> > +
> >
> > +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
> >
> > +      Permissions |= EFI_MEMORY_XP;
> >
> > +    }
> >
> > +
> >
> > +    Status = MemoryPpi->SetPermissions (
> >
> > +                          MemoryPpi,
> >
> > +                          SectionAddress,
> >
> > +                          Section[Index].Misc.VirtualSize,
> >
> > +                          Permissions,
> >
> > +                          Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP
> >
> > +                          );
> >
> > +    ASSERT_EFI_ERROR (Status);
> >
> > +  }
> >
> > +}
> >
> > +
> >
> > +STATIC
> >
> > +VOID
> >
> > +RelocateAndRemapDriversInPlace (
> >
> > +  VOID
> >
> > +  )
> >
> > +{
> >
> > +  EFI_STATUS                    Status;
> >
> > +  UINTN                         Instance;
> >
> > +  EFI_PEI_FV_HANDLE             VolumeHandle;
> >
> > +  EFI_PEI_FILE_HANDLE           FileHandle;
> >
> > +  PE_COFF_LOADER_IMAGE_CONTEXT  ImageContext;
> >
> > +  UINT32                        AuthenticationState;
> >
> > +  EDKII_MEMORY_ATTRIBUTE_PPI    *MemoryPpi;
> >
> > +
> >
> > +  MemoryPpi = NULL;
> >
> > +  PeiServicesLocatePpi (&gEdkiiMemoryAttributePpiGuid, 0, NULL, (VOID
> > **)&MemoryPpi);
> >
> > +
> >
> > +  Instance = 0;
> >
> > +  do {
> >
> > +    //
> >
> > +    // Traverse all firmware volume instances
> >
> > +    //
> >
> > +    Status = PeiServicesFfsFindNextVolume (Instance, &VolumeHandle);
> >
> > +    if (Status == EFI_NOT_FOUND) {
> >
> > +      return;
> >
> > +    }
> >
> > +
> >
> > +    ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > +    FileHandle = NULL;
> >
> > +    do {
> >
> > +      Status = PeiServicesFfsFindNextFile (
> >
> > +                 EFI_FV_FILETYPE_DRIVER,
> >
> > +                 VolumeHandle,
> >
> > +                 &FileHandle);
> >
> > +      if (Status == EFI_NOT_FOUND) {
> >
> > +        break;
> >
> > +      }
> >
> > +
> >
> > +      ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > +      ZeroMem (&ImageContext, sizeof (ImageContext));
> >
> > +
> >
> > +      Status = PeiServicesFfsFindSectionData3 (
> >
> > +                 EFI_SECTION_PE32,
> >
> > +                 0,
> >
> > +                 FileHandle,
> >
> > +                 &ImageContext.Handle,
> >
> > +                 &AuthenticationState
> >
> > +                 );
> >
> > +      if (Status == EFI_NOT_FOUND) {
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > +      ImageContext.ImageRead = PeiImageRead;
> >
> > +      Status                 = PeCoffLoaderGetImageInfo (&ImageContext);
> >
> > +      if (EFI_ERROR (Status)) {
> >
> > +        ASSERT_EFI_ERROR (Status);
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      ImageContext.ImageAddress =
> > (PHYSICAL_ADDRESS)(UINTN)ImageContext.Handle;
> >
> > +      if ((ImageContext.ImageAddress & (ImageContext.SectionAlignment - 1)) !=
> > 0) {
> >
> > +        DEBUG ((DEBUG_VERBOSE, "%a: skip PE image at %p\n", __func__,
> > ImageContext.Handle));
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      DEBUG ((
> >
> > +        DEBUG_INFO,
> >
> > +        "%a: relocate PE image at %p for execution in place\n",
> >
> > +        __func__,
> >
> > +        ImageContext.Handle
> >
> > +        ));
> >
> > +
> >
> > +      //
> >
> > +      // 'Load' the image in-place - this just performs a sanity check on
> >
> > +      // the PE metadata but does not actually move any data
> >
> > +      //
> >
> > +      Status = PeCoffLoaderLoadImage (&ImageContext);
> >
> > +      if (EFI_ERROR (Status)) {
> >
> > +        ASSERT_EFI_ERROR (Status);
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      //
> >
> > +      // Relocate this driver in place
> >
> > +      //
> >
> > +      Status = PeCoffLoaderRelocateImage (&ImageContext);
> >
> > +      if (EFI_ERROR (Status)) {
> >
> > +        ASSERT_EFI_ERROR (Status);
> >
> > +        continue;
> >
> > +      }
> >
> > +
> >
> > +      //
> >
> > +      // Apply section permissions to the page tables
> >
> > +      //
> >
> > +      RemapImage (MemoryPpi, &ImageContext);
> >
> > +
> >
> > +    } while (TRUE);
> >
> > +
> >
> > +    Instance++;
> >
> > +  } while (TRUE);
> >
> > +}
> >
> > +
> >
> >  /**
> >
> >     Main entry point to last PEIM.
> >
> >
> >
> > @@ -436,6 +630,8 @@ DxeLoadCore (
> >      DxeCoreEntryPoint
> >
> >      );
> >
> >
> >
> > +  RelocateAndRemapDriversInPlace ();
> >
> > +
> >
> >    //
> >
> >    // Report Status Code EFI_SW_PEI_PC_HANDOFF_TO_NEXT
> >
> >    //
> >
> > --
> > 2.39.2
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#105370): https://edk2.groups.io/g/devel/message/105370
> > Mute This Topic: https://groups.io/mt/99197142/1712937
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> > -=-=-=-=-=-=
> >
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105429): https://edk2.groups.io/g/devel/message/105429
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Ard,

Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.

BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.

Are there any docs w.r.t. the use-case for this? Since when are DXE XIPs a thing? The code above is opportunistic (i.e., failures are ignored), how does the dispatcher take this into account? Why is this applied during PEI, how this this work with the notion of payloads?

Thanks!

Best regards,
Marvin

[1] https://github.com/tianocore/edk2/blob/0f9283429dd487deeeb264ee5670551d596fc208/BaseTools/Conf/tools_def.template#L670-L672

[2] https://github.com/tianocore/edk2-platforms/blob/02fb8459d9c48a8ed4691e9c22f2516c15073835/Silicon/Intel/CoffeelakeSiliconPkg/SiPkgBuildOption.dsc#L129

[3] https://github.com/tianocore/edk2/blob/0f9283429dd487deeeb264ee5670551d596fc208/BaseTools/Source/C/GenFv/GenFvInternalLib.c#L3881-L3897


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105437): https://edk2.groups.io/g/devel/message/105437
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 30 May 2023 at 11:07, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Ard,
>
> Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.
>
> BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.
>

If XIP for PE images with 4k section alignment is an issue, we could
always explore loading them into a separate allocation from PEI, just
like we do with DXE core itself.

This would actually simplify the loader code quite a lot, as we'd be
able to use the PEI core image loader directly. However, it means we'd
have to pass this information (array of <guid, base address> tuples
describing which images were already loaded by DxeIpl) via a HOB or
some other method.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105439): https://edk2.groups.io/g/devel/message/105439
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> On 30. May 2023, at 11:38, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Tue, 30 May 2023 at 11:07, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Hi Ard,
>> 
>> Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.
>> 
>> BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.
>> 
> 
> If XIP for PE images with 4k section alignment is an issue, we could
> always explore loading them into a separate allocation from PEI, just
> like we do with DXE core itself.
> 
> This would actually simplify the loader code quite a lot, as we'd be
> able to use the PEI core image loader directly. However, it means we'd
> have to pass this information (array of <guid, base address> tuples
> describing which images were already loaded by DxeIpl) via a HOB or
> some other method.

I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded or is there another reason this is not handled by DxeCore itself?

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105440): https://edk2.groups.io/g/devel/message/105440
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 30 May 2023 at 11:42, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 30. May 2023, at 11:38, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Tue, 30 May 2023 at 11:07, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Hi Ard,
> >>
> >> Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.
> >>
> >> BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.
> >>
> >
> > If XIP for PE images with 4k section alignment is an issue, we could
> > always explore loading them into a separate allocation from PEI, just
> > like we do with DXE core itself.
> >
> > This would actually simplify the loader code quite a lot, as we'd be
> > able to use the PEI core image loader directly. However, it means we'd
> > have to pass this information (array of <guid, base address> tuples
> > describing which images were already loaded by DxeIpl) via a HOB or
> > some other method.
>
> I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded

Yes.

> or is there another reason this is not handled by DxeCore itself?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105442): https://edk2.groups.io/g/devel/message/105442
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 30 May 2023 at 11:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 30 May 2023 at 11:42, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >
> >
> > > On 30. May 2023, at 11:38, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Tue, 30 May 2023 at 11:07, Marvin Häuser <mhaeuser@posteo.de> wrote:
> > >>
> > >> Hi Ard,
> > >>
> > >> Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.
> > >>
> > >> BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.
> > >>
> > >
> > > If XIP for PE images with 4k section alignment is an issue, we could
> > > always explore loading them into a separate allocation from PEI, just
> > > like we do with DXE core itself.
> > >
> > > This would actually simplify the loader code quite a lot, as we'd be
> > > able to use the PEI core image loader directly. However, it means we'd
> > > have to pass this information (array of <guid, base address> tuples
> > > describing which images were already loaded by DxeIpl) via a HOB or
> > > some other method.
> >
> > I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded
>
> Yes.
>

Well, actually, the first part of the series gets rid of some
pointless memory copies, which is an improvement in itself.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105443): https://edk2.groups.io/g/devel/message/105443
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> On 30. May 2023, at 11:48, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Tue, 30 May 2023 at 11:47, Ard Biesheuvel <ardb@kernel.org <mailto:ardb@kernel.org>> wrote:
>> 
>> On Tue, 30 May 2023 at 11:42, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>> 
>>> 
>>>> On 30. May 2023, at 11:38, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> 
>>>> On Tue, 30 May 2023 at 11:07, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>>>> 
>>>>> Hi Ard,
>>>>> 
>>>>> Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.
>>>>> 
>>>>> BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.
>>>>> 
>>>> 
>>>> If XIP for PE images with 4k section alignment is an issue, we could
>>>> always explore loading them into a separate allocation from PEI, just
>>>> like we do with DXE core itself.
>>>> 
>>>> This would actually simplify the loader code quite a lot, as we'd be
>>>> able to use the PEI core image loader directly. However, it means we'd
>>>> have to pass this information (array of <guid, base address> tuples
>>>> describing which images were already loaded by DxeIpl) via a HOB or
>>>> some other method.
>>> 
>>> I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded
>> 
>> Yes.
>> 
> 
> Well, actually, the first part of the series gets rid of some
> pointless memory copies, which is an improvement in itself.

Sorry, I didn't mean the series, I meant the choice to handle things in DxeIpl over DxeCore in particular.

Is there even a non-FOSS producer of the CpuArch protocol? To my understanding, all (common) Intel platforms use the one in UefiCpuPkg. ARM and friends feel a lot less proprietary to begin with, but I could be wrong. We were quite successful so far with just merging CpuArch into DxeCore and setting it up quite early, but of course not at an industry-wide scale. :)

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105444): https://edk2.groups.io/g/devel/message/105444
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 30 May 2023 at 11:53, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
>
> On 30. May 2023, at 11:48, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 30 May 2023 at 11:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>
>
> On Tue, 30 May 2023 at 11:42, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded
>
>
> Yes.
>
>
> Well, actually, the first part of the series gets rid of some
> pointless memory copies, which is an improvement in itself.
>
>
> Sorry, I didn't mean the series, I meant the choice to handle things in DxeIpl over DxeCore in particular.
>
> Is there even a non-FOSS producer of the CpuArch protocol? To my understanding, all (common) Intel platforms use the one in UefiCpuPkg. ARM and friends feel a lot less proprietary to begin with, but I could be wrong. We were quite successful so far with just merging CpuArch into DxeCore and setting it up quite early, but of course not at an industry-wide scale. :)

What about the dependencies of CpuArch protocol? On ARM, this is the
GIC driver (for the interrupt controller), which has its own platform
specific dependencies.

So this is not tractable in general, and the only options we have (imo) are

- add memory permission attribute handling to DxeCore directly (via a
library with arch specific implementations)
- add a way to dispatch the CpuDxe *and its dependencies* without the
need to manipulate memory permissions

Clumping everything together into DxeCore does not appear to me as a
sustainable approach for this.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105445): https://edk2.groups.io/g/devel/message/105445
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> On 30. May 2023, at 12:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Tue, 30 May 2023 at 11:53, Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> wrote:
>> 
>> 
>> 
>> On 30. May 2023, at 11:48, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> On Tue, 30 May 2023 at 11:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> 
>> On Tue, 30 May 2023 at 11:42, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> 
>> I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded
>> 
>> 
>> Yes.
>> 
>> 
>> Well, actually, the first part of the series gets rid of some
>> pointless memory copies, which is an improvement in itself.
>> 
>> 
>> Sorry, I didn't mean the series, I meant the choice to handle things in DxeIpl over DxeCore in particular.
>> 
>> Is there even a non-FOSS producer of the CpuArch protocol? To my understanding, all (common) Intel platforms use the one in UefiCpuPkg. ARM and friends feel a lot less proprietary to begin with, but I could be wrong. We were quite successful so far with just merging CpuArch into DxeCore and setting it up quite early, but of course not at an industry-wide scale. :)
> 
> What about the dependencies of CpuArch protocol? On ARM, this is the
> GIC driver (for the interrupt controller), which has its own platform
> specific dependencies.

Hmm, was that for the exception handler? I forgot that was in CpuDxe too, I specifically meant the memory permission related things, sorry!

> 
> So this is not tractable in general, and the only options we have (imo) are
> 
> - add memory permission attribute handling to DxeCore directly (via a
> library with arch specific implementations)

Yes, this.

> - add a way to dispatch the CpuDxe *and its dependencies* without the
> need to manipulate memory permissions

That would be awful and I'd prefer your current solution over this.

> 
> Clumping everything together into DxeCore does not appear to me as a
> sustainable approach for this.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105447): https://edk2.groups.io/g/devel/message/105447
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

When a PE has a global large variable, there could be a .bss section whose actual size is 0 or very small but the eventual size when loading to memory will be larger.
Can XIP work with this section? @Liu, Zhiguang<mailto:zhiguang.liu@intel.com> brought this question when discussing with me offline😊

Thanks,
Ray

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Tuesday, May 30, 2023 6:25 PM
To: Ard Biesheuvel <ardb@kernel.org>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers


On 30. May 2023, at 12:02, Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:

On Tue, 30 May 2023 at 11:53, Marvin Häuser <mhaeuser@posteo.de<mailto:mhaeuser@posteo.de>> wrote:




On 30. May 2023, at 11:48, Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:

On Tue, 30 May 2023 at 11:47, Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:


On Tue, 30 May 2023 at 11:42, Marvin Häuser <mhaeuser@posteo.de<mailto:mhaeuser@posteo.de>> wrote:


I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded


Yes.


Well, actually, the first part of the series gets rid of some
pointless memory copies, which is an improvement in itself.


Sorry, I didn't mean the series, I meant the choice to handle things in DxeIpl over DxeCore in particular.

Is there even a non-FOSS producer of the CpuArch protocol? To my understanding, all (common) Intel platforms use the one in UefiCpuPkg. ARM and friends feel a lot less proprietary to begin with, but I could be wrong. We were quite successful so far with just merging CpuArch into DxeCore and setting it up quite early, but of course not at an industry-wide scale. :)

What about the dependencies of CpuArch protocol? On ARM, this is the
GIC driver (for the interrupt controller), which has its own platform
specific dependencies.

Hmm, was that for the exception handler? I forgot that was in CpuDxe too, I specifically meant the memory permission related things, sorry!



So this is not tractable in general, and the only options we have (imo) are

- add memory permission attribute handling to DxeCore directly (via a
library with arch specific implementations)

Yes, this.


- add a way to dispatch the CpuDxe *and its dependencies* without the
need to manipulate memory permissions

That would be awful and I'd prefer your current solution over this.



Clumping everything together into DxeCore does not appear to me as a
sustainable approach for this.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105481): https://edk2.groups.io/g/devel/message/105481
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> On 30. May 2023, at 11:06, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> Hi Ard,
> 
> Native PE toolchains *generally* also generate XIP images (/ALIGN = /FILEALIGN, but with 32 Byte rather than 64 Byte alignment compared to GCC49+ / CLANGDWARF) [1]. However, because they are underaligned by default (even for RT images that run in an OS context and MM drivers... sigh...), platforms manually override SectionAlignment, but not necessarily FileAlignment [2], breaking XIP. I don't think what you are doing is perfectly safe for these, as they will have FileAlignment < SectionAlignment (and by all chances, BaseTools is borked too). In my opinion check for FileAlignment == SectionAlignment. I can't vouch for how likely FileAlignment has a sane value, the AUDK loader does not read it at all and instead checks PointerToRawData == VirtualAddress, etc.
> 
> BaseTools generally has poor support for non-XIP vs XIP, probably due to notorious underalignment since the very beginning. For PEIM XIPs for example, which must ship pre-relocated at least for Intel, GenFv just relocates the image in-memory and then copies the changes back to the FFS file [3]. There is no concept of changing the image file size within the procedure and as such, a non-XIP image cannot be converted to XIP on demand. This would be useful for a distinction between pre-memory and post-memory PEIMs, the former of which must be XIP (thus aligned), while the latter can be loaded and relocated in-RAM (thus can be underaligned w.r.t. FileAlignment), but alas.
> 
> Are there any docs w.r.t. the use-case for this? Since when are DXE XIPs a thing? The code above is opportunistic (i.e., failures are ignored), how does the dispatcher take this into account? Why is this applied during PEI, how this this work with the notion of payloads?

Sorry, I just realized this was a patch *series*! :(
For the questions that are answered in commit messages or implicitly by the code, please disregard, I'll read through them. Not sure the payload situation is obvious, as it's not all that obvious to me how they work in the first place (though I do know, you can both load a payload from edk2 PEI and have edk2 DXE loaded as a payload from something else, so this definitely is a concern in some way).

> 
> Thanks!
> 
> Best regards,
> Marvin
> 
> [1] https://github.com/tianocore/edk2/blob/0f9283429dd487deeeb264ee5670551d596fc208/BaseTools/Conf/tools_def.template#L670-L672
> 
> [2] https://github.com/tianocore/edk2-platforms/blob/02fb8459d9c48a8ed4691e9c22f2516c15073835/Silicon/Intel/CoffeelakeSiliconPkg/SiPkgBuildOption.dsc#L129
> 
> [3] https://github.com/tianocore/edk2/blob/0f9283429dd487deeeb264ee5670551d596fc208/BaseTools/Source/C/GenFv/GenFvInternalLib.c#L3881-L3897



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105438): https://edk2.groups.io/g/devel/message/105438
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, May 30, 2023 3:58 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-
> denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Leif Lindholm <quic_llindhol@quicinc.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate
> and remap XIP capable DXE drivers
> 
> On Tue, 30 May 2023 at 08:45, Ni, Ray <ray.ni@intel.com> wrote:
> >
> > 1. is it possible that a PE image sits in the right location but the
> SectionAlignment is larger than FileAlignment so each section still needs to be
> copied to the aligned memory location?
> >
> 
> That is a good question. Currently, the ELF toolchains rely on GenFw
> to construct the PE images in a way that permits execution in place,
> but I have no idea how this works with native PE/COFF toolchains.
> 
> > 2. PeCoffLoaderRelocateImage() might not be called for XIP
> >
> 
> Are you saying it is not permitted? Or that it may not happen?
> 
> In any case, relocating the image in place is exactly what the
> BaseTools do for XIP PEIMs, so I think applying the same logic here is
> reasonable.

But when the image is in SPI flash (MMIO device, rather in physical memory), relocating the image in place should not be performed.
Or you require platform build strips the relocation section for drivers XIP in SPI flash?
 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105430): https://edk2.groups.io/g/devel/message/105430
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 30 May 2023 at 10:02, Ni, Ray <ray.ni@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > Biesheuvel
> > Sent: Tuesday, May 30, 2023 3:58 PM
> > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-
> > denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Leif Lindholm <quic_llindhol@quicinc.com>; Michael Kubacki
> > <mikuback@linux.microsoft.com>
> > Subject: Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate
> > and remap XIP capable DXE drivers
> >
> > On Tue, 30 May 2023 at 08:45, Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > 1. is it possible that a PE image sits in the right location but the
> > SectionAlignment is larger than FileAlignment so each section still needs to be
> > copied to the aligned memory location?
> > >
> >
> > That is a good question. Currently, the ELF toolchains rely on GenFw
> > to construct the PE images in a way that permits execution in place,
> > but I have no idea how this works with native PE/COFF toolchains.
> >
> > > 2. PeCoffLoaderRelocateImage() might not be called for XIP
> > >
> >
> > Are you saying it is not permitted? Or that it may not happen?
> >
> > In any case, relocating the image in place is exactly what the
> > BaseTools do for XIP PEIMs, so I think applying the same logic here is
> > reasonable.
>
> But when the image is in SPI flash (MMIO device, rather in physical memory), relocating the image in place should not be performed.
> Or you require platform build strips the relocation section for drivers XIP in SPI flash?
>

This code only operates on EFI_FV_FILETYPE_DRIVER, which does not
cover the PEI core or PEIMs.

But we should probably only operate on FVs that are covered by PEI
permanent memory anyway.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105431): https://edk2.groups.io/g/devel/message/105431
Mute This Topic: https://groups.io/mt/99197142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-