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.biesheu...@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;
> +}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54286): https://edk2.groups.io/g/devel/message/54286
Mute This Topic: https://groups.io/mt/71177416/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
