Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges

["Laszlo Ersek" <lersek@redhat.com>]

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@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/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