[edk2-devel] [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots
Laszlo Ersek
lersek at redhat.com
Wed Jan 6 10:27:56 UTC 2021
On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> Extract functions that support extra pci roots from
> OvmfPkg/PciHostBridgeLib to share this feature with other packages.
>
> 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 | 2 -
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 4 +
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 58 ++++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +--------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 212 ++++++++++++++++++++
> 5 files changed, 285 insertions(+), 149 deletions(-)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index 4c56f3c90b3b..8bdb76899111 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -40,8 +40,6 @@ [LibraryClasses]
> DevicePathLib
> MemoryAllocationLib
> PciHostBridgeUtilityLib
> - PciLib
(1) Removing the PciLib class dependency is incorrect. The
"XenSupport.c" file continues using PciRead32(), for example.
> - QemuFwCfgLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> index 1ba8ec3e03c7..a10afbe30c6b 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -30,8 +30,12 @@ [Sources]
> PciHostBridgeUtilityLib.c
>
> [Packages]
> + MdeModulePkg/MdeModulePkg.dec
(2) Why is this needed? Both the MemoryAllocationLib and PciLib class
headers are in MdePkg:
MdePkg/Include/Library/MemoryAllocationLib.h
MdePkg/Include/Library/PciLib.h
... Ah, wait, I see it. This is needed because, unfortunately, we cannot
avoid a dependency on the PCI_ROOT_BRIDGE type, from file
MdeModulePkg/Include/Library/PciHostBridgeLib.h
OK then.
> MdePkg/MdePkg.dec
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> DebugLib
> + MemoryAllocationLib
> + PciLib
> + QemuFwCfgLib
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> index f932d412aa10..1d1c86c69064 100644
> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -14,6 +14,64 @@
> #define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>
>
> +#include <Library/PciHostBridgeLib.h>
> +
> +
> +/**
> + Utility function to free root bridge instances array.
> +
> + @param The root bridge instances array.
> + @param The count of the array.
> +**/
> +VOID
> +EFIAPI
> +PciHostBridgeUtilityFreeRootBridges (
> + PCI_ROOT_BRIDGE *Bridges,
> + UINTN Count
> + );
> +
> +
> +/**
> + Utility function to return all the root bridge instances in an array.
> +
> + @param Count The number of root bridge instances.
> +
> + @param[in] BusMin The min bus number.
> +
> + @param[in] BusMax The max bus number.
> +
> + @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
> +PciHostBridgeUtilityExtraRoots (
> + UINTN *Count,
> + UINT32 BusMin,
> + UINT32 BusMax,
> + 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
> + );
> +
> +
> /**
> 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 4a176347fd49..19c6e9fa421a 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -21,8 +21,6 @@
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeLib.h>
> #include <Library/PciHostBridgeUtilityLib.h>
> -#include <Library/PciLib.h>
> -#include <Library/QemuFwCfgLib.h>
> #include "PciHostBridge.h"
>
>
> @@ -160,23 +158,6 @@ InitRootBridge (
> }
>
>
> -/**
> - Uninitialize a PCI_ROOT_BRIDGE structure set up with InitRootBridge().
> -
> - param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
> - initialized with InitRootBridge(), that should be
> - uninitialized. This function doesn't free RootBus.
> -**/
> -STATIC
> -VOID
> -UninitRootBridge (
> - IN PCI_ROOT_BRIDGE *RootBus
> - )
> -{
> - FreePool (RootBus->DevicePath);
> -}
> -
> -
> /**
> Return all the root bridge instances in an array.
>
> @@ -192,14 +173,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;
> @@ -239,117 +212,18 @@ PciHostBridgeGetRootBridges (
>
> *Count = 0;
>
> - //
> - // 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 = InitRootBridge (
> - Attributes,
> - Attributes,
> - AllocationAttributes,
> - (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 = InitRootBridge (
> - Attributes,
> - Attributes,
> - AllocationAttributes,
> - (UINT8) LastRootBridgeNumber,
> + return PciHostBridgeUtilityExtraRoots (
> + Count,
> + 0,
> PCI_MAX_BUS,
> - &Io,
> - &Mem,
> - &MemAbove4G,
> - &mNonExistAperture,
> - &mNonExistAperture,
> - &Bridges[Initialized]
> + Attributes,
> + AllocationAttributes,
> + Io,
> + Mem,
> + MemAbove4G,
> + mNonExistAperture,
> + mNonExistAperture
> );
> - if (EFI_ERROR (Status)) {
> - goto FreeBridges;
> - }
> - ++Initialized;
> -
> - *Count = Initialized;
> - return Bridges;
> -
> -FreeBridges:
> - while (Initialized > 0) {
> - --Initialized;
> - UninitRootBridge (&Bridges[Initialized]);
> - }
> -
> - FreePool (Bridges);
> - return NULL;
> }
>
>
> @@ -367,17 +241,7 @@ PciHostBridgeFreeRootBridges (
> UINTN Count
> )
> {
> - if (Bridges == NULL && Count == 0) {
> - return;
> - }
> - ASSERT (Bridges != NULL && Count > 0);
> -
> - do {
> - --Count;
> - UninitRootBridge (&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 bbaa5f830c98..a0cfd24ce477 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -11,8 +11,13 @@
> **/
>
> #include <IndustryStandard/Acpi10.h>
> +#include <IndustryStandard/Pci.h>
> #include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeUtilityLib.h>
> +#include <Library/PciLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +#include "Library/PciHostBridgeLib/PciHostBridge.h"
(3) This approach is wrong.
Library instances in edk2 are supposed to be self-contained. It is
non-idiomatic in edk2 for a consumed library class to expect a
*dependent* (= consumer) library instance to provide a *callback*, with
a particular funcion name. Whenever such generality is necessary, such
that a callback cannot be avoided, then a typedef (for a function
pointer type) must be introduced in the consumed library class, and the
consumer (= dependent) library instance passes in the concrete callback
as a pointer-to-function parameter -- wherever necessary.
However, we do not need that level of generality here -- we don't need
any callbacks.
(3a) You first need to extract the InitRootBridge() and
UninitRootBridge() functions to PciHostBridgeUtilityLib(), in a
separate patch.
In the same patch, move the OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH
typedef, and the "mRootBridgeDevicePathTemplate" global variable
too. (Notice that the latter is identical to
"mEfiPciRootBridgeDevicePath" in
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c").
No change of functionality; only rename the functions, in addition
to moving them.
Note that his code extraction / rename will affect the
"XenSupport.c" file too!
(3b) Then, in another patch, you need to extend the parameter list of
PciHostBridgeUtilityInitRootBridge() with the following parameters:
- DmaAbove4G
- NoExtendedConfigSpace
(3c) Then, in another patch, rebase ArmVirtPkg's PciHostBridgeLib
instance to the new PciHostBridgeUtilityInitRootBridge() /
PciHostBridgeUtilityUninitRootBridge() functions. Note that this
will affect the *single* root bus, at this point; namely the
PciHostBridgeGetRootBridges() function in
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c".
This will allow you to verify whether the new (extended)
PciHostBridgeUtilityInitRootBridge() interface is flexible enough.
The EFI_PCI_ROOT_BRIDGE_DEVICE_PATH typedef and the
"mEfiPciRootBridgeDevicePath" variable should disappear from
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c", at
this point.
(3d) Then you can proceed to extracting the fw_cfg-parsing logic from
OvmfPkg's PciHostBridgeLib instance to PciHostBridgeUtilityLib.
At first, there should be no change in functionality; in
particular, the DmaAbove4G and NoExtendedConfigSpace parameters
should remain hard-coded, in the call(s) to
PciHostBridgeUtilityInitRootBridge().
The name of the new functions should be
PciHostBridgeUtilityGetRootBridges() -- not
PciHostBridgeUtilityExtraRoots() --, and
PciHostBridgeUtilityFreeRootBridges() -- already named correctly in
your patch, apparently.
(3e) Extend PciHostBridgeUtilityGetRootBridges() with the DmaAbove4G and
NoExtendedConfigSpace parameters, so they are propagated from the
outside into PciHostBridgeUtilityInitRootBridge().
(3f) Call PciHostBridgeUtilityGetRootBridges() in ArmVirtPkg's
FdtPciHostBridgeLib.
>
>
> GLOBAL_REMOVE_IF_UNREFERENCED
> @@ -21,6 +26,213 @@ CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
> };
>
>
> +/**
> + Uninitialize a PCI_ROOT_BRIDGE structure set up with InitRootBridge().
> +
> + param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
> + initialized with InitRootBridge(), that should be
> + uninitialized. This function doesn't free RootBus.
> +**/
> +STATIC
> +VOID
> +UninitRootBridge (
> + IN PCI_ROOT_BRIDGE *RootBus
> + )
> +{
> + FreePool (RootBus->DevicePath);
> +}
> +
> +
> +/**
> + Utility function to free root bridge instances array.
> +
> + @param The root bridge instances array.
> + @param 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;
> + UninitRootBridge (&Bridges[Count]);
> + } while (Count > 0);
> +
> + FreePool (Bridges);
> +}
> +
> +
> +/**
> + Utility function to return all the root bridge instances in an array.
> +
> + @param Count The number of root bridge instances.
(4) This should be @param[out].
Thanks
Laszlo
> +
> + @param[in] BusMin The min bus number.
> +
> + @param[in] BusMax The max bus number.
> +
> + @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
> +PciHostBridgeUtilityExtraRoots (
> + UINTN *Count,
> + UINT32 BusMin,
> + UINT32 BusMax,
> + 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;
> + PCI_ROOT_BRIDGE *Bridges;
> + FIRMWARE_CONFIG_ITEM FwCfgItem;
> + UINTN FwCfgSize;
> + UINT64 ExtraRootBridges;
> + 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 > BusMax - BusMin) {
> + 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 = BusMin;
> +
> + //
> + // 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 = BusMin + 1;
> + RootBridgeNumber <= BusMax && 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 = InitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + (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 = InitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + (UINT8) LastRootBridgeNumber,
> + BusMax,
> + &Io,
> + &Mem,
> + &MemAbove4G,
> + &PMem,
> + &PMemAbove4G,
> + &Bridges[Initialized]
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBridges;
> + }
> + ++Initialized;
> +
> + *Count = Initialized;
> + return Bridges;
> +
> +FreeBridges:
> + while (Initialized > 0) {
> + --Initialized;
> + UninitRootBridge (&Bridges[Initialized]);
> + }
> +
> + FreePool (Bridges);
> + return NULL;
> +}
> +
> +
> /**
> 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 (#69805): https://edk2.groups.io/g/devel/message/69805
Mute This Topic: https://groups.io/mt/79146006/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