From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.3386.1581506695178978401 for ; Wed, 12 Feb 2020 03:24:55 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JkwnB495; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581506694; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gVCGqgdJehs/Fo0/sIi6BSbjxaSTJmhjLMjFiCqs4NU=; b=JkwnB4958phdCcfU/vT06MyPrFHeUt4A0atfNGhWmKYNlrLVFC7d1Bci9t458CDIv4z2nk tLVypQ8x1Z2dbC8VfzQLSlu76+w89LTrs0o745/icoGMhKIjlMKjgXpxeTxJNi5TAt5FYQ YlWmZqpDe8TWB1e8bgi3D7L8ZoS0nwY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-301-vXTkiYAVPU2xRRQslINiPw-1; Wed, 12 Feb 2020 06:24:52 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0B450DBA3; Wed, 12 Feb 2020 11:24:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-72.ams2.redhat.com [10.36.117.72]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6302C5D9E2; Wed, 12 Feb 2020 11:24:46 +0000 (UTC) Subject: Re: [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path To: Ard Biesheuvel , devel@edk2.groups.io Cc: leif@nuviainc.com, philmd@redhat.com, Ray Ni , Zhichao Gao References: <20200211180304.21669-1-ard.biesheuvel@arm.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 12 Feb 2020 12:24:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200211180304.21669-1-ard.biesheuvel@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-MC-Unique: vXTkiYAVPU2xRRQslINiPw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > --- > 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.
> +# > +# 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.
> +// 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 \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.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#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""; > +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; > +}