Re: [edk2-devel] [PATCH v2 10/31] OvmfPkg/XenPlatformPei: Detect OVMF_INFO from hvmloader

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

On 04/09/19 13:08, Anthony PERARD wrote:
> This struct is only useful to retrieve the E820 table. The

(1) please replace "this struct" with the name of the struct.

> mXenHvmloaderInfo isn't used yet, but will be use in a further patch to
> retrieve the E820 table.
> 
> Also remove the unused pointer from the XenInfo HOB as that information
> is only useful in the XenPlatformPei.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/Include/Guid/XenInfo.h |  4 ----
>  OvmfPkg/PlatformPei/Xen.c      |  3 ---
>  OvmfPkg/XenPlatformPei/Xen.c   | 23 ++++++++++++++++++--
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/OvmfPkg/Include/Guid/XenInfo.h b/OvmfPkg/Include/Guid/XenInfo.h
> index d512b0b63f..5d4579f244 100644
> --- a/OvmfPkg/Include/Guid/XenInfo.h
> +++ b/OvmfPkg/Include/Guid/XenInfo.h
> @@ -24,10 +24,6 @@ typedef struct {
>    ///
>    VOID *HyperPages;
>    ///
> -  /// Location of the hvm_info page.
> -  ///
> -  VOID *HvmInfo;
> -  ///
>    /// Hypervisor major version.
>    ///
>    UINT16 VersionMajor;
> diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
> index ab38f97a67..75181be2fd 100644
> --- a/OvmfPkg/PlatformPei/Xen.c
> +++ b/OvmfPkg/PlatformPei/Xen.c
> @@ -104,9 +104,6 @@ XenConnect (
>    mXenInfo.VersionMajor = (UINT16)(XenVersion >> 16);
>    mXenInfo.VersionMinor = (UINT16)(XenVersion & 0xFFFF);
>  
> -  /* TBD: Locate hvm_info and reserve it away. */
> -  mXenInfo.HvmInfo = NULL;
> -
>    BuildGuidDataHob (
>      &gEfiXenInfoGuid,
>      &mXenInfo,
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index 7c82e5b69b..a866641b67 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -39,6 +39,12 @@ STATIC UINT32 mXenLeaf = 0;
>  
>  EFI_XEN_INFO mXenInfo;
>  
> +//
> +// Location of the firmware info struct setup by hvmloader.
> +// Only the E820 table is used by OVMF.
> +//
> +EFI_XEN_OVMF_INFO *mXenHvmloaderInfo;
> +
>  /**
>    Returns E820 map provided by Xen
>  
> @@ -84,6 +90,8 @@ XenConnect (
>    UINT32 TransferReg;
>    UINT32 TransferPages;
>    UINT32 XenVersion;
> +  EFI_XEN_OVMF_INFO *Info;
> +  CHAR8 Sig[sizeof (Info->Signature) + 1];
>  
>    AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL);
>    mXenInfo.HyperPages = AllocatePages (TransferPages);
> @@ -103,8 +111,19 @@ XenConnect (
>    mXenInfo.VersionMajor = (UINT16)(XenVersion >> 16);
>    mXenInfo.VersionMinor = (UINT16)(XenVersion & 0xFFFF);
>  
> -  /* TBD: Locate hvm_info and reserve it away. */
> -  mXenInfo.HvmInfo = NULL;
> +  //
> +  // Check if there are information left by hvmloader
> +  //
> +
> +  Info = (EFI_XEN_OVMF_INFO *)(UINTN) OVMF_INFO_PHYSICAL_ADDRESS;
> +  // Copy the signature, and make it null-terminated.

(2) Please use the following style:

  //
  // Copy the signature, and make it null-terminated.
  //

> +  AsciiStrnCpyS(Sig, sizeof (Sig),
> +                (CHAR8 *) &Info->Signature, sizeof (Info->Signature));

(3) This should be indented as:

  AsciiStrnCpyS (
    Sig,
    sizeof (Sig),
    (CHAR8 *) &Info->Signature,
    sizeof (Info->Signature)
    );

or:

  AsciiStrnCpyS (Sig, sizeof (Sig), (CHAR8 *) &Info->Signature,
    sizeof (Info->Signature));

(note the whitespace before the opening paren, after the function
designator)


I'm not checking the correctness of the parameters, for now.

With (1) and (3) addressed:

Acked-by: Laszlo Ersek <lersek@redhat.com>

If you can, please verify the comment style and the indentation style in
the other patches too.

Thanks
Laszlo

> +  if (AsciiStrCmp (Sig, "XenHVMOVMF") == 0) {
> +    mXenHvmloaderInfo = Info;
> +  } else {
> +    mXenHvmloaderInfo = NULL;
> +  }
>  
>    BuildGuidDataHob (
>      &gEfiXenInfoGuid,
>