* [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions
@ 2020-04-09 16:09 Ard Biesheuvel
2020-04-09 16:09 ` [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2020-04-09 16:09 UTC (permalink / raw)
To: devel; +Cc: jian.j.wang, hao.a.wu, 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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC 2020-04-09 16:09 [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Ard Biesheuvel @ 2020-04-09 16:09 ` Ard Biesheuvel 2020-04-09 19:13 ` [EXTERNAL] [edk2-devel] " Bret Barkelew 2020-04-09 16:09 ` [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments Ard Biesheuvel 2020-04-10 13:59 ` [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Liming Gao 2 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2020-04-09 16:09 UTC (permalink / raw) To: devel; +Cc: jian.j.wang, hao.a.wu, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC 2020-04-09 16:09 ` [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC Ard Biesheuvel @ 2020-04-09 19:13 ` Bret Barkelew 0 siblings, 0 replies; 8+ messages in thread From: Bret Barkelew @ 2020-04-09 19:13 UTC (permalink / raw) To: devel@edk2.groups.io, ard.biesheuvel@arm.com Cc: jian.j.wang@intel.com, hao.a.wu@intel.com, Ard Biesheuvel [-- 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 --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments 2020-04-09 16:09 [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Ard Biesheuvel 2020-04-09 16:09 ` [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC Ard Biesheuvel @ 2020-04-09 16:09 ` Ard Biesheuvel 2020-04-09 19:14 ` [EXTERNAL] [edk2-devel] " Bret Barkelew 2020-06-01 8:13 ` Dandan Bi 2020-04-10 13:59 ` [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Liming Gao 2 siblings, 2 replies; 8+ messages in thread From: Ard Biesheuvel @ 2020-04-09 16:09 UTC (permalink / raw) To: devel; +Cc: jian.j.wang, hao.a.wu, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments 2020-04-09 16:09 ` [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments Ard Biesheuvel @ 2020-04-09 19:14 ` Bret Barkelew 2020-06-01 8:13 ` Dandan Bi 1 sibling, 0 replies; 8+ messages in thread From: Bret Barkelew @ 2020-04-09 19:14 UTC (permalink / raw) To: devel@edk2.groups.io, ard.biesheuvel@arm.com Cc: jian.j.wang@intel.com, hao.a.wu@intel.com, Ard Biesheuvel [-- 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 --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments 2020-04-09 16:09 ` [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments Ard Biesheuvel 2020-04-09 19:14 ` [EXTERNAL] [edk2-devel] " Bret Barkelew @ 2020-06-01 8:13 ` Dandan Bi 1 sibling, 0 replies; 8+ messages in thread From: Dandan Bi @ 2020-06-01 8:13 UTC (permalink / raw) To: devel@edk2.groups.io, ard.biesheuvel@arm.com; +Cc: Wang, Jian J, Wu, Hao A 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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions 2020-04-09 16:09 [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Ard Biesheuvel 2020-04-09 16:09 ` [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC Ard Biesheuvel 2020-04-09 16:09 ` [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments Ard Biesheuvel @ 2020-04-10 13:59 ` Liming Gao 2020-04-10 14:20 ` Ard Biesheuvel 2 siblings, 1 reply; 8+ messages in thread From: Liming Gao @ 2020-04-10 13:59 UTC (permalink / raw) To: devel@edk2.groups.io, ard.biesheuvel@arm.com Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan 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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions 2020-04-10 13:59 ` [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Liming Gao @ 2020-04-10 14:20 ` Ard Biesheuvel 0 siblings, 0 replies; 8+ messages in thread From: Ard Biesheuvel @ 2020-04-10 14:20 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io; +Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan 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 >> >> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-01 8:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-09 16:09 [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Ard Biesheuvel 2020-04-09 16:09 ` [PATCH 1/2] MdeModulePkg/DxeCore/Image: make local functions STATIC Ard Biesheuvel 2020-04-09 19:13 ` [EXTERNAL] [edk2-devel] " Bret Barkelew 2020-04-09 16:09 ` [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments Ard Biesheuvel 2020-04-09 19:14 ` [EXTERNAL] [edk2-devel] " Bret Barkelew 2020-06-01 8:13 ` Dandan Bi 2020-04-10 13:59 ` [edk2-devel] [PATCH 0/2] MdeModulePkg/DxeCore: clean up image loading functions Liming Gao 2020-04-10 14:20 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox