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 <jordan.l.jus...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ard Biesheuvel <ard.biesheu...@arm.com> > Signed-off-by: Jiahui Cen <cenjia...@huawei.com> > Signed-off-by: Yubo Miao <miaoy...@huawei.com> > --- > 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 <PiDxe.h> > > -#include <IndustryStandard/Pci.h> > -#include <IndustryStandard/Q35MchIch9.h> > - > #include <Protocol/PciHostBridgeResourceAllocation.h> > #include <Protocol/PciRootBridgeIo.h> > > @@ -21,8 +18,6 @@ > #include <Library/PcdLib.h> > #include <Library/PciHostBridgeLib.h> > #include <Library/PciHostBridgeUtilityLib.h> > -#include <Library/PciLib.h> > -#include <Library/QemuFwCfgLib.h> > #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 <IndustryStandard/Acpi10.h> > +#include <IndustryStandard/Pci.h> > +#include <IndustryStandard/Q35MchIch9.h> > #include <Library/BaseMemoryLib.h> > #include <Library/DebugLib.h> > #include <Library/DevicePathLib.h> > #include <Library/MemoryAllocationLib.h> > +#include <Library/PcdLib.h> > #include <Library/PciHostBridgeUtilityLib.h> > +#include <Library/PciLib.h> > +#include <Library/QemuFwCfgLib.h> > > > #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 <ler...@redhat.com> 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. > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70578): https://edk2.groups.io/g/devel/message/70578 Mute This Topic: https://groups.io/mt/79941626/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-