From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web10.98.1610621181952230932 for ; Thu, 14 Jan 2021 02:46:22 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=C0esNg9u; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610621180; 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=qJ6rYk97LRbfThY5oNSYHX2MW4RfMgcEGBPPZtyO2vQ=; b=C0esNg9uOfKFcmAC8kjFjnNpTLqi58cTWnemUb3OjPEuXOG0PDYvSJAPuvcUgA/9pVijMq 6yTp1/DpXCO0YLtU5NHBB0JaUvu+h35CaoFm7T271RGLB1ZBwL/1ZH0og19batiiqjrW6Y BNWbdqVs/fmIoKWuQzLo28ISXdhNNqM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-32-z8y-XC7RMpiq95pHouHrkQ-1; Thu, 14 Jan 2021 05:46:16 -0500 X-MC-Unique: z8y-XC7RMpiq95pHouHrkQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4230080059D; Thu, 14 Jan 2021 10:46:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-110.ams2.redhat.com [10.36.114.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 72AAD12D7E; Thu, 14 Jan 2021 10:46:09 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges To: devel@edk2.groups.io, cenjiahui@huawei.com Cc: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , Julien Grall , Leif Lindholm , Sami Mujawar , xieyingtai@huawei.com, wu.wubin@huawei.com, Yubo Miao References: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-8-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: <28c4d432-f281-91dc-1b93-faf67297f181@redhat.com> Date: Thu, 14 Jan 2021 11:46:08 +0100 MIME-Version: 1.0 In-Reply-To: <20210112094549.10238-8-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/12/21 10:45, Jiahui Cen via groups.io wrote: > Extend parameter list of PciHostBridgeUtilityGetRootBridges() with > DmaAbove4G, NoExtendedConfigSpace, BusMin and BusMax to support for > ArmVirtPkg. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059 > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Jiahui Cen > Signed-off-by: Yubo Miao > --- > OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 -- > OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 30 +++++++++---- > OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 7 +++ > OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 47 ++++++++++++-------- > 4 files changed, 57 insertions(+), 30 deletions(-) > > diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > index 4cdf367ffee2..83a734c1725e 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > @@ -41,6 +41,3 @@ [LibraryClasses] > MemoryAllocationLib > PciLib > QemuFwCfgLib > - > -[Pcd] > - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > index 29ea19f2d7e5..ed2b386001e1 100644 > --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > @@ -100,23 +100,31 @@ PciHostBridgeUtilityUninitRootBridge ( > /** > Utility function to return all the root bridge instances in an array. > > - @param[out] Count The number of root bridge instances. > + @param[out] Count The number of root bridge instances. > > - @param[in] Attributes Initial attributes. > + @param[in] Attributes Initial attributes. > > - @param[in] AllocAttributes Allocation attributes. > + @param[in] AllocAttributes Allocation attributes. > > - @param[in] Io IO aperture. > + @param[in] DmaAbove4G DMA above 4GB memory. > > - @param[in] Mem MMIO aperture. > + @param[in] NoExtendedConfigSpace No Extended Config Space. > > - @param[in] MemAbove4G MMIO aperture above 4G. > + @param[in] BusMin Minimum Bus number > > - @param[in] PMem Prefetchable MMIO aperture. > + @param[in] BusMax Maximum Bus number > > - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G. > + @param[in] Io IO aperture. > > - @return All the root bridge instances in an array. > + @param[in] Mem MMIO aperture. > + > + @param[in] MemAbove4G MMIO aperture above 4G. > + > + @param[in] PMem Prefetchable MMIO aperture. > + > + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G. > + > + @return All the root bridge instances in an array. > **/ > PCI_ROOT_BRIDGE * > EFIAPI > @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges ( > UINTN *Count, > UINT64 Attributes, > UINT64 AllocationAttributes, > + BOOLEAN DmaAbove4G, > + BOOLEAN NoExtendedConfigSpace, > + UINT32 BusMin, > + UINT32 BusMax, > PCI_ROOT_BRIDGE_APERTURE *Io, > PCI_ROOT_BRIDGE_APERTURE *Mem, > PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, (1) You forgot to annotate the new params with IN. (Also update the C file please, in sync.) (2) The BusMin / BusMax addition must absolutely be a separate patch, so that we can discuss and review it separately. It's not a simple data propagation change -- it generalizes the function internally. (3) BusMax should be documented as an inclusive maximum (but see more on this below). (4) I don't understand where the UINT32 type for BusMin / BusMax comes from. PciHostBridgeUtilityInitRootBridge() takes UINT8 bus numbers (which makes sense). And the scanning uses UINTN values, see e.g. RootBridgeNumber, which also makes sense (for convenience). UINT32 matches neither. It's not necessarily wrong, but confusing. ... if you chose UINT32 because ProcessPciHost() [ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c] outputs BusMin and BusMax as UINT32, then please use UINTN instead. > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > index 0a1d94a29bf3..28ad32752cab 100644 > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > @@ -9,6 +9,9 @@ > **/ > #include > > +#include > +#include > + > #include > #include > > @@ -81,6 +84,10 @@ PciHostBridgeGetRootBridges ( > Count, > Attributes, > AllocationAttributes, > + FALSE, > + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > + 0, > + PCI_MAX_BUS, > &Io, > &Mem, > &MemAbove4G, > diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > index 629ebcd7a5be..fd2f54a139e2 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > @@ -12,7 +12,6 @@ > > #include > #include > -#include > #include > #include > #include > @@ -189,23 +188,31 @@ PciHostBridgeUtilityUninitRootBridge ( > /** > Utility function to return all the root bridge instances in an array. > > - @param[out] Count The number of root bridge instances. > + @param[out] Count The number of root bridge instances. > > - @param[in] Attributes Initial attributes. > + @param[in] Attributes Initial attributes. > > - @param[in] AllocAttributes Allocation attributes. > + @param[in] AllocAttributes Allocation attributes. > > - @param[in] Io IO aperture. > + @param[in] DmaAbove4G DMA above 4GB memory. > > - @param[in] Mem MMIO aperture. > + @param[in] NoExtendedConfigSpace No Extended Config Space. > > - @param[in] MemAbove4G MMIO aperture above 4G. > + @param[in] BusMin Minimum Bus number > > - @param[in] PMem Prefetchable MMIO aperture. > + @param[in] BusMax Maximum Bus number > > - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G. > + @param[in] Io IO aperture. > > - @return All the root bridge instances in an array. > + @param[in] Mem MMIO aperture. > + > + @param[in] MemAbove4G MMIO aperture above 4G. > + > + @param[in] PMem Prefetchable MMIO aperture. > + > + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G. > + > + @return All the root bridge instances in an array. > **/ > PCI_ROOT_BRIDGE * > EFIAPI > @@ -213,6 +220,10 @@ PciHostBridgeUtilityGetRootBridges ( > UINTN *Count, > UINT64 Attributes, > UINT64 AllocationAttributes, > + BOOLEAN DmaAbove4G, > + BOOLEAN NoExtendedConfigSpace, > + UINT32 BusMin, > + UINT32 BusMax, > PCI_ROOT_BRIDGE_APERTURE *Io, > PCI_ROOT_BRIDGE_APERTURE *Mem, > PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > @@ -240,7 +251,7 @@ PciHostBridgeUtilityGetRootBridges ( > QemuFwCfgSelectItem (FwCfgItem); > QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges); > > - if (ExtraRootBridges > PCI_MAX_BUS) { > + if (ExtraRootBridges > BusMax - BusMin) { > DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) " > "reported by QEMU\n", __FUNCTION__, ExtraRootBridges)); > return NULL; (5) The code change looks valid, but please add a comment here (in the patch dedicated to the bus numbers, that is). Because BusMax is inclusive, the max bus count is (BusMax - BusMin + 1). From that, the "main" root bus is is a given, so the max count for the "extra" root bridges is one less, i.e. (BusMax - BusMin). If the QEMU hint exceeds that, we have invalid behavior. (6) In the patch that will deal with the bus numbers exlusively, please add a sanity check near the top of the function: BusMin > BusMax || BusMax > PCI_MAX_BUS If this condition evaluates to TRUE, the function should set (*Count) to 0, and return NULL, at once. > @@ -262,15 +273,15 @@ PciHostBridgeUtilityGetRootBridges ( > // > // The "main" root bus is always there. > // > - LastRootBridgeNumber = 0; > + LastRootBridgeNumber = BusMin; > > // > // Scan all other root buses. If function 0 of any device on a bus returns a > // VendorId register value different from all-bits-one, then that bus is > // alive. > // > - for (RootBridgeNumber = 1; > - RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges; > + for (RootBridgeNumber = BusMin + 1; > + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges; > ++RootBridgeNumber) { > UINTN Device; > > @@ -290,8 +301,8 @@ PciHostBridgeUtilityGetRootBridges ( > Attributes, > Attributes, > AllocationAttributes, > - FALSE, > - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > + DmaAbove4G, > + NoExtendedConfigSpace, > (UINT8) LastRootBridgeNumber, > (UINT8) (RootBridgeNumber - 1), > Io, > @@ -317,8 +328,8 @@ PciHostBridgeUtilityGetRootBridges ( > Attributes, > Attributes, > AllocationAttributes, > - FALSE, > - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > + DmaAbove4G, > + NoExtendedConfigSpace, > (UINT8) LastRootBridgeNumber, > PCI_MAX_BUS, > Io, > (7) You missed replacing PCI_MAX_BUS with BusMax here. (But it belongs in the separate patch that will deal with the bus numbers, and only with the bus numbers.) ... Which in turn makes me ask you to please test your changes more carefully. I believe this bug here is actually shown in the firmware debug log. Namely, the "virt" machine type only supports buses 0x0..0xf, inclusive (if I remember correctly), because its MMCONFIG space is quite limited. Now, assume the common case, with the "virt" machine type, where you don't add any pxb devices to the QEMU cmdline -- just go with the main bus (bus#0). With this bug, the "max sub bus number" on bus#0 goes from 0xf to 0xff. I'm fairly sure that change is visible in the messages that are logged by PciHostBridgeDxe. You're supposed to regression test the previous use case with the patched code -- there should be no change in behavior. Comparing the before-after logs is one of the checks someone should do for this. ... I've found the bus number stuff in this patch so distracting that I'm actually not capable of reviewing the patch wrt. its "original" purpose, namely the exposure of the DmaAbove4G and NoExtendedConfigSpace parameters. I'll do that in v6, once you have split this patch in two. Laszlo