[PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig

[PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig

[Pedro Falcato]

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

Re: [edk2-devel] [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig

[Laszlo Ersek]

+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

Re: [edk2-devel] [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig

[Pedro Falcato]

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