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

["Pedro Falcato" <pedro.falcato@gmail.com>]

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