* [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig @ 2023-01-12 16:03 Pedro Falcato 2023-01-12 16:58 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Pedro Falcato @ 2023-01-12 16:03 UTC (permalink / raw) To: devel; +Cc: Pedro Falcato, Jian J Wang, Liming Gao, Hao A Wu, Ray Ni Virtualization software like QEMU can introduce interesting impossible topologies like PCIe devices on legacy PCI buses. Add a new PCD PcdPciProbePcieConfig to control PCIe extended configuration space probing, given that currently PCIe config space probing can trigger asserts in BasePciCf8Lib, making it hard to use on virtual platforms. Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 + MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 4 ++++ MdeModulePkg/MdeModulePkg.dec | 6 ++++++ 3 files changed, 11 insertions(+) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf index e317169d9c57..c3e52ccd3e2e 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf @@ -107,6 +107,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport ## CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdPciProbePcieConfig ## CONSUMES [UserExtensions.TianoCore."ExtraFiles"] PciBusDxeExtra.uni diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c index ba4b099bc5c1..1c4e7fa23cb5 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c @@ -211,6 +211,10 @@ LocatePciExpressCapabilityRegBlock ( return EFI_UNSUPPORTED; } + if (PcdGetBool (PcdPciProbePcieConfig) == FALSE) { + return EFI_UNSUPPORTED; + } + if (*Offset != 0) { CapabilityPtr = *Offset; } else { diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index be5e829ca9c5..d5005535e8fb 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -2098,6 +2098,12 @@ # @Prompt Enable PCIe Resizable BAR Capability support. gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport|FALSE|BOOLEAN|0x10000024 + ## Indicates if the PCIe configuration space should be probed. + # TRUE - Probe PCIe space for PCIe devices. + # FALSE - Do not touch PCIe configuration space. + # @Prompt Enable PCIe configuration space probing + gEfiMdeModulePkgTokenSpaceGuid.PcdPciProbePcieConfig|FALSE|BOOLEAN|0x10000026 + ## This PCD holds the shared bit mask for page table entries when Tdx is enabled. # @Prompt The shared bit mask when Intel Tdx is enabled. gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0|UINT64|0x10000025 -- 2.39.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig 2023-01-12 16:03 [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig Pedro Falcato @ 2023-01-12 16:58 ` Laszlo Ersek 2023-01-12 22:53 ` Pedro Falcato 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2023-01-12 16:58 UTC (permalink / raw) To: devel, pedro.falcato Cc: Jian J Wang, Liming Gao, Hao A Wu, Ray Ni, Gerd Hoffmann +Gerd; comment below On 1/12/23 17:03, Pedro Falcato wrote: > Virtualization software like QEMU can introduce interesting impossible > topologies like PCIe devices on legacy PCI buses. > > Add a new PCD PcdPciProbePcieConfig to control PCIe extended configuration space > probing, given that currently PCIe config space probing can trigger > asserts in BasePciCf8Lib, making it hard to use on virtual > platforms. > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 + > MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 4 ++++ > MdeModulePkg/MdeModulePkg.dec | 6 ++++++ > 3 files changed, 11 insertions(+) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > index e317169d9c57..c3e52ccd3e2e 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > @@ -107,6 +107,7 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## SOMETIMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdPciProbePcieConfig ## CONSUMES > > [UserExtensions.TianoCore."ExtraFiles"] > PciBusDxeExtra.uni > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c > index ba4b099bc5c1..1c4e7fa23cb5 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c > @@ -211,6 +211,10 @@ LocatePciExpressCapabilityRegBlock ( > return EFI_UNSUPPORTED; > } > > + if (PcdGetBool (PcdPciProbePcieConfig) == FALSE) { > + return EFI_UNSUPPORTED; > + } > + > if (*Offset != 0) { > CapabilityPtr = *Offset; > } else { > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index be5e829ca9c5..d5005535e8fb 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -2098,6 +2098,12 @@ > # @Prompt Enable PCIe Resizable BAR Capability support. > gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport|FALSE|BOOLEAN|0x10000024 > > + ## Indicates if the PCIe configuration space should be probed. > + # TRUE - Probe PCIe space for PCIe devices. > + # FALSE - Do not touch PCIe configuration space. > + # @Prompt Enable PCIe configuration space probing > + gEfiMdeModulePkgTokenSpaceGuid.PcdPciProbePcieConfig|FALSE|BOOLEAN|0x10000026 > + > ## This PCD holds the shared bit mask for page table entries when Tdx is enabled. > # @Prompt The shared bit mask when Intel Tdx is enabled. > gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0|UINT64|0x10000025 (1) This patch should not be necessary. Please refer to commit 014b472053ae ("MdeModulePkg: PciHostBridgeDxe: don't assume extended config space", 2016-03-03). If your PciHostBridgeLib instance correctly sets the NoExtendedConfigSpace flag to YES, when running on QEMU, then your ASSERT() failures should go away. For example, see PciHostBridgeGetRootBridges() in "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c". It delegates most of the work to PciHostBridgeUtilityGetRootBridges() [OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c], but regarding specifically the "NoExtendedConfigSpace" parameter, it passes PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID Thus, on Q35, extended config space will be attempted, while on i440fx, it will be gracefully prevented -- it will be caught before you get an assertion failure in BasePciCf8Lib. (2) Now, in case you try to work with PCI(e) config space *underneath* PciBusDxe (that is, not through the PCI IO protocol, but through one of the "direct" PCI libraries in edk2), *then* you do need to limit the accesses yourself. For example, refer to the following commit range: 1 392a31467f41 OvmfPkg: introduce PciCapLib 2 6a744d40d0c7 OvmfPkg: introduce PciCapPciSegmentLib 3 02b9a8343ffc OvmfPkg: introduce PciCapPciIoLib 4 e9b2cf7bf015 OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib 5 65cefddcdece ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib 6 3815101ff854 OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib 7 5685a243b6f8 OvmfPkg/Virtio10Dxe: convert to PciCapLib This series introduced a new library, PciCapLib, for working with PCI(e) capabilities in a really convenient way, in patch#1. The actual config space access is delegated to a suitable new data structure, called PCI_CAP_DEV. Patch#2 introduced a new library (PciCapPciSegmentLib) for providing *one* kind of PCI_CAP_DEV, namely one backed by the most generic PCI config space access library class that edk2 offers -- PciSegmentLib. PciCapPciSegmentLib makes PciCapLib usable without the PCI IO protocol; that's useful for DXE_DRIVERs for example. (PciBusDxe is a UEFI_DRIVER, it starts in BDS when platform BDS connects the root bridges. So DXE_DRIVER code running in the DXE phase that accesses PCI(e) config space needs a more direct way to access config space.) Patch#3 introduced another new library (PciCapPciIoLib) that provides *another* kind of PCI_CAP_DEV -- this one going through EFI_PCI_IO_PROTOCOL. In Patch#6, the capabilities parsing of PciHotPlugInitDxe was converted to PciCapLib. As PCI_CAP_DEV provider, PciCapPciSegmentLib was used (PciHotPlugInitDxe is a DXE_DRIVER). In Patch#7, the capabilities parsing of Virtio10Dxe was converted to PciCapLib. As PCI_CAP_DEV provider, PciCapPciIoLib was used (Virtio10Dxe is a UEFI_DRIVER following the UEFI driver model that binds suitable EFI_PCI_IO_PROTOCOL anyway). I recommend reading through the commit messages of these commits. Here, I want to highlight the "MaxDomain" parameter from patch#2. If you want to access config space using PciSegmentLib (or one of the more limited PCI lib classes in edk2), then you need to limit the register space manually *before* calling that library. This is what patch#2 enables for the *caller*. And this "MaxDomain" parameter is put to use in patch#6, again accordingly to whether the code runs on i440fx or q35: refer to the "mPciExtConfSpaceSupported" global variable. Summary: - If you consume EFI_PCI_IO_PROTOCOL, you need to detect i440fx yourself, and set "NoExtendedConfigSpace" properly in your PciHostBridgeLib instance. - If you go through the PciLib, PciCf8Lib, PciExpressLib, or PciSegmentLib classes, then you need to detect i440fx yourself, and limit the Register parameter explicitly, in the caller. Therefore, this patch should be unnecessary. Laszlo ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig 2023-01-12 16:58 ` [edk2-devel] " Laszlo Ersek @ 2023-01-12 22:53 ` Pedro Falcato 0 siblings, 0 replies; 3+ messages in thread From: Pedro Falcato @ 2023-01-12 22:53 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, Jian J Wang, Liming Gao, Hao A Wu, Ray Ni, Gerd Hoffmann On Thu, Jan 12, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com> wrote: > > +Gerd; comment below > > On 1/12/23 17:03, Pedro Falcato wrote: > > Virtualization software like QEMU can introduce interesting impossible > > topologies like PCIe devices on legacy PCI buses. > > > > Add a new PCD PcdPciProbePcieConfig to control PCIe extended configuration space > > probing, given that currently PCIe config space probing can trigger > > asserts in BasePciCf8Lib, making it hard to use on virtual > > platforms. > > > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > > Cc: Hao A Wu <hao.a.wu@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 + > > MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 4 ++++ > > MdeModulePkg/MdeModulePkg.dec | 6 ++++++ > > 3 files changed, 11 insertions(+) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > index e317169d9c57..c3e52ccd3e2e 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > @@ -107,6 +107,7 @@ > > gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport ## CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## SOMETIMES_CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport ## CONSUMES > > + gEfiMdeModulePkgTokenSpaceGuid.PcdPciProbePcieConfig ## CONSUMES > > > > [UserExtensions.TianoCore."ExtraFiles"] > > PciBusDxeExtra.uni > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c > > index ba4b099bc5c1..1c4e7fa23cb5 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c > > @@ -211,6 +211,10 @@ LocatePciExpressCapabilityRegBlock ( > > return EFI_UNSUPPORTED; > > } > > > > + if (PcdGetBool (PcdPciProbePcieConfig) == FALSE) { > > + return EFI_UNSUPPORTED; > > + } > > + > > if (*Offset != 0) { > > CapabilityPtr = *Offset; > > } else { > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > > index be5e829ca9c5..d5005535e8fb 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -2098,6 +2098,12 @@ > > # @Prompt Enable PCIe Resizable BAR Capability support. > > gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport|FALSE|BOOLEAN|0x10000024 > > > > + ## Indicates if the PCIe configuration space should be probed. > > + # TRUE - Probe PCIe space for PCIe devices. > > + # FALSE - Do not touch PCIe configuration space. > > + # @Prompt Enable PCIe configuration space probing > > + gEfiMdeModulePkgTokenSpaceGuid.PcdPciProbePcieConfig|FALSE|BOOLEAN|0x10000026 > > + > > ## This PCD holds the shared bit mask for page table entries when Tdx is enabled. > > # @Prompt The shared bit mask when Intel Tdx is enabled. > > gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask|0x0|UINT64|0x10000025 > > (1) This patch should not be necessary. > > Please refer to commit 014b472053ae ("MdeModulePkg: PciHostBridgeDxe: > don't assume extended config space", 2016-03-03). > > If your PciHostBridgeLib instance correctly sets the > NoExtendedConfigSpace flag to YES, when running on QEMU, then your > ASSERT() failures should go away. > > For example, see PciHostBridgeGetRootBridges() in > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c". It delegates most > of the work to PciHostBridgeUtilityGetRootBridges() > [OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c], but > regarding specifically the "NoExtendedConfigSpace" parameter, it passes > > PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID > > Thus, on Q35, extended config space will be attempted, while on i440fx, > it will be gracefully prevented -- it will be caught before you get an > assertion failure in BasePciCf8Lib. > > > (2) Now, in case you try to work with PCI(e) config space *underneath* > PciBusDxe (that is, not through the PCI IO protocol, but through one of > the "direct" PCI libraries in edk2), *then* you do need to limit the > accesses yourself. > > For example, refer to the following commit range: > > 1 392a31467f41 OvmfPkg: introduce PciCapLib > 2 6a744d40d0c7 OvmfPkg: introduce PciCapPciSegmentLib > 3 02b9a8343ffc OvmfPkg: introduce PciCapPciIoLib > 4 e9b2cf7bf015 OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib > 5 65cefddcdece ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib > 6 3815101ff854 OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib > 7 5685a243b6f8 OvmfPkg/Virtio10Dxe: convert to PciCapLib > > This series introduced a new library, PciCapLib, for working with PCI(e) > capabilities in a really convenient way, in patch#1. The actual config > space access is delegated to a suitable new data structure, called > PCI_CAP_DEV. > > Patch#2 introduced a new library (PciCapPciSegmentLib) for providing > *one* kind of PCI_CAP_DEV, namely one backed by the most generic PCI > config space access library class that edk2 offers -- PciSegmentLib. > PciCapPciSegmentLib makes PciCapLib usable without the PCI IO protocol; > that's useful for DXE_DRIVERs for example. (PciBusDxe is a UEFI_DRIVER, > it starts in BDS when platform BDS connects the root bridges. So > DXE_DRIVER code running in the DXE phase that accesses PCI(e) config > space needs a more direct way to access config space.) > > Patch#3 introduced another new library (PciCapPciIoLib) that provides > *another* kind of PCI_CAP_DEV -- this one going through > EFI_PCI_IO_PROTOCOL. > > In Patch#6, the capabilities parsing of PciHotPlugInitDxe was converted > to PciCapLib. As PCI_CAP_DEV provider, PciCapPciSegmentLib was used > (PciHotPlugInitDxe is a DXE_DRIVER). > > In Patch#7, the capabilities parsing of Virtio10Dxe was converted to > PciCapLib. As PCI_CAP_DEV provider, PciCapPciIoLib was used (Virtio10Dxe > is a UEFI_DRIVER following the UEFI driver model that binds suitable > EFI_PCI_IO_PROTOCOL anyway). > > I recommend reading through the commit messages of these commits. > > Here, I want to highlight the "MaxDomain" parameter from patch#2. If you > want to access config space using PciSegmentLib (or one of the more > limited PCI lib classes in edk2), then you need to limit the register > space manually *before* calling that library. This is what patch#2 > enables for the *caller*. > > And this "MaxDomain" parameter is put to use in patch#6, again > accordingly to whether the code runs on i440fx or q35: refer to the > "mPciExtConfSpaceSupported" global variable. > > Summary: > > - If you consume EFI_PCI_IO_PROTOCOL, you need to detect i440fx > yourself, and set "NoExtendedConfigSpace" properly in your > PciHostBridgeLib instance. > > - If you go through the PciLib, PciCf8Lib, PciExpressLib, or > PciSegmentLib classes, then you need to detect i440fx yourself, and > limit the Register parameter explicitly, in the caller. > > Therefore, this patch should be unnecessary. > > Laszlo > Hah. Thank you so much for the insight Laszlo! Turns out I completely overlooked NoExtendedConfigSpace. Maintainers, please drop this patch. Thanks, Pedro ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-12 22:54 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-12 16:03 [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig Pedro Falcato 2023-01-12 16:58 ` [edk2-devel] " Laszlo Ersek 2023-01-12 22:53 ` Pedro Falcato
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox