1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
  4. View patch

[edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib

Jiahui Cen via groups.io posted 4 patches 5 years, 3 months ago
There is a newer version of this series
[edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
Posted by Jiahui Cen via groups.io 5 years, 3 months ago 
From: Yubo Miao <miaoyubo@huawei.com>

Eliminate the currently duplicated code in ArmVirtPkg and use the
extracted PciHostBridgeResourceConflict from PciHostBridgeUtilityLib.

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

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

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>

@@ -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

@@ -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

-                   );

-  }

-}

-- 
2.28.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67182): https://edk2.groups.io/g/devel/message/67182
Mute This Topic: https://groups.io/mt/78135580/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
Posted by Laszlo Ersek 5 years, 2 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67294): https://edk2.groups.io/g/devel/message/67294
Mute This Topic: https://groups.io/mt/78135580/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
Posted by Jiahui Cen via groups.io 5 years, 2 months ago 
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@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
> 
> 
> 
> 
> 
> 
> .
> 


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.