[PATCH] OvmfPkg/XenPlatformPei: Grab 64-bit PCI MMIO hole size from OVMF info table

[PATCH] OvmfPkg/XenPlatformPei: Grab 64-bit PCI MMIO hole size from OVMF info table

[Igor Druzhinin]

We faced a problem with passing through a PCI device with 64GB BAR to UEFI
guest. The BAR is expectedly programmed into 64-bit PCI aperture at
64G address which pushes physical address space to 37 bits. That is above
36-bit width that OVMF exposes currently to a guest without tweaking
PcdPciMmio64Size knob.

We can't really set this knob to a very high value and expect that to work
on all CPUs as the max physical address width varies singnificantly between
models. We also can't simply take CPU address width at runtime and use that
as we need to cover all of that with indentity pages for DXE phase.

The exact size of upper PCI MMIO hole is only known after hvmloader placed
all of the BARs and calculated the necessary aperture which is exposed
through ACPI. This information is not readily available to OVMF at PEI phase.
So let's expose it using the existing extensible binary interface between
OVMF and hvmloader preserving compatibility.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---

The change is backward compatible and has a companion change for hvmloader.

Are we still waiting to clean up Xen stuff from QEMU platform? Or I need
to duplicate my changed there (I hope not)?

---
 OvmfPkg/XenPlatformPei/MemDetect.c |  6 ++++-
 OvmfPkg/XenPlatformPei/Platform.h  |  8 +++++++
 OvmfPkg/XenPlatformPei/Xen.c       | 47 ++++++++++++++++++++++++++++++++++++++
 OvmfPkg/XenPlatformPei/Xen.h       | 12 +++++++++-
 4 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
index 1f81eee..4175a2f 100644
--- a/OvmfPkg/XenPlatformPei/MemDetect.c
+++ b/OvmfPkg/XenPlatformPei/MemDetect.c
@@ -227,6 +227,7 @@ GetFirstNonAddress (
   UINT64               FirstNonAddress;
   UINT64               Pci64Base, Pci64Size;
   RETURN_STATUS        PcdStatus;
+  EFI_STATUS           Status;
 
   FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
 
@@ -245,7 +246,10 @@ GetFirstNonAddress (
   // Otherwise, in order to calculate the highest address plus one, we must
   // consider the 64-bit PCI host aperture too. Fetch the default size.
   //
-  Pci64Size = PcdGet64 (PcdPciMmio64Size);
+  Status = XenGetPciMmioInfo (NULL, NULL, &Pci64Base, &Pci64Size);
+  if (EFI_ERROR (Status)) {
+    Pci64Size = PcdGet64 (PcdPciMmio64Size);
+  }
 
   if (Pci64Size == 0) {
     if (mBootMode != BOOT_ON_S3_RESUME) {
diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
index 7661f4a..6024cae 100644
--- a/OvmfPkg/XenPlatformPei/Platform.h
+++ b/OvmfPkg/XenPlatformPei/Platform.h
@@ -127,6 +127,14 @@ XenGetE820Map (
   UINT32 *Count
   );
 
+EFI_STATUS
+XenGetPciMmioInfo (
+  UINT64 *PciBase,
+  UINT64 *PciSize,
+  UINT64 *Pci64Base,
+  UINT64 *Pci64Size
+  );
+
 extern EFI_BOOT_MODE mBootMode;
 
 extern UINT8 mPhysMemAddressWidth;
diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index c41fecd..3c1970d 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -114,6 +114,53 @@ XenGetE820Map (
 }
 
 /**
+  Returns layouts of PCI MMIO holes provided by hvmloader
+
+  @param PciBase      Pointer to MMIO hole base address
+  @param PciSize      Pointer to MMIO hole size
+  @param Pci64Base    Pointer to upper MMIO hole base address
+  @param Pci64Size    Pointer to upper MMIO hole size
+
+  @return EFI_STATUS
+**/
+EFI_STATUS
+XenGetPciMmioInfo (
+  UINT64 *PciBase,
+  UINT64 *PciSize,
+  UINT64 *Pci64Base,
+  UINT64 *Pci64Size
+  )
+{
+  UINT64 *Tables;
+  EFI_XEN_OVMF_PCI_INFO *PciInfo;
+
+  if (mXenHvmloaderInfo == NULL) {
+    return EFI_NOT_FOUND;
+  }
+
+  if (mXenHvmloaderInfo->TablesCount < OVMF_INFO_PCI_TABLE + 1) {
+    return EFI_UNSUPPORTED;
+  }
+
+  ASSERT (mXenHvmloaderInfo->Tables &&
+          mXenHvmloaderInfo->Tables < MAX_ADDRESS);
+  Tables = (UINT64 *) mXenHvmloaderInfo->Tables;
+  PciInfo = (EFI_XEN_OVMF_PCI_INFO *) Tables[OVMF_INFO_PCI_TABLE];
+
+  ASSERT (PciInfo && (EFI_PHYSICAL_ADDRESS) PciInfo < MAX_ADDRESS);
+  if (PciBase && PciSize) {
+    *PciBase = (UINT64) PciInfo->LowStart;
+    *PciSize = (UINT64) (PciInfo->LowEnd - PciInfo->LowStart);
+  }
+  if (Pci64Base && Pci64Size) {
+    *Pci64Base = (UINT64) PciInfo->HiStart;
+    *Pci64Size = (UINT64) (PciInfo->HiEnd - PciInfo->HiStart);
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Connects to the Hypervisor.
 
   @return EFI_STATUS
diff --git a/OvmfPkg/XenPlatformPei/Xen.h b/OvmfPkg/XenPlatformPei/Xen.h
index 2605481..c6e5fbb 100644
--- a/OvmfPkg/XenPlatformPei/Xen.h
+++ b/OvmfPkg/XenPlatformPei/Xen.h
@@ -15,7 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 // Physical address of OVMF info
 #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
 
-// This structure must match the definition on Xen side
+// These structures must match the definition on Xen side
 #pragma pack(1)
 typedef struct {
   CHAR8 Signature[14]; // XenHVMOVMF\0
@@ -34,6 +34,16 @@ typedef struct {
   EFI_PHYSICAL_ADDRESS E820;
   UINT32 E820EntriesCount;
 } EFI_XEN_OVMF_INFO;
+
+// This extra table gives layout of PCI apertures in a Xen guest
+#define OVMF_INFO_PCI_TABLE 0
+
+typedef struct {
+  EFI_PHYSICAL_ADDRESS LowStart;
+  EFI_PHYSICAL_ADDRESS LowEnd;
+  EFI_PHYSICAL_ADDRESS HiStart;
+  EFI_PHYSICAL_ADDRESS HiEnd;
+} EFI_XEN_OVMF_PCI_INFO;
 #pragma pack()
 
 #endif /* __XEN_H__ */
-- 
2.7.4

Re: [PATCH] OvmfPkg/XenPlatformPei: Grab 64-bit PCI MMIO hole size from OVMF info table

[Laszlo Ersek]

On 01/11/21 04:45, Igor Druzhinin wrote:
> We faced a problem with passing through a PCI device with 64GB BAR to UEFI
> guest. The BAR is expectedly programmed into 64-bit PCI aperture at
> 64G address which pushes physical address space to 37 bits. That is above
> 36-bit width that OVMF exposes currently to a guest without tweaking
> PcdPciMmio64Size knob.
> 
> We can't really set this knob to a very high value and expect that to work
> on all CPUs as the max physical address width varies singnificantly between
> models. We also can't simply take CPU address width at runtime and use that
> as we need to cover all of that with indentity pages for DXE phase.
> 
> The exact size of upper PCI MMIO hole is only known after hvmloader placed
> all of the BARs and calculated the necessary aperture which is exposed
> through ACPI. This information is not readily available to OVMF at PEI phase.
> So let's expose it using the existing extensible binary interface between
> OVMF and hvmloader preserving compatibility.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> 
> The change is backward compatible and has a companion change for hvmloader.
> 
> Are we still waiting to clean up Xen stuff from QEMU platform?

Yes, this BZ is still open:

  https://bugzilla.tianocore.org/show_bug.cgi?id=2122

That said...

> Or I need to duplicate my changed there (I hope not)?

... I hope not, as well.

(The automated solution of this issue remains unsolved for QEMU; we
sometimes revise it, but it's a very tough nut to crack. Users have been
able to tweak the aperture size with the experimental (no support
promised) fw_cfg file "opt/ovmf/X-PciMmio64Mb". But Xen has no fw_cfg,
so this patch certainly looks justified.)

Some style comments:

> 
> ---
>  OvmfPkg/XenPlatformPei/MemDetect.c |  6 ++++-
>  OvmfPkg/XenPlatformPei/Platform.h  |  8 +++++++
>  OvmfPkg/XenPlatformPei/Xen.c       | 47 ++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenPlatformPei/Xen.h       | 12 +++++++++-
>  4 files changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
> index 1f81eee..4175a2f 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -227,6 +227,7 @@ GetFirstNonAddress (
>    UINT64               FirstNonAddress;
>    UINT64               Pci64Base, Pci64Size;
>    RETURN_STATUS        PcdStatus;
> +  EFI_STATUS           Status;
>  
>    FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>  
> @@ -245,7 +246,10 @@ GetFirstNonAddress (
>    // Otherwise, in order to calculate the highest address plus one, we must
>    // consider the 64-bit PCI host aperture too. Fetch the default size.
>    //
> -  Pci64Size = PcdGet64 (PcdPciMmio64Size);
> +  Status = XenGetPciMmioInfo (NULL, NULL, &Pci64Base, &Pci64Size);
> +  if (EFI_ERROR (Status)) {
> +    Pci64Size = PcdGet64 (PcdPciMmio64Size);
> +  }
>  
>    if (Pci64Size == 0) {
>      if (mBootMode != BOOT_ON_S3_RESUME) {
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 7661f4a..6024cae 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -127,6 +127,14 @@ XenGetE820Map (
>    UINT32 *Count
>    );
>  
> +EFI_STATUS
> +XenGetPciMmioInfo (
> +  UINT64 *PciBase,
> +  UINT64 *PciSize,
> +  UINT64 *Pci64Base,
> +  UINT64 *Pci64Size
> +  );
> +
>  extern EFI_BOOT_MODE mBootMode;
>  
>  extern UINT8 mPhysMemAddressWidth;
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index c41fecd..3c1970d 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -114,6 +114,53 @@ XenGetE820Map (
>  }
>  
>  /**
> +  Returns layouts of PCI MMIO holes provided by hvmloader
> +
> +  @param PciBase      Pointer to MMIO hole base address
> +  @param PciSize      Pointer to MMIO hole size
> +  @param Pci64Base    Pointer to upper MMIO hole base address
> +  @param Pci64Size    Pointer to upper MMIO hole size
> +
> +  @return EFI_STATUS
> +**/
> +EFI_STATUS
> +XenGetPciMmioInfo (
> +  UINT64 *PciBase,
> +  UINT64 *PciSize,
> +  UINT64 *Pci64Base,
> +  UINT64 *Pci64Size
> +  )
> +{
> +  UINT64 *Tables;
> +  EFI_XEN_OVMF_PCI_INFO *PciInfo;
> +
> +  if (mXenHvmloaderInfo == NULL) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  if (mXenHvmloaderInfo->TablesCount < OVMF_INFO_PCI_TABLE + 1) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  ASSERT (mXenHvmloaderInfo->Tables &&

(1) Please spell out:

  mXenHvmloaderInfo->Tables != 0

> +          mXenHvmloaderInfo->Tables < MAX_ADDRESS);
> +  Tables = (UINT64 *) mXenHvmloaderInfo->Tables;
> +  PciInfo = (EFI_XEN_OVMF_PCI_INFO *) Tables[OVMF_INFO_PCI_TABLE];
> +
> +  ASSERT (PciInfo && (EFI_PHYSICAL_ADDRESS) PciInfo < MAX_ADDRESS);

(2) Please spell out

  PciInfo != NULL

> +  if (PciBase && PciSize) {

(3) Same here -- please use explicit comparisons.

> +    *PciBase = (UINT64) PciInfo->LowStart;
> +    *PciSize = (UINT64) (PciInfo->LowEnd - PciInfo->LowStart);
> +  }
> +  if (Pci64Base && Pci64Size) {

(4) Ditto

I'll wait for feedback from the OvmfPkg Xen reviewers; from my side,
with the style warts fixed:

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

Thanks
Laszlo

> +    *Pci64Base = (UINT64) PciInfo->HiStart;
> +    *Pci64Size = (UINT64) (PciInfo->HiEnd - PciInfo->HiStart);
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Connects to the Hypervisor.
>  
>    @return EFI_STATUS
> diff --git a/OvmfPkg/XenPlatformPei/Xen.h b/OvmfPkg/XenPlatformPei/Xen.h
> index 2605481..c6e5fbb 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.h
> +++ b/OvmfPkg/XenPlatformPei/Xen.h
> @@ -15,7 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  // Physical address of OVMF info
>  #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
>  
> -// This structure must match the definition on Xen side
> +// These structures must match the definition on Xen side
>  #pragma pack(1)
>  typedef struct {
>    CHAR8 Signature[14]; // XenHVMOVMF\0
> @@ -34,6 +34,16 @@ typedef struct {
>    EFI_PHYSICAL_ADDRESS E820;
>    UINT32 E820EntriesCount;
>  } EFI_XEN_OVMF_INFO;
> +
> +// This extra table gives layout of PCI apertures in a Xen guest
> +#define OVMF_INFO_PCI_TABLE 0
> +
> +typedef struct {
> +  EFI_PHYSICAL_ADDRESS LowStart;
> +  EFI_PHYSICAL_ADDRESS LowEnd;
> +  EFI_PHYSICAL_ADDRESS HiStart;
> +  EFI_PHYSICAL_ADDRESS HiEnd;
> +} EFI_XEN_OVMF_PCI_INFO;
>  #pragma pack()
>  
>  #endif /* __XEN_H__ */
> 

Re: [PATCH] OvmfPkg/XenPlatformPei: Grab 64-bit PCI MMIO hole size from OVMF info table

[Anthony PERARD]

On Mon, Jan 11, 2021 at 03:45:18AM +0000, Igor Druzhinin wrote:
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
> index 1f81eee..4175a2f 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -227,6 +227,7 @@ GetFirstNonAddress (
>    UINT64               FirstNonAddress;
>    UINT64               Pci64Base, Pci64Size;
>    RETURN_STATUS        PcdStatus;
> +  EFI_STATUS           Status;
>  
>    FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>  
> @@ -245,7 +246,10 @@ GetFirstNonAddress (
>    // Otherwise, in order to calculate the highest address plus one, we must
>    // consider the 64-bit PCI host aperture too. Fetch the default size.
>    //
> -  Pci64Size = PcdGet64 (PcdPciMmio64Size);
> +  Status = XenGetPciMmioInfo (NULL, NULL, &Pci64Base, &Pci64Size);

Pci64Base is overridden later (25 line bellow) by the value from
FirstNonAddress, shouldn't this be avoided?
    Pci64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);

> diff --git a/OvmfPkg/XenPlatformPei/Xen.h b/OvmfPkg/XenPlatformPei/Xen.h
> index 2605481..c6e5fbb 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.h
> +++ b/OvmfPkg/XenPlatformPei/Xen.h
> @@ -34,6 +34,16 @@ typedef struct {
>    EFI_PHYSICAL_ADDRESS E820;
>    UINT32 E820EntriesCount;
>  } EFI_XEN_OVMF_INFO;
> +
> +// This extra table gives layout of PCI apertures in a Xen guest
> +#define OVMF_INFO_PCI_TABLE 0
> +
> +typedef struct {
> +  EFI_PHYSICAL_ADDRESS LowStart;
> +  EFI_PHYSICAL_ADDRESS LowEnd;
> +  EFI_PHYSICAL_ADDRESS HiStart;
> +  EFI_PHYSICAL_ADDRESS HiEnd;

In the hvmloader patch, these are uint64. It doesn't seems like a good
idea to use the type EFI_PHYSICAL_ADDRESS here. Could you change to
UINT64 here?

(even if EFI_PHYSICAL_ADDRESS seems to always be UINT64, in the source
code.)

> +} EFI_XEN_OVMF_PCI_INFO;
>  #pragma pack()
>  
>  #endif /* __XEN_H__ */

Thanks,

-- 
Anthony PERARD

Re: [PATCH] OvmfPkg/XenPlatformPei: Grab 64-bit PCI MMIO hole size from OVMF info table

[igor.druzhinin]

On 19/01/2021 13:20, Anthony PERARD wrote:
> On Mon, Jan 11, 2021 at 03:45:18AM +0000, Igor Druzhinin wrote:
>> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
>> index 1f81eee..4175a2f 100644
>> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
>> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
>> @@ -227,6 +227,7 @@ GetFirstNonAddress (
>>    UINT64               FirstNonAddress;
>>    UINT64               Pci64Base, Pci64Size;
>>    RETURN_STATUS        PcdStatus;
>> +  EFI_STATUS           Status;
>>  
>>    FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>>  
>> @@ -245,7 +246,10 @@ GetFirstNonAddress (
>>    // Otherwise, in order to calculate the highest address plus one, we must
>>    // consider the 64-bit PCI host aperture too. Fetch the default size.
>>    //
>> -  Pci64Size = PcdGet64 (PcdPciMmio64Size);
>> +  Status = XenGetPciMmioInfo (NULL, NULL, &Pci64Base, &Pci64Size);
> 
> Pci64Base is overridden later (25 line bellow) by the value from
> FirstNonAddress, shouldn't this be avoided?
>     Pci64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);
> 
>> diff --git a/OvmfPkg/XenPlatformPei/Xen.h b/OvmfPkg/XenPlatformPei/Xen.h
>> index 2605481..c6e5fbb 100644
>> --- a/OvmfPkg/XenPlatformPei/Xen.h
>> +++ b/OvmfPkg/XenPlatformPei/Xen.h
>> @@ -34,6 +34,16 @@ typedef struct {
>>    EFI_PHYSICAL_ADDRESS E820;
>>    UINT32 E820EntriesCount;
>>  } EFI_XEN_OVMF_INFO;
>> +
>> +// This extra table gives layout of PCI apertures in a Xen guest
>> +#define OVMF_INFO_PCI_TABLE 0
>> +
>> +typedef struct {
>> +  EFI_PHYSICAL_ADDRESS LowStart;
>> +  EFI_PHYSICAL_ADDRESS LowEnd;
>> +  EFI_PHYSICAL_ADDRESS HiStart;
>> +  EFI_PHYSICAL_ADDRESS HiEnd;
> 
> In the hvmloader patch, these are uint64. It doesn't seems like a good
> idea to use the type EFI_PHYSICAL_ADDRESS here. Could you change to
> UINT64 here?
> 
> (even if EFI_PHYSICAL_ADDRESS seems to always be UINT64, in the source
> code.)
> 

Anthony, this patch is obsolete now - see discussion between Jan and Laszlo.
The new patch is:
https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00789.html

Igor