From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.10219.1605115649917532163 for ; Wed, 11 Nov 2020 09:27:30 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IpDOoOdd; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605115649; 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=9E94sC59ptOgr3F2IBl7vNPKWmyzKz4UUd4PT0M4504=; b=IpDOoOdd0XFH2+5hko1mtAUGKwzEj7s2tD05sT6zJ3m7jv2FaV33SRV4PZr1knWox7Adwl 6UQI3G8HjOXwRR2NyF/C1584lmlZ6OE6lRFnACL9+NglmzsZ9Ch0hXUroKdUHT/MsfNte7 32uda3xJLjAKcRBKiivRTiywYeZli6g= 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-511-4YxAlZnLOFexujs3bR1ctg-1; Wed, 11 Nov 2020 12:27:26 -0500 X-MC-Unique: 4YxAlZnLOFexujs3bR1ctg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E8BD810B9CDF; Wed, 11 Nov 2020 17:27:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-85.ams2.redhat.com [10.36.113.85]) by smtp.corp.redhat.com (Postfix) with ESMTP id C3B547514C; Wed, 11 Nov 2020 17:27:14 +0000 (UTC) Subject: Re: [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib To: Jiahui Cen , devel@edk2.groups.io Cc: jordan.l.justen@intel.com, ard.biesheuvel@arm.com, leif@nuviainc.com, xieyingtai@huawei.com, miaoyubo@huawei.com References: <20201109130511.5946-1-cenjiahui@huawei.com> <20201109130511.5946-3-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 11 Nov 2020 18:27:13 +0100 MIME-Version: 1.0 In-Reply-To: <20201109130511.5946-3-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 11/09/20 14:05, Jiahui Cen wrote: > From: Yubo Miao > > Eliminate the currently duplicated code in ArmVirtPkg and use the > extracted PciHostBridgeResourceConflict from PciHostBridgeUtilityLib. (1) Please update the function name above, to "PciHostBridgeUtilityResourceConflict". > > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Signed-off-by: Yubo Miao > Signed-off-by: Jiahui Cen > --- > ArmVirtPkg/ArmVirt.dsc.inc | 1 + > ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 + > ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 61 +------------------- > 3 files changed, 4 insertions(+), 60 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index 4dafd1fa0f1d..593a523171ff 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -144,6 +144,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 (2) Please place this lib class resolution in the following files, instead: - ArmVirtPkg/ArmVirtKvmTool.dsc - ArmVirtPkg/ArmVirtQemu.dsc - ArmVirtPkg/ArmVirtQemuKernel.dsc just below where each one of those files resolves "PciHostBridgeLib" to "ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf". ("ArmVirtXen.dsc" consumes "ArmVirt.dsc.inc" but does not use "FdtPciHostBridgeLib.inf", therefore it does not need to inherit a PciHostBridgeUtilityLib resolution from "ArmVirt.dsc.inc".) > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > index 277ccfd24546..97e9368c8e9f 100644 > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > @@ -31,6 +31,7 @@ [Packages] > ArmVirtPkg/ArmVirtPkg.dec > MdeModulePkg/MdeModulePkg.dec > MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > DebugLib > @@ -38,6 +39,7 @@ [LibraryClasses] > DxeServicesTableLib > MemoryAllocationLib > PciPcdProducerLib > + PciHostBridgeUtilityLib > > [FixedPcd] > gArmTokenSpaceGuid.PcdPciMmio32Translation (3) Please keep the entries under [LibraryClasses] sorted. > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > index 496b192d2291..3952f511b4d2 100644 > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > > #include > #include (4) Okay, I'm going to cheat a bit here... Because the #include list is not sorted in the first place, unfortunately. So please do this: > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > index 496b192d2291..1c0321ce404b 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 Normally we should keep such unrelated changes separate, but in itself, it's moving just one line, so I prefer sneaking it into this patch. Back to your patch: On 11/09/20 14:05, Jiahui Cen wrote: > @@ -51,9 +52,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 OK. > @@ -414,60 +412,3 @@ PciHostBridgeFreeRootBridges ( > ASSERT (Count == 1); > } > > -/** > - Inform the platform that the resource conflict happens. > - > - @param HostBridgeHandle Handle of the Host Bridge. > - @param Configuration Pointer to PCI I/O and PCI memory resource > - descriptors. The Configuration contains the resources > - for all the root bridges. The resource for each root > - bridge is terminated with END descriptor and an > - additional END is appended indicating the end of the > - entire resources. The resource descriptor field > - values follow the description in > - EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL > - .SubmitResources(). > -**/ > -VOID > -EFIAPI > -PciHostBridgeResourceConflict ( > - EFI_HANDLE HostBridgeHandle, > - 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 > - ); > - } > -} > (5) Please replace only the body of the function, with a call to PciHostBridgeUtilityResourceConflict(). I'm finishing the review of v2 at this point -- I'd like to ask you to address the requests I made thus far, and post v3: - I've asked for trimming the Library #include lists in C files, and the [LibraryClasses] sections in INF files, to the required minimums. This means that some dependencies will move from patch#1 to patch#3, and I wouldn't like to re-review those parts in patch#3. - Some points I've made thus far apply to the rest of the series, and I would like you to ensure them globally, before I first look at the rest of the patches. In particular: - Keep #include lists and [LibraryClasses] sections (and almost all other INF file sections) sorted. - Keep #include lists and [LibraryClasses] sections (and other INF file sections) minimal. This applies to such modules too that you move code *from* -- removing code may mean that you no longer need various #includes and lib classes in the original module! - Functions declared in the PciHostBridgeUtilityLib class header should be called PciHostBridgeUtility*. Among other things this means that you can't / shouldn't move PciHostBridgeLib function imlementations whole-sale; you should only move their bodies. It also implies the renaming of functions that are currently internal to OVMF's PciHostBridgeLib instance. - The tree should build at every stage throughout the series. In general, please go through my points under patch #1, and see if / how each applies to patches #3 and #4. (6) Finally: it seems you have most of the necessary git settings implemented in your edk2 clone. Can you please check if you have "sendemail.transferEncoding=8bit" as well? (You might have to pass "--transfer-encoding=8bit" to git-send-email manually.) Beyond "8bit", "base64" is also an option; just avoid the default "quoted-printable", please. Thanks! Laszlo