From: "Jiahui Cen" <cenjiahui@huawei.com>
To: Laszlo Ersek <lersek@redhat.com>, <devel@edk2.groups.io>
Cc: Jordan Justen <jordan.l.justen@intel.com>,
Ard Biesheuvel <ard.biesheuvel@arm.com>,
Rebecca Cran <rebecca@bsdio.com>,
Peter Grehan <grehan@freebsd.org>,
Anthony Perard <anthony.perard@citrix.com>,
"Julien Grall" <julien@xen.org>,
Leif Lindholm <leif@nuviainc.com>,
Sami Mujawar <sami.mujawar@arm.com>, <xieyingtai@huawei.com>,
<wu.wubin@huawei.com>, "Yubo Miao" <miaoyubo@huawei.com>
Subject: Re: [edk2-devel] [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
Date: Thu, 7 Jan 2021 13:44:25 +0800 [thread overview]
Message-ID: <59b7169c-80e3-a459-1b20-835e4066fa3a@huawei.com> (raw)
In-Reply-To: <3eebeece-d7ae-c75a-c249-f596880a109a@redhat.com>
Hi Laszlo,
On 2021/1/6 17:12, Laszlo Ersek wrote:
> 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.
>
Sorry for missing these review points. Will fix them in v4.
Thanks,
Jiahui
>
> 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);
>> }
>>
>
> .
>
next prev parent reply other threads:[~2021-01-07 5:44 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
2021-01-06 8:51 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:44 ` Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
2021-01-06 9:12 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:44 ` Jiahui Cen [this message]
2020-12-22 9:59 ` [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots Jiahui Cen
2021-01-06 10:27 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:47 ` Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 4/5] ArmVirtPkg: Add support " Jiahui Cen
2021-01-06 10:28 ` [edk2-devel] " Laszlo Ersek
2020-12-22 9:59 ` [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
2021-01-06 10:31 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:47 ` Jiahui Cen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=59b7169c-80e3-a459-1b20-835e4066fa3a@huawei.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox