Re: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible

["Ni, Ray" <ray.ni@intel.com>]

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