From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web11.1831.1583230745476333543 for ; Tue, 03 Mar 2020 02:19:06 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=lSqVfHVK; spf=pass (domain: linaro.org, ip: 209.85.128.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f68.google.com with SMTP id g83so2231185wme.1 for ; Tue, 03 Mar 2020 02:19:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=DjRc9UTNSpYUIwOWjiA7ykSGKtYWHLd9B1bstdpvE+I=; b=lSqVfHVKZuXvNQNHDkQQKr6esI5jfE6Sv8CGvL7NtQMwHYi7qdtT2WAA+qAVMdkSaK 9on7w4S7hiSRTHFXrfNuy4WASfpRp3V1ElEQ4+BWEh7dMV8bD50HRS8a2Sq/lW+mQXAZ v0DNYD/FRMEsfeCRqI/7lbLayhb9YjmLhbehXYzSABeiSJUAfBVYp6iK0ARxrw648NhZ Mb/m4XXjY/AiumxgScG2pVmmiHeUe/GjhUqqI6bNVlDkBQ5TBNRTRbSwZL7COI5XT22u iCQi+DF3Av1Ag5eer5U7+yA8s0K1UEawsnmuhh1Gdf3B4E7NSeQdoM1rovDZvAP+ACps D/yA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DjRc9UTNSpYUIwOWjiA7ykSGKtYWHLd9B1bstdpvE+I=; b=gCEzp9M2gQeQ1ZoS1bfI1QS/n3vH9500woAYJzW6DpUqu2kyiLSk10JpHT/TfsKfzl ycNA01PYj72yHi8Pqv9llA//3EOu26ZmJtRzoUhKzualKNWenONwtouy6Zrmu9adJZrI 1GUMaG7fwRQAmXABmgparl163GTgYhFN6yKIiUXoeLKOYXl3ZrJQLqAgnP1NdZ9vPYbi SnXUMDu4jN39Oj79YivNuFjnTERi0wSJvxYwjt22O5Ksjnl0yx8oWwYgSG7Yvgg/oHpA IRsSGYJyaPsHNWa6OnWddolC8O0iXl5ltodWtw8U/38fIkA4pNwwTu3ge9HuWjS5/c+o gR7Q== X-Gm-Message-State: ANhLgQ3ge94dGtzv+RDktVBrcvvDheFNwTxz5gBC8HyAS/KyOiPTvr1I wCBsbSCiXqlEpyFFNCyzjs7fsPbbTBTevHM1IwNTkg== X-Google-Smtp-Source: ADFU+vtS3P+PqaCN55oHNPfMlQ+RuXiI+oM8ZMEf3es2Y3PoyTtnC3eDgldlUYbhY5VLV0/mC8JIsWdgCTqzGct/8+c= X-Received: by 2002:a7b:cb93:: with SMTP id m19mr3775082wmi.133.1583230743870; Tue, 03 Mar 2020 02:19:03 -0800 (PST) MIME-Version: 1.0 References: <20200302072936.29221-1-ard.biesheuvel@linaro.org> <20200302072936.29221-13-ard.biesheuvel@linaro.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 3 Mar 2020 11:18:53 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 12/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for new Linux initrd device path To: Laszlo Ersek Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" On Tue, 3 Mar 2020 at 11:10, Laszlo Ersek wrote: > > On 03/02/20 08:29, Ard Biesheuvel wrote: > > Linux v5.7 will introduce a new method to load the initial ramdisk > > (initrd) from the loader, using the LoadFile2 protocol installed on a > > special vendor GUIDed media device path. > > > > Add support for this to our QEMU command line kernel/initrd loader. > > > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566 > > Signed-off-by: Ard Biesheuvel > > --- > > OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 79 ++++++++++++++++++++ > > OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf | 2 + > > 2 files changed, 81 insertions(+) > > > > diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c > > index 77d8fedb738a..415946edf22a 100644 > > --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c > > +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c > > @@ -13,15 +13,18 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > #include > > #include > > +#include > > #include > > > > // > > @@ -84,6 +87,19 @@ STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mFileSystemDevicePath = { > > } > > }; > > > > +STATIC CONST SINGLE_VENMEDIA_NODE_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) } > > + } > > +}; > > + > > // > > // The "file in the EFI stub filesystem" abstraction. > > // > > @@ -839,6 +855,48 @@ STATIC CONST EFI_SIMPLE_FILE_SYSTEM_PROTOCOL mFileSystem = { > > StubFileSystemOpenVolume > > }; > > > > +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 > > + ) > > +{ > > + CONST KERNEL_BLOB *InitrdBlob = &mKernelBlob[KernelBlobTypeInitrd]; > > + > > + ASSERT (InitrdBlob->Size > 0); > > + > > + 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) { > > + return EFI_NOT_FOUND; > > + } > > + > > + if (Buffer == NULL || *BufferSize < InitrdBlob->Size) { > > + *BufferSize = InitrdBlob->Size; > > + return EFI_BUFFER_TOO_SMALL; > > + } > > + > > + CopyMem (Buffer, InitrdBlob->Data, InitrdBlob->Size); > > + > > + *BufferSize = InitrdBlob->Size; > > + return EFI_SUCCESS; > > +} > > + > > +STATIC CONST EFI_LOAD_FILE2_PROTOCOL mInitrdLoadFile2 = { > > + InitrdLoadFile2, > > +}; > > > > // > > // Utility functions. > > @@ -945,6 +1003,7 @@ QemuKernelLoaderFsDxeEntrypoint ( > > KERNEL_BLOB *KernelBlob; > > EFI_STATUS Status; > > EFI_HANDLE FileSystemHandle; > > + EFI_HANDLE InitrdLoadFile2Handle; > > > > if (!QemuFwCfgIsAvailable ()) { > > return EFI_NOT_FOUND; > > @@ -989,8 +1048,28 @@ QemuKernelLoaderFsDxeEntrypoint ( > > goto FreeBlobs; > > } > > > > + if (KernelBlob[KernelBlobTypeInitrd].Size > 0) { > > + InitrdLoadFile2Handle = NULL; > > + Status = gBS->InstallMultipleProtocolInterfaces (&InitrdLoadFile2Handle, > > + &gEfiDevicePathProtocolGuid, &mInitrdDevicePath, > > + &gEfiLoadFile2ProtocolGuid, &mInitrdLoadFile2, > > + NULL); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "%a: InstallMultipleProtocolInterfaces(): %r\n", > > + __FUNCTION__, Status)); > > + goto UninstallFileSystemHandle; > > + } > > + } > > + > > return EFI_SUCCESS; > > > > +UninstallFileSystemHandle: > > + Status = gBS->UninstallMultipleProtocolInterfaces (FileSystemHandle, > > + &gEfiDevicePathProtocolGuid, &mFileSystemDevicePath, > > + &gEfiSimpleFileSystemProtocolGuid, &mFileSystem, > > + NULL); > > + ASSERT_EFI_ERROR (Status); > > + > > FreeBlobs: > > while (BlobType > 0) { > > CurrentBlob = &mKernelBlob[--BlobType]; > > diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf > > index f4b50c265027..737f9b87a7c7 100644 > > --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf > > +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf > > @@ -28,6 +28,7 @@ [LibraryClasses] > > BaseLib > > BaseMemoryLib > > DebugLib > > + DevicePathLib > > MemoryAllocationLib > > UefiBootServicesTableLib > > QemuFwCfgLib > > @@ -42,6 +43,7 @@ [Guids] > > > > [Protocols] > > gEfiDevicePathProtocolGuid ## PRODUCES > > + gEfiLoadFile2ProtocolGuid ## PRODUCES > > gEfiSimpleFileSystemProtocolGuid ## PRODUCES > > > > [Depex] > > > > I think this patch conflicts with your other patch: > > [edk2-devel] [PATCH v3 2/6] OvmfPkg: add 'initrd' shell command to > expose Linux initrd via device path > > There should not be two different handles in the handle database that > carry the same device path (I mean "same device path" by contents, not > by reference). > > I believe that it's possible that the command-line specified -kernel / > -initrd are attempted first, but the kernel's stub returns for some > reason. Then the user can still go to the UEFI shell, and use the new > dynamic shell command. In that case I think the device path will cease > being unique (by contents). > > I think you can solve this as follows: > > - in both modules (this driver, and the dynamic shell command driver), > install a NULL protocol interface with GUID = gEfiCallerIdGuid on the > same handle that carries the device path and LoadFile2 > > - in both modules, before you touch the handle that carries the device > path, try to locate it first. If it exists but doesn't carry the subject > module's FILE_GUID as a NULL protocol interface, we know the device path > was created by the "other" module, and we can bail. > > Just an idea. > Thanks. I did wonder about the rules regarding duplicates in the device path space. If the DXE core doesn't catch that, this seems like a gross oversight to me. But it raises an interesting point: the idea is that any boot stage could elect to provide this interface, not just UEFI or the shell itself, but also shim, grub or whatever executes in between. That means that, in general, it should probably be the responsibility of the code that installs the protocol to ensure that it doesn't exist already. For this code, it means it could simply install it, since it guarantees to execute first. For the initrd implementation, it means we should check whether any other implementation exists already, which could simply be done [in that particular case] by locating the protocol and checking the address against the statically allocated one in the driver. I am aware that both are DXE drivers, but since the dynamic shell command is not invoked by any driver that is invoked before EndOfDxe, I think the above should be sufficient.