From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.5604.1609924329547935849 for ; Wed, 06 Jan 2021 01:12:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PjXIEkaE; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1609924328; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TpixXZvvAloNfgtMHqLpi3NoY9CNSgMQvuIIuQdPsP4=; b=PjXIEkaE/BfmyVY5P6FFv1l6g/50Se2Vyz3ozOj/vEFV8G/W84I4ySKdtF4XosML6LpULV xgwzgx72ys4d4RMgovUq9zEOaNUqrQSgsOmPHBtIj72Op8g9XW0kDxt1WChIzK+SLYvW3U dUtee7RYnY5WOE+nRtZmWlsBUmzA0Hc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-100-0qxfvXDXMumtB8WAKSIzRQ-1; Wed, 06 Jan 2021 04:12:06 -0500 X-MC-Unique: 0qxfvXDXMumtB8WAKSIzRQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A3BC4801AC1; Wed, 6 Jan 2021 09:12:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-192.ams2.redhat.com [10.36.112.192]) by smtp.corp.redhat.com (Postfix) with ESMTP id 298625D9CD; Wed, 6 Jan 2021 09:12:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib To: devel@edk2.groups.io, cenjiahui@huawei.com Cc: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , Julien Grall , Leif Lindholm , Sami Mujawar , xieyingtai@huawei.com, wu.wubin@huawei.com, Yubo Miao References: <20201222095944.8686-1-cenjiahui@huawei.com> <20201222095944.8686-3-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: <3eebeece-d7ae-c75a-c249-f596880a109a@redhat.com> Date: Wed, 6 Jan 2021 10:12:00 +0100 MIME-Version: 1.0 In-Reply-To: <20201222095944.8686-3-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Cc: Sami Mujawar > Signed-off-by: Jiahui Cen > Signed-off-by: Yubo Miao > --- > 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 > -#include > #include > #include > #include > #include > #include > +#include > +#include > #include > > #include > @@ -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); > } >