From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.354.1582674230597150060 for ; Tue, 25 Feb 2020 15:43:51 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Msy7TpYi; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582674229; 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=rDy7Ve6It+s8vNKjV09FS6Lf3aHz0j47fc8z6TORXtk=; b=Msy7TpYi0lmW9ZvGM9EoHlWSSK4jSBitXzrszOKzlZwb/sGAT5DnEtvEpcb2ZzAiWJ6aF8 aEE2UNGU3NMCBt7d+Qc5WmIPKV55b2T6ADtYhDqd0KVlmb93LtpyyUeBRZFFazyOdP3mRY RzRyzO6E8jjtcbW+j+u0dhm29Viry20= 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-456-OMbYczkrP9O5QK0q2zJjeg-1; Tue, 25 Feb 2020 18:43:45 -0500 X-MC-Unique: OMbYczkrP9O5QK0q2zJjeg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AF31C8017CC; Tue, 25 Feb 2020 23:43:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-104.ams2.redhat.com [10.36.117.104]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3653B60BE2; Tue, 25 Feb 2020 23:43:39 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: leif@nuviainc.com, pjones@redhat.com, mjg59@google.com, agraf@csgraf.de, daniel.kiper@oracle.com, michael.d.kinney@intel.com, jian.j.wang@intel.com, hao.a.wu@intel.com, ray.ni@intel.com, zhichao.gao@intel.com References: <20200225093908.6707-1-ard.biesheuvel@linaro.org> <20200225093908.6707-3-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <2192beca-d8af-beae-cf72-ba63ad0883f7@redhat.com> Date: Wed, 26 Feb 2020 00:43:38 +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: <20200225093908.6707-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 On 02/25/20 10:39, 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 > --- > OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c | 423 ++++++++++++++++++++ > OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf | 52 +++ > OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni | 49 +++ > 3 files changed, 524 insertions(+) > > diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c > new file mode 100644 > index 000000000000..2e8541be07d5 > --- /dev/null > +++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.c > @@ -0,0 +1,423 @@ > +/** @file > + Provides 'initrd' dynamic UEFI shell 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 (1) Please add gEfiDevicePathProtocolGuid to the INF file, under [Protocols]. > +#include > +#include > +#include > + > +#pragma pack(1) > +typedef struct { > + VENDOR_DEVICE_PATH VenMediaNode; > + EFI_DEVICE_PATH_PROTOCOL EndNode; > +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH; > +#pragma pack() (2) please insert spaces before the opening parens. > + > +STATIC EFI_HII_HANDLE mLinuxInitrdShellCommandHiiHandle; > +STATIC EFI_PHYSICAL_ADDRESS mInitrdFileAddress; > +STATIC UINT64 mInitrdFileSize; > +STATIC EFI_HANDLE mInitrdLoadFile2Handle; > + > +STATIC CONST SHELL_PARAM_ITEM ParamList[] = { > + {L"-u", TypeFlag}, > + {NULL, TypeMax} > + }; > + > +STATIC CONST SINGLE_NODE_VENDOR_MEDIA_DEVPATH mInitrdDevicePath = { > + { > + { > + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) } > + }, > + LINUX_EFI_INITRD_MEDIA_GUID > + }, { > + 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 (3) Possibly a bug in a standard edk2 header file (or maybe even in the spec), but I think Buffer should be marked OUT. > + ) > +{ > + 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 || > + mInitrdFileSize == 0) { > + return EFI_NOT_FOUND; > + } > + > + if (Buffer == NULL || *BufferSize < mInitrdFileSize) { > + *BufferSize = mInitrdFileSize; > + return EFI_BUFFER_TOO_SMALL; > + } > + > + ASSERT (mInitrdFileAddress != 0); > + > + gBS->CopyMem (Buffer, (VOID *)(UINTN)mInitrdFileAddress, mInitrdFileSize); > + *BufferSize = mInitrdFileSize; > + return EFI_SUCCESS; > +} > + > +STATIC CONST EFI_LOAD_FILE2_PROTOCOL mInitrdLoadFile2 = { > + InitrdLoadFile2, > +}; > + > +STATIC > +EFI_STATUS > +UninstallLoadFile2Protocol ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + > + if (mInitrdLoadFile2Handle) { (4) Please spell out the "!= NULL" explicitly (per edk2 coding style / preference) > + Status = gBS->UninstallMultipleProtocolInterfaces (mInitrdLoadFile2Handle, > + &gEfiDevicePathProtocolGuid, &mInitrdDevicePath, > + &gEfiLoadFile2ProtocolGuid, &mInitrdLoadFile2, > + NULL); > + if (!EFI_ERROR (Status)) { > + mInitrdLoadFile2Handle = NULL; > + } > + } > + return Status; > +} > + > +STATIC > +VOID > +FreeInitrdFile ( > + VOID > + ) > +{ > + if (mInitrdFileSize != 0) { > + gBS->FreePages (mInitrdFileAddress, EFI_SIZE_TO_PAGES (mInitrdFileSize)); > + mInitrdFileSize = 0; > + } > +} > + > +STATIC > +EFI_STATUS > +CacheInitrdFile ( > + IN SHELL_FILE_HANDLE FileHandle > + ) > +{ > + EFI_STATUS Status; > + UINT64 FileSize; > + UINTN ReadSize; > + > + Status = gEfiShellProtocol->GetFileSize (FileHandle, &FileSize); > + if (EFI_ERROR(Status)) { > + return Status; > + } So, this is tricky. The method called above indeed outputs a UINT64, into FileSize. I guess that's why you also made "mInitrdFileSize" a UINT64. However, every *read* of "mInitrdFileSize" in this file actually wants the variable to be UINTN. I've checked: - (*BufferSize) in InitrdLoadFile2() is a UINTN - gBS->CopyMem() -- EFI_COPY_MEM -- takes a UINTN - EFI_SIZE_TO_PAGES() takes a UINTN - gBS->FreePages() takes a UINTN So please, (5a) flip the type of "mInitrdFileSize" to UINTN, (5b) right here, please check if FileSize (which *should* remain a UINT64) exceeds MAX_UINTN. If it does, the function should return EFI_UNSUPPORTED. (Effectively, that could happen if the user attempted to load a >=4GB file as initrd in a 32-bit shell.) (5c) (mInitrdFileSize == 0) carries special meaning in this implementation. I suggest catching (FileSize==0) right here, and returning with EFI_UNSUPPORTED also. (That will at once eliminate questions like, "how does AllocatePages() react to zero pages requested?", too.) > + > + Status = gBS->AllocatePages (AllocateAnyPages, EfiLoaderData, Hmmm. Ultimately, the allocation will be performed by the UEFI shell, and so EfiLoaderData is justified, per "Table 29. Memory Type Usage before ExitBootServices()". OK. > + EFI_SIZE_TO_PAGES (FileSize), &mInitrdFileAddress); (6) Please cast FileSize to UINTN here. > + if (EFI_ERROR(Status)) { (7) missing whitespace (please check the other EFI_ERROR() invocations too, there are other instances) > + return Status; > + } > + > + ReadSize = FileSize; (Right, this assignment depends on (5b) too) > + Status = gEfiShellProtocol->ReadFile (FileHandle, &ReadSize, > + (VOID *)(UINTN)mInitrdFileAddress); > + if (EFI_ERROR(Status) || ReadSize < FileSize) { > + DEBUG ((DEBUG_WARN, "%a: failed to read initrd file - %r 0x%llx 0x%llx\n", > + __FUNCTION__, Status, (UINT64)ReadSize, FileSize)); (8) Please use just "%lx" for logging UINT64, not "%llx". (PrintLib peculiarity.) I guess the double ell might not hurt, but it suggests that the second "l" has extra meaning, and it doesn't. ('l' and 'L' simply set LONG_TYPE in BasePrintLibSPrintMarker(), file "MdePkg/Library/BasePrintLib/PrintLibInternal.c".) > + goto FreeMemory; > + } > + > + if (mInitrdLoadFile2Handle == NULL) { > + Status = gBS->InstallMultipleProtocolInterfaces (&mInitrdLoadFile2Handle, > + &gEfiDevicePathProtocolGuid, &mInitrdDevicePath, > + &gEfiLoadFile2ProtocolGuid, &mInitrdLoadFile2, > + NULL); > + ASSERT_EFI_ERROR (Status); OK (although it's not difficult to jump to FreeMemory here too) > + } > + > + mInitrdFileSize = FileSize; > + return EFI_SUCCESS; > + > +FreeMemory: > + gBS->FreePages (mInitrdFileAddress, EFI_SIZE_TO_PAGES (FileSize)); (9) Please cast FileSize to UINTN. > + return Status; > +} > + > +/** > + 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). > +**/ > +STATIC > +SHELL_STATUS > +EFIAPI > +RunInitrd ( > + 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; > + SHELL_FILE_HANDLE FileHandle; > + > + 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), > + mLinuxInitrdShellCommandHiiHandle, 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), > + mLinuxInitrdShellCommandHiiHandle, L"initrd"); > + ShellStatus = SHELL_INVALID_PARAMETER; > + } else if (ShellCommandLineGetCount (Package) < 2) { > + if (ShellCommandLineGetFlag(Package, L"-u")) { (10) missing whitespace > + FreeInitrdFile (); > + UninstallLoadFile2Protocol (); > + } else { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_FEW), > + mLinuxInitrdShellCommandHiiHandle, L"initrd"); > + ShellStatus = SHELL_INVALID_PARAMETER; > + } > + } else { > + Param = ShellCommandLineGetRawValue (Package, 1); > + ASSERT (Param != NULL); > + > + Filename = ShellFindFilePath (Param); (11) Filename should be freed at some point; currently, it's leaked (if the call succeeds). > + if (Filename == NULL) { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FIND_FAIL), > + mLinuxInitrdShellCommandHiiHandle, L"initrd", Param); > + ShellStatus = SHELL_NOT_FOUND; > + } else { > + FreeInitrdFile (); > + > + Status = ShellOpenFileByName (Filename, &FileHandle, > + EFI_FILE_MODE_READ, 0); > + if (!EFI_ERROR (Status)) { (12) You could delay FreeInitrdFile() until this point. > + Status = CacheInitrdFile (FileHandle); > + } > + if (EFI_ERROR (Status)) { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL), > + mLinuxInitrdShellCommandHiiHandle, L"initrd", Param); > + ShellStatus = SHELL_NOT_FOUND; > + } > + ShellCloseFile (&FileHandle); (13) If ShellOpenFileByName() fails, it seems to set FileHandle to NULL. Can ShellCloseFile() cope with NULL? FWIW, its sole parameter is not marked OPTIONAL. > + } > + } > + } > + return ShellStatus; > +} (14) I think it would be more idiomatic for edk2 if we used a copious amount of "goto" statements (jumping to error handling labels at the end) rather than nesting "if"s this deeply. I think it also might help with releasing resources on error. > + > + > +/** > + This is the shell command handler function pointer callback type. This > + function handles the command when it is invoked in the shell. > + > + @param[in] This The instance of the > + EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL. > + @param[in] SystemTable The pointer to the system table. > + @param[in] ShellParameters The parameters associated with the command. > + @param[in] Shell The instance of the shell protocol used in > + the context of processing this command. > + > + @return EFI_SUCCESS the operation was successful > + @return other the operation failed. > +**/ > +SHELL_STATUS > +EFIAPI > +LinuxInitrdCommandHandler ( > + IN EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL *This, > + IN EFI_SYSTEM_TABLE *SystemTable, > + IN EFI_SHELL_PARAMETERS_PROTOCOL *ShellParameters, > + IN EFI_SHELL_PROTOCOL *Shell > + ) > +{ > + gEfiShellParametersProtocol = ShellParameters; > + gEfiShellProtocol = Shell; > + > + return RunInitrd (gImageHandle, SystemTable); > +} > + > +/** > + This is the command help handler function pointer callback type. This > + function is responsible for displaying help information for the associated > + command. > + > + @param[in] This The instance of the > + EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL. > + @param[in] Language The pointer to the language string to use. > + > + @return string Pool allocated help string, must be freed > + by caller > +**/ > +STATIC > +CHAR16 * > +EFIAPI > +LinuxInitrdGetHelp ( > + IN EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL *This, > + IN CONST CHAR8 *Language > + ) > +{ > + return HiiGetString (mLinuxInitrdShellCommandHiiHandle, > + STRING_TOKEN (STR_GET_HELP_INITRD), Language); > +} > + > +STATIC EFI_SHELL_DYNAMIC_COMMAND_PROTOCOL mLinuxInitrdDynamicCommand = { > + L"initrd", > + LinuxInitrdCommandHandler, > + LinuxInitrdGetHelp > +}; > + > +/** > + Retrieve HII package list from ImageHandle and publish to HII database. > + > + @param ImageHandle The image handle of the process. > + > + @return HII handle. > +**/ > +STATIC > +EFI_HII_HANDLE > +InitializeHiiPackage ( > + EFI_HANDLE ImageHandle > + ) > +{ > + EFI_STATUS Status; > + EFI_HII_PACKAGE_LIST_HEADER *PackageList; > + EFI_HII_HANDLE HiiHandle; > + > + // > + // Retrieve HII package list from ImageHandle > + // > + Status = gBS->OpenProtocol (ImageHandle, &gEfiHiiPackageListProtocolGuid, > + (VOID **)&PackageList, ImageHandle, NULL, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL); > + ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + return NULL; > + } > + > + // > + // Publish HII package list to HII Database. > + // > + Status = gHiiDatabase->NewPackageList (gHiiDatabase, PackageList, NULL, > + &HiiHandle); > + ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + return NULL; > + } > + return HiiHandle; > +} > + > +/** > + Entry point of Linux Initrd dynamic UEFI Shell command. > + > + Produce the DynamicCommand protocol to handle "initrd" command. > + > + @param ImageHandle The image handle of the process. > + @param SystemTable The EFI System Table pointer. > + > + @retval EFI_SUCCESS Initrd command is executed successfully. > + @retval EFI_ABORTED HII package was failed to initialize. > + @retval others Other errors when executing Initrd command. > +**/ > +EFI_STATUS > +EFIAPI > +LinuxInitrdDynamicShellCommandEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + mLinuxInitrdShellCommandHiiHandle = InitializeHiiPackage (ImageHandle); > + if (mLinuxInitrdShellCommandHiiHandle == NULL) { > + return EFI_ABORTED; > + } > + > + Status = gBS->InstallProtocolInterface (&ImageHandle, > + &gEfiShellDynamicCommandProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &mLinuxInitrdDynamicCommand); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} > + > +/** > + Unload the dynamic UEFI Shell command. > + > + @param ImageHandle The image handle of the process. > + > + @retval EFI_SUCCESS The image is unloaded. > + @retval Others Failed to unload the image. > +**/ > +EFI_STATUS > +EFIAPI > +LinuxInitrdDynamicShellCommandUnload ( > + IN EFI_HANDLE ImageHandle > +) > +{ > + EFI_STATUS Status; > + > + Status = UninstallLoadFile2Protocol (); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = gBS->UninstallProtocolInterface (ImageHandle, > + &gEfiShellDynamicCommandProtocolGuid, > + &mLinuxInitrdDynamicCommand); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + HiiRemovePackages (mLinuxInitrdShellCommandHiiHandle); > + return EFI_SUCCESS; > +} (15) *Assuming* we want to tear down everything here (after all, removing loadfile2 will make the initrd unloadable), I think we should also call FreeInitrdFile(). Otherwise, we leak it. But, more on the assumption later. > diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf > new file mode 100644 > index 000000000000..cdfdddf77a4d > --- /dev/null > +++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf > @@ -0,0 +1,52 @@ > +## @file > +# Provides 'initrd' dynamic UEFI shell 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 = LinuxInitrdDynamicShellCommand > + FILE_GUID = 2f30da26-f51b-4b6f-85c4-31873c281bca > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = LinuxInitrdDynamicShellCommandEntryPoint > + UNLOAD_IMAGE = LinuxInitrdDynamicShellCommandUnload > + UEFI_HII_RESOURCE_SECTION = TRUE > + > +# > +# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64 EBC > +# > + > +[Sources.common] > + LinuxInitrdDynamicShellCommand.c > + LinuxInitrdDynamicShellCommand.uni > + > +[Packages] > + MdePkg/MdePkg.dec > + ShellPkg/ShellPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + DebugLib > + DevicePathLib > + HiiLib > + MemoryAllocationLib > + ShellLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + UefiHiiServicesLib > + > +[Protocols] > + gEfiHiiPackageListProtocolGuid ## CONSUMES > + gEfiLoadFile2ProtocolGuid ## SOMETIMES_PRODUCES > + gEfiShellDynamicCommandProtocolGuid ## PRODUCES > + > +[DEPEX] > + TRUE Seems OK. > diff --git a/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni > new file mode 100644 > index 000000000000..a88fa6e3641b > --- /dev/null > +++ b/OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.uni > @@ -0,0 +1,49 @@ > +// /** > +// > +// Copyright (c) 2020, Arm, Ltd. All rights reserved.
> +// SPDX-License-Identifier: BSD-2-Clause-Patent > +// > +// Module Name: > +// > +// LinuxInitrdDynamicShellCommand.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 initrd 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" > +".SH DESCRIPTION\r\n" > +" \r\n" > +"NOTES:\r\n" > +" 1. Only a single file can be loaded as initrd at any given time. Using the\r\n" > +" command twice with a option will result in the first file to\r\n" > +" be unloaded again, regardless of whether the second invocation succeeded\r\n" > +" or not.\r\n" > +" 2. The initrd is not unloaded when the shell exits, and will remain active\r\n" > +" until it is unloaded again by a different invocation of the shell.\r\n" > +" Consumers of the LoadFile2 protocol on the LINUX_EFI_INITRD_MEDIA_GUID\r\n" > +" device path that are started via means other than the shell will be able\r\n" > +" to locate the protocol and invoke it.\r\n" > (16) So, I don't see how paragraph#2 works here. When the shell exits, all our global variables disappear. (LoadFile2 disappears too.) The next time the shell is launched (in the same "UEFI session", so to say), we won't know that we had loaded an initrd before. Is that right? If we really want the initrd to be a system-wide resource, then we have to use the associated device path (which is fixed, and global) as a lookup key, for determining if an initrd already exists. Accordingly, we should likely store our data that's currently kept in global variables -- and so die when the shell exits -- in a container structure that embeds both the device path protocol and the loadfile2 protocol. Then we can use the CR() macro in the loadfile2 implementation to get back at the private data. #define LOADED_INITRD_SIG \ SIGNATURE_64 ('L', 'D', 'I', 'N', 'I', 'T', 'R', 'D') typedef struct { // // LOADED_INITRD_SIGNATURE // UINT64 Signature; // // memory block base and size // EFI_PHYSICAL_ADDRESS Address; UINTN Size; // // mInitrdDevicePath is still needed for populating the one below, // and for lookup with gBS->LocateDevicePath() // SINGLE_NODE_VENDOR_MEDIA_DEVPATH InstalledDevicePath; // // LF2 implementation // EFI_LOAD_FILE2_PROTOCOL LoadFile2; } LOADED_INITRD; #define LOADED_INITRD_FROM_LOADED_FILE(LoadFile2Ptr) \ CR (LoadFile2Ptr, LOADED_INITRD, LoadFile2, LOADED_INITRD_SIG) Thanks Laszlo