Re: [edk2-devel] [PATCH v6 00/11] Add extra pci roots support for Arm

["Jiahui Cen" <cenjiahui@huawei.com>]

Hi Laszlo,

On 2021/1/21 0:26, Laszlo Ersek wrote:
> On 01/19/21 02:12, Jiahui Cen via groups.io wrote:
>> v5->v6:
>> * Annotate library functions with EFIAPI, IN/OUT, @param[in]/[out].
>> * Split BusMin/BusMax addtion in PciHostBridgeUtilityGetRootBridges() into
>>   a separate patch [09/11].
>> * Add more detailed commit message for patch [10/11].
> 
> Merged via <https://github.com/tianocore/edk2/pull/1372> as commit range
> ca272b9513a6..e843a21e23ea.
> 

Thanks for the review and these fixups.

Thanks,
Jiahui

> The following git-range-diff output shows my fixups on top of the posted
> v6 series:
> 
>>  1:  b757f3127929 =  1:  7a6172f88bb0 OvmfPkg: Introduce PciHostBridgeUtilityLib class
>>  2:  87b14160b8ec =  2:  166a32d09ad9 ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
>>  3:  c31534b6ac1e !  3:  517055d29886 OvmfPkg/PciHostBridgeLib: List missing PcdLib dependency
>>     @@ -15,6 +15,7 @@
>>          Cc: Julien Grall <julien@xen.org>
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Message-Id: <20210119011302.10908-4-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>>      --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>>  4:  b4a56730b94a !  4:  7ac1f28d4dbd OvmfPkg/PciHostBridgeLib: Extract InitRootBridge() / UninitRootBridge()
>>     @@ -15,6 +15,7 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-5-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>      --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>  5:  edb622820369 !  5:  e1b259da426b OvmfPkg/PciHostBridgeUtilityLib: Extend parameters of InitRootBridge()
>>     @@ -16,6 +16,7 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-6-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>      --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>  6:  a678abfd5f0c !  6:  aa445ce02b69 ArmVirtPkg/FdtPciHostBridgeLib: Refactor init/uninit of root bridge
>>     @@ -14,6 +14,7 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-7-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>>      --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>>  7:  f072f24ca41f !  7:  4edba29651e3 OvmfPkg/PciHostBridgeLib: Extract GetRootBridges() / FreeRootBridges()
>>     @@ -16,6 +16,8 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-8-cenjiahui@huawei.com>
>>     +    [lersek@redhat.com: keep zeroing of (*Count) centralized]
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>      --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>     @@ -355,6 +357,8 @@
>>      +  UINTN                LastRootBridgeNumber;
>>      +  UINTN                RootBridgeNumber;
>>      +
>>     ++  *Count = 0;
>>     ++
>>      +  //
>>      +  // QEMU provides the number of extra root buses, shortening the exhaustive
>>      +  // search below. If there is no hint, the feature is missing.
>>     @@ -369,7 +373,6 @@
>>      +    if (ExtraRootBridges > PCI_MAX_BUS) {
>>      +      DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
>>      +        "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
>>     -+      *Count = 0;
>>      +      return NULL;
>>      +    }
>>      +    DEBUG ((DEBUG_INFO, "%a: %Lu extra root buses reported by QEMU\n",
>>     @@ -382,7 +385,6 @@
>>      +  Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges);
>>      +  if (Bridges == NULL) {
>>      +    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
>>     -+    *Count = 0;
>>      +    return NULL;
>>      +  }
>>      +  Initialized = 0;
>>     @@ -471,7 +473,6 @@
>>      +  }
>>      +
>>      +  FreePool (Bridges);
>>     -+  *Count = 0;
>>      +  return NULL;
>>      +}
>>      +
>>  8:  5607d15dab2f !  8:  14d4b6be568d OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
>>     @@ -13,6 +13,7 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-9-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>      --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>  9:  7a1681682daa !  9:  f4a257a355b7 OvmfPkg/PciHostBridgeUtilityLib: Extend GetRootBridges() with BusMin/BusMax
>>     @@ -13,6 +13,10 @@
>>          Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Message-Id: <20210119011302.10908-10-cenjiahui@huawei.com>
>>     +    [lersek@redhat.com: fix logging of UINTN values BusMin, BusMax]
>>     +    [lersek@redhat.com: keep zeroing of (*Count) centralized]
>>     +    [lersek@redhat.com: fix typos in ExtraRootBridges comment]
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>      --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>     @@ -75,13 +79,12 @@
>>         IN  PCI_ROOT_BRIDGE_APERTURE *Mem,
>>         IN  PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>>      @@
>>     -   UINTN                LastRootBridgeNumber;
>>     -   UINTN                RootBridgeNumber;
>>     +
>>     +   *Count = 0;
>>
>>      +  if (BusMin > BusMax || BusMax > PCI_MAX_BUS) {
>>     -+    DEBUG ((DEBUG_ERROR, "%a: invalid bus range with BusMin %d and BusMax %d\n",
>>     -+      __FUNCTION__, BusMin, BusMax));
>>     -+    *Count = 0;
>>     ++    DEBUG ((DEBUG_ERROR, "%a: invalid bus range with BusMin %Lu and BusMax "
>>     ++      "%Lu\n", __FUNCTION__, (UINT64)BusMin, (UINT64)BusMax));
>>      +    return NULL;
>>      +  }
>>      +
>>     @@ -96,14 +99,14 @@
>>      +    //
>>      +    // Validate the number of extra root bridges. As BusMax is inclusive, the
>>      +    // max bus count is (BusMax - BusMin + 1). From that, the "main" root bus
>>     -+    // is always givin, so the max count for the "extra" root bridges is one
>>     -+    // less, i.e. (BusMax - BusMin). If QEME hint exceeds that, we have invalid
>>     -+    // behavior.
>>     ++    // is always 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.
>>      +    //
>>      +    if (ExtraRootBridges > BusMax - BusMin) {
>>             DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
>>               "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
>>     -       *Count = 0;
>>     +       return NULL;
>>      @@
>>         //
>>         // The "main" root bus is always there.
>> 10:  bb3538547b3e ! 10:  c4cbd864933a ArmVirtPkg/FdtPciHostBridgeLib: Add extra pci root buses support
>>     @@ -19,6 +19,7 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-11-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>      --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> 11:  fbf533fe9c10 ! 11:  e843a21e23ea ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
>>     @@ -13,6 +13,7 @@
>>          Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>          Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>          Message-Id: <20210119011302.10908-12-cenjiahui@huawei.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>      --- a/ArmVirtPkg/ArmVirtQemu.dsc
> 
> Thanks
> Laszlo
> 
>>
>> v4->v5:
>> * Revert some risky renames.
>> * Explicitly list PcdLib dependency.
>> * Fix issues reported by PatchCheck.py.
>>
>> v3->v4:
>> * Refactor InitRootBridges/UninitRootBridges/GetRootBridges/FreeRootBridges.
>> * Fix library dependencies.
>>
>> v2->v3:
>> * Rename utility functions under the PciHostBridgeUtilityLib namespace.
>> * Remove some unused Library dependencies.
>> * Sort the Include headers.
>>
>> v1->v2:
>> * Separated into four patches.
>> * Factor the same logic parts into a new library.
>>
>> v4: https://edk2.groups.io/g/devel/message/70147
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>> QEMU: https://lore.kernel.org/qemu-devel/20201119014841.7298-1-cenjiahui@huawei.com/
>>
>> This patch series adds support for extra pci roots for ARM.
>>
>> In order to avoid duplicated codes, we introduce a new library
>> PciHostBridgeUtilityLib which extracts common interfaces from
>> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci
>> roots scanning. Using the utility lib, the uefi could scan for extra
>> root buses and recognize multiple roots for ARM.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Cc: Peter Grehan <grehan@freebsd.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>>
>> Jiahui Cen (11):
>>   OvmfPkg: Introduce PciHostBridgeUtilityLib class
>>   ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
>>   OvmfPkg/PciHostBridgeLib: List missing PcdLib dependency
>>   OvmfPkg/PciHostBridgeLib: Extract InitRootBridge() /
>>     UninitRootBridge()
>>   OvmfPkg/PciHostBridgeUtilityLib: Extend parameters of InitRootBridge()
>>   ArmVirtPkg/FdtPciHostBridgeLib: Refactor init/uninit of root bridge
>>   OvmfPkg/PciHostBridgeLib: Extract GetRootBridges() / FreeRootBridges()
>>   OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of
>>     GetRootBridges
>>   OvmfPkg/PciHostBridgeUtilityLib: Extend GetRootBridges() with
>>     BusMin/BusMax
>>   ArmVirtPkg/FdtPciHostBridgeLib: Add extra pci root buses support
>>   ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
>>
>>  OvmfPkg/OvmfPkg.dec                                                                                        |   4 +
>>  ArmVirtPkg/ArmVirtKvmTool.dsc                                                                              |   1 +
>>  ArmVirtPkg/ArmVirtQemu.dsc                                                                                 |   2 +
>>  ArmVirtPkg/ArmVirtQemuKernel.dsc                                                                           |   2 +
>>  OvmfPkg/AmdSev/AmdSevX64.dsc                                                                               |   1 +
>>  OvmfPkg/Bhyve/BhyveX64.dsc                                                                                 |   1 +
>>  OvmfPkg/OvmfPkgIa32.dsc                                                                                    |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                                                                 |   1 +
>>  OvmfPkg/OvmfPkgX64.dsc                                                                                     |   1 +
>>  OvmfPkg/OvmfXen.dsc                                                                                        |   1 +
>>  ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf                                             |   3 +
>>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf                                                      |   4 +-
>>  OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf                                        |  43 +++
>>  OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h                                                          | 185 +++++++++++
>>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h                                                           |  56 ----
>>  ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c                                               | 164 ++++------
>>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c                                                        | 324 +-------------------
>>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c                                                              |   5 +-
>>  OvmfPkg/Library/{PciHostBridgeLib/PciHostBridgeLib.c => PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c} | 281 +++++++++--------
>>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                                       |   1 +
>>  20 files changed, 467 insertions(+), 614 deletions(-)
>>  create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>>  create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>  copy OvmfPkg/Library/{PciHostBridgeLib/PciHostBridgeLib.c => PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c} (50%)
>>
> 
> 
> 
> 
> 
> 
> .
>