Reviewed-by: Ray Ni <ray...@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 <a...@kernel.org>
> Sent: Monday, May 29, 2023 6:17 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <a...@kernel.org>; Ni, Ray <ray...@intel.com>; Yao, Jiewen
> <jiewen....@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Taylor Beebe
> <t...@taylorbeebe.com>; Oliver Smith-Denny <o...@smith-denny.com>; Bi, Dandan
> <dandan...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Kinney,
> Michael D <michael.d.kin...@intel.com>; Leif Lindholm
> <quic_llind...@quicinc.com>; Michael Kubacki <mikub...@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 <a...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105406): https://edk2.groups.io/g/devel/message/105406
Mute This Topic: https://groups.io/mt/99197134/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
