Re: [edk2-platform][PATCH] UefiPayloadPkg: Fix PciHostBridgeLib

["Ma, Maurice" <maurice.ma@intel.com>]

Reviewed-by: Maurice Ma <maurice.ma@intel.com>

> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Monday, February 7, 2022 2:01
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You,
> Benjamin <benjamin.you@intel.com>; Dong, Guo <guo.dong@intel.com>
> Subject: [edk2-platform][PATCH] UefiPayloadPkg: Fix PciHostBridgeLib
> 
> On modern platforms with TBT devices the coreboot resource allocator
> opens large PCI bridge MMIO windows above 4GiB to place hotplugable PCI
> BARs there as they won't fit below 4GiB. In addition modern GPGPU devices
> have very big PCI bars that doesn't fit below 4GiB.
> 
> The PciHostBridgeLib made lots of assumptions about the coreboot resource
> allocator that were not verified at runtime and are no longer true.
> 
> Remove all of the 'coreboot specific' code and implement the same logic as
> OvmfPkg's ScanForRootBridges.
> 
> Fixes assertion
> "ASSERT [PciHostBridgeDxe] Bridge->Mem.Limit < 0x0000000100000000ULL".
> 
> Tested with coreboot as bootloader on platforms that have PCI resources
> above 4GiB and on platforms that don't have resources above 4GiB.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c | 137 +++-
> ----------------
>  1 file changed, 18 insertions(+), 119 deletions(-)
> 
> diff --git a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> index bf2d10f4bf..8a890b6b53 100644
> --- a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> +++ b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> @@ -18,92 +18,6 @@
>  #include <Library/PciLib.h> #include "PciHostBridge.h" -/**-  Adjust the
> collected PCI resource.--  @param[in]  Io               IO aperture.--  @param[in]
> Mem              MMIO aperture.--  @param[in]  MemAbove4G       MMIO
> aperture above 4G.--  @param[in]  PMem             Prefetchable MMIO
> aperture.--  @param[in]  PMemAbove4G      Prefetchable MMIO aperture
> above 4G.-**/-VOID-AdjustRootBridgeResource (-  IN
> PCI_ROOT_BRIDGE_APERTURE  *Io,-  IN  PCI_ROOT_BRIDGE_APERTURE
> *Mem,-  IN  PCI_ROOT_BRIDGE_APERTURE  *MemAbove4G,-  IN
> PCI_ROOT_BRIDGE_APERTURE  *PMem,-  IN  PCI_ROOT_BRIDGE_APERTURE
> *PMemAbove4G-  )-{-  UINT64  Mask;--  //-  // For now try to downgrade
> everything into MEM32 since-  // - coreboot does not assign resource above
> 4GB-  // - coreboot might allocate interleaved MEM32 and PMEM32
> resource-  //   in some cases-  //-  if (PMem->Base < Mem->Base) {-    Mem-
> >Base = PMem->Base;-  }--  if (PMem->Limit > Mem->Limit) {-    Mem->Limit
> = PMem->Limit;-  }--  PMem->Base  = MAX_UINT64;-  PMem->Limit = 0;--  if
> (MemAbove4G->Base < 0x100000000ULL) {-    if (MemAbove4G->Base <
> Mem->Base) {-      Mem->Base = MemAbove4G->Base;-    }--    if
> (MemAbove4G->Limit > Mem->Limit) {-      Mem->Limit = MemAbove4G-
> >Limit;-    }--    MemAbove4G->Base  = MAX_UINT64;-    MemAbove4G->Limit
> = 0;-  }--  if (PMemAbove4G->Base < 0x100000000ULL) {-    if
> (PMemAbove4G->Base < Mem->Base) {-      Mem->Base = PMemAbove4G-
> >Base;-    }--    if (PMemAbove4G->Limit > Mem->Limit) {-      Mem->Limit =
> PMemAbove4G->Limit;-    }--    PMemAbove4G->Base  = MAX_UINT64;-
> PMemAbove4G->Limit = 0;-  }--  //-  // Align IO  resource at 4K  boundary-  //-
> Mask      = 0xFFFULL;-  Io->Limit = ((Io->Limit + Mask) & ~Mask) - 1;-  if (Io-
> >Base != MAX_UINT64) {-    Io->Base &= ~Mask;-  }--  //-  // Align MEM
> resource at 1MB boundary-  //-  Mask       = 0xFFFFFULL;-  Mem->Limit =
> ((Mem->Limit + Mask) & ~Mask) - 1;-  if (Mem->Base != MAX_UINT64) {-
> Mem->Base &= ~Mask;-  }-}- /**   Probe a bar is existed or not. @@ -114,28
> +28,24 @@ AdjustRootBridgeResource (
>  STATIC VOID PcatPciRootBridgeBarExisted (-  IN  UINT64  Address,+  IN
> UINTN   Address,   OUT UINT32  *OriginalValue,   OUT UINT32  *Value   ) {-
> UINTN  PciAddress;--  PciAddress = (UINTN)Address;-   //   // Preserve the
> original value   //-  *OriginalValue = PciRead32 (PciAddress);+  *OriginalValue
> = PciRead32 (Address);    //   // Disable timer interrupt while the BAR is
> probed   //   DisableInterrupts (); -  PciWrite32 (PciAddress, 0xFFFFFFFF);-
> *Value = PciRead32 (PciAddress);-  PciWrite32 (PciAddress, *OriginalValue);+
> PciWrite32 (Address, 0xFFFFFFFF);+  *Value = PciRead32 (Address);+
> PciWrite32 (Address, *OriginalValue);    //   // Enable interrupt@@ -179,9
> +89,7 @@ PcatPciRootBridgeParseBars (
>    IN UINTN                     BarOffsetEnd,   IN PCI_ROOT_BRIDGE_APERTURE  *Io,
> IN PCI_ROOT_BRIDGE_APERTURE  *Mem,-  IN
> PCI_ROOT_BRIDGE_APERTURE  *MemAbove4G,-  IN
> PCI_ROOT_BRIDGE_APERTURE  *PMem,-  IN PCI_ROOT_BRIDGE_APERTURE
> *PMemAbove4G+  IN PCI_ROOT_BRIDGE_APERTURE  *MemAbove4G    )
> {@@ -246,11 +154,7 @@ PcatPciRootBridgeParseBars (
>            //           Length = ((~Length) + 1) & 0xffffffff; -          if ((Value & BIT3) ==
> BIT3) {-            MemAperture = PMem;-          } else {-            MemAperture =
> Mem;-          }+          MemAperture = Mem;         } else {           //           //
> 64bit@@ -269,8 +173,8 @@ PcatPciRootBridgeParseBars (
>              Length = LShiftU64 (1ULL, LowBit);           } -          if ((Value & BIT3) ==
> BIT3) {-            MemAperture = PMemAbove4G;+          if (Base < BASE_4GB) {+
> MemAperture = Mem;           } else {             MemAperture =
> MemAbove4G;           }@@ -291,6 +195,8 @@ PcatPciRootBridgeParseBars (
>    } } +STATIC PCI_ROOT_BRIDGE_APERTURE  mNonExistAperture =
> { MAX_UINT64, 0 };+ /**   Scan for all root bridges in platform. @@ -317,8
> +223,6 @@ ScanForRootBridges (
>    PCI_ROOT_BRIDGE_APERTURE  Io;   PCI_ROOT_BRIDGE_APERTURE  Mem;
> PCI_ROOT_BRIDGE_APERTURE  MemAbove4G;-
> PCI_ROOT_BRIDGE_APERTURE  PMem;-  PCI_ROOT_BRIDGE_APERTURE
> PMemAbove4G;   PCI_ROOT_BRIDGE_APERTURE  *MemAperture;
> PCI_ROOT_BRIDGE           *RootBridges;   UINTN                     BarOffsetEnd;@@ -
> 338,9 +242,7 @@ ScanForRootBridges (
>      ZeroMem (&Io, sizeof (Io));     ZeroMem (&Mem, sizeof (Mem));
> ZeroMem (&MemAbove4G, sizeof (MemAbove4G));-    ZeroMem (&PMem,
> sizeof (PMem));-    ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));-
> Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base =
> PMemAbove4G.Base = MAX_UINT64;+    Io.Base = Mem.Base =
> MemAbove4G.Base = MAX_UINT64;     //     // Scan all the PCI devices on the
> primary bus of the PCI root bridge     //@@ -446,16 +348,17 @@
> ScanForRootBridges (
>             //           // Get the Prefetchable Memory range that the PPB is
> decoding+          // and merge it into Memory range           //           Value =
> Pci.Bridge.PrefetchableMemoryBase & 0x0f;           Base  =
> ((UINT32)Pci.Bridge.PrefetchableMemoryBase & 0xfff0) << 16;           Limit =
> (((UINT32)Pci.Bridge.PrefetchableMemoryLimit & 0xfff0)                    << 16) |
> 0xfffff;-          MemAperture = &PMem;+          MemAperture = &Mem;           if
> (Value == BIT0) {             Base       |= LShiftU64
> (Pci.Bridge.PrefetchableBaseUpper32, 32);             Limit      |= LShiftU64
> (Pci.Bridge.PrefetchableLimitUpper32, 32);-            MemAperture =
> &PMemAbove4G;+            MemAperture = &MemAbove4G;           }            if
> ((Base > 0) && (Base < Limit)) {@@ -513,9 +416,7 @@ ScanForRootBridges (
>            BarOffsetEnd,           &Io,           &Mem,-          &MemAbove4G,-
> &PMem,-          &PMemAbove4G+          &MemAbove4G           );          //@@ -
> 593,8 +494,6 @@ ScanForRootBridges (
>                        );       ASSERT (RootBridges != NULL); -
> AdjustRootBridgeResource (&Io, &Mem, &MemAbove4G, &PMem,
> &PMemAbove4G);-       InitRootBridge (         Attributes,         Attributes,@@ -
> 604,8 +503,8 @@ ScanForRootBridges (
>          &Io,         &Mem,         &MemAbove4G,-        &PMem,-
> &PMemAbove4G,+        &mNonExistAperture,+        &mNonExistAperture,
> &RootBridges[*NumberOfRootBridges]         );
> RootBridges[*NumberOfRootBridges].ResourceAssigned = TRUE;--
> 2.34.1