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

Join devel@edk2.groups.io to automatically receive all group messages.