From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.27850.1611151315023719859 for ; Wed, 20 Jan 2021 06:01:55 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KZkLWIVm; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611151314; 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=Haj05rYPw9ym98PxtQx4xyB7BuDMjFJrkrFuxT4v20k=; b=KZkLWIVmmmoUQMeXSq5vDHbSXGGKTVQyA2s6Cg83i7WTbrsYzbvB0emtab+ZZK8flYGGA0 jKqzeE+LmkFtf9Dr0GXcBbdoCmgeU+7nifqTNWQqtc4Yi5KxZRW7G8lR1W5Gv2nrmABwSF 8kvQHpv87jy+5nr8qGxxR0yy8LAjQ1U= 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-585-UYxsXx5CNWyPlv2nMpH2JA-1; Wed, 20 Jan 2021 09:01:49 -0500 X-MC-Unique: UYxsXx5CNWyPlv2nMpH2JA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D497518A078E; Wed, 20 Jan 2021 14:01:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-96.ams2.redhat.com [10.36.115.96]) by smtp.corp.redhat.com (Postfix) with ESMTP id B0BA85D9DD; Wed, 20 Jan 2021 14:01:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v6 09/11] OvmfPkg/PciHostBridgeUtilityLib: Extend GetRootBridges() with BusMin/BusMax 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 References: <20210119011302.10908-1-cenjiahui@huawei.com> <20210119011302.10908-10-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 20 Jan 2021 15:01:43 +0100 MIME-Version: 1.0 In-Reply-To: <20210119011302.10908-10-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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/19/21 02:13, Jiahui Cen via groups.io wrote: > Extend parameter list of PciHostBridgeUtilityGetRootBridges() with BusMin/ > BusMax, so that the utility function could be compatible with ArmVirtPkg > who uses mutable bus range [BusMin, BusMax] insteand of [0, PCI_MAX_BUS]. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059 > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Jiahui Cen > --- > OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 6 ++++ > OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 2 ++ > OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 30 ++++++++++++++++---- > 3 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > index a0ea44d96a67..d2dc18a1afad 100644 > --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > @@ -112,6 +112,10 @@ PciHostBridgeUtilityUninitRootBridge ( > > @param[in] NoExtendedConfigSpace No Extended Config Space. > > + @param[in] BusMin Minimum Bus number, inclusive. > + > + @param[in] BusMax Maximum Bus number, inclusive. > + > @param[in] Io IO aperture. > > @param[in] Mem MMIO aperture. > @@ -132,6 +136,8 @@ PciHostBridgeUtilityGetRootBridges ( > IN UINT64 AllocationAttributes, > IN BOOLEAN DmaAbove4G, > IN BOOLEAN NoExtendedConfigSpace, > + IN UINTN BusMin, > + IN UINTN BusMax, > IN PCI_ROOT_BRIDGE_APERTURE *Io, > IN PCI_ROOT_BRIDGE_APERTURE *Mem, > IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > index 91b9e6baa1e8..7d9fb0fb293a 100644 > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > @@ -85,6 +85,8 @@ PciHostBridgeGetRootBridges ( > 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 1d78984b83ad..69bed5c7843f 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > @@ -200,6 +200,10 @@ PciHostBridgeUtilityUninitRootBridge ( > > @param[in] NoExtendedConfigSpace No Extended Config Space. > > + @param[in] BusMin Minimum Bus number, inclusive. > + > + @param[in] BusMax Maximum Bus number, inclusive. > + > @param[in] Io IO aperture. > > @param[in] Mem MMIO aperture. > @@ -220,6 +224,8 @@ PciHostBridgeUtilityGetRootBridges ( > IN UINT64 AllocationAttributes, > IN BOOLEAN DmaAbove4G, > IN BOOLEAN NoExtendedConfigSpace, > + IN UINTN BusMin, > + IN UINTN BusMax, > IN PCI_ROOT_BRIDGE_APERTURE *Io, > IN PCI_ROOT_BRIDGE_APERTURE *Mem, > IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > @@ -236,6 +242,13 @@ PciHostBridgeUtilityGetRootBridges ( > UINTN LastRootBridgeNumber; > UINTN RootBridgeNumber; > > + if (BusMin > BusMax || BusMax > PCI_MAX_BUS) { > + DEBUG ((DEBUG_ERROR, "%a: invalid bus range with BusMin %d and BusMax %d\n", > + __FUNCTION__, BusMin, BusMax)); (1) The "%d" format specifier is appropriate for INT32 values. But UINTN is either UINT32 or UINT64 (dependent on the edk2 architecture the platform is built for). The safe way to print a UINTN value is to (a) cast it to UINT64, and (b) use the "%Lu" (or "%Lx") format specifier. I'll fix this up for you. > + *Count = 0; (2) As I said under patch v6 07/11, this zeroing should be centralized at the top of the function. I'll fix it up for you. > + return NULL; > + } > + > // > // QEMU provides the number of extra root buses, shortening the exhaustive > // search below. If there is no hint, the feature is missing. > @@ -247,7 +260,14 @@ PciHostBridgeUtilityGetRootBridges ( > QemuFwCfgSelectItem (FwCfgItem); > QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges); > > - if (ExtraRootBridges > PCI_MAX_BUS) { > + // > + // Validate the number of extra root bridges. As BusMax is inclusive, the > + // max bus count is (BusMax - BusMin + 1). From that, the "main" root bus > + // is always givin, so the max count for the "extra" root bridges is one (3) s/givin/a given/ I'll fix it. > + // less, i.e. (BusMax - BusMin). If QEME hint exceeds that, we have invalid (4) s/QEME/the QEMU/ I'll fix it. > + // behavior. > + // > + if (ExtraRootBridges > BusMax - BusMin) { > DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) " > "reported by QEMU\n", __FUNCTION__, ExtraRootBridges)); > *Count = 0; > @@ -271,15 +291,15 @@ PciHostBridgeUtilityGetRootBridges ( > // > // The "main" root bus is always there. > // > - LastRootBridgeNumber = 0; > + LastRootBridgeNumber = BusMin; Side remark (nothing to fix): I'm slightly curious what would happen if the DTB on the arm/aarch64 "virt" machine reported a BusMin different from 0. What does that mean for the MMCONFIG offsets? If BusMin is nonzero, does that mean we have a unused slice at the start of MMCONFIG? Or does it mean that we need to shift down the bus references so that BusMin does map to offset 0 in MMCONFIG? (See also the "PCI_ROOT_BRIDGE.Bus.Translation" field.) Anyway, we can look at this later, if such a DTB ever occurs in practice. (I doubt that it ever will.) > > // > // 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; > > @@ -329,7 +349,7 @@ PciHostBridgeUtilityGetRootBridges ( > DmaAbove4G, > NoExtendedConfigSpace, > (UINT8) LastRootBridgeNumber, > - PCI_MAX_BUS, > + (UINT8) BusMax, > Io, > Mem, > MemAbove4G, > Reviewed-by: Laszlo Ersek Thanks Laszlo