From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) by mx.groups.io with SMTP id smtpd.web11.71573.1673564049068038647 for ; Thu, 12 Jan 2023 14:54:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=OS2uB2YI; spf=pass (domain: gmail.com, ip: 209.85.214.174, mailfrom: pedro.falcato@gmail.com) Received: by mail-pl1-f174.google.com with SMTP id w3so21692613ply.3 for ; Thu, 12 Jan 2023 14:54:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=FQ1kdrmN3jfg8i0DN9rvzvDXg9cROlVT7qtdYCr/jnM=; b=OS2uB2YIkxRk3d8s1K0T/D1kW5EiFSJsLwk+y7w3Cg10GMLB+qRt5pf8jNqVp1gxpV T8yUXi/e5lvgX1AmdMjt52k1sk9vOBQNFxRuzQg1IIhtzqsKwG8MnfkXp3aOuBAcr4jc aqtM/+tSttDfcbLLel7OTGeOl3MKhFC+XEY7prKk+Cd3LIeuW7byZwo91UJKYf3LX/Mq Jrj0cPhiQEshMcaOLuV0W2D7pkA2LnHdJBSsbwTdsCamZ1RBvb3NBAfwX+HGNqZGqYAf hg6zCMKc9iEcVWaj1OnPtYKD32JGLKN+0YNdNu7ZhanpkMKAvDFK/WZVHsJxUE8A/c/d 4BXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=FQ1kdrmN3jfg8i0DN9rvzvDXg9cROlVT7qtdYCr/jnM=; b=htCHnezbj7gBvifTOjXRvSrk2XMNPxO7FNi7SmqcAGCBk+td3BCy1nbJbvFPJse4Np nzwbohU6CckgLZo26NfaUDt7oO2CBdN+LJP24rS/xejhZ6QZoP3Sobry2tlyDZRQOEsu r0QnQGeVutT15SttGooolcHFxOq9JpHTqBsjfG/ubYM05EIY0VSKZKtob4ipNsxhaLad CshWiFE6odp11bXKKY4iocZnvFr8DcIDLWLHEApRqR4gTemZ4IUr3RMvjl7DWC/SZcuo o5hDkqJhCww9Lu6wEs1MpmlZ+iLck8vGBuxnGuy2zr+WkGwi//AFr3zEgQL4y1MwQu5P N6dw== X-Gm-Message-State: AFqh2kqh/pf3iY8D2YxiLKAYP5hNRN34Bt2nRdAOEev3RgNxkRrRapuj cuMNGyB5sJlxCtGQYaMLJqRFjzFRyyXKGA8MdbE= X-Google-Smtp-Source: AMrXdXvUbpmU8CIKaQFRYf8bESoHWdB5hNp6rdT4iyb8NZ7qUBRTdP408RbgvT3tdoWzDDnj5DsTB/EplzpBzxgbi8c= X-Received: by 2002:a17:90a:b294:b0:229:14a7:4046 with SMTP id c20-20020a17090ab29400b0022914a74046mr231723pjr.44.1673564048481; Thu, 12 Jan 2023 14:54:08 -0800 (PST) MIME-Version: 1.0 References: <20230112160320.411772-1-pedro.falcato@gmail.com> <8f7c9e29-5019-b0f4-7dc3-4336d5457db8@redhat.com> In-Reply-To: <8f7c9e29-5019-b0f4-7dc3-4336d5457db8@redhat.com> From: "Pedro Falcato" Date: Thu, 12 Jan 2023 22:53:57 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig To: Laszlo Ersek Cc: devel@edk2.groups.io, Jian J Wang , Liming Gao , Hao A Wu , Ray Ni , Gerd Hoffmann Content-Type: text/plain; charset="UTF-8" On Thu, Jan 12, 2023 at 4:58 PM Laszlo Ersek 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 > > Cc: Jian J Wang > > Cc: Liming Gao > > Cc: Hao A Wu > > Cc: Ray Ni > > --- > > 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