Re: [edk2-devel] [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib

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

On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> Eliminate currently duplicated code in ArmVirtPkg with the common utility
> class PciHostBridgeUtilityLib.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 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>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc                                     |  1 +
>  ArmVirtPkg/ArmVirtKvmTool.dsc                                  |  1 +
>  ArmVirtPkg/ArmVirtQemu.dsc                                     |  1 +
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                               |  1 +
>  ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf |  2 +
>  ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c   | 44 ++------------------
>  6 files changed, 9 insertions(+), 41 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 9ec92930472d..b9a0cd362416 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -145,6 +145,7 @@ [LibraryClasses.common]
>    PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
>    PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
>    PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> +  PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>  
>    # USB Libraries
>    UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index bf008be50fcb..f817132ba7b0 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -65,6 +65,7 @@ [LibraryClasses.common]
>    PlatformPeiLib|ArmVirtPkg/Library/KvmtoolPlatformPeiLib/KvmtoolPlatformPeiLib.inf
>  
>    PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> +  PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>    PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
>    SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
>  
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index ef5d6dbeaddc..a11ffd9ba553 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -78,6 +78,7 @@ [LibraryClasses.common]
>    PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
>    PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>    PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +  PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>  
>  !if $(TPM2_ENABLE) == TRUE
>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index f8f5f7f4b94b..c27752b4d5e5 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -76,6 +76,7 @@ [LibraryClasses.common]
>    PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
>    PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>    PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +  PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>    TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>  
>  [LibraryClasses.common.DXE_DRIVER]

Please pay more attention to review feedback. In v2, I requested two
things, and those have not been resolved precisely:

(1) The files

- ArmVirtPkg/ArmVirtKvmTool.dsc
- ArmVirtPkg/ArmVirtQemu.dsc
- ArmVirtPkg/ArmVirtQemuKernel.dsc

should be updated *instead of* "ArmVirtPkg/ArmVirt.dsc.inc" -- not "in
addition to" the latter.

So please drop the "ArmVirtPkg/ArmVirt.dsc.inc" hunk.

(2) In "ArmVirtPkg/ArmVirtKvmTool.dsc", you didn't place the
PciHostBridgeUtilityLib class resolution right after the
PciHostBridgeLib one.

So please move the new lib class resolution into said (more intuitive) spot.


With the above updates, the patch is going to be OK.

Thanks,
Laszlo

> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> index 277ccfd24546..01d39626d14c 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> @@ -31,12 +31,14 @@ [Packages]
>    ArmVirtPkg/ArmVirtPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
>    DebugLib
>    DevicePathLib
>    DxeServicesTableLib
>    MemoryAllocationLib
> +  PciHostBridgeUtilityLib
>    PciPcdProducerLib
>  
>  [FixedPcd]
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 496b192d2291..d554479bf0de 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -7,12 +7,13 @@
>  
>  **/
>  #include <PiDxe.h>
> -#include <Library/PciHostBridgeLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PciHostBridgeLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
>  #include <Protocol/FdtClient.h>
> @@ -50,11 +51,6 @@ STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
>    }
>  };
>  
> -GLOBAL_REMOVE_IF_UNREFERENCED
> -CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> -  L"Mem", L"I/O", L"Bus"
> -};
> -
>  //
>  // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
>  // records like this.
> @@ -435,39 +431,5 @@ PciHostBridgeResourceConflict (
>    VOID                              *Configuration
>    )
>  {
> -  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> -  UINTN                             RootBridgeIndex;
> -  DEBUG ((EFI_D_ERROR, "PciHostBridge: Resource conflict happens!\n"));
> -
> -  RootBridgeIndex = 0;
> -  Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
> -  while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> -    DEBUG ((EFI_D_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
> -    for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
> -      ASSERT (Descriptor->ResType <
> -              (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) /
> -               sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0])
> -               )
> -              );
> -      DEBUG ((EFI_D_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
> -              mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
> -              Descriptor->AddrLen, Descriptor->AddrRangeMax
> -              ));
> -      if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> -        DEBUG ((EFI_D_ERROR, "     Granularity/SpecificFlag = %ld / %02x%s\n",
> -                Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
> -                ((Descriptor->SpecificFlag &
> -                  EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE
> -                  ) != 0) ? L" (Prefetchable)" : L""
> -                ));
> -      }
> -    }
> -    //
> -    // Skip the END descriptor for root bridge
> -    //
> -    ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
> -    Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
> -                   (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1
> -                   );
> -  }
> +  PciHostBridgeUtilityResourceConflict (Configuration);
>  }
>