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

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

On 04/11/19 14:23, Laszlo Ersek wrote:
> 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".

Sigh, I clearly need to cease reviewing this series for today. Here's
another remark:

(5) if you want to print the name of the module (such as
"AcpiPlatformDxe") in a log message, please use the "%a" format
specifier (for "ASCII string"), and use the following auto-generated
variable:

  gEfiCallerBaseName

It will contain the BASE_NAME string from the module INF file.

This variable is particularly useful in files that are built into
multiple modules:

- for example, a module directory may contain two INF files, with
  different BASE_NAMEs, and include a common .c file. If that .c file
  produces log messages, gEfiCallerBaseName is helpful.

- the same applies even more to log messages generated from libraries --
  you'll want to see the name of the driver or application that the
  library is linked into. In that case, gEfiCallerBaseName refers to the
  BASE_NAME of the driver/app INF file, and not the lib instance INF
  file.

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