[PATCH v5 0/6] OvmfPkg/Microvm/pcie: add pcie support

[PATCH v5 0/6] OvmfPkg/Microvm/pcie: add pcie support

[Gerd Hoffmann]

Needs two little tweaks in PCI code because microvm supports mmio only.
Other than that just wire up the existing code (the PCIe host adapter
used by microvm is the same (virtual) hardware used by the arm/aarch64
virtual machines).

v5:
 - codestyle (uncrustify) fix.

v4:
 - update PciHostBridge check (Abner Chang).

v3:
 - rebase to latest master, adapt to PlatformInitLib.
 - rework PhysMemAddressWidth handling for microvm.

v2:
 - rebase to latest master
 - pick up review tags

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

Gerd Hoffmann (6):
  MdeModulePkg/PciHostBridge: io range is not mandatory
  OvmfPkg/FdtPciHostBridgeLib: io range is not mandatory
  OvmfPkg/Platform: unfix PcdPciExpressBaseAddress
  OvmfPkg/Microvm/pcie: no vbeshim please
  OvmfPkg/Microvm/pcie: mPhysMemAddressWidth tweak
  OvmfPkg/Microvm/pcie: add pcie support

 OvmfPkg/Microvm/MicrovmX64.dsc                | 40 ++++++++++-------
 .../PlatformInitLib/PlatformInitLib.inf       |  4 +-
 OvmfPkg/PlatformPei/PlatformPei.inf           |  2 +-
 .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c  |  3 ++
 .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 45 ++++++++++---------
 OvmfPkg/Library/PlatformInitLib/MemDetect.c   | 45 ++++++++++++++++++-
 OvmfPkg/Library/PlatformInitLib/Platform.c    |  4 +-
 OvmfPkg/PlatformPei/Platform.c                |  2 +-
 OvmfPkg/QemuVideoDxe/VbeShim.c                |  2 +
 OvmfPkg/Microvm/README                        |  2 +-
 10 files changed, 104 insertions(+), 45 deletions(-)

-- 
2.35.1

[PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

io range is not mandatory according to pcie spec,
so allow bridge configurations without io address
space assigned.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index b20bcd310ad5..712662707931 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1085,6 +1085,9 @@ NotifyPhase (
               RootBridge->ResAllocNode[Index].Base   = BaseAddress;
               RootBridge->ResAllocNode[Index].Status = ResAllocated;
               DEBUG ((DEBUG_INFO, "Success\n"));
+            } else if ((Index == TypeIo) && (RootBridge->Io.Base == MAX_UINT64)) {
+              /* optional on PCIe */
+              DEBUG ((DEBUG_INFO, "PCI Root Bridge does not provide IO Resources.\n"));
             } else {
               ReturnStatus = EFI_OUT_OF_RESOURCES;
               DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
-- 
2.35.1

[PATCH v5 2/6] OvmfPkg/FdtPciHostBridgeLib: io range is not mandatory

[Gerd Hoffmann]

io range is not mandatory according to pcie spec,
so allow host bridges without io address space.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 45 ++++++++++---------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 98828e0b262b..823ea47c80a3 100644
--- a/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -292,13 +292,8 @@ ProcessPciHost (
     }
   }
 
-  if ((*IoSize == 0) || (*Mmio32Size == 0)) {
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: %a space empty\n",
-      __FUNCTION__,
-      (*IoSize == 0) ? "IO" : "MMIO32"
-      ));
+  if (*Mmio32Size == 0) {
+    DEBUG ((DEBUG_ERROR, "%a: MMIO32 space empty\n", __FUNCTION__));
     return EFI_PROTOCOL_ERROR;
   }
 
@@ -333,13 +328,15 @@ ProcessPciHost (
     return Status;
   }
 
-  //
-  // Map the MMIO window that provides I/O access - the PCI host bridge code
-  // is not aware of this translation and so it will only map the I/O view
-  // in the GCD I/O map.
-  //
-  Status = MapGcdMmioSpace (*IoBase + IoTranslation, *IoSize);
-  ASSERT_EFI_ERROR (Status);
+  if (*IoSize) {
+    //
+    // Map the MMIO window that provides I/O access - the PCI host bridge code
+    // is not aware of this translation and so it will only map the I/O view
+    // in the GCD I/O map.
+    //
+    Status = MapGcdMmioSpace (*IoBase + IoTranslation, *IoSize);
+    ASSERT_EFI_ERROR (Status);
+  }
 
   return Status;
 }
@@ -413,17 +410,21 @@ PciHostBridgeGetRootBridges (
 
   AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
 
-  Io.Base   = IoBase;
-  Io.Limit  = IoBase + IoSize - 1;
+  if (IoSize) {
+    Io.Base  = IoBase;
+    Io.Limit = IoBase + IoSize - 1;
+  } else {
+    Io.Base  = MAX_UINT64;
+    Io.Limit = 0;
+  }
+
   Mem.Base  = Mmio32Base;
   Mem.Limit = Mmio32Base + Mmio32Size - 1;
 
-  if (sizeof (UINTN) == sizeof (UINT64)) {
-    MemAbove4G.Base  = Mmio64Base;
-    MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
-    if (Mmio64Size > 0) {
-      AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
-    }
+  if ((sizeof (UINTN) == sizeof (UINT64)) && Mmio64Size) {
+    MemAbove4G.Base       = Mmio64Base;
+    MemAbove4G.Limit      = Mmio64Base + Mmio64Size - 1;
+    AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
   } else {
     //
     // UEFI mandates a 1:1 virtual-to-physical mapping, so on a 32-bit
-- 
2.35.1

[PATCH v5 3/6] OvmfPkg/Platform: unfix PcdPciExpressBaseAddress

[Gerd Hoffmann]

Will be set by FdtPciHostBridgeLib, so it can't be an fixed when we
want use that library.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 4 +++-
 OvmfPkg/PlatformPei/PlatformPei.inf                 | 2 +-
 OvmfPkg/Library/PlatformInitLib/MemDetect.c         | 4 ++--
 OvmfPkg/Library/PlatformInitLib/Platform.c          | 4 ++--
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
index d0fab5cc1f4f..d2a0bec43452 100644
--- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
+++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
@@ -54,8 +54,10 @@ [LibraryClasses]
 [LibraryClasses.X64]
   TdxLib
 
-[FixedPcd]
+[Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+
+[FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 00372fa0ebb5..3cd83e6ec3e5 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -95,6 +95,7 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtr
   gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtrSize
   gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
@@ -118,7 +119,6 @@ [Pcd]
 [FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 4c1dedf863c3..83a7b6726bb7 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -61,8 +61,8 @@ PlatformQemuUc32BaseInitialization (
     // [PcdPciExpressBaseAddress, 4GB) range require a very small number of
     // variable MTRRs (preferably 1 or 2).
     //
-    ASSERT (FixedPcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
-    PlatformInfoHob->Uc32Base = (UINT32)FixedPcdGet64 (PcdPciExpressBaseAddress);
+    ASSERT (PcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
+    PlatformInfoHob->Uc32Base = (UINT32)PcdGet64 (PcdPciExpressBaseAddress);
     return;
   }
 
diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index 101074f6100d..60a30a01f3b5 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -154,7 +154,7 @@ PlatformMemMapInitialization (
     // The MMCONFIG area is expected to fall between the top of low RAM and
     // the base of the 32-bit PCI host aperture.
     //
-    PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
+    PciExBarBase = PcdGet64 (PcdPciExpressBaseAddress);
     ASSERT (TopOfLowRam <= PciExBarBase);
     ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
     PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
@@ -278,7 +278,7 @@ PciExBarInitialization (
   // determined in AddressWidthInitialization(), i.e., 36 bits, will suffice
   // for DXE's page tables to cover the MMCONFIG area.
   //
-  PciExBarBase.Uint64 = FixedPcdGet64 (PcdPciExpressBaseAddress);
+  PciExBarBase.Uint64 = PcdGet64 (PcdPciExpressBaseAddress);
   ASSERT ((PciExBarBase.Uint32[1] & MCH_PCIEXBAR_HIGHMASK) == 0);
   ASSERT ((PciExBarBase.Uint32[0] & MCH_PCIEXBAR_LOWMASK) == 0);
 
-- 
2.35.1

[PATCH v5 4/6] OvmfPkg/Microvm/pcie: no vbeshim please

[Gerd Hoffmann]

Those old windows versions which need the vbeshim hack
will not run on microvm anyway.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/QemuVideoDxe/VbeShim.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
index 8faa146b6cce..2a048211a823 100644
--- a/OvmfPkg/QemuVideoDxe/VbeShim.c
+++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
@@ -156,6 +156,8 @@ InstallVbeShim (
     case INTEL_Q35_MCH_DEVICE_ID:
       Pam1Address = DRAMC_REGISTER_Q35 (MCH_PAM1);
       break;
+    case MICROVM_PSEUDO_DEVICE_ID:
+      return;
     default:
       DEBUG ((
         DEBUG_ERROR,
-- 
2.35.1

[PATCH v5 5/6] OvmfPkg/Microvm/pcie: mPhysMemAddressWidth tweak

[Gerd Hoffmann]

microvm places the 64bit mmio space at the end of the physical address
space.  So mPhysMemAddressWidth must be correct, otherwise the pci host
bridge setup throws an error because it thinks the 64bit mmio window is
not addressable.

On microvm we can simply use standard cpuid to figure the address width
because the host-phys-bits option (-cpu ${name},host-phys-bits=on) is
forced to be enabled.  Side note: For 'pc' and 'q35' this is not the
case for backward compatibility reasons.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 41 +++++++++++++++++++++
 OvmfPkg/PlatformPei/Platform.c              |  2 +-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 83a7b6726bb7..c28d7601f87e 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -491,6 +491,42 @@ PlatformGetFirstNonAddress (
   return FirstNonAddress;
 }
 
+/*
+ * Use CPUID to figure physical address width.  Does *not* work
+ * reliable on qemu.  For historical reasons qemu returns phys-bits=40
+ * even in case the host machine supports less than that.
+ *
+ * qemu has a cpu property (host-phys-bits={on,off}) to change that
+ * and make sure guest phys-bits are not larger than host phys-bits.,
+ * but it is off by default.  Exception: microvm machine type
+ * hard-wires that property to on.
+ */
+VOID
+EFIAPI
+PlatformAddressWidthFromCpuid (
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT32  RegEax;
+
+  AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
+  if (RegEax >= 0x80000008) {
+    AsmCpuid (0x80000008, &RegEax, NULL, NULL, NULL);
+    PlatformInfoHob->PhysMemAddressWidth = (UINT8)RegEax;
+  } else {
+    PlatformInfoHob->PhysMemAddressWidth = 36;
+  }
+
+  PlatformInfoHob->FirstNonAddress = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
+
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: cpuid: phys-bits is %d\n",
+    __FUNCTION__,
+    PlatformInfoHob->PhysMemAddressWidth
+    ));
+}
+
 /**
   Initialize the PhysMemAddressWidth field in PlatformInfoHob based on guest RAM size.
 **/
@@ -503,6 +539,11 @@ PlatformAddressWidthInitialization (
   UINT64  FirstNonAddress;
   UINT8   PhysMemAddressWidth;
 
+  if (PlatformInfoHob->HostBridgeDevId == 0xffff /* microvm */) {
+    PlatformAddressWidthFromCpuid (PlatformInfoHob);
+    return;
+  }
+
   //
   // As guest-physical memory size grows, the permanent PEI RAM requirements
   // are dominated by the identity-mapping page tables built by the DXE IPL.
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index f006755d5fdb..009db67ee60a 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -357,12 +357,12 @@ InitializePlatform (
 
   S3Verification ();
   BootModeInitialization (&mPlatformInfoHob);
-  AddressWidthInitialization (&mPlatformInfoHob);
 
   //
   // Query Host Bridge DID
   //
   mPlatformInfoHob.HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
+  AddressWidthInitialization (&mPlatformInfoHob);
 
   MaxCpuCountInitialization (&mPlatformInfoHob);
 
-- 
2.35.1

[PATCH v5 6/6] OvmfPkg/Microvm/pcie: add pcie support

[Gerd Hoffmann]

Link in pcie and host bridge bits.  Enables support for PCIe in microvm
(qemu-system-x86_64 -M microvm,pcie=on).

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3777
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Microvm/MicrovmX64.dsc | 40 +++++++++++++++++++++-------------
 OvmfPkg/Microvm/README         |  2 +-
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index c9c843e116a9..82e3f2195aff 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -336,7 +336,9 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
 !endif
   UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
-  PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+#  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
+#  PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+#  PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
 
@@ -353,7 +355,9 @@ [LibraryClasses.common.UEFI_DRIVER]
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
   UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
-  PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
+  PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+  PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
 
 [LibraryClasses.common.DXE_DRIVER]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -375,7 +379,9 @@ [LibraryClasses.common.DXE_DRIVER]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
-  PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
+  PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+  PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
@@ -391,7 +397,9 @@ [LibraryClasses.common.UEFI_APPLICATION]
 !else
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
-  PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
+  PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+  PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
 
 [LibraryClasses.common.DXE_SMM_DRIVER]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -412,7 +420,9 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
 !endif
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
-  PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
+  PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+  PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
 
 [LibraryClasses.common.SMM_CORE]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -428,7 +438,9 @@ [LibraryClasses.common.SMM_CORE]
 !else
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
-  PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
+  PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+  PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
 
 ################################################################################
 #
@@ -503,14 +515,6 @@ [PcdsFixedAtBuild]
   gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F
 !endif
 
-  # This PCD is used to set the base address of the PCI express hierarchy. It
-  # is only consulted when OVMF runs on Q35. In that case it is programmed into
-  # the PCIEXBAR register.
-  #
-  # On Q35 machine types that QEMU intends to support in the long term, QEMU
-  # never lets the RAM below 4 GB exceed 2816 MB.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
-
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
 !endif
@@ -576,6 +580,12 @@ [PcdsDynamicDefault]
   gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
 
+  # set PcdPciExpressBaseAddress to MAX_UINT64, which signifies that this
+  # PCD and PcdPciDisableBusEnumeration below have not been assigned yet
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xFFFFFFFFFFFFFFFF
+  gEfiMdePkgTokenSpaceGuid.PcdPciIoTranslation|0x0
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|TRUE
+
   # Set video resolution for text setup.
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
@@ -676,7 +686,7 @@ [Components]
   OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
     <LibraryClasses>
-      PciHostBridgeLib|MdeModulePkg/Library/PciHostBridgeLibNull/PciHostBridgeLibNull.inf
+      PciHostBridgeLib|OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
       PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
       NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
   }
diff --git a/OvmfPkg/Microvm/README b/OvmfPkg/Microvm/README
index 540d39f2ec21..813920d92a60 100644
--- a/OvmfPkg/Microvm/README
+++ b/OvmfPkg/Microvm/README
@@ -29,7 +29,7 @@ features
  [working] serial console
  [working] direct kernel boot
  [working] virtio-mmio support
- [in progress] pcie support
+ [working] pcie support
 
 known limitations
 -----------------
-- 
2.35.1

Re: [PATCH v5 2/6] OvmfPkg/FdtPciHostBridgeLib: io range is not mandatory

[Abner Chang]

Gerd, I have some comments in line.

With those fixes,
Reviewed-by: Abner Chang <abner.chang@hpe.com>

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, April 22, 2022 3:37 PM
> To: devel@edk2.groups.io
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Liming Gao <gaoliming@byosoft.com.cn>;
> Hao A Wu <hao.a.wu@intel.com>; Ray Ni <ray.ni@intel.com>; Oliver Steffen
> <osteffen@redhat.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Jordan
> Justen <jordan.l.justen@intel.com>; Jiewen Yao <jiewen.yao@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Chang, Abner (HPS SW/FW
> Technologist) <abner.chang@hpe.com>; Jian J Wang
> <jian.j.wang@intel.com>
> Subject: [PATCH v5 2/6] OvmfPkg/FdtPciHostBridgeLib: io range is not
> mandatory
> 
> io range is not mandatory according to pcie spec,
> so allow host bridges without io address space.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 45 ++++++++++---------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> b/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 98828e0b262b..823ea47c80a3 100644
> --- a/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/OvmfPkg/Fdt/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -292,13 +292,8 @@ ProcessPciHost (
>      }
>    }
> 
> -  if ((*IoSize == 0) || (*Mmio32Size == 0)) {
> -    DEBUG ((
> -      DEBUG_ERROR,
> -      "%a: %a space empty\n",
> -      __FUNCTION__,
> -      (*IoSize == 0) ? "IO" : "MMIO32"
> -      ));
> +  if (*Mmio32Size == 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: MMIO32 space empty\n",
> __FUNCTION__));
>      return EFI_PROTOCOL_ERROR;
>    }
> 
> @@ -333,13 +328,15 @@ ProcessPciHost (
>      return Status;
>    }
> 
> -  //
> -  // Map the MMIO window that provides I/O access - the PCI host bridge
> code
> -  // is not aware of this translation and so it will only map the I/O view
> -  // in the GCD I/O map.
> -  //
> -  Status = MapGcdMmioSpace (*IoBase + IoTranslation, *IoSize);
> -  ASSERT_EFI_ERROR (Status);
> +  if (*IoSize) {
I think I missed this in the previous review.
According to coding standard, this line should be
if (*IoSize != 0)

> +    //
> +    // Map the MMIO window that provides I/O access - the PCI host bridge
> code
> +    // is not aware of this translation and so it will only map the I/O view
> +    // in the GCD I/O map.
> +    //
> +    Status = MapGcdMmioSpace (*IoBase + IoTranslation, *IoSize);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> 
>    return Status;
>  }
> @@ -413,17 +410,21 @@ PciHostBridgeGetRootBridges (
> 
>    AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
> 
> -  Io.Base   = IoBase;
> -  Io.Limit  = IoBase + IoSize - 1;
> +  if (IoSize) {
Same here

> +    Io.Base  = IoBase;
> +    Io.Limit = IoBase + IoSize - 1;
> +  } else {
> +    Io.Base  = MAX_UINT64;
> +    Io.Limit = 0;
> +  }
> +
>    Mem.Base  = Mmio32Base;
>    Mem.Limit = Mmio32Base + Mmio32Size - 1;
> 
> -  if (sizeof (UINTN) == sizeof (UINT64)) {
> -    MemAbove4G.Base  = Mmio64Base;
> -    MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
> -    if (Mmio64Size > 0) {
> -      AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
> -    }
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && Mmio64Size) {
Same here for Mmio64Size.

Abner

> +    MemAbove4G.Base       = Mmio64Base;
> +    MemAbove4G.Limit      = Mmio64Base + Mmio64Size - 1;
> +    AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
>    } else {
>      //
>      // UEFI mandates a 1:1 virtual-to-physical mapping, so on a 32-bit
> --
> 2.35.1

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ard Biesheuvel]

On Fri, 22 Apr 2022 at 09:37, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> io range is not mandatory according to pcie spec,
> so allow bridge configurations without io address
> space assigned.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Could one of the MdeModulePkg maintainers please get this reviewed? Thanks.

> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index b20bcd310ad5..712662707931 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -1085,6 +1085,9 @@ NotifyPhase (
>                RootBridge->ResAllocNode[Index].Base   = BaseAddress;
>                RootBridge->ResAllocNode[Index].Status = ResAllocated;
>                DEBUG ((DEBUG_INFO, "Success\n"));
> +            } else if ((Index == TypeIo) && (RootBridge->Io.Base == MAX_UINT64)) {
> +              /* optional on PCIe */
> +              DEBUG ((DEBUG_INFO, "PCI Root Bridge does not provide IO Resources.\n"));
>              } else {
>                ReturnStatus = EFI_OUT_OF_RESOURCES;
>                DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
> --
> 2.35.1
>
>
>
> 
>
>

回复: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[gaoliming]

Acked-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: Ard Biesheuvel <ardb@kernel.org>
> 发送时间: 2022年4月26日 4:49
> 收件人: edk2-devel-groups-io <devel@edk2.groups.io>; Gerd Hoffmann
> <kraxel@redhat.com>
> 抄送: Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Liming Gao <gaoliming@byosoft.com.cn>; Hao
> A Wu <hao.a.wu@intel.com>; Ray Ni <ray.ni@intel.com>; Oliver Steffen
> <osteffen@redhat.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Jordan
> Justen <jordan.l.justen@intel.com>; Jiewen Yao <jiewen.yao@intel.com>;
> Abner Chang <abner.chang@hpe.com>; Jian J Wang <jian.j.wang@intel.com>
> 主题: Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io
> range is not mandatory
> 
> On Fri, 22 Apr 2022 at 09:37, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > io range is not mandatory according to pcie spec,
> > so allow bridge configurations without io address
> > space assigned.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> Could one of the MdeModulePkg maintainers please get this reviewed?
> Thanks.
> 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index b20bcd310ad5..712662707931 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -1085,6 +1085,9 @@ NotifyPhase (
> >                RootBridge->ResAllocNode[Index].Base   =
> BaseAddress;
> >                RootBridge->ResAllocNode[Index].Status = ResAllocated;
> >                DEBUG ((DEBUG_INFO, "Success\n"));
> > +            } else if ((Index == TypeIo) && (RootBridge->Io.Base ==
> MAX_UINT64)) {
> > +              /* optional on PCIe */
> > +              DEBUG ((DEBUG_INFO, "PCI Root Bridge does not
> provide IO Resources.\n"));
> >              } else {
> >                ReturnStatus = EFI_OUT_OF_RESOURCES;
> >                DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
> > --
> > 2.35.1
> >
> >
> >
> > 
> >
> >

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ni, Ray]

Ard,
can you explain more?

Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Tuesday, April 26, 2022 4:49 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>; Gerd Hoffmann <kraxel@redhat.com>
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Oliver Steffen
> <osteffen@redhat.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Chang, Abner <abner.chang@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> 
> On Fri, 22 Apr 2022 at 09:37, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > io range is not mandatory according to pcie spec,
> > so allow bridge configurations without io address
> > space assigned.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> Could one of the MdeModulePkg maintainers please get this reviewed? Thanks.
> 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index b20bcd310ad5..712662707931 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -1085,6 +1085,9 @@ NotifyPhase (
> >                RootBridge->ResAllocNode[Index].Base   = BaseAddress;
> >                RootBridge->ResAllocNode[Index].Status = ResAllocated;
> >                DEBUG ((DEBUG_INFO, "Success\n"));
> > +            } else if ((Index == TypeIo) && (RootBridge->Io.Base == MAX_UINT64)) {
> > +              /* optional on PCIe */
> > +              DEBUG ((DEBUG_INFO, "PCI Root Bridge does not provide IO Resources.\n"));
> >              } else {
> >                ReturnStatus = EFI_OUT_OF_RESOURCES;
> >                DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
> > --
> > 2.35.1
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ni, Ray]

I thought the patch is from Ard but it was from Gerd.
So, the question is for Gerd.:)

I am a bit nervous on this change because it's a behavior change and may cause
certain devices malfunction and it's a silent failure.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Wednesday, April 27, 2022 11:09 AM
> To: devel@edk2.groups.io; ardb@kernel.org; Gerd Hoffmann <kraxel@redhat.com>
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Oliver Steffen <osteffen@redhat.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Chang,
> Abner <abner.chang@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> 
> Ard,
> can you explain more?
> 
> Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
> If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation
> to override the IO request from the devices?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > Sent: Tuesday, April 26, 2022 4:49 AM
> > To: edk2-devel-groups-io <devel@edk2.groups.io>; Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Oliver Steffen
> > <osteffen@redhat.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Chang, Abner <abner.chang@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> >
> > On Fri, 22 Apr 2022 at 09:37, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > io range is not mandatory according to pcie spec,
> > > so allow bridge configurations without io address
> > > space assigned.
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Could one of the MdeModulePkg maintainers please get this reviewed? Thanks.
> >
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > index b20bcd310ad5..712662707931 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > @@ -1085,6 +1085,9 @@ NotifyPhase (
> > >                RootBridge->ResAllocNode[Index].Base   = BaseAddress;
> > >                RootBridge->ResAllocNode[Index].Status = ResAllocated;
> > >                DEBUG ((DEBUG_INFO, "Success\n"));
> > > +            } else if ((Index == TypeIo) && (RootBridge->Io.Base == MAX_UINT64)) {
> > > +              /* optional on PCIe */
> > > +              DEBUG ((DEBUG_INFO, "PCI Root Bridge does not provide IO Resources.\n"));
> > >              } else {
> > >                ReturnStatus = EFI_OUT_OF_RESOURCES;
> > >                DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
> > > --
> > > 2.35.1
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

On Wed, Apr 27, 2022 at 03:08:50AM +0000, Ni, Ray wrote:
> Ard,
> can you explain more?
> 
> Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
> If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Hmm, it's a problem indeed, device initialization fails in case an
io bar is present even if the bar is not required to drive the device.

Suggestions how to deal with this best?  ovmf has it's own
IncompatiblePciDevice Protocol implementation, so I could
handle it there because only OvmfPkg/Microvm needs this.

Or should the MdeModulePkg version be updated too?

thanks,
  Gerd

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ard Biesheuvel]

On Fri, 29 Apr 2022 at 08:50, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Apr 27, 2022 at 03:08:50AM +0000, Ni, Ray wrote:
> > Ard,
> > can you explain more?
> >
> > Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
> > If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?
>
> Hmm, it's a problem indeed, device initialization fails in case an
> io bar is present even if the bar is not required to drive the device.
>

I'd say the risk for regressions is rather low, though, given that it
only affects configurations that would fail PCI resource allocation
today. Or am I missing something?

In any case, the PCIe spec is clear about this: I/O space is optional,
and we need to incorporate this into the generic code at *some* point.
It makes no sense for every individual platform to keep adding these
hacks.

> Suggestions how to deal with this best?  ovmf has it's own
> IncompatiblePciDevice Protocol implementation, so I could
> handle it there because only OvmfPkg/Microvm needs this.
>
> Or should the MdeModulePkg version be updated too?
>

I'd say we do both, to avoid stalling your series forever :-)

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ni, Ray]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Friday, April 29, 2022 3:00 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Gao, Liming <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Oliver Steffen
> <osteffen@redhat.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Chang, Abner <abner.chang@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> 
> On Fri, 29 Apr 2022 at 08:50, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Wed, Apr 27, 2022 at 03:08:50AM +0000, Ni, Ray wrote:
> > > Ard,
> > > can you explain more?
> > >
> > > Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
> > > If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol
> implementation to override the IO request from the devices?
> >
> > Hmm, it's a problem indeed, device initialization fails in case an
> > io bar is present even if the bar is not required to drive the device.
> >
> 
> I'd say the risk for regressions is rather low, though, given that it
> only affects configurations that would fail PCI resource allocation
> today. Or am I missing something?
> 
> In any case, the PCIe spec is clear about this: I/O space is optional,
> and we need to incorporate this into the generic code at *some* point.
> It makes no sense for every individual platform to keep adding these
> hacks.
Do you know how Linux handles this?
Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?

> 
> > Suggestions how to deal with this best?  ovmf has it's own
> > IncompatiblePciDevice Protocol implementation, so I could
> > handle it there because only OvmfPkg/Microvm needs this.
> >
> > Or should the MdeModulePkg version be updated too?
> >
> 
> I'd say we do both, to avoid stalling your series forever :-)

Why changing the MdeModulePkg's IncompatiblePciDevice driver can avoid
stalling the patch series?
I feel it's enough to just change the OvmfPkg version.

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

  Hi,

> > I'd say the risk for regressions is rather low, though, given that it
> > only affects configurations that would fail PCI resource allocation
> > today. Or am I missing something?
> > 
> > In any case, the PCIe spec is clear about this: I/O space is optional,
> > and we need to incorporate this into the generic code at *some* point.
> > It makes no sense for every individual platform to keep adding these
> > hacks.
> Do you know how Linux handles this?
> Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?

Yes.  Details depend a bit on the specific configuration, but in general
linux will try assign io address space to pcie root ports and devices
plugged into those ports.  A failure is not considered fatal though.

A more common case than the pci root bridge not supporting io address
space at all is having more than 16 pcie root ports.  Given io bride
windows are 1k in size and we have 16k total there is simply not enough
io address space in that case, so some of the root ports stay without
io and linux is fine with that.

> Why changing the MdeModulePkg's IncompatiblePciDevice driver can avoid
> stalling the patch series?
> I feel it's enough to just change the OvmfPkg version.

It's not much of a problem for ovmf even without such an update,
typically the devices used with microvm don't have io bars in the first
place.

Also note that without this series pcie devices are not supported at all
on microvm, so not supporting all devices initially wouldn't be a
regression.

I'll look into it in any case.

take care,
  Gerd

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ni, Ray]

> 
>   Hi,
> 
> > > I'd say the risk for regressions is rather low, though, given that it
> > > only affects configurations that would fail PCI resource allocation
> > > today. Or am I missing something?
> > >
> > > In any case, the PCIe spec is clear about this: I/O space is optional,
> > > and we need to incorporate this into the generic code at *some* point.
> > > It makes no sense for every individual platform to keep adding these
> > > hacks.
> > Do you know how Linux handles this?
> > Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?
> 
> Yes.  Details depend a bit on the specific configuration, but in general
> linux will try assign io address space to pcie root ports and devices
> plugged into those ports.  A failure is not considered fatal though.

An error message and continue? 

> 
> A more common case than the pci root bridge not supporting io address
> space at all is having more than 16 pcie root ports.  Given io bride
> windows are 1k in size and we have 16k total there is simply not enough
> io address space in that case, so some of the root ports stay without
> io and linux is fine with that.

Does it have some certain policy that IO resource for first root bridge should
be satisfied?

> 
> > Why changing the MdeModulePkg's IncompatiblePciDevice driver can avoid
> > stalling the patch series?
> > I feel it's enough to just change the OvmfPkg version.
> 
> It's not much of a problem for ovmf even without such an update,
> typically the devices used with microvm don't have io bars in the first
> place.
> 
> Also note that without this series pcie devices are not supported at all
> on microvm, so not supporting all devices initially wouldn't be a
> regression.
> 
> I'll look into it in any case.

The safest way is to change OVMF now.
Add @Nong, Foster and @Albecki, Mateusz for comments.

> 
> take care,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

  Hi,

> > > Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?
> > 
> > Yes.  Details depend a bit on the specific configuration, but in general
> > linux will try assign io address space to pcie root ports and devices
> > plugged into those ports.  A failure is not considered fatal though.
> 
> An error message and continue? 

Not even an error message.  In case the pci core code assigns a io
window to the pci root port it will log a message saying so.  In case
it doesn't it stays silent.

> > A more common case than the pci root bridge not supporting io address
> > space at all is having more than 16 pcie root ports.  Given io bride
> > windows are 1k in size and we have 16k total there is simply not enough
> > io address space in that case, so some of the root ports stay without
> > io and linux is fine with that.
> 
> Does it have some certain policy that IO resource for first root bridge should
> be satisfied?

I don't know for sure.  From the boot logs it looks like the kernel
simply assigns resources in pci scan order, and when it runs out of
resources it stops assigning.

take care,
  Gerd

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

  Hi,

> If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Hmm, how can the IncompatiblePciDevice protocol specify that IO bars
should be ignored?

Seems I can override the size using
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR.AddrLen.  I can't set the length to
zero though because setting AddrLen to 0 means "no overide".

Is there another way to have edk2 ignore an PCI bar?  The spec isn't
verbose here (looking at PI spec 1.7a, table 5-20).

thanks,
  Gerd

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

@Ni, Ray

I think EDK2 needs to provide a way for root port to operate without IO space assigned in a platform-independent way. I can think of the following cases when root port didn't get IO space:

1. We have run out of IO space but it's fine since the device under the root port doesn't use IO or has only non-critical functionalities under IO
2. We have run out of IO space and it's really not fine since device needs IO
3. We are running on a CPU which doesn't support IO

For 1. the question is whether the device driver in EDK2 understands that IO bar for that device is optional and will bother to check if it has been assigned and either fail gracefully or continue operation in limited capacity. For 2. the question is whether the driver will fail gracefully. 3 is for completeness at this point I think since the only other architecture that uses EDK2 is ARM which has to deal with it in some way right now which I think maps IO region into MMIO so in a way it supports IO.

I've checked the device driver behavior in EDK2 for devices which use IO bar here is the rundown:
1. IDE - Doesn't check if IO has been assigned, not giving IO results in undefined behavior
2. SerialIo -> Doesn't check, will assert the system when IO is not assigned (although the logic there is really strange as it can use 3 different access methods)
3. UHCI -> Checks but too late, will most likely result in undefined behavior

Even with those bad device drivers I would agree that taking this change presents low risk given that those devices are pretty old and should be mostly unused on new systems(SerialIo being an exception but that one is usually an RCIEP). That said I think we are missing a larger issue here - why are we running out of IO when we have 16 root ports? Surely we don't have a device with IO requirement behind each of those root ports so is the BIOS blindly assigning IO to root ports which have no requirement? I see on my system that when we don't have IO requirement behind the root port BIOS sets IOBASE to 0xF0 and IOLIMIT to 0x0 which means no IO decode will be performed.

Thanks,
Mateusz

[-- Attachment #2: Type: text/html, Size: 2207 bytes --]

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

On Mon, May 23, 2022 at 04:48:05AM -0700, Albecki, Mateusz wrote:
> @Ni, Ray
> 
> I think EDK2 needs to provide a way for root port to operate without IO space assigned in a platform-independent way. I can think of the following cases when root port didn't get IO space:
> 
> 1. We have run out of IO space but it's fine since the device under the root port doesn't use IO or has only non-critical functionalities under IO
> 2. We have run out of IO space and it's really not fine since device needs IO
> 3. We are running on a CPU which doesn't support IO
> 
> For 1. the question is whether the device driver in EDK2 understands that IO bar for that device is optional and will bother to check if it has been assigned and either fail gracefully or continue operation in limited capacity. For 2. the question is whether the driver will fail gracefully. 3 is for completeness at this point I think since the only other architecture that uses EDK2 is ARM which has to deal with it in some way right now which I think maps IO region into MMIO so in a way it supports IO.

Well, the case I'm trying to handle here is qemu microvm.  It's x86, but
io address space support for pcie devices is not wired up.   So the pcie
host bridge doesn't support io, which is rather close to case (3).

> I've checked the device driver behavior in EDK2 for devices which use IO bar here is the rundown:
> 1. IDE - Doesn't check if IO has been assigned, not giving IO results in undefined behavior
> 2. SerialIo -> Doesn't check, will assert the system when IO is not assigned (although the logic there is really strange as it can use 3 different access methods)
> 3. UHCI -> Checks but too late, will most likely result in undefined behavior

Current edk2 behavior is that the initialization of the pcie host bridge
fails in case no io space is present (and all devices connected to it are
not initialized either of course).

With this patch applied pcie host bridge initialization works.  PCIe
devices without io bars are enumerated and initialized sucessfully.
PCIe devices with io bar fail to initialize.  That isn't much of a
problem tough as a qemu microvm typically has no pcie devices with io
bars.

> Even with those bad device drivers I would agree that taking this
> change presents low risk given that those devices are pretty old and
> should be mostly unused on new systems(SerialIo being an exception but
> that one is usually an RCIEP).

Also note that for pcie root bridges which do support io address space
this patch changes nothing.

> That said I think we are missing a larger issue here - why are we
> running out of IO when we have 16 root ports?

I don't think so.  I see the *linux kernel* hand out io address space to
pcie root ports (until it runs out).  edk2 doesn't.

take care,
  Gerd

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 5931 bytes --]

On Mon, May 23, 2022 at 11:24 PM, Gerd Hoffmann wrote:

> 
> On Mon, May 23, 2022 at 04:48:05AM -0700, Albecki, Mateusz wrote:
> 
>> @Ni, Ray
>> 
>> I think EDK2 needs to provide a way for root port to operate without IO
>> space assigned in a platform-independent way. I can think of the following
>> cases when root port didn't get IO space:
>> 
>> 1. We have run out of IO space but it's fine since the device under the
>> root port doesn't use IO or has only non-critical functionalities under IO
>> 
>> 2. We have run out of IO space and it's really not fine since device needs
>> IO
>> 3. We are running on a CPU which doesn't support IO
>> 
>> For 1. the question is whether the device driver in EDK2 understands that
>> IO bar for that device is optional and will bother to check if it has been
>> assigned and either fail gracefully or continue operation in limited
>> capacity. For 2. the question is whether the driver will fail gracefully.
>> 3 is for completeness at this point I think since the only other
>> architecture that uses EDK2 is ARM which has to deal with it in some way
>> right now which I think maps IO region into MMIO so in a way it supports
>> IO.
> 
> Well, the case I'm trying to handle here is qemu microvm. It's x86, but
> io address space support for pcie devices is not wired up. So the pcie
> host bridge doesn't support io, which is rather close to case (3).
> 
> 
>> I've checked the device driver behavior in EDK2 for devices which use IO
>> bar here is the rundown:
>> 1. IDE - Doesn't check if IO has been assigned, not giving IO results in
>> undefined behavior
>> 2. SerialIo -> Doesn't check, will assert the system when IO is not
>> assigned (although the logic there is really strange as it can use 3
>> different access methods)
>> 3. UHCI -> Checks but too late, will most likely result in undefined
>> behavior
> 
> Current edk2 behavior is that the initialization of the pcie host bridge
> fails in case no io space is present (and all devices connected to it are
> not initialized either of course).
> 
> With this patch applied pcie host bridge initialization works. PCIe
> devices without io bars are enumerated and initialized sucessfully.
> PCIe devices with io bar fail to initialize. That isn't much of a
> problem tough as a qemu microvm typically has no pcie devices with io
> bars.

You mention that devices with IO bar fail to initialize but that is contrary to what I would expect from code review. I've run an experiment with your change in which I am telling
EDK2 that no IO space is available on the system by not updating the IO range in PciHostBridgeLib. Sure enough Devices that need an IO are still enumerated, device path and PciIo are installed
and in general everything works as it used to. If I had an UHCI controller on that system UHCI driver would be loaded and it could potentially result in some strange behavior since that driver isn't smart enough to check
if IO space has been allocated for the device.

To make things worse I see that if we return success there EDK2 will actually go ahead and start assigning trash addresses to the device and enable IO space decoding in case of the PCI root port which means that device will try to decode
invalid IO ranges. Not an issue for a system without an IO but for a system in which we have run out of the IO and we have entered this code branch this new behavior is potentially more dangerous then simply not enumerating the device.

Logs:
PciBus: Resource Map for Root Bridge PciRoot(0x0)
Type =   Io16; Base = 0xFFFFFFFFFFFFFFFF; Length = 0x1000; Alignment = 0xFFF
Base = 0x0; Length = 0x4; Alignment = 0x3; Owner = PPB [00|06|04:14]
Base = 0x0; Length = 0x4; Alignment = 0x3; Owner = PPB [00|06|00:14]
Base = 0xFFFFFFFC; Length = 0x4; Alignment = 0x3; Owner = PCI [00|04|00:24]
Base = 0xFFFFFFFC; Length = 0x4; Alignment = 0x3; Owner = PCI [00|04|00:20]
Base = 0xFFFFFFFC; Length = 0x4; Alignment = 0x3; Owner = PCI [00|04|00:1C]
Base = 0xFFFFFFFC; Length = 0x4; Alignment = 0x3; Owner = PCI [00|04|00:18]

> 
> 
>> Even with those bad device drivers I would agree that taking this
>> change presents low risk given that those devices are pretty old and
>> should be mostly unused on new systems(SerialIo being an exception but
>> that one is usually an RCIEP).
> 
> Also note that for pcie root bridges which do support io address space
> this patch changes nothing.

It seems to me like it does. Specifically the error scenario where the system has run out of IO space will not be handled properly I think.

> 
> 
>> That said I think we are missing a larger issue here - why are we
>> running out of IO when we have 16 root ports?
> 
> I don't think so. I see the *linux kernel* hand out io address space to
> pcie root ports (until it runs out). edk2 doesn't.

Ok I misunderstood previous mails

> 
> take care,
> Gerd

I think to really handle it we would have to have a more involved change. Specifically in the PciHostBridge.c:NoitfyPhase function we need to have a way to tell the PciBus driver which resources specifically failed to allocate and treat this as a condition we need to handle instead of panicking and asserting the system or dropping the entire host bridge. When PciBus driver sees that IO failed to allocate it would skip IO bars and would not allow to set the IO space enable bit in the root bridge. We also would need to change the logic in PciResourceSupport.c:ProgramBar because that function is very optimistic and assumes that if we were able to program one BAR then surely all resources for the device are allocated:

//
// Indicate pci bus driver has allocated
// resource for this device
// It might be a temporary solution here since
// pci device could have multiple bar
//
Node->PciDev->Allocated = TRUE;

Which simply isn't the case.

Thanks,
Mateusz

[-- Attachment #2: Type: text/html, Size: 7254 bytes --]

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Gerd Hoffmann]

  Hi,
 
> To make things worse I see that if we return success there EDK2 will
> actually go ahead and start assigning trash addresses to the device
> and enable IO space decoding in case of the PCI root port which means
> that device will try to decode invalid IO ranges.

> Logs:
> PciBus: Resource Map for Root Bridge PciRoot(0x0)
> Type =   Io16; Base = 0xFFFFFFFFFFFFFFFF; Length = 0x1000; Alignment = 0xFFF

That is wrong indeed.

I think *this* should work ...

+            } else if ((Index == TypeIo) &&
+                       (RootBridge->Io.Base == MAX_UINT64) &&
+                       (RootBridge->ResAllocNode[Index].Length == 0)) {
+              /* I/O is optional on PCIe */
+              DEBUG ((DEBUG_INFO, "Success (PCIe NoIO)\n"));

... i.e. return success only in case there are no allocation requests
for IO ranges.

> > Also note that for pcie root bridges which do support io address space
> > this patch changes nothing.
> 
> It seems to me like it does.

It doesn't.  When io address space is present the "RootBridge->Io.Base
== MAX_UINT64" check will never be true.

But the "no io address space" case was wrong indeed.

> I think to really handle it we would have to have a more involved
> change.

If we want support PCIe devices with I/O bars behind a PCIe host bridge
without I/O window, then yes.  My main focus is supporting PCIe devices
without I/O bars though.  This doesn't work currently because the code
considers a pcie host bridge without I/O window a hard failure even in
case there are no I/O allocation requests.  For fixing that the five
lines above should be enough I think.

thanks & take care,
  Gerd

Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

[Ni, Ray]

> This doesn't work currently because the code
> considers a pcie host bridge without I/O window a hard failure even in
> case there are no I/O allocation requests.  For fixing that the five
> lines above should be enough I think.

That's a real bug we should fix. I agree!!