From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, ard.biesheuvel@linaro.org
Subject: Re: [edk2-devel] [PATCH v2 04/14] OvmfPkg: provide a generic implementation of QemuLoadImageLib
Date: Thu, 5 Mar 2020 12:29:06 +0100 [thread overview]
Message-ID: <6c1d2510-7714-9eb7-c99e-bc5f59b411ac@redhat.com> (raw)
In-Reply-To: <1da5be78-4fe4-a672-9f57-d029a99cc606@redhat.com>
On 03/05/20 10:51, Laszlo Ersek wrote:
> On 03/04/20 10:52, Ard Biesheuvel wrote:
>> Implement QemuLoadImageLib, and make it load the image provided by the
>> QEMU_EFI_LOADER_FS_MEDIA_GUID/kernel device path that we implemented
>> in a preceding patch in a separate DXE driver, using only the standard
>> LoadImage and StartImage boot services.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 278 ++++++++++++++++++++
>> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 38 +++
>> 2 files changed, 316 insertions(+)
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
One request though:
>
>> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
>> new file mode 100644
>> index 000000000000..f5edb43cc0b9
>> --- /dev/null
>> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
>> @@ -0,0 +1,278 @@
>> +/** @file
>> + Generic implementation of QemuLoadImageLib library class interface.
>> +
>> + Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#include <Uefi.h>
>> +
>> +#include <Base.h>
>> +#include <Guid/QemuKernelLoaderFsMedia.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/PrintLib.h>
>> +#include <Library/QemuFwCfgLib.h>
>> +#include <Library/QemuLoadImageLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Protocol/DevicePath.h>
>> +#include <Protocol/LoadedImage.h>
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> + EFI_DEVICE_PATH_PROTOCOL FilePathHeader;
>> + CHAR16 FilePath[ARRAY_SIZE (L"kernel")];
>> +} KERNEL_FILE_DEVPATH;
>> +
>> +typedef struct {
>> + VENDOR_DEVICE_PATH VenMediaNode;
>> + KERNEL_FILE_DEVPATH FileNode;
>> + EFI_DEVICE_PATH_PROTOCOL EndNode;
>> +} KERNEL_VENMEDIA_FILE_DEVPATH;
>> +#pragma pack ()
>> +
>> +STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
>> + {
>> + {
>> + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
>> + { sizeof (VENDOR_DEVICE_PATH) }
>> + },
>> + QEMU_KERNEL_LOADER_FS_MEDIA_GUID
>> + }, {
>> + {
>> + MEDIA_DEVICE_PATH, MEDIA_FILEPATH_DP,
>> + { sizeof (KERNEL_FILE_DEVPATH) }
>> + },
>> + L"kernel",
>> + }, {
>> + END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> + { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
>> + }
>> +};
>> +
>> +/**
>> + Download the kernel, the initial ramdisk, and the kernel command line from
>> + QEMU's fw_cfg. The kernel will be instructed via its command line to load
>> + the initrd from the same Simple FileSystem where the kernel was loaded from.
>> +
>> + @param[out] ImageHandle The image handle that was allocated for
>> + loading the image
>> + @param[out] LoadedImage The loaded image protocol that was installed
>> + on ImageHandle by the LoadImage boot service.
(1) Please remove this parameter. (I've noticed this now, after diffing
the two implementations of this function, including leading comments.)
The R-b stands.
Thanks!
Laszlo
>> +
>> + @retval EFI_SUCCESS The image was loaded successfully.
>> + @retval EFI_NOT_FOUND Kernel image was not found.
>> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
>> + @retval EFI_PROTOCOL_ERROR Unterminated kernel command line.
>> + @retval EFI_ACCESS_DENIED The underlying LoadImage boot service call
>> + returned EFI_SECURITY_VIOLATION, and the image
>> + was unloaded again.
>> +
>> + @return Error codes from any of the underlying
>> + functions.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +QemuLoadKernelImage (
>> + OUT EFI_HANDLE *ImageHandle
>> + )
>> +{
>> + EFI_STATUS Status;
>> + EFI_HANDLE KernelImageHandle;
>> + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
>> + UINTN CommandLineSize;
>> + CHAR8 *CommandLine;
>> + UINTN InitrdSize;
>> +
>> + //
>> + // Load the image. This should call back into the QEMU EFI loader file system.
>> + //
>> + Status = gBS->LoadImage (
>> + FALSE, // BootPolicy: exact match required
>> + gImageHandle, // ParentImageHandle
>> + (EFI_DEVICE_PATH_PROTOCOL *)&mKernelDevicePath,
>> + NULL, // SourceBuffer
>> + 0, // SourceSize
>> + &KernelImageHandle
>> + );
>> + switch (Status) {
>> + case EFI_SUCCESS:
>> + break;
>> +
>> + case EFI_SECURITY_VIOLATION:
>> + //
>> + // In this case, the image was loaded but failed to authenticate.
>> + //
>> + Status = EFI_ACCESS_DENIED;
>> + goto UnloadImage;
>> +
>> + default:
>> + DEBUG ((DEBUG_ERROR, "%a: LoadImage(): %r\n", __FUNCTION__, Status));
>> + return Status;
>> + }
>> +
>> + //
>> + // Construct the kernel command line.
>> + //
>> + Status = gBS->OpenProtocol (
>> + KernelImageHandle,
>> + &gEfiLoadedImageProtocolGuid,
>> + (VOID **)&KernelLoadedImage,
>> + gImageHandle, // AgentHandle
>> + NULL, // ControllerHandle
>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL
>> + );
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
>> + CommandLineSize = (UINTN)QemuFwCfgRead32 ();
>> +
>> + if (CommandLineSize == 0) {
>> + KernelLoadedImage->LoadOptionsSize = 0;
>> + } else {
>> + CommandLine = AllocatePool (CommandLineSize);
>> + if (CommandLine == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto UnloadImage;
>> + }
>> +
>> + QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
>> + QemuFwCfgReadBytes (CommandLineSize, CommandLine);
>> +
>> + //
>> + // Verify NUL-termination of the command line.
>> + //
>> + if (CommandLine[CommandLineSize - 1] != '\0') {
>> + DEBUG ((DEBUG_ERROR, "%a: kernel command line is not NUL-terminated\n",
>> + __FUNCTION__));
>> + Status = EFI_PROTOCOL_ERROR;
>> + goto FreeCommandLine;
>> + }
>> +
>> + //
>> + // Drop the terminating NUL, convert to UTF-16.
>> + //
>> + KernelLoadedImage->LoadOptionsSize = (CommandLineSize - 1) * 2;
>> + }
>> +
>> + QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
>> + InitrdSize = (UINTN)QemuFwCfgRead32 ();
>> +
>> + if (InitrdSize > 0) {
>> + //
>> + // Append ' initrd=initrd' in UTF-16.
>> + //
>> + KernelLoadedImage->LoadOptionsSize += sizeof (L" initrd=initrd") - 2;
>> + }
>> +
>> + if (KernelLoadedImage->LoadOptionsSize == 0) {
>> + KernelLoadedImage->LoadOptions = NULL;
>> + } else {
>> + //
>> + // NUL-terminate in UTF-16.
>> + //
>> + KernelLoadedImage->LoadOptionsSize += 2;
>> +
>> + KernelLoadedImage->LoadOptions = AllocatePool (
>> + KernelLoadedImage->LoadOptionsSize);
>> + if (KernelLoadedImage->LoadOptions == NULL) {
>> + KernelLoadedImage->LoadOptionsSize = 0;
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto FreeCommandLine;
>> + }
>> +
>> + UnicodeSPrintAsciiFormat (
>> + KernelLoadedImage->LoadOptions,
>> + KernelLoadedImage->LoadOptionsSize,
>> + "%a%a",
>> + (CommandLineSize == 0) ? "" : CommandLine,
>> + (InitrdSize == 0) ? "" : " initrd=initrd"
>> + );
>> + DEBUG ((DEBUG_INFO, "%a: command line: \"%s\"\n", __FUNCTION__,
>> + (CHAR16 *)KernelLoadedImage->LoadOptions));
>> + }
>> +
>> + *ImageHandle = KernelImageHandle;
>> + return EFI_SUCCESS;
>> +
>> +FreeCommandLine:
>> + if (CommandLineSize > 0) {
>> + FreePool (CommandLine);
>> + }
>> +UnloadImage:
>> + gBS->UnloadImage (KernelImageHandle);
>> +
>> + return Status;
>> +}
>> +
>> +/**
>> + Transfer control to a kernel image loaded with QemuLoadKernelImage ()
>> +
>> + @param[in,out] ImageHandle Handle of image to be started. May assume a
>> + different value on return if the image was
>> + reloaded.
>> +
>> + @retval EFI_INVALID_PARAMETER ImageHandle is either an invalid image handle
>> + or the image has already been initialized with
>> + StartImage
>> + @retval EFI_SECURITY_VIOLATION The current platform policy specifies that the
>> + image should not be started.
>> +
>> + @return Error codes returned by the started image
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +QemuStartKernelImage (
>> + IN OUT EFI_HANDLE *ImageHandle
>> + )
>> +{
>> + return gBS->StartImage (
>> + *ImageHandle,
>> + NULL, // ExitDataSize
>> + NULL // ExitData
>> + );
>> +}
>> +
>> +/**
>> + Unloads an image loaded with QemuLoadKernelImage ().
>> +
>> + @param ImageHandle Handle that identifies the image to be
>> + unloaded.
>> +
>> + @retval EFI_SUCCESS The image has been unloaded.
>> + @retval EFI_UNSUPPORTED The image has been started, and does not
>> + support unload.
>> + @retval EFI_INVALID_PARAMETER ImageHandle is not a valid image handle.
>> +
>> + @return Exit code from the image’s unload function.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +QemuUnloadKernelImage (
>> + IN EFI_HANDLE ImageHandle
>> + )
>> +{
>> + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
>> + EFI_STATUS Status;
>> +
>> + Status = gBS->OpenProtocol (
>> + ImageHandle,
>> + &gEfiLoadedImageProtocolGuid,
>> + (VOID **)&KernelLoadedImage,
>> + gImageHandle, // AgentHandle
>> + NULL, // ControllerHandle
>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL
>> + );
>> + if (EFI_ERROR (Status)) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + if (KernelLoadedImage->LoadOptions != NULL) {
>> + FreePool (KernelLoadedImage->LoadOptions);
>> + KernelLoadedImage->LoadOptions = NULL;
>> + }
>> + KernelLoadedImage->LoadOptionsSize = 0;
>> +
>> + return gBS->UnloadImage (ImageHandle);
>> +}
>> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>> new file mode 100644
>> index 000000000000..b262cb926a4d
>> --- /dev/null
>> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>> @@ -0,0 +1,38 @@
>> +## @file
>> +# Generic implementation of QemuLoadImageLib library class interface.
>> +#
>> +# Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>> +[Defines]
>> + INF_VERSION = 1.27
>> + BASE_NAME = GenericQemuLoadImageLib
>> + FILE_GUID = 9e3e28da-c7b5-4f85-841a-84e6a9a1f1a0
>> + MODULE_TYPE = BASE
>> + VERSION_STRING = 1.0
>> + LIBRARY_CLASS = QemuLoadImageLib|DXE_DRIVER
>> +
>> +[Sources]
>> + GenericQemuLoadImageLib.c
>> +
>> +[Packages]
>> + MdeModulePkg/MdeModulePkg.dec
>> + MdePkg/MdePkg.dec
>> + OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> + DebugLib
>> + MemoryAllocationLib
>> + PrintLib
>> + QemuFwCfgLib
>> + UefiBootServicesTableLib
>> +
>> +[Protocols]
>> + gEfiDevicePathProtocolGuid
>> + gEfiLoadedImageProtocolGuid
>> +
>> +[Guids]
>> + gQemuKernelLoaderFsMediaGuid
>>
>
next prev parent reply other threads:[~2020-03-05 11:29 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-04 9:52 [PATCH v2 00/14] Ovmf: use LoadImage/StartImage for loading command line images Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 01/14] OvmfPkg: add GUID for the QEMU kernel loader fs media device path Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 02/14] OvmfPkg: export abstract QEMU blob filesystem in standalone driver Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 03/14] OvmfPkg: introduce QemuLoadImageLib library class Ard Biesheuvel
2020-03-05 9:37 ` [edk2-devel] " Laszlo Ersek
2020-03-05 9:39 ` Laszlo Ersek
2020-03-05 10:22 ` Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 04/14] OvmfPkg: provide a generic implementation of QemuLoadImageLib Ard Biesheuvel
2020-03-05 9:51 ` [edk2-devel] " Laszlo Ersek
2020-03-05 11:29 ` Laszlo Ersek [this message]
2020-03-05 11:37 ` Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 05/14] ArmVirtPkg: incorporate the new QEMU kernel loader driver and library Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 06/14] ArmVirtPkg/PlatformBootManagerLib: switch to separate QEMU loader Ard Biesheuvel
2020-03-05 10:01 ` [edk2-devel] " Laszlo Ersek
2020-03-04 9:52 ` [PATCH v2 07/14] OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 08/14] OvmfPkg/QemuKernelLoaderFsDxe: add support for the kernel setup block Ard Biesheuvel
2020-03-05 10:12 ` [edk2-devel] " Laszlo Ersek
2020-03-04 9:52 ` [PATCH v2 09/14] OvmfPkg: create protocol and GUID header for legacy loaded images Ard Biesheuvel
2020-03-05 10:31 ` [edk2-devel] " Laszlo Ersek
2020-03-05 10:40 ` Ard Biesheuvel
2020-03-05 14:29 ` Laszlo Ersek
2020-03-04 9:52 ` [PATCH v2 10/14] OvmfPkg: implement QEMU loader library for X86 with legacy fallback Ard Biesheuvel
2020-03-05 12:33 ` [edk2-devel] " Laszlo Ersek
2020-03-04 9:52 ` [PATCH v2 11/14] OvmfPkg: add new QEMU kernel image loader components Ard Biesheuvel
2020-03-04 9:52 ` [PATCH v2 12/14] OvmfPkg/PlatformBootManagerLib: switch to QemuLoadImageLib Ard Biesheuvel
2020-03-05 12:57 ` [edk2-devel] " Laszlo Ersek
2020-03-04 9:52 ` [PATCH v2 13/14] OvmfPkg/QemuKernelLoaderFsDxe: add support for new Linux initrd device path Ard Biesheuvel
2020-03-05 13:19 ` Laszlo Ersek
2020-03-04 9:52 ` [PATCH v2 14/14] OvmfPkg: use generic QEMU image loader for secure boot enabled builds Ard Biesheuvel
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=6c1d2510-7714-9eb7-c99e-bc5f59b411ac@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