From: "Laszlo Ersek" <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH 12/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for new Linux initrd device path
Date: Tue, 3 Mar 2020 12:27:06 +0100 [thread overview]
Message-ID: <ef1ed7f6-020a-cbdf-31e5-115dd261013b@redhat.com> (raw)
In-Reply-To: <CAKv+Gu8PMq6Y4_6HHJ8RJ7DO7KXSBWogC_AdZ3H73dkicLL5uw@mail.gmail.com>
On 03/03/20 11:18, Ard Biesheuvel wrote:
> On Tue, 3 Mar 2020 at 11:10, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
>>> ---
>>> 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 <Guid/FileInfo.h>
>>> #include <Guid/FileSystemInfo.h>
>>> #include <Guid/FileSystemVolumeLabelInfo.h>
>>> +#include <Guid/LinuxEfiInitrdMedia.h>
>>> #include <Guid/QemuKernelLoaderFsMedia.h>
>>> #include <Library/BaseLib.h>
>>> #include <Library/BaseMemoryLib.h>
>>> #include <Library/DebugLib.h>
>>> +#include <Library/DevicePathLib.h>
>>> #include <Library/MemoryAllocationLib.h>
>>> #include <Library/QemuFwCfgLib.h>
>>> #include <Library/UefiBootServicesTableLib.h>
>>> #include <Library/UefiRuntimeServicesTableLib.h>
>>> #include <Protocol/DevicePath.h>
>>> +#include <Protocol/LoadFile2.h>
>>> #include <Protocol/SimpleFileSystem.h>
>>>
>>> //
>>> @@ -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.
>
Sounds OK to me.
Thanks
Laszlo
next prev parent reply other threads:[~2020-03-03 11:27 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 7:29 [PATCH 00/13] Ovmf: use LoadImage/StartImage for loading command line images Ard Biesheuvel
2020-03-02 7:29 ` [PATCH 01/13] OvmfPkg: add GUID for the QEMU kernel loader fs media device path Ard Biesheuvel
2020-03-02 13:22 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 02/13] OvmfPkg: export abstract QEMU blob filesystem in standalone driver Ard Biesheuvel
2020-03-02 13:45 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 03/13] OvmfPkg: introduce QemuLoadImageLib library class Ard Biesheuvel
2020-03-02 14:07 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 04/13] OvmfPkg: provide a generic implementation of QemuLoadImageLib Ard Biesheuvel
2020-03-02 17:12 ` [edk2-devel] " Laszlo Ersek
2020-03-03 7:36 ` Laszlo Ersek
2020-03-02 7:29 ` [PATCH 05/13] ArmVirtPkg: incorporate the new QEMU kernel loader driver and library Ard Biesheuvel
2020-03-02 17:15 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 06/13] ArmVirtPkg/PlatformBootManagerLib: switch to separate QEMU loader Ard Biesheuvel
2020-03-02 17:26 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 07/13] OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line Ard Biesheuvel
2020-03-02 17:31 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 08/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for the kernel setup block Ard Biesheuvel
2020-03-02 17:58 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 09/13] OvmfPkg: implement QEMU loader library for X86 with legacy fallback Ard Biesheuvel
2020-03-03 9:45 ` [edk2-devel] " Laszlo Ersek
2020-03-03 10:08 ` Ard Biesheuvel
2020-03-03 11:20 ` Laszlo Ersek
2020-03-02 7:29 ` [PATCH 10/13] OvmfPkg: add new QEMU kernel image loader components Ard Biesheuvel
2020-03-03 9:47 ` [edk2-devel] " Laszlo Ersek
2020-03-02 7:29 ` [PATCH 11/13] OvmfPkg/PlatformBootManagerLib: switch to QemuLoadImageLib Ard Biesheuvel
2020-03-03 9:52 ` [edk2-devel] " Laszlo Ersek
2020-03-03 9:53 ` Laszlo Ersek
2020-03-02 7:29 ` [PATCH 12/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for new Linux initrd device path Ard Biesheuvel
2020-03-03 10:10 ` [edk2-devel] " Laszlo Ersek
2020-03-03 10:18 ` Ard Biesheuvel
2020-03-03 11:27 ` Laszlo Ersek [this message]
2020-03-02 7:29 ` [PATCH 13/13] OvmfPkg: use generic QEMU image loader for secure boot enabled builds Ard Biesheuvel
2020-03-03 10:13 ` [edk2-devel] " Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ef1ed7f6-020a-cbdf-31e5-115dd261013b@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox