From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.60936.1673542735782723585 for ; Thu, 12 Jan 2023 08:58:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UDuZhmU4; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673542734; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zw9A2KBMbOPXT0XbN9xuTEfapydmdg/R+KxXYryfgVk=; b=UDuZhmU4L5ie/kvg2q6OpkAhuIWvhSMs+PTODCbXpl8sW9rvCrRq55PSc/AzYTH5nOKelt T6fAjIGbTGYdEZ3M8KJ5IH9SQnysswBEXo2NkQTn02o2qmf3rH/QTI5jLLXP3emGovaYHk 4DfwGn9hMsdsFVj8XnWpbvuPku08Fzo= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-564-qsnisqRPMWWlYSNrmQwAFA-1; Thu, 12 Jan 2023 11:58:38 -0500 X-MC-Unique: qsnisqRPMWWlYSNrmQwAFA-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A5ED9280482D; Thu, 12 Jan 2023 16:58:37 +0000 (UTC) Received: from [10.39.192.93] (unknown [10.39.192.93]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9E3CF492B00; Thu, 12 Jan 2023 16:58:34 +0000 (UTC) Message-ID: <8f7c9e29-5019-b0f4-7dc3-4336d5457db8@redhat.com> Date: Thu, 12 Jan 2023 17:58:33 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] PciBusDxe: Add PcdPciProbePcieConfig To: devel@edk2.groups.io, pedro.falcato@gmail.com Cc: Jian J Wang , Liming Gao , Hao A Wu , Ray Ni , Gerd Hoffmann References: <20230112160320.411772-1-pedro.falcato@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20230112160320.411772-1-pedro.falcato@gmail.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit +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