Re: [edk2-devel] [PATCH v2 04/14] OvmfPkg: provide a generic implementation of QemuLoadImageLib

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

On Thu, 5 Mar 2020 at 12:29, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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.
>

Ah yes - that param went out of date a while ago :-)