[edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
Laszlo Ersek
lersek at redhat.com
Thu Jan 14 10:46:08 UTC 2021
On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Extend parameter list of PciHostBridgeUtilityGetRootBridges() with
> DmaAbove4G, NoExtendedConfigSpace, BusMin and BusMax to support for
> ArmVirtPkg.
>
> 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/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 --
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 30 +++++++++----
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 7 +++
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 47 ++++++++++++--------
> 4 files changed, 57 insertions(+), 30 deletions(-)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> index 4cdf367ffee2..83a734c1725e 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -41,6 +41,3 @@ [LibraryClasses]
> MemoryAllocationLib
> PciLib
> QemuFwCfgLib
> -
> -[Pcd]
> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> index 29ea19f2d7e5..ed2b386001e1 100644
> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -100,23 +100,31 @@ PciHostBridgeUtilityUninitRootBridge (
> /**
> Utility function to return all the root bridge instances in an array.
>
> - @param[out] Count The number of root bridge instances.
> + @param[out] Count The number of root bridge instances.
>
> - @param[in] Attributes Initial attributes.
> + @param[in] Attributes Initial attributes.
>
> - @param[in] AllocAttributes Allocation attributes.
> + @param[in] AllocAttributes Allocation attributes.
>
> - @param[in] Io IO aperture.
> + @param[in] DmaAbove4G DMA above 4GB memory.
>
> - @param[in] Mem MMIO aperture.
> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>
> - @param[in] MemAbove4G MMIO aperture above 4G.
> + @param[in] BusMin Minimum Bus number
>
> - @param[in] PMem Prefetchable MMIO aperture.
> + @param[in] BusMax Maximum Bus number
>
> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
> + @param[in] Io IO aperture.
>
> - @return All the root bridge instances in an array.
> + @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
> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
> UINTN *Count,
> UINT64 Attributes,
> UINT64 AllocationAttributes,
> + BOOLEAN DmaAbove4G,
> + BOOLEAN NoExtendedConfigSpace,
> + UINT32 BusMin,
> + UINT32 BusMax,
> PCI_ROOT_BRIDGE_APERTURE *Io,
> PCI_ROOT_BRIDGE_APERTURE *Mem,
> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
(1) You forgot to annotate the new params with IN. (Also update the C
file please, in sync.)
(2) The BusMin / BusMax addition must absolutely be a separate patch, so
that we can discuss and review it separately. It's not a simple data
propagation change -- it generalizes the function internally.
(3) BusMax should be documented as an inclusive maximum (but see more on
this below).
(4) I don't understand where the UINT32 type for BusMin / BusMax comes
from. PciHostBridgeUtilityInitRootBridge() takes UINT8 bus numbers
(which makes sense). And the scanning uses UINTN values, see e.g.
RootBridgeNumber, which also makes sense (for convenience). UINT32
matches neither. It's not necessarily wrong, but confusing.
... if you chose UINT32 because ProcessPciHost()
[ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c] outputs
BusMin and BusMax as UINT32, then please use UINTN instead.
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index 0a1d94a29bf3..28ad32752cab 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -9,6 +9,9 @@
> **/
> #include <PiDxe.h>
>
> +#include <IndustryStandard/Pci.h>
> +#include <IndustryStandard/Q35MchIch9.h>
> +
> #include <Protocol/PciHostBridgeResourceAllocation.h>
> #include <Protocol/PciRootBridgeIo.h>
>
> @@ -81,6 +84,10 @@ PciHostBridgeGetRootBridges (
> Count,
> Attributes,
> AllocationAttributes,
> + FALSE,
> + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + 0,
> + PCI_MAX_BUS,
> &Io,
> &Mem,
> &MemAbove4G,
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> index 629ebcd7a5be..fd2f54a139e2 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -12,7 +12,6 @@
>
> #include <IndustryStandard/Acpi10.h>
> #include <IndustryStandard/Pci.h>
> -#include <IndustryStandard/Q35MchIch9.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> @@ -189,23 +188,31 @@ PciHostBridgeUtilityUninitRootBridge (
> /**
> Utility function to return all the root bridge instances in an array.
>
> - @param[out] Count The number of root bridge instances.
> + @param[out] Count The number of root bridge instances.
>
> - @param[in] Attributes Initial attributes.
> + @param[in] Attributes Initial attributes.
>
> - @param[in] AllocAttributes Allocation attributes.
> + @param[in] AllocAttributes Allocation attributes.
>
> - @param[in] Io IO aperture.
> + @param[in] DmaAbove4G DMA above 4GB memory.
>
> - @param[in] Mem MMIO aperture.
> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>
> - @param[in] MemAbove4G MMIO aperture above 4G.
> + @param[in] BusMin Minimum Bus number
>
> - @param[in] PMem Prefetchable MMIO aperture.
> + @param[in] BusMax Maximum Bus number
>
> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
> + @param[in] Io IO aperture.
>
> - @return All the root bridge instances in an array.
> + @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
> @@ -213,6 +220,10 @@ PciHostBridgeUtilityGetRootBridges (
> UINTN *Count,
> UINT64 Attributes,
> UINT64 AllocationAttributes,
> + BOOLEAN DmaAbove4G,
> + BOOLEAN NoExtendedConfigSpace,
> + UINT32 BusMin,
> + UINT32 BusMax,
> PCI_ROOT_BRIDGE_APERTURE *Io,
> PCI_ROOT_BRIDGE_APERTURE *Mem,
> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
> @@ -240,7 +251,7 @@ PciHostBridgeUtilityGetRootBridges (
> QemuFwCfgSelectItem (FwCfgItem);
> QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
>
> - if (ExtraRootBridges > PCI_MAX_BUS) {
> + if (ExtraRootBridges > BusMax - BusMin) {
> DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
> "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
> return NULL;
(5) The code change looks valid, but please add a comment here (in the
patch dedicated to the bus numbers, that is). Because BusMax is
inclusive, the max bus count is (BusMax - BusMin + 1). From that, the
"main" root bus is is a given, so the max count for the "extra" root
bridges is one less, i.e. (BusMax - BusMin). If the QEMU hint exceeds
that, we have invalid behavior.
(6) In the patch that will deal with the bus numbers exlusively, please
add a sanity check near the top of the function:
BusMin > BusMax || BusMax > PCI_MAX_BUS
If this condition evaluates to TRUE, the function should set (*Count) to
0, and return NULL, at once.
> @@ -262,15 +273,15 @@ PciHostBridgeUtilityGetRootBridges (
> //
> // The "main" root bus is always there.
> //
> - LastRootBridgeNumber = 0;
> + 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 = 1;
> - RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges;
> + for (RootBridgeNumber = BusMin + 1;
> + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
> ++RootBridgeNumber) {
> UINTN Device;
>
> @@ -290,8 +301,8 @@ PciHostBridgeUtilityGetRootBridges (
> Attributes,
> Attributes,
> AllocationAttributes,
> - FALSE,
> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + DmaAbove4G,
> + NoExtendedConfigSpace,
> (UINT8) LastRootBridgeNumber,
> (UINT8) (RootBridgeNumber - 1),
> Io,
> @@ -317,8 +328,8 @@ PciHostBridgeUtilityGetRootBridges (
> Attributes,
> Attributes,
> AllocationAttributes,
> - FALSE,
> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + DmaAbove4G,
> + NoExtendedConfigSpace,
> (UINT8) LastRootBridgeNumber,
> PCI_MAX_BUS,
> Io,
>
(7) You missed replacing PCI_MAX_BUS with BusMax here. (But it belongs
in the separate patch that will deal with the bus numbers, and only with
the bus numbers.)
... Which in turn makes me ask you to please test your changes more
carefully. I believe this bug here is actually shown in the firmware
debug log. Namely, the "virt" machine type only supports buses 0x0..0xf,
inclusive (if I remember correctly), because its MMCONFIG space is quite
limited.
Now, assume the common case, with the "virt" machine type, where you
don't add any pxb devices to the QEMU cmdline -- just go with the main
bus (bus#0). With this bug, the "max sub bus number" on bus#0 goes from
0xf to 0xff. I'm fairly sure that change is visible in the messages that
are logged by PciHostBridgeDxe.
You're supposed to regression test the previous use case with the
patched code -- there should be no change in behavior. Comparing the
before-after logs is one of the checks someone should do for this.
... I've found the bus number stuff in this patch so distracting that
I'm actually not capable of reviewing the patch wrt. its "original"
purpose, namely the exposure of the DmaAbove4G and NoExtendedConfigSpace
parameters. I'll do that in v6, once you have split this patch in two.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70279): https://edk2.groups.io/g/devel/message/70279
Mute This Topic: https://groups.io/mt/79619797/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