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

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

On 04/11/19 13:47, Laszlo Ersek wrote:
> 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:

sigh, I meant "(1) through (3)".

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