| Message ID | 1610336718-7064-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OvmfPkg/XenPlatformPei: Grab 64-bit PCI MMIO hole size from OVMF info table | expand |
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__ */ >
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,
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
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__ */
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(-)