public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
@ 2019-04-14 19:52 Ard Biesheuvel
  2019-04-14 19:52 ` [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol Ard Biesheuvel
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Add the basic plumbing to DXE core, the PCI bus driver and the boot manager
to allow PE/COFF images to be dispatched that target an architecture that is
not native for the platform, but which is supported by one of potentially
several available emulators.

One implementation of such an emulator can be found here:
https://github.com/ardbiesheuvel/X86EmulatorPkg/tree/upstream-v4

This also allows us to get rid of the special treatment of EBC images in
core code. Instead, the EbcDxe driver is augmented with an implementation
of the EDK2 PE/COFF image emulator protocol so that internal knowledge of
how EBC is implemented (I-cache flushing, thunks) is removed from the DXE
core.

Changes since v5:
- Fix build issues reported by Mike Kinney (#2, #5)
- Fix CheckPatch.py issue regarding the use of EFI_D_ERROR (#2) [This is code
  that is only being moved around but I fixed it nonetheless]
- Use InstallMultipleProtocolInterfaces() to install the EBC protocol and the
  PE/COFF emu protocol at the same time (to simplify/fix the error path) (#5)
- Add Mike's Reviewed-by

Changes since v4:
- Fix an issue in the protocol notify handler pointed out by Mike Kinney (#2)

Changes since v3:
- Simplify the handling of option ROMs and Driver#### images, by simply
  deferring to the LoadImage() boot service to decide whether an image
  can be supported or not - this removes some redundant checks from the
  BDS layer and the PCI bus driver.
- Move the machine type supported by the emulator into the protocol struct,
  so we can optimize away calls into the emulator for each image loaded.
  Instead, the LoadImage() code will only invoke the IsSupported() method for
  images that are known to have a matching machine type.

Note that I have considered, but ultimately dismissed the suggestion to
register and unregister emulators via a new protocol. The main issue is
that registering and unregistering structs containing sets of function
pointers is awfully similar to managing a protocol database, and we already
have the code to do that in EDK2.

So instead, I have removed all the code that iterates over a handle buffer
of emu protocols and invokes each one to see if it will support the image.
Instead, this is all done by CoreLoadImage().

Changes since v2:
- incorporate feedback from Andrew Fish (delivered in person):
  * pass a device path into the IsImageSupported() protocol method so that an
    implementation can blacklist or whitelist certain devices, or implement
    other policies that depend on the device where the driver originated
  * allow the emulator to supersede the native loading of the image - this
    permits things like X86 on X86 emulators for security sandboxing or debug

Changes since v1:
- subsume the EBC handling into the EDK2 emulator protocol and abstract
  away from EBC specifics in core code.
- allow multiple emulator implementations to co-exist
- incorporate Star's review feedback

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>

Ard Biesheuvel (7):
  MdeModulePkg: introduce PE/COFF image emulator protocol
  MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images
  MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures
  MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images
  MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol
  MdePkg/UefiBaseType.h: treat EBC as a non-native machine type
  MdeModulePkg/DxeCore: remove explicit EBC handling

 .../Bus/Pci/PciBusDxe/PciOptionRomSupport.c   |   7 -
 MdeModulePkg/Core/Dxe/DxeMain.h               |   6 +-
 MdeModulePkg/Core/Dxe/DxeMain.inf             |   2 +-
 MdeModulePkg/Core/Dxe/Image/Image.c           | 210 +++++++++++++-----
 .../Include/Protocol/PeCoffImageEmulator.h    | 107 +++++++++
 .../Library/UefiBootManagerLib/BmLoadOption.c |   6 +-
 MdeModulePkg/MdeModulePkg.dec                 |   4 +
 MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf |   3 +
 MdeModulePkg/Universal/EbcDxe/EbcDxe.inf      |   3 +
 MdeModulePkg/Universal/EbcDxe/EbcInt.c        | 121 +++++++++-
 MdeModulePkg/Universal/EbcDxe/EbcInt.h        |   3 +
 MdePkg/Include/Uefi/UefiBaseType.h            |   6 +-
 12 files changed, 402 insertions(+), 76 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-15  1:23   ` Wu, Hao A
  2019-04-14 19:52 ` [PATCH v6 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images Ard Biesheuvel
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Introduce a protocol that can be invoked by the image loading services
to execute foreign architecture PE/COFF images via an emulator.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h | 107 ++++++++++++++++++++
 MdeModulePkg/MdeModulePkg.dec                       |   4 +
 2 files changed, 111 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
new file mode 100644
index 000000000000..1ca302440e4a
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
@@ -0,0 +1,107 @@
+/** @file
+  Copyright (c) 2019, Linaro, Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
+#define PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
+
+#define EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID \
+  { 0x96F46153, 0x97A7, 0x4793, { 0xAC, 0xC1, 0xFA, 0x19, 0xBF, 0x78, 0xEA, 0x97 } }
+
+typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
+
+/**
+  Check whether the emulator supports executing a certain PE/COFF image
+
+  @param[in] This         This pointer for EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
+                          structure
+  @param[in] ImageType    Whether the image is an application, a boot time
+                          driver or a runtime driver.
+  @param[in] DevicePath   Path to device where the image originated
+                          (e.g., a PCI option ROM)
+
+  @retval TRUE            The image is supported by the emulator
+  @retval FALSE           The image is not supported by the emulator.
+**/
+typedef
+BOOLEAN
+(EFIAPI *EDKII_PECOFF_IMAGE_EMULATOR_IS_IMAGE_SUPPORTED) (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN  UINT16                                  ImageType,
+  IN  EFI_DEVICE_PATH_PROTOCOL                *DevicePath   OPTIONAL
+  );
+
+/**
+  Register a supported PE/COFF image with the emulator. After this call
+  completes successfully, the PE/COFF image may be started as usual, and
+  it is the responsibility of the emulator implementation that any branch
+  into the code section of the image (including returns from functions called
+  from the foreign code) is executed as if it were running on the machine
+  type it was built for.
+
+  @param[in]      This          This pointer for
+                                EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL structure
+  @param[in]      ImageBase     The base address in memory of the PE/COFF image
+  @param[in]      ImageSize     The size in memory of the PE/COFF image
+  @param[in,out]  EntryPoint    The entry point of the PE/COFF image. Passed by
+                                reference so that the emulator may modify it.
+
+  @retval EFI_SUCCESS           The image was registered with the emulator and
+                                can be started as usual.
+  @retval other                 The image could not be registered.
+
+  If the PE/COFF machine type or image type are not supported by the emulator,
+  then ASSERT().
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_PECOFF_IMAGE_EMULATOR_REGISTER_IMAGE) (
+  IN      EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN      EFI_PHYSICAL_ADDRESS                    ImageBase,
+  IN      UINT64                                  ImageSize,
+  IN  OUT EFI_IMAGE_ENTRY_POINT                   *EntryPoint
+  );
+
+/**
+  Unregister a PE/COFF image that has been registered with the emulator.
+  This should be done before the image is unloaded from memory.
+
+  @param[in] This         This pointer for EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
+                          structure
+  @param[in] ImageBase    The base address in memory of the PE/COFF image
+
+  @retval EFI_SUCCESS     The image was unregistered with the emulator.
+  @retval other           Image could not be unloaded.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_PECOFF_IMAGE_EMULATOR_UNREGISTER_IMAGE) (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN  EFI_PHYSICAL_ADDRESS                    ImageBase
+  );
+
+#define EDKII_PECOFF_IMAGE_EMULATOR_VERSION         0x1
+
+typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL {
+  EDKII_PECOFF_IMAGE_EMULATOR_IS_IMAGE_SUPPORTED    IsImageSupported;
+  EDKII_PECOFF_IMAGE_EMULATOR_REGISTER_IMAGE        RegisterImage;
+  EDKII_PECOFF_IMAGE_EMULATOR_UNREGISTER_IMAGE      UnregisterImage;
+
+  // Protocol version implemented by the emulator
+  UINT32                                            Version;
+  // The machine type implemented by the emulator
+  UINT16                                            MachineType;
+} EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
+
+extern EFI_GUID gEdkiiPeCoffImageEmulatorProtocolGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2130bc43991..c2b4e7f69367 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -629,6 +629,10 @@
 
   ## Include/Protocol/AtaAtapiPolicy.h
   gEdkiiAtaAtapiPolicyProtocolGuid = { 0xe59cd769, 0x5083, 0x4f26,{ 0x90, 0x94, 0x6c, 0x91, 0x9f, 0x91, 0x6c, 0x4e } }
+
+  ## Include/Protocol/PeCoffImageEmulator.h
+  gEdkiiPeCoffImageEmulatorProtocolGuid = { 0x96f46153, 0x97a7, 0x4793, { 0xac, 0xc1, 0xfa, 0x19, 0xbf, 0x78, 0xea, 0x97 } }
+
 #
 # [Error.gEfiMdeModulePkgTokenSpaceGuid]
 #   0x80000001 | Invalid value provided.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
  2019-04-14 19:52 ` [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-15  1:23   ` Wu, Hao A
  2019-04-14 19:52 ` [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures Ard Biesheuvel
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.h     |   3 +
 MdeModulePkg/Core/Dxe/DxeMain.inf   |   1 +
 MdeModulePkg/Core/Dxe/Image/Image.c | 171 ++++++++++++++++++--
 3 files changed, 164 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..57330636b5f3 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);
+    Status = CoreLocateHandle (
+               ByRegisterNotify,
+               NULL,
+               mPeCoffEmuProtocolNotifyRegistration,
+               &BufferSize,
+               &EmuHandle
+               );
+    if (EFI_ERROR (Status)) {
+      //
+      // If no more notification events exit
+      //
+      return;
+    }
+
+    Entry = AllocateZeroPool (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,15 @@ 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_UNSUPPORTED.
+    //
+    DEBUG ((DEBUG_ERROR, "Image type %s can't be loaded on %s UEFI system.\n",
+      GetMachineTypeName (Image->ImageContext.Machine),
+      GetMachineTypeName (mDxeCoreImageMachineType)));
+    return EFI_UNSUPPORTED;
   }
 
   //
@@ -687,6 +818,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 +1015,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 +1747,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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
  2019-04-14 19:52 ` [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol Ard Biesheuvel
  2019-04-14 19:52 ` [PATCH v6 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-15 20:23   ` [edk2-devel] " Ni, Ray
  2019-04-16  6:02   ` Gary Lin
  2019-04-14 19:52 ` [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images Ard Biesheuvel
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Delete the explicit machine type check for option ROM images, and instead,
rely on the LoadImage() boot service to decide whether an option ROM can
be dispatched or not. This permits platforms to ship with emulators to
execute option ROMs that are not native to the processor architecture.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
index c75ef1a82505..54cf4251cc86 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
@@ -699,13 +699,6 @@ ProcessOpRomImage (
       goto NextImage;
     }
 
-    //
-    // Skip the EFI PCI Option ROM image if its machine type is not supported
-    //
-    if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (EfiRomHeader->EfiMachineType)) {
-      goto NextImage;
-    }
-
     //
     // Ignore the EFI PCI Option ROM image if it is an EFI application
     //
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2019-04-14 19:52 ` [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-15 20:22   ` [edk2-devel] " Ni, Ray
  2019-04-14 19:52 ` [PATCH v6 5/7] MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol Ard Biesheuvel
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Allow PE/COFF images that must execute under emulation for Driver####
options, by removing the redundant machine type check from the BDS code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
index 7bf96646c690..8e6caaa63548 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
@@ -1233,10 +1233,8 @@ BmIsLoadOptionPeHeaderValid (
       // Check PE32 or PE32+ magic, and machine type
       //
       OptionalHeader = (EFI_IMAGE_OPTIONAL_HEADER32 *) &PeHeader->Pe32.OptionalHeader;
-      if ((OptionalHeader->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC ||
-           OptionalHeader->Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) &&
-          EFI_IMAGE_MACHINE_TYPE_SUPPORTED (PeHeader->Pe32.FileHeader.Machine)
-          ) {
+      if (OptionalHeader->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC ||
+          OptionalHeader->Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
         //
         // Check the Subsystem:
         //   Driver#### must be of type BootServiceDriver or RuntimeDriver
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 5/7] MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2019-04-14 19:52 ` [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-14 19:52 ` [PATCH v6 6/7] MdePkg/UefiBaseType.h: treat EBC as a non-native machine type Ard Biesheuvel
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Implement the new EDK2 PE/COFF image emulator protocol so that we can
remove the EBC specific handling in the DXE core and other places in
the core code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf |   3 +
 MdeModulePkg/Universal/EbcDxe/EbcDxe.inf      |   3 +
 MdeModulePkg/Universal/EbcDxe/EbcInt.c        | 121 +++++++++++++++++++-
 MdeModulePkg/Universal/EbcDxe/EbcInt.h        |   3 +
 4 files changed, 126 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf b/MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf
index 8f293f5c7c29..c7a9d519b080 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf
+++ b/MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf
@@ -89,6 +89,8 @@
   BaseMemoryLib
   DebugLib
   BaseLib
+  CacheMaintenanceLib
+  PeCoffLib
 
 [Protocols]
   gEfiDebugSupportProtocolGuid                  ## PRODUCES
@@ -98,6 +100,7 @@
   gEfiEbcSimpleDebuggerProtocolGuid             ## SOMETIMES_CONSUMES
   gEfiPciRootBridgeIoProtocolGuid               ## SOMETIMES_CONSUMES
   gEfiSimpleFileSystemProtocolGuid              ## SOMETIMES_CONSUMES
+  gEdkiiPeCoffImageEmulatorProtocolGuid         ## PRODUCES
 
 [Guids]
   gEfiFileInfoGuid                              ## SOMETIMES_CONSUMES ## GUID
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcDxe.inf b/MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
index d6ee6194a0c8..ecccf2c57ffe 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
+++ b/MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
@@ -57,7 +57,9 @@
   MdeModulePkg/MdeModulePkg.dec
 
 [LibraryClasses]
+  CacheMaintenanceLib
   MemoryAllocationLib
+  PeCoffLib
   UefiBootServicesTableLib
   BaseMemoryLib
   UefiDriverEntryPoint
@@ -68,6 +70,7 @@
 [Protocols]
   gEfiDebugSupportProtocolGuid                  ## PRODUCES
   gEfiEbcProtocolGuid                           ## PRODUCES
+  gEdkiiPeCoffImageEmulatorProtocolGuid         ## PRODUCES
   gEfiEbcVmTestProtocolGuid                     ## SOMETIMES_PRODUCES
   gEfiEbcSimpleDebuggerProtocolGuid             ## SOMETIMES_CONSUMES
 
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcInt.c b/MdeModulePkg/Universal/EbcDxe/EbcInt.c
index 727ba8bcae44..3376b5e45184 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcInt.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcInt.c
@@ -349,6 +349,119 @@ UINTN                  mStackNum = 0;
 EFI_EVENT              mEbcPeriodicEvent;
 VM_CONTEXT             *mVmPtr = NULL;
 
+/**
+  Check whether the emulator supports executing a certain PE/COFF image
+
+  @param[in] This         This pointer for EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
+                          structure
+  @param[in] ImageType    Whether the image is an application, a boot time
+                          driver or a runtime driver.
+  @param[in] DevicePath   Path to device where the image originated
+                          (e.g., a PCI option ROM)
+
+  @retval TRUE            The image is supported by the emulator
+  @retval FALSE           The image is not supported by the emulator.
+**/
+BOOLEAN
+EFIAPI
+EbcIsImageSupported (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN  UINT16                                  ImageType,
+  IN  EFI_DEVICE_PATH_PROTOCOL                *DevicePath   OPTIONAL
+  )
+{
+  if (ImageType != EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION &&
+      ImageType != EFI_IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER) {
+    return FALSE;
+  }
+  return TRUE;
+}
+
+/**
+  Register a supported PE/COFF image with the emulator. After this call
+  completes successfully, the PE/COFF image may be started as usual, and
+  it is the responsibility of the emulator implementation that any branch
+  into the code section of the image (including returns from functions called
+  from the foreign code) is executed as if it were running on the machine
+  type it was built for.
+
+  @param[in]      This          This pointer for
+                                EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL structure
+  @param[in]      ImageBase     The base address in memory of the PE/COFF image
+  @param[in]      ImageSize     The size in memory of the PE/COFF image
+  @param[in,out]  EntryPoint    The entry point of the PE/COFF image. Passed by
+                                reference so that the emulator may modify it.
+
+  @retval EFI_SUCCESS           The image was registered with the emulator and
+                                can be started as usual.
+  @retval other                 The image could not be registered.
+
+  If the PE/COFF machine type or image type are not supported by the emulator,
+  then ASSERT().
+**/
+EFI_STATUS
+EFIAPI
+EbcRegisterImage (
+  IN      EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN      EFI_PHYSICAL_ADDRESS                    ImageBase,
+  IN      UINT64                                  ImageSize,
+  IN  OUT EFI_IMAGE_ENTRY_POINT                   *EntryPoint
+  )
+{
+  DEBUG_CODE_BEGIN ();
+    PE_COFF_LOADER_IMAGE_CONTEXT  ImageContext;
+    EFI_STATUS                    Status;
+
+    ZeroMem (&ImageContext, sizeof (ImageContext));
+
+    ImageContext.Handle    = (VOID *)(UINTN)ImageBase;
+    ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory;
+
+    Status = PeCoffLoaderGetImageInfo (&ImageContext);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+    ASSERT (ImageContext.Machine == EFI_IMAGE_MACHINE_EBC);
+    ASSERT (ImageContext.ImageType == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION ||
+            ImageContext.ImageType == EFI_IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER);
+  DEBUG_CODE_END ();
+
+  EbcRegisterICacheFlush (NULL,
+    (EBC_ICACHE_FLUSH)InvalidateInstructionCacheRange);
+
+  return EbcCreateThunk (NULL, (VOID *)(UINTN)ImageBase,
+           (VOID *)(UINTN)*EntryPoint, (VOID **)EntryPoint);
+}
+
+/**
+  Unregister a PE/COFF image that has been registered with the emulator.
+  This should be done before the image is unloaded from memory.
+
+  @param[in] This         This pointer for EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
+                          structure
+  @param[in] ImageBase    The base address in memory of the PE/COFF image
+
+  @retval EFI_SUCCESS     The image was unregistered with the emulator.
+  @retval other           Image could not be unloaded.
+**/
+EFI_STATUS
+EFIAPI
+EbcUnregisterImage (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN  EFI_PHYSICAL_ADDRESS                    ImageBase
+  )
+{
+  return EbcUnloadImage (NULL, (VOID *)(UINTN)ImageBase);
+}
+
+STATIC EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL mPeCoffEmuProtocol = {
+  EbcIsImageSupported,
+  EbcRegisterImage,
+  EbcUnregisterImage,
+  EDKII_PECOFF_IMAGE_EMULATOR_VERSION,
+  EFI_IMAGE_MACHINE_EBC
+};
 
 /**
   Initializes the VM EFI interface.  Allocates memory for the VM interface
@@ -437,11 +550,11 @@ InitializeEbcDriver (
   // Add the protocol so someone can locate us if we haven't already.
   //
   if (!Installed) {
-    Status = gBS->InstallProtocolInterface (
+    Status = gBS->InstallMultipleProtocolInterfaces (
                     &ImageHandle,
-                    &gEfiEbcProtocolGuid,
-                    EFI_NATIVE_INTERFACE,
-                    EbcProtocol
+                    &gEfiEbcProtocolGuid, EbcProtocol,
+                    &gEdkiiPeCoffImageEmulatorProtocolGuid, &mPeCoffEmuProtocol,
+                    NULL
                     );
     if (EFI_ERROR (Status)) {
       FreePool (EbcProtocol);
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcInt.h b/MdeModulePkg/Universal/EbcDxe/EbcInt.h
index 8aa7a4abbd63..9b25e91f951c 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcInt.h
+++ b/MdeModulePkg/Universal/EbcDxe/EbcInt.h
@@ -23,9 +23,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Protocol/Ebc.h>
 #include <Protocol/EbcVmTest.h>
 #include <Protocol/EbcSimpleDebugger.h>
+#include <Protocol/PeCoffImageEmulator.h>
 
 #include <Library/BaseLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
+#include <Library/PeCoffLib.h>
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/UefiBootServicesTableLib.h>
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 6/7] MdePkg/UefiBaseType.h: treat EBC as a non-native machine type
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
                   ` (4 preceding siblings ...)
  2019-04-14 19:52 ` [PATCH v6 5/7] MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-14 19:52 ` [PATCH v6 7/7] MdeModulePkg/DxeCore: remove explicit EBC handling Ard Biesheuvel
  2019-04-15  1:25 ` [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Wu, Hao A
  7 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Instead of classifying EBC as a supported machine type and have special
handling in DXE core for loading EBC images, make it a foreign type and
rely on the EDK2 PE/COFF image emulator protocol to claim the image when
the DXE core finds that it cannot be supported natively.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdePkg/Include/Uefi/UefiBaseType.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Include/Uefi/UefiBaseType.h b/MdePkg/Include/Uefi/UefiBaseType.h
index 8c9d571eb1ce..33b872af91a9 100644
--- a/MdePkg/Include/Uefi/UefiBaseType.h
+++ b/MdePkg/Include/Uefi/UefiBaseType.h
@@ -250,14 +250,14 @@ typedef union {
 #if   defined (MDE_CPU_IA32)
 
 #define EFI_IMAGE_MACHINE_TYPE_SUPPORTED(Machine) \
-  (((Machine) == EFI_IMAGE_MACHINE_IA32) || ((Machine) == EFI_IMAGE_MACHINE_EBC))
+  ((Machine) == EFI_IMAGE_MACHINE_IA32)
 
 #define EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED(Machine) ((Machine) == EFI_IMAGE_MACHINE_X64)
 
 #elif defined (MDE_CPU_X64)
 
 #define EFI_IMAGE_MACHINE_TYPE_SUPPORTED(Machine) \
-  (((Machine) == EFI_IMAGE_MACHINE_X64) || ((Machine) == EFI_IMAGE_MACHINE_EBC))
+  ((Machine) == EFI_IMAGE_MACHINE_X64)
 
 #define EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED(Machine) ((Machine) == EFI_IMAGE_MACHINE_IA32)
 
@@ -270,7 +270,7 @@ typedef union {
 #elif defined (MDE_CPU_AARCH64)
 
 #define EFI_IMAGE_MACHINE_TYPE_SUPPORTED(Machine) \
-  (((Machine) == EFI_IMAGE_MACHINE_AARCH64) || ((Machine) == EFI_IMAGE_MACHINE_EBC))
+  ((Machine) == EFI_IMAGE_MACHINE_AARCH64)
 
 #define EFI_IMAGE_MACHINE_CROSS_TYPE_SUPPORTED(Machine) (FALSE)
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 7/7] MdeModulePkg/DxeCore: remove explicit EBC handling
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
                   ` (5 preceding siblings ...)
  2019-04-14 19:52 ` [PATCH v6 6/7] MdePkg/UefiBaseType.h: treat EBC as a non-native machine type Ard Biesheuvel
@ 2019-04-14 19:52 ` Ard Biesheuvel
  2019-04-15  1:25 ` [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Wu, Hao A
  7 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-14 19:52 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Michael D Kinney, Andrew Fish, Leif Lindholm,
	Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey,
	Steven Shi, Jian J Wang, Hao Wu

Now that the EBC machine type is no longer classified as a
natively supported machine type on the architectures that can
support it via the EBC interpreter, the EBC specific handling
in DXE core is no longer used and can be removed.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.h     |  3 --
 MdeModulePkg/Core/Dxe/DxeMain.inf   |  1 -
 MdeModulePkg/Core/Dxe/Image/Image.c | 53 ++------------------
 3 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 48ec30a48aa2..6e83f15a4541 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -42,7 +42,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Protocol/LoadPe32Image.h>
 #include <Protocol/Security.h>
 #include <Protocol/Security2.h>
-#include <Protocol/Ebc.h>
 #include <Protocol/Reset.h>
 #include <Protocol/Cpu.h>
 #include <Protocol/Metronome.h>
@@ -227,8 +226,6 @@ typedef struct {
   BASE_LIBRARY_JUMP_BUFFER    *JumpContext;
   /// Machine type from PE image
   UINT16                      Machine;
-  /// EBC Protocol pointer
-  EFI_EBC_PROTOCOL            *Ebc;
   /// PE/COFF Image Emulator Protocol pointer
   EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *PeCoffEmu;
   /// Runtime image list
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index ce6fc19be5e4..68413517f2a7 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -160,7 +160,6 @@
   gEfiLoadedImageProtocolGuid                   ## PRODUCES
   gEfiLoadedImageDevicePathProtocolGuid         ## PRODUCES
   gEfiHiiPackageListProtocolGuid                ## SOMETIMES_PRODUCES
-  gEfiEbcProtocolGuid                           ## SOMETIMES_CONSUMES
   gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
   gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES
 
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 57330636b5f3..d5e890dd5312 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -75,7 +75,6 @@ LOADED_IMAGE_PRIVATE_DATA mCorePrivateImage  = {
   NULL,                       // JumpBuffer
   NULL,                       // JumpContext
   0,                          // Machine
-  NULL,                       // Ebc
   NULL,                       // PeCoffEmu
   NULL,                       // RuntimeData
   NULL                        // LoadedImageDevicePath
@@ -92,9 +91,6 @@ typedef struct {
   CHAR16  *MachineTypeName;
 } MACHINE_TYPE_INFO;
 
-//
-// EBC machine is not listed in this table, because EBC is in the default supported scopes of other machine type.
-//
 GLOBAL_REMOVE_IF_UNREFERENCED MACHINE_TYPE_INFO  mMachineTypeInfo[] = {
   {EFI_IMAGE_MACHINE_IA32,           L"IA32"},
   {EFI_IMAGE_MACHINE_IA64,           L"IA64"},
@@ -774,51 +770,15 @@ CoreLoadPeImage (
   InvalidateInstructionCacheRange ((VOID *)(UINTN)Image->ImageContext.ImageAddress, (UINTN)Image->ImageContext.ImageSize);
 
   //
-  // Copy the machine type from the context to the image private data. This
-  // is needed during image unload to know if we should call an EBC protocol
-  // to unload the image.
+  // Copy the machine type from the context to the image private data.
   //
   Image->Machine = Image->ImageContext.Machine;
 
   //
-  // Get the image entry point. If it's an EBC image, then call into the
-  // interpreter to create a thunk for the entry point and use the returned
-  // value for the entry point.
+  // Get the image entry point.
   //
   Image->EntryPoint   = (EFI_IMAGE_ENTRY_POINT)(UINTN)Image->ImageContext.EntryPoint;
-  if (Image->ImageContext.Machine == EFI_IMAGE_MACHINE_EBC) {
-    //
-    // Locate the EBC interpreter protocol
-    //
-    Status = CoreLocateProtocol (&gEfiEbcProtocolGuid, NULL, (VOID **)&Image->Ebc);
-    if (EFI_ERROR(Status) || Image->Ebc == NULL) {
-      DEBUG ((DEBUG_LOAD | DEBUG_ERROR, "CoreLoadPeImage: There is no EBC interpreter for an EBC image.\n"));
-      goto Done;
-    }
-
-    //
-    // Register a callback for flushing the instruction cache so that created
-    // thunks can be flushed.
-    //
-    Status = Image->Ebc->RegisterICacheFlush (Image->Ebc, (EBC_ICACHE_FLUSH)InvalidateInstructionCacheRange);
-    if (EFI_ERROR(Status)) {
-      goto Done;
-    }
-
-    //
-    // Create a thunk for the image's entry point. This will be the new
-    // entry point for the image.
-    //
-    Status = Image->Ebc->CreateThunk (
-                           Image->Ebc,
-                           Image->Handle,
-                           (VOID *)(UINTN) Image->ImageContext.EntryPoint,
-                           (VOID **) &Image->EntryPoint
-                           );
-    if (EFI_ERROR(Status)) {
-      goto Done;
-    }
-  } else if (Image->PeCoffEmu != NULL) {
+  if (Image->PeCoffEmu != NULL) {
     Status = Image->PeCoffEmu->RegisterImage (Image->PeCoffEmu,
                                  Image->ImageBasePage,
                                  EFI_PAGES_TO_SIZE (Image->NumberOfPages),
@@ -1008,13 +968,6 @@ CoreUnloadAndCloseImage (
 
   UnprotectUefiImage (&Image->Info, Image->LoadedImageDevicePath);
 
-  if (Image->Ebc != NULL) {
-    //
-    // If EBC protocol exists we must perform cleanups for this image.
-    //
-    Image->Ebc->UnloadImage (Image->Ebc, Image->Handle);
-  }
-
   if (Image->PeCoffEmu != NULL) {
     //
     // If the PE/COFF Emulator protocol exists we must unregister the image.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
  2019-04-14 19:52 ` [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol Ard Biesheuvel
@ 2019-04-15  1:23   ` Wu, Hao A
  2019-04-15  9:02     ` Leif Lindholm
  0 siblings, 1 reply; 20+ messages in thread
From: Wu, Hao A @ 2019-04-15  1:23 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Kinney, Michael D, Andrew Fish, Leif Lindholm, Zeng, Star,
	Dong, Eric, Ni, Ray, Gao, Liming, Carsey, Jaben, Shi, Steven,
	Wang, Jian J

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, April 15, 2019 3:52 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel; Kinney, Michael D; Andrew Fish; Leif Lindholm; Zeng, Star;
> Dong, Eric; Ni, Ray; Gao, Liming; Carsey, Jaben; Shi, Steven; Wang, Jian J; Wu,
> Hao A
> Subject: [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator
> protocol
> 
> Introduce a protocol that can be invoked by the image loading services
> to execute foreign architecture PE/COFF images via an emulator.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h | 107
> ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                       |   4 +
>  2 files changed, 111 insertions(+)
> 
> diff --git a/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
> b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
> new file mode 100644
> index 000000000000..1ca302440e4a
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
> @@ -0,0 +1,107 @@
> +/** @file
> +  Copyright (c) 2019, Linaro, Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials are licensed and made
> available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> +#define PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H

Hello Ard,

Sorry for the delayed response.

Could you help to use:
_PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H_

here to please the Ecc checker when you push the series?

Best Regards,
Hao Wu

> +
> +#define EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID \
> +  { 0x96F46153, 0x97A7, 0x4793, { 0xAC, 0xC1, 0xFA, 0x19, 0xBF, 0x78, 0xEA,
> 0x97 } }
> +
> +typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
> EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
> +
> +/**
> +  Check whether the emulator supports executing a certain PE/COFF image
> +
> +  @param[in] This         This pointer for
> EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
> +                          structure
> +  @param[in] ImageType    Whether the image is an application, a boot time
> +                          driver or a runtime driver.
> +  @param[in] DevicePath   Path to device where the image originated
> +                          (e.g., a PCI option ROM)
> +
> +  @retval TRUE            The image is supported by the emulator
> +  @retval FALSE           The image is not supported by the emulator.
> +**/
> +typedef
> +BOOLEAN
> +(EFIAPI *EDKII_PECOFF_IMAGE_EMULATOR_IS_IMAGE_SUPPORTED) (
> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> +  IN  UINT16                                  ImageType,
> +  IN  EFI_DEVICE_PATH_PROTOCOL                *DevicePath   OPTIONAL
> +  );
> +
> +/**
> +  Register a supported PE/COFF image with the emulator. After this call
> +  completes successfully, the PE/COFF image may be started as usual, and
> +  it is the responsibility of the emulator implementation that any branch
> +  into the code section of the image (including returns from functions called
> +  from the foreign code) is executed as if it were running on the machine
> +  type it was built for.
> +
> +  @param[in]      This          This pointer for
> +                                EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL structure
> +  @param[in]      ImageBase     The base address in memory of the PE/COFF
> image
> +  @param[in]      ImageSize     The size in memory of the PE/COFF image
> +  @param[in,out]  EntryPoint    The entry point of the PE/COFF image. Passed
> by
> +                                reference so that the emulator may modify it.
> +
> +  @retval EFI_SUCCESS           The image was registered with the emulator and
> +                                can be started as usual.
> +  @retval other                 The image could not be registered.
> +
> +  If the PE/COFF machine type or image type are not supported by the
> emulator,
> +  then ASSERT().
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_PECOFF_IMAGE_EMULATOR_REGISTER_IMAGE) (
> +  IN      EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> +  IN      EFI_PHYSICAL_ADDRESS                    ImageBase,
> +  IN      UINT64                                  ImageSize,
> +  IN  OUT EFI_IMAGE_ENTRY_POINT                   *EntryPoint
> +  );
> +
> +/**
> +  Unregister a PE/COFF image that has been registered with the emulator.
> +  This should be done before the image is unloaded from memory.
> +
> +  @param[in] This         This pointer for
> EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
> +                          structure
> +  @param[in] ImageBase    The base address in memory of the PE/COFF
> image
> +
> +  @retval EFI_SUCCESS     The image was unregistered with the emulator.
> +  @retval other           Image could not be unloaded.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_PECOFF_IMAGE_EMULATOR_UNREGISTER_IMAGE) (
> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> +  IN  EFI_PHYSICAL_ADDRESS                    ImageBase
> +  );
> +
> +#define EDKII_PECOFF_IMAGE_EMULATOR_VERSION         0x1
> +
> +typedef struct _EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL {
> +  EDKII_PECOFF_IMAGE_EMULATOR_IS_IMAGE_SUPPORTED
> IsImageSupported;
> +  EDKII_PECOFF_IMAGE_EMULATOR_REGISTER_IMAGE        RegisterImage;
> +  EDKII_PECOFF_IMAGE_EMULATOR_UNREGISTER_IMAGE
> UnregisterImage;
> +
> +  // Protocol version implemented by the emulator
> +  UINT32                                            Version;
> +  // The machine type implemented by the emulator
> +  UINT16                                            MachineType;
> +} EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL;
> +
> +extern EFI_GUID gEdkiiPeCoffImageEmulatorProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index a2130bc43991..c2b4e7f69367 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -629,6 +629,10 @@
> 
>    ## Include/Protocol/AtaAtapiPolicy.h
>    gEdkiiAtaAtapiPolicyProtocolGuid = { 0xe59cd769, 0x5083, 0x4f26,{ 0x90,
> 0x94, 0x6c, 0x91, 0x9f, 0x91, 0x6c, 0x4e } }
> +
> +  ## Include/Protocol/PeCoffImageEmulator.h
> +  gEdkiiPeCoffImageEmulatorProtocolGuid = { 0x96f46153, 0x97a7, 0x4793,
> { 0xac, 0xc1, 0xfa, 0x19, 0xbf, 0x78, 0xea, 0x97 } }
> +
>  #
>  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>  #   0x80000001 | Invalid value provided.
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images
  2019-04-14 19:52 ` [PATCH v6 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images Ard Biesheuvel
@ 2019-04-15  1:23   ` Wu, Hao A
  0 siblings, 0 replies; 20+ messages in thread
From: Wu, Hao A @ 2019-04-15  1:23 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Kinney, Michael D, Andrew Fish, Leif Lindholm, Zeng, Star,
	Dong, Eric, Ni, Ray, Gao, Liming, Carsey, Jaben, Shi, Steven,
	Wang, Jian J

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, April 15, 2019 3:52 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel; Kinney, Michael D; Andrew Fish; Leif Lindholm; Zeng, Star;
> Dong, Eric; Ni, Ray; Gao, Liming; Carsey, Jaben; Shi, Steven; Wang, Jian J; Wu,
> Hao A
> Subject: [PATCH v6 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h     |   3 +
>  MdeModulePkg/Core/Dxe/DxeMain.inf   |   1 +
>  MdeModulePkg/Core/Dxe/Image/Image.c | 171 ++++++++++++++++++--
>  3 files changed, 164 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..57330636b5f3 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);
> +    Status = CoreLocateHandle (
> +               ByRegisterNotify,
> +               NULL,
> +               mPeCoffEmuProtocolNotifyRegistration,
> +               &BufferSize,
> +               &EmuHandle
> +               );
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // If no more notification events exit
> +      //
> +      return;
> +    }
> +
> +    Entry = AllocateZeroPool (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

Hello Ard,

Sorry for the delayed response.

Could you help to update the above comments to:
'@param[in, out]  Image ...'

here to please the Ecc checker when you push the series?

Best Regards,
Hao Wu

> +
> +  @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,15 @@ 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_UNSUPPORTED.
> +    //
> +    DEBUG ((DEBUG_ERROR, "Image type %s can't be loaded on %s UEFI
> system.\n",
> +      GetMachineTypeName (Image->ImageContext.Machine),
> +      GetMachineTypeName (mDxeCoreImageMachineType)));
> +    return EFI_UNSUPPORTED;
>    }
> 
>    //
> @@ -687,6 +818,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 +1015,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 +1747,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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
  2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
                   ` (6 preceding siblings ...)
  2019-04-14 19:52 ` [PATCH v6 7/7] MdeModulePkg/DxeCore: remove explicit EBC handling Ard Biesheuvel
@ 2019-04-15  1:25 ` Wu, Hao A
  2019-04-15  2:04   ` [edk2-devel] " Ard Biesheuvel
  7 siblings, 1 reply; 20+ messages in thread
From: Wu, Hao A @ 2019-04-15  1:25 UTC (permalink / raw)
  To: Ard Biesheuvel, devel@edk2.groups.io
  Cc: Kinney, Michael D, Andrew Fish, Leif Lindholm, Zeng, Star,
	Dong, Eric, Ni, Ray, Gao, Liming, Carsey, Jaben, Shi, Steven,
	Wang, Jian J

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, April 15, 2019 3:52 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel; Kinney, Michael D; Andrew Fish; Leif Lindholm; Zeng, Star;
> Dong, Eric; Ni, Ray; Gao, Liming; Carsey, Jaben; Shi, Steven; Wang, Jian J; Wu,
> Hao A
> Subject: [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign
> arch PE/COFF images
> 
> Add the basic plumbing to DXE core, the PCI bus driver and the boot manager
> to allow PE/COFF images to be dispatched that target an architecture that is
> not native for the platform, but which is supported by one of potentially
> several available emulators.
> 
> One implementation of such an emulator can be found here:
> https://github.com/ardbiesheuvel/X86EmulatorPkg/tree/upstream-v4
> 
> This also allows us to get rid of the special treatment of EBC images in
> core code. Instead, the EbcDxe driver is augmented with an implementation
> of the EDK2 PE/COFF image emulator protocol so that internal knowledge of
> how EBC is implemented (I-cache flushing, thunks) is removed from the DXE
> core.
> 
> Changes since v5:
> - Fix build issues reported by Mike Kinney (#2, #5)
> - Fix CheckPatch.py issue regarding the use of EFI_D_ERROR (#2) [This is code
>   that is only being moved around but I fixed it nonetheless]
> - Use InstallMultipleProtocolInterfaces() to install the EBC protocol and the
>   PE/COFF emu protocol at the same time (to simplify/fix the error path) (#5)
> - Add Mike's Reviewed-by

The series looks good to be.
Please help to address the format issues as I replied in 1/7 and 2/7
patches.

With those addressed, for the series,
Reviewed-by: Hao Wu <hao.a.wu@intel.com>


Best Regards,
Hao Wu

> 
> Changes since v4:
> - Fix an issue in the protocol notify handler pointed out by Mike Kinney (#2)
> 
> Changes since v3:
> - Simplify the handling of option ROMs and Driver#### images, by simply
>   deferring to the LoadImage() boot service to decide whether an image
>   can be supported or not - this removes some redundant checks from the
>   BDS layer and the PCI bus driver.
> - Move the machine type supported by the emulator into the protocol struct,
>   so we can optimize away calls into the emulator for each image loaded.
>   Instead, the LoadImage() code will only invoke the IsSupported() method for
>   images that are known to have a matching machine type.
> 
> Note that I have considered, but ultimately dismissed the suggestion to
> register and unregister emulators via a new protocol. The main issue is
> that registering and unregistering structs containing sets of function
> pointers is awfully similar to managing a protocol database, and we already
> have the code to do that in EDK2.
> 
> So instead, I have removed all the code that iterates over a handle buffer
> of emu protocols and invokes each one to see if it will support the image.
> Instead, this is all done by CoreLoadImage().
> 
> Changes since v2:
> - incorporate feedback from Andrew Fish (delivered in person):
>   * pass a device path into the IsImageSupported() protocol method so that an
>     implementation can blacklist or whitelist certain devices, or implement
>     other policies that depend on the device where the driver originated
>   * allow the emulator to supersede the native loading of the image - this
>     permits things like X86 on X86 emulators for security sandboxing or debug
> 
> Changes since v1:
> - subsume the EBC handling into the EDK2 emulator protocol and abstract
>   away from EBC specifics in core code.
> - allow multiple emulator implementations to co-exist
> - incorporate Star's review feedback
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> 
> Ard Biesheuvel (7):
>   MdeModulePkg: introduce PE/COFF image emulator protocol
>   MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images
>   MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures
>   MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images
>   MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol
>   MdePkg/UefiBaseType.h: treat EBC as a non-native machine type
>   MdeModulePkg/DxeCore: remove explicit EBC handling
> 
>  .../Bus/Pci/PciBusDxe/PciOptionRomSupport.c   |   7 -
>  MdeModulePkg/Core/Dxe/DxeMain.h               |   6 +-
>  MdeModulePkg/Core/Dxe/DxeMain.inf             |   2 +-
>  MdeModulePkg/Core/Dxe/Image/Image.c           | 210 +++++++++++++-----
>  .../Include/Protocol/PeCoffImageEmulator.h    | 107 +++++++++
>  .../Library/UefiBootManagerLib/BmLoadOption.c |   6 +-
>  MdeModulePkg/MdeModulePkg.dec                 |   4 +
>  MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf |   3 +
>  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf      |   3 +
>  MdeModulePkg/Universal/EbcDxe/EbcInt.c        | 121 +++++++++-
>  MdeModulePkg/Universal/EbcDxe/EbcInt.h        |   3 +
>  MdePkg/Include/Uefi/UefiBaseType.h            |   6 +-
>  12 files changed, 402 insertions(+), 76 deletions(-)
>  create mode 100644
> MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
> 
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [edk2-devel] [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
  2019-04-15  1:25 ` [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Wu, Hao A
@ 2019-04-15  2:04   ` Ard Biesheuvel
  0 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-15  2:04 UTC (permalink / raw)
  To: edk2-devel-groups-io, Wu, Hao A
  Cc: Kinney, Michael D, Andrew Fish, Leif Lindholm, Zeng, Star,
	Dong, Eric, Ni, Ray, Gao, Liming, Carsey, Jaben, Shi, Steven,
	Wang, Jian J

On Sun, 14 Apr 2019 at 18:25, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Monday, April 15, 2019 3:52 AM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel; Kinney, Michael D; Andrew Fish; Leif Lindholm; Zeng, Star;
> > Dong, Eric; Ni, Ray; Gao, Liming; Carsey, Jaben; Shi, Steven; Wang, Jian J; Wu,
> > Hao A
> > Subject: [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign
> > arch PE/COFF images
> >
> > Add the basic plumbing to DXE core, the PCI bus driver and the boot manager
> > to allow PE/COFF images to be dispatched that target an architecture that is
> > not native for the platform, but which is supported by one of potentially
> > several available emulators.
> >
> > One implementation of such an emulator can be found here:
> > https://github.com/ardbiesheuvel/X86EmulatorPkg/tree/upstream-v4
> >
> > This also allows us to get rid of the special treatment of EBC images in
> > core code. Instead, the EbcDxe driver is augmented with an implementation
> > of the EDK2 PE/COFF image emulator protocol so that internal knowledge of
> > how EBC is implemented (I-cache flushing, thunks) is removed from the DXE
> > core.
> >
> > Changes since v5:
> > - Fix build issues reported by Mike Kinney (#2, #5)
> > - Fix CheckPatch.py issue regarding the use of EFI_D_ERROR (#2) [This is code
> >   that is only being moved around but I fixed it nonetheless]
> > - Use InstallMultipleProtocolInterfaces() to install the EBC protocol and the
> >   PE/COFF emu protocol at the same time (to simplify/fix the error path) (#5)
> > - Add Mike's Reviewed-by
>
> The series looks good to be.
> Please help to address the format issues as I replied in 1/7 and 2/7
> patches.
>
> With those addressed, for the series,
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
>


Thank you Hao

I have made the changes you requested, and pushed the series as
174232fa9a90..2e21e8c4b896

Thanks to all for the effort,
Ard.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
  2019-04-15  1:23   ` Wu, Hao A
@ 2019-04-15  9:02     ` Leif Lindholm
  2019-04-15 14:22       ` [edk2-devel] " Liming Gao
  2019-04-15 15:53       ` Michael D Kinney
  0 siblings, 2 replies; 20+ messages in thread
From: Leif Lindholm @ 2019-04-15  9:02 UTC (permalink / raw)
  To: Wu, Hao A
  Cc: Ard Biesheuvel, devel@edk2.groups.io, Kinney, Michael D,
	Andrew Fish, Zeng, Star, Dong, Eric, Ni, Ray, Gao, Liming,
	Carsey, Jaben, Shi, Steven, Wang, Jian J

On Mon, Apr 15, 2019 at 01:23:11AM +0000, Wu, Hao A wrote:
> > +  This program and the accompanying materials are licensed and made
> > available
> > +  under the terms and conditions of the BSD License which accompanies this
> > +  distribution.  The full text of the license may be found at
> > +  http://opensource.org/licenses/bsd-license.php
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS,
> > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> > +#define PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> 
> Hello Ard,
> 
> Sorry for the delayed response.
> 
> Could you help to use:
> _PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H_
> 
> here to please the Ecc checker when you push the series?

Hmm...

Not a major issue, but I'll mention it anyway (which I mentioned to
Andrew/Mike at Linaro Connect in Vancouver last year):
clang has a warning, enabled by -Wreserved-id-macro, which complains
about this.

The Coding Style matches this, suggesting
MACROS_SHOULD_BE_WRITTEN_THUS_. So do we need to fix Ecc?

Regards,

Leif

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [edk2-devel] [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
  2019-04-15  9:02     ` Leif Lindholm
@ 2019-04-15 14:22       ` Liming Gao
  2019-04-15 15:53       ` Michael D Kinney
  1 sibling, 0 replies; 20+ messages in thread
From: Liming Gao @ 2019-04-15 14:22 UTC (permalink / raw)
  To: devel@edk2.groups.io, leif.lindholm@linaro.org, Wu, Hao A
  Cc: Ard Biesheuvel, Kinney, Michael D, Andrew Fish, Zeng, Star,
	Dong, Eric, Ni, Ray, Carsey, Jaben, Shi, Steven, Wang, Jian J

Leif:
  How about submit one BZ to catch it first? Then, revisit it later. 

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
> Sent: Monday, April 15, 2019 5:02 PM
> To: Wu, Hao A <hao.a.wu@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Andrew Fish
> <afish@apple.com>; 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>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
> 
> On Mon, Apr 15, 2019 at 01:23:11AM +0000, Wu, Hao A wrote:
> > > +  This program and the accompanying materials are licensed and made
> > > available
> > > +  under the terms and conditions of the BSD License which accompanies this
> > > +  distribution.  The full text of the license may be found at
> > > +  http://opensource.org/licenses/bsd-license.php
> > > +
> > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > > BASIS,
> > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > > EXPRESS OR IMPLIED.
> > > +
> > > +**/
> > > +
> > > +#ifndef PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> > > +#define PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> >
> > Hello Ard,
> >
> > Sorry for the delayed response.
> >
> > Could you help to use:
> > _PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H_
> >
> > here to please the Ecc checker when you push the series?
> 
> Hmm...
> 
> Not a major issue, but I'll mention it anyway (which I mentioned to
> Andrew/Mike at Linaro Connect in Vancouver last year):
> clang has a warning, enabled by -Wreserved-id-macro, which complains
> about this.
> 
> The Coding Style matches this, suggesting
> MACROS_SHOULD_BE_WRITTEN_THUS_. So do we need to fix Ecc?
> 
> Regards,
> 
> Leif
> 
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
  2019-04-15  9:02     ` Leif Lindholm
  2019-04-15 14:22       ` [edk2-devel] " Liming Gao
@ 2019-04-15 15:53       ` Michael D Kinney
  2019-04-15 21:52         ` Michael D Kinney
  1 sibling, 1 reply; 20+ messages in thread
From: Michael D Kinney @ 2019-04-15 15:53 UTC (permalink / raw)
  To: Leif Lindholm, Wu, Hao A, Kinney, Michael D
  Cc: Ard Biesheuvel, devel@edk2.groups.io, Andrew Fish, Zeng, Star,
	Dong, Eric, Ni, Ray, Gao, Liming, Carsey, Jaben, Shi, Steven,
	Wang, Jian J

Hi Leif,

This is not a macro that is to be used by C code.  It is a technique
to prevent recursive includes on a .h file.  As a result, we do not want
to use the C Coding Standard macro style.

Are there other techniques to prevent recursive includes?

I agree that macros/symbols that start with single or double '_'
are reserved by compilers/linkers, which is why the warning is 
triggered and it would be better if we use a different technique.

The current style assumes that the define symbol used will not
collide with a compiler/linker symbol.  The names are verbose,
so the chances of a collision are very low.

Mike

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Monday, April 15, 2019 2:02 AM
> To: Wu, Hao A <hao.a.wu@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Andrew Fish
> <afish@apple.com>; 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>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [PATCH v6 1/7] MdeModulePkg: introduce
> PE/COFF image emulator protocol
> 
> On Mon, Apr 15, 2019 at 01:23:11AM +0000, Wu, Hao A
> wrote:
> > > +  This program and the accompanying materials are
> licensed and made
> > > available
> > > +  under the terms and conditions of the BSD
> License which accompanies this
> > > +  distribution.  The full text of the license may
> be found at
> > > +  http://opensource.org/licenses/bsd-license.php
> > > +
> > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE
> ON AN "AS IS"
> > > BASIS,
> > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER
> > > EXPRESS OR IMPLIED.
> > > +
> > > +**/
> > > +
> > > +#ifndef PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> > > +#define PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> >
> > Hello Ard,
> >
> > Sorry for the delayed response.
> >
> > Could you help to use:
> > _PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H_
> >
> > here to please the Ecc checker when you push the
> series?
> 
> Hmm...
> 
> Not a major issue, but I'll mention it anyway (which I
> mentioned to
> Andrew/Mike at Linaro Connect in Vancouver last year):
> clang has a warning, enabled by -Wreserved-id-macro,
> which complains
> about this.
> 
> The Coding Style matches this, suggesting
> MACROS_SHOULD_BE_WRITTEN_THUS_. So do we need to fix
> Ecc?
> 
> Regards,
> 
> Leif

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [edk2-devel] [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images
  2019-04-14 19:52 ` [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images Ard Biesheuvel
@ 2019-04-15 20:22   ` Ni, Ray
  0 siblings, 0 replies; 20+ messages in thread
From: Ni, Ray @ 2019-04-15 20:22 UTC (permalink / raw)
  To: devel@edk2.groups.io, ard.biesheuvel@linaro.org
  Cc: Kinney, Michael D, Andrew Fish, Leif Lindholm, Zeng, Star,
	Dong, Eric, Gao, Liming, Carsey, Jaben, Shi, Steven, Wang, Jian J,
	Wu, Hao A

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Ard Biesheuvel
> Sent: Sunday, April 14, 2019 12:53 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; 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>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
> Hao A <hao.a.wu@intel.com>
> Subject: [edk2-devel] [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib:
> allow foreign Driver#### images
> 
> Allow PE/COFF images that must execute under emulation for Driver####
> options, by removing the redundant machine type check from the BDS code.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> index 7bf96646c690..8e6caaa63548 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> @@ -1233,10 +1233,8 @@ BmIsLoadOptionPeHeaderValid (
>        // Check PE32 or PE32+ magic, and machine type
>        //
>        OptionalHeader = (EFI_IMAGE_OPTIONAL_HEADER32 *) &PeHeader-
> >Pe32.OptionalHeader;
> -      if ((OptionalHeader->Magic ==
> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC ||
> -           OptionalHeader->Magic ==
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) &&
> -          EFI_IMAGE_MACHINE_TYPE_SUPPORTED (PeHeader-
> >Pe32.FileHeader.Machine)
> -          ) {
> +      if (OptionalHeader->Magic ==
> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC ||
> +          OptionalHeader->Magic ==
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>          //
>          // Check the Subsystem:
>          //   Driver#### must be of type BootServiceDriver or RuntimeDriver
> --
> 2.17.1
> 
> 
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [edk2-devel] [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures
  2019-04-14 19:52 ` [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures Ard Biesheuvel
@ 2019-04-15 20:23   ` Ni, Ray
  2019-04-16  6:02   ` Gary Lin
  1 sibling, 0 replies; 20+ messages in thread
From: Ni, Ray @ 2019-04-15 20:23 UTC (permalink / raw)
  To: devel@edk2.groups.io, ard.biesheuvel@linaro.org
  Cc: Kinney, Michael D, Andrew Fish, Leif Lindholm, Zeng, Star,
	Dong, Eric, Gao, Liming, Carsey, Jaben, Shi, Steven, Wang, Jian J,
	Wu, Hao A

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Ard Biesheuvel
> Sent: Sunday, April 14, 2019 12:52 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; 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>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
> Hao A <hao.a.wu@intel.com>
> Subject: [edk2-devel] [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch
> option ROMs for foreign architectures
> 
> Delete the explicit machine type check for option ROM images, and instead,
> rely on the LoadImage() boot service to decide whether an option ROM can
> be dispatched or not. This permits platforms to ship with emulators to
> execute option ROMs that are not native to the processor architecture.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c75ef1a82505..54cf4251cc86 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -699,13 +699,6 @@ ProcessOpRomImage (
>        goto NextImage;
>      }
> 
> -    //
> -    // Skip the EFI PCI Option ROM image if its machine type is not
> supported
> -    //
> -    if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (EfiRomHeader-
> >EfiMachineType)) {
> -      goto NextImage;
> -    }
> -
>      //
>      // Ignore the EFI PCI Option ROM image if it is an EFI application
>      //
> --
> 2.17.1
> 
> 
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol
  2019-04-15 15:53       ` Michael D Kinney
@ 2019-04-15 21:52         ` Michael D Kinney
  0 siblings, 0 replies; 20+ messages in thread
From: Michael D Kinney @ 2019-04-15 21:52 UTC (permalink / raw)
  To: Leif Lindholm, Wu, Hao A, Kinney, Michael D
  Cc: Ard Biesheuvel, devel@edk2.groups.io, Andrew Fish, Zeng, Star,
	Dong, Eric, Ni, Ray, Gao, Liming, Carsey, Jaben, Shi, Steven,
	Wang, Jian J

Leif,

How about using the following pragma instead if include guards?
	
	#pragma once

https://en.wikipedia.org/wiki/Pragma_once

This appears to be supports by GCC 3.4 and higher, clang, and VS.

Might even speed up builds a bit.

Mike

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Monday, April 15, 2019 8:53 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Wu, Hao A
> <hao.a.wu@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
> 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>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: RE: [PATCH v6 1/7] MdeModulePkg: introduce
> PE/COFF image emulator protocol
> 
> Hi Leif,
> 
> This is not a macro that is to be used by C code.  It
> is a technique
> to prevent recursive includes on a .h file.  As a
> result, we do not want
> to use the C Coding Standard macro style.
> 
> Are there other techniques to prevent recursive
> includes?
> 
> I agree that macros/symbols that start with single or
> double '_'
> are reserved by compilers/linkers, which is why the
> warning is
> triggered and it would be better if we use a different
> technique.
> 
> The current style assumes that the define symbol used
> will not
> collide with a compiler/linker symbol.  The names are
> verbose,
> so the chances of a collision are very low.
> 
> Mike
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Monday, April 15, 2019 2:02 AM
> > To: Wu, Hao A <hao.a.wu@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Andrew Fish
> > <afish@apple.com>; 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>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > Subject: Re: [PATCH v6 1/7] MdeModulePkg: introduce
> > PE/COFF image emulator protocol
> >
> > On Mon, Apr 15, 2019 at 01:23:11AM +0000, Wu, Hao A
> > wrote:
> > > > +  This program and the accompanying materials
> are
> > licensed and made
> > > > available
> > > > +  under the terms and conditions of the BSD
> > License which accompanies this
> > > > +  distribution.  The full text of the license
> may
> > be found at
> > > > +  http://opensource.org/licenses/bsd-license.php
> > > > +
> > > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD
> LICENSE
> > ON AN "AS IS"
> > > > BASIS,
> > > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND, EITHER
> > > > EXPRESS OR IMPLIED.
> > > > +
> > > > +**/
> > > > +
> > > > +#ifndef PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> > > > +#define PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H
> > >
> > > Hello Ard,
> > >
> > > Sorry for the delayed response.
> > >
> > > Could you help to use:
> > > _PECOFF_IMAGE_EMULATOR_PROTOCOL_GUID_H_
> > >
> > > here to please the Ecc checker when you push the
> > series?
> >
> > Hmm...
> >
> > Not a major issue, but I'll mention it anyway (which
> I
> > mentioned to
> > Andrew/Mike at Linaro Connect in Vancouver last
> year):
> > clang has a warning, enabled by -Wreserved-id-macro,
> > which complains
> > about this.
> >
> > The Coding Style matches this, suggesting
> > MACROS_SHOULD_BE_WRITTEN_THUS_. So do we need to fix
> > Ecc?
> >
> > Regards,
> >
> > Leif

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [edk2-devel] [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures
  2019-04-14 19:52 ` [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures Ard Biesheuvel
  2019-04-15 20:23   ` [edk2-devel] " Ni, Ray
@ 2019-04-16  6:02   ` Gary Lin
  2019-04-16 16:20     ` Ard Biesheuvel
  1 sibling, 1 reply; 20+ messages in thread
From: Gary Lin @ 2019-04-16  6:02 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: Michael D Kinney, Andrew Fish, Leif Lindholm, Star Zeng,
	Eric Dong, Ruiyu Ni, Liming Gao, Jaben Carsey, Steven Shi,
	Jian J Wang, Hao Wu

On Sun, Apr 14, 2019 at 12:52:29PM -0700, Ard Biesheuvel wrote:
> Delete the explicit machine type check for option ROM images, and instead,
> rely on the LoadImage() boot service to decide whether an option ROM can
> be dispatched or not. This permits platforms to ship with emulators to
> execute option ROMs that are not native to the processor architecture.
> 
After applying this patch, my OVMF VM failed to boot if e1000 or iPXE
were enabled. It failed with the following message:

Loading driver at 0x0007E537000 EntryPoint=0x0007E53C06D 8086100e.efi
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7F003B98
ProtectUefiImageCommon - 0x7F002BC0
  - 0x000000007E537000 - 0x000000000009F900
Image type IA32 can't be started on X64 UEFI system.
ASSERT MdeModulePkg/Core/Dxe/Mem/Pool.c(698): Head->Signature == ((('p') | ('h' << 8)) | ((('d') | ('0' << 8)) << 16)) || Head->Signature == ((('p') | ('h' << 8)) | ((('d') | ('1' << 8)) << 16))

It seems OVMF was trying to load the IA32 driver and failed.

Gary Lin

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c75ef1a82505..54cf4251cc86 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -699,13 +699,6 @@ ProcessOpRomImage (
>        goto NextImage;
>      }
>  
> -    //
> -    // Skip the EFI PCI Option ROM image if its machine type is not supported
> -    //
> -    if (!EFI_IMAGE_MACHINE_TYPE_SUPPORTED (EfiRomHeader->EfiMachineType)) {
> -      goto NextImage;
> -    }
> -
>      //
>      // Ignore the EFI PCI Option ROM image if it is an EFI application
>      //
> -- 
> 2.17.1
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [edk2-devel] [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures
  2019-04-16  6:02   ` Gary Lin
@ 2019-04-16 16:20     ` Ard Biesheuvel
  0 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2019-04-16 16:20 UTC (permalink / raw)
  To: Gary Lin
  Cc: edk2-devel-groups-io, Michael D Kinney, Andrew Fish,
	Leif Lindholm, Star Zeng, Eric Dong, Ruiyu Ni, Liming Gao,
	Jaben Carsey, Steven Shi, Jian J Wang, Hao Wu

On Mon, 15 Apr 2019 at 23:02, Gary Lin <glin@suse.com> wrote:
>
> On Sun, Apr 14, 2019 at 12:52:29PM -0700, Ard Biesheuvel wrote:
> > Delete the explicit machine type check for option ROM images, and instead,
> > rely on the LoadImage() boot service to decide whether an option ROM can
> > be dispatched or not. This permits platforms to ship with emulators to
> > execute option ROMs that are not native to the processor architecture.
> >
> After applying this patch, my OVMF VM failed to boot if e1000 or iPXE
> were enabled. It failed with the following message:
>
> Loading driver at 0x0007E537000 EntryPoint=0x0007E53C06D 8086100e.efi
> InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7F003B98
> ProtectUefiImageCommon - 0x7F002BC0
>   - 0x000000007E537000 - 0x000000000009F900
> Image type IA32 can't be started on X64 UEFI system.
> ASSERT MdeModulePkg/Core/Dxe/Mem/Pool.c(698): Head->Signature == ((('p') | ('h' << 8)) | ((('d') | ('0' << 8)) << 16)) || Head->Signature == ((('p') | ('h' << 8)) | ((('d') | ('1' << 8)) << 16))
>
> It seems OVMF was trying to load the IA32 driver and failed.
>

Thanks Gary, I am looking into this.

It should fail gracefully after failing to start the image, but
perhaps we should not attempt to start cross-type images in the first
place (which is closer to the previous situation)

-- 
Ard.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-04-16 16:21 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-14 19:52 [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Ard Biesheuvel
2019-04-14 19:52 ` [PATCH v6 1/7] MdeModulePkg: introduce PE/COFF image emulator protocol Ard Biesheuvel
2019-04-15  1:23   ` Wu, Hao A
2019-04-15  9:02     ` Leif Lindholm
2019-04-15 14:22       ` [edk2-devel] " Liming Gao
2019-04-15 15:53       ` Michael D Kinney
2019-04-15 21:52         ` Michael D Kinney
2019-04-14 19:52 ` [PATCH v6 2/7] MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images Ard Biesheuvel
2019-04-15  1:23   ` Wu, Hao A
2019-04-14 19:52 ` [PATCH v6 3/7] MdeModulePkg/PciBusDxe: dispatch option ROMs for foreign architectures Ard Biesheuvel
2019-04-15 20:23   ` [edk2-devel] " Ni, Ray
2019-04-16  6:02   ` Gary Lin
2019-04-16 16:20     ` Ard Biesheuvel
2019-04-14 19:52 ` [PATCH v6 4/7] MdeModulePkg/UefiBootManagerLib: allow foreign Driver#### images Ard Biesheuvel
2019-04-15 20:22   ` [edk2-devel] " Ni, Ray
2019-04-14 19:52 ` [PATCH v6 5/7] MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol Ard Biesheuvel
2019-04-14 19:52 ` [PATCH v6 6/7] MdePkg/UefiBaseType.h: treat EBC as a non-native machine type Ard Biesheuvel
2019-04-14 19:52 ` [PATCH v6 7/7] MdeModulePkg/DxeCore: remove explicit EBC handling Ard Biesheuvel
2019-04-15  1:25 ` [PATCH v6 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Wu, Hao A
2019-04-15  2:04   ` [edk2-devel] " Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox