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.61]) by mx.groups.io with SMTP id smtpd.web12.11690.1583411636206496843 for ; Thu, 05 Mar 2020 04:33:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Pdrgty26; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583411635; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FizzPcfwZRIrkOvNWb2AscEfo57a9tfc9rMgRjgxdLQ=; b=Pdrgty26l7Fp4i+6PmEly7wB3kHC20lyGFLLFHNn/a1Vhq4XIsVoyFS0Hpza9xitUXLXw6 Xh3O/vpvTjXsQiwM35FE4XvS7a38eglWvxQUVttDvziuof8VE+JMEVchq6Uy3+nCh4zATg jAPi1VKNrZQq1Ici3JD3SWWo7eijHys= 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-16-13G05eGgOsyOHCNtzw1vTg-1; Thu, 05 Mar 2020 07:33:50 -0500 X-MC-Unique: 13G05eGgOsyOHCNtzw1vTg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 098118017DF; Thu, 5 Mar 2020 12:33:50 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-216.ams2.redhat.com [10.36.117.216]) by smtp.corp.redhat.com (Postfix) with ESMTP id 06E4B10016EB; Thu, 5 Mar 2020 12:33:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 10/14] OvmfPkg: implement QEMU loader library for X86 with legacy fallback To: devel@edk2.groups.io, ard.biesheuvel@linaro.org References: <20200304095233.21046-1-ard.biesheuvel@linaro.org> <20200304095233.21046-11-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <5804d507-be60-ff76-b159-e721d84a8046@redhat.com> Date: Thu, 5 Mar 2020 13:33:48 +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: <20200304095233.21046-11-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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/04/20 10:52, Ard Biesheuvel wrote: > Implement another version of QemuLoadImageLib that uses LoadImage and > StartImage, but falls back to the legacy Linux loader code if that > fails. The logic in the legacy fallback routines is identical to the > current QEMU linux loader for X64 and IA32. > > Note the use of a LoadedImage pseudo-protocol for the legacy loaded > image: this makes it possible to expose the LoadImage/StartImage > abstraction for the legacy loader, using the EFI paradigm of > identifying loaded image solely by a handle. The pseudo-protocol > record type and the use of CR() is to get DEBUG coverage for the code > that deals with these handles. (1) Please remove the last sentence; it no longer applies. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566 > Signed-off-by: Ard Biesheuvel > --- > OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 564 ++++++++++++++++++++ > OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 42 ++ > 2 files changed, 606 insertions(+) > > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > new file mode 100644 > index 000000000000..da7a90d9c829 > --- /dev/null > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > @@ -0,0 +1,564 @@ > +/** @file > + X86 specific implementation of QemuLoadImageLib library class interface > + with support for loading mixed mode images and non-EFI stub images > + > + Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
> + 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 > +#include > +#include > + > +#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) } > + } > +}; > + > +typedef struct { > + VOID *SetupBuf; > + VOID *KernelBuf; > + CHAR8 *CommandLine; > + VOID *InitrdData; > + UINTN SetupSize; > + UINTN KernelInitialSize; > + UINTN InitrdSize; > + UINTN CommandLineSize; > +} QEMU_LEGACY_LOADED_IMAGE; > + > +STATIC > +VOID > +FreeLegacyImage ( > + IN QEMU_LEGACY_LOADED_IMAGE *LoadedImage > + ) > +{ > + if (LoadedImage->SetupBuf != NULL) { > + FreePages (LoadedImage->SetupBuf, > + EFI_SIZE_TO_PAGES (LoadedImage->SetupSize)); > + } > + if (LoadedImage->KernelBuf != NULL) { > + FreePages (LoadedImage->KernelBuf, > + EFI_SIZE_TO_PAGES (LoadedImage->KernelInitialSize)); > + } > + if (LoadedImage->CommandLine != NULL) { > + FreePages (LoadedImage->CommandLine, > + EFI_SIZE_TO_PAGES (LoadedImage->CommandLineSize)); > + } > + if (LoadedImage->InitrdData != NULL) { > + FreePages (LoadedImage->InitrdData, > + EFI_SIZE_TO_PAGES (LoadedImage->InitrdSize)); > + } > +} > + > +STATIC > +EFI_STATUS > +QemuLoadLegacyImage ( > + OUT EFI_HANDLE *ImageHandle > + ) > +{ > + EFI_STATUS Status; > + UINTN KernelSize; > + UINTN SetupSize; > + QEMU_LEGACY_LOADED_IMAGE *LoadedImage; > + > + QemuFwCfgSelectItem (QemuFwCfgItemKernelSize); > + KernelSize = (UINTN)QemuFwCfgRead32 (); > + > + QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize); > + SetupSize = (UINTN)QemuFwCfgRead32 (); > + > + if (KernelSize == 0 || SetupSize == 0) { > + DEBUG ((DEBUG_INFO, "qemu -kernel was not used.\n")); > + return EFI_NOT_FOUND; > + } > + > + LoadedImage = AllocateZeroPool (sizeof (*LoadedImage)); > + if (LoadedImage == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + LoadedImage->SetupSize = SetupSize; > + LoadedImage->SetupBuf = LoadLinuxAllocateKernelSetupPages ( > + EFI_SIZE_TO_PAGES (LoadedImage->SetupSize)); > + if (LoadedImage->SetupBuf == NULL) { > + DEBUG ((DEBUG_ERROR, "Unable to allocate memory for kernel setup!\n")); > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeImageDesc; > + } > + > + DEBUG ((DEBUG_INFO, "Setup size: 0x%x\n", (UINT32)LoadedImage->SetupSize)); > + DEBUG ((DEBUG_INFO, "Reading kernel setup image ...")); > + QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupData); > + QemuFwCfgReadBytes (LoadedImage->SetupSize, LoadedImage->SetupBuf); > + DEBUG ((DEBUG_INFO, " [done]\n")); > + > + Status = LoadLinuxCheckKernelSetup (LoadedImage->SetupBuf, > + LoadedImage->SetupSize); > + if (EFI_ERROR (Status)) { > + goto FreeImage; > + } > + > + Status = LoadLinuxInitializeKernelSetup (LoadedImage->SetupBuf); > + if (EFI_ERROR (Status)) { > + goto FreeImage; > + } > + > + LoadedImage->KernelInitialSize = LoadLinuxGetKernelSize ( > + LoadedImage->SetupBuf, KernelSize); > + if (LoadedImage->KernelInitialSize == 0) { > + Status = EFI_UNSUPPORTED; > + goto FreeImage; > + } > + > + LoadedImage->KernelBuf = LoadLinuxAllocateKernelPages ( > + LoadedImage->SetupBuf, > + EFI_SIZE_TO_PAGES (LoadedImage->KernelInitialSize) > + ); > + if (LoadedImage->KernelBuf == NULL) { > + DEBUG ((DEBUG_ERROR, "Unable to allocate memory for kernel!\n")); > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeImage; > + } > + > + DEBUG ((DEBUG_INFO, "Kernel size: 0x%x\n", (UINT32)KernelSize)); > + DEBUG ((DEBUG_INFO, "Reading kernel image ...")); > + QemuFwCfgSelectItem (QemuFwCfgItemKernelData); > + QemuFwCfgReadBytes (KernelSize, LoadedImage->KernelBuf); > + DEBUG ((DEBUG_INFO, " [done]\n")); > + > + QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize); > + LoadedImage->CommandLineSize = (UINTN)QemuFwCfgRead32 (); > + > + if (LoadedImage->CommandLineSize > 0) { > + LoadedImage->CommandLine = LoadLinuxAllocateCommandLinePages ( > + EFI_SIZE_TO_PAGES ( > + LoadedImage->CommandLineSize)); > + QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); > + QemuFwCfgReadBytes (LoadedImage->CommandLineSize, LoadedImage->CommandLine); > + } > + > + Status = LoadLinuxSetCommandLine (LoadedImage->SetupBuf, > + LoadedImage->CommandLine); > + if (EFI_ERROR (Status)) { > + goto FreeImage; > + } > + > + QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize); > + LoadedImage->InitrdSize = (UINTN)QemuFwCfgRead32 (); > + > + if (LoadedImage->InitrdSize > 0) { > + LoadedImage->InitrdData = LoadLinuxAllocateInitrdPages ( > + LoadedImage->SetupBuf, > + EFI_SIZE_TO_PAGES (LoadedImage->InitrdSize)); > + DEBUG ((DEBUG_INFO, "Initrd size: 0x%x\n", > + (UINT32)LoadedImage->InitrdSize)); > + DEBUG ((DEBUG_INFO, "Reading initrd image ...")); > + QemuFwCfgSelectItem (QemuFwCfgItemInitrdData); > + QemuFwCfgReadBytes (LoadedImage->InitrdSize, LoadedImage->InitrdData); > + DEBUG ((DEBUG_INFO, " [done]\n")); > + } > + > + Status = LoadLinuxSetInitrd (LoadedImage->SetupBuf, LoadedImage->InitrdData, > + LoadedImage->InitrdSize); > + if (EFI_ERROR (Status)) { > + goto FreeImage; > + } > + > + *ImageHandle = NULL; I agree, this belongs here. > + Status = gBS->InstallProtocolInterface (ImageHandle, > + &gX86QemuKernelLoadedImageGuid, EFI_NATIVE_INTERFACE, > + LoadedImage); > + if (EFI_ERROR (Status)) { > + goto FreeImage; > + } > + return EFI_SUCCESS; > + > +FreeImage: > + FreeLegacyImage (LoadedImage); > +FreeImageDesc: > + FreePool (LoadedImage); > + return Status; > +} > + > +STATIC > +EFI_STATUS > +QemuStartLegacyImage ( > + IN EFI_HANDLE ImageHandle > + ) > +{ > + EFI_STATUS Status; > + QEMU_LEGACY_LOADED_IMAGE *LoadedImage; > + > + Status = gBS->OpenProtocol (ImageHandle, > + &gX86QemuKernelLoadedImageGuid, > + (VOID **)&LoadedImage, > + gImageHandle, // AgentHandle > + NULL, // ControllerHandle > + EFI_OPEN_PROTOCOL_GET_PROTOCOL > + ); > + if (EFI_ERROR (Status)) { > + return EFI_INVALID_PARAMETER; > + } > + > + return LoadLinux (LoadedImage->KernelBuf, LoadedImage->SetupBuf); > +} > + > +STATIC > +EFI_STATUS > +QemuUnloadLegacyImage ( > + IN EFI_HANDLE ImageHandle > + ) > +{ > + EFI_STATUS Status; > + QEMU_LEGACY_LOADED_IMAGE *LoadedImage; > + > + Status = gBS->OpenProtocol (ImageHandle, > + &gX86QemuKernelLoadedImageGuid, > + (VOID **)&LoadedImage, > + gImageHandle, // AgentHandle > + NULL, // ControllerHandle > + EFI_OPEN_PROTOCOL_GET_PROTOCOL > + ); > + if (EFI_ERROR (Status)) { > + return EFI_INVALID_PARAMETER; > + } > + > + Status = gBS->UninstallProtocolInterface (ImageHandle, > + &gX86QemuKernelLoadedImageGuid, ImageHandle); (2) Typo: the last argument (for parameter "Interface") should be LoadedImage, not ImageHandle. > + ASSERT_EFI_ERROR (Status); > + > + FreeLegacyImage (LoadedImage); > + FreePool (LoadedImage); > + return EFI_SUCCESS; > +} > + > +/** > + 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 > + > + @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. > + > + @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_NOT_FOUND: > + // > + // The image does not exist - no -kernel image was supplied via the > + // command line so no point in invoking the legacy fallback > + // > + return EFI_NOT_FOUND; > + > + case EFI_SECURITY_VIOLATION: > + // > + // We are running with UEFI secure boot enabled, and the image failed to > + // authenticate. For compatibility reasons, we fall back to the legacy > + // loader in this case. Since the image has been loaded, we need to unload > + // it before proceeding > + // > + gBS->UnloadImage (KernelImageHandle); > + // > + // Fall through > + // > + case EFI_UNSUPPORTED: > + // > + // The image is not natively supported or cross-type supported. Let's try > + // loading it using the loader that parses the bzImage metadata directly. > + // > + Status = QemuLoadLegacyImage (&KernelImageHandle); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: QemuLoadLegacyImage(): %r\n", __FUNCTION__, > + Status)); > + return Status; > + } > + *ImageHandle = KernelImageHandle; > + return EFI_SUCCESS; > + > + default: > + DEBUG ((DEBUG_ERROR, "%a: LoadImage(): %r\n", __FUNCTION__, Status)); > + return Status; > + } OK. So we reach this point only if the gBS->LoadImage() above succeeds. Therefore the rest of the function below follows the generic method: > + > + // > + // 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); > + ASSERT (CommandLine != NULL); > + > + 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: > + FreePool (CommandLine); > + gBS->UnloadImage (KernelImageHandle); > + > + return Status; > +} (3) AFAICT you forgot to sync the "CommandLine" lifecycle fixes from the generic instance to this instance. There's no UnloadImage label, and no (CommandLineSize > 0) check before freeing "CommandLine". Here's a diff (between both implementations of this API) that shows what's needed here: > @@ -99,7 +79,10 @@ > KernelLoadedImage->LoadOptionsSize = 0; > } else { > CommandLine = AllocatePool (CommandLineSize); > - ASSERT (CommandLine != NULL); > + if (CommandLine == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto UnloadImage; > + } > > QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); > QemuFwCfgReadBytes (CommandLineSize, CommandLine); > @@ -121,7 +104,7 @@ > } > > QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize); > - InitrdSize = (UINTN) QemuFwCfgRead32 (); > + InitrdSize = (UINTN)QemuFwCfgRead32 (); > > if (InitrdSize > 0) { > // > @@ -161,7 +144,10 @@ > return EFI_SUCCESS; > > FreeCommandLine: > - FreePool (CommandLine); > + if (CommandLineSize > 0) { > + FreePool (CommandLine); > + } > +UnloadImage: > gBS->UnloadImage (KernelImageHandle); > > return Status; Back to this patch: > + > +/** > + 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 > + ) > +{ > + EFI_STATUS Status; > + QEMU_LEGACY_LOADED_IMAGE *LoadedImage; > + EFI_HANDLE KernelImageHandle; > + > + Status = gBS->OpenProtocol (*ImageHandle, (4) Please break this argument to a separate line, too. > + &gX86QemuKernelLoadedImageGuid, > + (VOID **)&LoadedImage, > + gImageHandle, // AgentHandle > + NULL, // ControllerHandle > + EFI_OPEN_PROTOCOL_GET_PROTOCOL > + ); > + if (!EFI_ERROR (Status)) { > + return QemuStartLegacyImage (*ImageHandle); > + } > + > + Status = gBS->StartImage ( > + *ImageHandle, > + NULL, // ExitDataSize > + NULL // ExitData > + ); > +#ifdef MDE_CPU_IA32 > + if (Status == EFI_UNSUPPORTED) { > + // > + // On IA32, EFI_UNSUPPORTED means that the image's machine type is X64 while > + // we are expecting a IA32 one, and the StartImage () boot service is unable > + // to handle it, either because the image does not have the special .compat > + // PE/COFF section that Linux specifies for mixed mode capable images, or > + // because we are running without the support code for that. So unload the > + // image, and reload and start it using the legacy loader. > + // > + QemuUnloadKernelImage (*ImageHandle); > + > + Status = QemuLoadLegacyImage (&KernelImageHandle); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + *ImageHandle = KernelImageHandle; > + return QemuStartLegacyImage (KernelImageHandle); > + } > +#endif > + return Status; > +} This function is now safe, with regard to the life cycle of handles created with LoadImage(). However, there is a code path in the function that releases the former image handle, and creates no new image handle. It happens when QemuLoadLegacyImage() fails. That will cause a problem for the caller. In both patch#6 ("ArmVirtPkg/PlatformBootManagerLib: switch to separate QEMU loader") and patch#12 ("OvmfPkg/PlatformBootManagerLib: switch to QemuLoadImageLib"), QemuUnloadKernelImage() is called -- correctly! -- after QemuStartKernelImage() fails. That works fine if QemuStartKernelImage() leaves the handle intact, or replaces it with a new (but also valid) handle. It doesn't work if QemuStartKernelImage() only invalidates the handle. (5) Therefore I suggest calling QemuLoadLegacyImage() first, and QemuUnloadKernelImage() second: #ifdef MDE_CPU_IA32 if (Status == EFI_UNSUPPORTED) { // // explain what's what // Status = QemuLoadLegacyImage (&KernelImageHandle); if (EFI_ERROR (Status)) { // // Note: no change to (*ImageHandle), the caller will release it. // return Status; } // // Swap in the legacy-loaded image. // QemuUnloadKernelImage (*ImageHandle); *ImageHandle = KernelImageHandle; return QemuStartLegacyImage (KernelImageHandle); } #endif Considering the "handle count", this approach uses 1 -> 2 -> 1, rather than 1 -> 0 -> 1. Holding two handles "in the middle" is safe, holding zero is not. Note: I realize that with this, we're returning to the original order of operations, seen in the previous version. But now we're going to use *different* handles! This also means that only *two* use cases remain for QemuUnloadKernelImage() -- at any point in time, either the custom (legacy) protocol is installed on the handle, or the standard protocol. They are mutually exclusive, at all times. > + > +/** > + 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 images 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 (Status == EFI_UNSUPPORTED) { > + // > + // The handle exists but does not have an instance of the standard loaded > + // image protocol installed on it. Attempt to unload it as a legacy image > + // instead. > + // > + return QemuUnloadLegacyImage (ImageHandle); > + } > + > + if (EFI_ERROR (Status)) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // We are unloading an normal, non-legacy loaded image, either on behalf of (6) s/an normal/a normal/ > + // an external caller, or called from QemuStartKernelImage() on IA32, while > + // switching from the normal to the legacy method to load and start a X64 > + // image. > + // > + if (KernelLoadedImage->LoadOptions != NULL) { > + FreePool (KernelLoadedImage->LoadOptions); > + KernelLoadedImage->LoadOptions = NULL; > + } > + KernelLoadedImage->LoadOptionsSize = 0; > + > + return gBS->UnloadImage (ImageHandle); > +} Perfect. > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf > new file mode 100644 > index 000000000000..1568a02bbd4f > --- /dev/null > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf > @@ -0,0 +1,42 @@ > +## @file > +# X86 specific implementation of QemuLoadImageLib library class interface > +# with support for loading mixed mode images and non-EFI stub images > +# > +# Copyright (c) 2020, ARM Ltd. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 1.27 > + BASE_NAME = X86QemuLoadImageLib > + FILE_GUID = 2304df80-e21d-4170-9c3c-113c878f7ac0 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = QemuLoadImageLib|DXE_DRIVER > + > +[Sources] > + X86QemuLoadImageLib.c > + > +[Packages] > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + DebugLib > + MemoryAllocationLib > + LoadLinuxLib > + PrintLib > + QemuFwCfgLib > + ReportStatusCodeLib > + UefiBootServicesTableLib > + > +[Protocols] > + gEfiDevicePathProtocolGuid > + gEfiLoadedImageProtocolGuid > + gX86QemuKernelLoadedImageGuid > + > +[Guids] > + gQemuKernelLoaderFsMediaGuid > Looks good. Thanks! Laszlo