[RFC PATCH 00/11] Permit DXE drivers to execute in place

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [RFC PATCH 00/11] Permit DXE drivers to execute in place
@ 2023-05-29 10:16 Ard Biesheuvel
  2023-05-29 10:16 ` [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage Ard Biesheuvel
                   ` (11 more replies)
  0 siblings, 12 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:16 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

TL;DR - allow DXE drivers to execute in place from the decompressed FV
loaded into memory by DxeIpl so we can apply strict permissions before
dispatching DXE core.

Currently, executable images loaded from firmware volumes are copied at
least three times: once in the firmware volume driver, once in the DXE
core load image code, and finally when the PE sections are populated in
memory based on the section descriptions in the file.

At least two of these copies serve little purpose, given that most
drivers are typically dispatched from a memory-mapped firmware volume
that is loaded into DRAM by DxeIpl from a compressed image in the boot
FV, and so we can take a short-cut in the DXE image loader so that the
PE/COFF library that performs the load uses the image in the memory
mapped FV as its source directly. This is implemented by the first 6
patches (where the first 3 are just cleanups)

With this logic in place, we can go one step further, and actually
dispatch the image in place (similar to how XIP PEIMs are dispatched),
without over moving it out of the decompressed firmware volume. This
requires the image to be aligned sufficiently inside the FV, but this is
also the same logic that applies to XIP PEIMs, and this can be achieved
trivially by tweaking some FDF image generation rules. (Note that this
adds padding to the FV, but this generally compresses well, and we
ultimately uses less memory at runtime by not making a copy of the
image).

This requires the DXE IPL (which is the component that decompresses the
firmware volumes to memory) to iterate over the contents and relocate
these drivers in place. Given that DXE IPL is already in charge of
applying NX permissions to the stack and to other memory regions, we can
trivially extend it to apply restricted permissions to the XIP DXE
drivers after relocation.

This means we enter DXE core with those DXE drivers ready to be
dispatched, removing the need to perform manipulation of memory
attributes before the CPU arch protocol is dispatched, which is a bit of
a catch-22 otherwise.

With these changes in place, the platform no longer needs to map memory
writable and executable by default, and all DRAM can be mapped
non-executable right out of reset.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Taylor Beebe <t@taylorbeebe.com>
Cc: Oliver Smith-Denny <osd@smith-denny.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>

Ard Biesheuvel (11):
  MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage
  MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage
  MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage
  MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files
  MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image
    copy
  MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible
  MdeModulePkg/DxeCore: Execute loaded images in place if possible
  MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state
  ArmVirtPkg/ArmVirtQemu: Allow CPU arch protocol DXE to execute in
    place
  ArmVirtPkg/ArmVirtQemu: Map all DRAM non-execute by default

 ArmVirtPkg/ArmVirtQemu.dsc                                 |   1 +
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                       |  17 +-
 ArmVirtPkg/ArmVirtRules.fdf.inc                            |   9 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c |   4 +-
 ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf                |   2 +-
 MdeModulePkg/Core/Dxe/DxeMain.h                            |   1 +
 MdeModulePkg/Core/Dxe/DxeMain.inf                          |   3 +
 MdeModulePkg/Core/Dxe/FwVol/FwVol.c                        | 113 ++++++-
 MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h                  |  31 ++
 MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c                    |  22 --
 MdeModulePkg/Core/Dxe/Image/Image.c                        | 322 ++++++++++----------
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c              |   7 +
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                    |   1 +
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c                     | 196 ++++++++++++
 MdeModulePkg/Include/Protocol/MemoryMappedFv.h             |  59 ++++
 MdeModulePkg/MdeModulePkg.dec                              |   6 +
 16 files changed, 607 insertions(+), 187 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/MemoryMappedFv.h

-- 
2.39.2

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
@ 2023-05-29 10:16 ` Ard Biesheuvel
  2023-05-30  5:54   ` Ni, Ray
  2023-05-29 10:16 ` [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage Ard Biesheuvel
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:16 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

CoreLoadImageCommon's only user passes NULL for its EntryPoint argument,
so it has no purpose and can simply be dropped. While at it, make
CoreLoadImageCommon STATIC to prevent it from being accessed from other
translation units.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 9dbfb2a1fad22ced..2f2dfe5d0496dc4f 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -560,7 +560,6 @@ CoreIsImageTypeSupported (
   @param  Pe32Handle              The handle of PE32 image
   @param  Image                   PE image to be loaded
   @param  DstBuffer               The buffer to store the image
-  @param  EntryPoint              A pointer to the entry point
   @param  Attribute               The bit mask of attributes to set for the load
                                   PE image
 
@@ -577,7 +576,6 @@ CoreLoadPeImage (
   IN VOID                       *Pe32Handle,
   IN LOADED_IMAGE_PRIVATE_DATA  *Image,
   IN EFI_PHYSICAL_ADDRESS       DstBuffer    OPTIONAL,
-  OUT EFI_PHYSICAL_ADDRESS      *EntryPoint  OPTIONAL,
   IN  UINT32                    Attribute
   )
 {
@@ -810,13 +808,6 @@ CoreLoadPeImage (
     }
   }
 
-  //
-  // Fill in the entry point of the image if it is available
-  //
-  if (EntryPoint != NULL) {
-    *EntryPoint = Image->ImageContext.EntryPoint;
-  }
-
   //
   // Print the load address and the PDB file name if it is available
   //
@@ -1111,7 +1102,6 @@ CoreUnloadAndCloseImage (
                                   this parameter contains the required number.
   @param  ImageHandle             Pointer to the returned image handle that is
                                   created when the image is successfully loaded.
-  @param  EntryPoint              A pointer to the entry point
   @param  Attribute               The bit mask of attributes to set for the load
                                   PE image
 
@@ -1134,6 +1124,7 @@ CoreUnloadAndCloseImage (
                                   platform policy specifies that the image should not be started.
 
 **/
+STATIC
 EFI_STATUS
 CoreLoadImageCommon (
   IN  BOOLEAN                   BootPolicy,
@@ -1144,7 +1135,6 @@ CoreLoadImageCommon (
   IN  EFI_PHYSICAL_ADDRESS      DstBuffer           OPTIONAL,
   IN OUT UINTN                  *NumberOfPages      OPTIONAL,
   OUT EFI_HANDLE                *ImageHandle,
-  OUT EFI_PHYSICAL_ADDRESS      *EntryPoint         OPTIONAL,
   IN  UINT32                    Attribute
   )
 {
@@ -1375,9 +1365,9 @@ CoreLoadImageCommon (
   }
 
   //
-  // Load the image.  If EntryPoint is Null, it will not be set.
+  // Load the image.
   //
-  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, EntryPoint, Attribute);
+  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, Attribute);
   if (EFI_ERROR (Status)) {
     if ((Status == EFI_BUFFER_TOO_SMALL) || (Status == EFI_OUT_OF_RESOURCES)) {
       if (NumberOfPages != NULL) {
@@ -1559,7 +1549,6 @@ CoreLoadImage (
              (EFI_PHYSICAL_ADDRESS)(UINTN)NULL,
              NULL,
              ImageHandle,
-             NULL,
              EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION | EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRATION
              );
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
  2023-05-29 10:16 ` [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage Ard Biesheuvel
@ 2023-05-29 10:16 ` Ard Biesheuvel
  2023-05-30  5:58   ` Ni, Ray
  2023-05-29 10:16 ` [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage Ard Biesheuvel
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:16 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

The DstBuffer and NumberOfPages arguments to CoreLoadImageCommon () are
never set by its only caller CoreLoadImage() so let's drop them from the
prototype.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 174 +++++++-------------
 1 file changed, 56 insertions(+), 118 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 2f2dfe5d0496dc4f..6625d0cd0ff82107 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -559,7 +559,6 @@ CoreIsImageTypeSupported (
                                   boot selection.
   @param  Pe32Handle              The handle of PE32 image
   @param  Image                   PE image to be loaded
-  @param  DstBuffer               The buffer to store the image
   @param  Attribute               The bit mask of attributes to set for the load
                                   PE image
 
@@ -570,17 +569,16 @@ CoreIsImageTypeSupported (
   @retval EFI_BUFFER_TOO_SMALL    Buffer for image is too small
 
 **/
+STATIC
 EFI_STATUS
 CoreLoadPeImage (
   IN BOOLEAN                    BootPolicy,
   IN VOID                       *Pe32Handle,
   IN LOADED_IMAGE_PRIVATE_DATA  *Image,
-  IN EFI_PHYSICAL_ADDRESS       DstBuffer    OPTIONAL,
   IN  UINT32                    Attribute
   )
 {
   EFI_STATUS  Status;
-  BOOLEAN     DstBufAlocated;
   UINTN       Size;
 
   ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext));
@@ -633,99 +631,67 @@ CoreLoadPeImage (
   }
 
   //
-  // Allocate memory of the correct memory type aligned on the required image boundary
+  // Allocate Destination Buffer as caller did not pass it in
   //
-  DstBufAlocated = FALSE;
-  if (DstBuffer == 0) {
-    //
-    // Allocate Destination Buffer as caller did not pass it in
-    //
 
-    if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
-      Size = (UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment;
-    } else {
-      Size = (UINTN)Image->ImageContext.ImageSize;
-    }
+  if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
+    Size = (UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment;
+  } else {
+    Size = (UINTN)Image->ImageContext.ImageSize;
+  }
 
-    Image->NumberOfPages = EFI_SIZE_TO_PAGES (Size);
+  Image->NumberOfPages = EFI_SIZE_TO_PAGES (Size);
 
-    //
-    // If the image relocations have not been stripped, then load at any address.
-    // Otherwise load at the address at which it was linked.
-    //
-    // Memory below 1MB should be treated reserved for CSM and there should be
-    // no modules whose preferred load addresses are below 1MB.
-    //
-    Status = EFI_OUT_OF_RESOURCES;
-    //
-    // If Loading Module At Fixed Address feature is enabled, the module should be loaded to
-    // a specified address.
-    //
-    if (PcdGet64 (PcdLoadModuleAtFixAddressEnable) != 0 ) {
-      Status = GetPeCoffImageFixLoadingAssignedAddress (&(Image->ImageContext));
-
-      if (EFI_ERROR (Status)) {
-        //
-        // If the code memory is not ready, invoke CoreAllocatePage with AllocateAnyPages to load the driver.
-        //
-        DEBUG ((DEBUG_INFO|DEBUG_LOAD, "LOADING MODULE FIXED ERROR: Loading module at fixed address failed since specified memory is not available.\n"));
-
-        Status = CoreAllocatePages (
-                   AllocateAnyPages,
-                   (EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
-                   Image->NumberOfPages,
-                   &Image->ImageContext.ImageAddress
-                   );
-      }
-    } else {
-      if ((Image->ImageContext.ImageAddress >= 0x100000) || Image->ImageContext.RelocationsStripped) {
-        Status = CoreAllocatePages (
-                   AllocateAddress,
-                   (EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
-                   Image->NumberOfPages,
-                   &Image->ImageContext.ImageAddress
-                   );
-      }
-
-      if (EFI_ERROR (Status) && !Image->ImageContext.RelocationsStripped) {
-        Status = CoreAllocatePages (
-                   AllocateAnyPages,
-                   (EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
-                   Image->NumberOfPages,
-                   &Image->ImageContext.ImageAddress
-                   );
-      }
-    }
+  //
+  // If the image relocations have not been stripped, then load at any address.
+  // Otherwise load at the address at which it was linked.
+  //
+  // Memory below 1MB should be treated reserved for CSM and there should be
+  // no modules whose preferred load addresses are below 1MB.
+  //
+  Status = EFI_OUT_OF_RESOURCES;
+  //
+  // If Loading Module At Fixed Address feature is enabled, the module should be loaded to
+  // a specified address.
+  //
+  if (PcdGet64 (PcdLoadModuleAtFixAddressEnable) != 0 ) {
+    Status = GetPeCoffImageFixLoadingAssignedAddress (&(Image->ImageContext));
 
     if (EFI_ERROR (Status)) {
-      return Status;
-    }
+      //
+      // If the code memory is not ready, invoke CoreAllocatePage with AllocateAnyPages to load the driver.
+      //
+      DEBUG ((DEBUG_INFO|DEBUG_LOAD, "LOADING MODULE FIXED ERROR: Loading module at fixed address failed since specified memory is not available.\n"));
 
-    DstBufAlocated = TRUE;
+      Status = CoreAllocatePages (
+                 AllocateAnyPages,
+                 (EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
+                 Image->NumberOfPages,
+                 &Image->ImageContext.ImageAddress
+                 );
+    }
   } else {
-    //
-    // Caller provided the destination buffer
-    //
-
-    if (Image->ImageContext.RelocationsStripped && (Image->ImageContext.ImageAddress != DstBuffer)) {
-      //
-      // If the image relocations were stripped, and the caller provided a
-      // destination buffer address that does not match the address that the
-      // image is linked at, then the image cannot be loaded.
-      //
-      return EFI_INVALID_PARAMETER;
+    if ((Image->ImageContext.ImageAddress >= 0x100000) || Image->ImageContext.RelocationsStripped) {
+      Status = CoreAllocatePages (
+                 AllocateAddress,
+                 (EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
+                 Image->NumberOfPages,
+                 &Image->ImageContext.ImageAddress
+                 );
     }
 
-    if ((Image->NumberOfPages != 0) &&
-        (Image->NumberOfPages <
-         (EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment))))
-    {
-      Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
-      return EFI_BUFFER_TOO_SMALL;
+    if (EFI_ERROR (Status) && !Image->ImageContext.RelocationsStripped) {
+      Status = CoreAllocatePages (
+                 AllocateAnyPages,
+                 (EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
+                 Image->NumberOfPages,
+                 &Image->ImageContext.ImageAddress
+                 );
     }
+  }
 
-    Image->NumberOfPages             = EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
-    Image->ImageContext.ImageAddress = DstBuffer;
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
   Image->ImageBasePage = Image->ImageContext.ImageAddress;
@@ -875,12 +841,9 @@ CoreLoadPeImage (
   //
   // Free memory.
   //
-
-  if (DstBufAlocated) {
-    CoreFreePages (Image->ImageContext.ImageAddress, Image->NumberOfPages);
-    Image->ImageContext.ImageAddress = 0;
-    Image->ImageBasePage             = 0;
-  }
+  CoreFreePages (Image->ImageContext.ImageAddress, Image->NumberOfPages);
+  Image->ImageContext.ImageAddress = 0;
+  Image->ImageBasePage             = 0;
 
   if (Image->ImageContext.FixupData != NULL) {
     CoreFreePool (Image->ImageContext.FixupData);
@@ -1094,12 +1057,6 @@ CoreUnloadAndCloseImage (
   @param  SourceBuffer            If not NULL, a pointer to the memory location
                                   containing a copy of the image to be loaded.
   @param  SourceSize              The size in bytes of SourceBuffer.
-  @param  DstBuffer               The buffer to store the image
-  @param  NumberOfPages           If not NULL, it inputs a pointer to the page
-                                  number of DstBuffer and outputs a pointer to
-                                  the page number of the image. If this number is
-                                  not enough,  return EFI_BUFFER_TOO_SMALL and
-                                  this parameter contains the required number.
   @param  ImageHandle             Pointer to the returned image handle that is
                                   created when the image is successfully loaded.
   @param  Attribute               The bit mask of attributes to set for the load
@@ -1132,8 +1089,6 @@ CoreLoadImageCommon (
   IN  EFI_DEVICE_PATH_PROTOCOL  *FilePath,
   IN  VOID                      *SourceBuffer       OPTIONAL,
   IN  UINTN                     SourceSize,
-  IN  EFI_PHYSICAL_ADDRESS      DstBuffer           OPTIONAL,
-  IN OUT UINTN                  *NumberOfPages      OPTIONAL,
   OUT EFI_HANDLE                *ImageHandle,
   IN  UINT32                    Attribute
   )
@@ -1342,12 +1297,7 @@ CoreLoadImageCommon (
   Image->Info.Revision     = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
   Image->Info.FilePath     = DuplicateDevicePath (FilePath);
   Image->Info.ParentHandle = ParentImageHandle;
-
-  if (NumberOfPages != NULL) {
-    Image->NumberOfPages = *NumberOfPages;
-  } else {
-    Image->NumberOfPages = 0;
-  }
+  Image->NumberOfPages     = 0;
 
   //
   // Install the protocol interfaces for this image
@@ -1367,21 +1317,11 @@ CoreLoadImageCommon (
   //
   // Load the image.
   //
-  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, Attribute);
+  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, Attribute);
   if (EFI_ERROR (Status)) {
-    if ((Status == EFI_BUFFER_TOO_SMALL) || (Status == EFI_OUT_OF_RESOURCES)) {
-      if (NumberOfPages != NULL) {
-        *NumberOfPages = Image->NumberOfPages;
-      }
-    }
-
     goto Done;
   }
 
-  if (NumberOfPages != NULL) {
-    *NumberOfPages = Image->NumberOfPages;
-  }
-
   //
   // Register the image in the Debug Image Info Table if the attribute is set
   //
@@ -1473,7 +1413,7 @@ CoreLoadImageCommon (
   //
   if (EFI_ERROR (Status)) {
     if (Image != NULL) {
-      CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0));
+      CoreUnloadAndCloseImage (Image, TRUE);
       Image = NULL;
     }
   } else if (EFI_ERROR (SecurityStatus)) {
@@ -1546,8 +1486,6 @@ CoreLoadImage (
              FilePath,
              SourceBuffer,
              SourceSize,
-             (EFI_PHYSICAL_ADDRESS)(UINTN)NULL,
-             NULL,
              ImageHandle,
              EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION | EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRATION
              );
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
  2023-05-29 10:16 ` [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage Ard Biesheuvel
  2023-05-29 10:16 ` [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage Ard Biesheuvel
@ 2023-05-29 10:16 ` Ard Biesheuvel
  2023-05-30  5:59   ` Ni, Ray
  2023-05-29 10:16 ` [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files Ard Biesheuvel
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:16 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

The FreePage argument to CoreUnloadAndCloseImage () is now always TRUE
so drop it from the prototype. While at it, make the function static as
it is never called from another translation unit.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 6625d0cd0ff82107..f30e369370a09609 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -888,13 +888,12 @@ CoreLoadedImageInfo (
   Unloads EFI image from memory.
 
   @param  Image                   EFI image
-  @param  FreePage                Free allocated pages
 
 **/
+STATIC
 VOID
 CoreUnloadAndCloseImage (
-  IN LOADED_IMAGE_PRIVATE_DATA  *Image,
-  IN BOOLEAN                    FreePage
+  IN LOADED_IMAGE_PRIVATE_DATA  *Image
   )
 {
   EFI_STATUS                           Status;
@@ -1022,7 +1021,7 @@ CoreUnloadAndCloseImage (
   //
   // Free the Image from memory
   //
-  if ((Image->ImageBasePage != 0) && FreePage) {
+  if (Image->ImageBasePage != 0) {
     CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
   }
 
@@ -1413,7 +1412,7 @@ CoreLoadImageCommon (
   //
   if (EFI_ERROR (Status)) {
     if (Image != NULL) {
-      CoreUnloadAndCloseImage (Image, TRUE);
+      CoreUnloadAndCloseImage (Image);
       Image = NULL;
     }
   } else if (EFI_ERROR (SecurityStatus)) {
@@ -1711,7 +1710,7 @@ CoreStartImage (
   // unload it
   //
   if (EFI_ERROR (Image->Status) || (Image->Type == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION)) {
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
     //
     // ImageHandle may be invalid after the image is unloaded, so use NULL handle to record perf log.
     //
@@ -1776,7 +1775,7 @@ CoreExit (
     //
     // The image has not been started so just free its resources
     //
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
     Status = EFI_SUCCESS;
     goto Done;
   }
@@ -1874,7 +1873,7 @@ CoreUnloadImage (
     //
     // if the Image was not started or Unloaded O.K. then clean up
     //
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
   }
 
 Done:
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2023-05-29 10:16 ` [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage Ard Biesheuvel
@ 2023-05-29 10:16 ` Ard Biesheuvel
  2023-05-30  6:03   ` Ni, Ray
  2023-05-29 10:16 ` [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy Ard Biesheuvel
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:16 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

If a firmware volume is memory mapped, it means we can access it
contents directly, and so caching serves little purpose beyond
potentially a minor performance improvement. However, given that most
files are read only a single time, and dispatched from a decompressed
firmware volume in DRAM, we can just avoid the redundant caching here.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c | 22 --------------------
 1 file changed, 22 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
index 2ff22c93aad48d7e..69df96685d680868 100644
--- a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
+++ b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
@@ -284,7 +284,6 @@ FvReadFile (
   UINT8                   *SrcPtr;
   EFI_FFS_FILE_HEADER     *FfsHeader;
   UINTN                   InputBufferSize;
-  UINTN                   WholeFileSize;
 
   if (NameGuid == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -316,27 +315,6 @@ FvReadFile (
   // Get a pointer to the header
   //
   FfsHeader = FvDevice->LastKey->FfsHeader;
-  if (FvDevice->IsMemoryMapped) {
-    //
-    // Memory mapped FV has not been cached, so here is to cache by file.
-    //
-    if (!FvDevice->LastKey->FileCached) {
-      //
-      // Cache FFS file to memory buffer.
-      //
-      WholeFileSize = IS_FFS_FILE2 (FfsHeader) ? FFS_FILE2_SIZE (FfsHeader) : FFS_FILE_SIZE (FfsHeader);
-      FfsHeader     = AllocateCopyPool (WholeFileSize, FfsHeader);
-      if (FfsHeader == NULL) {
-        return EFI_OUT_OF_RESOURCES;
-      }
-
-      //
-      // Let FfsHeader in FfsFileEntry point to the cached file buffer.
-      //
-      FvDevice->LastKey->FfsHeader  = FfsHeader;
-      FvDevice->LastKey->FileCached = TRUE;
-    }
-  }
 
   //
   // Remember callers buffer size
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2023-05-29 10:16 ` [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files Ard Biesheuvel
@ 2023-05-29 10:16 ` Ard Biesheuvel
  2023-05-30  6:21   ` Ni, Ray
  2023-05-29 10:17 ` [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible Ard Biesheuvel
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:16 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

Use the memory mapped FV protocol to obtain the existing location in
memory and the size of an image being loaded from a firmware volume.
This removes the need to do a memcopy of the file data.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/DxeMain.h                |   1 +
 MdeModulePkg/Core/Dxe/DxeMain.inf              |   3 +
 MdeModulePkg/Core/Dxe/Image/Image.c            | 111 +++++++++++++++++---
 MdeModulePkg/Include/Protocol/MemoryMappedFv.h |  59 +++++++++++
 MdeModulePkg/MdeModulePkg.dec                  |   3 +
 5 files changed, 163 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 43daa037be441150..a695b457c79b65bb 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/HiiPackageList.h>
 #include <Protocol/SmmBase2.h>
 #include <Protocol/PeCoffImageEmulator.h>
+#include <Protocol/MemoryMappedFv.h>
 #include <Guid/MemoryTypeInformation.h>
 #include <Guid/FirmwareFileSystem2.h>
 #include <Guid/FirmwareFileSystem3.h>
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 35d5bf0dee6f7f3f..a7175cb364b9b5de 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -153,6 +153,9 @@ [Protocols]
   gEfiLoadedImageDevicePathProtocolGuid         ## PRODUCES
   gEfiHiiPackageListProtocolGuid                ## SOMETIMES_PRODUCES
   gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
+  ## PRODUCES
+  ## CONSUMES
+  gEdkiiMemoryMappedFvProtocolGuid
   gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES
 
   # Arch Protocols
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index f30e369370a09609..3dfab4829b3ca17f 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1043,6 +1043,76 @@ CoreUnloadAndCloseImage (
   CoreFreePool (Image);
 }
 
+/**
+  Get the image file data and size directly from a memory mapped FV
+
+  If FilePath is NULL, then NULL is returned.
+  If FileSize is NULL, then NULL is returned.
+  If AuthenticationStatus is NULL, then NULL is returned.
+
+  @param[in]       FvHandle             The firmware volume handle
+  @param[in]       FilePath             The pointer to the device path of the file
+                                        that is abstracted to the file buffer.
+  @param[out]      FileSize             The pointer to the size of the abstracted
+                                        file buffer.
+  @param[out]      AuthenticationStatus Pointer to the authentication status.
+
+  @retval NULL   FilePath is NULL, or FileSize is NULL, or AuthenticationStatus
+                 is NULL, or the file is not memory mapped
+  @retval other  The abstracted file buffer.
+**/
+STATIC
+VOID *
+GetFileFromMemoryMappedFv (
+  IN  EFI_HANDLE                      FvHandle,
+  IN  CONST EFI_DEVICE_PATH_PROTOCOL  *FilePath,
+  OUT UINTN                           *FileSize,
+  OUT UINT32                          *AuthenticationStatus
+  )
+{
+  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *MemMappedFv;
+  CONST EFI_GUID                   *NameGuid;
+  EFI_PHYSICAL_ADDRESS             Address;
+  EFI_STATUS                       Status;
+
+  if ((FilePath == NULL) ||
+      (FileSize == NULL) ||
+      (AuthenticationStatus == NULL))
+  {
+    return NULL;
+  }
+
+  NameGuid = EfiGetNameGuidFromFwVolDevicePathNode (
+               (CONST MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)FilePath);
+  if (NameGuid == NULL) {
+    return NULL;
+  }
+
+  Status = gBS->HandleProtocol (
+                  FvHandle,
+                  &gEdkiiMemoryMappedFvProtocolGuid,
+                  (VOID **)&MemMappedFv
+                  );
+  if (EFI_ERROR (Status)) {
+    ASSERT (Status == EFI_UNSUPPORTED);
+    return NULL;
+  }
+
+  Status = MemMappedFv->GetLocationAndSize (
+                          MemMappedFv,
+                          NameGuid,
+                          EFI_SECTION_PE32,
+                          &Address,
+                          FileSize,
+                          AuthenticationStatus
+                          );
+  if (EFI_ERROR (Status) || (Address > (MAX_ADDRESS - *FileSize))) {
+    return NULL;
+  }
+
+  return (VOID *)(UINTN)Address;
+}
+
 /**
   Loads an EFI image into memory and returns a handle to the image.
 
@@ -1164,6 +1234,16 @@ CoreLoadImageCommon (
     Status = CoreLocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, &HandleFilePath, &DeviceHandle);
     if (!EFI_ERROR (Status)) {
       ImageIsFromFv = TRUE;
+
+      //
+      // If possible, use the memory mapped file image directly, rather than copying it into a buffer
+      //
+      FHand.Source = GetFileFromMemoryMappedFv (
+                       DeviceHandle,
+                       HandleFilePath,
+                       &FHand.SourceSize,
+                       &AuthenticationStatus
+                       );
     } else {
       HandleFilePath = FilePath;
       Status         = CoreLocateDevicePath (&gEfiSimpleFileSystemProtocolGuid, &HandleFilePath, &DeviceHandle);
@@ -1187,21 +1267,24 @@ CoreLoadImageCommon (
     //
     // Get the source file buffer by its device path.
     //
-    FHand.Source = GetFileBufferByFilePath (
-                     BootPolicy,
-                     FilePath,
-                     &FHand.SourceSize,
-                     &AuthenticationStatus
-                     );
     if (FHand.Source == NULL) {
-      Status = EFI_NOT_FOUND;
-    } else {
-      FHand.FreeBuffer = TRUE;
-      if (ImageIsFromLoadFile) {
-        //
-        // LoadFile () may cause the device path of the Handle be updated.
-        //
-        OriginalFilePath = AppendDevicePath (DevicePathFromHandle (DeviceHandle), Node);
+      FHand.Source = GetFileBufferByFilePath (
+                       BootPolicy,
+                       FilePath,
+                       &FHand.SourceSize,
+                       &AuthenticationStatus
+                       );
+
+      if (FHand.Source == NULL) {
+        Status = EFI_NOT_FOUND;
+      } else {
+        FHand.FreeBuffer = TRUE;
+        if (ImageIsFromLoadFile) {
+          //
+          // LoadFile () may cause the device path of the Handle be updated.
+          //
+          OriginalFilePath = AppendDevicePath (DevicePathFromHandle (DeviceHandle), Node);
+        }
       }
     }
   }
diff --git a/MdeModulePkg/Include/Protocol/MemoryMappedFv.h b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
new file mode 100644
index 0000000000000000..821009122113a658
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
@@ -0,0 +1,59 @@
+/** @file
+  Protocol to obtain information about files in memory mapped firmware volumes
+
+  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef EDKII_MEMORY_MAPPED_FV_H_
+#define EDKII_MEMORY_MAPPED_FV_H_
+
+#define EDKII_MEMORY_MAPPED_FV_PROTOCOL_GUID \
+  { 0xb9bfa973, 0x5384, 0x441e, { 0xa4, 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e, 0x0f } }
+
+typedef struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL EDKII_MEMORY_MAPPED_FV_PROTOCOL;
+
+//
+// Function Prototypes
+//
+
+/**
+  Get the physical address and size of a file's section in a memory mapped FV
+
+  @param[in]  This          The protocol pointer
+  @param[in]  NameGuid      The name GUID of the file
+  @param[in]  SectionType   The file section from which to retrieve address and size
+  @param[out] FileAddress   The physical address of the file
+  @param[out] FileSize      The size of the file
+  @param[out] AuthStatus    The authentication status associated with the file
+
+  @retval EFI_SUCCESS            Information about the file was retrieved successfully.
+  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL, AuthStatus
+                                 was NULL.
+  @retval EFI_NOT_FOUND          No section of the specified type could be located in
+                                 the specified file.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *GET_LOCATION_AND_SIZE)(
+  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
+  IN  CONST EFI_GUID                   *NameGuid,
+  IN  EFI_SECTION_TYPE                 SectionType,
+  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
+  OUT UINTN                            *FileSize,
+  OUT UINT32                           *AuthStatus
+  );
+
+//
+// Protocol interface structure
+//
+struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL {
+  GET_LOCATION_AND_SIZE   GetLocationAndSize;
+};
+
+extern EFI_GUID  gEdkiiMemoryMappedFvProtocolGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index d65dae18aa81e569..2d72ac733d82195e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -679,6 +679,9 @@ [Protocols]
   ## Include/Protocol/PlatformBootManager.h
   gEdkiiPlatformBootManagerProtocolGuid = { 0xaa17add4, 0x756c, 0x460d, { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 0x59 } }
 
+  ## Include/Protocol/MemoryMappedFv.h
+  gEdkiiMemoryMappedFvProtocolGuid = { 0xb9bfa973, 0x5384, 0x441e, { 0xa4, 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e, 0x0f } }
+
 #
 # [Error.gEfiMdeModulePkgTokenSpaceGuid]
 #   0x80000001 | Invalid value provided.
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (4 preceding siblings ...)
  2023-05-29 10:16 ` [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy Ard Biesheuvel
@ 2023-05-29 10:17 ` Ard Biesheuvel
  2023-05-30  6:22   ` Ni, Ray
  2023-05-29 10:17 ` [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible Ard Biesheuvel
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:17 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

Expose the EDK2 specific memory mapped FV protocol in addition to the
firmware volume protocol defined by PI when the underlying firmware
volume block protocol informs us that the firmware volume is memory
mapped. This permits the image loader to access any FFS files in the
image directly, rather than requiring it to load a cached copy into
memory via the ReadFile() API. This avoids a redundant memcpy().

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/FwVol/FwVol.c       | 113 +++++++++++++++++++-
 MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h |  31 ++++++
 2 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVol.c b/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
index 153bfecafa7772ea..f7f236496686bc36 100644
--- a/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
+++ b/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
@@ -41,7 +41,8 @@ FV_DEVICE  mFvDevice = {
   0,
   0,
   FALSE,
-  FALSE
+  FALSE,
+  { MemoryMappedFvGetLocationAndSize },
 };
 
 //
@@ -676,11 +677,13 @@ NotifyFwVolBlock (
         //
         // Install an New FV protocol on the existing handle
         //
-        Status = CoreInstallProtocolInterface (
+        Status = CoreInstallMultipleProtocolInterfaces (
                    &Handle,
                    &gEfiFirmwareVolume2ProtocolGuid,
-                   EFI_NATIVE_INTERFACE,
-                   &FvDevice->Fv
+                   &FvDevice->Fv,
+                   (FvDevice->IsMemoryMapped ? &gEdkiiMemoryMappedFvProtocolGuid : NULL),
+                   &FvDevice->MemoryMappedFv,
+                   NULL
                    );
         ASSERT_EFI_ERROR (Status);
       } else {
@@ -722,3 +725,105 @@ FwVolDriverInit (
                           );
   return EFI_SUCCESS;
 }
+
+/**
+  Get the physical address and size of a file's section in a memory mapped FV
+
+  @param[in]  This          The protocol pointer
+  @param[in]  NameGuid      The name GUID of the file
+  @param[in]  SectionType   The file section from which to retrieve address and size
+  @param[out] FileAddress   The physical address of the file
+  @param[out] FileSize      The size of the file
+  @param[out] AuthStatus    The authentication status associated with the file
+
+  @retval EFI_SUCCESS            Information about the file was retrieved successfully.
+  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL, AuthStatus
+                                 was NULL.
+  @retval EFI_NOT_FOUND          No section of the specified type could be located in
+                                 the specified file.
+
+**/
+EFI_STATUS
+EFIAPI
+MemoryMappedFvGetLocationAndSize (
+  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
+  IN  CONST EFI_GUID                   *NameGuid,
+  IN  EFI_SECTION_TYPE                 SectionType,
+  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
+  OUT UINTN                            *FileSize,
+  OUT UINT32                           *AuthStatus
+  )
+{
+  FV_DEVICE                  *FvDevice;
+  EFI_STATUS                 Status;
+  EFI_FV_FILETYPE            FileType;
+  EFI_FV_FILE_ATTRIBUTES     FileAttributes;
+  EFI_GUID                   FileNameGuid;
+  FFS_FILE_LIST_ENTRY        *FfsFileEntry;
+  EFI_COMMON_SECTION_HEADER  *Section;
+  UINTN                      FvFileSize;
+  UINTN                      SectionLength;
+  UINTN                      HeaderLength;
+
+  FvDevice     = FV_DEVICE_FROM_MMFV (This);
+  FfsFileEntry = NULL;
+
+  do {
+    FileType = 0;
+    Status   = FvGetNextFile (
+                 &FvDevice->Fv,
+                 &FfsFileEntry,
+                 &FileType,
+                 &FileNameGuid,
+                 &FileAttributes,
+                 &FvFileSize
+                 );
+    if (EFI_ERROR (Status)) {
+      return EFI_NOT_FOUND;
+    }
+  } while (!CompareGuid (&FileNameGuid, NameGuid));
+
+  //
+  // Skip over file header
+  //
+  if (IS_FFS_FILE2 (FfsFileEntry->FfsHeader)) {
+    Section = (VOID *)((UINT8 *)FfsFileEntry->FfsHeader +
+                       sizeof (EFI_FFS_FILE_HEADER2));
+  } else {
+    Section = (VOID *)((UINT8 *)FfsFileEntry->FfsHeader +
+                       sizeof (EFI_FFS_FILE_HEADER));
+  }
+
+  do {
+    if (IS_SECTION2 (Section)) {
+      SectionLength = SECTION2_SIZE (Section);
+      HeaderLength  = sizeof (EFI_COMMON_SECTION_HEADER2);
+    } else {
+      SectionLength = SECTION_SIZE (Section);
+      HeaderLength  = sizeof (EFI_COMMON_SECTION_HEADER);
+    }
+
+    if (SectionLength > FvFileSize) {
+      DEBUG ((DEBUG_WARN, "%a: %x > %x\n", __func__, SectionLength, FvFileSize));
+      break;
+    }
+
+    if (Section->Type == SectionType) {
+      *FileAddress = (UINTN)Section + HeaderLength;
+      *FileSize    = SectionLength - HeaderLength;
+      *AuthStatus  = FvDevice->AuthenticationStatus;
+
+      return EFI_SUCCESS;
+    }
+
+    //
+    // SectionLength is adjusted it is 4 byte aligned.
+    // Go to the next section
+    //
+    SectionLength = ALIGN_VALUE (SectionLength, 4);
+    Section       = (EFI_COMMON_SECTION_HEADER *)((UINT8 *)Section + SectionLength);
+    FvFileSize   -= SectionLength;
+  } while (TRUE);
+
+  return EFI_NOT_FOUND;
+}
diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h b/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
index 3403c812b2ebcb45..eb1b702c1ee0bd4f 100644
--- a/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
+++ b/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
@@ -40,10 +40,13 @@ typedef struct {
   UINT8                                 ErasePolarity;
   BOOLEAN                               IsFfs3Fv;
   BOOLEAN                               IsMemoryMapped;
+  EDKII_MEMORY_MAPPED_FV_PROTOCOL       MemoryMappedFv;
 } FV_DEVICE;
 
 #define FV_DEVICE_FROM_THIS(a)  CR(a, FV_DEVICE, Fv, FV2_DEVICE_SIGNATURE)
 
+#define FV_DEVICE_FROM_MMFV(a)  CR(a, FV_DEVICE, MemoryMappedFv, FV2_DEVICE_SIGNATURE)
+
 /**
   Retrieves attributes, insures positive polarity of attribute bits, returns
   resulting attributes in output parameter.
@@ -384,4 +387,32 @@ IsValidFfsFile (
   IN EFI_FFS_FILE_HEADER  *FfsHeader
   );
 
+/**
+  Get the physical address and size of a file's section in a memory mapped FV
+
+  @param[in]  This          The protocol pointer
+  @param[in]  NameGuid      The name GUID of the file
+  @param[in]  SectionType   The file section from which to retrieve address and size
+  @param[out] FileAddress   The physical address of the file
+  @param[out] FileSize      The size of the file
+  @param[out] AuthStatus    The authentication status associated with the file
+
+  @retval EFI_SUCCESS            Information about the file was retrieved successfully.
+  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL, AuthStatus
+                                 was NULL.
+  @retval EFI_NOT_FOUND          No section of the specified type could be located in
+                                 the specified file.
+
+**/
+EFI_STATUS
+EFIAPI
+MemoryMappedFvGetLocationAndSize (
+  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
+  IN  CONST EFI_GUID                   *NameGuid,
+  IN  EFI_SECTION_TYPE                 SectionType,
+  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
+  OUT UINTN                            *FileSize,
+  OUT UINT32                           *AuthStatus
+  );
+
 #endif
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (5 preceding siblings ...)
  2023-05-29 10:17 ` [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible Ard Biesheuvel
@ 2023-05-29 10:17 ` Ard Biesheuvel
  2023-05-30  6:32   ` Ni, Ray
  2023-05-29 10:17 ` [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers Ard Biesheuvel
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:17 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

In the image loader, check whether an image has already been relocated
to the address from which it is being loaded. This is not something that
can happen by accident, and so we can assume that this means that the
image was intended to be executed in place.

This removes a redundant copy of the image contents, and also permits
the image to be mapped with restricted permissions even before the CPU
arch protocol has been dispatched.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 3dfab4829b3ca17f..621637e869daf62d 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -573,7 +573,7 @@ STATIC
 EFI_STATUS
 CoreLoadPeImage (
   IN BOOLEAN                    BootPolicy,
-  IN VOID                       *Pe32Handle,
+  IN IMAGE_FILE_HANDLE          *Pe32Handle,
   IN LOADED_IMAGE_PRIVATE_DATA  *Image,
   IN  UINT32                    Attribute
   )
@@ -630,10 +630,16 @@ CoreLoadPeImage (
       return EFI_UNSUPPORTED;
   }
 
+  //
+  // Check whether the loaded image can be executed in place
+  //
+  if (Image->ImageContext.ImageAddress == (PHYSICAL_ADDRESS)(UINTN)Pe32Handle->Source) {
+    goto ExecuteInPlace;
+  }
+
   //
   // Allocate Destination Buffer as caller did not pass it in
   //
-
   if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
     Size = (UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment;
   } else {
@@ -704,6 +710,7 @@ CoreLoadPeImage (
   //
   // Load the image from the file into the allocated memory
   //
+ExecuteInPlace:
   Status = PeCoffLoaderLoadImage (&Image->ImageContext);
   if (EFI_ERROR (Status)) {
     goto Done;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (6 preceding siblings ...)
  2023-05-29 10:17 ` [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible Ard Biesheuvel
@ 2023-05-29 10:17 ` Ard Biesheuvel
  2023-05-30  6:45   ` [edk2-devel] " Ni, Ray
  2023-05-29 10:17 ` [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state Ard Biesheuvel
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:17 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

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


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (7 preceding siblings ...)
  2023-05-29 10:17 ` [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers Ard Biesheuvel
@ 2023-05-29 10:17 ` Ard Biesheuvel
  2023-05-30  6:54   ` Ni, Ray
  2023-05-29 10:17 ` [RFC PATCH 10/11] ArmVirtPkg/ArmVirtQemu: Allow CPU arch protocol DXE to execute in place Ard Biesheuvel
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:17 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

Introduce a new bit in the NX memory protection policy PCD mask that
specifies that the platform enters DXE with all unused and all non-code
regions mapped with non-execute permissions.

This removes the need to do a pass over all memory regions to update
their NX memory attributes.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 +++++++
 MdeModulePkg/MdeModulePkg.dec                 | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 7cc829b17402c2bc..983ed450f143d62d 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -861,6 +861,13 @@ InitializeDxeNxMemoryProtectionPolicy (
     ASSERT (StackBase != 0);
   }
 
+  //
+  // If the platform maps all DRAM non-execute by default, we are done here.
+  //
+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT61) != 0) {
+    return;
+  }
+
   DEBUG ((
     DEBUG_INFO,
     "%a: applying strict permissions to active memory regions\n",
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 2d72ac733d82195e..d2bd0cbb40300889 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1416,12 +1416,15 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   #  EfiMemoryMappedIOPortSpace     0x1000<BR>
   #  EfiPalCode                     0x2000<BR>
   #  EfiPersistentMemory            0x4000<BR>
+  #  Default state      0x2000000000000000<BR>
   #  OEM Reserved       0x4000000000000000<BR>
   #  OS Reserved        0x8000000000000000<BR>
   #
   # NOTE: User must NOT set NX protection for EfiLoaderCode / EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
   #       User MUST set the same NX protection for EfiBootServicesData and EfiConventionalMemory. <BR>
   #
+  # If the platform enters DXE with all unused and non-code regions mapped NX, bit 61 should be set.<BR>
+  #
   # e.g. 0x7FD5 can be used for all memory except Code. <BR>
   # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved. <BR>
   #
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 10/11] ArmVirtPkg/ArmVirtQemu: Allow CPU arch protocol DXE to execute in place
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (8 preceding siblings ...)
  2023-05-29 10:17 ` [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state Ard Biesheuvel
@ 2023-05-29 10:17 ` Ard Biesheuvel
  2023-05-29 10:17 ` [RFC PATCH 11/11] ArmVirtPkg/ArmVirtQemu: Map all DRAM non-execute by default Ard Biesheuvel
  2023-06-01 14:53 ` [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place Oliver Smith-Denny
  11 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:17 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

Add ArmCpuDxe and its dependencies to the APRIORI DXE section, and use a
rule override to emit the executable images in a way that permits them
to execute in place from the firmware volume. This allows them to be
mapped with the appropriate permissions before dispatching the DXE core.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 17 ++++++++++++-----
 ArmVirtPkg/ArmVirtRules.fdf.inc      |  9 +++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 8a063bac04ac287c..24d5c8dd1dc99ca6 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -38,16 +38,23 @@ [FV.FvMain]
 READ_LOCK_CAP      = TRUE
 READ_LOCK_STATUS   = TRUE
 
+  APRIORI DXE {
+    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
+    INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
+    INF EmbeddedPkg/Drivers/FdtClientDxe/FdtClientDxe.inf
+    INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+    INF ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+  }
   INF MdeModulePkg/Core/Dxe/DxeMain.inf
-  INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
+  INF RuleOverride=DXE_XIP MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
   INF OvmfPkg/Fdt/VirtioFdtDxe/VirtioFdtDxe.inf
-  INF EmbeddedPkg/Drivers/FdtClientDxe/FdtClientDxe.inf
+  INF RuleOverride=DXE_XIP EmbeddedPkg/Drivers/FdtClientDxe/FdtClientDxe.inf
   INF OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf
 
   #
   # PI DXE Drivers producing Architectural Protocols (EFI Services)
   #
-  INF ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+  INF RuleOverride=DXE_XIP ArmPkg/Drivers/CpuDxe/CpuDxe.inf
   INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
   INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
   INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
@@ -71,7 +78,7 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
   INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
 
-  INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+  INF RuleOverride=DXE_XIP ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
   INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
   INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
   INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
@@ -107,7 +114,7 @@ [FV.FvMain]
   #
   # Bds
   #
-  INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
+  INF RuleOverride=DXE_XIP MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
   INF MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
diff --git a/ArmVirtPkg/ArmVirtRules.fdf.inc b/ArmVirtPkg/ArmVirtRules.fdf.inc
index b8ec040d2330deb3..0b9acc6d9031d9cf 100644
--- a/ArmVirtPkg/ArmVirtRules.fdf.inc
+++ b/ArmVirtPkg/ArmVirtRules.fdf.inc
@@ -79,6 +79,15 @@ [Rule.Common.DXE_DRIVER]
     RAW          ASL   Optional               |.aml
   }
 
+[Rule.Common.DXE_DRIVER.DXE_XIP]
+  FILE DRIVER = $(NAMED_GUID) {
+    DXE_DEPEX    DXE_DEPEX              Optional $(INF_OUTPUT)/$(MODULE_NAME).depex
+    PE32         PE32 Align = Auto      $(INF_OUTPUT)/$(MODULE_NAME).efi
+    UI           STRING="$(MODULE_NAME)" Optional
+    RAW          ACPI  Optional               |.acpi
+    RAW          ASL   Optional               |.aml
+  }
+
 [Rule.Common.DXE_RUNTIME_DRIVER]
   FILE DRIVER = $(NAMED_GUID) {
     DXE_DEPEX    DXE_DEPEX              Optional $(INF_OUTPUT)/$(MODULE_NAME).depex
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [RFC PATCH 11/11] ArmVirtPkg/ArmVirtQemu: Map all DRAM non-execute by default
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (9 preceding siblings ...)
  2023-05-29 10:17 ` [RFC PATCH 10/11] ArmVirtPkg/ArmVirtQemu: Allow CPU arch protocol DXE to execute in place Ard Biesheuvel
@ 2023-05-29 10:17 ` Ard Biesheuvel
  2023-06-01 14:53 ` [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place Oliver Smith-Denny
  11 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-29 10:17 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

Now that both PEI and DXE can deal with memory being mapped non-execute
by default, update the early mapping code to create non-execute mappings
for all of DRAM. While at it, map the NOR flash read-only as well.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc                                 | 1 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 4 ++--
 ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf                | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 449e73b9e1329111..7d159f27cfea8790 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -151,6 +151,7 @@ [PcdsFeatureFlag.common]
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xE000000000007FD5
 !endif
 
   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 23bd0fe68ef79d98..1ed5815989594ebd 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -91,7 +91,7 @@ ArmVirtGetMemoryMap (
   VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
   VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
   VirtualMemoryTable[0].Length       = *(UINT64 *)GET_GUID_HOB_DATA (MemorySizeHob);
-  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP;
 
   DEBUG ((
     DEBUG_INFO,
@@ -115,7 +115,7 @@ ArmVirtGetMemoryMap (
   VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
   VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
   VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
-  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO;
 
   // End of Table
   ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
diff --git a/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf b/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
index 2039f71a0ebecd5d..6e70bf6eaa245b7a 100644
--- a/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
+++ b/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
@@ -56,4 +56,4 @@ [FixedPcd]
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
 
 [Depex]
-  TRUE
+  gEdkiiMemoryAttributePpiGuid
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage
  2023-05-29 10:16 ` [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage Ard Biesheuvel
@ 2023-05-30  5:54   ` Ni, Ray
  2023-05-30  7:36     ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  5:54 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

I really like the code simplification! Just 2 comments below:

1. Can you describe the calling flow explicitly in commit message:
  CoreLoadImage (public api) -> CoreLoadImageCommon -> CoreLoadPeImage

2. You added "static" to CoreLoadImageCommon but didn't add to CoreLoadPeImage.
I think you are trying to guarantee no lib implementation that silently calls to the
internal functions like CoreLoadImageCommon.
But, why?
Without adding "static", the linking still fails if there is such lib implementation because
of the function prototype change. An error such as "parameter mismatch" would appear.
And if there is a reason for adding "static", why not add that to CoreLoadPeImage as well?

Thanks,
Ray

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused
> 'EntryPoint' argument to LoadImage
> 
> CoreLoadImageCommon's only user passes NULL for its EntryPoint argument,
> so it has no purpose and can simply be dropped. While at it, make
> CoreLoadImageCommon STATIC to prevent it from being accessed from other
> translation units.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Image/Image.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 9dbfb2a1fad22ced..2f2dfe5d0496dc4f 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -560,7 +560,6 @@ CoreIsImageTypeSupported (
>    @param  Pe32Handle              The handle of PE32 image
> 
>    @param  Image                   PE image to be loaded
> 
>    @param  DstBuffer               The buffer to store the image
> 
> -  @param  EntryPoint              A pointer to the entry point
> 
>    @param  Attribute               The bit mask of attributes to set for the load
> 
>                                    PE image
> 
> 
> 
> @@ -577,7 +576,6 @@ CoreLoadPeImage (
>    IN VOID                       *Pe32Handle,
> 
>    IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> 
>    IN EFI_PHYSICAL_ADDRESS       DstBuffer    OPTIONAL,
> 
> -  OUT EFI_PHYSICAL_ADDRESS      *EntryPoint  OPTIONAL,
> 
>    IN  UINT32                    Attribute
> 
>    )
> 
>  {
> 
> @@ -810,13 +808,6 @@ CoreLoadPeImage (
>      }
> 
>    }
> 
> 
> 
> -  //
> 
> -  // Fill in the entry point of the image if it is available
> 
> -  //
> 
> -  if (EntryPoint != NULL) {
> 
> -    *EntryPoint = Image->ImageContext.EntryPoint;
> 
> -  }
> 
> -
> 
>    //
> 
>    // Print the load address and the PDB file name if it is available
> 
>    //
> 
> @@ -1111,7 +1102,6 @@ CoreUnloadAndCloseImage (
>                                    this parameter contains the required number.
> 
>    @param  ImageHandle             Pointer to the returned image handle that is
> 
>                                    created when the image is successfully loaded.
> 
> -  @param  EntryPoint              A pointer to the entry point
> 
>    @param  Attribute               The bit mask of attributes to set for the load
> 
>                                    PE image
> 
> 
> 
> @@ -1134,6 +1124,7 @@ CoreUnloadAndCloseImage (
>                                    platform policy specifies that the image should not be started.
> 
> 
> 
>  **/
> 
> +STATIC
> 
>  EFI_STATUS
> 
>  CoreLoadImageCommon (
> 
>    IN  BOOLEAN                   BootPolicy,
> 
> @@ -1144,7 +1135,6 @@ CoreLoadImageCommon (
>    IN  EFI_PHYSICAL_ADDRESS      DstBuffer           OPTIONAL,
> 
>    IN OUT UINTN                  *NumberOfPages      OPTIONAL,
> 
>    OUT EFI_HANDLE                *ImageHandle,
> 
> -  OUT EFI_PHYSICAL_ADDRESS      *EntryPoint         OPTIONAL,
> 
>    IN  UINT32                    Attribute
> 
>    )
> 
>  {
> 
> @@ -1375,9 +1365,9 @@ CoreLoadImageCommon (
>    }
> 
> 
> 
>    //
> 
> -  // Load the image.  If EntryPoint is Null, it will not be set.
> 
> +  // Load the image.
> 
>    //
> 
> -  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, EntryPoint,
> Attribute);
> 
> +  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, Attribute);
> 
>    if (EFI_ERROR (Status)) {
> 
>      if ((Status == EFI_BUFFER_TOO_SMALL) || (Status ==
> EFI_OUT_OF_RESOURCES)) {
> 
>        if (NumberOfPages != NULL) {
> 
> @@ -1559,7 +1549,6 @@ CoreLoadImage (
>               (EFI_PHYSICAL_ADDRESS)(UINTN)NULL,
> 
>               NULL,
> 
>               ImageHandle,
> 
> -             NULL,
> 
>               EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION |
> EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRATION
> 
>               );
> 
> 
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage
  2023-05-29 10:16 ` [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage Ard Biesheuvel
@ 2023-05-30  5:58   ` Ni, Ray
  0 siblings, 0 replies; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  5:58 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

Reviewed-by: Ray Ni <ray.ni@intel.com>

One comment regarding the needing of "static".
(I saw you added static" to CoreLoadPeImage in this patch😊)
But let's discuss in the first patch thread.

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer
> arg from LoadImage
> 
> The DstBuffer and NumberOfPages arguments to CoreLoadImageCommon () are
> never set by its only caller CoreLoadImage() so let's drop them from the
> prototype.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Image/Image.c | 174 +++++++-------------
>  1 file changed, 56 insertions(+), 118 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 2f2dfe5d0496dc4f..6625d0cd0ff82107 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -559,7 +559,6 @@ CoreIsImageTypeSupported (
>                                    boot selection.
> 
>    @param  Pe32Handle              The handle of PE32 image
> 
>    @param  Image                   PE image to be loaded
> 
> -  @param  DstBuffer               The buffer to store the image
> 
>    @param  Attribute               The bit mask of attributes to set for the load
> 
>                                    PE image
> 
> 
> 
> @@ -570,17 +569,16 @@ CoreIsImageTypeSupported (
>    @retval EFI_BUFFER_TOO_SMALL    Buffer for image is too small
> 
> 
> 
>  **/
> 
> +STATIC
> 
>  EFI_STATUS
> 
>  CoreLoadPeImage (
> 
>    IN BOOLEAN                    BootPolicy,
> 
>    IN VOID                       *Pe32Handle,
> 
>    IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> 
> -  IN EFI_PHYSICAL_ADDRESS       DstBuffer    OPTIONAL,
> 
>    IN  UINT32                    Attribute
> 
>    )
> 
>  {
> 
>    EFI_STATUS  Status;
> 
> -  BOOLEAN     DstBufAlocated;
> 
>    UINTN       Size;
> 
> 
> 
>    ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext));
> 
> @@ -633,99 +631,67 @@ CoreLoadPeImage (
>    }
> 
> 
> 
>    //
> 
> -  // Allocate memory of the correct memory type aligned on the required image
> boundary
> 
> +  // Allocate Destination Buffer as caller did not pass it in
> 
>    //
> 
> -  DstBufAlocated = FALSE;
> 
> -  if (DstBuffer == 0) {
> 
> -    //
> 
> -    // Allocate Destination Buffer as caller did not pass it in
> 
> -    //
> 
> 
> 
> -    if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
> 
> -      Size = (UINTN)Image->ImageContext.ImageSize + Image-
> >ImageContext.SectionAlignment;
> 
> -    } else {
> 
> -      Size = (UINTN)Image->ImageContext.ImageSize;
> 
> -    }
> 
> +  if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
> 
> +    Size = (UINTN)Image->ImageContext.ImageSize + Image-
> >ImageContext.SectionAlignment;
> 
> +  } else {
> 
> +    Size = (UINTN)Image->ImageContext.ImageSize;
> 
> +  }
> 
> 
> 
> -    Image->NumberOfPages = EFI_SIZE_TO_PAGES (Size);
> 
> +  Image->NumberOfPages = EFI_SIZE_TO_PAGES (Size);
> 
> 
> 
> -    //
> 
> -    // If the image relocations have not been stripped, then load at any address.
> 
> -    // Otherwise load at the address at which it was linked.
> 
> -    //
> 
> -    // Memory below 1MB should be treated reserved for CSM and there should
> be
> 
> -    // no modules whose preferred load addresses are below 1MB.
> 
> -    //
> 
> -    Status = EFI_OUT_OF_RESOURCES;
> 
> -    //
> 
> -    // If Loading Module At Fixed Address feature is enabled, the module should be
> loaded to
> 
> -    // a specified address.
> 
> -    //
> 
> -    if (PcdGet64 (PcdLoadModuleAtFixAddressEnable) != 0 ) {
> 
> -      Status = GetPeCoffImageFixLoadingAssignedAddress (&(Image-
> >ImageContext));
> 
> -
> 
> -      if (EFI_ERROR (Status)) {
> 
> -        //
> 
> -        // If the code memory is not ready, invoke CoreAllocatePage with
> AllocateAnyPages to load the driver.
> 
> -        //
> 
> -        DEBUG ((DEBUG_INFO|DEBUG_LOAD, "LOADING MODULE FIXED ERROR:
> Loading module at fixed address failed since specified memory is not
> available.\n"));
> 
> -
> 
> -        Status = CoreAllocatePages (
> 
> -                   AllocateAnyPages,
> 
> -                   (EFI_MEMORY_TYPE)(Image-
> >ImageContext.ImageCodeMemoryType),
> 
> -                   Image->NumberOfPages,
> 
> -                   &Image->ImageContext.ImageAddress
> 
> -                   );
> 
> -      }
> 
> -    } else {
> 
> -      if ((Image->ImageContext.ImageAddress >= 0x100000) || Image-
> >ImageContext.RelocationsStripped) {
> 
> -        Status = CoreAllocatePages (
> 
> -                   AllocateAddress,
> 
> -                   (EFI_MEMORY_TYPE)(Image-
> >ImageContext.ImageCodeMemoryType),
> 
> -                   Image->NumberOfPages,
> 
> -                   &Image->ImageContext.ImageAddress
> 
> -                   );
> 
> -      }
> 
> -
> 
> -      if (EFI_ERROR (Status) && !Image->ImageContext.RelocationsStripped) {
> 
> -        Status = CoreAllocatePages (
> 
> -                   AllocateAnyPages,
> 
> -                   (EFI_MEMORY_TYPE)(Image-
> >ImageContext.ImageCodeMemoryType),
> 
> -                   Image->NumberOfPages,
> 
> -                   &Image->ImageContext.ImageAddress
> 
> -                   );
> 
> -      }
> 
> -    }
> 
> +  //
> 
> +  // If the image relocations have not been stripped, then load at any address.
> 
> +  // Otherwise load at the address at which it was linked.
> 
> +  //
> 
> +  // Memory below 1MB should be treated reserved for CSM and there should be
> 
> +  // no modules whose preferred load addresses are below 1MB.
> 
> +  //
> 
> +  Status = EFI_OUT_OF_RESOURCES;
> 
> +  //
> 
> +  // If Loading Module At Fixed Address feature is enabled, the module should be
> loaded to
> 
> +  // a specified address.
> 
> +  //
> 
> +  if (PcdGet64 (PcdLoadModuleAtFixAddressEnable) != 0 ) {
> 
> +    Status = GetPeCoffImageFixLoadingAssignedAddress (&(Image-
> >ImageContext));
> 
> 
> 
>      if (EFI_ERROR (Status)) {
> 
> -      return Status;
> 
> -    }
> 
> +      //
> 
> +      // If the code memory is not ready, invoke CoreAllocatePage with
> AllocateAnyPages to load the driver.
> 
> +      //
> 
> +      DEBUG ((DEBUG_INFO|DEBUG_LOAD, "LOADING MODULE FIXED ERROR:
> Loading module at fixed address failed since specified memory is not
> available.\n"));
> 
> 
> 
> -    DstBufAlocated = TRUE;
> 
> +      Status = CoreAllocatePages (
> 
> +                 AllocateAnyPages,
> 
> +                 (EFI_MEMORY_TYPE)(Image-
> >ImageContext.ImageCodeMemoryType),
> 
> +                 Image->NumberOfPages,
> 
> +                 &Image->ImageContext.ImageAddress
> 
> +                 );
> 
> +    }
> 
>    } else {
> 
> -    //
> 
> -    // Caller provided the destination buffer
> 
> -    //
> 
> -
> 
> -    if (Image->ImageContext.RelocationsStripped && (Image-
> >ImageContext.ImageAddress != DstBuffer)) {
> 
> -      //
> 
> -      // If the image relocations were stripped, and the caller provided a
> 
> -      // destination buffer address that does not match the address that the
> 
> -      // image is linked at, then the image cannot be loaded.
> 
> -      //
> 
> -      return EFI_INVALID_PARAMETER;
> 
> +    if ((Image->ImageContext.ImageAddress >= 0x100000) || Image-
> >ImageContext.RelocationsStripped) {
> 
> +      Status = CoreAllocatePages (
> 
> +                 AllocateAddress,
> 
> +                 (EFI_MEMORY_TYPE)(Image-
> >ImageContext.ImageCodeMemoryType),
> 
> +                 Image->NumberOfPages,
> 
> +                 &Image->ImageContext.ImageAddress
> 
> +                 );
> 
>      }
> 
> 
> 
> -    if ((Image->NumberOfPages != 0) &&
> 
> -        (Image->NumberOfPages <
> 
> -         (EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize + Image-
> >ImageContext.SectionAlignment))))
> 
> -    {
> 
> -      Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image-
> >ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
> 
> -      return EFI_BUFFER_TOO_SMALL;
> 
> +    if (EFI_ERROR (Status) && !Image->ImageContext.RelocationsStripped) {
> 
> +      Status = CoreAllocatePages (
> 
> +                 AllocateAnyPages,
> 
> +                 (EFI_MEMORY_TYPE)(Image-
> >ImageContext.ImageCodeMemoryType),
> 
> +                 Image->NumberOfPages,
> 
> +                 &Image->ImageContext.ImageAddress
> 
> +                 );
> 
>      }
> 
> +  }
> 
> 
> 
> -    Image->NumberOfPages             = EFI_SIZE_TO_PAGES ((UINTN)Image-
> >ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
> 
> -    Image->ImageContext.ImageAddress = DstBuffer;
> 
> +  if (EFI_ERROR (Status)) {
> 
> +    return Status;
> 
>    }
> 
> 
> 
>    Image->ImageBasePage = Image->ImageContext.ImageAddress;
> 
> @@ -875,12 +841,9 @@ CoreLoadPeImage (
>    //
> 
>    // Free memory.
> 
>    //
> 
> -
> 
> -  if (DstBufAlocated) {
> 
> -    CoreFreePages (Image->ImageContext.ImageAddress, Image-
> >NumberOfPages);
> 
> -    Image->ImageContext.ImageAddress = 0;
> 
> -    Image->ImageBasePage             = 0;
> 
> -  }
> 
> +  CoreFreePages (Image->ImageContext.ImageAddress, Image-
> >NumberOfPages);
> 
> +  Image->ImageContext.ImageAddress = 0;
> 
> +  Image->ImageBasePage             = 0;
> 
> 
> 
>    if (Image->ImageContext.FixupData != NULL) {
> 
>      CoreFreePool (Image->ImageContext.FixupData);
> 
> @@ -1094,12 +1057,6 @@ CoreUnloadAndCloseImage (
>    @param  SourceBuffer            If not NULL, a pointer to the memory location
> 
>                                    containing a copy of the image to be loaded.
> 
>    @param  SourceSize              The size in bytes of SourceBuffer.
> 
> -  @param  DstBuffer               The buffer to store the image
> 
> -  @param  NumberOfPages           If not NULL, it inputs a pointer to the page
> 
> -                                  number of DstBuffer and outputs a pointer to
> 
> -                                  the page number of the image. If this number is
> 
> -                                  not enough,  return EFI_BUFFER_TOO_SMALL and
> 
> -                                  this parameter contains the required number.
> 
>    @param  ImageHandle             Pointer to the returned image handle that is
> 
>                                    created when the image is successfully loaded.
> 
>    @param  Attribute               The bit mask of attributes to set for the load
> 
> @@ -1132,8 +1089,6 @@ CoreLoadImageCommon (
>    IN  EFI_DEVICE_PATH_PROTOCOL  *FilePath,
> 
>    IN  VOID                      *SourceBuffer       OPTIONAL,
> 
>    IN  UINTN                     SourceSize,
> 
> -  IN  EFI_PHYSICAL_ADDRESS      DstBuffer           OPTIONAL,
> 
> -  IN OUT UINTN                  *NumberOfPages      OPTIONAL,
> 
>    OUT EFI_HANDLE                *ImageHandle,
> 
>    IN  UINT32                    Attribute
> 
>    )
> 
> @@ -1342,12 +1297,7 @@ CoreLoadImageCommon (
>    Image->Info.Revision     = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> 
>    Image->Info.FilePath     = DuplicateDevicePath (FilePath);
> 
>    Image->Info.ParentHandle = ParentImageHandle;
> 
> -
> 
> -  if (NumberOfPages != NULL) {
> 
> -    Image->NumberOfPages = *NumberOfPages;
> 
> -  } else {
> 
> -    Image->NumberOfPages = 0;
> 
> -  }
> 
> +  Image->NumberOfPages     = 0;
> 
> 
> 
>    //
> 
>    // Install the protocol interfaces for this image
> 
> @@ -1367,21 +1317,11 @@ CoreLoadImageCommon (
>    //
> 
>    // Load the image.
> 
>    //
> 
> -  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, Attribute);
> 
> +  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, Attribute);
> 
>    if (EFI_ERROR (Status)) {
> 
> -    if ((Status == EFI_BUFFER_TOO_SMALL) || (Status ==
> EFI_OUT_OF_RESOURCES)) {
> 
> -      if (NumberOfPages != NULL) {
> 
> -        *NumberOfPages = Image->NumberOfPages;
> 
> -      }
> 
> -    }
> 
> -
> 
>      goto Done;
> 
>    }
> 
> 
> 
> -  if (NumberOfPages != NULL) {
> 
> -    *NumberOfPages = Image->NumberOfPages;
> 
> -  }
> 
> -
> 
>    //
> 
>    // Register the image in the Debug Image Info Table if the attribute is set
> 
>    //
> 
> @@ -1473,7 +1413,7 @@ CoreLoadImageCommon (
>    //
> 
>    if (EFI_ERROR (Status)) {
> 
>      if (Image != NULL) {
> 
> -      CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0));
> 
> +      CoreUnloadAndCloseImage (Image, TRUE);
> 
>        Image = NULL;
> 
>      }
> 
>    } else if (EFI_ERROR (SecurityStatus)) {
> 
> @@ -1546,8 +1486,6 @@ CoreLoadImage (
>               FilePath,
> 
>               SourceBuffer,
> 
>               SourceSize,
> 
> -             (EFI_PHYSICAL_ADDRESS)(UINTN)NULL,
> 
> -             NULL,
> 
>               ImageHandle,
> 
>               EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION |
> EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRATION
> 
>               );
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage
  2023-05-29 10:16 ` [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage Ard Biesheuvel
@ 2023-05-30  5:59   ` Ni, Ray
  0 siblings, 0 replies; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  5:59 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage
> argument from CoreUnloadImage
> 
> The FreePage argument to CoreUnloadAndCloseImage () is now always TRUE
> so drop it from the prototype. While at it, make the function static as
> it is never called from another translation unit.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Image/Image.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 6625d0cd0ff82107..f30e369370a09609 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -888,13 +888,12 @@ CoreLoadedImageInfo (
>    Unloads EFI image from memory.
> 
> 
> 
>    @param  Image                   EFI image
> 
> -  @param  FreePage                Free allocated pages
> 
> 
> 
>  **/
> 
> +STATIC
> 
>  VOID
> 
>  CoreUnloadAndCloseImage (
> 
> -  IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> 
> -  IN BOOLEAN                    FreePage
> 
> +  IN LOADED_IMAGE_PRIVATE_DATA  *Image
> 
>    )
> 
>  {
> 
>    EFI_STATUS                           Status;
> 
> @@ -1022,7 +1021,7 @@ CoreUnloadAndCloseImage (
>    //
> 
>    // Free the Image from memory
> 
>    //
> 
> -  if ((Image->ImageBasePage != 0) && FreePage) {
> 
> +  if (Image->ImageBasePage != 0) {
> 
>      CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
> 
>    }
> 
> 
> 
> @@ -1413,7 +1412,7 @@ CoreLoadImageCommon (
>    //
> 
>    if (EFI_ERROR (Status)) {
> 
>      if (Image != NULL) {
> 
> -      CoreUnloadAndCloseImage (Image, TRUE);
> 
> +      CoreUnloadAndCloseImage (Image);
> 
>        Image = NULL;
> 
>      }
> 
>    } else if (EFI_ERROR (SecurityStatus)) {
> 
> @@ -1711,7 +1710,7 @@ CoreStartImage (
>    // unload it
> 
>    //
> 
>    if (EFI_ERROR (Image->Status) || (Image->Type ==
> EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION)) {
> 
> -    CoreUnloadAndCloseImage (Image, TRUE);
> 
> +    CoreUnloadAndCloseImage (Image);
> 
>      //
> 
>      // ImageHandle may be invalid after the image is unloaded, so use NULL
> handle to record perf log.
> 
>      //
> 
> @@ -1776,7 +1775,7 @@ CoreExit (
>      //
> 
>      // The image has not been started so just free its resources
> 
>      //
> 
> -    CoreUnloadAndCloseImage (Image, TRUE);
> 
> +    CoreUnloadAndCloseImage (Image);
> 
>      Status = EFI_SUCCESS;
> 
>      goto Done;
> 
>    }
> 
> @@ -1874,7 +1873,7 @@ CoreUnloadImage (
>      //
> 
>      // if the Image was not started or Unloaded O.K. then clean up
> 
>      //
> 
> -    CoreUnloadAndCloseImage (Image, TRUE);
> 
> +    CoreUnloadAndCloseImage (Image);
> 
>    }
> 
> 
> 
>  Done:
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files
  2023-05-29 10:16 ` [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files Ard Biesheuvel
@ 2023-05-30  6:03   ` Ni, Ray
  2023-05-30  7:47     ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  6:03 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io, Gao, Liming,
	Kinney, Michael D
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Leif Lindholm, Michael Kubacki

I don't know why the FFS file is cached. Without knowing the reason of caching, I cannot say it's good to remove the caching logic.
I will let @Gao, Liming, @Kinney, Michael D to comment.


> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory
> mapped FFS files
> 
> If a firmware volume is memory mapped, it means we can access it
> contents directly, and so caching serves little purpose beyond
> potentially a minor performance improvement. However, given that most
> files are read only a single time, and dispatched from a decompressed
> firmware volume in DRAM, we can just avoid the redundant caching here.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c | 22 --------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> index 2ff22c93aad48d7e..69df96685d680868 100644
> --- a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> +++ b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> @@ -284,7 +284,6 @@ FvReadFile (
>    UINT8                   *SrcPtr;
> 
>    EFI_FFS_FILE_HEADER     *FfsHeader;
> 
>    UINTN                   InputBufferSize;
> 
> -  UINTN                   WholeFileSize;
> 
> 
> 
>    if (NameGuid == NULL) {
> 
>      return EFI_INVALID_PARAMETER;
> 
> @@ -316,27 +315,6 @@ FvReadFile (
>    // Get a pointer to the header
> 
>    //
> 
>    FfsHeader = FvDevice->LastKey->FfsHeader;
> 
> -  if (FvDevice->IsMemoryMapped) {
> 
> -    //
> 
> -    // Memory mapped FV has not been cached, so here is to cache by file.
> 
> -    //
> 
> -    if (!FvDevice->LastKey->FileCached) {
> 
> -      //
> 
> -      // Cache FFS file to memory buffer.
> 
> -      //
> 
> -      WholeFileSize = IS_FFS_FILE2 (FfsHeader) ? FFS_FILE2_SIZE (FfsHeader) :
> FFS_FILE_SIZE (FfsHeader);
> 
> -      FfsHeader     = AllocateCopyPool (WholeFileSize, FfsHeader);
> 
> -      if (FfsHeader == NULL) {
> 
> -        return EFI_OUT_OF_RESOURCES;
> 
> -      }
> 
> -
> 
> -      //
> 
> -      // Let FfsHeader in FfsFileEntry point to the cached file buffer.
> 
> -      //
> 
> -      FvDevice->LastKey->FfsHeader  = FfsHeader;
> 
> -      FvDevice->LastKey->FileCached = TRUE;
> 
> -    }
> 
> -  }
> 
> 
> 
>    //
> 
>    // Remember callers buffer size
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy
  2023-05-29 10:16 ` [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy Ard Biesheuvel
@ 2023-05-30  6:21   ` Ni, Ray
  2023-05-30  7:51     ` [edk2-devel] " Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  6:21 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

GetFileBufferByFilePath() always returns a copy of file buffer even when the file
is in a memory-mapped device.
So, your patch adds a new implementation (abstracted through the new MM FV protocol) that can directly return the file location in the MMIO device.

Several comments:
1. I am not sure if any negative impact due to this change. For example: old logic reads the MMIO device but doesn't execute in the MMIO device. Does MMIO device always support execution in place?
2. If the MMFV protocol is only produced by DxeCore and consumed by DxeCore, can we just implement a local function instead? The challenge might be how to pass the FV_DEVICE instance to the local function. Can we "handle" the "FirmwareVolume2" protocol from the Handle first and use CR_FROM_THIS() macro to retrieve the FV_DEVICE pointer? This helps to not add the MMFV protocol, letting the change a pure DxeCore internal thing.


Thanks,
Ray

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV
> protocol to avoid image copy
> 
> Use the memory mapped FV protocol to obtain the existing location in
> memory and the size of an image being loaded from a firmware volume.
> This removes the need to do a memcopy of the file data.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h                |   1 +
>  MdeModulePkg/Core/Dxe/DxeMain.inf              |   3 +
>  MdeModulePkg/Core/Dxe/Image/Image.c            | 111 +++++++++++++++++---
>  MdeModulePkg/Include/Protocol/MemoryMappedFv.h |  59 +++++++++++
>  MdeModulePkg/MdeModulePkg.dec                  |   3 +
>  5 files changed, 163 insertions(+), 14 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 43daa037be441150..a695b457c79b65bb 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Protocol/HiiPackageList.h>
> 
>  #include <Protocol/SmmBase2.h>
> 
>  #include <Protocol/PeCoffImageEmulator.h>
> 
> +#include <Protocol/MemoryMappedFv.h>
> 
>  #include <Guid/MemoryTypeInformation.h>
> 
>  #include <Guid/FirmwareFileSystem2.h>
> 
>  #include <Guid/FirmwareFileSystem3.h>
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 35d5bf0dee6f7f3f..a7175cb364b9b5de 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -153,6 +153,9 @@ [Protocols]
>    gEfiLoadedImageDevicePathProtocolGuid         ## PRODUCES
> 
>    gEfiHiiPackageListProtocolGuid                ## SOMETIMES_PRODUCES
> 
>    gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
> 
> +  ## PRODUCES
> 
> +  ## CONSUMES
> 
> +  gEdkiiMemoryMappedFvProtocolGuid
> 
>    gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES
> 
> 
> 
>    # Arch Protocols
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index f30e369370a09609..3dfab4829b3ca17f 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -1043,6 +1043,76 @@ CoreUnloadAndCloseImage (
>    CoreFreePool (Image);
> 
>  }
> 
> 
> 
> +/**
> 
> +  Get the image file data and size directly from a memory mapped FV
> 
> +
> 
> +  If FilePath is NULL, then NULL is returned.
> 
> +  If FileSize is NULL, then NULL is returned.
> 
> +  If AuthenticationStatus is NULL, then NULL is returned.
> 
> +
> 
> +  @param[in]       FvHandle             The firmware volume handle
> 
> +  @param[in]       FilePath             The pointer to the device path of the file
> 
> +                                        that is abstracted to the file buffer.
> 
> +  @param[out]      FileSize             The pointer to the size of the abstracted
> 
> +                                        file buffer.
> 
> +  @param[out]      AuthenticationStatus Pointer to the authentication status.
> 
> +
> 
> +  @retval NULL   FilePath is NULL, or FileSize is NULL, or AuthenticationStatus
> 
> +                 is NULL, or the file is not memory mapped
> 
> +  @retval other  The abstracted file buffer.
> 
> +**/
> 
> +STATIC
> 
> +VOID *
> 
> +GetFileFromMemoryMappedFv (
> 
> +  IN  EFI_HANDLE                      FvHandle,
> 
> +  IN  CONST EFI_DEVICE_PATH_PROTOCOL  *FilePath,
> 
> +  OUT UINTN                           *FileSize,
> 
> +  OUT UINT32                          *AuthenticationStatus
> 
> +  )
> 
> +{
> 
> +  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *MemMappedFv;
> 
> +  CONST EFI_GUID                   *NameGuid;
> 
> +  EFI_PHYSICAL_ADDRESS             Address;
> 
> +  EFI_STATUS                       Status;
> 
> +
> 
> +  if ((FilePath == NULL) ||
> 
> +      (FileSize == NULL) ||
> 
> +      (AuthenticationStatus == NULL))
> 
> +  {
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
> +  NameGuid = EfiGetNameGuidFromFwVolDevicePathNode (
> 
> +               (CONST MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)FilePath);
> 
> +  if (NameGuid == NULL) {
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
> +  Status = gBS->HandleProtocol (
> 
> +                  FvHandle,
> 
> +                  &gEdkiiMemoryMappedFvProtocolGuid,
> 
> +                  (VOID **)&MemMappedFv
> 
> +                  );
> 
> +  if (EFI_ERROR (Status)) {
> 
> +    ASSERT (Status == EFI_UNSUPPORTED);
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
> +  Status = MemMappedFv->GetLocationAndSize (
> 
> +                          MemMappedFv,
> 
> +                          NameGuid,
> 
> +                          EFI_SECTION_PE32,
> 
> +                          &Address,
> 
> +                          FileSize,
> 
> +                          AuthenticationStatus
> 
> +                          );
> 
> +  if (EFI_ERROR (Status) || (Address > (MAX_ADDRESS - *FileSize))) {
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
> +  return (VOID *)(UINTN)Address;
> 
> +}
> 
> +
> 
>  /**
> 
>    Loads an EFI image into memory and returns a handle to the image.
> 
> 
> 
> @@ -1164,6 +1234,16 @@ CoreLoadImageCommon (
>      Status = CoreLocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid,
> &HandleFilePath, &DeviceHandle);
> 
>      if (!EFI_ERROR (Status)) {
> 
>        ImageIsFromFv = TRUE;
> 
> +
> 
> +      //
> 
> +      // If possible, use the memory mapped file image directly, rather than
> copying it into a buffer
> 
> +      //
> 
> +      FHand.Source = GetFileFromMemoryMappedFv (
> 
> +                       DeviceHandle,
> 
> +                       HandleFilePath,
> 
> +                       &FHand.SourceSize,
> 
> +                       &AuthenticationStatus
> 
> +                       );
> 
>      } else {
> 
>        HandleFilePath = FilePath;
> 
>        Status         = CoreLocateDevicePath (&gEfiSimpleFileSystemProtocolGuid,
> &HandleFilePath, &DeviceHandle);
> 
> @@ -1187,21 +1267,24 @@ CoreLoadImageCommon (
>      //
> 
>      // Get the source file buffer by its device path.
> 
>      //
> 
> -    FHand.Source = GetFileBufferByFilePath (
> 
> -                     BootPolicy,
> 
> -                     FilePath,
> 
> -                     &FHand.SourceSize,
> 
> -                     &AuthenticationStatus
> 
> -                     );
> 
>      if (FHand.Source == NULL) {
> 
> -      Status = EFI_NOT_FOUND;
> 
> -    } else {
> 
> -      FHand.FreeBuffer = TRUE;
> 
> -      if (ImageIsFromLoadFile) {
> 
> -        //
> 
> -        // LoadFile () may cause the device path of the Handle be updated.
> 
> -        //
> 
> -        OriginalFilePath = AppendDevicePath (DevicePathFromHandle
> (DeviceHandle), Node);
> 
> +      FHand.Source = GetFileBufferByFilePath (
> 
> +                       BootPolicy,
> 
> +                       FilePath,
> 
> +                       &FHand.SourceSize,
> 
> +                       &AuthenticationStatus
> 
> +                       );
> 
> +
> 
> +      if (FHand.Source == NULL) {
> 
> +        Status = EFI_NOT_FOUND;
> 
> +      } else {
> 
> +        FHand.FreeBuffer = TRUE;
> 
> +        if (ImageIsFromLoadFile) {
> 
> +          //
> 
> +          // LoadFile () may cause the device path of the Handle be updated.
> 
> +          //
> 
> +          OriginalFilePath = AppendDevicePath (DevicePathFromHandle
> (DeviceHandle), Node);
> 
> +        }
> 
>        }
> 
>      }
> 
>    }
> 
> diff --git a/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> new file mode 100644
> index 0000000000000000..821009122113a658
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> @@ -0,0 +1,59 @@
> +/** @file
> 
> +  Protocol to obtain information about files in memory mapped firmware
> volumes
> 
> +
> 
> +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> 
> +
> 
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> +
> 
> +**/
> 
> +
> 
> +#ifndef EDKII_MEMORY_MAPPED_FV_H_
> 
> +#define EDKII_MEMORY_MAPPED_FV_H_
> 
> +
> 
> +#define EDKII_MEMORY_MAPPED_FV_PROTOCOL_GUID \
> 
> +  { 0xb9bfa973, 0x5384, 0x441e, { 0xa4, 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e,
> 0x0f } }
> 
> +
> 
> +typedef struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL
> EDKII_MEMORY_MAPPED_FV_PROTOCOL;
> 
> +
> 
> +//
> 
> +// Function Prototypes
> 
> +//
> 
> +
> 
> +/**
> 
> +  Get the physical address and size of a file's section in a memory mapped FV
> 
> +
> 
> +  @param[in]  This          The protocol pointer
> 
> +  @param[in]  NameGuid      The name GUID of the file
> 
> +  @param[in]  SectionType   The file section from which to retrieve address and
> size
> 
> +  @param[out] FileAddress   The physical address of the file
> 
> +  @param[out] FileSize      The size of the file
> 
> +  @param[out] AuthStatus    The authentication status associated with the file
> 
> +
> 
> +  @retval EFI_SUCCESS            Information about the file was retrieved
> successfully.
> 
> +  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL,
> AuthStatus
> 
> +                                 was NULL.
> 
> +  @retval EFI_NOT_FOUND          No section of the specified type could be
> located in
> 
> +                                 the specified file.
> 
> +
> 
> +**/
> 
> +typedef
> 
> +EFI_STATUS
> 
> +(EFIAPI *GET_LOCATION_AND_SIZE)(
> 
> +  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
> 
> +  IN  CONST EFI_GUID                   *NameGuid,
> 
> +  IN  EFI_SECTION_TYPE                 SectionType,
> 
> +  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
> 
> +  OUT UINTN                            *FileSize,
> 
> +  OUT UINT32                           *AuthStatus
> 
> +  );
> 
> +
> 
> +//
> 
> +// Protocol interface structure
> 
> +//
> 
> +struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL {
> 
> +  GET_LOCATION_AND_SIZE   GetLocationAndSize;
> 
> +};
> 
> +
> 
> +extern EFI_GUID  gEdkiiMemoryMappedFvProtocolGuid;
> 
> +
> 
> +#endif
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index d65dae18aa81e569..2d72ac733d82195e 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -679,6 +679,9 @@ [Protocols]
>    ## Include/Protocol/PlatformBootManager.h
> 
>    gEdkiiPlatformBootManagerProtocolGuid = { 0xaa17add4, 0x756c, 0x460d,
> { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 0x59 } }
> 
> 
> 
> +  ## Include/Protocol/MemoryMappedFv.h
> 
> +  gEdkiiMemoryMappedFvProtocolGuid = { 0xb9bfa973, 0x5384, 0x441e, { 0xa4,
> 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e, 0x0f } }
> 
> +
> 
>  #
> 
>  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> 
>  #   0x80000001 | Invalid value provided.
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible
  2023-05-29 10:17 ` [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible Ard Biesheuvel
@ 2023-05-30  6:22   ` Ni, Ray
  0 siblings, 0 replies; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  6:22 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

As I replied to patch #5, we may avoid installing the MMFV protocol.

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped
> FV protocol when possible
> 
> Expose the EDK2 specific memory mapped FV protocol in addition to the
> firmware volume protocol defined by PI when the underlying firmware
> volume block protocol informs us that the firmware volume is memory
> mapped. This permits the image loader to access any FFS files in the
> image directly, rather than requiring it to load a cached copy into
> memory via the ReadFile() API. This avoids a redundant memcpy().
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/FwVol/FwVol.c       | 113 +++++++++++++++++++-
>  MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h |  31 ++++++
>  2 files changed, 140 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
> b/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
> index 153bfecafa7772ea..f7f236496686bc36 100644
> --- a/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
> +++ b/MdeModulePkg/Core/Dxe/FwVol/FwVol.c
> @@ -41,7 +41,8 @@ FV_DEVICE  mFvDevice = {
>    0,
> 
>    0,
> 
>    FALSE,
> 
> -  FALSE
> 
> +  FALSE,
> 
> +  { MemoryMappedFvGetLocationAndSize },
> 
>  };
> 
> 
> 
>  //
> 
> @@ -676,11 +677,13 @@ NotifyFwVolBlock (
>          //
> 
>          // Install an New FV protocol on the existing handle
> 
>          //
> 
> -        Status = CoreInstallProtocolInterface (
> 
> +        Status = CoreInstallMultipleProtocolInterfaces (
> 
>                     &Handle,
> 
>                     &gEfiFirmwareVolume2ProtocolGuid,
> 
> -                   EFI_NATIVE_INTERFACE,
> 
> -                   &FvDevice->Fv
> 
> +                   &FvDevice->Fv,
> 
> +                   (FvDevice->IsMemoryMapped ?
> &gEdkiiMemoryMappedFvProtocolGuid : NULL),
> 
> +                   &FvDevice->MemoryMappedFv,
> 
> +                   NULL
> 
>                     );
> 
>          ASSERT_EFI_ERROR (Status);
> 
>        } else {
> 
> @@ -722,3 +725,105 @@ FwVolDriverInit (
>                            );
> 
>    return EFI_SUCCESS;
> 
>  }
> 
> +
> 
> +/**
> 
> +  Get the physical address and size of a file's section in a memory mapped FV
> 
> +
> 
> +  @param[in]  This          The protocol pointer
> 
> +  @param[in]  NameGuid      The name GUID of the file
> 
> +  @param[in]  SectionType   The file section from which to retrieve address and
> size
> 
> +  @param[out] FileAddress   The physical address of the file
> 
> +  @param[out] FileSize      The size of the file
> 
> +  @param[out] AuthStatus    The authentication status associated with the file
> 
> +
> 
> +  @retval EFI_SUCCESS            Information about the file was retrieved
> successfully.
> 
> +  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL,
> AuthStatus
> 
> +                                 was NULL.
> 
> +  @retval EFI_NOT_FOUND          No section of the specified type could be
> located in
> 
> +                                 the specified file.
> 
> +
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +MemoryMappedFvGetLocationAndSize (
> 
> +  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
> 
> +  IN  CONST EFI_GUID                   *NameGuid,
> 
> +  IN  EFI_SECTION_TYPE                 SectionType,
> 
> +  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
> 
> +  OUT UINTN                            *FileSize,
> 
> +  OUT UINT32                           *AuthStatus
> 
> +  )
> 
> +{
> 
> +  FV_DEVICE                  *FvDevice;
> 
> +  EFI_STATUS                 Status;
> 
> +  EFI_FV_FILETYPE            FileType;
> 
> +  EFI_FV_FILE_ATTRIBUTES     FileAttributes;
> 
> +  EFI_GUID                   FileNameGuid;
> 
> +  FFS_FILE_LIST_ENTRY        *FfsFileEntry;
> 
> +  EFI_COMMON_SECTION_HEADER  *Section;
> 
> +  UINTN                      FvFileSize;
> 
> +  UINTN                      SectionLength;
> 
> +  UINTN                      HeaderLength;
> 
> +
> 
> +  FvDevice     = FV_DEVICE_FROM_MMFV (This);
> 
> +  FfsFileEntry = NULL;
> 
> +
> 
> +  do {
> 
> +    FileType = 0;
> 
> +    Status   = FvGetNextFile (
> 
> +                 &FvDevice->Fv,
> 
> +                 &FfsFileEntry,
> 
> +                 &FileType,
> 
> +                 &FileNameGuid,
> 
> +                 &FileAttributes,
> 
> +                 &FvFileSize
> 
> +                 );
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      return EFI_NOT_FOUND;
> 
> +    }
> 
> +  } while (!CompareGuid (&FileNameGuid, NameGuid));
> 
> +
> 
> +  //
> 
> +  // Skip over file header
> 
> +  //
> 
> +  if (IS_FFS_FILE2 (FfsFileEntry->FfsHeader)) {
> 
> +    Section = (VOID *)((UINT8 *)FfsFileEntry->FfsHeader +
> 
> +                       sizeof (EFI_FFS_FILE_HEADER2));
> 
> +  } else {
> 
> +    Section = (VOID *)((UINT8 *)FfsFileEntry->FfsHeader +
> 
> +                       sizeof (EFI_FFS_FILE_HEADER));
> 
> +  }
> 
> +
> 
> +  do {
> 
> +    if (IS_SECTION2 (Section)) {
> 
> +      SectionLength = SECTION2_SIZE (Section);
> 
> +      HeaderLength  = sizeof (EFI_COMMON_SECTION_HEADER2);
> 
> +    } else {
> 
> +      SectionLength = SECTION_SIZE (Section);
> 
> +      HeaderLength  = sizeof (EFI_COMMON_SECTION_HEADER);
> 
> +    }
> 
> +
> 
> +    if (SectionLength > FvFileSize) {
> 
> +      DEBUG ((DEBUG_WARN, "%a: %x > %x\n", __func__, SectionLength,
> FvFileSize));
> 
> +      break;
> 
> +    }
> 
> +
> 
> +    if (Section->Type == SectionType) {
> 
> +      *FileAddress = (UINTN)Section + HeaderLength;
> 
> +      *FileSize    = SectionLength - HeaderLength;
> 
> +      *AuthStatus  = FvDevice->AuthenticationStatus;
> 
> +
> 
> +      return EFI_SUCCESS;
> 
> +    }
> 
> +
> 
> +    //
> 
> +    // SectionLength is adjusted it is 4 byte aligned.
> 
> +    // Go to the next section
> 
> +    //
> 
> +    SectionLength = ALIGN_VALUE (SectionLength, 4);
> 
> +    Section       = (EFI_COMMON_SECTION_HEADER *)((UINT8 *)Section +
> SectionLength);
> 
> +    FvFileSize   -= SectionLength;
> 
> +  } while (TRUE);
> 
> +
> 
> +  return EFI_NOT_FOUND;
> 
> +}
> 
> diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
> b/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
> index 3403c812b2ebcb45..eb1b702c1ee0bd4f 100644
> --- a/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
> +++ b/MdeModulePkg/Core/Dxe/FwVol/FwVolDriver.h
> @@ -40,10 +40,13 @@ typedef struct {
>    UINT8                                 ErasePolarity;
> 
>    BOOLEAN                               IsFfs3Fv;
> 
>    BOOLEAN                               IsMemoryMapped;
> 
> +  EDKII_MEMORY_MAPPED_FV_PROTOCOL       MemoryMappedFv;
> 
>  } FV_DEVICE;
> 
> 
> 
>  #define FV_DEVICE_FROM_THIS(a)  CR(a, FV_DEVICE, Fv,
> FV2_DEVICE_SIGNATURE)
> 
> 
> 
> +#define FV_DEVICE_FROM_MMFV(a)  CR(a, FV_DEVICE, MemoryMappedFv,
> FV2_DEVICE_SIGNATURE)
> 
> +
> 
>  /**
> 
>    Retrieves attributes, insures positive polarity of attribute bits, returns
> 
>    resulting attributes in output parameter.
> 
> @@ -384,4 +387,32 @@ IsValidFfsFile (
>    IN EFI_FFS_FILE_HEADER  *FfsHeader
> 
>    );
> 
> 
> 
> +/**
> 
> +  Get the physical address and size of a file's section in a memory mapped FV
> 
> +
> 
> +  @param[in]  This          The protocol pointer
> 
> +  @param[in]  NameGuid      The name GUID of the file
> 
> +  @param[in]  SectionType   The file section from which to retrieve address and
> size
> 
> +  @param[out] FileAddress   The physical address of the file
> 
> +  @param[out] FileSize      The size of the file
> 
> +  @param[out] AuthStatus    The authentication status associated with the file
> 
> +
> 
> +  @retval EFI_SUCCESS            Information about the file was retrieved
> successfully.
> 
> +  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL,
> AuthStatus
> 
> +                                 was NULL.
> 
> +  @retval EFI_NOT_FOUND          No section of the specified type could be
> located in
> 
> +                                 the specified file.
> 
> +
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +MemoryMappedFvGetLocationAndSize (
> 
> +  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
> 
> +  IN  CONST EFI_GUID                   *NameGuid,
> 
> +  IN  EFI_SECTION_TYPE                 SectionType,
> 
> +  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
> 
> +  OUT UINTN                            *FileSize,
> 
> +  OUT UINT32                           *AuthStatus
> 
> +  );
> 
> +
> 
>  #endif
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible
  2023-05-29 10:17 ` [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible Ard Biesheuvel
@ 2023-05-30  6:32   ` Ni, Ray
  2023-05-30  7:54     ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  6:32 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

I didn't review the existing logic carefully. Several comments:
1. Assignments of several fields are skipped when executing in place. Do they matter?
    a). Image->NumberOfPages
    b). Image->ImageBasePage

2. PeCoffLoaderRelocateImage() is called even for XIP case. But I don't think it's expected that relocation really happens. Even if the fixed data is the same as the original data stored in MMIO device, MMIO writing might cause unexpected behavior.

3.  CoreFreePages() is called when image is not loaded successfully. Is it expected for XIP case?

Thanks,
Ray

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in
> place if possible
> 
> In the image loader, check whether an image has already been relocated
> to the address from which it is being loaded. This is not something that
> can happen by accident, and so we can assume that this means that the
> image was intended to be executed in place.
> 
> This removes a redundant copy of the image contents, and also permits
> the image to be mapped with restricted permissions even before the CPU
> arch protocol has been dispatched.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 3dfab4829b3ca17f..621637e869daf62d 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -573,7 +573,7 @@ STATIC
>  EFI_STATUS
> 
>  CoreLoadPeImage (
> 
>    IN BOOLEAN                    BootPolicy,
> 
> -  IN VOID                       *Pe32Handle,
> 
> +  IN IMAGE_FILE_HANDLE          *Pe32Handle,
> 
>    IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> 
>    IN  UINT32                    Attribute
> 
>    )
> 
> @@ -630,10 +630,16 @@ CoreLoadPeImage (
>        return EFI_UNSUPPORTED;
> 
>    }
> 
> 
> 
> +  //
> 
> +  // Check whether the loaded image can be executed in place
> 
> +  //
> 
> +  if (Image->ImageContext.ImageAddress ==
> (PHYSICAL_ADDRESS)(UINTN)Pe32Handle->Source) {
> 
> +    goto ExecuteInPlace;
> 
> +  }
> 
> +
> 
>    //
> 
>    // Allocate Destination Buffer as caller did not pass it in
> 
>    //
> 
> -
> 
>    if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
> 
>      Size = (UINTN)Image->ImageContext.ImageSize + Image-
> >ImageContext.SectionAlignment;
> 
>    } else {
> 
> @@ -704,6 +710,7 @@ CoreLoadPeImage (
>    //
> 
>    // Load the image from the file into the allocated memory
> 
>    //
> 
> +ExecuteInPlace:
> 
>    Status = PeCoffLoaderLoadImage (&Image->ImageContext);
> 
>    if (EFI_ERROR (Status)) {
> 
>      goto Done;
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-29 10:17 ` [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers Ard Biesheuvel
@ 2023-05-30  6:45   ` Ni, Ray
  2023-05-30  7:58     ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  6:45 UTC (permalink / raw)
  To: devel@edk2.groups.io, ardb@kernel.org
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

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]
> -=-=-=-=-=-=
> 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state
  2023-05-29 10:17 ` [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state Ard Biesheuvel
@ 2023-05-30  6:54   ` Ni, Ray
  2023-05-30  8:33     ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  6:54 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

1. Do we want to catch a case that platform wrongly sets BIT61 but drivers run before CpuDxe are not XIP?
2. Why BIT61 set is the "Default state"?

The setting of BIT61 is a bit confusing. Is there a way to avoid adding BIT61 through code optimization?

Thanks,
Ray

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> 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: [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for
> default NX state
> 
> Introduce a new bit in the NX memory protection policy PCD mask that
> specifies that the platform enters DXE with all unused and all non-code
> regions mapped with non-execute permissions.
> 
> This removes the need to do a pass over all memory regions to update
> their NX memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 +++++++
>  MdeModulePkg/MdeModulePkg.dec                 | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 7cc829b17402c2bc..983ed450f143d62d 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -861,6 +861,13 @@ InitializeDxeNxMemoryProtectionPolicy (
>      ASSERT (StackBase != 0);
> 
>    }
> 
> 
> 
> +  //
> 
> +  // If the platform maps all DRAM non-execute by default, we are done here.
> 
> +  //
> 
> +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT61) != 0) {
> 
> +    return;
> 
> +  }
> 
> +
> 
>    DEBUG ((
> 
>      DEBUG_INFO,
> 
>      "%a: applying strict permissions to active memory regions\n",
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index 2d72ac733d82195e..d2bd0cbb40300889 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1416,12 +1416,15 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    #  EfiMemoryMappedIOPortSpace     0x1000<BR>
> 
>    #  EfiPalCode                     0x2000<BR>
> 
>    #  EfiPersistentMemory            0x4000<BR>
> 
> +  #  Default state      0x2000000000000000<BR>
> 
>    #  OEM Reserved       0x4000000000000000<BR>
> 
>    #  OS Reserved        0x8000000000000000<BR>
> 
>    #
> 
>    # NOTE: User must NOT set NX protection for EfiLoaderCode /
> EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
> 
>    #       User MUST set the same NX protection for EfiBootServicesData and
> EfiConventionalMemory. <BR>
> 
>    #
> 
> +  # If the platform enters DXE with all unused and non-code regions mapped NX,
> bit 61 should be set.<BR>
> 
> +  #
> 
>    # e.g. 0x7FD5 can be used for all memory except Code. <BR>
> 
>    # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved.
> <BR>
> 
>    #
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage
  2023-05-30  5:54   ` Ni, Ray
@ 2023-05-30  7:36     ` Ard Biesheuvel
  0 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  7:36 UTC (permalink / raw)
  To: Ni, Ray
  Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On Tue, 30 May 2023 at 07:54, Ni, Ray <ray.ni@intel.com> wrote:
>
> I really like the code simplification! Just 2 comments below:
>
> 1. Can you describe the calling flow explicitly in commit message:
>   CoreLoadImage (public api) -> CoreLoadImageCommon -> CoreLoadPeImage
>

Ok

> 2. You added "static" to CoreLoadImageCommon but didn't add to CoreLoadPeImage.
> I think you are trying to guarantee no lib implementation that silently calls to the
> internal functions like CoreLoadImageCommon.
> But, why?
> Without adding "static", the linking still fails if there is such lib implementation because
> of the function prototype change. An error such as "parameter mismatch" would appear.

This function is not declared in a header, so any existing user would
carry its own EXTERN declaration.
So the compiler would not notice, and the linker does not verify
protoype compatibility, so we would only notice this at runtime.

I am not aware of any downstream platforms that do such a thing, so
perhaps I am being overly cautious. But generally, functions should be
STATIC unless there is a need for them to have external linkage, and
changing the prototype is a reasonable justification IMHO for adding
it at this point.

> And if there is a reason for adding "static", why not add that to CoreLoadPeImage as well?
>

This is added in the next patch.

>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 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: [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused
> > 'EntryPoint' argument to LoadImage
> >
> > CoreLoadImageCommon's only user passes NULL for its EntryPoint argument,
> > so it has no purpose and can simply be dropped. While at it, make
> > CoreLoadImageCommon STATIC to prevent it from being accessed from other
> > translation units.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/Image/Image.c | 17 +++--------------
> >  1 file changed, 3 insertions(+), 14 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 9dbfb2a1fad22ced..2f2dfe5d0496dc4f 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -560,7 +560,6 @@ CoreIsImageTypeSupported (
> >    @param  Pe32Handle              The handle of PE32 image
> >
> >    @param  Image                   PE image to be loaded
> >
> >    @param  DstBuffer               The buffer to store the image
> >
> > -  @param  EntryPoint              A pointer to the entry point
> >
> >    @param  Attribute               The bit mask of attributes to set for the load
> >
> >                                    PE image
> >
> >
> >
> > @@ -577,7 +576,6 @@ CoreLoadPeImage (
> >    IN VOID                       *Pe32Handle,
> >
> >    IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> >
> >    IN EFI_PHYSICAL_ADDRESS       DstBuffer    OPTIONAL,
> >
> > -  OUT EFI_PHYSICAL_ADDRESS      *EntryPoint  OPTIONAL,
> >
> >    IN  UINT32                    Attribute
> >
> >    )
> >
> >  {
> >
> > @@ -810,13 +808,6 @@ CoreLoadPeImage (
> >      }
> >
> >    }
> >
> >
> >
> > -  //
> >
> > -  // Fill in the entry point of the image if it is available
> >
> > -  //
> >
> > -  if (EntryPoint != NULL) {
> >
> > -    *EntryPoint = Image->ImageContext.EntryPoint;
> >
> > -  }
> >
> > -
> >
> >    //
> >
> >    // Print the load address and the PDB file name if it is available
> >
> >    //
> >
> > @@ -1111,7 +1102,6 @@ CoreUnloadAndCloseImage (
> >                                    this parameter contains the required number.
> >
> >    @param  ImageHandle             Pointer to the returned image handle that is
> >
> >                                    created when the image is successfully loaded.
> >
> > -  @param  EntryPoint              A pointer to the entry point
> >
> >    @param  Attribute               The bit mask of attributes to set for the load
> >
> >                                    PE image
> >
> >
> >
> > @@ -1134,6 +1124,7 @@ CoreUnloadAndCloseImage (
> >                                    platform policy specifies that the image should not be started.
> >
> >
> >
> >  **/
> >
> > +STATIC
> >
> >  EFI_STATUS
> >
> >  CoreLoadImageCommon (
> >
> >    IN  BOOLEAN                   BootPolicy,
> >
> > @@ -1144,7 +1135,6 @@ CoreLoadImageCommon (
> >    IN  EFI_PHYSICAL_ADDRESS      DstBuffer           OPTIONAL,
> >
> >    IN OUT UINTN                  *NumberOfPages      OPTIONAL,
> >
> >    OUT EFI_HANDLE                *ImageHandle,
> >
> > -  OUT EFI_PHYSICAL_ADDRESS      *EntryPoint         OPTIONAL,
> >
> >    IN  UINT32                    Attribute
> >
> >    )
> >
> >  {
> >
> > @@ -1375,9 +1365,9 @@ CoreLoadImageCommon (
> >    }
> >
> >
> >
> >    //
> >
> > -  // Load the image.  If EntryPoint is Null, it will not be set.
> >
> > +  // Load the image.
> >
> >    //
> >
> > -  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, EntryPoint,
> > Attribute);
> >
> > +  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, Attribute);
> >
> >    if (EFI_ERROR (Status)) {
> >
> >      if ((Status == EFI_BUFFER_TOO_SMALL) || (Status ==
> > EFI_OUT_OF_RESOURCES)) {
> >
> >        if (NumberOfPages != NULL) {
> >
> > @@ -1559,7 +1549,6 @@ CoreLoadImage (
> >               (EFI_PHYSICAL_ADDRESS)(UINTN)NULL,
> >
> >               NULL,
> >
> >               ImageHandle,
> >
> > -             NULL,
> >
> >               EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION |
> > EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRATION
> >
> >               );
> >
> >
> >
> > --
> > 2.39.2
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files
  2023-05-30  6:03   ` Ni, Ray
@ 2023-05-30  7:47     ` Ard Biesheuvel
  0 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  7:47 UTC (permalink / raw)
  To: Ni, Ray
  Cc: devel@edk2.groups.io, Gao, Liming, Kinney, Michael D, Yao, Jiewen,
	Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny, Bi, Dandan,
	Leif Lindholm, Michael Kubacki

On Tue, 30 May 2023 at 08:03, Ni, Ray <ray.ni@intel.com> wrote:
>
> I don't know why the FFS file is cached. Without knowing the reason of caching, I cannot say it's good to remove the caching logic.
> I will let @Gao, Liming, @Kinney, Michael D to comment.
>

Fair enought.

I found this commit by Start

commit eb1cace292ff0c66ca11eff4703c9fa16219c2a1
Author: Star Zeng <star.zeng@intel.com>
Date:   Wed Aug 27 08:31:44 2014 +0000

    MdeModulePkg DxeCore: Don't cache memory mapped IO FV.

which changes the caching logic to cache FFS files individually
instead of caching the entire firmware volume if it is memory mapped.

AIUI, firmware volumes could be mapped uncached, so if caching is
still needed in some cases, perhaps we might try to disambiguate these
cases based on the underlying memory region? I.e., if it is system
memory, no caching is used.

The decompressed FV does not need caching on any architecture or
platform, afaict.


>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 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: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory
> > mapped FFS files
> >
> > If a firmware volume is memory mapped, it means we can access it
> > contents directly, and so caching serves little purpose beyond
> > potentially a minor performance improvement. However, given that most
> > files are read only a single time, and dispatched from a decompressed
> > firmware volume in DRAM, we can just avoid the redundant caching here.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c | 22 --------------------
> >  1 file changed, 22 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > index 2ff22c93aad48d7e..69df96685d680868 100644
> > --- a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > +++ b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > @@ -284,7 +284,6 @@ FvReadFile (
> >    UINT8                   *SrcPtr;
> >
> >    EFI_FFS_FILE_HEADER     *FfsHeader;
> >
> >    UINTN                   InputBufferSize;
> >
> > -  UINTN                   WholeFileSize;
> >
> >
> >
> >    if (NameGuid == NULL) {
> >
> >      return EFI_INVALID_PARAMETER;
> >
> > @@ -316,27 +315,6 @@ FvReadFile (
> >    // Get a pointer to the header
> >
> >    //
> >
> >    FfsHeader = FvDevice->LastKey->FfsHeader;
> >
> > -  if (FvDevice->IsMemoryMapped) {
> >
> > -    //
> >
> > -    // Memory mapped FV has not been cached, so here is to cache by file.
> >
> > -    //
> >
> > -    if (!FvDevice->LastKey->FileCached) {
> >
> > -      //
> >
> > -      // Cache FFS file to memory buffer.
> >
> > -      //
> >
> > -      WholeFileSize = IS_FFS_FILE2 (FfsHeader) ? FFS_FILE2_SIZE (FfsHeader) :
> > FFS_FILE_SIZE (FfsHeader);
> >
> > -      FfsHeader     = AllocateCopyPool (WholeFileSize, FfsHeader);
> >
> > -      if (FfsHeader == NULL) {
> >
> > -        return EFI_OUT_OF_RESOURCES;
> >
> > -      }
> >
> > -
> >
> > -      //
> >
> > -      // Let FfsHeader in FfsFileEntry point to the cached file buffer.
> >
> > -      //
> >
> > -      FvDevice->LastKey->FfsHeader  = FfsHeader;
> >
> > -      FvDevice->LastKey->FileCached = TRUE;
> >
> > -    }
> >
> > -  }
> >
> >
> >
> >    //
> >
> >    // Remember callers buffer size
> >
> > --
> > 2.39.2
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy
  2023-05-30  6:21   ` Ni, Ray
@ 2023-05-30  7:51     ` Ard Biesheuvel
  2023-05-30  8:40       ` Ni, Ray
  0 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  7:51 UTC (permalink / raw)
  To: devel, ray.ni
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

On Tue, 30 May 2023 at 08:21, Ni, Ray <ray.ni@intel.com> wrote:
>
> GetFileBufferByFilePath() always returns a copy of file buffer even when the file
> is in a memory-mapped device.
> So, your patch adds a new implementation (abstracted through the new MM FV protocol) that can directly return the file location in the MMIO device.
>
> Several comments:
> 1. I am not sure if any negative impact due to this change. For example: old logic reads the MMIO device but doesn't execute in the MMIO device. Does MMIO device always support execution in place?

At this point, we are not executing anything in place. The buffer is
only used by the PE/COFF loader to access the file contents, but it
still creates the sections in memory as before, and copies the data
into them.

This is similar to how gBS->Loadimage() with a buffer/size only uses
the buffer contents to access the file data, it does not execute the
image from the buffer.

> 2. If the MMFV protocol is only produced by DxeCore and consumed by DxeCore, can we just implement a local function instead? The challenge might be how to pass the FV_DEVICE instance to the local function. Can we "handle" the "FirmwareVolume2" protocol from the Handle first and use CR_FROM_THIS() macro to retrieve the FV_DEVICE pointer? This helps to not add the MMFV protocol, letting the change a pure DxeCore internal thing.
>

The loader does not know whether the FirmwareVolume2 protocol was
produced by DXE core or by some other component, so we cannot assume
that CR_FROM_THIS() is usable.



> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 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: [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV
> > protocol to avoid image copy
> >
> > Use the memory mapped FV protocol to obtain the existing location in
> > memory and the size of an image being loaded from a firmware volume.
> > This removes the need to do a memcopy of the file data.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/DxeMain.h                |   1 +
> >  MdeModulePkg/Core/Dxe/DxeMain.inf              |   3 +
> >  MdeModulePkg/Core/Dxe/Image/Image.c            | 111 +++++++++++++++++---
> >  MdeModulePkg/Include/Protocol/MemoryMappedFv.h |  59 +++++++++++
> >  MdeModulePkg/MdeModulePkg.dec                  |   3 +
> >  5 files changed, 163 insertions(+), 14 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > b/MdeModulePkg/Core/Dxe/DxeMain.h
> > index 43daa037be441150..a695b457c79b65bb 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #include <Protocol/HiiPackageList.h>
> >
> >  #include <Protocol/SmmBase2.h>
> >
> >  #include <Protocol/PeCoffImageEmulator.h>
> >
> > +#include <Protocol/MemoryMappedFv.h>
> >
> >  #include <Guid/MemoryTypeInformation.h>
> >
> >  #include <Guid/FirmwareFileSystem2.h>
> >
> >  #include <Guid/FirmwareFileSystem3.h>
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 35d5bf0dee6f7f3f..a7175cb364b9b5de 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -153,6 +153,9 @@ [Protocols]
> >    gEfiLoadedImageDevicePathProtocolGuid         ## PRODUCES
> >
> >    gEfiHiiPackageListProtocolGuid                ## SOMETIMES_PRODUCES
> >
> >    gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
> >
> > +  ## PRODUCES
> >
> > +  ## CONSUMES
> >
> > +  gEdkiiMemoryMappedFvProtocolGuid
> >
> >    gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES
> >
> >
> >
> >    # Arch Protocols
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index f30e369370a09609..3dfab4829b3ca17f 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -1043,6 +1043,76 @@ CoreUnloadAndCloseImage (
> >    CoreFreePool (Image);
> >
> >  }
> >
> >
> >
> > +/**
> >
> > +  Get the image file data and size directly from a memory mapped FV
> >
> > +
> >
> > +  If FilePath is NULL, then NULL is returned.
> >
> > +  If FileSize is NULL, then NULL is returned.
> >
> > +  If AuthenticationStatus is NULL, then NULL is returned.
> >
> > +
> >
> > +  @param[in]       FvHandle             The firmware volume handle
> >
> > +  @param[in]       FilePath             The pointer to the device path of the file
> >
> > +                                        that is abstracted to the file buffer.
> >
> > +  @param[out]      FileSize             The pointer to the size of the abstracted
> >
> > +                                        file buffer.
> >
> > +  @param[out]      AuthenticationStatus Pointer to the authentication status.
> >
> > +
> >
> > +  @retval NULL   FilePath is NULL, or FileSize is NULL, or AuthenticationStatus
> >
> > +                 is NULL, or the file is not memory mapped
> >
> > +  @retval other  The abstracted file buffer.
> >
> > +**/
> >
> > +STATIC
> >
> > +VOID *
> >
> > +GetFileFromMemoryMappedFv (
> >
> > +  IN  EFI_HANDLE                      FvHandle,
> >
> > +  IN  CONST EFI_DEVICE_PATH_PROTOCOL  *FilePath,
> >
> > +  OUT UINTN                           *FileSize,
> >
> > +  OUT UINT32                          *AuthenticationStatus
> >
> > +  )
> >
> > +{
> >
> > +  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *MemMappedFv;
> >
> > +  CONST EFI_GUID                   *NameGuid;
> >
> > +  EFI_PHYSICAL_ADDRESS             Address;
> >
> > +  EFI_STATUS                       Status;
> >
> > +
> >
> > +  if ((FilePath == NULL) ||
> >
> > +      (FileSize == NULL) ||
> >
> > +      (AuthenticationStatus == NULL))
> >
> > +  {
> >
> > +    return NULL;
> >
> > +  }
> >
> > +
> >
> > +  NameGuid = EfiGetNameGuidFromFwVolDevicePathNode (
> >
> > +               (CONST MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)FilePath);
> >
> > +  if (NameGuid == NULL) {
> >
> > +    return NULL;
> >
> > +  }
> >
> > +
> >
> > +  Status = gBS->HandleProtocol (
> >
> > +                  FvHandle,
> >
> > +                  &gEdkiiMemoryMappedFvProtocolGuid,
> >
> > +                  (VOID **)&MemMappedFv
> >
> > +                  );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    ASSERT (Status == EFI_UNSUPPORTED);
> >
> > +    return NULL;
> >
> > +  }
> >
> > +
> >
> > +  Status = MemMappedFv->GetLocationAndSize (
> >
> > +                          MemMappedFv,
> >
> > +                          NameGuid,
> >
> > +                          EFI_SECTION_PE32,
> >
> > +                          &Address,
> >
> > +                          FileSize,
> >
> > +                          AuthenticationStatus
> >
> > +                          );
> >
> > +  if (EFI_ERROR (Status) || (Address > (MAX_ADDRESS - *FileSize))) {
> >
> > +    return NULL;
> >
> > +  }
> >
> > +
> >
> > +  return (VOID *)(UINTN)Address;
> >
> > +}
> >
> > +
> >
> >  /**
> >
> >    Loads an EFI image into memory and returns a handle to the image.
> >
> >
> >
> > @@ -1164,6 +1234,16 @@ CoreLoadImageCommon (
> >      Status = CoreLocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid,
> > &HandleFilePath, &DeviceHandle);
> >
> >      if (!EFI_ERROR (Status)) {
> >
> >        ImageIsFromFv = TRUE;
> >
> > +
> >
> > +      //
> >
> > +      // If possible, use the memory mapped file image directly, rather than
> > copying it into a buffer
> >
> > +      //
> >
> > +      FHand.Source = GetFileFromMemoryMappedFv (
> >
> > +                       DeviceHandle,
> >
> > +                       HandleFilePath,
> >
> > +                       &FHand.SourceSize,
> >
> > +                       &AuthenticationStatus
> >
> > +                       );
> >
> >      } else {
> >
> >        HandleFilePath = FilePath;
> >
> >        Status         = CoreLocateDevicePath (&gEfiSimpleFileSystemProtocolGuid,
> > &HandleFilePath, &DeviceHandle);
> >
> > @@ -1187,21 +1267,24 @@ CoreLoadImageCommon (
> >      //
> >
> >      // Get the source file buffer by its device path.
> >
> >      //
> >
> > -    FHand.Source = GetFileBufferByFilePath (
> >
> > -                     BootPolicy,
> >
> > -                     FilePath,
> >
> > -                     &FHand.SourceSize,
> >
> > -                     &AuthenticationStatus
> >
> > -                     );
> >
> >      if (FHand.Source == NULL) {
> >
> > -      Status = EFI_NOT_FOUND;
> >
> > -    } else {
> >
> > -      FHand.FreeBuffer = TRUE;
> >
> > -      if (ImageIsFromLoadFile) {
> >
> > -        //
> >
> > -        // LoadFile () may cause the device path of the Handle be updated.
> >
> > -        //
> >
> > -        OriginalFilePath = AppendDevicePath (DevicePathFromHandle
> > (DeviceHandle), Node);
> >
> > +      FHand.Source = GetFileBufferByFilePath (
> >
> > +                       BootPolicy,
> >
> > +                       FilePath,
> >
> > +                       &FHand.SourceSize,
> >
> > +                       &AuthenticationStatus
> >
> > +                       );
> >
> > +
> >
> > +      if (FHand.Source == NULL) {
> >
> > +        Status = EFI_NOT_FOUND;
> >
> > +      } else {
> >
> > +        FHand.FreeBuffer = TRUE;
> >
> > +        if (ImageIsFromLoadFile) {
> >
> > +          //
> >
> > +          // LoadFile () may cause the device path of the Handle be updated.
> >
> > +          //
> >
> > +          OriginalFilePath = AppendDevicePath (DevicePathFromHandle
> > (DeviceHandle), Node);
> >
> > +        }
> >
> >        }
> >
> >      }
> >
> >    }
> >
> > diff --git a/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> > b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> > new file mode 100644
> > index 0000000000000000..821009122113a658
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> > @@ -0,0 +1,59 @@
> > +/** @file
> >
> > +  Protocol to obtain information about files in memory mapped firmware
> > volumes
> >
> > +
> >
> > +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> >
> > +
> >
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +
> >
> > +**/
> >
> > +
> >
> > +#ifndef EDKII_MEMORY_MAPPED_FV_H_
> >
> > +#define EDKII_MEMORY_MAPPED_FV_H_
> >
> > +
> >
> > +#define EDKII_MEMORY_MAPPED_FV_PROTOCOL_GUID \
> >
> > +  { 0xb9bfa973, 0x5384, 0x441e, { 0xa4, 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e,
> > 0x0f } }
> >
> > +
> >
> > +typedef struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL
> > EDKII_MEMORY_MAPPED_FV_PROTOCOL;
> >
> > +
> >
> > +//
> >
> > +// Function Prototypes
> >
> > +//
> >
> > +
> >
> > +/**
> >
> > +  Get the physical address and size of a file's section in a memory mapped FV
> >
> > +
> >
> > +  @param[in]  This          The protocol pointer
> >
> > +  @param[in]  NameGuid      The name GUID of the file
> >
> > +  @param[in]  SectionType   The file section from which to retrieve address and
> > size
> >
> > +  @param[out] FileAddress   The physical address of the file
> >
> > +  @param[out] FileSize      The size of the file
> >
> > +  @param[out] AuthStatus    The authentication status associated with the file
> >
> > +
> >
> > +  @retval EFI_SUCCESS            Information about the file was retrieved
> > successfully.
> >
> > +  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was NULL,
> > AuthStatus
> >
> > +                                 was NULL.
> >
> > +  @retval EFI_NOT_FOUND          No section of the specified type could be
> > located in
> >
> > +                                 the specified file.
> >
> > +
> >
> > +**/
> >
> > +typedef
> >
> > +EFI_STATUS
> >
> > +(EFIAPI *GET_LOCATION_AND_SIZE)(
> >
> > +  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
> >
> > +  IN  CONST EFI_GUID                   *NameGuid,
> >
> > +  IN  EFI_SECTION_TYPE                 SectionType,
> >
> > +  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
> >
> > +  OUT UINTN                            *FileSize,
> >
> > +  OUT UINT32                           *AuthStatus
> >
> > +  );
> >
> > +
> >
> > +//
> >
> > +// Protocol interface structure
> >
> > +//
> >
> > +struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL {
> >
> > +  GET_LOCATION_AND_SIZE   GetLocationAndSize;
> >
> > +};
> >
> > +
> >
> > +extern EFI_GUID  gEdkiiMemoryMappedFvProtocolGuid;
> >
> > +
> >
> > +#endif
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > index d65dae18aa81e569..2d72ac733d82195e 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -679,6 +679,9 @@ [Protocols]
> >    ## Include/Protocol/PlatformBootManager.h
> >
> >    gEdkiiPlatformBootManagerProtocolGuid = { 0xaa17add4, 0x756c, 0x460d,
> > { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 0x59 } }
> >
> >
> >
> > +  ## Include/Protocol/MemoryMappedFv.h
> >
> > +  gEdkiiMemoryMappedFvProtocolGuid = { 0xb9bfa973, 0x5384, 0x441e, { 0xa4,
> > 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e, 0x0f } }
> >
> > +
> >
> >  #
> >
> >  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> >
> >  #   0x80000001 | Invalid value provided.
> >
> > --
> > 2.39.2
>
>
>
> 
>
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible
  2023-05-30  6:32   ` Ni, Ray
@ 2023-05-30  7:54     ` Ard Biesheuvel
  0 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  7:54 UTC (permalink / raw)
  To: Ni, Ray
  Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On Tue, 30 May 2023 at 08:32, Ni, Ray <ray.ni@intel.com> wrote:
>
> I didn't review the existing logic carefully. Several comments:
> 1. Assignments of several fields are skipped when executing in place. Do they matter?
>     a). Image->NumberOfPages
>     b). Image->ImageBasePage
>

These are used when freeing the image again, so NumberOfPages should
remain 0x0 in this case, so that unloading the image does not free any
pages.

> 2. PeCoffLoaderRelocateImage() is called even for XIP case. But I don't think it's expected that relocation really happens. Even if the fixed data is the same as the original data stored in MMIO device, MMIO writing might cause unexpected behavior.
>

PeCoffLoaderRelocateImage() currently calculates the adjustment
offset, and does nothing to the image if it is 0x0.

> 3.  CoreFreePages() is called when image is not loaded successfully. Is it expected for XIP case?
>

Yes, but Image->NumberOfPages will be 0x0.

>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 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: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in
> > place if possible
> >
> > In the image loader, check whether an image has already been relocated
> > to the address from which it is being loaded. This is not something that
> > can happen by accident, and so we can assume that this means that the
> > image was intended to be executed in place.
> >
> > This removes a redundant copy of the image contents, and also permits
> > the image to be mapped with restricted permissions even before the CPU
> > arch protocol has been dispatched.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 3dfab4829b3ca17f..621637e869daf62d 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -573,7 +573,7 @@ STATIC
> >  EFI_STATUS
> >
> >  CoreLoadPeImage (
> >
> >    IN BOOLEAN                    BootPolicy,
> >
> > -  IN VOID                       *Pe32Handle,
> >
> > +  IN IMAGE_FILE_HANDLE          *Pe32Handle,
> >
> >    IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> >
> >    IN  UINT32                    Attribute
> >
> >    )
> >
> > @@ -630,10 +630,16 @@ CoreLoadPeImage (
> >        return EFI_UNSUPPORTED;
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // Check whether the loaded image can be executed in place
> >
> > +  //
> >
> > +  if (Image->ImageContext.ImageAddress ==
> > (PHYSICAL_ADDRESS)(UINTN)Pe32Handle->Source) {
> >
> > +    goto ExecuteInPlace;
> >
> > +  }
> >
> > +
> >
> >    //
> >
> >    // Allocate Destination Buffer as caller did not pass it in
> >
> >    //
> >
> > -
> >
> >    if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
> >
> >      Size = (UINTN)Image->ImageContext.ImageSize + Image-
> > >ImageContext.SectionAlignment;
> >
> >    } else {
> >
> > @@ -704,6 +710,7 @@ CoreLoadPeImage (
> >    //
> >
> >    // Load the image from the file into the allocated memory
> >
> >    //
> >
> > +ExecuteInPlace:
> >
> >    Status = PeCoffLoaderLoadImage (&Image->ImageContext);
> >
> >    if (EFI_ERROR (Status)) {
> >
> >      goto Done;
> >
> > --
> > 2.39.2
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  6:45   ` [edk2-devel] " Ni, Ray
@ 2023-05-30  7:58     ` Ard Biesheuvel
  2023-05-30  8:02       ` Ni, Ray
  2023-05-30  9:06       ` Marvin Häuser
  0 siblings, 2 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  7:58 UTC (permalink / raw)
  To: devel, ray.ni
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

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]
> > -=-=-=-=-=-=
> >
>
>
>
> 
>
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  7:58     ` Ard Biesheuvel
@ 2023-05-30  8:02       ` Ni, Ray
  2023-05-30  8:29         ` Ard Biesheuvel
  2023-05-30  9:06       ` Marvin Häuser
  1 sibling, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  8:02 UTC (permalink / raw)
  To: devel@edk2.groups.io, ardb@kernel.org
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki



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


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  8:02       ` Ni, Ray
@ 2023-05-30  8:29         ` Ard Biesheuvel
  0 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  8:29 UTC (permalink / raw)
  To: devel, ray.ni
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki

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.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state
  2023-05-30  6:54   ` Ni, Ray
@ 2023-05-30  8:33     ` Ard Biesheuvel
  0 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  8:33 UTC (permalink / raw)
  To: Ni, Ray
  Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On Tue, 30 May 2023 at 08:54, Ni, Ray <ray.ni@intel.com> wrote:
>
> 1. Do we want to catch a case that platform wrongly sets BIT61 but drivers run before CpuDxe are not XIP?
> 2. Why BIT61 set is the "Default state"?
>
> The setting of BIT61 is a bit confusing. Is there a way to avoid adding BIT61 through code optimization?
>

The idea is that we can inform the DXE core about whether the mappings
created during PEI are NX or not. So perhaps 'default state' should be
renamed to 'initial state'.

This is important for two reasons:
- The DXE drivers that execute in place are covered by
EfiBootServicesData allocations, and we should not remap those XP when
the CPU arch protocol is dispatched
- Going over all of memory to update the mappings is unnecessary if
the mappings are already XP.




>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 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: [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for
> > default NX state
> >
> > Introduce a new bit in the NX memory protection policy PCD mask that
> > specifies that the platform enters DXE with all unused and all non-code
> > regions mapped with non-execute permissions.
> >
> > This removes the need to do a pass over all memory regions to update
> > their NX memory attributes.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 +++++++
> >  MdeModulePkg/MdeModulePkg.dec                 | 3 +++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > index 7cc829b17402c2bc..983ed450f143d62d 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > @@ -861,6 +861,13 @@ InitializeDxeNxMemoryProtectionPolicy (
> >      ASSERT (StackBase != 0);
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // If the platform maps all DRAM non-execute by default, we are done here.
> >
> > +  //
> >
> > +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT61) != 0) {
> >
> > +    return;
> >
> > +  }
> >
> > +
> >
> >    DEBUG ((
> >
> >      DEBUG_INFO,
> >
> >      "%a: applying strict permissions to active memory regions\n",
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > index 2d72ac733d82195e..d2bd0cbb40300889 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1416,12 +1416,15 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
> >    #  EfiMemoryMappedIOPortSpace     0x1000<BR>
> >
> >    #  EfiPalCode                     0x2000<BR>
> >
> >    #  EfiPersistentMemory            0x4000<BR>
> >
> > +  #  Default state      0x2000000000000000<BR>
> >
> >    #  OEM Reserved       0x4000000000000000<BR>
> >
> >    #  OS Reserved        0x8000000000000000<BR>
> >
> >    #
> >
> >    # NOTE: User must NOT set NX protection for EfiLoaderCode /
> > EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
> >
> >    #       User MUST set the same NX protection for EfiBootServicesData and
> > EfiConventionalMemory. <BR>
> >
> >    #
> >
> > +  # If the platform enters DXE with all unused and non-code regions mapped NX,
> > bit 61 should be set.<BR>
> >
> > +  #
> >
> >    # e.g. 0x7FD5 can be used for all memory except Code. <BR>
> >
> >    # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved.
> > <BR>
> >
> >    #
> >
> > --
> > 2.39.2
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy
  2023-05-30  7:51     ` [edk2-devel] " Ard Biesheuvel
@ 2023-05-30  8:40       ` Ni, Ray
  2023-05-30  8:51         ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-30  8:40 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
	Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
	Michael Kubacki



> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, May 30, 2023 3:52 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 05/11] MdeModulePkg/DxeCore: Use
> memory mapped FV protocol to avoid image copy
> 
> On Tue, 30 May 2023 at 08:21, Ni, Ray <ray.ni@intel.com> wrote:
> >
> > GetFileBufferByFilePath() always returns a copy of file buffer even when the file
> > is in a memory-mapped device.
> > So, your patch adds a new implementation (abstracted through the new MM FV
> protocol) that can directly return the file location in the MMIO device.
> >
> > Several comments:
> > 1. I am not sure if any negative impact due to this change. For example: old
> logic reads the MMIO device but doesn't execute in the MMIO device. Does
> MMIO device always support execution in place?
> 
> At this point, we are not executing anything in place. The buffer is
> only used by the PE/COFF loader to access the file contents, but it
> still creates the sections in memory as before, and copies the data
> into them.
> 
> This is similar to how gBS->Loadimage() with a buffer/size only uses
> the buffer contents to access the file data, it does not execute the
> image from the buffer.

OK. 

> 
> > 2. If the MMFV protocol is only produced by DxeCore and consumed by
> DxeCore, can we just implement a local function instead? The challenge might be
> how to pass the FV_DEVICE instance to the local function. Can we "handle" the
> "FirmwareVolume2" protocol from the Handle first and use CR_FROM_THIS()
> macro to retrieve the FV_DEVICE pointer? This helps to not add the MMFV
> protocol, letting the change a pure DxeCore internal thing.
> >
> 
> The loader does not know whether the FirmwareVolume2 protocol was
> produced by DXE core or by some other component, so we cannot assume
> that CR_FROM_THIS() is usable.

I see. How about:
a. Define a GUID in DxeCore module internal
b. Install that GUID in the FV handle (the accordingly instance of that GUID can be simply NULL) 
    as a signature to tell the FV is produced by DxeCore
c. Implement a local function that return location inside the FV when the FvHandle has the
    private GUID installed.


> 
> 
> 
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > 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: [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped
> FV
> > > protocol to avoid image copy
> > >
> > > Use the memory mapped FV protocol to obtain the existing location in
> > > memory and the size of an image being loaded from a firmware volume.
> > > This removes the need to do a memcopy of the file data.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdeModulePkg/Core/Dxe/DxeMain.h                |   1 +
> > >  MdeModulePkg/Core/Dxe/DxeMain.inf              |   3 +
> > >  MdeModulePkg/Core/Dxe/Image/Image.c            | 111 +++++++++++++++++-
> --
> > >  MdeModulePkg/Include/Protocol/MemoryMappedFv.h |  59 +++++++++++
> > >  MdeModulePkg/MdeModulePkg.dec                  |   3 +
> > >  5 files changed, 163 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > b/MdeModulePkg/Core/Dxe/DxeMain.h
> > > index 43daa037be441150..a695b457c79b65bb 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > > @@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >  #include <Protocol/HiiPackageList.h>
> > >
> > >  #include <Protocol/SmmBase2.h>
> > >
> > >  #include <Protocol/PeCoffImageEmulator.h>
> > >
> > > +#include <Protocol/MemoryMappedFv.h>
> > >
> > >  #include <Guid/MemoryTypeInformation.h>
> > >
> > >  #include <Guid/FirmwareFileSystem2.h>
> > >
> > >  #include <Guid/FirmwareFileSystem3.h>
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > index 35d5bf0dee6f7f3f..a7175cb364b9b5de 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > @@ -153,6 +153,9 @@ [Protocols]
> > >    gEfiLoadedImageDevicePathProtocolGuid         ## PRODUCES
> > >
> > >    gEfiHiiPackageListProtocolGuid                ## SOMETIMES_PRODUCES
> > >
> > >    gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
> > >
> > > +  ## PRODUCES
> > >
> > > +  ## CONSUMES
> > >
> > > +  gEdkiiMemoryMappedFvProtocolGuid
> > >
> > >    gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES
> > >
> > >
> > >
> > >    # Arch Protocols
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > > index f30e369370a09609..3dfab4829b3ca17f 100644
> > > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > > @@ -1043,6 +1043,76 @@ CoreUnloadAndCloseImage (
> > >    CoreFreePool (Image);
> > >
> > >  }
> > >
> > >
> > >
> > > +/**
> > >
> > > +  Get the image file data and size directly from a memory mapped FV
> > >
> > > +
> > >
> > > +  If FilePath is NULL, then NULL is returned.
> > >
> > > +  If FileSize is NULL, then NULL is returned.
> > >
> > > +  If AuthenticationStatus is NULL, then NULL is returned.
> > >
> > > +
> > >
> > > +  @param[in]       FvHandle             The firmware volume handle
> > >
> > > +  @param[in]       FilePath             The pointer to the device path of the file
> > >
> > > +                                        that is abstracted to the file buffer.
> > >
> > > +  @param[out]      FileSize             The pointer to the size of the abstracted
> > >
> > > +                                        file buffer.
> > >
> > > +  @param[out]      AuthenticationStatus Pointer to the authentication status.
> > >
> > > +
> > >
> > > +  @retval NULL   FilePath is NULL, or FileSize is NULL, or AuthenticationStatus
> > >
> > > +                 is NULL, or the file is not memory mapped
> > >
> > > +  @retval other  The abstracted file buffer.
> > >
> > > +**/
> > >
> > > +STATIC
> > >
> > > +VOID *
> > >
> > > +GetFileFromMemoryMappedFv (
> > >
> > > +  IN  EFI_HANDLE                      FvHandle,
> > >
> > > +  IN  CONST EFI_DEVICE_PATH_PROTOCOL  *FilePath,
> > >
> > > +  OUT UINTN                           *FileSize,
> > >
> > > +  OUT UINT32                          *AuthenticationStatus
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *MemMappedFv;
> > >
> > > +  CONST EFI_GUID                   *NameGuid;
> > >
> > > +  EFI_PHYSICAL_ADDRESS             Address;
> > >
> > > +  EFI_STATUS                       Status;
> > >
> > > +
> > >
> > > +  if ((FilePath == NULL) ||
> > >
> > > +      (FileSize == NULL) ||
> > >
> > > +      (AuthenticationStatus == NULL))
> > >
> > > +  {
> > >
> > > +    return NULL;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  NameGuid = EfiGetNameGuidFromFwVolDevicePathNode (
> > >
> > > +               (CONST MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)FilePath);
> > >
> > > +  if (NameGuid == NULL) {
> > >
> > > +    return NULL;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  Status = gBS->HandleProtocol (
> > >
> > > +                  FvHandle,
> > >
> > > +                  &gEdkiiMemoryMappedFvProtocolGuid,
> > >
> > > +                  (VOID **)&MemMappedFv
> > >
> > > +                  );
> > >
> > > +  if (EFI_ERROR (Status)) {
> > >
> > > +    ASSERT (Status == EFI_UNSUPPORTED);
> > >
> > > +    return NULL;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  Status = MemMappedFv->GetLocationAndSize (
> > >
> > > +                          MemMappedFv,
> > >
> > > +                          NameGuid,
> > >
> > > +                          EFI_SECTION_PE32,
> > >
> > > +                          &Address,
> > >
> > > +                          FileSize,
> > >
> > > +                          AuthenticationStatus
> > >
> > > +                          );
> > >
> > > +  if (EFI_ERROR (Status) || (Address > (MAX_ADDRESS - *FileSize))) {
> > >
> > > +    return NULL;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  return (VOID *)(UINTN)Address;
> > >
> > > +}
> > >
> > > +
> > >
> > >  /**
> > >
> > >    Loads an EFI image into memory and returns a handle to the image.
> > >
> > >
> > >
> > > @@ -1164,6 +1234,16 @@ CoreLoadImageCommon (
> > >      Status = CoreLocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid,
> > > &HandleFilePath, &DeviceHandle);
> > >
> > >      if (!EFI_ERROR (Status)) {
> > >
> > >        ImageIsFromFv = TRUE;
> > >
> > > +
> > >
> > > +      //
> > >
> > > +      // If possible, use the memory mapped file image directly, rather than
> > > copying it into a buffer
> > >
> > > +      //
> > >
> > > +      FHand.Source = GetFileFromMemoryMappedFv (
> > >
> > > +                       DeviceHandle,
> > >
> > > +                       HandleFilePath,
> > >
> > > +                       &FHand.SourceSize,
> > >
> > > +                       &AuthenticationStatus
> > >
> > > +                       );
> > >
> > >      } else {
> > >
> > >        HandleFilePath = FilePath;
> > >
> > >        Status         = CoreLocateDevicePath (&gEfiSimpleFileSystemProtocolGuid,
> > > &HandleFilePath, &DeviceHandle);
> > >
> > > @@ -1187,21 +1267,24 @@ CoreLoadImageCommon (
> > >      //
> > >
> > >      // Get the source file buffer by its device path.
> > >
> > >      //
> > >
> > > -    FHand.Source = GetFileBufferByFilePath (
> > >
> > > -                     BootPolicy,
> > >
> > > -                     FilePath,
> > >
> > > -                     &FHand.SourceSize,
> > >
> > > -                     &AuthenticationStatus
> > >
> > > -                     );
> > >
> > >      if (FHand.Source == NULL) {
> > >
> > > -      Status = EFI_NOT_FOUND;
> > >
> > > -    } else {
> > >
> > > -      FHand.FreeBuffer = TRUE;
> > >
> > > -      if (ImageIsFromLoadFile) {
> > >
> > > -        //
> > >
> > > -        // LoadFile () may cause the device path of the Handle be updated.
> > >
> > > -        //
> > >
> > > -        OriginalFilePath = AppendDevicePath (DevicePathFromHandle
> > > (DeviceHandle), Node);
> > >
> > > +      FHand.Source = GetFileBufferByFilePath (
> > >
> > > +                       BootPolicy,
> > >
> > > +                       FilePath,
> > >
> > > +                       &FHand.SourceSize,
> > >
> > > +                       &AuthenticationStatus
> > >
> > > +                       );
> > >
> > > +
> > >
> > > +      if (FHand.Source == NULL) {
> > >
> > > +        Status = EFI_NOT_FOUND;
> > >
> > > +      } else {
> > >
> > > +        FHand.FreeBuffer = TRUE;
> > >
> > > +        if (ImageIsFromLoadFile) {
> > >
> > > +          //
> > >
> > > +          // LoadFile () may cause the device path of the Handle be updated.
> > >
> > > +          //
> > >
> > > +          OriginalFilePath = AppendDevicePath (DevicePathFromHandle
> > > (DeviceHandle), Node);
> > >
> > > +        }
> > >
> > >        }
> > >
> > >      }
> > >
> > >    }
> > >
> > > diff --git a/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> > > b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> > > new file mode 100644
> > > index 0000000000000000..821009122113a658
> > > --- /dev/null
> > > +++ b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h
> > > @@ -0,0 +1,59 @@
> > > +/** @file
> > >
> > > +  Protocol to obtain information about files in memory mapped firmware
> > > volumes
> > >
> > > +
> > >
> > > +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> > >
> > > +
> > >
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > +
> > >
> > > +**/
> > >
> > > +
> > >
> > > +#ifndef EDKII_MEMORY_MAPPED_FV_H_
> > >
> > > +#define EDKII_MEMORY_MAPPED_FV_H_
> > >
> > > +
> > >
> > > +#define EDKII_MEMORY_MAPPED_FV_PROTOCOL_GUID \
> > >
> > > +  { 0xb9bfa973, 0x5384, 0x441e, { 0xa4, 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e,
> > > 0x0f } }
> > >
> > > +
> > >
> > > +typedef struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL
> > > EDKII_MEMORY_MAPPED_FV_PROTOCOL;
> > >
> > > +
> > >
> > > +//
> > >
> > > +// Function Prototypes
> > >
> > > +//
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Get the physical address and size of a file's section in a memory mapped FV
> > >
> > > +
> > >
> > > +  @param[in]  This          The protocol pointer
> > >
> > > +  @param[in]  NameGuid      The name GUID of the file
> > >
> > > +  @param[in]  SectionType   The file section from which to retrieve address
> and
> > > size
> > >
> > > +  @param[out] FileAddress   The physical address of the file
> > >
> > > +  @param[out] FileSize      The size of the file
> > >
> > > +  @param[out] AuthStatus    The authentication status associated with the
> file
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS            Information about the file was retrieved
> > > successfully.
> > >
> > > +  @retval EFI_INVALID_PARAMETER  FileAddress was NULL, FileSize was
> NULL,
> > > AuthStatus
> > >
> > > +                                 was NULL.
> > >
> > > +  @retval EFI_NOT_FOUND          No section of the specified type could be
> > > located in
> > >
> > > +                                 the specified file.
> > >
> > > +
> > >
> > > +**/
> > >
> > > +typedef
> > >
> > > +EFI_STATUS
> > >
> > > +(EFIAPI *GET_LOCATION_AND_SIZE)(
> > >
> > > +  IN  EDKII_MEMORY_MAPPED_FV_PROTOCOL  *This,
> > >
> > > +  IN  CONST EFI_GUID                   *NameGuid,
> > >
> > > +  IN  EFI_SECTION_TYPE                 SectionType,
> > >
> > > +  OUT EFI_PHYSICAL_ADDRESS             *FileAddress,
> > >
> > > +  OUT UINTN                            *FileSize,
> > >
> > > +  OUT UINT32                           *AuthStatus
> > >
> > > +  );
> > >
> > > +
> > >
> > > +//
> > >
> > > +// Protocol interface structure
> > >
> > > +//
> > >
> > > +struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL {
> > >
> > > +  GET_LOCATION_AND_SIZE   GetLocationAndSize;
> > >
> > > +};
> > >
> > > +
> > >
> > > +extern EFI_GUID  gEdkiiMemoryMappedFvProtocolGuid;
> > >
> > > +
> > >
> > > +#endif
> > >
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > > b/MdeModulePkg/MdeModulePkg.dec
> > > index d65dae18aa81e569..2d72ac733d82195e 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -679,6 +679,9 @@ [Protocols]
> > >    ## Include/Protocol/PlatformBootManager.h
> > >
> > >    gEdkiiPlatformBootManagerProtocolGuid = { 0xaa17add4, 0x756c, 0x460d,
> > > { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 0x59 } }
> > >
> > >
> > >
> > > +  ## Include/Protocol/MemoryMappedFv.h
> > >
> > > +  gEdkiiMemoryMappedFvProtocolGuid = { 0xb9bfa973, 0x5384, 0x441e,
> { 0xa4,
> > > 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e, 0x0f } }
> > >
> > > +
> > >
> > >  #
> > >
> > >  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> > >
> > >  #   0x80000001 | Invalid value provided.
> > >
> > > --
> > > 2.39.2
> >
> >
> >
> > 
> >
> >

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy
  2023-05-30  8:40       ` Ni, Ray
@ 2023-05-30  8:51         ` Ard Biesheuvel
  0 siblings, 0 replies; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  8:51 UTC (permalink / raw)
  To: Ni, Ray
  Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On Tue, 30 May 2023 at 10:41, Ni, Ray <ray.ni@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, May 30, 2023 3:52 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 05/11] MdeModulePkg/DxeCore: Use
> > memory mapped FV protocol to avoid image copy
> >
> > On Tue, 30 May 2023 at 08:21, Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > GetFileBufferByFilePath() always returns a copy of file buffer even when the file
> > > is in a memory-mapped device.
> > > So, your patch adds a new implementation (abstracted through the new MM FV
> > protocol) that can directly return the file location in the MMIO device.
> > >
> > > Several comments:
> > > 1. I am not sure if any negative impact due to this change. For example: old
> > logic reads the MMIO device but doesn't execute in the MMIO device. Does
> > MMIO device always support execution in place?
> >
> > At this point, we are not executing anything in place. The buffer is
> > only used by the PE/COFF loader to access the file contents, but it
> > still creates the sections in memory as before, and copies the data
> > into them.
> >
> > This is similar to how gBS->Loadimage() with a buffer/size only uses
> > the buffer contents to access the file data, it does not execute the
> > image from the buffer.
>
> OK.
>
> >
> > > 2. If the MMFV protocol is only produced by DxeCore and consumed by
> > DxeCore, can we just implement a local function instead? The challenge might be
> > how to pass the FV_DEVICE instance to the local function. Can we "handle" the
> > "FirmwareVolume2" protocol from the Handle first and use CR_FROM_THIS()
> > macro to retrieve the FV_DEVICE pointer? This helps to not add the MMFV
> > protocol, letting the change a pure DxeCore internal thing.
> > >
> >
> > The loader does not know whether the FirmwareVolume2 protocol was
> > produced by DXE core or by some other component, so we cannot assume
> > that CR_FROM_THIS() is usable.
>
> I see. How about:
> a. Define a GUID in DxeCore module internal
> b. Install that GUID in the FV handle (the accordingly instance of that GUID can be simply NULL)
>     as a signature to tell the FV is produced by DxeCore
> c. Implement a local function that return location inside the FV when the FvHandle has the
>     private GUID installed.
>

How is this any different from implementing a protocol? Installing a
GUID on the handle and associating it with some code is exactly what
this code does, but in the 'official' way.

I'm not sure what we will gain by doing the same thing using local
routines, other than making the code more difficult to follow. At
least the protocol has a clearly defined interface, whereas the
private GUID has no intrinsic meaning associated with it.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  7:58     ` Ard Biesheuvel
  2023-05-30  8:02       ` Ni, Ray
@ 2023-05-30  9:06       ` Marvin Häuser
  2023-05-30  9:18         ` Marvin Häuser
  2023-05-30  9:38         ` Ard Biesheuvel
  1 sibling, 2 replies; 45+ messages in thread
From: Marvin Häuser @ 2023-05-30  9:06 UTC (permalink / raw)
  To: Ard Biesheuvel, devel

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

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

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:06       ` Marvin Häuser
@ 2023-05-30  9:18         ` Marvin Häuser
  2023-05-30  9:38         ` Ard Biesheuvel
  1 sibling, 0 replies; 45+ messages in thread
From: Marvin Häuser @ 2023-05-30  9:18 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel-groups-io; +Cc: Ray Ni

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



> 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


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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:06       ` Marvin Häuser
  2023-05-30  9:18         ` Marvin Häuser
@ 2023-05-30  9:38         ` Ard Biesheuvel
  2023-05-30  9:41           ` Marvin Häuser
  1 sibling, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  9:38 UTC (permalink / raw)
  To: Marvin Häuser; +Cc: devel

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.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:38         ` Ard Biesheuvel
@ 2023-05-30  9:41           ` Marvin Häuser
  2023-05-30  9:47             ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Marvin Häuser @ 2023-05-30  9:41 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: devel


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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:41           ` Marvin Häuser
@ 2023-05-30  9:47             ` Ard Biesheuvel
  2023-05-30  9:48               ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  9:47 UTC (permalink / raw)
  To: Marvin Häuser; +Cc: devel

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?

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:47             ` Ard Biesheuvel
@ 2023-05-30  9:48               ` Ard Biesheuvel
  2023-05-30  9:52                 ` Marvin Häuser
  0 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30  9:48 UTC (permalink / raw)
  To: Marvin Häuser; +Cc: devel

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.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:48               ` Ard Biesheuvel
@ 2023-05-30  9:52                 ` Marvin Häuser
  2023-05-30 10:02                   ` Ard Biesheuvel
  0 siblings, 1 reply; 45+ messages in thread
From: Marvin Häuser @ 2023-05-30  9:52 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel-groups-io, Ray Ni

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



> 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. :)

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30  9:52                 ` Marvin Häuser
@ 2023-05-30 10:02                   ` Ard Biesheuvel
  2023-05-30 10:25                     ` Marvin Häuser
  0 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-05-30 10:02 UTC (permalink / raw)
  To: devel, mhaeuser; +Cc: Ray Ni

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.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30 10:02                   ` Ard Biesheuvel
@ 2023-05-30 10:25                     ` Marvin Häuser
  2023-05-31  7:13                       ` Ni, Ray
  0 siblings, 1 reply; 45+ messages in thread
From: Marvin Häuser @ 2023-05-30 10:25 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel-groups-io, Ray Ni

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


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


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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-30 10:25                     ` Marvin Häuser
@ 2023-05-31  7:13                       ` Ni, Ray
  2023-05-31  8:05                         ` Marvin Häuser
  0 siblings, 1 reply; 45+ messages in thread
From: Ni, Ray @ 2023-05-31  7:13 UTC (permalink / raw)
  To: devel@edk2.groups.io, mhaeuser@posteo.de, Ard Biesheuvel,
	Liu, Zhiguang

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

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.

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers
  2023-05-31  7:13                       ` Ni, Ray
@ 2023-05-31  8:05                         ` Marvin Häuser
  0 siblings, 0 replies; 45+ messages in thread
From: Marvin Häuser @ 2023-05-31  8:05 UTC (permalink / raw)
  To: Ni, Ray; +Cc: devel, Ard Biesheuvel, Liu, Zhiguang

[-- Attachment #1: Type: text/html, Size: 10258 bytes --]

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place
  2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
                   ` (10 preceding siblings ...)
  2023-05-29 10:17 ` [RFC PATCH 11/11] ArmVirtPkg/ArmVirtQemu: Map all DRAM non-execute by default Ard Biesheuvel
@ 2023-06-01 14:53 ` Oliver Smith-Denny
  2023-06-01 18:11   ` Ard Biesheuvel
  11 siblings, 1 reply; 45+ messages in thread
From: Oliver Smith-Denny @ 2023-06-01 14:53 UTC (permalink / raw)
  To: devel, ardb
  Cc: Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On 5/29/2023 3:16 AM, Ard Biesheuvel wrote:
> TL;DR - allow DXE drivers to execute in place from the decompressed FV
> 
> loaded into memory by DxeIpl so we can apply strict permissions before
> 
> dispatching DXE core.
> 
> 
> 
> Currently, executable images loaded from firmware volumes are copied at
> 
> least three times: once in the firmware volume driver, once in the DXE
> 
> core load image code, and finally when the PE sections are populated in
> 
> memory based on the section descriptions in the file.
> 
> 
> 
> At least two of these copies serve little purpose, given that most
> 
> drivers are typically dispatched from a memory-mapped firmware volume
> 
> that is loaded into DRAM by DxeIpl from a compressed image in the boot
> 
> FV, and so we can take a short-cut in the DXE image loader so that the
> 
> PE/COFF library that performs the load uses the image in the memory
> 
> mapped FV as its source directly. This is implemented by the first 6
> 
> patches (where the first 3 are just cleanups)
> 
> 
> 
> With this logic in place, we can go one step further, and actually
> 
> dispatch the image in place (similar to how XIP PEIMs are dispatched),
> 
> without over moving it out of the decompressed firmware volume. This
> 
> requires the image to be aligned sufficiently inside the FV, but this is
> 
> also the same logic that applies to XIP PEIMs, and this can be achieved
> 
> trivially by tweaking some FDF image generation rules. (Note that this
> 
> adds padding to the FV, but this generally compresses well, and we
> 
> ultimately uses less memory at runtime by not making a copy of the
> 
> image).
> 
> 
> 
> This requires the DXE IPL (which is the component that decompresses the
> 
> firmware volumes to memory) to iterate over the contents and relocate
> 
> these drivers in place. Given that DXE IPL is already in charge of
> 
> applying NX permissions to the stack and to other memory regions, we can
> 
> trivially extend it to apply restricted permissions to the XIP DXE
> 
> drivers after relocation.
> 
> 
> 
> This means we enter DXE core with those DXE drivers ready to be
> 
> dispatched, removing the need to perform manipulation of memory
> 
> attributes before the CPU arch protocol is dispatched, which is a bit of
> 
> a catch-22 otherwise.
> 
> 
> 
> With these changes in place, the platform no longer needs to map memory
> 
> writable and executable by default, and all DRAM can be mapped
> 
> non-executable right out of reset.
> 
> 

Hi Ard,

Thanks for sending out this RFC, great to see more work on the memory
protections front. A few questions and thoughts:

This seems a good effort (in conjunction with your last RFC) to close
the protection gap between DxeCore launch and CpuDxe launch for marking
non-code regions NX. Do you see other protections (guard pages for
example) fitting into this method? I believe for any dynamic protections
during this timeframe we would need the ability to manipulate the page
tables directly from DxeCore.

Similarly, in order to lessen the complexity of the DXE driver usage of
memory resources and avoiding sync issues (e.g. a driver allocates pages
that are mapped NX by default, then it sets a cacheability attribute
and accidentally clears NX), I think further work would be valuable to
reduce that complexity. I think your new PPI that allows setting and
preserving bits independently of what is passed in is a very good step
towards reducing this complexity.

This patchset would move all properly aligned DXE drivers to be XIP,
correct? Because we are XIP in DRAM, this should not have any
performance implications (other than a benefit from reducing the extra
copies in your first few patches), aside from potential space
differences, which as you note compression will likely do away with,
right?

Thanks,
Oliver

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place
  2023-06-01 14:53 ` [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place Oliver Smith-Denny
@ 2023-06-01 18:11   ` Ard Biesheuvel
  2023-06-01 18:30     ` Oliver Smith-Denny
  0 siblings, 1 reply; 45+ messages in thread
From: Ard Biesheuvel @ 2023-06-01 18:11 UTC (permalink / raw)
  To: Oliver Smith-Denny
  Cc: devel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On Thu, 1 Jun 2023 at 16:53, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Thanks for sending out this RFC, great to see more work on the memory
> protections front. A few questions and thoughts:
>
> This seems a good effort (in conjunction with your last RFC) to close
> the protection gap between DxeCore launch and CpuDxe launch for marking
> non-code regions NX. Do you see other protections (guard pages for
> example) fitting into this method? I believe for any dynamic protections
> during this timeframe we would need the ability to manipulate the page
> tables directly from DxeCore.
>

The use case of guard pages did not really occur to me, to be honest,
and this is obviously something that doesn't work either before the
CPU arch protocol is dispatched

I still think it would be preferable to add the ability to manage
memory mapping permissions to the DXE core itself, and separate it
from the CPU arch protocol.

Note that clumping everything together does not really help in this
respect either: if the memory permission manipulation logically
remains a part of the CPU arch protocol, which cannot be installed
until its dependencies are satisfied, we are still in a situation
where dispatching those dependencies may result in page allocations
being created before we can unmap the guard pages.

> Similarly, in order to lessen the complexity of the DXE driver usage of
> memory resources and avoiding sync issues (e.g. a driver allocates pages
> that are mapped NX by default, then it sets a cacheability attribute
> and accidentally clears NX), I think further work would be valuable to
> reduce that complexity. I think your new PPI that allows setting and
> preserving bits independently of what is passed in is a very good step
> towards reducing this complexity.
>

Hopefully, we'll be able to do something at the library/driver level
here (AllocatePages in the DMA or PCI layer).

Another thing we might entertain (which maps really well onto the WXN
thing we have on ARM) is to add a GCD memory region capability that
makes memory XP unless it is RO. But I haven't really experimented
with that yet - I'll keep you posted on that.

> This patchset would move all properly aligned DXE drivers to be XIP,
> correct?

Yes.

> Because we are XIP in DRAM, this should not have any
> performance implications (other than a benefit from reducing the extra
> copies in your first few patches), aside from potential space
> differences, which as you note compression will likely do away with,
> right?
>

Exactly.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place
  2023-06-01 18:11   ` Ard Biesheuvel
@ 2023-06-01 18:30     ` Oliver Smith-Denny
  0 siblings, 0 replies; 45+ messages in thread
From: Oliver Smith-Denny @ 2023-06-01 18:30 UTC (permalink / raw)
  To: devel, ardb
  Cc: Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
	Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
	Leif Lindholm, Michael Kubacki

On 6/1/2023 11:11 AM, Ard Biesheuvel wrote:
> On Thu, 1 Jun 2023 at 16:53, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> Thanks for sending out this RFC, great to see more work on the memory
>> protections front. A few questions and thoughts:
>>
>> This seems a good effort (in conjunction with your last RFC) to close
>> the protection gap between DxeCore launch and CpuDxe launch for marking
>> non-code regions NX. Do you see other protections (guard pages for
>> example) fitting into this method? I believe for any dynamic protections
>> during this timeframe we would need the ability to manipulate the page
>> tables directly from DxeCore.
>>
> 
> The use case of guard pages did not really occur to me, to be honest,
> and this is obviously something that doesn't work either before the
> CPU arch protocol is dispatched
> 
> I still think it would be preferable to add the ability to manage
> memory mapping permissions to the DXE core itself, and separate it
> from the CPU arch protocol.
> 
> Note that clumping everything together does not really help in this
> respect either: if the memory permission manipulation logically
> remains a part of the CPU arch protocol, which cannot be installed
> until its dependencies are satisfied, we are still in a situation
> where dispatching those dependencies may result in page allocations
> being created before we can unmap the guard pages.
> 

I agree. I think that the ability for DXE Core to manage the memory
attributes itself is the central simplification that could go a long
ways towards cleaning up the existing interfaces and making the code
more maintainable with less potential pitfalls (that we currently see
hit frequently).

I think the memory attribute management can be split apart from CpuDxe
without bringing all of CpuDxe into Dxe Core and certainly if that is
the better path towards tackling the problem we face, I support it.

>> Similarly, in order to lessen the complexity of the DXE driver usage of
>> memory resources and avoiding sync issues (e.g. a driver allocates pages
>> that are mapped NX by default, then it sets a cacheability attribute
>> and accidentally clears NX), I think further work would be valuable to
>> reduce that complexity. I think your new PPI that allows setting and
>> preserving bits independently of what is passed in is a very good step
>> towards reducing this complexity.
>>
> 
> Hopefully, we'll be able to do something at the library/driver level
> here (AllocatePages in the DMA or PCI layer).
> 
> Another thing we might entertain (which maps really well onto the WXN
> thing we have on ARM) is to add a GCD memory region capability that
> makes memory XP unless it is RO. But I haven't really experimented
> with that yet - I'll keep you posted on that.
> 

This is definitely interesting to me as well. I would love this
capability :). I've been doing some investigations in this area,
but not specifically towards ARM's WXN, that seems a great place
to intersect.

Thanks,
Oliver

>> This patchset would move all properly aligned DXE drivers to be XIP,
>> correct?
> 
> Yes.
> 
>> Because we are XIP in DRAM, this should not have any
>> performance implications (other than a benefit from reducing the extra
>> copies in your first few patches), aside from potential space
>> differences, which as you note compression will likely do away with,
>> right?
>>
> 
> Exactly.
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

end of thread, other threads:[~2023-06-01 18:30 UTC | newest]

Thread overview: 45+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-29 10:16 [RFC PATCH 00/11] Permit DXE drivers to execute in place Ard Biesheuvel
2023-05-29 10:16 ` [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage Ard Biesheuvel
2023-05-30  5:54   ` Ni, Ray
2023-05-30  7:36     ` Ard Biesheuvel
2023-05-29 10:16 ` [RFC PATCH 02/11] MdeModulePkg/DxeCore: Remove unused DstBuffer arg from LoadImage Ard Biesheuvel
2023-05-30  5:58   ` Ni, Ray
2023-05-29 10:16 ` [RFC PATCH 03/11] MdeModulePkg/DxeCore: Remove FreePage argument from CoreUnloadImage Ard Biesheuvel
2023-05-30  5:59   ` Ni, Ray
2023-05-29 10:16 ` [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files Ard Biesheuvel
2023-05-30  6:03   ` Ni, Ray
2023-05-30  7:47     ` Ard Biesheuvel
2023-05-29 10:16 ` [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy Ard Biesheuvel
2023-05-30  6:21   ` Ni, Ray
2023-05-30  7:51     ` [edk2-devel] " Ard Biesheuvel
2023-05-30  8:40       ` Ni, Ray
2023-05-30  8:51         ` Ard Biesheuvel
2023-05-29 10:17 ` [RFC PATCH 06/11] MdeModulePkg/DxeCore: Expose memory mapped FV protocol when possible Ard Biesheuvel
2023-05-30  6:22   ` Ni, Ray
2023-05-29 10:17 ` [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible Ard Biesheuvel
2023-05-30  6:32   ` Ni, Ray
2023-05-30  7:54     ` Ard Biesheuvel
2023-05-29 10:17 ` [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers Ard Biesheuvel
2023-05-30  6:45   ` [edk2-devel] " Ni, Ray
2023-05-30  7:58     ` Ard Biesheuvel
2023-05-30  8:02       ` Ni, Ray
2023-05-30  8:29         ` Ard Biesheuvel
2023-05-30  9:06       ` Marvin Häuser
2023-05-30  9:18         ` Marvin Häuser
2023-05-30  9:38         ` Ard Biesheuvel
2023-05-30  9:41           ` Marvin Häuser
2023-05-30  9:47             ` Ard Biesheuvel
2023-05-30  9:48               ` Ard Biesheuvel
2023-05-30  9:52                 ` Marvin Häuser
2023-05-30 10:02                   ` Ard Biesheuvel
2023-05-30 10:25                     ` Marvin Häuser
2023-05-31  7:13                       ` Ni, Ray
2023-05-31  8:05                         ` Marvin Häuser
2023-05-29 10:17 ` [RFC PATCH 09/11] MdeModulePkg/DxeCore: Add PCD NX policy bit for default NX state Ard Biesheuvel
2023-05-30  6:54   ` Ni, Ray
2023-05-30  8:33     ` Ard Biesheuvel
2023-05-29 10:17 ` [RFC PATCH 10/11] ArmVirtPkg/ArmVirtQemu: Allow CPU arch protocol DXE to execute in place Ard Biesheuvel
2023-05-29 10:17 ` [RFC PATCH 11/11] ArmVirtPkg/ArmVirtQemu: Map all DRAM non-execute by default Ard Biesheuvel
2023-06-01 14:53 ` [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place Oliver Smith-Denny
2023-06-01 18:11   ` Ard Biesheuvel
2023-06-01 18:30     ` Oliver Smith-Denny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox