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

["Laszlo Ersek" <lersek@redhat.com>]

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.)

> +
> +  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