From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.2332.1583234833849473795 for ; Tue, 03 Mar 2020 03:27:14 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fjTfNXgQ; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583234833; 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=OunOvTH9ss3ZoaP0lYLeg0vM2wnu2OcOYTHG/FvS478=; b=fjTfNXgQNOpCPMER7zEVzAdlC2aFhRTDa40uS/8OytvhcImZzvGW7VGjyUhYjD3cucGb/J g4fehK8NIfVGEwpUPUcs5L6oHfDX+YEPAkWVETsMQNlSfGgWMcH6d3k59bA+5KtIWU92BO KMOC/woGrTnUqrK12y1AOKCtOsXENwQ= 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-137-GD40NhApOWGphwftnQulhQ-1; Tue, 03 Mar 2020 06:27:09 -0500 X-MC-Unique: GD40NhApOWGphwftnQulhQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 04F60107B102; Tue, 3 Mar 2020 11:27:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-34.ams2.redhat.com [10.36.117.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4091090CEE; Tue, 3 Mar 2020 11:27:07 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 12/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for new Linux initrd device path To: Ard Biesheuvel Cc: edk2-devel-groups-io References: <20200302072936.29221-1-ard.biesheuvel@linaro.org> <20200302072936.29221-13-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: Date: Tue, 3 Mar 2020 12:27:06 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/03/20 11:18, Ard Biesheuvel wrote: > 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. > Sounds OK to me. Thanks Laszlo