Re: [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges


Laszlo Ersek
 

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 <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
---
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 <PiDxe.h>

-#include <IndustryStandard/Pci.h>
-#include <IndustryStandard/Q35MchIch9.h>
-
#include <Protocol/PciHostBridgeResourceAllocation.h>
#include <Protocol/PciRootBridgeIo.h>

@@ -20,8 +17,6 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
-#include <Library/PciLib.h>
-#include <Library/QemuFwCfgLib.h>
#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 <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/PciHostBridgeUtilityLib.h>
+#include <Library/PciLib.h>
+#include <Library/QemuFwCfgLib.h>


#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.

Join devel@edk2.groups.io to automatically receive all group messages.