Re: [edk2-devel] [PATCH v2 14/31] OvmfPkg/AcpiPlatformDxe: Use PVH RSDP if exist

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

On 04/11/19 14:20, Laszlo Ersek wrote:
> On 04/09/19 13:08, Anthony PERARD wrote:
>> If the firmware have been started via the PVH entry point, a RSDP
>> pointer would have been provided. Use it.
>>
>> Also, use XenDetect() from the new XenPlatformLib.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  OvmfPkg/OvmfPkgIa32.dsc                     |  1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                  |  1 +
>>  OvmfPkg/OvmfPkgX64.dsc                      |  1 +
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf |  2 +-
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h      |  6 +--
>>  OvmfPkg/AcpiPlatformDxe/Xen.c               | 41 ++++++++------------
>>  6 files changed, 22 insertions(+), 30 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index f55ab5a3d2..cab9764cab 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -205,6 +205,7 @@ [LibraryClasses]
>>    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
>>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>> +  XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>>  
>>  !if $(TPM2_ENABLE) == TRUE
>>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 5c9bdf034e..a156358690 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -210,6 +210,7 @@ [LibraryClasses]
>>    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
>>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>> +  XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>>  
>>  !if $(TPM2_ENABLE) == TRUE
>>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 2943e9e8af..9d8dc442b4 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -210,6 +210,7 @@ [LibraryClasses]
>>    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
>>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>> +  XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>>  
>>  !if $(TPM2_ENABLE) == TRUE
>>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
>> index 8440e7b343..ca54a3bd9e 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
>> @@ -51,13 +51,13 @@ [LibraryClasses]
>>    DebugLib
>>    UefiBootServicesTableLib
>>    UefiDriverEntryPoint
>> -  HobLib
>>    QemuFwCfgLib
>>    QemuFwCfgS3Lib
>>    MemoryAllocationLib
>>    BaseLib
>>    DxeServicesTableLib
>>    OrderedCollectionLib
>> +  XenPlatformLib
>>  
>>  [Protocols]
>>    gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> index 83b981ee00..85f37128dd 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> @@ -25,6 +25,7 @@
>>  #include <Library/UefiBootServicesTableLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/PcdLib.h>
>> +#include <Library/XenPlatformLib.h>
>>  
>>  #include <IndustryStandard/Acpi.h>
>>  
>> @@ -58,11 +59,6 @@ QemuInstallAcpiTable (
>>    OUT  UINTN                         *TableKey
>>    );
>>  
>> -BOOLEAN
>> -XenDetected (
>> -  VOID
>> -  );
>> -
>>  EFI_STATUS
>>  EFIAPI
>>  InstallXenTables (
>> diff --git a/OvmfPkg/AcpiPlatformDxe/Xen.c b/OvmfPkg/AcpiPlatformDxe/Xen.c
>> index 618ac58b42..7e9a7797d7 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/Xen.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/Xen.c
>> @@ -15,8 +15,6 @@
>>  **/ 
>>  
>>  #include "AcpiPlatform.h"
>> -#include <Library/HobLib.h>
>> -#include <Guid/XenInfo.h>
>>  #include <Library/BaseLib.h>
>>  
>>  #define XEN_ACPI_PHYSICAL_ADDRESS         0x000EA020
>> @@ -24,28 +22,6 @@
>>  
>>  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *XenAcpiRsdpStructurePtr = NULL;
>>  
>> -/**
>> -  This function detects if OVMF is running on Xen.
>> -
>> -**/
>> -BOOLEAN
>> -XenDetected (
>> -  VOID
>> -  )
>> -{
>> -  EFI_HOB_GUID_TYPE         *GuidHob;
>> -
>> -  //
>> -  // See if a XenInfo HOB is available
>> -  //
>> -  GuidHob = GetFirstGuidHob (&gEfiXenInfoGuid);
>> -  if (GuidHob == NULL) {
>> -    return FALSE;
>> -  }
>> -
>> -  return TRUE;
>> -}
>> -
>>  /**
>>    Get the address of Xen ACPI Root System Description Pointer (RSDP)
>>    structure.
>> @@ -66,10 +42,27 @@ GetXenAcpiRsdp (
>>    EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER   *RsdpStructurePtr;
>>    UINT8                                          *XenAcpiPtr;
>>    UINT8                                          Sum;
>> +  EFI_XEN_INFO                                   *XenInfo;
>>  
>>    //
>>    // Detect the RSDP structure
>>    //
>> +
>> +  //
>> +  // First look for PVH one
>> +  //
>> +  XenInfo = XenGetInfoHOB ();
>> +  ASSERT (XenInfo != NULL);
>> +  if (XenInfo->RsdpPvh != NULL) {
>> +    DEBUG ((DEBUG_INFO, "AcpiPlatformDxe: Use ACPI RSDP table at 0x%08x\n",
>> +            XenInfo->RsdpPvh));

(4) sorry, missed this one: please

- either print pointers with %p,

- or convert them with

  (UINT64)(UINTN)XenInfo->RsdpPvh

and log them with "0x%016Lx".

Thanks,
Laszlo

>> +    *RsdpPtr = XenInfo->RsdpPvh;
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  //
>> +  // Otherwise, look for the HVM one
>> +  //
>>    for (XenAcpiPtr = (UINT8*)(UINTN) XEN_ACPI_PHYSICAL_ADDRESS;
>>         XenAcpiPtr < (UINT8*)(UINTN) XEN_BIOS_PHYSICAL_END;
>>         XenAcpiPtr += 0x10) {
>>
> 
> Can you please split this patch in two:
> 
> (1) the first patch should only rebase AcpiPlatformDxe to the new
> library, without functional changes.
> 
> (2) the second patch should add the new feature, of looking for the PVH
> RSDP.
> 
> (3) In the 2nd patch, pls mention Xen in the subject line, such as:
> 
>   OvmfPkg/AcpiPlatformDxe: Use Xen PVH RSDP if it exist
> 
> Thanks!
> Laszlo
>