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.web09.5522.1610615559290329608 for ; Thu, 14 Jan 2021 01:12:39 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=L5QR4aMd; 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=1610615558; 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=3eu2ScN3SGEPVVhd7X4OmfVqYZcHDW51GbFrXDe2QyI=; b=L5QR4aMdR2FcwiQJ1QFjiKastiKHBdhYodk9yEn0G+wvdQgApI+7vts/dEwzcaOCg0UUfw ER24os760XWAq4+qbd/xIqMB/AxUpUuyX+afKyvX22GHEi/tUFS4IYKDrdvwIMflxegGgm F8QdZUPDdUJlURzERPfIbTxv4zqd9ak= 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-439-QD3oWHAPNxOwOY-8nvWDCQ-1; Thu, 14 Jan 2021 04:12:34 -0500 X-MC-Unique: QD3oWHAPNxOwOY-8nvWDCQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 291081015C80; Thu, 14 Jan 2021 09:12:32 +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 E84196F981; Thu, 14 Jan 2021 09:12:28 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 6/9] 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: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-7-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: <099f52ec-5d52-c793-858b-75dfae8f3aa4@redhat.com> Date: Thu, 14 Jan 2021 10:12:27 +0100 MIME-Version: 1.0 In-Reply-To: <20210112094549.10238-7-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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: > Extract PciHostBridgeGetRootBridges() / PciHostBridgeFreeRootBridges() to > PciHostBridgeUtilityLib as common utility functions. > > 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 | 5 + > OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 50 +++++ > OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 136 +------------- > OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 193 ++++++++++++++++++++ > 5 files changed, 252 insertions(+), 133 deletions(-) > > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > index 463c05c94b07..93ba440c9009 100644 > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > @@ -40,7 +40,6 @@ [LibraryClasses] > MemoryAllocationLib > PciHostBridgeUtilityLib > PciLib > - QemuFwCfgLib > > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase > diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > index 4d6764b702f4..4cdf367ffee2 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf > @@ -39,3 +39,8 @@ [LibraryClasses] > DebugLib > DevicePathLib > MemoryAllocationLib > + PciLib > + QemuFwCfgLib > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId (1) We'll have to re-add the PcdLib dependency then (#include too). > diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > index 13c591641d7a..29ea19f2d7e5 100644 > --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h > @@ -97,6 +97,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 ( > + UINTN *Count, > + UINT64 Attributes, > + UINT64 AllocationAttributes, > + PCI_ROOT_BRIDGE_APERTURE *Io, > + PCI_ROOT_BRIDGE_APERTURE *Mem, > + PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > + PCI_ROOT_BRIDGE_APERTURE *PMem, > + PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G > + ); (2) You missed annotating the parameters as IN / OUT (should be in sync with the @param[...] comments above). Don't forget to sync the C file too. > + > + > +/** > + 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 ( > + PCI_ROOT_BRIDGE *Bridges, > + UINTN Count > + ); > + (3) You missed annotating the parameters as IN / OUT (should be in sync with the @param[...] comments above). Don't forget to sync the C file too. > + > /** > 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 d9c057021598..0a1d94a29bf3 100644 > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > @@ -9,9 +9,6 @@ > **/ > #include > > -#include > -#include > - > #include > #include > > @@ -20,8 +17,6 @@ > #include > #include > #include > -#include > -#include > #include "PciHostBridge.h" > > > @@ -43,14 +38,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; > @@ -90,121 +77,16 @@ PciHostBridgeGetRootBridges ( > > *Count = 0; (4) Please move this assignment as well into the extracted function. Thanks Laszlo > > - // > - // QEMU provides the number of extra root buses, shortening the exhaustive > - // search below. If there is no hint, the feature is missing. > - // > - 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; > } > > > @@ -222,17 +104,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 9101aedc8888..629ebcd7a5be 100644 > --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c > @@ -11,11 +11,15 @@ > **/ > > #include > +#include > +#include > #include > #include > #include > #include > #include > +#include > +#include > > > #pragma pack(1) > @@ -182,6 +186,195 @@ 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 ( > + UINTN *Count, > + UINT64 Attributes, > + UINT64 AllocationAttributes, > + PCI_ROOT_BRIDGE_APERTURE *Io, > + PCI_ROOT_BRIDGE_APERTURE *Mem, > + PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > + PCI_ROOT_BRIDGE_APERTURE *PMem, > + 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. > + // > + 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, > + 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); > + 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 ( > + PCI_ROOT_BRIDGE *Bridges, > + 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. > >