| Message ID | 1456532616-32475-2-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/27/16 03:13, Ni, Ruiyu wrote: > 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:lersek@redhat.com] >> Sent: Saturday, February 27, 2016 8:23 AM >> To: edk2-devel-01 <edk2-devel@ml01.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Marcel Apfelbaum <marcel@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. I agree that the assertion is good. BasePciCf8Lib should indeed catch invalid addresses, and complain *loudly*. However, my patch is not about suppressing this assertion. My patch corrects an error in the RootBridgeIoCheckParameter() function in "MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c". The error in the RootBridgeIoCheckParameter() function is that it accepts PCI config space accesses as valid up to register offset 0xFFF, even if the platform doesn't support that. On a platform where only the 0xCF8 access method is available, accepting config space offsets up to 0xFFF is a bug. On such a platform, offsets >= 0x100 should be rejected as invalid. That's all. The rest of the commit message just describes how this error in RootBridgeIoCheckParameter() leads to a failed assertion later. The failed assertion is *justified*. It is the job of RootBridgeIoCheckParameter() to recognize and catch the incorrect offset *earlier*. So, hard-coding 0xFFF in RootBridgeIoCheckParameter() is the bug. The platform must be able to provide that value as a parameter. That's why I listed the other host bridge drivers in the commit message, as examples: in those drivers, the config access method and the maximum config offset are *in sync*. In the case of this driver however, they are out of sync, if the PciSegmentLib library can only provide 0xCF8 access. Your suggestion that the platform developer choose the "correct" PciSegmentLib instance is not valid -- the platform developer has zero liberty in "choosing" the library. The platform (the hardware) *dictates* that choice. If the platform is only capable of 0xCF8, and incapable of PCI Express (ECAM), then 0xFFF as a config space limit is invalid, plain and simple, and the PciSegmentLib *instance* cannot do anything about it. *Normally*, it would be the job of the PciSegmentLib *class* to provide an interface where the maximum config space offset can be queried. Then PciHostBridgeDxe could call that interface -- which would return 0xFF, 0xFFF, or whatever else matching the internals of the PciSegmentLib *instance* -- and RootBridgeIoCheckParameter() could enforce *that* limit. However, the PciSegmentLib *class* lacks such an API. So we need another way to tell the limit to PciHostBridgeDxe. Thus, this argument comprises two parts: 1. PciHostBridgeDxe needs to use a platform dependent config space limit: 0xFF, 0xFFF, or something else that matches the platform's PciSegmentLib instance. Correcting this bug is not optional, it is unavoidable. Hard-coding an ECAM assumption in the driver is a bug. 2. The other question is *whence exactly* this information should come. There are several alternatives here; let me list a few: * Add a new API to PciSegmentLib that returns the limit. * Or, add a new API to PciHostBridgeLib that returns the limit. * Or, add a new PCD that communicates the limit. * Or, in the driver, investigate PcdPciExpressBaseAddress (which is the central PCD for the base of the MMCONFIG / ECAM area). If the value is MAX_UINT64 -- which is obviously unusable as ECAM --, then assume a limit of 0xFF. Otherwise, assume 0xFFF. * Or, pass the limit as part of the structure that PciHostBridgeLib already produces. This patch implements this alternative. I'm absolutely fine with any one of the options under (2). But, (1) must be fixed in *some* way. Thanks Laszlo > >> >> 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.ni@intel.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@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
On 02/28/16 08:16, Ni, Ruiyu wrote: >>> 在 2016年2月27日，下午6:17，Laszlo Ersek <lersek@redhat.com> 写道： >> Thus, this argument comprises two parts: >> >> 1. PciHostBridgeDxe needs to use a platform dependent config space >> limit: 0xFF, 0xFFF, or something else that matches the platform's >> PciSegmentLib instance. Correcting this bug is not optional, it is >> unavoidable. Hard-coding an ECAM assumption in the driver is a bug. > You didn't convince me here but given the fact you told me that qemu > is too old to support pci express way access, Small clarification: the "motherboard (chipset) services" that QEMU provides *vary*. First, you have to pick an architecture to emulate (i386, x86_64, arm, aarch64, and so on). This is called "target". The different targets are built into different QEMU executables. For system emulation, they look like: - qemu-system-x86_64 - qemu-system-aarch64 - etc Once you picked the binary (i.e., you selected the target), QEMU provides several "machine types" for that target. The targets can be listed with "-machine help": $ qemu-system-x86_64 -machine help Supported machines are: pc Standard PC (i440FX + PIIX, 1996) (alias of pc-i440fx-2.6) pc-i440fx-2.6 Standard PC (i440FX + PIIX, 1996) (default) pc-i440fx-2.5 Standard PC (i440FX + PIIX, 1996) pc-i440fx-2.4 Standard PC (i440FX + PIIX, 1996) pc-i440fx-2.3 Standard PC (i440FX + PIIX, 1996) pc-i440fx-2.2 Standard PC (i440FX + PIIX, 1996) pc-i440fx-2.1 Standard PC (i440FX + PIIX, 1996) pc-i440fx-2.0 Standard PC (i440FX + PIIX, 1996) pc-i440fx-1.7 Standard PC (i440FX + PIIX, 1996) pc-i440fx-1.6 Standard PC (i440FX + PIIX, 1996) pc-i440fx-1.5 Standard PC (i440FX + PIIX, 1996) pc-i440fx-1.4 Standard PC (i440FX + PIIX, 1996) pc-1.3 Standard PC (i440FX + PIIX, 1996) pc-1.2 Standard PC (i440FX + PIIX, 1996) pc-1.1 Standard PC (i440FX + PIIX, 1996) pc-1.0 Standard PC (i440FX + PIIX, 1996) pc-0.15 Standard PC (i440FX + PIIX, 1996) pc-0.14 Standard PC (i440FX + PIIX, 1996) pc-0.13 Standard PC (i440FX + PIIX, 1996) pc-0.12 Standard PC (i440FX + PIIX, 1996) pc-0.11 Standard PC (i440FX + PIIX, 1996) pc-0.10 Standard PC (i440FX + PIIX, 1996) q35 Standard PC (Q35 + ICH9, 2009) (alias of pc-q35-2.6) pc-q35-2.6 Standard PC (Q35 + ICH9, 2009) pc-q35-2.5 Standard PC (Q35 + ICH9, 2009) pc-q35-2.4 Standard PC (Q35 + ICH9, 2009) pc-q35-2.3 Standard PC (Q35 + ICH9, 2009) pc-q35-2.2 Standard PC (Q35 + ICH9, 2009) pc-q35-2.1 Standard PC (Q35 + ICH9, 2009) pc-q35-2.0 Standard PC (Q35 + ICH9, 2009) pc-q35-1.7 Standard PC (Q35 + ICH9, 2009) pc-q35-1.6 Standard PC (Q35 + ICH9, 2009) pc-q35-1.5 Standard PC (Q35 + ICH9, 2009) pc-q35-1.4 Standard PC (Q35 + ICH9, 2009) isapc ISA-only PC none empty machine You can identify two facts from this output. First, some machine types are versioned. This is related to migration compatibility, and not very interesting here. The other fact is that there are two machine type "families", i440fx, and q35. Those are quite different. i440fx is the older machine type. It lacks a number of things that Q35 provides. For example: - it lacks ECAM / MMCONFIG - it also lacks SMM. (Remember <http://thread.gmane.org/gmane.comp.bios.edk2.devel/8078>?) Q35 is the newer, more modern machine type. It does have ECAM / MMCONFIG, but Q35 is not the default machine type. i440fx is way more "mature"; it is almost universally used by users. For example, our own RHEL-7.2 release does not officially support Q35 machine types. Hence, the clarification I'd like to make is: QEMU (the project) is definitely not too old to support ECAM. Instead, the i440fx machine type in particular, emulated by QEMU, is too old (2016 - 1996 = 20 years). For another example, the "virt" machine type of "qemu-system-aarch64" emulates ECAM as well. (And "ArmVirtPkg/PciHostBridgeDxe" uses ECAM.) So, it is not QEMU in general that is limited to 0xCF8 / 0xCFC; it is one specific (emulation target, machine type) pair that has this limitation. i440fx happens to be the most widely used machine type. I'm not aware of any specific plans or timeline to switch the default to Q35. > I agree I didn't think > there might be a such old platform. Yes, the virtual hardware of the i440fx machine type mostly reflects 1996, as the help output above says. It is based on the "Intel ® 82371AB PIIX4" and the "Intel ® 440FX PCIset 82441FX (PMC)" specs. The PDFs that I have on my disk for understanding and occasionally developing i440fx are: - document # 290549-001 - document # 290562-001 - document # 297654-006 - document # 297738-017 (They are listed in "OvmfPkg/Include/IndustryStandard/I440FxPiix4.h" as well.) > So enhancing my driver to support > it is acceptable. Thank you. I hugely appreciate it. >> 2. The other question is *whence exactly* this information should come. >> There are several alternatives here; let me list a few: >> >> * Add a new API to PciSegmentLib that returns the limit. > Not good because then pci lib also needs such api. >> >> * Or, add a new API to PciHostBridgeLib that returns the limit. > Intend to use this way. In a extreme case, a platform may contain two > root bridges, one support MMIO cfg space access, the other doesn't. > So we may need to use the way you proposed. But I think a flag is > enough rather than a aperture range. Did you see platform that has > range other than [0,ff] [0,fff]? I'm not aware of anything different from 0x0-0xff and 0x0-0xfff. Therefore a simple boolean flag (per root bridge structure) could do the job indeed. I will update my patch and submit it separately, for your review. >> >> * Or, add a new PCD that communicates the limit. >> >> * Or, in the driver, investigate PcdPciExpressBaseAddress (which is >> the central PCD for the base of the MMCONFIG / ECAM area). If the >> value is MAX_UINT64 -- which is obviously unusable as ECAM --, then >> assume a limit of 0xFF. Otherwise, assume 0xFFF. > A multi segment platform may also set such PCD to an invalid value because there are multiple base addresses. Good point! Thank you! Laszlo
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) {
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.) 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. 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.ni@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@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(-) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel