[PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions

[PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions

[Ard Biesheuvel]

A pair of cleanup patches for the LoadImage/StartImage implementations
in DxeCore.

https://github.com/ardbiesheuvel/edk2/pull/new/dxecore-loadimage-cleanup

Ard Biesheuvel (2):
  MdeModulePkg/DxeCore/Image: make local functions STATIC
  MdeModulePkg/DxeCore/Image: remove unused function arguments

 MdeModulePkg/Core/Dxe/Image/Image.c | 91 ++++----------------
 1 file changed, 17 insertions(+), 74 deletions(-)

-- 
2.17.1

[PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC

[Ard Biesheuvel]

Use static linkage for functions that are only used locally, to help
the compiler optimize away unused code in non-LTO builds.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index d86da89ee704..aae2c1eaa516 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -86,6 +86,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED MACHINE_TYPE_INFO  mMachineTypeInfo[] = {
 
 UINT16 mDxeCoreImageMachineType = 0;
 
+#ifndef MDEPKG_NDEBUG
 /**
  Return machine type name.
 
@@ -93,6 +94,7 @@ UINT16 mDxeCoreImageMachineType = 0;
 
  @return machine type name
 **/
+STATIC
 CHAR16 *
 GetMachineTypeName (
   UINT16 MachineType
@@ -108,6 +110,7 @@ GetMachineTypeName (
 
   return L"<Unknown>";
 }
+#endif
 
 /**
   Notification event handler registered by CoreInitializeImageServices () to
@@ -286,6 +289,7 @@ CoreInitializeImageServices (
                                  into buffer.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 CoreReadImageFile (
@@ -334,6 +338,7 @@ CoreReadImageFile (
   @retval EFI_SUCCESS              The memory range the image will be loaded in is available
   @retval EFI_NOT_FOUND            The memory range the image will be loaded in is not available
 **/
+STATIC
 EFI_STATUS
 CheckAndMarkFixLoadingMemoryUsageBitMap (
   IN  EFI_PHYSICAL_ADDRESS          ImageBase,
@@ -407,6 +412,7 @@ CheckAndMarkFixLoadingMemoryUsageBitMap (
   @retval EFI_NOT_FOUND             The image has no assigned fixed loading address.
 
 **/
+STATIC
 EFI_STATUS
 GetPeCoffImageFixLoadingAssignedAddress(
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
@@ -552,6 +558,7 @@ CoreIsImageTypeSupported (
   @retval EFI_BUFFER_TOO_SMALL    Buffer for image is too small
 
 **/
+STATIC
 EFI_STATUS
 CoreLoadPeImage (
   IN BOOLEAN                     BootPolicy,
@@ -877,6 +884,7 @@ CoreLoadPeImage (
   @return Return the image private data associated with ImageHandle.
 
 **/
+STATIC
 LOADED_IMAGE_PRIVATE_DATA *
 CoreLoadedImageInfo (
   IN EFI_HANDLE  ImageHandle
@@ -909,6 +917,7 @@ CoreLoadedImageInfo (
   @param  FreePage                Free allocated pages
 
 **/
+STATIC
 VOID
 CoreUnloadAndCloseImage (
   IN LOADED_IMAGE_PRIVATE_DATA  *Image,
@@ -1105,6 +1114,7 @@ CoreUnloadAndCloseImage (
                                   platform policy specifies that the image should not be started.
 
 **/
+STATIC
 EFI_STATUS
 CoreLoadImageCommon (
   IN  BOOLEAN                          BootPolicy,
-- 
2.17.1

[PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments

[Ard Biesheuvel]

Image.c defines a CoreLoadImageCommon() function that has some arguments
that are marked OPTIONAL. The function only has a single caller living in
the same file, and it does not pass any arguments for these OPTIONAL
arguments.

So let's simplify the code, and remove these arguments and all the
transitive conditional dependencies on them.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

NOTE: this patch was generated with the -w option, to suppress indentation
changes from cluttering the diff.

 MdeModulePkg/Core/Dxe/Image/Image.c | 81 ++------------------
 1 file changed, 7 insertions(+), 74 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index aae2c1eaa516..9fdde87f2df3 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -564,13 +564,10 @@ CoreLoadPeImage (
   IN BOOLEAN                     BootPolicy,
   IN VOID                        *Pe32Handle,
   IN LOADED_IMAGE_PRIVATE_DATA   *Image,
-  IN EFI_PHYSICAL_ADDRESS        DstBuffer    OPTIONAL,
-  OUT EFI_PHYSICAL_ADDRESS       *EntryPoint  OPTIONAL,
   IN  UINT32                     Attribute
   )
 {
   EFI_STATUS                Status;
-  BOOLEAN                   DstBufAlocated;
   UINTN                     Size;
 
   ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext));
@@ -619,15 +616,6 @@ CoreLoadPeImage (
     return EFI_UNSUPPORTED;
   }
 
-  //
-  // Allocate memory of the correct memory type aligned on the required image boundary
-  //
-  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 {
@@ -685,31 +673,6 @@ CoreLoadPeImage (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-    DstBufAlocated = TRUE;
-  } 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->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;
-    }
-
-    Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
-    Image->ImageContext.ImageAddress = DstBuffer;
-  }
 
   Image->ImageBasePage = Image->ImageContext.ImageAddress;
   if (!Image->ImageContext.IsTeImage) {
@@ -790,13 +753,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
   //
@@ -861,11 +817,9 @@ CoreLoadPeImage (
   // Free memory.
   //
 
-  if (DstBufAlocated) {
   CoreFreePages (Image->ImageContext.ImageAddress, Image->NumberOfPages);
   Image->ImageContext.ImageAddress = 0;
   Image->ImageBasePage = 0;
-  }
 
   if (Image->ImageContext.FixupData != NULL) {
     CoreFreePool (Image->ImageContext.FixupData);
@@ -920,8 +874,7 @@ CoreLoadedImageInfo (
 STATIC
 VOID
 CoreUnloadAndCloseImage (
-  IN LOADED_IMAGE_PRIVATE_DATA  *Image,
-  IN BOOLEAN                    FreePage
+  IN LOADED_IMAGE_PRIVATE_DATA  *Image
   )
 {
   EFI_STATUS                          Status;
@@ -1047,7 +1000,7 @@ CoreUnloadAndCloseImage (
   //
   // Free the Image from memory
   //
-  if ((Image->ImageBasePage != 0) && FreePage) {
+  if ((Image->ImageBasePage != 0)) {
     CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
   }
 
@@ -1122,10 +1075,7 @@ 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,
-  OUT EFI_PHYSICAL_ADDRESS             *EntryPoint         OPTIONAL,
   IN  UINT32                           Attribute
   )
 {
@@ -1330,12 +1280,7 @@ CoreLoadImageCommon (
   Image->Info.FilePath     = DuplicateDevicePath (FilePath);
   Image->Info.ParentHandle = ParentImageHandle;
 
-
-  if (NumberOfPages != NULL) {
-    Image->NumberOfPages = *NumberOfPages ;
-  } else {
   Image->NumberOfPages = 0 ;
-  }
 
   //
   // Install the protocol interfaces for this image
@@ -1355,20 +1300,11 @@ CoreLoadImageCommon (
   //
   // Load the image.  If EntryPoint is Null, it will not be set.
   //
-  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, EntryPoint, 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
   //
@@ -1448,7 +1384,7 @@ CoreLoadImageCommon (
   //
   if (EFI_ERROR (Status)) {
     if (Image != NULL) {
-      CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0));
+      CoreUnloadAndCloseImage (Image);
       Image = NULL;
     }
   } else if (EFI_ERROR (SecurityStatus)) {
@@ -1524,10 +1460,7 @@ CoreLoadImage (
              FilePath,
              SourceBuffer,
              SourceSize,
-             (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
              );
 
@@ -1744,7 +1677,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.
     //
@@ -1809,7 +1742,7 @@ CoreExit (
     //
     // The image has not been started so just free its resources
     //
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
     Status = EFI_SUCCESS;
     goto Done;
   }
@@ -1911,7 +1844,7 @@ CoreUnloadImage (
     //
     // if the Image was not started or Unloaded O.K. then clean up
     //
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
   }
 
 Done:
-- 
2.17.1

Re: [EXTERNAL] [edk2-devel] [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC

[Bret Barkelew]

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

Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>

- Bret

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard Biesheuvel via groups.io <ard.biesheuvel=arm.com@groups.io>
Sent: Thursday, April 9, 2020 9:09:47 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: jian.j.wang@intel.com <jian.j.wang@intel.com>; hao.a.wu@intel.com <hao.a.wu@intel.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: [EXTERNAL] [edk2-devel] [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC

Use static linkage for functions that are only used locally, to help
the compiler optimize away unused code in non-LTO builds.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index d86da89ee704..aae2c1eaa516 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -86,6 +86,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED MACHINE_TYPE_INFO  mMachineTypeInfo[] = {

 UINT16 mDxeCoreImageMachineType = 0;

+#ifndef MDEPKG_NDEBUG
 /**
  Return machine type name.

@@ -93,6 +94,7 @@ UINT16 mDxeCoreImageMachineType = 0;

  @return machine type name
 **/
+STATIC
 CHAR16 *
 GetMachineTypeName (
   UINT16 MachineType
@@ -108,6 +110,7 @@ GetMachineTypeName (

   return L"<Unknown>";
 }
+#endif

 /**
   Notification event handler registered by CoreInitializeImageServices () to
@@ -286,6 +289,7 @@ CoreInitializeImageServices (
                                  into buffer.

 **/
+STATIC
 EFI_STATUS
 EFIAPI
 CoreReadImageFile (
@@ -334,6 +338,7 @@ CoreReadImageFile (
   @retval EFI_SUCCESS              The memory range the image will be loaded in is available
   @retval EFI_NOT_FOUND            The memory range the image will be loaded in is not available
 **/
+STATIC
 EFI_STATUS
 CheckAndMarkFixLoadingMemoryUsageBitMap (
   IN  EFI_PHYSICAL_ADDRESS          ImageBase,
@@ -407,6 +412,7 @@ CheckAndMarkFixLoadingMemoryUsageBitMap (
   @retval EFI_NOT_FOUND             The image has no assigned fixed loading address.

 **/
+STATIC
 EFI_STATUS
 GetPeCoffImageFixLoadingAssignedAddress(
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
@@ -552,6 +558,7 @@ CoreIsImageTypeSupported (
   @retval EFI_BUFFER_TOO_SMALL    Buffer for image is too small

 **/
+STATIC
 EFI_STATUS
 CoreLoadPeImage (
   IN BOOLEAN                     BootPolicy,
@@ -877,6 +884,7 @@ CoreLoadPeImage (
   @return Return the image private data associated with ImageHandle.

 **/
+STATIC
 LOADED_IMAGE_PRIVATE_DATA *
 CoreLoadedImageInfo (
   IN EFI_HANDLE  ImageHandle
@@ -909,6 +917,7 @@ CoreLoadedImageInfo (
   @param  FreePage                Free allocated pages

 **/
+STATIC
 VOID
 CoreUnloadAndCloseImage (
   IN LOADED_IMAGE_PRIVATE_DATA  *Image,
@@ -1105,6 +1114,7 @@ CoreUnloadAndCloseImage (
                                   platform policy specifies that the image should not be started.

 **/
+STATIC
 EFI_STATUS
 CoreLoadImageCommon (
   IN  BOOLEAN                          BootPolicy,
--
2.17.1





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

Re: [EXTERNAL] [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments

[Bret Barkelew]

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

Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>

- Bret

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard Biesheuvel via groups.io <ard.biesheuvel=arm.com@groups.io>
Sent: Thursday, April 9, 2020 9:09:48 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: jian.j.wang@intel.com <jian.j.wang@intel.com>; hao.a.wu@intel.com <hao.a.wu@intel.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: [EXTERNAL] [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments

Image.c defines a CoreLoadImageCommon() function that has some arguments
that are marked OPTIONAL. The function only has a single caller living in
the same file, and it does not pass any arguments for these OPTIONAL
arguments.

So let's simplify the code, and remove these arguments and all the
transitive conditional dependencies on them.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

NOTE: this patch was generated with the -w option, to suppress indentation
changes from cluttering the diff.

 MdeModulePkg/Core/Dxe/Image/Image.c | 81 ++------------------
 1 file changed, 7 insertions(+), 74 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index aae2c1eaa516..9fdde87f2df3 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -564,13 +564,10 @@ CoreLoadPeImage (
   IN BOOLEAN                     BootPolicy,
   IN VOID                        *Pe32Handle,
   IN LOADED_IMAGE_PRIVATE_DATA   *Image,
-  IN EFI_PHYSICAL_ADDRESS        DstBuffer    OPTIONAL,
-  OUT EFI_PHYSICAL_ADDRESS       *EntryPoint  OPTIONAL,
   IN  UINT32                     Attribute
   )
 {
   EFI_STATUS                Status;
-  BOOLEAN                   DstBufAlocated;
   UINTN                     Size;

   ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext));
@@ -619,15 +616,6 @@ CoreLoadPeImage (
     return EFI_UNSUPPORTED;
   }

-  //
-  // Allocate memory of the correct memory type aligned on the required image boundary
-  //
-  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 {
@@ -685,31 +673,6 @@ CoreLoadPeImage (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-    DstBufAlocated = TRUE;
-  } 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->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;
-    }
-
-    Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
-    Image->ImageContext.ImageAddress = DstBuffer;
-  }

   Image->ImageBasePage = Image->ImageContext.ImageAddress;
   if (!Image->ImageContext.IsTeImage) {
@@ -790,13 +753,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
   //
@@ -861,11 +817,9 @@ CoreLoadPeImage (
   // Free memory.
   //

-  if (DstBufAlocated) {
   CoreFreePages (Image->ImageContext.ImageAddress, Image->NumberOfPages);
   Image->ImageContext.ImageAddress = 0;
   Image->ImageBasePage = 0;
-  }

   if (Image->ImageContext.FixupData != NULL) {
     CoreFreePool (Image->ImageContext.FixupData);
@@ -920,8 +874,7 @@ CoreLoadedImageInfo (
 STATIC
 VOID
 CoreUnloadAndCloseImage (
-  IN LOADED_IMAGE_PRIVATE_DATA  *Image,
-  IN BOOLEAN                    FreePage
+  IN LOADED_IMAGE_PRIVATE_DATA  *Image
   )
 {
   EFI_STATUS                          Status;
@@ -1047,7 +1000,7 @@ CoreUnloadAndCloseImage (
   //
   // Free the Image from memory
   //
-  if ((Image->ImageBasePage != 0) && FreePage) {
+  if ((Image->ImageBasePage != 0)) {
     CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
   }

@@ -1122,10 +1075,7 @@ 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,
-  OUT EFI_PHYSICAL_ADDRESS             *EntryPoint         OPTIONAL,
   IN  UINT32                           Attribute
   )
 {
@@ -1330,12 +1280,7 @@ CoreLoadImageCommon (
   Image->Info.FilePath     = DuplicateDevicePath (FilePath);
   Image->Info.ParentHandle = ParentImageHandle;

-
-  if (NumberOfPages != NULL) {
-    Image->NumberOfPages = *NumberOfPages ;
-  } else {
   Image->NumberOfPages = 0 ;
-  }

   //
   // Install the protocol interfaces for this image
@@ -1355,20 +1300,11 @@ CoreLoadImageCommon (
   //
   // Load the image.  If EntryPoint is Null, it will not be set.
   //
-  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, EntryPoint, 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
   //
@@ -1448,7 +1384,7 @@ CoreLoadImageCommon (
   //
   if (EFI_ERROR (Status)) {
     if (Image != NULL) {
-      CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0));
+      CoreUnloadAndCloseImage (Image);
       Image = NULL;
     }
   } else if (EFI_ERROR (SecurityStatus)) {
@@ -1524,10 +1460,7 @@ CoreLoadImage (
              FilePath,
              SourceBuffer,
              SourceSize,
-             (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
              );

@@ -1744,7 +1677,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.
     //
@@ -1809,7 +1742,7 @@ CoreExit (
     //
     // The image has not been started so just free its resources
     //
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
     Status = EFI_SUCCESS;
     goto Done;
   }
@@ -1911,7 +1844,7 @@ CoreUnloadImage (
     //
     // if the Image was not started or Unloaded O.K. then clean up
     //
-    CoreUnloadAndCloseImage (Image, TRUE);
+    CoreUnloadAndCloseImage (Image);
   }

 Done:
--
2.17.1





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

Re: [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions

[Liming Gao]

Ard:
  What's purpose for this change? Only code clean up? Is there any other benefit?

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Friday, April 10, 2020 12:10 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions
> 
> A pair of cleanup patches for the LoadImage/StartImage implementations
> in DxeCore.
> 
> https://github.com/ardbiesheuvel/edk2/pull/new/dxecore-loadimage-cleanup
> 
> Ard Biesheuvel (2):
>   MdeModulePkg/DxeCore/Image: make local functions STATIC
>   MdeModulePkg/DxeCore/Image: remove unused function arguments
> 
>  MdeModulePkg/Core/Dxe/Image/Image.c | 91 ++++----------------
>  1 file changed, 17 insertions(+), 74 deletions(-)
> 
> --
> 2.17.1
> 
> 
> 

Re: [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions

[Ard Biesheuvel]

On 4/10/20 3:59 PM, Gao, Liming wrote:
> Ard:
>    What's purpose for this change? Only code clean up? Is there any other benefit?
> 

Just code cleanup.

>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
>> Sent: Friday, April 10, 2020 12:10 AM
>> To: devel@edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Subject: [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions
>>
>> A pair of cleanup patches for the LoadImage/StartImage implementations
>> in DxeCore.
>>
>> https://github.com/ardbiesheuvel/edk2/pull/new/dxecore-loadimage-cleanup
>>
>> Ard Biesheuvel (2):
>>    MdeModulePkg/DxeCore/Image: make local functions STATIC
>>    MdeModulePkg/DxeCore/Image: remove unused function arguments
>>
>>   MdeModulePkg/Core/Dxe/Image/Image.c | 91 ++++----------------
>>   1 file changed, 17 insertions(+), 74 deletions(-)
>>
>> --
>> 2.17.1
>>
>>
>> 
> 

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments

[Dandan Bi]

Hi Ard,


Some minor comments,
1. Could you please also remove the arguments in related function comments?
2. Please make the code aligned after removing some condition check.


Thanks,
Dandan
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 10, 2020 12:10 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove
> unused function arguments
> 
> Image.c defines a CoreLoadImageCommon() function that has some
> arguments that are marked OPTIONAL. The function only has a single caller
> living in the same file, and it does not pass any arguments for these
> OPTIONAL arguments.
> 
> So let's simplify the code, and remove these arguments and all the transitive
> conditional dependencies on them.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> 
> NOTE: this patch was generated with the -w option, to suppress indentation
> changes from cluttering the diff.
> 
>  MdeModulePkg/Core/Dxe/Image/Image.c | 81 ++------------------
>  1 file changed, 7 insertions(+), 74 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index aae2c1eaa516..9fdde87f2df3 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -564,13 +564,10 @@ CoreLoadPeImage (
>    IN BOOLEAN                     BootPolicy,
>    IN VOID                        *Pe32Handle,
>    IN LOADED_IMAGE_PRIVATE_DATA   *Image,
> -  IN EFI_PHYSICAL_ADDRESS        DstBuffer    OPTIONAL,
> -  OUT EFI_PHYSICAL_ADDRESS       *EntryPoint  OPTIONAL,
>    IN  UINT32                     Attribute
>    )
>  {
>    EFI_STATUS                Status;
> -  BOOLEAN                   DstBufAlocated;
>    UINTN                     Size;
> 
>    ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext)); @@ -
> 619,15 +616,6 @@ CoreLoadPeImage (
>      return EFI_UNSUPPORTED;
>    }
> 
> -  //
> -  // Allocate memory of the correct memory type aligned on the required
> image boundary
> -  //
> -  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 {
> @@ -685,31 +673,6 @@ CoreLoadPeImage (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -    DstBufAlocated = TRUE;
> -  } 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->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;
> -    }
> -
> -    Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image-
> >ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
> -    Image->ImageContext.ImageAddress = DstBuffer;
> -  }
> 
>    Image->ImageBasePage = Image->ImageContext.ImageAddress;
>    if (!Image->ImageContext.IsTeImage) { @@ -790,13 +753,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
>    //
> @@ -861,11 +817,9 @@ CoreLoadPeImage (
>    // Free memory.
>    //
> 
> -  if (DstBufAlocated) {
>    CoreFreePages (Image->ImageContext.ImageAddress, Image-
> >NumberOfPages);
>    Image->ImageContext.ImageAddress = 0;
>    Image->ImageBasePage = 0;
> -  }
> 
>    if (Image->ImageContext.FixupData != NULL) {
>      CoreFreePool (Image->ImageContext.FixupData); @@ -920,8 +874,7 @@
> CoreLoadedImageInfo (  STATIC  VOID  CoreUnloadAndCloseImage (
> -  IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> -  IN BOOLEAN                    FreePage
> +  IN LOADED_IMAGE_PRIVATE_DATA  *Image
>    )
>  {
>    EFI_STATUS                          Status;
> @@ -1047,7 +1000,7 @@ CoreUnloadAndCloseImage (
>    //
>    // Free the Image from memory
>    //
> -  if ((Image->ImageBasePage != 0) && FreePage) {
> +  if ((Image->ImageBasePage != 0)) {
>      CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
>    }
> 
> @@ -1122,10 +1075,7 @@ 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,
> -  OUT EFI_PHYSICAL_ADDRESS             *EntryPoint         OPTIONAL,
>    IN  UINT32                           Attribute
>    )
>  {
> @@ -1330,12 +1280,7 @@ CoreLoadImageCommon (
>    Image->Info.FilePath     = DuplicateDevicePath (FilePath);
>    Image->Info.ParentHandle = ParentImageHandle;
> 
> -
> -  if (NumberOfPages != NULL) {
> -    Image->NumberOfPages = *NumberOfPages ;
> -  } else {
>    Image->NumberOfPages = 0 ;
> -  }
> 
>    //
>    // Install the protocol interfaces for this image @@ -1355,20 +1300,11 @@
> CoreLoadImageCommon (
>    //
>    // Load the image.  If EntryPoint is Null, it will not be set.
>    //
> -  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer,
> EntryPoint, 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
>    //
> @@ -1448,7 +1384,7 @@ CoreLoadImageCommon (
>    //
>    if (EFI_ERROR (Status)) {
>      if (Image != NULL) {
> -      CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0));
> +      CoreUnloadAndCloseImage (Image);
>        Image = NULL;
>      }
>    } else if (EFI_ERROR (SecurityStatus)) { @@ -1524,10 +1460,7 @@
> CoreLoadImage (
>               FilePath,
>               SourceBuffer,
>               SourceSize,
> -             (EFI_PHYSICAL_ADDRESS) (UINTN) NULL,
> -             NULL,
>               ImageHandle,
> -             NULL,
>               EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION |
> EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRA
> TION
>               );
> 
> @@ -1744,7 +1677,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.
>      //
> @@ -1809,7 +1742,7 @@ CoreExit (
>      //
>      // The image has not been started so just free its resources
>      //
> -    CoreUnloadAndCloseImage (Image, TRUE);
> +    CoreUnloadAndCloseImage (Image);
>      Status = EFI_SUCCESS;
>      goto Done;
>    }
> @@ -1911,7 +1844,7 @@ CoreUnloadImage (
>      //
>      // if the Image was not started or Unloaded O.K. then clean up
>      //
> -    CoreUnloadAndCloseImage (Image, TRUE);
> +    CoreUnloadAndCloseImage (Image);
>    }
> 
>  Done:
> --
> 2.17.1
> 
> 
>