Re: [edk2-devel] [PATCH v2 09/14] OvmfPkg: create protocol and GUID header for legacy loaded images

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

On Thu, 5 Mar 2020 at 11:31, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 03/04/20 10:52, Ard Biesheuvel wrote:
> > In preparation of moving the legacy x86 loading to an implementation
> > of the QEMU load image library class, introduce a protocol header
> > and GUID that we will use to identify legacy loaded images in the
> > protocol database.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  OvmfPkg/Include/Protocol/X86QemuKernelLoadedImage.h | 19 +++++++++++++++++++
> >  OvmfPkg/OvmfPkg.dec                                 |  1 +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/OvmfPkg/Include/Protocol/X86QemuKernelLoadedImage.h b/OvmfPkg/Include/Protocol/X86QemuKernelLoadedImage.h
> > new file mode 100644
> > index 000000000000..7e1bebaa6a07
> > --- /dev/null
> > +++ b/OvmfPkg/Include/Protocol/X86QemuKernelLoadedImage.h
> > @@ -0,0 +1,19 @@
> > +/** @file
> > +  Protocol/GUID definition to describe a kernel image loaded by the legacy X86
> > +  loader from the file specified on the QEMU command line via the -kernel
> > +  option.
>
> (1) Please add a comment that the interface structure associated with
> this protocol GUID is subject to change, and should not be used outside
> of the EDK II tree.
>
> (I'm requesting this comment regardless of point (5) below.)
>

Now that we're bikeshedding: :-)

Given the internal nature of this protocol, perhaps the name should
reflect it? And if we're changing it, perhaps make it more precise?

It is internal to OVMF so

gOvmfLoadedX86LinuxKernelProtocolGuid

and for the type

OVMF_LOADED_X86_LINUX_KERNEL

(given that the only thing it can represent is a loaded x86 Linux
kernel, but that is not specific to QEMU in principle)

This ignores the initrd aspect as well as the command line, but EFI's
loaded image subsumes the command line as well, so I think that is
fine.




> > +
> > +  Copyright (c) 2020, Arm, Ltd. All rights reserved.<BR>
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#ifndef X86_QEMU_KERNEL_LOADED_IMAGE_GUID_H__
> > +#define X86_QEMU_KERNEL_LOADED_IMAGE_GUID_H__
>
> (2) Please drop "_GUID" from the guard macro's name.
>
> > +
> > +#define X86_QEMU_KERNEL_LOADED_IMAGE_GUID \
> > +  {0xa3edc05d, 0xb618, 0x4ff6, {0x95, 0x52, 0x76, 0xd7, 0x88, 0x63, 0x43, 0xc8}}
>
> (3) Please replace "_GUID" with "_PROTOCOL_GUID" in the initializer
> macro's name.
>
> > +
> > +extern EFI_GUID gX86QemuKernelLoadedImageGuid;
>
> (4) Please replace "Guid" with "ProtocolGuid" in the variable name.
>
> > +
> > +#endif
>
> (5) Please consider moving the QEMU_LEGACY_LOADED_IMAGE typedef here,
> from the next patch. It's not a technical necessity at all, but edk2
> protocol headers always include the interface typedef too. And due to
> (1) above, I think it's safe too.
>
> If you don't like the idea, I won't insist. :)
>
> > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> > index 055caaa43041..06ffd4198d44 100644
> > --- a/OvmfPkg/OvmfPkg.dec
> > +++ b/OvmfPkg/OvmfPkg.dec
> > @@ -112,6 +112,7 @@ [Protocols]
> >    gEfiLegacyBiosPlatformProtocolGuid  = {0x783658a3, 0x4172, 0x4421, {0xa2, 0x99, 0xe0, 0x09, 0x07, 0x9c, 0x0c, 0xb4}}
> >    gEfiLegacyInterruptProtocolGuid     = {0x31ce593d, 0x108a, 0x485d, {0xad, 0xb2, 0x78, 0xf2, 0x1f, 0x29, 0x66, 0xbe}}
> >    gEfiVgaMiniPortProtocolGuid         = {0xc7735a2f, 0x88f5, 0x4882, {0xae, 0x63, 0xfa, 0xac, 0x8c, 0x8b, 0x86, 0xb3}}
> > +  gX86QemuKernelLoadedImageGuid       = {0xa3edc05d, 0xb618, 0x4ff6, {0x95, 0x52, 0x76, 0xd7, 0x88, 0x63, 0x43, 0xc8}}
>
> (6) Same as (4).
>
> >
> >  [PcdsFixedAtBuild]
> >    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0
> >
>
> With (1)-(4) and (6) fixed, and with (5) optionally implemented:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks!
> Laszlo
>
>
> 
>