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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 13 September 2018 at 12:23, Zeng, Star <star.zeng@intel.com> wrote:
> On 2018/9/12 21:21, Ard Biesheuvel wrote:
>>
>> 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.0
>> 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 | 39 +++++++++++++++++---
>>   3 files changed, 37 insertions(+), 6 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
>> b/MdeModulePkg/Core/Dxe/DxeMain.h
>> index 7ec82388a3f9..57b3861d9813 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>
>> @@ -229,6 +230,8 @@ typedef struct {
>>     UINT16                      Machine;
>>     /// EBC Protocol pointer
>>     EFI_EBC_PROTOCOL            *Ebc;
>> +  /// PE/COFF Image Emulator Protocol pointer
>> +  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *Emu;
>
>
> Hi Ard,
>
> How about using PeCoffEmu as the name to be more specific?
>

Good idea.

>
>>     /// 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 68fa0a01d9bd..d7591aa0da6d 100644
>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>> @@ -180,6 +180,7 @@
>>     gEfiVariableArchProtocolGuid                  ## CONSUMES
>>     gEfiCapsuleArchProtocolGuid                   ## CONSUMES
>>     gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
>> +  gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES
>>     [FeaturePcd]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport     ##
>> CONSUMES
>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
>> b/MdeModulePkg/Core/Dxe/Image/Image.c
>> index eddca140ee1a..e2dd80790657 100644
>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>> @@ -67,6 +67,7 @@ LOADED_IMAGE_PRIVATE_DATA mCorePrivateImage  = {
>>     NULL,                       // JumpContext
>>     0,                          // Machine
>>     NULL,                       // Ebc
>> +  NULL,                       // Emu
>>     NULL,                       // RuntimeData
>>     NULL                        // LoadedImageDevicePath
>>   };
>> @@ -476,12 +477,23 @@ CoreLoadPeImage (
>>     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.
>> +      // Locate the emulator protocol to check whether it supports this
>> +      // image.
>>         //
>> -      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;
>> +      Status = CoreLocateProtocol
>> (&gEdkiiPeCoffImageEmulatorProtocolGuid,
>> +                 NULL, (VOID **)&Image->Emu);
>> +      if (EFI_ERROR (Status) ||
>> +          !Image->Emu->IsImageSupported (Image->Emu,
>> +                                         Image->ImageContext.Machine,
>> +                                         Image->ImageContext.ImageType))
>> {
>> +        //
>> +        // 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 +699,14 @@ CoreLoadPeImage (
>>       if (EFI_ERROR(Status)) {
>>         goto Done;
>>       }
>> +  } else if (Image->Emu != NULL) {
>> +    Status = Image->Emu->RegisterImage (Image->Emu, Image->ImageBasePage,
>> +                           EFI_PAGES_TO_SIZE (Image->NumberOfPages));
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
>> +        "CoreLoadPeImage: Failed to load register foreign image with
>> emulator.\n"));
>
>
> 'load' should not be in the sentence, right?
>

Correct. I will remove it.

>> +      goto Done;
>> +    }
>>     }
>>       //
>> @@ -874,6 +894,13 @@ CoreUnloadAndCloseImage (
>>       Image->Ebc->UnloadImage (Image->Ebc, Image->Handle);
>>     }
>>   +  if (Image->Emu != NULL) {
>> +    //
>> +    // If the PE/COFF Emulator protocol exists we must unregister the
>> image.
>> +    //
>> +    Image->Emu->UnregisterImage (Image->Emu, Image->ImageBasePage);
>> +  }
>> +
>>     //
>>     // Unload image, free Image->ImageContext->ModHandle
>>     //
>> @@ -1599,7 +1626,7 @@ 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->Emu ==
>> NULL) {
>>       //
>>       // Do not ASSERT here, because image might be loaded via
>> EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED
>>       // But it can not be started.
>>
>