From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mx.groups.io with SMTP id smtpd.web12.17487.1605151863755343708 for ; Wed, 11 Nov 2020 19:31:04 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: cenjiahui@huawei.com) Received: from DGGEMS401-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4CWnDj73P6zLtJ4; Thu, 12 Nov 2020 11:30:45 +0800 (CST) Received: from [10.174.184.155] (10.174.184.155) by DGGEMS401-HUB.china.huawei.com (10.3.19.201) with Microsoft SMTP Server id 14.3.487.0; Thu, 12 Nov 2020 11:30:51 +0800 Subject: Re: [edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib To: , CC: , , , , References: <20201109130511.5946-1-cenjiahui@huawei.com> <20201109130511.5946-3-cenjiahui@huawei.com> From: "Jiahui Cen" Message-ID: <9fb2215b-15d6-7ddd-e6ec-f373b62565d5@huawei.com> Date: Thu, 12 Nov 2020 11:30:50 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.174.184.155] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, Thanks for your review. I'll update all the patches with your points. Jiahui On 2020/11/12 1:27, Laszlo Ersek wrote: > 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 > > > > > > > . >