Basically I don't agree to add a CFG space range field. See embedded reply in below.
Regards, Ray >-----Original Message----- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Saturday, February 27, 2016 8:23 AM >To: edk2-devel-01 <edk2-de...@ml01.01.org> >Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L ><jordan.l.jus...@intel.com>; Marcel Apfelbaum <mar...@redhat.com> >Subject: [PATCH 01/17] MdeModulePkg: PciHostBridgeDxe: don't assume extended >config space > >The "PcAtChipsetPkg/PciHostBridgeDxe" driver hard-codes the [0x00..0xFF] >range as valid config space offsets, in the RootBridgeIoCheckParameter() >function -- see the MAX_PCI_REG_ADDRESS macro. This driver uses IO ports >0xCF8 / 0xCFC to access PCI config space. > >The (soon to be removed) "OvmfPkg/PciHostBridgeDxe" does the same. > >The "ArmVirtPkg/PciHostBridgeDxe" uses PCI Express (ECAM) instead, and >hard-codes the [0x00..0xFFF] range as valid config space offsets. > >The "MdeModulePkg/Bus/Pci/PciHostBridgeDxe" driver hard-codes the >[0x00..0xFFF] range as valid config space offsets, without actually >knowing if the platform uses PCI Express (ECAM) or plain PCI (0xCF8 / >0xCFC). For this generalized driver, the config access method is >ultimately decided by the platform's PciSegmentLib instance. > >Quoting the "MdePkg/Include/Library/PciSegmentLib.h" library class file: > > These functions perform PCI configuration cycles using the default PCI > configuration access method. This may use I/O ports 0xCF8 and 0xCFC to > perform PCI configuration accesses, or it may use MMIO registers > relative to the PcdPciExpressBaseAddress, or it may use some alternate > access method. [...] > >Clearly the configuration access method determines the boundaries of the >config space as well, for each individual PCI function. However, the >PciSegmentLib class provides no API for PciHostBridgeDxe to query these >boundaries! > >In order to remedy this, add another PCI_ROOT_BRIDGE_APERTURE field to the >root bridge structures in both PciHostBridgeDxe and the PciHostBridgeLib >class. This way platforms can control the PCI config space boundaries for >the purposes of PciHostBridgeDxe, in accordance with their config access >methods. > >This bug causes the following symptoms with OVMF and QEMU: > >QEMU's i440fx machine type is a PCI (0xCF8 / 0xCFC) machine. It supports >physical device assignment. When assigning a physical EVGA GTX750 GPU to >the guest, the PCI bus driver correctly determines that the GPU is a PCIe >device, and tries to locate its extended capabilities (specifically, ARI), >starting at config space offset 0x100. > >When OVMF is built with "OvmfPkg/PciHostBridgeDxe", this config access is >caught and rejected by RootBridgeIoCheckParameter(), due to the limit >being 0xFF. (And the PCI bus driver gracefully recovers from this >rejection.) Does the platform expect the graceful recovery? I may treat it as a silent failure:) Some additional devices cannot come up due to this recovery/failure but platform developer without deep debugging cannot know the root cause, if ARI is disabled. > >However, "MdeModulePkg/Bus/Pci/PciHostBridgeDxe" lets the address through, >because its variant of the same function hard-codes 0xFFF as the config >space limit. > >Then RootBridgeIoPciAccess() sends the (invalid) address down the >following chain of libraries: > > BasePciSegmentLibPci [class: PciSegmentLib] > BasePciLibCf8 [class: PciLib] > BasePciCf8Lib [class: PciCf8Lib] > >Finally, ASSERT_INVALID_PCI_ADDRESS() in PciCf8Read32() realizes that >offset 0x100 is invalid for the 0xCF8 / 0xCFC config access method, and >the firmware halts. The silent failure is bad. So the assertion is a good to tell platform developer to choose the correct PciSegment library instance. > >Fix this by informing the generalized PciHostBridgeDxe driver about the >platform's config space boundaries, so that the driver can recognize and >reject out-of-bounds accesses in time. > >Cc: Ruiyu Ni <ruiyu...@intel.com> >Cc: Jordan Justen <jordan.l.jus...@intel.com> >Cc: Marcel Apfelbaum <mar...@redhat.com> >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Laszlo Ersek <ler...@redhat.com> >--- > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 1 + > MdeModulePkg/Include/Library/PciHostBridgeLib.h | 1 + > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 6 ++++-- > 3 files changed, 6 insertions(+), 2 deletions(-) > >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h >index b1e83f1c9089..2dfc8963f8e9 100644 >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h >@@ -71,6 +71,7 @@ typedef struct { > PCI_ROOT_BRIDGE_APERTURE PMem; > PCI_ROOT_BRIDGE_APERTURE MemAbove4G; > PCI_ROOT_BRIDGE_APERTURE PMemAbove4G; >+ PCI_ROOT_BRIDGE_APERTURE Pci; > BOOLEAN DmaAbove4G; > VOID *ConfigBuffer; > EFI_DEVICE_PATH_PROTOCOL *DevicePath; >diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h >b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >index b1dba0f754d7..a9e9ca308c7c 100644 >--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h >+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >@@ -44,6 +44,7 @@ typedef struct { > PCI_ROOT_BRIDGE_APERTURE MemAbove4G; ///< MMIO aperture above 4GB > which can be used by the >root bridge. > PCI_ROOT_BRIDGE_APERTURE PMem; ///< Prefetchable MMIO > aperture below 4GB which can be >used by the root bridge. > PCI_ROOT_BRIDGE_APERTURE PMemAbove4G; ///< Prefetchable MMIO > aperture above 4GB which can be >used by the root bridge. >+ PCI_ROOT_BRIDGE_APERTURE Pci; ///< PCI config space range >that is valid for the devices behind >the root bridge. > EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path. > } PCI_ROOT_BRIDGE; > >diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >index 332860eb3819..6b7bd74290a7 100644 >--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >@@ -90,6 +90,7 @@ CreateRootBridge ( > DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base, > Bridge->MemAbove4G.Limit)); > DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base, > Bridge->PMem.Limit)); > DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base, > Bridge->PMemAbove4G.Limit)); >+ DEBUG ((EFI_D_INFO, " Pci: %lx - %lx\n", Bridge->Pci.Base, >Bridge->Pci.Limit)); > > // > // Make sure Mem and MemAbove4G apertures are valid >@@ -168,6 +169,7 @@ CreateRootBridge ( > CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE)); > CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof (PCI_ROOT_BRIDGE_APERTURE)); > CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof > (PCI_ROOT_BRIDGE_APERTURE)); >+ CopyMem (&RootBridge->Pci, &Bridge->Pci, sizeof (PCI_ROOT_BRIDGE_APERTURE)); > > > for (Index = TypeIo; Index < TypeMax; Index++) { >@@ -350,8 +352,8 @@ RootBridgeIoCheckParameter ( > } else { > Address = PciRbAddr->Register; > } >- Base = 0; >- Limit = 0xFFF; >+ Base = RootBridge->Pci.Base; >+ Limit = RootBridge->Pci.Limit; > } > > if (Address < Base) { >-- >1.8.3.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
