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

Patrick Rudolph posted 1 patch 2 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20220207100108.108899-1-patrick.rudolph@9elements.com
There is a newer version of this series
UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c | 137 +++-----------------
1 file changed, 18 insertions(+), 119 deletions(-)
[edk2-devel] [edk2-platform][PATCH] UefiPayloadPkg: Fix PciHostBridgeLib
Posted by Patrick Rudolph 2 years, 2 months ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86407): https://edk2.groups.io/g/devel/message/86407
Mute This Topic: https://groups.io/mt/88968466/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [edk2-platform][PATCH] UefiPayloadPkg: Fix PciHostBridgeLib
Posted by Ma, Maurice 2 years, 2 months ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86720): https://edk2.groups.io/g/devel/message/86720
Mute This Topic: https://groups.io/mt/88968466/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-