Re: [edk2-devel] [PATCH 09/13] OvmfPkg: implement QEMU loader library for X86 with legacy fallback

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Tue, 3 Mar 2020 at 10:45, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 03/02/20 08:29, 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.
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c   | 562 ++++++++++++++++++++
> >  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf |  42 ++
> >  OvmfPkg/OvmfPkg.dec                                         |   1 +
> >  3 files changed, 605 insertions(+)
>
> (1) "-C 41 --find-copies-harder" displays this lib instance as a
> modified version of the generic instance (from patch#4). Please sync
> this instance too with those comments (whichever apply).
>
> > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> > new file mode 100644
> > index 000000000000..a1ced417d1cc
> > --- /dev/null
> > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> > @@ -0,0 +1,562 @@
> > +/**  @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.<BR>
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#include <Uefi.h>
> > +
> > +#include <Guid/QemuKernelLoaderFsMedia.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/LoadLinuxLib.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[sizeof (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;
> > +
> > +#define QEMU_LEGACY_LOADED_IMAGE_SIG \
> > +          SIGNATURE_64 ('Q', 'L', 'O', 'A', 'D', 'I', 'M', 'G')
> > +
> > +typedef struct {
> > +  UINT64                    Signature;
> > +  QEMU_LEGACY_LOADED_IMAGE  LoadedImage;
> > +} QEMU_LEGACY_LOADED_IMAGE_REC;
> > +
> > +#define QEMU_LEGACY_LOADED_IMAGE_REC_FROM_LOADED_IMAGE(ImagePointer) \
> > +        CR (ImagePointer, QEMU_LEGACY_LOADED_IMAGE_REC, LoadedImage, \
> > +          QEMU_LEGACY_LOADED_IMAGE_SIG)
> > +
>
> I don't understand:
>
> - why we define two structures here (QEMU_LEGACY_LOADED_IMAGE and
> QEMU_LEGACY_LOADED_IMAGE_REC),
>
> - and the related use of a Signature + CR().
>
> These tools help if we have a pre-defined -- e.g., standard -- protocol,
> for which we'd like to provide an implementation, and we need to carry
> some auxiliary ("private") data for every protocol instance that we
> produce. In that case, we embed the pre-defined protocol structure in
> our own, and use CR() / Signature etc. to arrive at our own container
> structure, from a pointer to the pre-defined protocol. This is all
> motivated by the fact that the member functions of the pre-defined
> protocol take a "This" pointer to said pre-defined protocol.
>
> But in our case here, this use case doesn't seem to apply. We do not
> embed a pre-defined protocol structure; we define a brand new inner
> structure, and we introduce a brand new protocol GUID for it. We don't
> have any pre-existent function prototype that can only take a pointer to
> QEMU_LEGACY_LOADED_IMAGE, from which we'd have to arrive at
> QEMU_LEGACY_LOADED_IMAGE_REC.
>

OK. I liked the extra check against the signature in debug mode, but
given that there should not be any other code using our GUID for
installing protocols, I guess it is redundant. I'll drop it.

> (2) So that makes me think we should simply drop the macro and the
> wrapper structure.
>

Ack

> > +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_REC    *LoadedImageRec;
> > +  QEMU_LEGACY_LOADED_IMAGE        *LoadedImage;
>
> (3) If I'm right to think that this patch incorporates non-trivial code
> from TryRunningQemuKernel()
> [OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c], then please (a)
> note that in the commit message, (b) please preserve the Intel copyright
> notice.
>

OK

> > +
> > +  QemuFwCfgSelectItem (QemuFwCfgItemKernelSize);
> > +  KernelSize = (UINTN)QemuFwCfgRead64 ();
>
> Sigh, now I understand where the "64" comes from -- it's a bug in the
> original code. QEMU exposes these values with fw_cfg_add_i32(); see
> load_image_to_fw_cfg() in "hw/arm/boot.c", and the direct calls in
> "hw/i386/x86.c".
>
> We used to get away with Read64 only because edk2 uses little endian,
> and fw_cfg is specified to produce zeroes when reading past the end of
> an item.
>
> (4) So we should use *32 for reading *all* these items, regardless of
> "OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c". (I'm not going to
> point out each one below.)
>

Sure

> > +
> > +  QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
> > +  SetupSize = (UINTN)QemuFwCfgRead64 ();
> > +
> > +  if (KernelSize == 0 || SetupSize == 0) {
> > +    DEBUG ((DEBUG_INFO, "qemu -kernel was not used.\n"));
> > +    return EFI_NOT_FOUND;
> > +  }
> > +
> > +  LoadedImageRec = AllocateZeroPool (sizeof (*LoadedImageRec));
> > +  if (LoadedImageRec == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  LoadedImageRec->Signature = QEMU_LEGACY_LOADED_IMAGE_SIG;
> > +  LoadedImage = &LoadedImageRec->LoadedImage;
> > +
> > +  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"));
> > +    return EFI_OUT_OF_RESOURCES;
>
> (5) This leaks "LoadedImageRec".
>
> > +  }
> > +
> > +  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 FreeAndReturn;
> > +  }
>
> OK, this seems to release everything correctly.
>
> > +
> > +  Status = LoadLinuxInitializeKernelSetup (LoadedImage->SetupBuf);
> > +  if (EFI_ERROR (Status)) {
> > +    goto FreeAndReturn;
> > +  }
> > +
> > +  LoadedImage->KernelInitialSize = LoadLinuxGetKernelSize (
> > +                                     LoadedImage->SetupBuf, KernelSize);
> > +  if (LoadedImage->KernelInitialSize == 0) {
> > +    Status = EFI_UNSUPPORTED;
> > +    goto FreeAndReturn;
> > +  }
> > +
> > +  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 FreeAndReturn;
> > +  }
> > +
> > +  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)QemuFwCfgRead64 ();
> > +
> > +  if (LoadedImage->CommandLineSize > 0) {
> > +    LoadedImage->CommandLine = LoadLinuxAllocateCommandLinePages (
> > +                                 EFI_SIZE_TO_PAGES (
> > +                                   LoadedImage->CommandLineSize));
> > +    QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
> > +    QemuFwCfgReadBytes (LoadedImage->CommandLineSize, LoadedImage->CommandLine);
> > +  } else {
> > +    LoadedImage->CommandLine = NULL;
>
> (6) This assignment is superfluous. We allocate (*LoadedImageRec) with
> AllocateZeroPool(), which contains (*LoadedImage); and we rely on the
> zero-initialization in FreeLegacyImage().
>

Indeed.

> > +  }
> > +
> > +  Status = LoadLinuxSetCommandLine (LoadedImage->SetupBuf,
> > +             LoadedImage->CommandLine);
> > +  if (EFI_ERROR (Status)) {
> > +    goto FreeAndReturn;
> > +  }
> > +
> > +  QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
> > +  LoadedImage->InitrdSize = (UINTN)QemuFwCfgRead64 ();
> > +
> > +  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"));
> > +  } else {
> > +    LoadedImage->InitrdData = NULL;
>
> (7) Same as (6).
>
> > +  }
> > +
> > +  Status = LoadLinuxSetInitrd (LoadedImage->SetupBuf, LoadedImage->InitrdData,
> > +             LoadedImage->InitrdSize);
> > +  if (EFI_ERROR (Status)) {
> > +    goto FreeAndReturn;
> > +  }
> > +
> > +  Status = gBS->InstallProtocolInterface (ImageHandle,
> > +                  &gX86QemuKernelLoadedImageGuid, EFI_NATIVE_INTERFACE,
> > +                  LoadedImage);
>
> (8) This indicates that the identifier should actually be a
> *ProtocolGuid, with the usual consequences:
>
> - it should be put in [Protocols], not [Guids], in the INF file and the
> DEC file,
>
> - we should introduce the protocol related artifacts (the extern
> declaration for *ProtocolGuid, the definition of the initializer macro,
> and the typedef for the protocol structure) in a new header file under
> OvmfPkg/Include/Protocol.
>
> It's OK if the new protocol is not standard and subject to change at any
> time -- we only need to state that explicitly in the new header file.
>
> I understand that you might not want to expose so many internals in a
> protocol header, but, after all, we do introduce the GUID in the DEC
> file too.
>

Fair enough.

> > +  if (EFI_ERROR (Status)) {
> > +    goto FreeAndReturn;
> > +  }
> > +  return EFI_SUCCESS;
> > +
> > +FreeAndReturn:
> > +  FreeLegacyImage (LoadedImage);
> > +  FreePool (LoadedImageRec);
> > +  return Status;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +QemuStartLegacyImage (
> > +  IN  EFI_HANDLE                ImageHandle
> > +  )
> > +{
> > +  EFI_STATUS                    Status;
> > +  QEMU_LEGACY_LOADED_IMAGE      *LoadedImage;
> > +  QEMU_LEGACY_LOADED_IMAGE_REC  *LoadedImageRec;
> > +
> > +  Status = gBS->OpenProtocol (ImageHandle,
> > +                  &gX86QemuKernelLoadedImageGuid,
> > +                  (VOID **)&LoadedImage,
> > +                  gImageHandle,                  // AgentHandle
> > +                  NULL,                          // ControllerHandle
> > +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    return EFI_INVALID_PARAMETER;
>
> (Or you could return "Status" too (up to you); OpenProtocol() returns
> EFI_UNSUPPORTED if the protocol is not on the handle.
>
> If you decide to update this, then please keep QemuUnloadLegacyImage()
> in sync.)
>
> > +  }
> > +
> > +  LoadedImageRec = QEMU_LEGACY_LOADED_IMAGE_REC_FROM_LOADED_IMAGE (LoadedImage);
> > +
> > +  return LoadLinux (LoadedImageRec->LoadedImage.KernelBuf,
> > +                    LoadedImageRec->LoadedImage.SetupBuf);
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +QemuUnloadLegacyImage (
> > +  IN  EFI_HANDLE          ImageHandle
> > +  )
> > +{
> > +  EFI_STATUS                    Status;
> > +  QEMU_LEGACY_LOADED_IMAGE      *LoadedImage;
> > +  QEMU_LEGACY_LOADED_IMAGE_REC  *LoadedImageRec;
> > +
> > +  Status = gBS->OpenProtocol (ImageHandle,
> > +                  &gX86QemuKernelLoadedImageGuid,
> > +                  (VOID **)&LoadedImage,
> > +                  gImageHandle,                  // AgentHandle
> > +                  NULL,                          // ControllerHandle
> > +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  LoadedImageRec = QEMU_LEGACY_LOADED_IMAGE_REC_FROM_LOADED_IMAGE (LoadedImage);
> > +
> > +  FreeLegacyImage (&LoadedImageRec->LoadedImage);
> > +  FreePool (LoadedImageRec);
> > +  return EFI_SUCCESS;
> > +}
>
> (9) Please uninstall the protocol interface, before freeing it.
>

Ah yes, good point.

> > +
> > +/**
> > +  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.
> > +
> > +  @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.
> > +
> > +  @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
> > +    //
>
> Yes, good point; this actually supports my request (4a) under patch#4 --
> EFI_SECURITY_VIOLATION should be handled internally to the library.
>
> > +  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.
> > +    //
> > +    KernelImageHandle = NULL;
> > +    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;
> > +  }
> > +
> > +  //
> > +  // 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) QemuFwCfgRead64 ();
> > +
> > +  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) QemuFwCfgRead64 ();
> > +
> > +  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;
> > +}
> > +
> > +/**
> > +  Transfer control to a kernel image loaded with QemuLoadKernelImage ()
> > +
> > +  @param[in]  ImageHandle         Handle of image to be started.
> > +
> > +  @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  EFI_HANDLE          ImageHandle
> > +  )
> > +{
> > +  EFI_STATUS                Status;
> > +
> > +  Status = gBS->StartImage (
> > +                  ImageHandle,
> > +                  NULL,              // ExitDataSize
> > +                  NULL               // ExitData
> > +                  );
>
> (10) I'd like to avoid calling StartImage on a handle we know has not
> been created with gBS->LoadImage(). Can you first check for the custom
> protocol on the handle, and base QemuStartLegacyImage() on that (rather
> than the EFI_INVALID_PARAMETER return value)?
>

OK

>
> > +
> > +  switch (Status) {
> > +#ifdef MDE_CPU_IA32
> > +  case 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
>
> (11) So does this series depend on your other patch
>
>   [edk2-devel] [PATCH v3 6/6]
>   OvmfPkg IA32: add support for loading X64 images
>
> (which is for <https://bugzilla.tianocore.org/show_bug.cgi?id=2564>)?
>
> The topic branch that you reference in the cover letter of this series
> seems to be based on those patches.
>
> Should we make <https://bugzilla.tianocore.org/show_bug.cgi?id=2566> --
> i.e. the BZ that tracks the present feature -- dependent on TianoCore#2564?
>
> > +    // because we are running without the support code for that. So do a legacy
> > +    // load instead, but do it first so we can reuse the same handle. Then,
> > +    // unload the normally loaded image.
> > +    //
> > +    Status = QemuLoadLegacyImage (&ImageHandle);
>
> Hmmm, I'm getting quite confused here. This seems to indicate that:
>
> - even though we do recognize *some* cases when QemuLoadLegacyImage()
> needs to be called from QemuLoadKernelImage() -- which makes sense to me --,
>
> - there are *other* cases needing QemuLoadLegacyImage() that we can only
> recognize here, in QemuStartKernelImage(), *after* the normal
> LoadImage() branch in QemuLoadKernelImage() succeeded.
>
> Is that right?
>

Yes. IA32 builds will permit the loading of X64 images, but will not
allow them to be started. So we need to fallback to the legacy
*Load*Image, in the implementation of QemuStartImage

> If so, then I guess the idea here is to first add another protocol on
> the existent ImageHandle (our own custom one), before allowing
> gBS->UnloadImage() in QemuUnloadKernelImage() to remove the protocol
> interfaces produced by gBS->LoadImage(), and thereby destroy the handle.
>
> I have two concerns:
>
> (12) I'd rather like this part of the code to know for sure that our
> custom protocol is not present yet on the image handle -- see (10) above,
>

OK.

> (13) I'm worried that we are installing custom protocols on a handle
> that was first created by LoadImage(), before we pass it to
> UnloadImage(). I don't know if, per spec, LoadImage() / UnloadImage()
> are allowed to associate such information with the specific image handle
> that is *beyond* the protocol interfaces visible on the handle.
>

I'm not sure I follow. Why would it not be allowed to install
additional protocols on that handle? Note that it is rather common for
drivers to install arbitrary protocols on the handle that they receive
via the PE/COFF entry point.


> Can we modify the logic somehow so that we don't have to silently call
> QemuLoadLegacyImage() inside QemuStartKernelImage()? If not, we should
> at least document that this is arguably a grey area per UEFI spec.
>

That would require QemuStartImage() to take a EFI_HANDLE* rather than
a EFI_HANDLE, which is what I was trying to avoid here.

>
> > +    if (EFI_ERROR (Status)) {
> > +      return Status;
> > +    }
> > +    QemuUnloadKernelImage (ImageHandle);
> > +    //
> > +    // Fall through
> > +    //
> > +#endif
> > +  case EFI_INVALID_PARAMETER:
> > +    return QemuStartLegacyImage (ImageHandle);
> > +  default:
> > +    break;
> > +  }
> > +  return Status;
> > +}
> > +
> > +/**
> > +  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.
> > +
> > +**/
> > +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) {
> > +    return QemuUnloadLegacyImage (ImageHandle);
> > +  } else if (EFI_ERROR (Status)) {
>
> (14) Please don't follow a "return" statement with an "else"; just
> unnest the second branch.
>

Sure

> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (KernelLoadedImage->LoadOptions != NULL) {
> > +    FreePool (KernelLoadedImage->LoadOptions);
> > +    KernelLoadedImage->LoadOptions = NULL;
> > +  }
> > +  KernelLoadedImage->LoadOptionsSize = 0;
> > +
> > +  return gBS->UnloadImage (ImageHandle);
> > +}
>
> This function has to cope with three situations:
>
> (i) The image is a normal UEFI executable that was loaded by the
> LoadImage() branch of QemuLoadKernelImage(). In this case,
> OpenProtocol() is supposed to succeed, and so is gBS->UnloadImage(). OK.
>
> (ii) The image was legacy-loaded by QemuLoadKernelImage() -->
> QemuLoadLegacyImage(). Meaning, gBS->LoadImage() failed there, or it
> returned EFI_SECURITY_VIOLATION, and we unloaded the kernel image right
> there, before performing the legacy load. In either case, OpenProtocol()
> here will fail with EFI_UNSUPPORTED, and we defer to
> QemuUnloadLegacyImage(). OK.
>
> (iii) The third case is when *both* protocol sets are available on the
> handle, and we're being called from QemuStartKernelImage(). In this case
> we're in the process of "replacing" EFI_LOADED_IMAGE_PROTOCOL (and
> EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL) with our custom protocol. The
> function takes the same path as (i) in this case, I think, and that
> seems right. OK.
>
> (15) This function needs to document (in comments) all three cases
> above, please.
>

OK

>
> > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
> > new file mode 100644
> > index 000000000000..4208f5da3b31
> > --- /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.<BR>
> > +#
> > +#  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
> > +
> > +[Guids]
> > +  gQemuKernelLoaderFsMediaGuid
> > +  gX86QemuKernelLoadedImageGuid
> > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> > index 26f977bad795..e66af38f4290 100644
> > --- a/OvmfPkg/OvmfPkg.dec
> > +++ b/OvmfPkg/OvmfPkg.dec
> > @@ -93,6 +93,7 @@ [Guids]
> >    gEfiLegacyDevOrderVariableGuid      = {0xa56074db, 0x65fe, 0x45f7, {0xbd, 0x21, 0x2d, 0x2b, 0xdd, 0x8e, 0x96, 0x52}}
> >    gLinuxEfiInitrdMediaGuid            = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
> >    gQemuKernelLoaderFsMediaGuid        = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
> > +  gX86QemuKernelLoadedImageGuid       = {0xa3edc05d, 0xb618, 0x4ff6, {0x95, 0x52, 0x76, 0xd7, 0x88, 0x63, 0x43, 0xc8}}
> >
> >  [Protocols]
> >    gVirtioDeviceProtocolGuid           = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
> >
>
> Thanks
> Laszlo
>