public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader
@ 2020-02-25  9:39 Ard Biesheuvel
  2020-02-25  9:39 ` [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID Ard Biesheuvel
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

This is tagged as a v2 since it is a followup to a couple of patches [0][1]
that have already been sent to the list.

This series is part of my effort to define a generic EFI boot protocol for
Linux, i.e,. one that is the same across all different architectures that
are able to boot Linux from EFI, and naturally reused the firmware's
infrastructure for authenticated boot and measured boot.

Path #1 ... #4 implement the 'initrd' dynamic shell command, which takes a
file and exposes it via the LoadFile2 protocol installed on a vendor media
device path with guid LINUX_EFI_INITRD_MEDIA_GUID. This is a Linux specific,
but arch-agnostic way for the OS loader to load an initial ramdisk, while
leaving the firmware (or bootloader) in charge of where the file contents
are served from. This supersedes the currently existing solutions on Linux,
which are either limited to loading from the same volume that the OS loader
was loaded from, or load the initrd into memory first, and use architecture
specific data structures to pass on the information regarding base and size.

Patch #5 is an update to the integration of the PE/COFF emulator protocol,
to align it more closely with how LoadImage() and StartImage() behave today:
LoadImage() is not restricted to images that can execute natively on the
platform, but also permits loading of cross-type supported images. This means
that any judgement on whether an image can be *started* needs to be deferred
until StartImage(), which is why the invocation of the RegisterImage()
callback needs to be deferred as well.

Patch #6 implements the PE/COFF emulator protocol so it can start X64 images
that have been loaded on IA32 firmware. This is needed for Linux's so-called
'mixed mode', which is an elaborate scheme of on-the-fly translation of data
structures and thunking into 32-bit compat mode, allowing X64 Linux kernels
to be used on X64 capable hardware that shipped with IA32 firmware. This
needs support from the loader, and is currently implemented in GRUB (and
OVMF's command line kernel loader) using the EFI handover protocol, which
relies far too much on knowledge of kernel internal data structures, and
circumvents LoadImage and StartImage entirely.
(Note: mixed mode support is mainly targeted at cheap Atom tablets that
shipped with a [cheaper] 32-bit version of Windows, and so this particular
patch is unlikely to help that use case, but it is useful for validation.)

With these changes in place, we can boot x86 mixed-mode Linux straight from
the UEFI Shell

Shell>initrd fs0:\initrd.img
Shell>fs0:\bzImage root=/dev/vda2

Another benefit of this approach is that we can exit cleanly from the loader
(and back to the shell) using the Exit() boot service if any errors occur,
whereas the EFI handover protocol enters a deadloop upon any error that
occurs during execution of the EFI stub.

Changes from v1:
- Use a dynamic UEFI shell command, which is the recommended way of implementing
  new shell commands that are not covered by the UEFI shell specification. It
  also makes the command more easily usable on existing platforms, since the
  driver can be loaded as an ordinary driver.
- split initrd patch into 4, as requested by Laszlo
- add patch to tweak the LoadImage/StartImage behavior wrt the PE/COFF emulator
  protocol
- return EFI_UNSUPPORTED from PeCoffEmu::RegisterImage() if the image does not
  have the required .compat section

[0] https://edk2.groups.io/g/devel/topic/rfc_patch_1_1_ovmfpkg_add/71177416
[1] https://edk2.groups.io/g/devel/topic/patch_1_1_ovmfpkg_ia32_add/71272266

Cc: lersek@redhat.com
Cc: leif@nuviainc.com
Cc: pjones@redhat.com
Cc: mjg59@google.com
Cc: agraf@csgraf.de
Cc: daniel.kiper@oracle.com
Cc: michael.d.kinney@intel.com
Cc: jian.j.wang@intel.com
Cc: hao.a.wu@intel.com
Cc: ray.ni@intel.com
Cc: zhichao.gao@intel.com

Ard Biesheuvel (6):
  OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID
  OvmfPkg: add 'initrd' shell command to expose Linux initrd via device
    path
  ArmVirtPkg: add the 'initrd' dynamic shell command
  OvmfPkg: add the 'initrd' dynamic shell command
  MdeModulePkg/DxeCore: defer PE/COFF emulator registration to
    StartImage
  OvmfPkg IA32: add support for loading X64 images

 ArmVirtPkg/ArmVirt.dsc.inc                    |   4 +
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc          |   1 +
 ArmVirtPkg/ArmVirtXen.fdf                     |   1 +
 MdeModulePkg/Core/Dxe/Image/Image.c           |  24 +-
 .../CompatImageLoaderDxe.c                    | 140 ++++++
 .../CompatImageLoaderDxe.inf                  |  36 ++
 OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h    |  17 +
 .../LinuxInitrdDynamicShellCommand.c          | 423 ++++++++++++++++++
 .../LinuxInitrdDynamicShellCommand.inf        |  52 +++
 .../LinuxInitrdDynamicShellCommand.uni        |  49 ++
 OvmfPkg/OvmfPkg.dec                           |   1 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   9 +
 OvmfPkg/OvmfPkgIa32.fdf                       |   5 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   4 +
 OvmfPkg/OvmfPkgIa32X64.fdf                    |   1 +
 OvmfPkg/OvmfPkgX64.dsc                        |   4 +
 OvmfPkg/OvmfPkgX64.fdf                        |   1 +
 OvmfPkg/OvmfXen.dsc                           |   4 +
 OvmfPkg/OvmfXen.fdf                           |   1 +
 19 files changed, 766 insertions(+), 11 deletions(-)
 create mode 100644 OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
 create mode 100644 OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
 create mode 100644 OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h
 create mode 100644 OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c
 create mode 100644 OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 create mode 100644 OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni

-- 
2.17.1


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

* [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID
  2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
@ 2020-02-25  9:39 ` Ard Biesheuvel
  2020-02-25 22:14   ` [edk2-devel] " Laszlo Ersek
  2020-02-25  9:39 ` [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path Ard Biesheuvel
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

Add LINUX_EFI_INITRD_MEDIA_GUID to our collection of GUID definitions,
it can be used in a media device path to specify a Linux style initrd
that can be loaded by the OS using the LoadFile2 protocol.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h | 17 +++++++++++++++++
 OvmfPkg/OvmfPkg.dec                        |  1 +
 2 files changed, 18 insertions(+)

diff --git a/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h b/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h
new file mode 100644
index 000000000000..83fc3fc79aa6
--- /dev/null
+++ b/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h
@@ -0,0 +1,17 @@
+/** @file
+  GUID definition for the Linux Initrd media device path
+
+  Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef LINUX_EFI_INITRD_MEDIA_GUID_H__
+#define LINUX_EFI_INITRD_MEDIA_GUID_H__
+
+#define LINUX_EFI_INITRD_MEDIA_GUID \
+  {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
+
+extern EFI_GUID gLinuxEfiInitrdMediaGuid;
+
+#endif
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4c5b6511cb97..6849a79cd8b0 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -86,6 +86,7 @@ [Guids]
   gMicrosoftVendorGuid                = {0x77fa9abd, 0x0359, 0x4d32, {0xbd, 0x60, 0x28, 0xf4, 0xe7, 0x8f, 0x78, 0x4b}}
   gEfiLegacyBiosGuid                  = {0x2E3044AC, 0x879F, 0x490F, {0x97, 0x60, 0xBB, 0xDF, 0xAF, 0x69, 0x5F, 0x50}}
   gEfiLegacyDevOrderVariableGuid      = {0xa56074db, 0x65fe, 0x45f7, {0xbd, 0x21, 0x2d, 0x2b, 0xdd, 0x8e, 0x96, 0x52}}
+  gLinuxEfiInitrdMediaGuid            = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
 
 [Protocols]
   gVirtioDeviceProtocolGuid           = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
-- 
2.17.1


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

* [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
  2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
  2020-02-25  9:39 ` [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID Ard Biesheuvel
@ 2020-02-25  9:39 ` Ard Biesheuvel
  2020-02-25 23:43   ` [edk2-devel] " Laszlo Ersek
  2020-02-25  9:39 ` [PATCH v2 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command Ard Biesheuvel
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

Add a new 'initrd' command to the UEFI Shell that allows any file that is
accessible to the shell to be registered as the initrd that is returned
when Linux's EFI stub loader invokes the LoadFile2 protocol on its special
vendor media device path.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c   | 423 ++++++++++++++++++++
 OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf |  52 +++
 OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni |  49 +++
 3 files changed, 524 insertions(+)

diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c
new file mode 100644
index 000000000000..2e8541be07d5
--- /dev/null
+++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c
@@ -0,0 +1,423 @@
+/** @file
+  Provides 'initrd' dynamic UEFI shell command to load a Linux initrd
+  via its GUIDed vendor media path
+
+  Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Uefi.h>
+
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/HiiLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/ShellLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiHiiServicesLib.h>
+
+#include <Guid/LinuxEfiInitrdMedia.h>
+
+#include <Protocol/DevicePath.h>
+#include <Protocol/HiiPackageList.h>
+#include <Protocol/LoadFile2.h>
+#include <Protocol/ShellDynamicCommand.h>
+
+#pragma pack(1)
+typedef struct {
+  VENDOR_DEVICE_PATH          VenMediaNode;
+  EFI_DEVICE_PATH_PROTOCOL    EndNode;
+} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
+#pragma pack()
+
+STATIC EFI_HII_HANDLE         mLinuxInitrdShellCommandHiiHandle;
+STATIC EFI_PHYSICAL_ADDRESS   mInitrdFileAddress;
+STATIC UINT64                 mInitrdFileSize;
+STATIC EFI_HANDLE             mInitrdLoadFile2Handle;
+
+STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
+  {L"-u", TypeFlag},
+  {NULL, TypeMax}
+  };
+
+STATIC CONST SINGLE_NODE_VENDOR_MEDIA_DEVPATH mInitrdDevicePath = {
+  {
+    {
+      MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
+    },
+    LINUX_EFI_INITRD_MEDIA_GUID
+  }, {
+    END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
+    { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
+  }
+};
+
+STATIC
+EFI_STATUS
+EFIAPI
+InitrdLoadFile2 (
+  IN EFI_LOAD_FILE2_PROTOCOL          *This,
+  IN EFI_DEVICE_PATH_PROTOCOL         *FilePath,
+  IN BOOLEAN                          BootPolicy,
+  IN OUT UINTN                        *BufferSize,
+  IN VOID                             *Buffer OPTIONAL
+  )
+{
+  if (BootPolicy) {
+    return EFI_UNSUPPORTED;
+  }
+
+  if (BufferSize == NULL || !IsDevicePathValid (FilePath, 0)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (FilePath->Type != END_DEVICE_PATH_TYPE ||
+      FilePath->SubType != END_ENTIRE_DEVICE_PATH_SUBTYPE ||
+      mInitrdFileSize == 0) {
+    return EFI_NOT_FOUND;
+  }
+
+  if (Buffer == NULL || *BufferSize < mInitrdFileSize) {
+    *BufferSize = mInitrdFileSize;
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  ASSERT (mInitrdFileAddress != 0);
+
+  gBS->CopyMem (Buffer, (VOID *)(UINTN)mInitrdFileAddress, mInitrdFileSize);
+  *BufferSize = mInitrdFileSize;
+  return EFI_SUCCESS;
+}
+
+STATIC CONST EFI_LOAD_FILE2_PROTOCOL     mInitrdLoadFile2 = {
+  InitrdLoadFile2,
+};
+
+STATIC
+EFI_STATUS
+UninstallLoadFile2Protocol (
+  VOID
+  )
+{
+  EFI_STATUS    Status;
+
+  if (mInitrdLoadFile2Handle) {
+    Status = gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle,
+                    &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
+                    &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
+                    NULL);
+    if (!EFI_ERROR (Status)) {
+      mInitrdLoadFile2Handle = NULL;
+    }
+  }
+  return Status;
+}
+
+STATIC
+VOID
+FreeInitrdFile (
+  VOID
+  )
+{
+  if (mInitrdFileSize != 0) {
+    gBS->FreePages (mInitrdFileAddress, EFI_SIZE_TO_PAGES (mInitrdFileSize));
+    mInitrdFileSize = 0;
+  }
+}
+
+STATIC
+EFI_STATUS
+CacheInitrdFile (
+  IN  SHELL_FILE_HANDLE   FileHandle
+  )
+{
+  EFI_STATUS              Status;
+  UINT64                  FileSize;
+  UINTN                   ReadSize;
+
+  Status = gEfiShellProtocol->GetFileSize (FileHandle, &FileSize);
+  if (EFI_ERROR(Status)) {
+    return Status;
+  }
+
+  Status = gBS->AllocatePages (AllocateAnyPages, EfiLoaderData,
+                  EFI_SIZE_TO_PAGES (FileSize), &mInitrdFileAddress);
+  if (EFI_ERROR(Status)) {
+    return Status;
+  }
+
+  ReadSize = FileSize;
+  Status = gEfiShellProtocol->ReadFile (FileHandle, &ReadSize,
+                                (VOID *)(UINTN)mInitrdFileAddress);
+  if (EFI_ERROR(Status) || ReadSize < FileSize) {
+    DEBUG ((DEBUG_WARN, "%a: failed to read initrd file - %r 0x%llx 0x%llx\n",
+      __FUNCTION__, Status, (UINT64)ReadSize, FileSize));
+    goto FreeMemory;
+  }
+
+  if (mInitrdLoadFile2Handle == NULL) {
+    Status = gBS->InstallMultipleProtocolInterfaces (&mInitrdLoadFile2Handle,
+                    &gEfiDevicePathProtocolGuid,  &mInitrdDevicePath,
+                    &gEfiLoadFile2ProtocolGuid,   &mInitrdLoadFile2,
+                    NULL);
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  mInitrdFileSize = FileSize;
+  return EFI_SUCCESS;
+
+FreeMemory:
+  gBS->FreePages (mInitrdFileAddress, EFI_SIZE_TO_PAGES (FileSize));
+  return Status;
+}
+
+/**
+  Function for 'initrd' command.
+
+  @param[in] ImageHandle  Handle to the Image (NULL if Internal).
+  @param[in] SystemTable  Pointer to the System Table (NULL if Internal).
+**/
+STATIC
+SHELL_STATUS
+EFIAPI
+RunInitrd (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS            Status;
+  LIST_ENTRY            *Package;
+  CHAR16                *ProblemParam;
+  CONST CHAR16          *Param;
+  CONST CHAR16          *Filename;
+  SHELL_STATUS          ShellStatus;
+  SHELL_FILE_HANDLE     FileHandle;
+
+  ProblemParam        = NULL;
+  ShellStatus         = SHELL_SUCCESS;
+
+  Status = ShellInitialize ();
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // parse the command line
+  //
+  Status = ShellCommandLineParse (ParamList, &Package, &ProblemParam, TRUE);
+  if (EFI_ERROR (Status)) {
+    if (Status == EFI_VOLUME_CORRUPTED && ProblemParam != NULL) {
+      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PROBLEM),
+        mLinuxInitrdShellCommandHiiHandle, L"initrd", ProblemParam);
+      FreePool (ProblemParam);
+      ShellStatus = SHELL_INVALID_PARAMETER;
+    } else {
+      ASSERT(FALSE);
+    }
+  } else {
+    if (ShellCommandLineGetCount (Package) > 2) {
+      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
+        mLinuxInitrdShellCommandHiiHandle, L"initrd");
+      ShellStatus = SHELL_INVALID_PARAMETER;
+    } else if (ShellCommandLineGetCount (Package) < 2) {
+      if (ShellCommandLineGetFlag(Package, L"-u")) {
+        FreeInitrdFile ();
+        UninstallLoadFile2Protocol ();
+      } else {
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_FEW),
+          mLinuxInitrdShellCommandHiiHandle, L"initrd");
+        ShellStatus = SHELL_INVALID_PARAMETER;
+      }
+    } else {
+      Param = ShellCommandLineGetRawValue (Package, 1);
+      ASSERT (Param != NULL);
+
+      Filename = ShellFindFilePath (Param);
+      if (Filename == NULL) {
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FIND_FAIL),
+          mLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
+        ShellStatus = SHELL_NOT_FOUND;
+      } else {
+        FreeInitrdFile ();
+
+        Status = ShellOpenFileByName (Filename, &FileHandle,
+                   EFI_FILE_MODE_READ, 0);
+        if (!EFI_ERROR (Status)) {
+          Status = CacheInitrdFile (FileHandle);
+        }
+        if (EFI_ERROR (Status)) {
+          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL),
+            mLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
+          ShellStatus = SHELL_NOT_FOUND;
+        }
+        ShellCloseFile (&FileHandle);
+      }
+    }
+  }
+  return ShellStatus;
+}
+
+
+/**
+  This is the shell command handler function pointer callback type.  This
+  function handles the command when it is invoked in the shell.
+
+  @param[in] This                   The instance of the
+                                    EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL.
+  @param[in] SystemTable            The pointer to the system table.
+  @param[in] ShellParameters        The parameters associated with the command.
+  @param[in] Shell                  The instance of the shell protocol used in
+                                    the context of processing this command.
+
+  @return EFI_SUCCESS               the operation was successful
+  @return other                     the operation failed.
+**/
+SHELL_STATUS
+EFIAPI
+LinuxInitrdCommandHandler (
+  IN EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL    *This,
+  IN EFI_SYSTEM_TABLE                      *SystemTable,
+  IN EFI_SHELL_PARAMETERS_PROTOCOL         *ShellParameters,
+  IN EFI_SHELL_PROTOCOL                    *Shell
+  )
+{
+  gEfiShellParametersProtocol = ShellParameters;
+  gEfiShellProtocol           = Shell;
+
+  return RunInitrd (gImageHandle, SystemTable);
+}
+
+/**
+  This is the command help handler function pointer callback type.  This
+  function is responsible for displaying help information for the associated
+  command.
+
+  @param[in] This                   The instance of the
+                                    EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL.
+  @param[in] Language               The pointer to the language string to use.
+
+  @return string                    Pool allocated help string, must be freed
+                                    by caller
+**/
+STATIC
+CHAR16 *
+EFIAPI
+LinuxInitrdGetHelp (
+  IN EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL    *This,
+  IN CONST CHAR8                           *Language
+  )
+{
+  return HiiGetString (mLinuxInitrdShellCommandHiiHandle,
+           STRING_TOKEN (STR_GET_HELP_INITRD), Language);
+}
+
+STATIC EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL mLinuxInitrdDynamicCommand = {
+  L"initrd",
+  LinuxInitrdCommandHandler,
+  LinuxInitrdGetHelp
+};
+
+/**
+  Retrieve HII package list from ImageHandle and publish to HII database.
+
+  @param ImageHandle            The image handle of the process.
+
+  @return HII handle.
+**/
+STATIC
+EFI_HII_HANDLE
+InitializeHiiPackage (
+  EFI_HANDLE                  ImageHandle
+  )
+{
+  EFI_STATUS                  Status;
+  EFI_HII_PACKAGE_LIST_HEADER *PackageList;
+  EFI_HII_HANDLE              HiiHandle;
+
+  //
+  // Retrieve HII package list from ImageHandle
+  //
+  Status = gBS->OpenProtocol (ImageHandle, &gEfiHiiPackageListProtocolGuid,
+                  (VOID **)&PackageList, ImageHandle, NULL,
+                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+
+  //
+  // Publish HII package list to HII Database.
+  //
+  Status = gHiiDatabase->NewPackageList (gHiiDatabase, PackageList, NULL,
+                           &HiiHandle);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+  return HiiHandle;
+}
+
+/**
+  Entry point of Linux Initrd dynamic UEFI Shell command.
+
+  Produce the DynamicCommand protocol to handle "initrd" command.
+
+  @param ImageHandle            The image handle of the process.
+  @param SystemTable            The EFI System Table pointer.
+
+  @retval EFI_SUCCESS           Initrd command is executed successfully.
+  @retval EFI_ABORTED           HII package was failed to initialize.
+  @retval others                Other errors when executing Initrd command.
+**/
+EFI_STATUS
+EFIAPI
+LinuxInitrdDynamicShellCommandEntryPoint (
+  IN EFI_HANDLE               ImageHandle,
+  IN EFI_SYSTEM_TABLE         *SystemTable
+  )
+{
+  EFI_STATUS                  Status;
+
+  mLinuxInitrdShellCommandHiiHandle = InitializeHiiPackage (ImageHandle);
+  if (mLinuxInitrdShellCommandHiiHandle == NULL) {
+    return EFI_ABORTED;
+  }
+
+  Status = gBS->InstallProtocolInterface (&ImageHandle,
+                  &gEfiShellDynamicCommandProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  &mLinuxInitrdDynamicCommand);
+  ASSERT_EFI_ERROR (Status);
+  return Status;
+}
+
+/**
+  Unload the dynamic UEFI Shell command.
+
+  @param ImageHandle            The image handle of the process.
+
+  @retval EFI_SUCCESS           The image is unloaded.
+  @retval Others                Failed to unload the image.
+**/
+EFI_STATUS
+EFIAPI
+LinuxInitrdDynamicShellCommandUnload (
+  IN EFI_HANDLE               ImageHandle
+)
+{
+  EFI_STATUS                  Status;
+
+  Status = UninstallLoadFile2Protocol ();
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = gBS->UninstallProtocolInterface (ImageHandle,
+                  &gEfiShellDynamicCommandProtocolGuid,
+                  &mLinuxInitrdDynamicCommand);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  HiiRemovePackages (mLinuxInitrdShellCommandHiiHandle);
+  return EFI_SUCCESS;
+}
diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
new file mode 100644
index 000000000000..cdfdddf77a4d
--- /dev/null
+++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
@@ -0,0 +1,52 @@
+##  @file
+# Provides 'initrd' dynamic UEFI shell command to load a Linux initrd
+# via its GUIDed vendor media path
+#
+# Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = LinuxInitrdDynamicShellCommand
+  FILE_GUID                      = 2f30da26-f51b-4b6f-85c4-31873c281bca
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = LinuxInitrdDynamicShellCommandEntryPoint
+  UNLOAD_IMAGE                   = LinuxInitrdDynamicShellCommandUnload
+  UEFI_HII_RESOURCE_SECTION      = TRUE
+
+#
+#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64 EBC
+#
+
+[Sources.common]
+  LinuxInitrdDynamicShellCommand.c
+  LinuxInitrdDynamicShellCommand.uni
+
+[Packages]
+  MdePkg/MdePkg.dec
+  ShellPkg/ShellPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  DebugLib
+  DevicePathLib
+  HiiLib
+  MemoryAllocationLib
+  ShellLib
+  UefiBootServicesTableLib
+  UefiDriverEntryPoint
+  UefiHiiServicesLib
+
+[Protocols]
+  gEfiHiiPackageListProtocolGuid                  ## CONSUMES
+  gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
+  gEfiShellDynamicCommandProtocolGuid             ## PRODUCES
+
+[DEPEX]
+  TRUE
diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni
new file mode 100644
index 000000000000..a88fa6e3641b
--- /dev/null
+++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni
@@ -0,0 +1,49 @@
+// /**
+//
+// Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// Module Name:
+//
+// LinuxInitrdDynamicShellCommand.uni
+//
+// Abstract:
+//
+// String definitions for 'initrd' UEFI Shell command
+//
+// **/
+
+/=#
+
+#langdef   en-US "english"
+
+#string STR_GEN_PROBLEM           #language en-US "%H%s%N: Unknown flag - '%H%s%N'\r\n"
+#string STR_GEN_TOO_MANY          #language en-US "%H%s%N: Too many arguments.\r\n"
+#string STR_GEN_TOO_FEW           #language en-US "%H%s%N: Too few arguments.\r\n"
+#string STR_GEN_FIND_FAIL         #language en-US "%H%s%N: File not found - '%H%s%N'\r\n"
+#string STR_GEN_FILE_OPEN_FAIL    #language en-US "%H%s%N: Cannot open file - '%H%s%N'\r\n"
+
+#string STR_GET_HELP_INITRD       #language en-US ""
+".TH initrd 0 "Registers or unregisters a file as Linux initrd."\r\n"
+".SH NAME\r\n"
+"Registers or unregisters a file as Linux initrd.\r\n"
+".SH SYNOPSIS\r\n"
+" \r\n"
+"initrd <FileName>\r\n"
+"initrd -u\r\n"
+".SH OPTIONS\r\n"
+" \r\n"
+"  FileName    - Specifies a file to register as initrd.\r\n"
+"  -u          - Unregisters any previously registered initrd files.\r\n"
+".SH DESCRIPTION\r\n"
+" \r\n"
+"NOTES:\r\n"
+"  1. Only a single file can be loaded as initrd at any given time. Using the\r\n"
+"     command twice with a <FileName> option will result in the first file to\r\n"
+"     be unloaded again, regardless of whether the second invocation succeeded\r\n"
+"     or not.\r\n"
+"  2. The initrd is not unloaded when the shell exits, and will remain active\r\n"
+"     until it is unloaded again by a different invocation of the shell.\r\n"
+"     Consumers of the LoadFile2 protocol on the LINUX_EFI_INITRD_MEDIA_GUID\r\n"
+"     device path that are started via means other than the shell will be able\r\n"
+"     to locate the protocol and invoke it.\r\n"
-- 
2.17.1


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

* [PATCH v2 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command
  2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
  2020-02-25  9:39 ` [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID Ard Biesheuvel
  2020-02-25  9:39 ` [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path Ard Biesheuvel
@ 2020-02-25  9:39 ` Ard Biesheuvel
  2020-02-25 23:46   ` [edk2-devel] " Laszlo Ersek
  2020-02-25  9:39 ` [PATCH v2 4/6] OvmfPkg: " Ard Biesheuvel
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

Add the 'initrd' dynamic shell command to the build so we can load
Linux initrds straight from the shell using the new generic protocol,
which does not rely on initrd= being passed on the command line.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc           | 4 ++++
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
 ArmVirtPkg/ArmVirtXen.fdf            | 1 +
 3 files changed, 6 insertions(+)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 10037c938eb8..608dfe1aa03d 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -401,6 +401,10 @@ [Components.common]
       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
       gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
   }
+  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 
 [Components.AARCH64]
   #
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 31f615a9d0f9..bfa380815f1a 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -103,6 +103,7 @@ [FV.FvMain]
   #
   INF ShellPkg/Application/Shell/Shell.inf
   INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+  INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 
   #
   # Bds
diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
index 38d4cc163524..6a97bceeacbc 100644
--- a/ArmVirtPkg/ArmVirtXen.fdf
+++ b/ArmVirtPkg/ArmVirtXen.fdf
@@ -182,6 +182,7 @@ [FV.FvMain]
   #
   INF ShellPkg/Application/Shell/Shell.inf
   INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+  INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 
   #
   # Bds
-- 
2.17.1


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

* [PATCH v2 4/6] OvmfPkg: add the 'initrd' dynamic shell command
  2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2020-02-25  9:39 ` [PATCH v2 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command Ard Biesheuvel
@ 2020-02-25  9:39 ` Ard Biesheuvel
  2020-02-25 23:48   ` [edk2-devel] " Laszlo Ersek
  2020-02-25  9:39 ` [PATCH v2 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage Ard Biesheuvel
  2020-02-25  9:39 ` [PATCH v2 6/6] OvmfPkg IA32: add support for loading X64 images Ard Biesheuvel
  5 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

Add the 'initrd' dynamic shell command to the build so we can load
Linux initrds straight from the shell using the new generic protocol,
which does not rely on initrd= being passed on the command line.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
 OvmfPkg/OvmfPkgIa32.fdf    | 1 +
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
 OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
 OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
 OvmfPkg/OvmfPkgX64.fdf     | 1 +
 OvmfPkg/OvmfXen.dsc        | 4 ++++
 OvmfPkg/OvmfXen.fdf        | 1 +
 8 files changed, 20 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 19728f20b34e..870eb7aa7429 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -843,6 +843,10 @@ [Components]
       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
       gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
   }
+  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 63607551ed75..b6cd5da4f2b3 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -282,6 +282,7 @@ [FV.DXEFV]
 INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
 !endif
 INF  ShellPkg/Application/Shell/Shell.inf
+INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 
 INF MdeModulePkg/Logo/LogoDxe.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 3c0c229e3a72..8022365e664c 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -856,6 +856,10 @@ [Components.X64]
       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
       gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
   }
+  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 0488e5d95ffe..159840836775 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -283,6 +283,7 @@ [FV.DXEFV]
 INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
 !endif
 INF  ShellPkg/Application/Shell/Shell.inf
+INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 
 INF MdeModulePkg/Logo/LogoDxe.inf
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f6c1d8d228c6..90c0bd16e6a0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -854,6 +854,10 @@ [Components]
       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
       gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
   }
+  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 0488e5d95ffe..159840836775 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -283,6 +283,7 @@ [FV.DXEFV]
 INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
 !endif
 INF  ShellPkg/Application/Shell/Shell.inf
+INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 
 INF MdeModulePkg/Logo/LogoDxe.inf
 
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 5751ff1f0352..6612018ad6a8 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -714,6 +714,10 @@ [Components]
       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
       gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
   }
+  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 
   OvmfPkg/PlatformDxe/Platform.inf
   OvmfPkg/AmdSevDxe/AmdSevDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 05138ffc5b4f..cfdb01a80a00 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -365,6 +365,7 @@ [FV.DXEFV]
 INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
 !endif
 INF  ShellPkg/Application/Shell/Shell.inf
+INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 
 INF MdeModulePkg/Logo/LogoDxe.inf
 
-- 
2.17.1


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

* [PATCH v2 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage
  2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2020-02-25  9:39 ` [PATCH v2 4/6] OvmfPkg: " Ard Biesheuvel
@ 2020-02-25  9:39 ` Ard Biesheuvel
  2020-02-25  9:39 ` [PATCH v2 6/6] OvmfPkg IA32: add support for loading X64 images Ard Biesheuvel
  5 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

EDK2's implementation of the LoadImage() boot service permits non-native
binaries to be loaded (i.e., X64 images on IA32 firmware), but any
attempts to start such an image using StartImage() will return
EFI_UNSUPPORTED.

The integration of the PE/COFF emulator protocol into the DXE core
deviates slightly from this paradigm, given that its IsImageSupported
hook as well as its RegisterImage hook are invoked from LoadImage,
and by the time StartImage is called, no opportunity is given to the
provider of the PE/COFF emulator protocol to prevent an image from
being started if it only supports loading it.

To address this disparity, let's move the invocation of RegisterImage()
to the implementation of the StartImage() boot service, allowing the
emulator to permit LoadImage() but reject StartImage() on images that
turn out not to meet the requirements of the emulator as it is being
started.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 24 +++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 22a87ecf6d7c..d86da89ee704 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -756,17 +756,6 @@ CoreLoadPeImage (
   // Get the image entry point.
   //
   Image->EntryPoint   = (EFI_IMAGE_ENTRY_POINT)(UINTN)Image->ImageContext.EntryPoint;
-  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;
-    }
-  }
 
   //
   // Fill in the image information for the Loaded Image Protocol
@@ -1603,6 +1592,19 @@ CoreStartImage (
     return EFI_UNSUPPORTED;
   }
 
+  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 - %r\n",
+          Status));
+      return Status;
+    }
+  }
+
   PERF_START_IMAGE_BEGIN (Handle);
 
 
-- 
2.17.1


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

* [PATCH v2 6/6] OvmfPkg IA32: add support for loading X64 images
  2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
                   ` (4 preceding siblings ...)
  2020-02-25  9:39 ` [PATCH v2 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage Ard Biesheuvel
@ 2020-02-25  9:39 ` Ard Biesheuvel
  2020-02-25 23:55   ` [edk2-devel] " Laszlo Ersek
  5 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2020-02-25  9:39 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	michael.d.kinney, jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

This is the UEFI counterpart to my Linux series which generalizes
mixed mode support into a feature that requires very little internal
knowledge about the architecture specifics of booting Linux on the
part of the bootloader or firmware.

Instead, we add a .compat PE/COFF header containing an array of
PE_COMPAT nodes containing <machine type, entrypoint> tuples that
describe alternate entrypoints into the image for different native
machine types, e.g., IA-32 in a 64-bit image so it can be booted
from IA-32 firmware.

This patch implements the PE/COFF emulator protocol to take this new
section into account, so that such images can simply be loaded via
LoadImage/StartImage, e.g., straight from the shell.

This feature is based on the EDK2 specific PE/COFF emulator protocol
that was introduced in commit 57df17fe26cd ("MdeModulePkg/DxeCore:
invoke the emulator protocol for foreign images", 2019-04-14).

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c   | 140 ++++++++++++++++++++
 OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf |  36 +++++
 OvmfPkg/OvmfPkgIa32.dsc                               |   5 +
 OvmfPkg/OvmfPkgIa32.fdf                               |   4 +
 4 files changed, 185 insertions(+)

diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
new file mode 100644
index 000000000000..d2ae03eabf7f
--- /dev/null
+++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
@@ -0,0 +1,140 @@
+/** @file
+ *  PE/COFF emulator protocol implementation to start Linux kernel
+ *  images from non-native firmware
+ *
+ *  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
+ *
+ *  SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ */
+
+#include <PiDxe.h>
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/PeCoffLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiDriverEntryPoint.h>
+
+#include <Protocol/PeCoffImageEmulator.h>
+
+#pragma pack(1)
+typedef struct {
+  UINT8   Type;
+  UINT8   Size;
+  UINT16  MachineType;
+  UINT32  EntryPoint;
+} PE_COMPAT_TYPE1;
+#pragma pack()
+
+STATIC
+BOOLEAN
+EFIAPI
+IsImageSupported (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN  UINT16                                  ImageType,
+  IN  EFI_DEVICE_PATH_PROTOCOL                *DevicePath   OPTIONAL
+  )
+{
+  return ImageType == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION;
+}
+
+STATIC
+EFI_IMAGE_ENTRY_POINT
+EFIAPI
+GetCompatEntryPoint (
+  IN  EFI_PHYSICAL_ADDRESS              ImageBase
+  )
+{
+  EFI_IMAGE_DOS_HEADER                  *DosHdr;
+  UINTN                                 PeCoffHeaderOffset;
+  EFI_IMAGE_NT_HEADERS32                *Pe32;
+  EFI_IMAGE_SECTION_HEADER              *Section;
+  UINTN                                 NumberOfSections;
+  PE_COMPAT_TYPE1                       *PeCompat;
+
+  DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageBase;
+  if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) {
+    return NULL;
+  }
+
+  PeCoffHeaderOffset = DosHdr->e_lfanew;
+  Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageBase + PeCoffHeaderOffset);
+
+  Section = (EFI_IMAGE_SECTION_HEADER *)((UINTN)&Pe32->OptionalHeader +
+                                         Pe32->FileHeader.SizeOfOptionalHeader);
+  NumberOfSections = (UINTN)Pe32->FileHeader.NumberOfSections;
+
+  while (NumberOfSections--) {
+    if (!CompareMem (Section->Name, ".compat", sizeof (Section->Name))) {
+      //
+      // Dereference the section contents to find the mixed mode entry point
+      //
+      PeCompat = (PE_COMPAT_TYPE1 *)((UINTN)ImageBase + Section->VirtualAddress);
+
+      while (PeCompat->Type != 0) {
+        if (PeCompat->Type == 1 &&
+            PeCompat->Size >= sizeof (PE_COMPAT_TYPE1) &&
+            EFI_IMAGE_MACHINE_TYPE_SUPPORTED (PeCompat->MachineType)) {
+
+          return (EFI_IMAGE_ENTRY_POINT)((UINTN)ImageBase + PeCompat->EntryPoint);
+        }
+        PeCompat = (PE_COMPAT_TYPE1 *)((UINTN)PeCompat + PeCompat->Size);
+      }
+    }
+    Section++;
+  }
+  return NULL;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+RegisterImage (
+  IN      EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN      EFI_PHYSICAL_ADDRESS                    ImageBase,
+  IN      UINT64                                  ImageSize,
+  IN  OUT EFI_IMAGE_ENTRY_POINT                   *EntryPoint
+  )
+{
+  EFI_IMAGE_ENTRY_POINT                           CompatEntryPoint;
+
+  CompatEntryPoint = GetCompatEntryPoint (ImageBase);
+  if (CompatEntryPoint == NULL) {
+    return EFI_UNSUPPORTED;
+  }
+
+  *EntryPoint = CompatEntryPoint;
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+UnregisterImage (
+  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
+  IN  EFI_PHYSICAL_ADDRESS                    ImageBase
+  )
+{
+  return EFI_SUCCESS;
+}
+
+STATIC EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL mCompatLoaderPeCoffEmuProtocol = {
+  IsImageSupported,
+  RegisterImage,
+  UnregisterImage,
+  EDKII_PECOFF_IMAGE_EMULATOR_VERSION,
+  EFI_IMAGE_MACHINE_X64
+};
+
+EFI_STATUS
+EFIAPI
+CompatImageLoaderDxeEntryPoint (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  return gBS->InstallProtocolInterface (&ImageHandle,
+                &gEdkiiPeCoffImageEmulatorProtocolGuid,
+                EFI_NATIVE_INTERFACE,
+                &mCompatLoaderPeCoffEmuProtocol);
+}
diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
new file mode 100644
index 000000000000..82369384fbe6
--- /dev/null
+++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
@@ -0,0 +1,36 @@
+## @file
+#  PE/COFF emulator protocol implementation to start Linux kernel
+#  images from non-native firmware
+#
+#  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = CompatImageLoaderDxe
+  FILE_GUID                      = 1019f54a-2560-41b2-87b0-6750b98f3eff
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = CompatImageLoaderDxeEntryPoint
+
+[Sources]
+  CompatImageLoaderDxe.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+
+[LibraryClasses]
+  BaseMemoryLib
+  PeCoffLib
+  UefiBootServicesTableLib
+  UefiDriverEntryPoint
+
+[Protocols]
+  gEdkiiPeCoffImageEmulatorProtocolGuid   ## PRODUCES
+
+[Depex]
+  TRUE
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 870eb7aa7429..a996c0a7a7c9 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -33,6 +33,7 @@ [Defines]
   DEFINE SOURCE_DEBUG_ENABLE     = FALSE
   DEFINE TPM2_ENABLE             = FALSE
   DEFINE TPM2_CONFIG_ENABLE      = FALSE
+  DEFINE LOAD_X64_ON_IA32_ENABLE = FALSE
 
   #
   # Network definition
@@ -932,3 +933,7 @@ [Components]
   SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
 !endif
 !endif
+
+!if $(LOAD_X64_ON_IA32_ENABLE) == TRUE
+  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
+!endif
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index b6cd5da4f2b3..ff8d80859fb9 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -354,6 +354,10 @@ [FV.DXEFV]
 !endif
 !endif
 
+!if $(LOAD_X64_ON_IA32_ENABLE) == TRUE
+INF  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
+!endif
+
 ################################################################################
 
 [FV.FVMAIN_COMPACT]
-- 
2.17.1


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

* Re: [edk2-devel] [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID
  2020-02-25  9:39 ` [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID Ard Biesheuvel
@ 2020-02-25 22:14   ` Laszlo Ersek
  0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-25 22:14 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/25/20 10:39, Ard Biesheuvel wrote:
> Add LINUX_EFI_INITRD_MEDIA_GUID to our collection of GUID definitions,
> it can be used in a media device path to specify a Linux style initrd
> that can be loaded by the OS using the LoadFile2 protocol.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h | 17 +++++++++++++++++
>  OvmfPkg/OvmfPkg.dec                        |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h b/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h
> new file mode 100644
> index 000000000000..83fc3fc79aa6
> --- /dev/null
> +++ b/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h
> @@ -0,0 +1,17 @@
> +/** @file
> +  GUID definition for the Linux Initrd media device path
> +
> +  Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef LINUX_EFI_INITRD_MEDIA_GUID_H__
> +#define LINUX_EFI_INITRD_MEDIA_GUID_H__
> +
> +#define LINUX_EFI_INITRD_MEDIA_GUID \
> +  {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
> +
> +extern EFI_GUID gLinuxEfiInitrdMediaGuid;
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 4c5b6511cb97..6849a79cd8b0 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -86,6 +86,7 @@ [Guids]
>    gMicrosoftVendorGuid                = {0x77fa9abd, 0x0359, 0x4d32, {0xbd, 0x60, 0x28, 0xf4, 0xe7, 0x8f, 0x78, 0x4b}}
>    gEfiLegacyBiosGuid                  = {0x2E3044AC, 0x879F, 0x490F, {0x97, 0x60, 0xBB, 0xDF, 0xAF, 0x69, 0x5F, 0x50}}
>    gEfiLegacyDevOrderVariableGuid      = {0xa56074db, 0x65fe, 0x45f7, {0xbd, 0x21, 0x2d, 0x2b, 0xdd, 0x8e, 0x96, 0x52}}
> +  gLinuxEfiInitrdMediaGuid            = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
>  
>  [Protocols]
>    gVirtioDeviceProtocolGuid           = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


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

* Re: [edk2-devel] [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
  2020-02-25  9:39 ` [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path Ard Biesheuvel
@ 2020-02-25 23:43   ` Laszlo Ersek
  2020-02-26  0:00     ` Laszlo Ersek
  2020-02-26  1:25     ` Laszlo Ersek
  0 siblings, 2 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-25 23:43 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/25/20 10:39, Ard Biesheuvel wrote:
> Add a new 'initrd' command to the UEFI Shell that allows any file that is
> accessible to the shell to be registered as the initrd that is returned
> when Linux's EFI stub loader invokes the LoadFile2 protocol on its special
> vendor media device path.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c   | 423 ++++++++++++++++++++
>  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf |  52 +++
>  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni |  49 +++
>  3 files changed, 524 insertions(+)
> 
> diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c
> new file mode 100644
> index 000000000000..2e8541be07d5
> --- /dev/null
> +++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c
> @@ -0,0 +1,423 @@
> +/** @file
> +  Provides 'initrd' dynamic UEFI shell command to load a Linux initrd
> +  via its GUIDed vendor media path
> +
> +  Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/HiiLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/ShellLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiHiiServicesLib.h>
> +
> +#include <Guid/LinuxEfiInitrdMedia.h>
> +
> +#include <Protocol/DevicePath.h>

(1) Please add gEfiDevicePathProtocolGuid to the INF file, under
[Protocols].


> +#include <Protocol/HiiPackageList.h>
> +#include <Protocol/LoadFile2.h>
> +#include <Protocol/ShellDynamicCommand.h>
> +
> +#pragma pack(1)
> +typedef struct {
> +  VENDOR_DEVICE_PATH          VenMediaNode;
> +  EFI_DEVICE_PATH_PROTOCOL    EndNode;
> +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
> +#pragma pack()

(2) please insert spaces before the opening parens.

> +
> +STATIC EFI_HII_HANDLE         mLinuxInitrdShellCommandHiiHandle;
> +STATIC EFI_PHYSICAL_ADDRESS   mInitrdFileAddress;
> +STATIC UINT64                 mInitrdFileSize;
> +STATIC EFI_HANDLE             mInitrdLoadFile2Handle;
> +
> +STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> +  {L"-u", TypeFlag},
> +  {NULL, TypeMax}
> +  };
> +
> +STATIC CONST SINGLE_NODE_VENDOR_MEDIA_DEVPATH mInitrdDevicePath = {
> +  {
> +    {
> +      MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
> +    },
> +    LINUX_EFI_INITRD_MEDIA_GUID
> +  }, {
> +    END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
> +  }
> +};
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +InitrdLoadFile2 (
> +  IN EFI_LOAD_FILE2_PROTOCOL          *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL         *FilePath,
> +  IN BOOLEAN                          BootPolicy,
> +  IN OUT UINTN                        *BufferSize,
> +  IN VOID                             *Buffer OPTIONAL

(3) Possibly a bug in a standard edk2 header file (or maybe even in the
spec), but I think Buffer should be marked OUT.

> +  )
> +{
> +  if (BootPolicy) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  if (BufferSize == NULL || !IsDevicePathValid (FilePath, 0)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (FilePath->Type != END_DEVICE_PATH_TYPE ||
> +      FilePath->SubType != END_ENTIRE_DEVICE_PATH_SUBTYPE ||
> +      mInitrdFileSize == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  if (Buffer == NULL || *BufferSize < mInitrdFileSize) {
> +    *BufferSize = mInitrdFileSize;
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  ASSERT (mInitrdFileAddress != 0);
> +
> +  gBS->CopyMem (Buffer, (VOID *)(UINTN)mInitrdFileAddress, mInitrdFileSize);
> +  *BufferSize = mInitrdFileSize;
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC CONST EFI_LOAD_FILE2_PROTOCOL     mInitrdLoadFile2 = {
> +  InitrdLoadFile2,
> +};
> +
> +STATIC
> +EFI_STATUS
> +UninstallLoadFile2Protocol (
> +  VOID
> +  )
> +{
> +  EFI_STATUS    Status;
> +
> +  if (mInitrdLoadFile2Handle) {

(4) Please spell out the "!= NULL" explicitly (per edk2 coding style /
preference)

> +    Status = gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle,
> +                    &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
> +                    &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
> +                    NULL);
> +    if (!EFI_ERROR (Status)) {
> +      mInitrdLoadFile2Handle = NULL;
> +    }
> +  }
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +FreeInitrdFile (
> +  VOID
> +  )
> +{
> +  if (mInitrdFileSize != 0) {
> +    gBS->FreePages (mInitrdFileAddress, EFI_SIZE_TO_PAGES (mInitrdFileSize));
> +    mInitrdFileSize = 0;
> +  }
> +}
> +
> +STATIC
> +EFI_STATUS
> +CacheInitrdFile (
> +  IN  SHELL_FILE_HANDLE   FileHandle
> +  )
> +{
> +  EFI_STATUS              Status;
> +  UINT64                  FileSize;
> +  UINTN                   ReadSize;
> +
> +  Status = gEfiShellProtocol->GetFileSize (FileHandle, &FileSize);
> +  if (EFI_ERROR(Status)) {
> +    return Status;
> +  }

So, this is tricky.

The method called above indeed outputs a UINT64, into FileSize. I guess
that's why you also made "mInitrdFileSize" a UINT64.

However, every *read* of "mInitrdFileSize" in this file actually wants
the variable to be UINTN. I've checked:

- (*BufferSize) in InitrdLoadFile2() is a UINTN

- gBS->CopyMem() -- EFI_COPY_MEM -- takes a UINTN

- EFI_SIZE_TO_PAGES() takes a UINTN

- gBS->FreePages() takes a UINTN

So please,

(5a) flip the type of "mInitrdFileSize" to UINTN,

(5b) right here, please check if FileSize (which *should* remain a
UINT64) exceeds MAX_UINTN. If it does, the function should return
EFI_UNSUPPORTED.

(Effectively, that could happen if the user attempted to load a >=4GB
file as initrd in a 32-bit shell.)

(5c) (mInitrdFileSize == 0) carries special meaning in this
implementation. I suggest catching (FileSize==0) right here, and
returning with EFI_UNSUPPORTED also. (That will at once eliminate
questions like, "how does AllocatePages() react to zero pages
requested?", too.)


> +
> +  Status = gBS->AllocatePages (AllocateAnyPages, EfiLoaderData,

Hmmm. Ultimately, the allocation will be performed by the UEFI shell,
and so EfiLoaderData is justified, per "Table 29. Memory Type Usage
before ExitBootServices()". OK.

> +                  EFI_SIZE_TO_PAGES (FileSize), &mInitrdFileAddress);

(6) Please cast FileSize to UINTN here.

> +  if (EFI_ERROR(Status)) {

(7) missing whitespace (please check the other EFI_ERROR() invocations
too, there are other instances)

> +    return Status;
> +  }
> +
> +  ReadSize = FileSize;

(Right, this assignment depends on (5b) too)

> +  Status = gEfiShellProtocol->ReadFile (FileHandle, &ReadSize,
> +                                (VOID *)(UINTN)mInitrdFileAddress);
> +  if (EFI_ERROR(Status) || ReadSize < FileSize) {
> +    DEBUG ((DEBUG_WARN, "%a: failed to read initrd file - %r 0x%llx 0x%llx\n",
> +      __FUNCTION__, Status, (UINT64)ReadSize, FileSize));

(8) Please use just "%lx" for logging UINT64, not "%llx". (PrintLib
peculiarity.)  I guess the double ell might not hurt, but it suggests
that the second "l" has extra meaning, and it doesn't. ('l' and 'L'
simply set LONG_TYPE in BasePrintLibSPrintMarker(), file
"MdePkg/Library/BasePrintLib/PrintLibInternal.c".)

> +    goto FreeMemory;
> +  }
> +
> +  if (mInitrdLoadFile2Handle == NULL) {
> +    Status = gBS->InstallMultipleProtocolInterfaces (&mInitrdLoadFile2Handle,
> +                    &gEfiDevicePathProtocolGuid,  &mInitrdDevicePath,
> +                    &gEfiLoadFile2ProtocolGuid,   &mInitrdLoadFile2,
> +                    NULL);
> +    ASSERT_EFI_ERROR (Status);

OK (although it's not difficult to jump to FreeMemory here too)

> +  }
> +
> +  mInitrdFileSize = FileSize;
> +  return EFI_SUCCESS;
> +
> +FreeMemory:
> +  gBS->FreePages (mInitrdFileAddress, EFI_SIZE_TO_PAGES (FileSize));

(9) Please cast FileSize to UINTN.

> +  return Status;
> +}
> +
> +/**
> +  Function for 'initrd' command.
> +
> +  @param[in] ImageHandle  Handle to the Image (NULL if Internal).
> +  @param[in] SystemTable  Pointer to the System Table (NULL if Internal).
> +**/
> +STATIC
> +SHELL_STATUS
> +EFIAPI
> +RunInitrd (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS            Status;
> +  LIST_ENTRY            *Package;
> +  CHAR16                *ProblemParam;
> +  CONST CHAR16          *Param;
> +  CONST CHAR16          *Filename;
> +  SHELL_STATUS          ShellStatus;
> +  SHELL_FILE_HANDLE     FileHandle;
> +
> +  ProblemParam        = NULL;
> +  ShellStatus         = SHELL_SUCCESS;
> +
> +  Status = ShellInitialize ();
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
> +  // parse the command line
> +  //
> +  Status = ShellCommandLineParse (ParamList, &Package, &ProblemParam, TRUE);
> +  if (EFI_ERROR (Status)) {
> +    if (Status == EFI_VOLUME_CORRUPTED && ProblemParam != NULL) {
> +      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PROBLEM),
> +        mLinuxInitrdShellCommandHiiHandle, L"initrd", ProblemParam);
> +      FreePool (ProblemParam);
> +      ShellStatus = SHELL_INVALID_PARAMETER;
> +    } else {
> +      ASSERT(FALSE);
> +    }
> +  } else {
> +    if (ShellCommandLineGetCount (Package) > 2) {
> +      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> +        mLinuxInitrdShellCommandHiiHandle, L"initrd");
> +      ShellStatus = SHELL_INVALID_PARAMETER;
> +    } else if (ShellCommandLineGetCount (Package) < 2) {
> +      if (ShellCommandLineGetFlag(Package, L"-u")) {

(10) missing whitespace

> +        FreeInitrdFile ();
> +        UninstallLoadFile2Protocol ();
> +      } else {
> +        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_FEW),
> +          mLinuxInitrdShellCommandHiiHandle, L"initrd");
> +        ShellStatus = SHELL_INVALID_PARAMETER;
> +      }
> +    } else {
> +      Param = ShellCommandLineGetRawValue (Package, 1);
> +      ASSERT (Param != NULL);
> +
> +      Filename = ShellFindFilePath (Param);

(11) Filename should be freed at some point; currently, it's leaked (if
the call succeeds).

> +      if (Filename == NULL) {
> +        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FIND_FAIL),
> +          mLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
> +        ShellStatus = SHELL_NOT_FOUND;
> +      } else {
> +        FreeInitrdFile ();
> +
> +        Status = ShellOpenFileByName (Filename, &FileHandle,
> +                   EFI_FILE_MODE_READ, 0);
> +        if (!EFI_ERROR (Status)) {

(12) You could delay FreeInitrdFile() until this point.

> +          Status = CacheInitrdFile (FileHandle);
> +        }
> +        if (EFI_ERROR (Status)) {
> +          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL),
> +            mLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
> +          ShellStatus = SHELL_NOT_FOUND;
> +        }
> +        ShellCloseFile (&FileHandle);

(13) If ShellOpenFileByName() fails, it seems to set FileHandle to NULL.

Can ShellCloseFile() cope with NULL?

FWIW, its sole parameter is not marked OPTIONAL.

> +      }
> +    }
> +  }
> +  return ShellStatus;
> +}

(14) I think it would be more idiomatic for edk2 if we used a copious
amount of "goto" statements (jumping to error handling labels at the
end) rather than nesting "if"s this deeply.

I think it also might help with releasing resources on error.

> +
> +
> +/**
> +  This is the shell command handler function pointer callback type.  This
> +  function handles the command when it is invoked in the shell.
> +
> +  @param[in] This                   The instance of the
> +                                    EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL.
> +  @param[in] SystemTable            The pointer to the system table.
> +  @param[in] ShellParameters        The parameters associated with the command.
> +  @param[in] Shell                  The instance of the shell protocol used in
> +                                    the context of processing this command.
> +
> +  @return EFI_SUCCESS               the operation was successful
> +  @return other                     the operation failed.
> +**/
> +SHELL_STATUS
> +EFIAPI
> +LinuxInitrdCommandHandler (
> +  IN EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL    *This,
> +  IN EFI_SYSTEM_TABLE                      *SystemTable,
> +  IN EFI_SHELL_PARAMETERS_PROTOCOL         *ShellParameters,
> +  IN EFI_SHELL_PROTOCOL                    *Shell
> +  )
> +{
> +  gEfiShellParametersProtocol = ShellParameters;
> +  gEfiShellProtocol           = Shell;
> +
> +  return RunInitrd (gImageHandle, SystemTable);
> +}
> +
> +/**
> +  This is the command help handler function pointer callback type.  This
> +  function is responsible for displaying help information for the associated
> +  command.
> +
> +  @param[in] This                   The instance of the
> +                                    EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL.
> +  @param[in] Language               The pointer to the language string to use.
> +
> +  @return string                    Pool allocated help string, must be freed
> +                                    by caller
> +**/
> +STATIC
> +CHAR16 *
> +EFIAPI
> +LinuxInitrdGetHelp (
> +  IN EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL    *This,
> +  IN CONST CHAR8                           *Language
> +  )
> +{
> +  return HiiGetString (mLinuxInitrdShellCommandHiiHandle,
> +           STRING_TOKEN (STR_GET_HELP_INITRD), Language);
> +}
> +
> +STATIC EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL mLinuxInitrdDynamicCommand = {
> +  L"initrd",
> +  LinuxInitrdCommandHandler,
> +  LinuxInitrdGetHelp
> +};
> +
> +/**
> +  Retrieve HII package list from ImageHandle and publish to HII database.
> +
> +  @param ImageHandle            The image handle of the process.
> +
> +  @return HII handle.
> +**/
> +STATIC
> +EFI_HII_HANDLE
> +InitializeHiiPackage (
> +  EFI_HANDLE                  ImageHandle
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  EFI_HII_PACKAGE_LIST_HEADER *PackageList;
> +  EFI_HII_HANDLE              HiiHandle;
> +
> +  //
> +  // Retrieve HII package list from ImageHandle
> +  //
> +  Status = gBS->OpenProtocol (ImageHandle, &gEfiHiiPackageListProtocolGuid,
> +                  (VOID **)&PackageList, ImageHandle, NULL,
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
> +  }
> +
> +  //
> +  // Publish HII package list to HII Database.
> +  //
> +  Status = gHiiDatabase->NewPackageList (gHiiDatabase, PackageList, NULL,
> +                           &HiiHandle);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
> +  }
> +  return HiiHandle;
> +}
> +
> +/**
> +  Entry point of Linux Initrd dynamic UEFI Shell command.
> +
> +  Produce the DynamicCommand protocol to handle "initrd" command.
> +
> +  @param ImageHandle            The image handle of the process.
> +  @param SystemTable            The EFI System Table pointer.
> +
> +  @retval EFI_SUCCESS           Initrd command is executed successfully.
> +  @retval EFI_ABORTED           HII package was failed to initialize.
> +  @retval others                Other errors when executing Initrd command.
> +**/
> +EFI_STATUS
> +EFIAPI
> +LinuxInitrdDynamicShellCommandEntryPoint (
> +  IN EFI_HANDLE               ImageHandle,
> +  IN EFI_SYSTEM_TABLE         *SystemTable
> +  )
> +{
> +  EFI_STATUS                  Status;
> +
> +  mLinuxInitrdShellCommandHiiHandle = InitializeHiiPackage (ImageHandle);
> +  if (mLinuxInitrdShellCommandHiiHandle == NULL) {
> +    return EFI_ABORTED;
> +  }
> +
> +  Status = gBS->InstallProtocolInterface (&ImageHandle,
> +                  &gEfiShellDynamicCommandProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  &mLinuxInitrdDynamicCommand);
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}
> +
> +/**
> +  Unload the dynamic UEFI Shell command.
> +
> +  @param ImageHandle            The image handle of the process.
> +
> +  @retval EFI_SUCCESS           The image is unloaded.
> +  @retval Others                Failed to unload the image.
> +**/
> +EFI_STATUS
> +EFIAPI
> +LinuxInitrdDynamicShellCommandUnload (
> +  IN EFI_HANDLE               ImageHandle
> +)
> +{
> +  EFI_STATUS                  Status;
> +
> +  Status = UninstallLoadFile2Protocol ();
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gBS->UninstallProtocolInterface (ImageHandle,
> +                  &gEfiShellDynamicCommandProtocolGuid,
> +                  &mLinuxInitrdDynamicCommand);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  HiiRemovePackages (mLinuxInitrdShellCommandHiiHandle);
> +  return EFI_SUCCESS;
> +}

(15) *Assuming* we want to tear down everything here (after all,
removing loadfile2 will make the initrd unloadable), I think we should
also call FreeInitrdFile(). Otherwise, we leak it. But, more on the
assumption later.

> diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> new file mode 100644
> index 000000000000..cdfdddf77a4d
> --- /dev/null
> +++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> @@ -0,0 +1,52 @@
> +##  @file
> +# Provides 'initrd' dynamic UEFI shell command to load a Linux initrd
> +# via its GUIDed vendor media path
> +#
> +# Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.27
> +  BASE_NAME                      = LinuxInitrdDynamicShellCommand
> +  FILE_GUID                      = 2f30da26-f51b-4b6f-85c4-31873c281bca
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = LinuxInitrdDynamicShellCommandEntryPoint
> +  UNLOAD_IMAGE                   = LinuxInitrdDynamicShellCommandUnload
> +  UEFI_HII_RESOURCE_SECTION      = TRUE
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64 EBC
> +#
> +
> +[Sources.common]
> +  LinuxInitrdDynamicShellCommand.c
> +  LinuxInitrdDynamicShellCommand.uni
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  ShellPkg/ShellPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  DevicePathLib
> +  HiiLib
> +  MemoryAllocationLib
> +  ShellLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +  UefiHiiServicesLib
> +
> +[Protocols]
> +  gEfiHiiPackageListProtocolGuid                  ## CONSUMES
> +  gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
> +  gEfiShellDynamicCommandProtocolGuid             ## PRODUCES
> +
> +[DEPEX]
> +  TRUE

Seems OK.

> diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni
> new file mode 100644
> index 000000000000..a88fa6e3641b
> --- /dev/null
> +++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni
> @@ -0,0 +1,49 @@
> +// /**
> +//
> +// Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// Module Name:
> +//
> +// LinuxInitrdDynamicShellCommand.uni
> +//
> +// Abstract:
> +//
> +// String definitions for 'initrd' UEFI Shell command
> +//
> +// **/
> +
> +/=#
> +
> +#langdef   en-US "english"
> +
> +#string STR_GEN_PROBLEM           #language en-US "%H%s%N: Unknown flag - '%H%s%N'\r\n"
> +#string STR_GEN_TOO_MANY          #language en-US "%H%s%N: Too many arguments.\r\n"
> +#string STR_GEN_TOO_FEW           #language en-US "%H%s%N: Too few arguments.\r\n"
> +#string STR_GEN_FIND_FAIL         #language en-US "%H%s%N: File not found - '%H%s%N'\r\n"
> +#string STR_GEN_FILE_OPEN_FAIL    #language en-US "%H%s%N: Cannot open file - '%H%s%N'\r\n"
> +
> +#string STR_GET_HELP_INITRD       #language en-US ""
> +".TH initrd 0 "Registers or unregisters a file as Linux initrd."\r\n"
> +".SH NAME\r\n"
> +"Registers or unregisters a file as Linux initrd.\r\n"
> +".SH SYNOPSIS\r\n"
> +" \r\n"
> +"initrd <FileName>\r\n"
> +"initrd -u\r\n"
> +".SH OPTIONS\r\n"
> +" \r\n"
> +"  FileName    - Specifies a file to register as initrd.\r\n"
> +"  -u          - Unregisters any previously registered initrd files.\r\n"
> +".SH DESCRIPTION\r\n"
> +" \r\n"
> +"NOTES:\r\n"
> +"  1. Only a single file can be loaded as initrd at any given time. Using the\r\n"
> +"     command twice with a <FileName> option will result in the first file to\r\n"
> +"     be unloaded again, regardless of whether the second invocation succeeded\r\n"
> +"     or not.\r\n"
> +"  2. The initrd is not unloaded when the shell exits, and will remain active\r\n"
> +"     until it is unloaded again by a different invocation of the shell.\r\n"
> +"     Consumers of the LoadFile2 protocol on the LINUX_EFI_INITRD_MEDIA_GUID\r\n"
> +"     device path that are started via means other than the shell will be able\r\n"
> +"     to locate the protocol and invoke it.\r\n"
> 

(16) So, I don't see how paragraph#2 works here. When the shell exits,
all our global variables disappear. (LoadFile2 disappears too.) The next
time the shell is launched (in the same "UEFI session", so to say), we
won't know that we had loaded an initrd before. Is that right?

If we really want the initrd to be a system-wide resource, then we have
to use the associated device path (which is fixed, and global) as a
lookup key, for determining if an initrd already exists.

Accordingly, we should likely store our data that's currently kept in
global variables -- and so die when the shell exits -- in a container
structure that embeds both the device path protocol and the loadfile2
protocol. Then we can use the CR() macro in the loadfile2 implementation
to get back at the private data.

#define LOADED_INITRD_SIG \
        SIGNATURE_64 ('L', 'D', 'I', 'N', 'I', 'T', 'R', 'D')

typedef struct {
  //
  // LOADED_INITRD_SIGNATURE
  //
  UINT64               Signature;
  //
  // memory block base and size
  //
  EFI_PHYSICAL_ADDRESS Address;
  UINTN                Size;
  //
  // mInitrdDevicePath is still needed for populating the one below,
  // and for lookup with gBS->LocateDevicePath()
  //
  SINGLE_NODE_VENDOR_MEDIA_DEVPATH InstalledDevicePath;
  //
  // LF2 implementation
  //
  EFI_LOAD_FILE2_PROTOCOL LoadFile2;
} LOADED_INITRD;

#define LOADED_INITRD_FROM_LOADED_FILE(LoadFile2Ptr) \
        CR (LoadFile2Ptr, LOADED_INITRD, LoadFile2, LOADED_INITRD_SIG)


Thanks
Laszlo


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

* Re: [edk2-devel] [PATCH v2 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command
  2020-02-25  9:39 ` [PATCH v2 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command Ard Biesheuvel
@ 2020-02-25 23:46   ` Laszlo Ersek
  0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-25 23:46 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/25/20 10:39, Ard Biesheuvel wrote:
> Add the 'initrd' dynamic shell command to the build so we can load
> Linux initrds straight from the shell using the new generic protocol,
> which does not rely on initrd= being passed on the command line.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc           | 4 ++++
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
>  ArmVirtPkg/ArmVirtXen.fdf            | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 10037c938eb8..608dfe1aa03d 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -401,6 +401,10 @@ [Components.common]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> +  }

Please move this higher up, near TftpDynamicCommand. (Otherwise,
Shell.inf gets embedded between TftpDynamicCommand and
LinuxInitrdDynamicShellCommand, which looks strange.)

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

>  
>  [Components.AARCH64]
>    #
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index 31f615a9d0f9..bfa380815f1a 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -103,6 +103,7 @@ [FV.FvMain]
>    #
>    INF ShellPkg/Application/Shell/Shell.inf
>    INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> +  INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  
>    #
>    # Bds
> diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
> index 38d4cc163524..6a97bceeacbc 100644
> --- a/ArmVirtPkg/ArmVirtXen.fdf
> +++ b/ArmVirtPkg/ArmVirtXen.fdf
> @@ -182,6 +182,7 @@ [FV.FvMain]
>    #
>    INF ShellPkg/Application/Shell/Shell.inf
>    INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> +  INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  
>    #
>    # Bds
> 


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

* Re: [edk2-devel] [PATCH v2 4/6] OvmfPkg: add the 'initrd' dynamic shell command
  2020-02-25  9:39 ` [PATCH v2 4/6] OvmfPkg: " Ard Biesheuvel
@ 2020-02-25 23:48   ` Laszlo Ersek
  0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-25 23:48 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/25/20 10:39, Ard Biesheuvel wrote:
> Add the 'initrd' dynamic shell command to the build so we can load
> Linux initrds straight from the shell using the new generic protocol,
> which does not rely on initrd= being passed on the command line.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
>  OvmfPkg/OvmfPkgIa32.fdf    | 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
>  OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
>  OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
>  OvmfPkg/OvmfPkgX64.fdf     | 1 +
>  OvmfPkg/OvmfXen.dsc        | 4 ++++
>  OvmfPkg/OvmfXen.fdf        | 1 +
>  8 files changed, 20 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 19728f20b34e..870eb7aa7429 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -843,6 +843,10 @@ [Components]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> +  }
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 63607551ed75..b6cd5da4f2b3 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -282,6 +282,7 @@ [FV.DXEFV]
>  INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>  !endif
>  INF  ShellPkg/Application/Shell/Shell.inf
> +INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  
>  INF MdeModulePkg/Logo/LogoDxe.inf
>  
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3c0c229e3a72..8022365e664c 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -856,6 +856,10 @@ [Components.X64]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> +  }
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 0488e5d95ffe..159840836775 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -283,6 +283,7 @@ [FV.DXEFV]
>  INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>  !endif
>  INF  ShellPkg/Application/Shell/Shell.inf
> +INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  
>  INF MdeModulePkg/Logo/LogoDxe.inf
>  
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index f6c1d8d228c6..90c0bd16e6a0 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -854,6 +854,10 @@ [Components]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> +  }
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 0488e5d95ffe..159840836775 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -283,6 +283,7 @@ [FV.DXEFV]
>  INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>  !endif
>  INF  ShellPkg/Application/Shell/Shell.inf
> +INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  
>  INF MdeModulePkg/Logo/LogoDxe.inf
>  
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 5751ff1f0352..6612018ad6a8 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -714,6 +714,10 @@ [Components]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> +  }
>  
>    OvmfPkg/PlatformDxe/Platform.inf
>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
> index 05138ffc5b4f..cfdb01a80a00 100644
> --- a/OvmfPkg/OvmfXen.fdf
> +++ b/OvmfPkg/OvmfXen.fdf
> @@ -365,6 +365,7 @@ [FV.DXEFV]
>  INF  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>  !endif
>  INF  ShellPkg/Application/Shell/Shell.inf
> +INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  
>  INF MdeModulePkg/Logo/LogoDxe.inf
>  
> 

Please add the new command right after the TFTP dynamic command, in both
the FDF and the DSC files.

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!


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

* Re: [edk2-devel] [PATCH v2 6/6] OvmfPkg IA32: add support for loading X64 images
  2020-02-25  9:39 ` [PATCH v2 6/6] OvmfPkg IA32: add support for loading X64 images Ard Biesheuvel
@ 2020-02-25 23:55   ` Laszlo Ersek
  0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-25 23:55 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/25/20 10:39, Ard Biesheuvel wrote:
> This is the UEFI counterpart to my Linux series which generalizes
> mixed mode support into a feature that requires very little internal
> knowledge about the architecture specifics of booting Linux on the
> part of the bootloader or firmware.
> 
> Instead, we add a .compat PE/COFF header containing an array of
> PE_COMPAT nodes containing <machine type, entrypoint> tuples that
> describe alternate entrypoints into the image for different native
> machine types, e.g., IA-32 in a 64-bit image so it can be booted
> from IA-32 firmware.
> 
> This patch implements the PE/COFF emulator protocol to take this new
> section into account, so that such images can simply be loaded via
> LoadImage/StartImage, e.g., straight from the shell.
> 
> This feature is based on the EDK2 specific PE/COFF emulator protocol
> that was introduced in commit 57df17fe26cd ("MdeModulePkg/DxeCore:
> invoke the emulator protocol for foreign images", 2019-04-14).
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c   | 140 ++++++++++++++++++++
>  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf |  36 +++++
>  OvmfPkg/OvmfPkgIa32.dsc                               |   5 +
>  OvmfPkg/OvmfPkgIa32.fdf                               |   4 +
>  4 files changed, 185 insertions(+)
> 
> diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> new file mode 100644
> index 000000000000..d2ae03eabf7f
> --- /dev/null
> +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> @@ -0,0 +1,140 @@
> +/** @file
> + *  PE/COFF emulator protocol implementation to start Linux kernel
> + *  images from non-native firmware
> + *
> + *  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
> + *
> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> + *
> + */
> +
> +#include <PiDxe.h>
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/PeCoffLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>

(1) Do we actually need anything from
"MdePkg/Include/Library/UefiDriverEntryPoint.h"?

(We need it in [LibraryClasses], in the INF file, yes, but likely not as
an #include in the C source.)

> +
> +#include <Protocol/PeCoffImageEmulator.h>
> +
> +#pragma pack(1)

(2) whitespace please

> +typedef struct {
> +  UINT8   Type;
> +  UINT8   Size;
> +  UINT16  MachineType;
> +  UINT32  EntryPoint;
> +} PE_COMPAT_TYPE1;
> +#pragma pack()

(3) ditto.

With these addressed:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

> +
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsImageSupported (
> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> +  IN  UINT16                                  ImageType,
> +  IN  EFI_DEVICE_PATH_PROTOCOL                *DevicePath   OPTIONAL
> +  )
> +{
> +  return ImageType == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION;
> +}
> +
> +STATIC
> +EFI_IMAGE_ENTRY_POINT
> +EFIAPI
> +GetCompatEntryPoint (
> +  IN  EFI_PHYSICAL_ADDRESS              ImageBase
> +  )
> +{
> +  EFI_IMAGE_DOS_HEADER                  *DosHdr;
> +  UINTN                                 PeCoffHeaderOffset;
> +  EFI_IMAGE_NT_HEADERS32                *Pe32;
> +  EFI_IMAGE_SECTION_HEADER              *Section;
> +  UINTN                                 NumberOfSections;
> +  PE_COMPAT_TYPE1                       *PeCompat;
> +
> +  DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageBase;
> +  if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) {
> +    return NULL;
> +  }
> +
> +  PeCoffHeaderOffset = DosHdr->e_lfanew;
> +  Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageBase + PeCoffHeaderOffset);
> +
> +  Section = (EFI_IMAGE_SECTION_HEADER *)((UINTN)&Pe32->OptionalHeader +
> +                                         Pe32->FileHeader.SizeOfOptionalHeader);
> +  NumberOfSections = (UINTN)Pe32->FileHeader.NumberOfSections;
> +
> +  while (NumberOfSections--) {
> +    if (!CompareMem (Section->Name, ".compat", sizeof (Section->Name))) {
> +      //
> +      // Dereference the section contents to find the mixed mode entry point
> +      //
> +      PeCompat = (PE_COMPAT_TYPE1 *)((UINTN)ImageBase + Section->VirtualAddress);
> +
> +      while (PeCompat->Type != 0) {
> +        if (PeCompat->Type == 1 &&
> +            PeCompat->Size >= sizeof (PE_COMPAT_TYPE1) &&
> +            EFI_IMAGE_MACHINE_TYPE_SUPPORTED (PeCompat->MachineType)) {
> +
> +          return (EFI_IMAGE_ENTRY_POINT)((UINTN)ImageBase + PeCompat->EntryPoint);
> +        }
> +        PeCompat = (PE_COMPAT_TYPE1 *)((UINTN)PeCompat + PeCompat->Size);
> +      }
> +    }
> +    Section++;
> +  }
> +  return NULL;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +RegisterImage (
> +  IN      EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> +  IN      EFI_PHYSICAL_ADDRESS                    ImageBase,
> +  IN      UINT64                                  ImageSize,
> +  IN  OUT EFI_IMAGE_ENTRY_POINT                   *EntryPoint
> +  )
> +{
> +  EFI_IMAGE_ENTRY_POINT                           CompatEntryPoint;
> +
> +  CompatEntryPoint = GetCompatEntryPoint (ImageBase);
> +  if (CompatEntryPoint == NULL) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  *EntryPoint = CompatEntryPoint;
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +UnregisterImage (
> +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> +  IN  EFI_PHYSICAL_ADDRESS                    ImageBase
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL mCompatLoaderPeCoffEmuProtocol = {
> +  IsImageSupported,
> +  RegisterImage,
> +  UnregisterImage,
> +  EDKII_PECOFF_IMAGE_EMULATOR_VERSION,
> +  EFI_IMAGE_MACHINE_X64
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +CompatImageLoaderDxeEntryPoint (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  return gBS->InstallProtocolInterface (&ImageHandle,
> +                &gEdkiiPeCoffImageEmulatorProtocolGuid,
> +                EFI_NATIVE_INTERFACE,
> +                &mCompatLoaderPeCoffEmuProtocol);
> +}
> diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> new file mode 100644
> index 000000000000..82369384fbe6
> --- /dev/null
> +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> @@ -0,0 +1,36 @@
> +## @file
> +#  PE/COFF emulator protocol implementation to start Linux kernel
> +#  images from non-native firmware
> +#
> +#  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.27
> +  BASE_NAME                      = CompatImageLoaderDxe
> +  FILE_GUID                      = 1019f54a-2560-41b2-87b0-6750b98f3eff
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = CompatImageLoaderDxeEntryPoint
> +
> +[Sources]
> +  CompatImageLoaderDxe.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  PeCoffLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gEdkiiPeCoffImageEmulatorProtocolGuid   ## PRODUCES
> +
> +[Depex]
> +  TRUE
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 870eb7aa7429..a996c0a7a7c9 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -33,6 +33,7 @@ [Defines]
>    DEFINE SOURCE_DEBUG_ENABLE     = FALSE
>    DEFINE TPM2_ENABLE             = FALSE
>    DEFINE TPM2_CONFIG_ENABLE      = FALSE
> +  DEFINE LOAD_X64_ON_IA32_ENABLE = FALSE
>  
>    #
>    # Network definition
> @@ -932,3 +933,7 @@ [Components]
>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>  !endif
>  !endif
> +
> +!if $(LOAD_X64_ON_IA32_ENABLE) == TRUE
> +  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> +!endif
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index b6cd5da4f2b3..ff8d80859fb9 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -354,6 +354,10 @@ [FV.DXEFV]
>  !endif
>  !endif
>  
> +!if $(LOAD_X64_ON_IA32_ENABLE) == TRUE
> +INF  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> +!endif
> +
>  ################################################################################
>  
>  [FV.FVMAIN_COMPACT]
> 


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

* Re: [edk2-devel] [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
  2020-02-25 23:43   ` [edk2-devel] " Laszlo Ersek
@ 2020-02-26  0:00     ` Laszlo Ersek
  2020-02-26  1:25     ` Laszlo Ersek
  1 sibling, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-26  0:00 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/26/20 00:43, Laszlo Ersek wrote:
> On 02/25/20 10:39, Ard Biesheuvel wrote:

>> +        Status = ShellOpenFileByName (Filename, &FileHandle,
>> +                   EFI_FILE_MODE_READ, 0);
>> +        if (!EFI_ERROR (Status)) {
>> +          Status = CacheInitrdFile (FileHandle);
>> +        }
>> +        if (EFI_ERROR (Status)) {
>> +          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL),
>> +            mLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
>> +          ShellStatus = SHELL_NOT_FOUND;
>> +        }
>> +        ShellCloseFile (&FileHandle);
> 
> (13) If ShellOpenFileByName() fails, it seems to set FileHandle to NULL.
> 
> Can ShellCloseFile() cope with NULL?
> 
> FWIW, its sole parameter is not marked OPTIONAL.

I missed the extra indirection here for a minute, sorry about that; the
question still remains whether ShellCloseFile() can cope with
(*FileHandle) being NULL.

Thanks
Laszlo


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

* Re: [edk2-devel] [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
  2020-02-25 23:43   ` [edk2-devel] " Laszlo Ersek
  2020-02-26  0:00     ` Laszlo Ersek
@ 2020-02-26  1:25     ` Laszlo Ersek
  1 sibling, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2020-02-26  1:25 UTC (permalink / raw)
  To: devel, ard.biesheuvel
  Cc: leif, pjones, mjg59, agraf, daniel.kiper, michael.d.kinney,
	jian.j.wang, hao.a.wu, ray.ni, zhichao.gao

On 02/26/20 00:43, Laszlo Ersek wrote:
> On 02/25/20 10:39, Ard Biesheuvel wrote:

>> +"  2. The initrd is not unloaded when the shell exits, and will remain active\r\n"
>> +"     until it is unloaded again by a different invocation of the shell.\r\n"
>> +"     Consumers of the LoadFile2 protocol on the LINUX_EFI_INITRD_MEDIA_GUID\r\n"
>> +"     device path that are started via means other than the shell will be able\r\n"
>> +"     to locate the protocol and invoke it.\r\n"
>>
> 
> (16) So, I don't see how paragraph#2 works here. When the shell exits,
> all our global variables disappear. (LoadFile2 disappears too.) The next
> time the shell is launched (in the same "UEFI session", so to say), we
> won't know that we had loaded an initrd before. Is that right?

Sorry, I guess I missed the core idea behind dynamic shell commands :(

The dynamic shell command seems like a separate DXE driver, after all.
The shell can call into it (via EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL), but
its lifecycle is independent of the shell's. The shell can also unload
it (like the shell can unload any other driver).

So please ignore my point (16).

(But then, I do think we leak the initrd, if one has been loaded, in
LinuxInitrdDynamicShellCommandUnload()). I.e., pls see my point (15).)

Thanks
Laszlo


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

end of thread, other threads:[~2020-02-26  1:25 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-25  9:39 [PATCH v2 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
2020-02-25  9:39 ` [PATCH v2 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID Ard Biesheuvel
2020-02-25 22:14   ` [edk2-devel] " Laszlo Ersek
2020-02-25  9:39 ` [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path Ard Biesheuvel
2020-02-25 23:43   ` [edk2-devel] " Laszlo Ersek
2020-02-26  0:00     ` Laszlo Ersek
2020-02-26  1:25     ` Laszlo Ersek
2020-02-25  9:39 ` [PATCH v2 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command Ard Biesheuvel
2020-02-25 23:46   ` [edk2-devel] " Laszlo Ersek
2020-02-25  9:39 ` [PATCH v2 4/6] OvmfPkg: " Ard Biesheuvel
2020-02-25 23:48   ` [edk2-devel] " Laszlo Ersek
2020-02-25  9:39 ` [PATCH v2 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage Ard Biesheuvel
2020-02-25  9:39 ` [PATCH v2 6/6] OvmfPkg IA32: add support for loading X64 images Ard Biesheuvel
2020-02-25 23:55   ` [edk2-devel] " Laszlo Ersek

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