[edk2-devel] [PATCH v6 07/11] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges() / FreeRootBridges()
Laszlo Ersek
lersek at redhat.com
Wed Jan 20 13:07:54 UTC 2021
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.justen at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> Signed-off-by: Jiahui Cen <cenjiahui at huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo at 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 <lersek at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list