Re: [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy

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

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