From: "Laszlo Ersek" <lersek@redhat.com>
To: Jiahui Cen <cenjiahui@huawei.com>, devel@edk2.groups.io
Cc: jordan.l.justen@intel.com, ard.biesheuvel@arm.com,
leif@nuviainc.com, xieyingtai@huawei.com, miaoyubo@huawei.com
Subject: Re: [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
Date: Wed, 11 Nov 2020 18:27:13 +0100 [thread overview]
Message-ID: <f3ec36ac-c060-5d2e-ad78-0b95f7d8532e@redhat.com> (raw)
In-Reply-To: <20201109130511.5946-3-cenjiahui@huawei.com>
On 11/09/20 14:05, Jiahui Cen wrote:
> From: Yubo Miao <miaoyubo@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@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@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
next prev parent reply other threads:[~2020-11-11 17:27 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-09 13:05 [PATCH v2 0/4] Add extra pci roots support for Arm Jiahui Cen
2020-11-09 13:05 ` [PATCH v2 1/4] OvmfPkg: Extract functions form PciHostBridgeLib Jiahui Cen
2020-11-11 16:45 ` Laszlo Ersek
2020-11-12 3:21 ` Jiahui Cen
2020-11-09 13:05 ` [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib Jiahui Cen
2020-11-11 17:27 ` Laszlo Ersek [this message]
2020-11-12 3:30 ` [edk2-devel] " Jiahui Cen
2020-11-09 13:05 ` [PATCH v2 3/4] OvmfPkg: Extract functions of extra pci roots Jiahui Cen
2020-11-09 13:05 ` [PATCH v2 4/4] ArmVirtPkg: Support " Jiahui Cen
2020-11-11 14:33 ` [PATCH v2 0/4] Add extra pci roots support for Arm Laszlo Ersek
2020-11-12 3:20 ` [edk2-devel] " Jiahui Cen
2020-12-04 6:48 ` Jiahui Cen
2020-12-04 15:08 ` Laszlo Ersek
2020-12-11 10:57 ` Ni, Ray
2020-12-15 12:52 ` Jiahui Cen
2020-12-17 13:23 ` Laszlo Ersek
2020-12-17 13:37 ` Ard Biesheuvel
2020-12-17 14:42 ` Jonathan Cameron
2020-12-17 13:52 ` Jiahui Cen
2020-11-12 8:49 ` Ard Biesheuvel
2020-11-13 19:44 ` Laszlo Ersek
2020-11-16 1:33 ` [edk2-devel] " Jiahui Cen
-- strict thread matches above, loose matches on Subject: below --
2020-11-07 7:40 Jiahui Cen
2020-11-07 7:40 ` [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib 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=f3ec36ac-c060-5d2e-ad78-0b95f7d8532e@redhat.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