[RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Ard Biesheuvel]

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@arm.com>
---
Note that the vendor media device path initrd loading method is still
under review on the Linux side, so this is for discussion only for now.

 ArmVirtPkg/ArmVirt.dsc.inc                                                |   1 +
 OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c   | 269 ++++++++++++++++++++
 OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf |  49 ++++
 OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni |  37 +++
 OvmfPkg/OvmfPkg.dec                                                       |   1 +
 OvmfPkg/OvmfPkgIa32.dsc                                                   |   1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                                |   1 +
 OvmfPkg/OvmfPkgX64.dsc                                                    |   1 +
 8 files changed, 360 insertions(+)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 10037c938eb8..0570eba6ed0c 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -382,6 +382,7 @@ [Components.common]
   ShellPkg/Application/Shell/Shell.inf {
     <LibraryClasses>
       ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf
+      NULL|OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
       NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
diff --git a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
new file mode 100644
index 000000000000..cfa4526df6d8
--- /dev/null
+++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
@@ -0,0 +1,269 @@
+/** @file
+  Provides initrd 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/ShellCommandLib.h>
+#include <Library/ShellLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/DevicePath.h>
+#include <Protocol/LoadFile2.h>
+
+#pragma pack(1)
+typedef struct {
+  VENDOR_DEVICE_PATH          VenMediaNode;
+  EFI_DEVICE_PATH_PROTOCOL    EndNode;
+} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
+#pragma pack()
+
+STATIC CONST CHAR16         mFileName[] = L"<unspecified>";
+STATIC EFI_HII_HANDLE       gLinuxInitrdShellCommandHiiHandle;
+STATIC SHELL_FILE_HANDLE    mInitrdFileHandle;
+STATIC EFI_HANDLE           mInitrdLoadFile2Handle;
+
+STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
+  {L"-u", TypeFlag},
+  {NULL, TypeMax}
+  };
+
+/**
+  Get the filename to get help text from if not using HII.
+
+  @retval The filename.
+**/
+STATIC
+CONST CHAR16*
+EFIAPI
+ShellCommandGetManFileNameInitrd (
+  VOID
+  )
+{
+  return mFileName;
+}
+
+STATIC CONST SINGLE_NODE_VENDOR_MEDIA_DEVPATH mInitrdDevicePath = {
+  {
+    {
+      MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
+    },
+    {
+      // LINUX_EFI_INITRD_MEDIA_GUID
+      0x5568e427, 0x68fc, 0x4f3d,
+      { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 }
+    }
+  },
+
+  {
+    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
+  )
+{
+  UINTN                     InitrdSize;
+  EFI_STATUS                Status;
+
+  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 ||
+      mInitrdFileHandle == NULL) {
+    return EFI_NOT_FOUND;
+  }
+
+  Status = gEfiShellProtocol->GetFileSize (mInitrdFileHandle, &InitrdSize);
+  ASSERT_EFI_ERROR(Status);
+
+  if (Buffer == NULL || *BufferSize < InitrdSize) {
+    *BufferSize = InitrdSize;
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  return gEfiShellProtocol->ReadFile (mInitrdFileHandle, BufferSize, Buffer);
+}
+
+STATIC CONST EFI_LOAD_FILE2_PROTOCOL     mInitrdLoadFile2 = {
+  InitrdLoadFile2,
+};
+
+/**
+  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).
+**/
+SHELL_STATUS
+EFIAPI
+ShellCommandRunInitrd (
+  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;
+
+  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),
+        gLinuxInitrdShellCommandHiiHandle, 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),
+        gLinuxInitrdShellCommandHiiHandle, L"initrd");
+      ShellStatus = SHELL_INVALID_PARAMETER;
+    } else if (ShellCommandLineGetCount (Package) < 2) {
+      if (ShellCommandLineGetFlag(Package, L"-u")) {
+        if (mInitrdFileHandle != NULL) {
+          ShellCloseFile (&mInitrdFileHandle);
+          mInitrdFileHandle = NULL;
+        }
+        if (mInitrdLoadFile2Handle) {
+            gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle,
+                   &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
+                   &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
+                   NULL);
+            mInitrdLoadFile2Handle = NULL;
+        }
+      } else {
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_FEW),
+          gLinuxInitrdShellCommandHiiHandle, 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),
+          gLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
+        ShellStatus = SHELL_NOT_FOUND;
+      } else {
+        if (mInitrdFileHandle != NULL) {
+          ShellCloseFile (&mInitrdFileHandle);
+          mInitrdFileHandle = NULL;
+        }
+        Status = ShellOpenFileByName (Filename, &mInitrdFileHandle,
+                   EFI_FILE_MODE_READ, 0);
+        if (EFI_ERROR (Status)) {
+          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL),
+            gLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
+          ShellStatus = SHELL_NOT_FOUND;
+        }
+        if (mInitrdLoadFile2Handle == NULL) {
+          Status = gBS->InstallMultipleProtocolInterfaces (
+                          &mInitrdLoadFile2Handle,
+                          &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
+                          &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
+                          NULL);
+          ASSERT_EFI_ERROR (Status);
+        }
+      }
+    }
+  }
+
+  return ShellStatus;
+}
+
+/**
+  Constructor for the 'initrd' UEFI Shell command library
+
+  @param ImageHandle    the image handle of the process
+  @param SystemTable    the EFI System Table pointer
+
+  @retval EFI_SUCCESS        the shell command handlers were installed sucessfully
+  @retval EFI_UNSUPPORTED    the shell level required was not found.
+**/
+EFI_STATUS
+EFIAPI
+LinuxInitrdShellCommandLibConstructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  gLinuxInitrdShellCommandHiiHandle = HiiAddPackages (&gShellInitrdHiiGuid,
+    gImageHandle, LinuxInitrdShellCommandLibStrings, NULL);
+  if (gLinuxInitrdShellCommandHiiHandle == NULL) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  ShellCommandRegisterCommandName (L"initrd", ShellCommandRunInitrd,
+    ShellCommandGetManFileNameInitrd, 0, L"initrd", TRUE,
+    gLinuxInitrdShellCommandHiiHandle, STRING_TOKEN(STR_GET_HELP_INITRD));
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Destructor for the library.  free any resources.
+
+  @param ImageHandle    The image handle of the process.
+  @param SystemTable    The EFI System Table pointer.
+
+  @retval EFI_SUCCESS   Always returned.
+**/
+EFI_STATUS
+EFIAPI
+LinuxInitrdShellCommandLibDestructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  if (gLinuxInitrdShellCommandHiiHandle != NULL) {
+    HiiRemovePackages (gLinuxInitrdShellCommandHiiHandle);
+  }
+
+  if (mInitrdLoadFile2Handle) {
+      gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle,
+             &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
+             &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
+             NULL);
+  }
+  return EFI_SUCCESS;
+}
diff --git a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
new file mode 100644
index 000000000000..ed8a0ca85e85
--- /dev/null
+++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
@@ -0,0 +1,49 @@
+##  @file
+# Provides initrd 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                      = LinuxInitrdShellCommandLib
+  FILE_GUID                      = 2f30da26-f51b-4b6f-85c4-31873c281bca
+  MODULE_TYPE                    = UEFI_APPLICATION
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = NULL|UEFI_APPLICATION UEFI_DRIVER
+  CONSTRUCTOR                    = LinuxInitrdShellCommandLibConstructor
+  DESTRUCTOR                     = LinuxInitrdShellCommandLibDestructor
+
+#
+#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
+#
+
+[Sources.common]
+  LinuxInitrdShellCommandLib.c
+  LinuxInitrdShellCommandLib.uni
+
+[Packages]
+  MdePkg/MdePkg.dec
+  ShellPkg/ShellPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  DebugLib
+  DevicePathLib
+  HiiLib
+  MemoryAllocationLib
+  ShellCommandLib
+  ShellLib
+  UefiBootServicesTableLib
+
+[Protocols]
+  gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
+
+[Guids]
+  gShellInitrdHiiGuid                             ## SOMETIMES_CONSUMES ## HII
diff --git a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
new file mode 100644
index 000000000000..d4fe798b1ea2
--- /dev/null
+++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
@@ -0,0 +1,37 @@
+// /**
+//
+// Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// Module Name:
+//
+// LinuxInitrdShellCommandLib.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 vol 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"
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index d5fee805ef4a..437b1d248895 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}}
+  gShellInitrdHiiGuid                 = {0xeda423e2, 0xf434, 0x4cdd, {0x8d, 0x5d, 0xb5, 0xb0, 0xc7, 0x2c, 0x56, 0xd9}}
 
 [Protocols]
   gVirtioDeviceProtocolGuid           = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 9a60eb8fe2b0..3ffaa9b3388f 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -817,6 +817,7 @@ [Components]
   ShellPkg/Application/Shell/Shell.inf {
     <LibraryClasses>
       ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf
+      NULL|OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
       NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 1d1480b50b02..691ba204a6ac 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -830,6 +830,7 @@ [Components.X64]
   ShellPkg/Application/Shell/Shell.inf {
     <LibraryClasses>
       ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf
+      NULL|OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
       NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c287a436f8ec..8df5390de132 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -828,6 +828,7 @@ [Components]
   ShellPkg/Application/Shell/Shell.inf {
     <LibraryClasses>
       ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf
+      NULL|OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
       NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
-- 
2.17.1

Re: [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Laszlo Ersek]

Adding Ray and Zhichao; comments below

On 02/11/20 19:03, 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@arm.com>
> ---
> Note that the vendor media device path initrd loading method is still
> under review on the Linux side, so this is for discussion only for now.
>
>  ArmVirtPkg/ArmVirt.dsc.inc                                                |   1 +
>  OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c   | 269 ++++++++++++++++++++
>  OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf |  49 ++++
>  OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni |  37 +++
>  OvmfPkg/OvmfPkg.dec                                                       |   1 +
>  OvmfPkg/OvmfPkgIa32.dsc                                                   |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                                                |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                                                    |   1 +
>  8 files changed, 360 insertions(+)

(1) As usual (and I'm sure you're aware -- this is just an RFC), please
split the ArmVirtPkg change to a different patch.

(2) "ShellPkg/Application/Shell/Shell.inf" is part of
"OvmfPkg/OvmfXen.dsc" too, so I'd suggest enabling the new shell command
there as well.

> diff --git a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
> new file mode 100644
> index 000000000000..ed8a0ca85e85
> --- /dev/null
> +++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
> @@ -0,0 +1,49 @@
> +##  @file
> +# Provides initrd 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                      = LinuxInitrdShellCommandLib
> +  FILE_GUID                      = 2f30da26-f51b-4b6f-85c4-31873c281bca
> +  MODULE_TYPE                    = UEFI_APPLICATION
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = NULL|UEFI_APPLICATION UEFI_DRIVER
> +  CONSTRUCTOR                    = LinuxInitrdShellCommandLibConstructor
> +  DESTRUCTOR                     = LinuxInitrdShellCommandLibDestructor
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources.common]
> +  LinuxInitrdShellCommandLib.c
> +  LinuxInitrdShellCommandLib.uni
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  ShellPkg/ShellPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  DevicePathLib
> +  HiiLib
> +  MemoryAllocationLib
> +  ShellCommandLib
> +  ShellLib
> +  UefiBootServicesTableLib
> +
> +[Protocols]
> +  gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
> +
> +[Guids]
> +  gShellInitrdHiiGuid                             ## SOMETIMES_CONSUMES ## HII

Seems reasonable, and to match e.g.
"ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf".

(3) However: I think this should be added as a Dynamic Command instead.
I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
Convert from NULL class library to Dynamic Command", 2017-11-28), which
is the first commit in edk2 ever to introduce a Dynamic Command.

And the commit message there says:

    The guideline is:
    1. Only use NULL class library for Shell spec defined commands.
    2. New commands can be provided as not only a standalone application
       but also a dynamic command. So it can be used either as an
       internal command, but also as a standalone application.

I'm not asking for the command to be usable as a separate application,
but I think we might want to follow the first guideline.

(I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
commands, it does not seem to spell out guideline#1. So I think it's
rather an edk2-specific guideline than a standard one. Nonetheless we
might want to adhere to it.)

Implementing the feature as a dynamic command would imply
MODULE_TYPE=DXE_DRIVER, and using ENTRY_POINT/UNLOAD_IMAGE rathern than
CONSTRUCTOR/DESTRUCTOR; I think.

> diff --git a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
> new file mode 100644
> index 000000000000..d4fe798b1ea2
> --- /dev/null
> +++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
> @@ -0,0 +1,37 @@
> +// /**
> +//
> +// Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// Module Name:
> +//
> +// LinuxInitrdShellCommandLib.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"

(4) If these are copied from another UNI file, please add a comment
here, or in the commit message. It's not really convenient to review
format strings in isolation :)

> +
> +#string STR_GET_HELP_INITRD       #language en-US ""
> +".TH vol 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"

Seems OK. (I won't check the formatting directives; I trust they look
good in the shell.)

> diff --git a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
> new file mode 100644
> index 000000000000..cfa4526df6d8
> --- /dev/null
> +++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
> @@ -0,0 +1,269 @@
> +/** @file
> +  Provides initrd 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/ShellCommandLib.h>
> +#include <Library/ShellLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/DevicePath.h>
> +#include <Protocol/LoadFile2.h>
> +
> +#pragma pack(1)
> +typedef struct {
> +  VENDOR_DEVICE_PATH          VenMediaNode;
> +  EFI_DEVICE_PATH_PROTOCOL    EndNode;
> +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
> +#pragma pack()
> +
> +STATIC CONST CHAR16         mFileName[] = L"<unspecified>";
> +STATIC EFI_HII_HANDLE       gLinuxInitrdShellCommandHiiHandle;
> +STATIC SHELL_FILE_HANDLE    mInitrdFileHandle;
> +STATIC EFI_HANDLE           mInitrdLoadFile2Handle;
> +
> +STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> +  {L"-u", TypeFlag},
> +  {NULL, TypeMax}
> +  };
> +
> +/**
> +  Get the filename to get help text from if not using HII.
> +
> +  @retval The filename.
> +**/
> +STATIC
> +CONST CHAR16*
> +EFIAPI
> +ShellCommandGetManFileNameInitrd (
> +  VOID
> +  )
> +{
> +  return mFileName;
> +}
> +
> +STATIC CONST SINGLE_NODE_VENDOR_MEDIA_DEVPATH mInitrdDevicePath = {
> +  {
> +    {
> +      MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
> +    },
> +    {
> +      // LINUX_EFI_INITRD_MEDIA_GUID
> +      0x5568e427, 0x68fc, 0x4f3d,
> +      { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 }

(5) It's probably better to introduce this in the OvmfPkg DEC file as a
GUID too, plus add a header file under OvmfPkg/Include/Guid.

(The latter primarily for the initializer macro.)

If you are fine using CopyGuid() programmatically (and dropping CONST
here), then I think the header file is not needed.

> +    }
> +  },
> +
> +  {
> +    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
> +  )
> +{
> +  UINTN                     InitrdSize;
> +  EFI_STATUS                Status;
> +
> +  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 ||
> +      mInitrdFileHandle == NULL) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  Status = gEfiShellProtocol->GetFileSize (mInitrdFileHandle, &InitrdSize);
> +  ASSERT_EFI_ERROR(Status);

(6) Probably not much of a burden to just propagate the error.

Either way, please insert a space character before the opening paren.

> +
> +  if (Buffer == NULL || *BufferSize < InitrdSize) {
> +    *BufferSize = InitrdSize;
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  return gEfiShellProtocol->ReadFile (mInitrdFileHandle, BufferSize, Buffer);

OK. Looks like EFI_SHELL_PROTOCOL.ReadFile() has the same EOF and
"buffer too small" semantics as EFI_LOAD_FILE2_PROTOCOL.LoadFile().

> +}
> +
> +STATIC CONST EFI_LOAD_FILE2_PROTOCOL     mInitrdLoadFile2 = {
> +  InitrdLoadFile2,
> +};
> +
> +/**
> +  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).
> +**/
> +SHELL_STATUS
> +EFIAPI
> +ShellCommandRunInitrd (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )

I think I'll mostly skip this function (and the ones below) for now; I
assume they could change once you rewrite the patch as a Dynamic
Command.

Just two logic-related comments below:

> +{
> +  EFI_STATUS            Status;
> +  LIST_ENTRY            *Package;
> +  CHAR16                *ProblemParam;
> +  CONST CHAR16          *Param;
> +  CONST CHAR16          *Filename;
> +  SHELL_STATUS          ShellStatus;
> +
> +  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),
> +        gLinuxInitrdShellCommandHiiHandle, 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),
> +        gLinuxInitrdShellCommandHiiHandle, L"initrd");
> +      ShellStatus = SHELL_INVALID_PARAMETER;
> +    } else if (ShellCommandLineGetCount (Package) < 2) {
> +      if (ShellCommandLineGetFlag(Package, L"-u")) {
> +        if (mInitrdFileHandle != NULL) {
> +          ShellCloseFile (&mInitrdFileHandle);
> +          mInitrdFileHandle = NULL;
> +        }
> +        if (mInitrdLoadFile2Handle) {
> +            gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle,
> +                   &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
> +                   &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
> +                   NULL);
> +            mInitrdLoadFile2Handle = NULL;
> +        }
> +      } else {
> +        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_FEW),
> +          gLinuxInitrdShellCommandHiiHandle, 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),
> +          gLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
> +        ShellStatus = SHELL_NOT_FOUND;
> +      } else {
> +        if (mInitrdFileHandle != NULL) {
> +          ShellCloseFile (&mInitrdFileHandle);
> +          mInitrdFileHandle = NULL;
> +        }
> +        Status = ShellOpenFileByName (Filename, &mInitrdFileHandle,
> +                   EFI_FILE_MODE_READ, 0);

(7) The help text given in STR_GET_HELP_INITRD is a bit unclear. I
suggest clarifying in STR_GET_HELP_INITRD that:

- at any time, at most one shell file may be registered as initrd,
- registering a 2nd file causes the 1st to be auto-deregistered,
- de-registration applies to the last (i.e., only) registered file, if any.

(8) Did you test this patch with:
- registering a file as initrd,
- deleting the file with the "rm" command (is that permitted or rejected?),
- booting linux?

I wonder if this test could trigger the ASSERT_EFI_ERROR() -- from
GetFileSize() -- in InitrdLoadFile2().

Basically I'm unsure if we are allowed to hang on to an
SHELL_FILE_HANDLE while the user may enter another command.

This question could be especially relevant when this feature is
implemented as a dynamic command -- the dynamic command woul dbe
provided by a DXE_DRIVER, and so the cached shell file handle wouldn't
even be owned by the shell. In that case, we might have to load the full
initrd image contents into a memory block at once, when the registration
happens -- and then LoadFile2 would return the file from memory.

We might want to consider removable media too (e.g. an initrd file
registered from a (virtual) CD-ROM, where an EFI_MEDIA_CHANGED retval
could be plausible upon later access).

I mean if we don't crash and the sudden absence of the file is correctly
propagated through LoadFile2 to the kernel's EFI stub, that's 100% fine
too.

Unfortunately I'm not familiar with the expected life cycle of shell
file handles in dynamic shell commands. Hopefully Ray and Zhichao can
advise us.

Thanks!
Laszlo

> +        if (EFI_ERROR (Status)) {
> +          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL),
> +            gLinuxInitrdShellCommandHiiHandle, L"initrd", Param);
> +          ShellStatus = SHELL_NOT_FOUND;
> +        }
> +        if (mInitrdLoadFile2Handle == NULL) {
> +          Status = gBS->InstallMultipleProtocolInterfaces (
> +                          &mInitrdLoadFile2Handle,
> +                          &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
> +                          &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
> +                          NULL);
> +          ASSERT_EFI_ERROR (Status);
> +        }
> +      }
> +    }
> +  }
> +
> +  return ShellStatus;
> +}
> +
> +/**
> +  Constructor for the 'initrd' UEFI Shell command library
> +
> +  @param ImageHandle    the image handle of the process
> +  @param SystemTable    the EFI System Table pointer
> +
> +  @retval EFI_SUCCESS        the shell command handlers were installed sucessfully
> +  @retval EFI_UNSUPPORTED    the shell level required was not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +LinuxInitrdShellCommandLibConstructor (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  gLinuxInitrdShellCommandHiiHandle = HiiAddPackages (&gShellInitrdHiiGuid,
> +    gImageHandle, LinuxInitrdShellCommandLibStrings, NULL);
> +  if (gLinuxInitrdShellCommandHiiHandle == NULL) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  ShellCommandRegisterCommandName (L"initrd", ShellCommandRunInitrd,
> +    ShellCommandGetManFileNameInitrd, 0, L"initrd", TRUE,
> +    gLinuxInitrdShellCommandHiiHandle, STRING_TOKEN(STR_GET_HELP_INITRD));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Destructor for the library.  free any resources.
> +
> +  @param ImageHandle    The image handle of the process.
> +  @param SystemTable    The EFI System Table pointer.
> +
> +  @retval EFI_SUCCESS   Always returned.
> +**/
> +EFI_STATUS
> +EFIAPI
> +LinuxInitrdShellCommandLibDestructor (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  if (gLinuxInitrdShellCommandHiiHandle != NULL) {
> +    HiiRemovePackages (gLinuxInitrdShellCommandHiiHandle);
> +  }
> +
> +  if (mInitrdLoadFile2Handle) {
> +      gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle,
> +             &gEfiDevicePathProtocolGuid,    &mInitrdDevicePath,
> +             &gEfiLoadFile2ProtocolGuid,     &mInitrdLoadFile2,
> +             NULL);
> +  }
> +  return EFI_SUCCESS;
> +}

Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Ni, Ray]

> (3) However: I think this should be added as a Dynamic Command instead.
> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
> Convert from NULL class library to Dynamic Command", 2017-11-28), which
> is the first commit in edk2 ever to introduce a Dynamic Command.
> 
> And the commit message there says:
> 
>     The guideline is:
>     1. Only use NULL class library for Shell spec defined commands.
>     2. New commands can be provided as not only a standalone application
>        but also a dynamic command. So it can be used either as an
>        internal command, but also as a standalone application.
> 
> I'm not asking for the command to be usable as a separate application,
> but I think we might want to follow the first guideline.
> 
> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
> commands, it does not seem to spell out guideline#1. So I think it's
> rather an edk2-specific guideline than a standard one. Nonetheless we
> might want to adhere to it.)

Laszlo, thanks for the comments😊.
I didn't remember that I said these guideline publicly.
The reason behind that is we can have the same shell binary everywhere
and new non-spec commands can be added through dynamic command without
impacting the shell binary.

Thanks,
Ray

Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Laszlo Ersek]

On 02/12/20 15:21, Ni, Ray wrote:
>> (3) However: I think this should be added as a Dynamic Command instead.
>> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
>> Convert from NULL class library to Dynamic Command", 2017-11-28), which
>> is the first commit in edk2 ever to introduce a Dynamic Command.
>>
>> And the commit message there says:
>>
>>     The guideline is:
>>     1. Only use NULL class library for Shell spec defined commands.
>>     2. New commands can be provided as not only a standalone application
>>        but also a dynamic command. So it can be used either as an
>>        internal command, but also as a standalone application.
>>
>> I'm not asking for the command to be usable as a separate application,
>> but I think we might want to follow the first guideline.
>>
>> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
>> commands, it does not seem to spell out guideline#1. So I think it's
>> rather an edk2-specific guideline than a standard one. Nonetheless we
>> might want to adhere to it.)
> 
> Laszlo, thanks for the comments😊.
> I didn't remember that I said these guideline publicly.
> The reason behind that is we can have the same shell binary everywhere
> and new non-spec commands can be added through dynamic command without
> impacting the shell binary.

Thanks for the explanation -- this means that the NULL class lib
approach is good for OvmfPkg after all. I'm putting the remaining parts
of this patch back on my review queue (it will take a while).

Thanks
Laszlo

Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Ni, Ray]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, February 14, 2020 7:15 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Ard Biesheuvel
> <ard.biesheuvel@arm.com>
> Cc: leif@nuviainc.com; philmd@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell
> command to expose Linux initrd via device path
> 
> On 02/12/20 15:21, Ni, Ray wrote:
> >> (3) However: I think this should be added as a Dynamic Command instead.
> >> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
> >> Convert from NULL class library to Dynamic Command", 2017-11-28),
> which
> >> is the first commit in edk2 ever to introduce a Dynamic Command.
> >>
> >> And the commit message there says:
> >>
> >>     The guideline is:
> >>     1. Only use NULL class library for Shell spec defined commands.
> >>     2. New commands can be provided as not only a standalone application
> >>        but also a dynamic command. So it can be used either as an
> >>        internal command, but also as a standalone application.
> >>
> >> I'm not asking for the command to be usable as a separate application,
> >> but I think we might want to follow the first guideline.
> >>
> >> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
> >> commands, it does not seem to spell out guideline#1. So I think it's
> >> rather an edk2-specific guideline than a standard one. Nonetheless we
> >> might want to adhere to it.)
> >
> > Laszlo, thanks for the comments😊.
> > I didn't remember that I said these guideline publicly.
> > The reason behind that is we can have the same shell binary everywhere
> > and new non-spec commands can be added through dynamic command
> without
> > impacting the shell binary.
> 
> Thanks for the explanation -- this means that the NULL class lib
> approach is good for OvmfPkg after all. I'm putting the remaining parts
> of this patch back on my review queue (it will take a while).

Please don't misunderstand my points. I still prefer to use dynamic commands
for all non-spec defined shell internal commands.
Sorry for the confusion caused by my previous mail.

> 
> Thanks
> Laszlo

Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Laszlo Ersek]

On 02/14/20 01:55, Ni, Ray wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Friday, February 14, 2020 7:15 AM
>> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Ard Biesheuvel
>> <ard.biesheuvel@arm.com>
>> Cc: leif@nuviainc.com; philmd@redhat.com; Gao, Zhichao
>> <zhichao.gao@intel.com>
>> Subject: Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell
>> command to expose Linux initrd via device path
>>
>> On 02/12/20 15:21, Ni, Ray wrote:
>>>> (3) However: I think this should be added as a Dynamic Command instead.
>>>> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
>>>> Convert from NULL class library to Dynamic Command", 2017-11-28),
>> which
>>>> is the first commit in edk2 ever to introduce a Dynamic Command.
>>>>
>>>> And the commit message there says:
>>>>
>>>>     The guideline is:
>>>>     1. Only use NULL class library for Shell spec defined commands.
>>>>     2. New commands can be provided as not only a standalone application
>>>>        but also a dynamic command. So it can be used either as an
>>>>        internal command, but also as a standalone application.
>>>>
>>>> I'm not asking for the command to be usable as a separate application,
>>>> but I think we might want to follow the first guideline.
>>>>
>>>> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
>>>> commands, it does not seem to spell out guideline#1. So I think it's
>>>> rather an edk2-specific guideline than a standard one. Nonetheless we
>>>> might want to adhere to it.)
>>>
>>> Laszlo, thanks for the comments😊.
>>> I didn't remember that I said these guideline publicly.
>>> The reason behind that is we can have the same shell binary everywhere
>>> and new non-spec commands can be added through dynamic command
>> without
>>> impacting the shell binary.
>>
>> Thanks for the explanation -- this means that the NULL class lib
>> approach is good for OvmfPkg after all. I'm putting the remaining parts
>> of this patch back on my review queue (it will take a while).
> 
> Please don't misunderstand my points.

OK. From your response, I thought that the guidelines you captured in
the commit message in question were only for internal shell builds.

> I still prefer to use dynamic commands
> for all non-spec defined shell internal commands.
> Sorry for the confusion caused by my previous mail.

It's OK, I understand better now. So I guess I'll de-queue the review of
the rest of this patch once again, and wait for the next version (with
the dynamic command implementation).

Thank you!
Laszlo

Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path

[Ard Biesheuvel]

On Fri, 14 Feb 2020 at 15:17, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/14/20 01:55, Ni, Ray wrote:
> >
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Friday, February 14, 2020 7:15 AM
> >> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Ard Biesheuvel
> >> <ard.biesheuvel@arm.com>
> >> Cc: leif@nuviainc.com; philmd@redhat.com; Gao, Zhichao
> >> <zhichao.gao@intel.com>
> >> Subject: Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell
> >> command to expose Linux initrd via device path
> >>
> >> On 02/12/20 15:21, Ni, Ray wrote:
> >>>> (3) However: I think this should be added as a Dynamic Command instead.
> >>>> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
> >>>> Convert from NULL class library to Dynamic Command", 2017-11-28),
> >> which
> >>>> is the first commit in edk2 ever to introduce a Dynamic Command.
> >>>>
> >>>> And the commit message there says:
> >>>>
> >>>>     The guideline is:
> >>>>     1. Only use NULL class library for Shell spec defined commands.
> >>>>     2. New commands can be provided as not only a standalone application
> >>>>        but also a dynamic command. So it can be used either as an
> >>>>        internal command, but also as a standalone application.
> >>>>
> >>>> I'm not asking for the command to be usable as a separate application,
> >>>> but I think we might want to follow the first guideline.
> >>>>
> >>>> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
> >>>> commands, it does not seem to spell out guideline#1. So I think it's
> >>>> rather an edk2-specific guideline than a standard one. Nonetheless we
> >>>> might want to adhere to it.)
> >>>
> >>> Laszlo, thanks for the comments.
> >>> I didn't remember that I said these guideline publicly.
> >>> The reason behind that is we can have the same shell binary everywhere
> >>> and new non-spec commands can be added through dynamic command
> >> without
> >>> impacting the shell binary.
> >>
> >> Thanks for the explanation -- this means that the NULL class lib
> >> approach is good for OvmfPkg after all. I'm putting the remaining parts
> >> of this patch back on my review queue (it will take a while).
> >
> > Please don't misunderstand my points.
>
> OK. From your response, I thought that the guidelines you captured in
> the commit message in question were only for internal shell builds.
>
> > I still prefer to use dynamic commands
> > for all non-spec defined shell internal commands.
> > Sorry for the confusion caused by my previous mail.
>
> It's OK, I understand better now. So I guess I'll de-queue the review of
> the rest of this patch once again, and wait for the next version (with
> the dynamic command implementation).
>

Thanks for the review and the clarification. I will change this into a
dynamic command for v2, but it may be a while before I get back to it,
since this feature is still under discussion on the Linux side as
well.