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

["Laszlo Ersek" <lersek@redhat.com>]

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;
> +}