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.web12.27524.1611148086019424434 for ; Wed, 20 Jan 2021 05:08:06 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=INu/Y/HT; 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=1611148085; 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=Vx10ZaIueTeO5W8eL7/tOA1r7XVRI+n7yWzR2yybzUk=; b=INu/Y/HTAlrFA8Xyio2n9kHi/APTeJ3CklVSjiHGBnW71ofjNzaBdRtcygCqh5NC1okwxm 8zC6urtEei67kL6j0FES43LsbhAYhSC/Phs42zVUp/k9LmBTgZyIZT8TeB1GcHsYvDmqr9 g1JAZwMh3r4lxQkiIvVKep1+/aIeCec= 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-384-qkK1DGL3NeeYuWem_SCUCw-1; Wed, 20 Jan 2021 08:08:00 -0500 X-MC-Unique: qkK1DGL3NeeYuWem_SCUCw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B63FD10054FF; Wed, 20 Jan 2021 13:07:58 +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 60B0418226; Wed, 20 Jan 2021 13:07:55 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v6 07/11] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges() / FreeRootBridges() 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: <20210119011302.10908-1-cenjiahui@huawei.com> <20210119011302.10908-8-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: <956db5de-f0e6-f1e9-df24-5954f4c9d983@redhat.com> Date: Wed, 20 Jan 2021 14:07:54 +0100 MIME-Version: 1.0 In-Reply-To: <20210119011302.10908-8-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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:12, Jiahui Cen via groups.io wrote: > Extract PciHostBridgeGetRootBridges() / PciHostBridgeFreeRootBridges() to > PciHostBridgeUtilityLib as common utility functions to share support for > scanning extra root bridges. > > No change of functionality. > > 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/PciHostBridgeLib/PciHostBridgeLib.inf | 1 - > OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 6 + > OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 50 +++++ > OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 138 +------------- > OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 197 ++++++++++++++++++++ > 5 files changed, 257 insertions(+), 135 deletions(-) > > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > index 72458262cb42..4610a0c1490b 100644 > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > @@ -41,7 +41,6 @@ [LibraryClasses] > PcdLib > PciHostBridgeUtilityLib > PciLib > - QemuFwCfgLib > > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase > diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > index 4d6764b702f4..fdae8cfe872e 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > @@ -39,3 +39,9 @@ [LibraryClasses] > DebugLib > DevicePathLib > MemoryAllocationLib > + PcdLib > + PciLib > + QemuFwCfgLib > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > index a44ad5034520..2b7d5d3725c3 100644 > --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > @@ -99,6 +99,56 @@ PciHostBridgeUtilityUninitRootBridge ( > ); > > > +/** > + Utility function to return all the root bridge instances in an array. > + > + @param[out] Count The number of root bridge instances. > + > + @param[in] Attributes Initial attributes. > + > + @param[in] AllocAttributes Allocation attributes. > + > + @param[in] Io IO aperture. > + > + @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 > +PciHostBridgeUtilityGetRootBridges ( > + OUT UINTN *Count, > + IN UINT64 Attributes, > + IN UINT64 AllocationAttributes, > + IN PCI_ROOT_BRIDGE_APERTURE *Io, > + IN PCI_ROOT_BRIDGE_APERTURE *Mem, > + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > + IN PCI_ROOT_BRIDGE_APERTURE *PMem, > + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G > + ); > + > + > +/** > + Utility function to free root bridge instances array from > + PciHostBridgeUtilityGetRootBridges(). > + > + @param[in] Bridges The root bridge instances array. > + @param[in] Count The count of the array. > +**/ > +VOID > +EFIAPI > +PciHostBridgeUtilityFreeRootBridges ( > + IN PCI_ROOT_BRIDGE *Bridges, > + IN UINTN Count > + ); > + > + > /** > Utility function to inform the platform that the resource conflict happens. > > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > index 8758d7c12bf0..6ac41ff853a9 100644 > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > @@ -9,9 +9,6 @@ > **/ > #include > > -#include > -#include > - > #include > #include > > @@ -21,8 +18,6 @@ > #include > #include > #include > -#include > -#include > #include "PciHostBridge.h" > > > @@ -44,14 +39,6 @@ PciHostBridgeGetRootBridges ( > UINTN *Count > ) > { > - EFI_STATUS Status; > - FIRMWARE_CONFIG_ITEM FwCfgItem; > - UINTN FwCfgSize; > - UINT64 ExtraRootBridges; > - PCI_ROOT_BRIDGE *Bridges; > - UINTN Initialized; > - UINTN LastRootBridgeNumber; > - UINTN RootBridgeNumber; > UINT64 Attributes; > UINT64 AllocationAttributes; > PCI_ROOT_BRIDGE_APERTURE Io; > @@ -89,123 +76,16 @@ PciHostBridgeGetRootBridges ( > Mem.Base = PcdGet64 (PcdPciMmio32Base); > Mem.Limit = PcdGet64 (PcdPciMmio32Base) + (PcdGet64 (PcdPciMmio32Size) - 1); > > - *Count = 0; > - > - // > - // QEMU provides the number of extra root buses, shortening the exhaustive > - // search below. If there is no hint, the feature is missing. > - // (1a) In v6, you are now removing the assignment to (*Count) here, as I asked under v4, but: > - Status = QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize); > - if (EFI_ERROR (Status) || FwCfgSize != sizeof ExtraRootBridges) { > - ExtraRootBridges = 0; > - } else { > - QemuFwCfgSelectItem (FwCfgItem); > - QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges); > - > - if (ExtraRootBridges > PCI_MAX_BUS) { > - DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) " > - "reported by QEMU\n", __FUNCTION__, ExtraRootBridges)); > - return NULL; > - } > - DEBUG ((DEBUG_INFO, "%a: %Lu extra root buses reported by QEMU\n", > - __FUNCTION__, ExtraRootBridges)); > - } > - > - // > - // Allocate the "main" root bridge, and any extra root bridges. > - // > - Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges); > - if (Bridges == NULL) { > - DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES)); > - return NULL; > - } > - Initialized = 0; > - > - // > - // The "main" root bus is always there. > - // > - LastRootBridgeNumber = 0; > - > - // > - // 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; > - ++RootBridgeNumber) { > - UINTN Device; > - > - for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) { > - if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0, > - PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) { > - break; > - } > - } > - if (Device <= PCI_MAX_DEVICE) { > - // > - // Found the next root bus. We can now install the *previous* one, > - // because now we know how big a bus number range *that* one has, for any > - // subordinate buses that might exist behind PCI bridges hanging off it. > - // > - Status = PciHostBridgeUtilityInitRootBridge ( > - Attributes, > - Attributes, > - AllocationAttributes, > - FALSE, > - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > - (UINT8) LastRootBridgeNumber, > - (UINT8) (RootBridgeNumber - 1), > - &Io, > - &Mem, > - &MemAbove4G, > - &mNonExistAperture, > - &mNonExistAperture, > - &Bridges[Initialized] > - ); > - if (EFI_ERROR (Status)) { > - goto FreeBridges; > - } > - ++Initialized; > - LastRootBridgeNumber = RootBridgeNumber; > - } > - } > - > - // > - // Install the last root bus (which might be the only, ie. main, root bus, if > - // we've found no extra root buses). > - // > - Status = PciHostBridgeUtilityInitRootBridge ( > - Attributes, > + return PciHostBridgeUtilityGetRootBridges ( > + Count, > Attributes, > AllocationAttributes, > - FALSE, > - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > - (UINT8) LastRootBridgeNumber, > - PCI_MAX_BUS, > &Io, > &Mem, > &MemAbove4G, > &mNonExistAperture, > - &mNonExistAperture, > - &Bridges[Initialized] > + &mNonExistAperture > ); > - if (EFI_ERROR (Status)) { > - goto FreeBridges; > - } > - ++Initialized; > - > - *Count = Initialized; > - return Bridges; > - > -FreeBridges: > - while (Initialized > 0) { > - --Initialized; > - PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]); > - } > - > - FreePool (Bridges); > - return NULL; > } > > > @@ -223,17 +103,7 @@ PciHostBridgeFreeRootBridges ( > UINTN Count > ) > { > - if (Bridges == NULL && Count == 0) { > - return; > - } > - ASSERT (Bridges != NULL && Count > 0); > - > - do { > - --Count; > - PciHostBridgeUtilityUninitRootBridge (&Bridges[Count]); > - } while (Count > 0); > - > - FreePool (Bridges); > + PciHostBridgeUtilityFreeRootBridges (Bridges, Count); > } > > > diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > index bed2d87ea89c..b1e74a469d50 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > @@ -11,11 +11,16 @@ > **/ > > #include > +#include > +#include > #include > #include > #include > #include > +#include > #include > +#include > +#include > > > #pragma pack(1) > @@ -184,6 +189,198 @@ PciHostBridgeUtilityUninitRootBridge ( > } > > > +/** > + Utility function to return all the root bridge instances in an array. > + > + @param[out] Count The number of root bridge instances. > + > + @param[in] Attributes Initial attributes. > + > + @param[in] AllocAttributes Allocation attributes. > + > + @param[in] Io IO aperture. > + > + @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 > +PciHostBridgeUtilityGetRootBridges ( > + OUT UINTN *Count, > + IN UINT64 Attributes, > + IN UINT64 AllocationAttributes, > + IN PCI_ROOT_BRIDGE_APERTURE *Io, > + IN PCI_ROOT_BRIDGE_APERTURE *Mem, > + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > + IN PCI_ROOT_BRIDGE_APERTURE *PMem, > + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G > + ) > +{ > + EFI_STATUS Status; > + FIRMWARE_CONFIG_ITEM FwCfgItem; > + UINTN FwCfgSize; > + UINT64 ExtraRootBridges; > + PCI_ROOT_BRIDGE *Bridges; > + UINTN Initialized; > + UINTN LastRootBridgeNumber; > + UINTN RootBridgeNumber; > + > + // > + // QEMU provides the number of extra root buses, shortening the exhaustive > + // search below. If there is no hint, the feature is missing. > + // (1b) you aren't introducing the assignment to the new function in the same spot (above the comment). Instead, the assignment is now triplicated to the "return NULL" statements: > + Status = QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize); > + if (EFI_ERROR (Status) || FwCfgSize != sizeof ExtraRootBridges) { > + ExtraRootBridges = 0; > + } else { > + QemuFwCfgSelectItem (FwCfgItem); > + QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges); > + > + if (ExtraRootBridges > PCI_MAX_BUS) { > + DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) " > + "reported by QEMU\n", __FUNCTION__, ExtraRootBridges)); > + *Count = 0; (1c) here > + return NULL; > + } > + DEBUG ((DEBUG_INFO, "%a: %Lu extra root buses reported by QEMU\n", > + __FUNCTION__, ExtraRootBridges)); > + } > + > + // > + // Allocate the "main" root bridge, and any extra root bridges. > + // > + Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges); > + if (Bridges == NULL) { > + DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES)); > + *Count = 0; (1d) here > + return NULL; > + } > + Initialized = 0; > + > + // > + // The "main" root bus is always there. > + // > + LastRootBridgeNumber = 0; > + > + // > + // 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; > + ++RootBridgeNumber) { > + UINTN Device; > + > + for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) { > + if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0, > + PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) { > + break; > + } > + } > + if (Device <= PCI_MAX_DEVICE) { > + // > + // Found the next root bus. We can now install the *previous* one, > + // because now we know how big a bus number range *that* one has, for any > + // subordinate buses that might exist behind PCI bridges hanging off it. > + // > + Status = PciHostBridgeUtilityInitRootBridge ( > + Attributes, > + Attributes, > + AllocationAttributes, > + FALSE, > + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > + (UINT8) LastRootBridgeNumber, > + (UINT8) (RootBridgeNumber - 1), > + Io, > + Mem, > + MemAbove4G, > + PMem, > + PMemAbove4G, > + &Bridges[Initialized] > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBridges; > + } > + ++Initialized; > + LastRootBridgeNumber = RootBridgeNumber; > + } > + } > + > + // > + // Install the last root bus (which might be the only, ie. main, root bus, if > + // we've found no extra root buses). > + // > + Status = PciHostBridgeUtilityInitRootBridge ( > + Attributes, > + Attributes, > + AllocationAttributes, > + FALSE, > + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID, > + (UINT8) LastRootBridgeNumber, > + PCI_MAX_BUS, > + Io, > + Mem, > + MemAbove4G, > + PMem, > + PMemAbove4G, > + &Bridges[Initialized] > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBridges; > + } > + ++Initialized; > + > + *Count = Initialized; > + return Bridges; > + > +FreeBridges: > + while (Initialized > 0) { > + --Initialized; > + PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]); > + } > + > + FreePool (Bridges); > + *Count = 0; (1e) and here. While functionally that's OK, it needlessly complicates the comparison between the old and the new function bodies. Considering that the subject of this patch is code extraction, I don't think such a change is warranted for. I can't see myself reviewing a v7 of this series (unless I find something critical in the rest of v6), so I'll try to centralize the zero-assignment to (*Count) for you, at the top of the function, before I merge v6. The other updates look OK. Reviewed-by: Laszlo Ersek Laszlo > + return NULL; > +} > + > + > +/** > + Utility function to free root bridge instances array from > + PciHostBridgeUtilityGetRootBridges(). > + > + @param[in] Bridges The root bridge instances array. > + @param[in] Count The count of the array. > +**/ > +VOID > +EFIAPI > +PciHostBridgeUtilityFreeRootBridges ( > + IN PCI_ROOT_BRIDGE *Bridges, > + IN UINTN Count > + ) > +{ > + if (Bridges == NULL && Count == 0) { > + return; > + } > + ASSERT (Bridges != NULL && Count > 0); > + > + do { > + --Count; > + PciHostBridgeUtilityUninitRootBridge (&Bridges[Count]); > + } while (Count > 0); > + > + FreePool (Bridges); > +} > + > + > /** > Utility function to inform the platform that the resource conflict happens. > >