[edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
Jiahui Cen via groups.io
cenjiahui=huawei.com at groups.io
Thu Nov 12 03:30:50 UTC 2020
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 <miaoyubo at huawei.com>
>>
>> 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 <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Leif Lindholm <leif at nuviainc.com>
>> Signed-off-by: Yubo Miao <miaoyubo at huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui at huawei.com>
>> ---
>> 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 <Library/MemoryAllocationLib.h>
>> #include <Library/PcdLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/PciHostBridgeUtilityLib.h>
>>
>> #include <Protocol/FdtClient.h>
>> #include <Protocol/PciRootBridgeIo.h>
>
> (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 <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>
>
> 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
>
>
>
>
>
>
> .
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67352): https://edk2.groups.io/g/devel/message/67352
Mute This Topic: https://groups.io/mt/78135580/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list