Re: [PATCH v5 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images

["Michael D Kinney" <michael.d.kinney@intel.com>]

Ard,

A could small comments below on use of sizeof().

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, April 11, 2019 11:58 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Zimmer,
> Vincent <vincent.zimmer@intel.com>; Richardson, Brian
> <brian.richardson@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Andrew Fish
> <afish@apple.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>; Shi, Steven
> <steven.shi@intel.com>
> Subject: [PATCH v5 2/7] MdeModulePkg/DxeCore: invoke
> the emulator protocol for foreign images
> 
> When encountering PE/COFF images that cannot be
> supported natively,
> attempt to locate an instance of the PE/COFF image
> emulator protocol,
> and if it supports the image, proceed with loading it
> and register it
> with the emulator.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h     |   3 +
>  MdeModulePkg/Core/Dxe/DxeMain.inf   |   1 +
>  MdeModulePkg/Core/Dxe/Image/Image.c | 170
> ++++++++++++++++++--
>  3 files changed, 163 insertions(+), 11 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 2dec9da5e35b..48ec30a48aa2 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -53,6 +53,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS
> OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Protocol/TcgService.h>
>  #include <Protocol/HiiPackageList.h>
>  #include <Protocol/SmmBase2.h>
> +#include <Protocol/PeCoffImageEmulator.h>
>  #include <Guid/MemoryTypeInformation.h>
>  #include <Guid/FirmwareFileSystem2.h>
>  #include <Guid/FirmwareFileSystem3.h>
> @@ -228,6 +229,8 @@ typedef struct {
>    UINT16                      Machine;
>    /// EBC Protocol pointer
>    EFI_EBC_PROTOCOL            *Ebc;
> +  /// PE/COFF Image Emulator Protocol pointer
> +  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *PeCoffEmu;
>    /// Runtime image list
>    EFI_RUNTIME_IMAGE_ENTRY     *RuntimeData;
>    /// Pointer to Loaded Image Device Path Protocol
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 10375443c0f4..ce6fc19be5e4 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -162,6 +162,7 @@
>    gEfiHiiPackageListProtocolGuid                ##
> SOMETIMES_PRODUCES
>    gEfiEbcProtocolGuid                           ##
> SOMETIMES_CONSUMES
>    gEfiSmmBase2ProtocolGuid                      ##
> SOMETIMES_CONSUMES
> +  gEdkiiPeCoffImageEmulatorProtocolGuid         ##
> SOMETIMES_CONSUMES
> 
>    # Arch Protocols
>    gEfiBdsArchProtocolGuid                       ##
> CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index eddca140ee1a..d00e50747040 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -29,6 +29,15 @@ LOAD_PE32_IMAGE_PRIVATE_DATA
> mLoadPe32PrivateData = {
>    }
>  };
> 
> +typedef struct {
> +  LIST_ENTRY                            Link;
> +  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *Emulator;
> +  UINT16                                MachineType;
> +} EMULATOR_ENTRY;
> +
> +STATIC LIST_ENTRY
> mAvailableEmulators;
> +STATIC EFI_EVENT
> mPeCoffEmuProtocolRegistrationEvent;
> +STATIC VOID
> *mPeCoffEmuProtocolNotifyRegistration;
> 
>  //
>  // This code is needed to build the Image handle for
> the DXE Core
> @@ -67,6 +76,7 @@ LOADED_IMAGE_PRIVATE_DATA
> mCorePrivateImage  = {
>    NULL,                       // JumpContext
>    0,                          // Machine
>    NULL,                       // Ebc
> +  NULL,                       // PeCoffEmu
>    NULL,                       // RuntimeData
>    NULL                        // LoadedImageDevicePath
>  };
> @@ -118,6 +128,61 @@ GetMachineTypeName (
>    return L"<Unknown>";
>  }
> 
> +/**
> +  Notification event handler registered by
> CoreInitializeImageServices () to
> +  keep track of which PE/COFF image emulators are
> available.
> +
> +  @param  Event          The Event that is being
> processed, not used.
> +  @param  Context        Event Context, not used.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PeCoffEmuProtocolNotify (
> +  IN  EFI_EVENT       Event,
> +  IN  VOID            *Context
> +  )
> +{
> +  EFI_STATUS          Status;
> +  UINTN               BufferSize;
> +  EFI_HANDLE          EmuHandle;
> +  EMULATOR_ENTRY      *Entry;
> +
> +  EmuHandle = NULL;
> +
> +  while (TRUE) {
> +    BufferSize = sizeof EmuHandle;

Should be sizeof (EmuHandle);

> +    Status = CoreLocateHandle (
> +               ByRegisterNotify,
> +               NULL,
> +               mPeCoffEmuProtocolNotifyRegistration,
> +               &BufferSize,
> +               &EmuHandle
> +               );
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // If no more notification events exit
> +      //
> +      return;
> +    }
> +
> +    Entry = AllocateZeroPool (sizeof *Entry);

Should be sizeof (*Entry)


> +    ASSERT (Entry != NULL);
> +
> +    Status = CoreHandleProtocol (
> +               EmuHandle,
> +               &gEdkiiPeCoffImageEmulatorProtocolGuid,
> +               (VOID **)&Entry->Emulator
> +               );
> +    ASSERT_EFI_ERROR (Status);
> +
> +    Entry->MachineType = Entry->Emulator->MachineType;
> +
> +    InsertTailList (&mAvailableEmulators, &Entry-
> >Link);
> +  }
> +}
> +
>  /**
>    Add the Image Services to EFI Boot Services Table
> and install the protocol
>    interfaces for this image.
> @@ -192,6 +257,30 @@ CoreInitializeImageServices (
>    gDxeCoreImageHandle = Image->Handle;
>    gDxeCoreLoadedImage = &Image->Info;
> 
> +  //
> +  // Create the PE/COFF emulator protocol registration
> event
> +  //
> +  Status = CoreCreateEvent (
> +             EVT_NOTIFY_SIGNAL,
> +             TPL_CALLBACK,
> +             PeCoffEmuProtocolNotify,
> +             NULL,
> +             &mPeCoffEmuProtocolRegistrationEvent
> +             );
> +  ASSERT_EFI_ERROR(Status);
> +
> +  //
> +  // Register for protocol notifications on this event
> +  //
> +  Status = CoreRegisterProtocolNotify (
> +             &gEdkiiPeCoffImageEmulatorProtocolGuid,
> +             mPeCoffEmuProtocolRegistrationEvent,
> +             &mPeCoffEmuProtocolNotifyRegistration
> +             );
> +  ASSERT_EFI_ERROR(Status);
> +
> +  InitializeListHead (&mAvailableEmulators);
> +
>    if (FeaturePcdGet
> (PcdFrameworkCompatibilitySupport)) {
>      //
>      // Export DXE Core PE Loader functionality for
> backward compatibility.
> @@ -425,6 +514,49 @@
> GetPeCoffImageFixLoadingAssignedAddress(
>     DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE
> FIXED INFO: Loading module at fixed address 0x%11p.
> Status = %r \n", (VOID *)(UINTN)(ImageContext-
> >ImageAddress), Status));
>     return Status;
>  }
> +
> +/**
> +  Decides whether a PE/COFF image can execute on this
> system, either natively
> +  or via emulation/interpretation. In the latter case,
> the PeCoffEmu member
> +  of the LOADED_IMAGE_PRIVATE_DATA struct pointer is
> populated with a pointer
> +  to the emulator protocol that supports this image.
> +
> +  @param[in]  Image         LOADED_IMAGE_PRIVATE_DATA
> struct pointer
> +
> +  @retval     TRUE          The image is supported
> +  @retval     FALSE         The image is not supported
> +
> +**/
> +STATIC
> +BOOLEAN
> +CoreIsImageTypeSupported (
> +  IN OUT LOADED_IMAGE_PRIVATE_DATA  *Image
> +  )
> +{
> +  LIST_ENTRY                        *Link;
> +  EMULATOR_ENTRY                    *Entry;
> +
> +  for (Link = GetFirstNode (&mAvailableEmulators);
> +       !IsNull (&mAvailableEmulators, Link);
> +       Link = GetNextNode (&mAvailableEmulators,
> Link)) {
> +
> +    Entry = BASE_CR (Link, EMULATOR_ENTRY, Link);
> +    if (Entry->MachineType != Image-
> >ImageContext.Machine) {
> +      continue;
> +    }
> +
> +    if (Entry->Emulator->IsImageSupported (Entry-
> >Emulator,
> +                           Image-
> >ImageContext.ImageType,
> +                           Image->Info.FilePath)) {
> +      Image->PeCoffEmu = Entry->Emulator;
> +      return TRUE;
> +    }
> +  }
> +
> +  return EFI_IMAGE_MACHINE_TYPE_SUPPORTED (Image-
> >ImageContext.Machine) ||
> +         EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED
> (Image->ImageContext.Machine);
> +}
> +
>  /**
>    Loads, relocates, and invokes a PE/COFF image
> 
> @@ -473,16 +605,14 @@ CoreLoadPeImage (
>      return Status;
>    }
> 
> -  if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (Image-
> >ImageContext.Machine)) {
> -    if (!EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED
> (Image->ImageContext.Machine)) {
> -      //
> -      // The PE/COFF loader can support loading image
> types that can be executed.
> -      // If we loaded an image type that we can not
> execute return EFI_UNSUPORTED.
> -      //
> -      DEBUG ((EFI_D_ERROR, "Image type %s can't be
> loaded ", GetMachineTypeName(Image-
> >ImageContext.Machine)));
> -      DEBUG ((EFI_D_ERROR, "on %s UEFI system.\n",
> GetMachineTypeName(mDxeCoreImageMachineType)));
> -      return EFI_UNSUPPORTED;
> -    }
> +  if (!CoreIsImageTypeSupported (Image)) {
> +    //
> +    // The PE/COFF loader can support loading image
> types that can be executed.
> +    // If we loaded an image type that we can not
> execute return EFI_UNSUPORTED.
> +    //
> +    DEBUG ((EFI_D_ERROR, "Image type %s can't be
> loaded ", GetMachineTypeName(Image-
> >ImageContext.Machine)));
> +    DEBUG ((EFI_D_ERROR, "on %s UEFI system.\n",
> GetMachineTypeName(mDxeCoreImageMachineType)));
> +    return EFI_UNSUPPORTED;
>    }
> 
>    //
> @@ -687,6 +817,16 @@ CoreLoadPeImage (
>      if (EFI_ERROR(Status)) {
>        goto Done;
>      }
> +  } else if (Image->PeCoffEmu != NULL) {
> +    Status = Image->PeCoffEmu->RegisterImage (Image-
> >PeCoffEmu,
> +                                 Image->ImageBasePage,
> +                                 EFI_PAGES_TO_SIZE
> (Image->NumberOfPages),
> +                                 &Image->EntryPoint);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
> +        "CoreLoadPeImage: Failed to register foreign
> image with emulator.\n"));
> +      goto Done;
> +    }
>    }
> 
>    //
> @@ -874,6 +1014,13 @@ CoreUnloadAndCloseImage (
>      Image->Ebc->UnloadImage (Image->Ebc, Image-
> >Handle);
>    }
> 
> +  if (Image->PeCoffEmu != NULL) {
> +    //
> +    // If the PE/COFF Emulator protocol exists we must
> unregister the image.
> +    //
> +    Image->PeCoffEmu->UnregisterImage (Image-
> >PeCoffEmu, Image->ImageBasePage);
> +  }
> +
>    //
>    // Unload image, free Image->ImageContext->ModHandle
>    //
> @@ -1599,7 +1746,8 @@ CoreStartImage (
>    //
>    // The image to be started must have the machine
> type supported by DxeCore.
>    //
> -  if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (Image-
> >Machine)) {
> +  if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (Image-
> >Machine) &&
> +      Image->PeCoffEmu == NULL) {
>      //
>      // Do not ASSERT here, because image might be
> loaded via EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED
>      // But it can not be started.
> --
> 2.17.1