* [PATCH v2 1/4] OvmfPkg: Extract functions form PciHostBridgeLib
2020-11-09 13:05 [PATCH v2 0/4] Add extra pci roots support for Arm Jiahui Cen
@ 2020-11-09 13:05 ` Jiahui Cen
2020-11-11 16:45 ` Laszlo Ersek
2020-11-09 13:05 ` [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib Jiahui Cen
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Jiahui Cen @ 2020-11-09 13:05 UTC (permalink / raw)
To: devel
Cc: jordan.l.justen, lersek, ard.biesheuvel, leif, xieyingtai,
miaoyubo, Jiahui Cen
From: Yubo Miao <miaoyubo@huawei.com>
Introduce a new PciHostBridgeUtilityLib class to share duplicate code
between OvmfPkg and ArmVirtPkg.
Extract function PciHostBridgeResourceConflict from
OvmfPkg/PciHostBridgeLib.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 49 +++++++++++++
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 33 +++++++++
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 64 +----------------
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 74 ++++++++++++++++++++
9 files changed, 162 insertions(+), 63 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 58d9f292f9ac..0c2bf0b13c34 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -738,6 +738,7 @@ [Components]
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
<LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 3551f9710a6c..baf36a4f7a54 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -752,6 +752,7 @@ [Components.X64]
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
<LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 7a8bdb8a8697..219b5f559b53 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -748,6 +748,7 @@ [Components]
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
<LibraryClasses>
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 34c9de19dfba..442c0730ef32 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -544,6 +544,7 @@ [Components]
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
<LibraryClasses>
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index 6ec9ec751478..7d01528c94f1 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -41,6 +41,7 @@ [LibraryClasses]
MemoryAllocationLib
PciLib
QemuFwCfgLib
+ PciHostBridgeUtilityLib
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
new file mode 100644
index 000000000000..c88ab8e4155d
--- /dev/null
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -0,0 +1,49 @@
+## @file
+# OVMF and Arm's instance of the PCI Host Bridge Utility Library.
+#
+# Copyright (C) 2016, Red Hat, Inc.
+# Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = PciHostBridgeUtilityLib
+ FILE_GUID = e3aa5932-527a-42e7-86f5-81b144c7e5f1
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = PciHostBridgeUtilityLib
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC
+#
+
+[Sources]
+ PciHostBridgeUtilityLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseMemoryLib
+ DebugLib
+ DevicePathLib
+ MemoryAllocationLib
+ PciLib
+ QemuFwCfgLib
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base
+ gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
+ gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
+ gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
new file mode 100644
index 000000000000..d2622fd907e6
--- /dev/null
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -0,0 +1,33 @@
+/** @file
+ PCI Host Bridge Library consumed by PciHostBridgeDxe driver returning
+ the platform specific information about the PCI Host Bridge.
+
+ Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#ifndef __PCI_HOST_BRIDGE_UTILITY_LIB_H__
+#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
+
+/**
+ 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
+ );
+
+#endif
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index e850f7d183ee..1c8f465834f3 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -22,6 +22,7 @@
#include <Library/PciHostBridgeLib.h>
#include <Library/PciLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/PciHostBridgeUtilityLib.h>
#include "PciHostBridge.h"
@@ -33,12 +34,6 @@ typedef struct {
#pragma pack ()
-GLOBAL_REMOVE_IF_UNREFERENCED
-CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
- L"Mem", L"I/O", L"Bus"
-};
-
-
STATIC
CONST
OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePathTemplate = {
@@ -384,60 +379,3 @@ PciHostBridgeFreeRootBridges (
FreePool (Bridges);
}
-
-
-/**
- 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 ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
-
- RootBridgeIndex = 0;
- Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
- while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
- DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
- for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
- ASSERT (Descriptor->ResType <
- ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr)
- );
- DEBUG ((DEBUG_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 ((DEBUG_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
- );
- }
-}
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
new file mode 100644
index 000000000000..7e9512dc08f1
--- /dev/null
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -0,0 +1,74 @@
+/** @file
+ OVMF's instance of the PCI Host Bridge Library.
+
+ Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include <Library/DebugLib.h>
+#include <Library/PciHostBridgeUtilityLib.h>
+
+
+GLOBAL_REMOVE_IF_UNREFERENCED
+CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
+ L"Mem", L"I/O", L"Bus"
+};
+
+
+/**
+ 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 ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
+
+ RootBridgeIndex = 0;
+ Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
+ while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
+ DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
+ for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
+ ASSERT (Descriptor->ResType <
+ ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr)
+ );
+ DEBUG ((DEBUG_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 ((DEBUG_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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 1/4] OvmfPkg: Extract functions form PciHostBridgeLib
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
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-11-11 16:45 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo
On 11/09/20 14:05, Jiahui Cen wrote:
> From: Yubo Miao <miaoyubo@huawei.com>
>
> Introduce a new PciHostBridgeUtilityLib class to share duplicate code
> between OvmfPkg and ArmVirtPkg.
>
> Extract function PciHostBridgeResourceConflict from
> OvmfPkg/PciHostBridgeLib.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/OvmfXen.dsc | 1 +
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 49 +++++++++++++
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 33 +++++++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 64 +----------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 74 ++++++++++++++++++++
> 9 files changed, 162 insertions(+), 63 deletions(-)
(1) There is a typo in the subject: s/form/from/
(2) "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf" is consumed
in the following platform DSC file as well -- please update it too:
OvmfPkg/Bhyve/BhyveX64.dsc
(This will affect the list of necessary CC's on this patch.)
(3) In my v1 review at <https://edk2.groups.io/g/devel/message/57062>
(msgid <57fd0043-d63a-55b0-9c55-4ca079331885@redhat.com>), I mentioned
"OvmfPkg/OvmfPkg.dec".
You forgot to extend that file, with the new library class. Please add
the following hunk to this patch:
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 118c968fda4e..b89252eeab2a 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -49,6 +49,10 @@ [LibraryClasses]
> # access.
> PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h
>
> + ## @libraryclass Provide common utility functions to PciHostBridgeLib
> + # instances in ArmVirtPkg and OvmfPkg.
> + PciHostBridgeUtilityLib|Include/Library/PciHostBridgeUtilityLib.h
> +
> ## @libraryclass Register a status code handler for printing the Boot
> # Manager's LoadImage() and StartImage() preparations, and
> # return codes, to the UEFI console.
(4) Issue (3) above would have been caught by CI, namely by the
"LibraryClassCheck" plugin:
https://github.com/tianocore/edk2/blob/master/.pytool/Readme.md#library-declaration-test---libraryclasscheck
In order to put your patches through CI before posting, I suggest
pushing them to a branch in your edk2 clone on github.com, and
submitting a pull request against the central repository. The
pull request will be auto-closed in the end, unconditionally, but it
will give you CI results.
(You can also run CI locally, but setting that up is laborious.)
Back to your patch:
On 11/09/20 14:05, Jiahui Cen wrote:
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 58d9f292f9ac..0c2bf0b13c34 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -738,6 +738,7 @@ [Components]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3551f9710a6c..baf36a4f7a54 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -752,6 +752,7 @@ [Components.X64]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 7a8bdb8a8697..219b5f559b53 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -748,6 +748,7 @@ [Components]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 34c9de19dfba..442c0730ef32 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -544,6 +544,7 @@ [Components]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
(5) Please keep the PciHostBridgeUtilityLib class resolution right next
to the PciHostBridgeLib one.
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index 6ec9ec751478..7d01528c94f1 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -41,6 +41,7 @@ [LibraryClasses]
> MemoryAllocationLib
> PciLib
> QemuFwCfgLib
> + PciHostBridgeUtilityLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
(6) Please keep the entries under [LibraryClasses] alphabetically
sorted.
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> new file mode 100644
> index 000000000000..c88ab8e4155d
> --- /dev/null
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -0,0 +1,49 @@
> +## @file
> +# OVMF and Arm's instance of the PCI Host Bridge Utility Library.
(7) I suggest the following description:
Provide common utility functions to PciHostBridgeLib instances in
ArmVirtPkg and OvmfPkg.
> +#
> +# Copyright (C) 2016, Red Hat, Inc.
> +# Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
(8) It makes sense to duplicate the (C) notices from the original file
(namely "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf"), but
this is still a new file due to your contribution.
Therefore, please add a proper Huawei copyright notice.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
(9) The latest specified INF version is 1.29, according to
<https://edk2-docs.gitbook.io/edk-ii-inf-specification/#edk-ii-module-information-inf-file-specification>;
please write
INF_VERSION = 1.29
> + BASE_NAME = PciHostBridgeUtilityLib
> + FILE_GUID = e3aa5932-527a-42e7-86f5-81b144c7e5f1
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = PciHostBridgeUtilityLib
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64 EBC
> +#
> +
(10) EBC is gone, plus we'll want to plug this lib into
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf", which
itself names AARCH64 and ARM for VALID_ARCHITECTURES.
Therefore, please state "IA32 X64 AARCH64 ARM" here.
> +[Sources]
> + PciHostBridgeUtilityLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + DevicePathLib
> + MemoryAllocationLib
> + PciLib
> + QemuFwCfgLib
(11) The list of required (consumed) library classes, along with the
library class header files included in "PciHostBridgeUtilityLib.c",
should be as minimal as possible (and alphabetically sorted).
Specifically, you only need to keep "DebugLib"; everything else should
be removed, as far as I can tell.
(We can extend the [LibraryClasses] section in later patches, if
necessary.)
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
(12) The [Pcd] section should be dropped altogether.
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> new file mode 100644
> index 000000000000..d2622fd907e6
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -0,0 +1,33 @@
> +/** @file
> + PCI Host Bridge Library consumed by PciHostBridgeDxe driver returning
> + the platform specific information about the PCI Host Bridge.
(13) This file-top comment should say:
Provide common utility functions to PciHostBridgeLib instances in
ArmVirtPkg and OvmfPkg.
> +
> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
(14) It's fine to keep the (C) notices from the file that you are using
as template, but you should please add a Huawei (C) notice as well.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> +#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> +
> +/**
> + 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
> + );
> +
(15) Please rename this function to
"PciHostBridgeUtilityResourceConflict".
The idea is that no PciHostBridgeUtilityLib instance should directly
implement a PciHostBridgeLib API. PciHostBridgeUtilityLib can offer APIs
that are as "fat" as necessary, taking over as much work as possible,
but we should keep the namespaces isolated.
(16) In our helper library, the "HostBridgeHandle" parameter is not
used; please remove it from both the leading comment on the function,
and also from the function declaration.
> +#endif
(17) Please append a comment to this line:
// __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index e850f7d183ee..1c8f465834f3 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -22,6 +22,7 @@
> #include <Library/PciHostBridgeLib.h>
> #include <Library/PciLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
> #include "PciHostBridge.h"
>
>
(18) Please keep the #include list of the Library class headers
alphabetically sorted.
> @@ -33,12 +34,6 @@ typedef struct {
> #pragma pack ()
>
>
> -GLOBAL_REMOVE_IF_UNREFERENCED
> -CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> - L"Mem", L"I/O", L"Bus"
> -};
> -
> -
> STATIC
> CONST
> OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePathTemplate = {
> @@ -384,60 +379,3 @@ PciHostBridgeFreeRootBridges (
>
> FreePool (Bridges);
> }
> -
> -
> -/**
> - 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 ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
> -
> - RootBridgeIndex = 0;
> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
> - while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> - DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
> - for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
> - ASSERT (Descriptor->ResType <
> - ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr)
> - );
> - DEBUG ((DEBUG_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 ((DEBUG_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
> - );
> - }
> -}
(19) Please do not remove the entire definition of this function.
According to my point (15), only replace the body of this function with
the following function call:
PciHostBridgeUtilityResourceConflict (Configuration);
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> new file mode 100644
> index 000000000000..7e9512dc08f1
> --- /dev/null
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -0,0 +1,74 @@
> +/** @file
> + OVMF's instance of the PCI Host Bridge Library.
> +
> + Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
(20) Please preserve the (C) notices from the original file (from where
the code is coming); namely
"OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c".
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#include <Library/DebugLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
(21) Is this #include list really sufficient to compile the tree when
only this patch is applied?
I would think the following directive is needed, additionally:
#include <IndustryStandard/Acpi10.h>
(I suggest placing it above the Library #includes.)
> +
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED
> +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> + L"Mem", L"I/O", L"Bus"
> +};
> +
> +
(22) Please rename this object to
"mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr" (you'll have to modify
the reference to it below as well).
> +/**
> + 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 (
(23) Please rename to "PciHostBridgeUtilityResourceConflict".
(24) Please drop "HostBridgeHandle", corresponding to point (16).
> + EFI_HANDLE HostBridgeHandle,
> + VOID *Configuration
> + )
> +{
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> + UINTN RootBridgeIndex;
> + DEBUG ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
> +
> + RootBridgeIndex = 0;
> + Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
> + while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> + DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
> + for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
> + ASSERT (Descriptor->ResType <
> + ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr)
> + );
> + DEBUG ((DEBUG_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 ((DEBUG_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
> + );
> + }
> +}
> +
>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 1/4] OvmfPkg: Extract functions form PciHostBridgeLib
2020-11-11 16:45 ` Laszlo Ersek
@ 2020-11-12 3:21 ` Jiahui Cen
0 siblings, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-11-12 3:21 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo
Hi Laszlo,
Thanks for your detailed review. Will fix them in v3.
Jiahui
On 2020/11/12 0:45, Laszlo Ersek wrote:
> On 11/09/20 14:05, Jiahui Cen wrote:
>> From: Yubo Miao <miaoyubo@huawei.com>
>>
>> Introduce a new PciHostBridgeUtilityLib class to share duplicate code
>> between OvmfPkg and ArmVirtPkg.
>>
>> Extract function PciHostBridgeResourceConflict from
>> OvmfPkg/PciHostBridgeLib.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>> OvmfPkg/OvmfXen.dsc | 1 +
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 49 +++++++++++++
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 33 +++++++++
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 64 +----------------
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 74 ++++++++++++++++++++
>> 9 files changed, 162 insertions(+), 63 deletions(-)
>
> (1) There is a typo in the subject: s/form/from/
>
>
> (2) "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf" is consumed
> in the following platform DSC file as well -- please update it too:
>
> OvmfPkg/Bhyve/BhyveX64.dsc
>
> (This will affect the list of necessary CC's on this patch.)
>
>
> (3) In my v1 review at <https://edk2.groups.io/g/devel/message/57062>
> (msgid <57fd0043-d63a-55b0-9c55-4ca079331885@redhat.com>), I mentioned
> "OvmfPkg/OvmfPkg.dec".
>
> You forgot to extend that file, with the new library class. Please add
> the following hunk to this patch:
>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 118c968fda4e..b89252eeab2a 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -49,6 +49,10 @@ [LibraryClasses]
>> # access.
>> PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h
>>
>> + ## @libraryclass Provide common utility functions to PciHostBridgeLib
>> + # instances in ArmVirtPkg and OvmfPkg.
>> + PciHostBridgeUtilityLib|Include/Library/PciHostBridgeUtilityLib.h
>> +
>> ## @libraryclass Register a status code handler for printing the Boot
>> # Manager's LoadImage() and StartImage() preparations, and
>> # return codes, to the UEFI console.
>
>
> (4) Issue (3) above would have been caught by CI, namely by the
> "LibraryClassCheck" plugin:
>
> https://github.com/tianocore/edk2/blob/master/.pytool/Readme.md#library-declaration-test---libraryclasscheck
>
> In order to put your patches through CI before posting, I suggest
> pushing them to a branch in your edk2 clone on github.com, and
> submitting a pull request against the central repository. The
> pull request will be auto-closed in the end, unconditionally, but it
> will give you CI results.
>
> (You can also run CI locally, but setting that up is laborious.)
>
> Back to your patch:
>
>
> On 11/09/20 14:05, Jiahui Cen wrote:
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 58d9f292f9ac..0c2bf0b13c34 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -738,6 +738,7 @@ [Components]
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> <LibraryClasses>
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 3551f9710a6c..baf36a4f7a54 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -752,6 +752,7 @@ [Components.X64]
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> <LibraryClasses>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 7a8bdb8a8697..219b5f559b53 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -748,6 +748,7 @@ [Components]
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> <LibraryClasses>
>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>> index 34c9de19dfba..442c0730ef32 100644
>> --- a/OvmfPkg/OvmfXen.dsc
>> +++ b/OvmfPkg/OvmfXen.dsc
>> @@ -544,6 +544,7 @@ [Components]
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> <LibraryClasses>
>
> (5) Please keep the PciHostBridgeUtilityLib class resolution right next
> to the PciHostBridgeLib one.
>
>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> index 6ec9ec751478..7d01528c94f1 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> @@ -41,6 +41,7 @@ [LibraryClasses]
>> MemoryAllocationLib
>> PciLib
>> QemuFwCfgLib
>> + PciHostBridgeUtilityLib
>>
>> [Pcd]
>> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
>
> (6) Please keep the entries under [LibraryClasses] alphabetically
> sorted.
>
>
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> new file mode 100644
>> index 000000000000..c88ab8e4155d
>> --- /dev/null
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> @@ -0,0 +1,49 @@
>> +## @file
>> +# OVMF and Arm's instance of the PCI Host Bridge Utility Library.
>
> (7) I suggest the following description:
>
> Provide common utility functions to PciHostBridgeLib instances in
> ArmVirtPkg and OvmfPkg.
>
>
>> +#
>> +# Copyright (C) 2016, Red Hat, Inc.
>> +# Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>
> (8) It makes sense to duplicate the (C) notices from the original file
> (namely "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf"), but
> this is still a new file due to your contribution.
>
> Therefore, please add a proper Huawei copyright notice.
>
>
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +#
>> +##
>> +
>> +[Defines]
>> + INF_VERSION = 0x00010005
>
> (9) The latest specified INF version is 1.29, according to
> <https://edk2-docs.gitbook.io/edk-ii-inf-specification/#edk-ii-module-information-inf-file-specification>;
> please write
>
> INF_VERSION = 1.29
>
>
>> + BASE_NAME = PciHostBridgeUtilityLib
>> + FILE_GUID = e3aa5932-527a-42e7-86f5-81b144c7e5f1
>> + MODULE_TYPE = DXE_DRIVER
>> + VERSION_STRING = 1.0
>> + LIBRARY_CLASS = PciHostBridgeUtilityLib
>> +
>> +#
>> +# The following information is for reference only and not required by the build
>> +# tools.
>> +#
>> +# VALID_ARCHITECTURES = IA32 X64 EBC
>> +#
>> +
>
> (10) EBC is gone, plus we'll want to plug this lib into
> "ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf", which
> itself names AARCH64 and ARM for VALID_ARCHITECTURES.
>
> Therefore, please state "IA32 X64 AARCH64 ARM" here.
>
>
>> +[Sources]
>> + PciHostBridgeUtilityLib.c
>> +
>> +[Packages]
>> + MdePkg/MdePkg.dec
>> + OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> + BaseMemoryLib
>> + DebugLib
>> + DevicePathLib
>> + MemoryAllocationLib
>> + PciLib
>> + QemuFwCfgLib
>
> (11) The list of required (consumed) library classes, along with the
> library class header files included in "PciHostBridgeUtilityLib.c",
> should be as minimal as possible (and alphabetically sorted).
>
> Specifically, you only need to keep "DebugLib"; everything else should
> be removed, as far as I can tell.
>
> (We can extend the [LibraryClasses] section in later patches, if
> necessary.)
>
>
>> +
>> +[Pcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
>> + gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize
>> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base
>> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
>> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
>> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>
> (12) The [Pcd] section should be dropped altogether.
>
>
>> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> new file mode 100644
>> index 000000000000..d2622fd907e6
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> @@ -0,0 +1,33 @@
>> +/** @file
>> + PCI Host Bridge Library consumed by PciHostBridgeDxe driver returning
>> + the platform specific information about the PCI Host Bridge.
>
> (13) This file-top comment should say:
>
> Provide common utility functions to PciHostBridgeLib instances in
> ArmVirtPkg and OvmfPkg.
>
>
>> +
>> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>
> (14) It's fine to keep the (C) notices from the file that you are using
> as template, but you should please add a Huawei (C) notice as well.
>
>
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +#ifndef __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>> +#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>> +
>> +/**
>> + 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
>> + );
>> +
>
> (15) Please rename this function to
> "PciHostBridgeUtilityResourceConflict".
>
> The idea is that no PciHostBridgeUtilityLib instance should directly
> implement a PciHostBridgeLib API. PciHostBridgeUtilityLib can offer APIs
> that are as "fat" as necessary, taking over as much work as possible,
> but we should keep the namespaces isolated.
>
>
> (16) In our helper library, the "HostBridgeHandle" parameter is not
> used; please remove it from both the leading comment on the function,
> and also from the function declaration.
>
>
>> +#endif
>
> (17) Please append a comment to this line:
>
> // __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>
>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> index e850f7d183ee..1c8f465834f3 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> @@ -22,6 +22,7 @@
>> #include <Library/PciHostBridgeLib.h>
>> #include <Library/PciLib.h>
>> #include <Library/QemuFwCfgLib.h>
>> +#include <Library/PciHostBridgeUtilityLib.h>
>> #include "PciHostBridge.h"
>>
>>
>
> (18) Please keep the #include list of the Library class headers
> alphabetically sorted.
>
>
>> @@ -33,12 +34,6 @@ typedef struct {
>> #pragma pack ()
>>
>>
>> -GLOBAL_REMOVE_IF_UNREFERENCED
>> -CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
>> - L"Mem", L"I/O", L"Bus"
>> -};
>> -
>> -
>> STATIC
>> CONST
>> OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePathTemplate = {
>> @@ -384,60 +379,3 @@ PciHostBridgeFreeRootBridges (
>>
>> FreePool (Bridges);
>> }
>> -
>> -
>> -/**
>> - 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 ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
>> -
>> - RootBridgeIndex = 0;
>> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
>> - while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
>> - DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
>> - for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
>> - ASSERT (Descriptor->ResType <
>> - ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr)
>> - );
>> - DEBUG ((DEBUG_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 ((DEBUG_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
>> - );
>> - }
>> -}
>
> (19) Please do not remove the entire definition of this function.
> According to my point (15), only replace the body of this function with
> the following function call:
>
> PciHostBridgeUtilityResourceConflict (Configuration);
>
>
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> new file mode 100644
>> index 000000000000..7e9512dc08f1
>> --- /dev/null
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> @@ -0,0 +1,74 @@
>> +/** @file
>> + OVMF's instance of the PCI Host Bridge Library.
>> +
>> + Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
>
> (20) Please preserve the (C) notices from the original file (from where
> the code is coming); namely
> "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c".
>
>
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +#include <Library/DebugLib.h>
>> +#include <Library/PciHostBridgeUtilityLib.h>
>
> (21) Is this #include list really sufficient to compile the tree when
> only this patch is applied?
>
> I would think the following directive is needed, additionally:
>
> #include <IndustryStandard/Acpi10.h>
>
> (I suggest placing it above the Library #includes.)
>
>
>> +
>> +
>> +GLOBAL_REMOVE_IF_UNREFERENCED
>> +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
>> + L"Mem", L"I/O", L"Bus"
>> +};
>> +
>> +
>
> (22) Please rename this object to
> "mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr" (you'll have to modify
> the reference to it below as well).
>
>
>> +/**
>> + 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 (
>
> (23) Please rename to "PciHostBridgeUtilityResourceConflict".
>
> (24) Please drop "HostBridgeHandle", corresponding to point (16).
>
>
>> + EFI_HANDLE HostBridgeHandle,
>> + VOID *Configuration
>> + )
>> +{
>> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
>> + UINTN RootBridgeIndex;
>> + DEBUG ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
>> +
>> + RootBridgeIndex = 0;
>> + Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
>> + while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
>> + DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
>> + for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
>> + ASSERT (Descriptor->ResType <
>> + ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr)
>> + );
>> + DEBUG ((DEBUG_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 ((DEBUG_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
>> + );
>> + }
>> +}
>> +
>>
>
> Thanks!
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
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-09 13:05 ` Jiahui Cen
2020-11-11 17:27 ` Laszlo Ersek
2020-11-09 13:05 ` [PATCH v2 3/4] OvmfPkg: Extract functions of extra pci roots Jiahui Cen
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Jiahui Cen @ 2020-11-09 13:05 UTC (permalink / raw)
To: devel
Cc: jordan.l.justen, lersek, ard.biesheuvel, leif, xieyingtai,
miaoyubo, Jiahui Cen
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
2020-11-09 13:05 ` [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib Jiahui Cen
@ 2020-11-11 17:27 ` Laszlo Ersek
2020-11-12 3:30 ` [edk2-devel] " Jiahui Cen
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-11-11 17:27 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [edk2-devel] [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
2020-11-11 17:27 ` Laszlo Ersek
@ 2020-11-12 3:30 ` Jiahui Cen
0 siblings, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-11-12 3:30 UTC (permalink / raw)
To: devel, lersek; +Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo
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
>
>
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/4] OvmfPkg: Extract functions of extra pci roots
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-09 13:05 ` [PATCH v2 2/4] ArmVirtPkg: Use extracted PciHostBridgeUtilityLib Jiahui Cen
@ 2020-11-09 13:05 ` Jiahui Cen
2020-11-09 13:05 ` [PATCH v2 4/4] ArmVirtPkg: Support " Jiahui Cen
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-11-09 13:05 UTC (permalink / raw)
To: devel
Cc: jordan.l.justen, lersek, ard.biesheuvel, leif, xieyingtai,
miaoyubo, Jiahui Cen
From: Yubo Miao <miaoyubo@huawei.com>
To use extra pci roots in other lib, it is necessary to extract
UninitRootBridge, PciHostBridgeFreeRootBridges and
PciHostBridgeExtraRoots from Ovmf/PciHostBridgeLib.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 2 +
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 65 ++++++
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 170 ++--------------
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 209 ++++++++++++++++++++
4 files changed, 287 insertions(+), 159 deletions(-)
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
index c88ab8e4155d..1a667870ee2f 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -28,6 +28,7 @@ [Sources]
PciHostBridgeUtilityLib.c
[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
@@ -38,6 +39,7 @@ [LibraryClasses]
MemoryAllocationLib
PciLib
QemuFwCfgLib
+ PciHostBridgeLib
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
index d2622fd907e6..35fe237b0801 100644
--- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -9,6 +9,71 @@
#ifndef __PCI_HOST_BRIDGE_UTILITY_LIB_H__
#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
+/**
+ Uninitialize a PCI_ROOT_BRIDGE structure set up with InitRootBridge().
+
+ param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
+ initialized with InitRootBridge(), that should be
+ uninitialized. This function doesn't free RootBus.
+**/
+VOID
+UninitRootBridge (
+ IN PCI_ROOT_BRIDGE *RootBus
+ );
+
+/**
+ Free the root bridge instances array returned from
+ PciHostBridgeGetRootBridges().
+
+ @param The root bridge instances array.
+ @param The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeFreeRootBridges (
+ PCI_ROOT_BRIDGE *Bridges,
+ UINTN Count
+ );
+
+/**
+ Return all the root bridge instances in an array.
+
+ @param Count Return the count of root bridge instances.
+
+ @param[in] PMem Prefetchable MMIO aperture.
+
+ @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+
+ @param[in] BusMax The max bus number.
+
+ @param[in] Attributes Initial attributes.
+
+ @param[in] AllocAttributes Allocation attributes.
+
+ @param[in] Io IO aperture.
+
+ @param[in] Mem MMIO aperture.
+
+ @param[in] MemAbove4G MMIO aperture above 4G.
+
+ @return All the root bridge instances in an array.
+ The array should be passed into PciHostBridgeFreeRootBridges()
+ when it's not used.
+**/
+PCI_ROOT_BRIDGE *
+EFIAPI
+PciHostBridgeExtraRoots (
+ UINTN *Count,
+ PCI_ROOT_BRIDGE_APERTURE PMem,
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G,
+ UINT32 BusMax,
+ UINT64 Attributes,
+ UINT64 AllocationAttributes,
+ PCI_ROOT_BRIDGE_APERTURE Io,
+ PCI_ROOT_BRIDGE_APERTURE Mem,
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G
+);
+
/**
Inform the platform that the resource conflict happens.
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index 1c8f465834f3..cd9896bee14b 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -159,24 +159,6 @@ InitRootBridge (
return EFI_SUCCESS;
}
-
-/**
- Uninitialize a PCI_ROOT_BRIDGE structure set up with InitRootBridge().
-
- param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
- initialized with InitRootBridge(), that should be
- uninitialized. This function doesn't free RootBus.
-**/
-STATIC
-VOID
-UninitRootBridge (
- IN PCI_ROOT_BRIDGE *RootBus
- )
-{
- FreePool (RootBus->DevicePath);
-}
-
-
/**
Return all the root bridge instances in an array.
@@ -192,14 +174,7 @@ PciHostBridgeGetRootBridges (
UINTN *Count
)
{
- EFI_STATUS Status;
- FIRMWARE_CONFIG_ITEM FwCfgItem;
- UINTN FwCfgSize;
- UINT64 ExtraRootBridges;
PCI_ROOT_BRIDGE *Bridges;
- UINTN Initialized;
- UINTN LastRootBridgeNumber;
- UINTN RootBridgeNumber;
UINT64 Attributes;
UINT64 AllocationAttributes;
PCI_ROOT_BRIDGE_APERTURE Io;
@@ -239,143 +214,20 @@ PciHostBridgeGetRootBridges (
*Count = 0;
- //
- // QEMU provides the number of extra root buses, shortening the exhaustive
- // search below. If there is no hint, the feature is missing.
- //
- Status = QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize);
- if (EFI_ERROR (Status) || FwCfgSize != sizeof ExtraRootBridges) {
- ExtraRootBridges = 0;
- } else {
- QemuFwCfgSelectItem (FwCfgItem);
- QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
-
- if (ExtraRootBridges > PCI_MAX_BUS) {
- DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
- "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
- return NULL;
- }
- DEBUG ((DEBUG_INFO, "%a: %Lu extra root buses reported by QEMU\n",
- __FUNCTION__, ExtraRootBridges));
- }
-
- //
- // Allocate the "main" root bridge, and any extra root bridges.
- //
- Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges);
- if (Bridges == NULL) {
- DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
- return NULL;
- }
- Initialized = 0;
-
- //
- // The "main" root bus is always there.
- //
- LastRootBridgeNumber = 0;
-
- //
- // Scan all other root buses. If function 0 of any device on a bus returns a
- // VendorId register value different from all-bits-one, then that bus is
- // alive.
- //
- for (RootBridgeNumber = 1;
- RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges;
- ++RootBridgeNumber) {
- UINTN Device;
-
- for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) {
- if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0,
- PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) {
- break;
- }
- }
- if (Device <= PCI_MAX_DEVICE) {
- //
- // Found the next root bus. We can now install the *previous* one,
- // because now we know how big a bus number range *that* one has, for any
- // subordinate buses that might exist behind PCI bridges hanging off it.
- //
- Status = InitRootBridge (
- Attributes,
- Attributes,
- AllocationAttributes,
- (UINT8) LastRootBridgeNumber,
- (UINT8) (RootBridgeNumber - 1),
- &Io,
- &Mem,
- &MemAbove4G,
- &mNonExistAperture,
- &mNonExistAperture,
- &Bridges[Initialized]
- );
- if (EFI_ERROR (Status)) {
- goto FreeBridges;
- }
- ++Initialized;
- LastRootBridgeNumber = RootBridgeNumber;
- }
- }
-
- //
- // Install the last root bus (which might be the only, ie. main, root bus, if
- // we've found no extra root buses).
- //
- Status = InitRootBridge (
- Attributes,
- Attributes,
- AllocationAttributes,
- (UINT8) LastRootBridgeNumber,
+ Bridges = PciHostBridgeExtraRoots (
+ Count,
+ mNonExistAperture,
+ mNonExistAperture,
PCI_MAX_BUS,
- &Io,
- &Mem,
- &MemAbove4G,
- &mNonExistAperture,
- &mNonExistAperture,
- &Bridges[Initialized]
+ Attributes,
+ AllocationAttributes,
+ Io,
+ Mem,
+ MemAbove4G
);
- if (EFI_ERROR (Status)) {
- goto FreeBridges;
+ if (Bridges) {
+ return Bridges;
}
- ++Initialized;
-
- *Count = Initialized;
- return Bridges;
-
-FreeBridges:
- while (Initialized > 0) {
- --Initialized;
- UninitRootBridge (&Bridges[Initialized]);
- }
-
- FreePool (Bridges);
return NULL;
}
-
-/**
- Free the root bridge instances array returned from
- PciHostBridgeGetRootBridges().
-
- @param The root bridge instances array.
- @param The count of the array.
-**/
-VOID
-EFIAPI
-PciHostBridgeFreeRootBridges (
- PCI_ROOT_BRIDGE *Bridges,
- UINTN Count
- )
-{
- if (Bridges == NULL && Count == 0) {
- return;
- }
- ASSERT (Bridges != NULL && Count > 0);
-
- do {
- --Count;
- UninitRootBridge (&Bridges[Count]);
- } while (Count > 0);
-
- FreePool (Bridges);
-}
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
index 7e9512dc08f1..422f46fef4dd 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -6,8 +6,14 @@
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
+#include <IndustryStandard/Pci.h>
#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PciHostBridgeLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/PciLib.h>
+#include "Library/PciHostBridgeLib/PciHostBridge.h"
GLOBAL_REMOVE_IF_UNREFERENCED
@@ -16,6 +22,209 @@ CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
};
+/**
+ Uninitialize a PCI_ROOT_BRIDGE structure set up with InitRootBridge().
+
+ param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
+ initialized with InitRootBridge(), that should be
+ uninitialized. This function doesn't free RootBus.
+**/
+VOID
+UninitRootBridge (
+ IN PCI_ROOT_BRIDGE *RootBus
+ )
+{
+ FreePool (RootBus->DevicePath);
+}
+
+/**
+ Free the root bridge instances array returned from
+ PciHostBridgeGetRootBridges().
+
+ @param The root bridge instances array.
+ @param The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeFreeRootBridges (
+ PCI_ROOT_BRIDGE *Bridges,
+ UINTN Count
+ )
+{
+ if (Bridges == NULL && Count == 0) {
+ return;
+ }
+ ASSERT (Bridges != NULL && Count > 0);
+
+ do {
+ --Count;
+ UninitRootBridge (&Bridges[Count]);
+ } while (Count > 0);
+
+ FreePool (Bridges);
+}
+
+/**
+ Return all the root bridge instances in an array.
+
+ @param Count Return the count of root bridge instances.
+
+ @param[in] PMem Prefetchable MMIO aperture.
+
+ @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+
+ @param[in] BusMax The max bus number.
+
+ @param[in] Attributes Initial attributes.
+
+ @param[in] AllocAttributes Allocation attributes.
+
+ @param[in] Io IO aperture.
+
+ @param[in] Mem MMIO aperture.
+
+ @param[in] MemAbove4G MMIO aperture above 4G.
+
+ @return All the root bridge instances in an array.
+ The array should be passed into PciHostBridgeFreeRootBridges()
+ when it's not used.
+**/
+
+PCI_ROOT_BRIDGE *
+EFIAPI
+PciHostBridgeExtraRoots (
+ UINTN *Count,
+ PCI_ROOT_BRIDGE_APERTURE PMem,
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G,
+ UINT32 BusMax,
+ UINT64 Attributes,
+ UINT64 AllocationAttributes,
+ PCI_ROOT_BRIDGE_APERTURE Io,
+ PCI_ROOT_BRIDGE_APERTURE Mem,
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G
+)
+{
+ EFI_STATUS Status;
+ PCI_ROOT_BRIDGE *Bridges;
+ FIRMWARE_CONFIG_ITEM FwCfgItem;
+ UINTN FwCfgSize;
+ UINT64 ExtraRootBridges;
+ UINTN Initialized;
+ UINTN LastRootBridgeNumber;
+ UINTN RootBridgeNumber;
+
+ //
+ // QEMU provides the number of extra root buses, shortening the exhaustive
+ // search below. If there is no hint, the feature is missing.
+ //
+ Status = QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize);
+ if (EFI_ERROR (Status) || FwCfgSize != sizeof ExtraRootBridges) {
+ ExtraRootBridges = 0;
+ } else {
+ QemuFwCfgSelectItem (FwCfgItem);
+ QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
+
+ if (ExtraRootBridges > BusMax) {
+ DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
+ "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
+ return NULL;
+ }
+ DEBUG ((DEBUG_INFO, "%a: %Lu extra root buses reported by QEMU\n",
+ __FUNCTION__, ExtraRootBridges));
+ }
+
+ //
+ // Allocate the "main" root bridge, and any extra root bridges.
+ //
+ Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges);
+ if (Bridges == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
+ return NULL;
+ }
+ Initialized = 0;
+
+ //
+ // The "main" root bus is always there.
+ //
+ LastRootBridgeNumber = 0;
+
+ //
+ // Scan all other root buses. If function 0 of any device on a bus returns a
+ // VendorId register value different from all-bits-one, then that bus is
+ // alive.
+ //
+ for (RootBridgeNumber = 1;
+ RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
+ ++RootBridgeNumber) {
+ UINTN Device;
+
+ for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) {
+ if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0,
+ PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) {
+ break;
+ }
+ }
+ if (Device <= PCI_MAX_DEVICE) {
+ //
+ // Found the next root bus. We can now install the *previous* one,
+ // because now we know how big a bus number range *that* one has, for any
+ // subordinate buses that might exist behind PCI bridges hanging off it.
+ //
+ Status = InitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ (UINT8) LastRootBridgeNumber,
+ (UINT8) (RootBridgeNumber - 1),
+ &Io,
+ &Mem,
+ &MemAbove4G,
+ &PMem,
+ &PMemAbove4G,
+ &Bridges[Initialized]
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBridges;
+ }
+ ++Initialized;
+ LastRootBridgeNumber = RootBridgeNumber;
+ }
+ }
+
+ //
+ // Install the last root bus (which might be the only, ie. main, root bus, if
+ // we've found no extra root buses).
+ //
+ Status = InitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ (UINT8) LastRootBridgeNumber,
+ BusMax,
+ &Io,
+ &Mem,
+ &MemAbove4G,
+ &PMem,
+ &PMemAbove4G,
+ &Bridges[Initialized]
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBridges;
+ }
+ ++Initialized;
+
+ *Count = Initialized;
+ return Bridges;
+
+FreeBridges:
+ while (Initialized > 0) {
+ --Initialized;
+ UninitRootBridge (&Bridges[Initialized]);
+ }
+
+ FreePool (Bridges);
+ return NULL;
+}
/**
Inform the platform that the resource conflict happens.
--
2.28.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 4/4] ArmVirtPkg: Support extra pci roots
2020-11-09 13:05 [PATCH v2 0/4] Add extra pci roots support for Arm Jiahui Cen
` (2 preceding siblings ...)
2020-11-09 13:05 ` [PATCH v2 3/4] OvmfPkg: Extract functions of extra pci roots Jiahui Cen
@ 2020-11-09 13:05 ` Jiahui Cen
2020-11-11 14:33 ` [PATCH v2 0/4] Add extra pci roots support for Arm Laszlo Ersek
2020-11-12 8:49 ` Ard Biesheuvel
5 siblings, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-11-09 13:05 UTC (permalink / raw)
To: devel
Cc: jordan.l.justen, lersek, ard.biesheuvel, leif, xieyingtai,
miaoyubo, Jiahui Cen
From: Yubo Miao <miaoyubo@huawei.com>
As the implementation of extra pci roots is shared in
PciHostBridgeUtilityLib, let's call PciHostBridgeExtraRoots
to support it.
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/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 3 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 162 +++++++++++++-------
2 files changed, 111 insertions(+), 54 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
index 97e9368c8e9f..1b0f9a2b9013 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
@@ -39,6 +39,9 @@ [LibraryClasses]
DxeServicesTableLib
MemoryAllocationLib
PciPcdProducerLib
+ PciHostBridgeLib
+ BaseMemoryLib
+ QemuFwCfgLib
PciHostBridgeUtilityLib
[FixedPcd]
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 3952f511b4d2..d33f833a5040 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -15,6 +15,11 @@
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/PciLib.h>
+#include <IndustryStandard/Pci.h>
+#include <Library/BaseMemoryLib.h>
+#include "Library/PciHostBridgeLib/PciHostBridge.h"
#include <Protocol/FdtClient.h>
#include <Protocol/PciRootBridgeIo.h>
@@ -304,7 +309,60 @@ ProcessPciHost (
return Status;
}
-STATIC PCI_ROOT_BRIDGE mRootBridge;
+EFI_STATUS
+InitRootBridge (
+ IN UINT64 Supports,
+ IN UINT64 Attributes,
+ IN UINT64 AllocAttributes,
+ IN UINT8 RootBusNumber,
+ IN UINT8 MaxSubBusNumber,
+ IN PCI_ROOT_BRIDGE_APERTURE *Io,
+ IN PCI_ROOT_BRIDGE_APERTURE *Mem,
+ IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
+ IN PCI_ROOT_BRIDGE_APERTURE *PMem,
+ IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G,
+ OUT PCI_ROOT_BRIDGE *RootBus
+ )
+{
+ EFI_PCI_ROOT_BRIDGE_DEVICE_PATH *DevicePath;
+
+ //
+ // Be safe if other fields are added to PCI_ROOT_BRIDGE later.
+ //
+ ZeroMem (RootBus, sizeof *RootBus);
+
+ RootBus->Segment = 0;
+ RootBus->ResourceAssigned = FALSE;
+ RootBus->Supports = Supports;
+ RootBus->Attributes = Attributes;
+
+ RootBus->DmaAbove4G = TRUE;
+
+ RootBus->AllocationAttributes = AllocAttributes;
+ RootBus->Bus.Base = RootBusNumber;
+ RootBus->Bus.Limit = MaxSubBusNumber;
+ CopyMem (&RootBus->Io, Io, sizeof (*Io));
+ CopyMem (&RootBus->Mem, Mem, sizeof (*Mem));
+ CopyMem (&RootBus->MemAbove4G, MemAbove4G, sizeof (*MemAbove4G));
+ CopyMem (&RootBus->PMem, PMem, sizeof (*PMem));
+ CopyMem (&RootBus->PMemAbove4G, PMemAbove4G, sizeof (*PMemAbove4G));
+
+ RootBus->NoExtendedConfigSpace = FALSE;
+
+ DevicePath = AllocateCopyPool (sizeof mEfiPciRootBridgeDevicePath,
+ &mEfiPciRootBridgeDevicePath);
+ if (DevicePath == NULL) {
+ DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ DevicePath->AcpiDevicePath.UID = RootBusNumber;
+ RootBus->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)DevicePath;
+
+ DEBUG ((EFI_D_INFO,
+ "%a: populated root bus %d, with room for %d subordinate bus(es)\n",
+ __FUNCTION__, RootBusNumber, MaxSubBusNumber - RootBusNumber));
+ return EFI_SUCCESS;
+}
/**
Return all the root bridge instances in an array.
@@ -321,11 +379,19 @@ PciHostBridgeGetRootBridges (
UINTN *Count
)
{
- UINT64 IoBase, IoSize;
- UINT64 Mmio32Base, Mmio32Size;
- UINT64 Mmio64Base, Mmio64Size;
- UINT32 BusMin, BusMax;
- EFI_STATUS Status;
+ UINT64 IoBase, IoSize;
+ UINT64 Mmio32Base, Mmio32Size;
+ UINT64 Mmio64Base, Mmio64Size;
+ UINT32 BusMin, BusMax;
+ PCI_ROOT_BRIDGE *Bridges;
+ UINT64 Attributes;
+ UINT64 AllocationAttributes;
+ EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_APERTURE Io;
+ PCI_ROOT_BRIDGE_APERTURE PMem;
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
+ PCI_ROOT_BRIDGE_APERTURE Mem;
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
if (PcdGet64 (PcdPciExpressBaseAddress) == 0) {
DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__));
@@ -343,33 +409,27 @@ PciHostBridgeGetRootBridges (
return NULL;
}
- *Count = 1;
+ ZeroMem (&Io, sizeof (Io));
+ ZeroMem (&Mem, sizeof (Mem));
+ ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
- mRootBridge.Segment = 0;
- mRootBridge.Supports = EFI_PCI_ATTRIBUTE_ISA_IO_16 |
- EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
- EFI_PCI_ATTRIBUTE_VGA_IO_16 |
- EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
- mRootBridge.Attributes = mRootBridge.Supports;
+ Attributes = EFI_PCI_ATTRIBUTE_ISA_IO_16 |
+ EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
+ EFI_PCI_ATTRIBUTE_VGA_IO_16 |
+ EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
- mRootBridge.DmaAbove4G = TRUE;
- mRootBridge.NoExtendedConfigSpace = FALSE;
- mRootBridge.ResourceAssigned = FALSE;
+ AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
- mRootBridge.AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
-
- mRootBridge.Bus.Base = BusMin;
- mRootBridge.Bus.Limit = BusMax;
- mRootBridge.Io.Base = IoBase;
- mRootBridge.Io.Limit = IoBase + IoSize - 1;
- mRootBridge.Mem.Base = Mmio32Base;
- mRootBridge.Mem.Limit = Mmio32Base + Mmio32Size - 1;
+ Io.Base = IoBase;
+ Io.Limit = IoBase + IoSize - 1;
+ Mem.Base = Mmio32Base;
+ Mem.Limit = Mmio32Base + Mmio32Size - 1;
if (sizeof (UINTN) == sizeof (UINT64)) {
- mRootBridge.MemAbove4G.Base = Mmio64Base;
- mRootBridge.MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
+ MemAbove4G.Base = Mmio64Base;
+ MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
if (Mmio64Size > 0) {
- mRootBridge.AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
+ AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
}
} else {
//
@@ -378,37 +438,31 @@ PciHostBridgeGetRootBridges (
// BARs unless they are allocated below 4 GB. So ignore the range above
// 4 GB in this case.
//
- mRootBridge.MemAbove4G.Base = MAX_UINT64;
- mRootBridge.MemAbove4G.Limit = 0;
+ MemAbove4G.Base = MAX_UINT64;
+ MemAbove4G.Limit = 0;
}
//
// No separate ranges for prefetchable and non-prefetchable BARs
//
- mRootBridge.PMem.Base = MAX_UINT64;
- mRootBridge.PMem.Limit = 0;
- mRootBridge.PMemAbove4G.Base = MAX_UINT64;
- mRootBridge.PMemAbove4G.Limit = 0;
+ PMem.Base = MAX_UINT64;
+ PMem.Limit = 0;
+ PMemAbove4G.Base = MAX_UINT64;
+ PMemAbove4G.Limit = 0;
- mRootBridge.DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath;
-
- return &mRootBridge;
+ Bridges = PciHostBridgeExtraRoots (
+ Count,
+ PMem,
+ PMemAbove4G,
+ BusMax,
+ Attributes,
+ AllocationAttributes,
+ Io,
+ Mem,
+ MemAbove4G
+ );
+ if (Bridges) {
+ return Bridges;
+ }
+ return NULL;
}
-
-/**
- Free the root bridge instances array returned from
- PciHostBridgeGetRootBridges().
-
- @param Bridges The root bridge instances array.
- @param Count The count of the array.
-**/
-VOID
-EFIAPI
-PciHostBridgeFreeRootBridges (
- PCI_ROOT_BRIDGE *Bridges,
- UINTN Count
- )
-{
- ASSERT (Count == 1);
-}
-
--
2.28.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 0/4] Add extra pci roots support for Arm
2020-11-09 13:05 [PATCH v2 0/4] Add extra pci roots support for Arm Jiahui Cen
` (3 preceding siblings ...)
2020-11-09 13:05 ` [PATCH v2 4/4] ArmVirtPkg: Support " Jiahui Cen
@ 2020-11-11 14:33 ` Laszlo Ersek
2020-11-12 3:20 ` [edk2-devel] " Jiahui Cen
2020-12-04 6:48 ` Jiahui Cen
2020-11-12 8:49 ` Ard Biesheuvel
5 siblings, 2 replies; 23+ messages in thread
From: Laszlo Ersek @ 2020-11-11 14:33 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo
On 11/09/20 14:05, Jiahui Cen wrote:
> Changes with v1
> v1->v2:
> Separated into four patches.
> Factor the same logic parts into a new library.
>
> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
(1) Each commit message in the series should reference this bugzilla.
But, in itself, that's no reason for a repost; such an update can be
made by maintainers when they merge the series.
(2) This is a feature addition, so it's merge material for the next
development cycle
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
(Also it depends on the QEMU work being merged first.) I'm going to
proceed with the review now.
Thanks,
Laszlo
> QEMU: https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>
> This patch series adds support for extra pci roots for ARM.
>
> In order to avoid duplicated codes, we introduce a new library
> PciHostBridgeUtilityLib which extracts common interfaces from
> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci
> roots scanning. Using the utility lib, the uefi could scan for extra
> root buses and recognize multiple roots for ARM.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 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>
>
> Yubo Miao (4):
> OvmfPkg: Extract functions form PciHostBridgeLib
> ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
> OvmfPkg: Extract functions of extra pci roots
> ArmVirtPkg: Support extra pci roots
>
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/OvmfXen.dsc | 1 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 5 +
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 51 ++++
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 98 +++++++
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 221 ++++++++-------
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 234 +---------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 283 ++++++++++++++++++++
> 12 files changed, 563 insertions(+), 335 deletions(-)
> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-11-11 14:33 ` [PATCH v2 0/4] Add extra pci roots support for Arm Laszlo Ersek
@ 2020-11-12 3:20 ` Jiahui Cen
2020-12-04 6:48 ` Jiahui Cen
1 sibling, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-11-12 3:20 UTC (permalink / raw)
To: devel, lersek; +Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo
Hi Laszlo,
On 2020/11/11 22:33, Laszlo Ersek wrote:
> On 11/09/20 14:05, Jiahui Cen wrote:
>> Changes with v1
>> v1->v2:
>> Separated into four patches.
>> Factor the same logic parts into a new library.
>>
>> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> (1) Each commit message in the series should reference this bugzilla.
>
> But, in itself, that's no reason for a repost; such an update can be
> made by maintainers when they merge the series.
Will add it.
>
> (2) This is a feature addition, so it's merge material for the next
> development cycle
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
> (Also it depends on the QEMU work being merged first.) I'm going to
> proceed with the review now.
Thanks for the material. I'll keep on developing it and the QEMU work.
Jiahui
>
> Thanks,
> Laszlo
>
>> QEMU: https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>>
>> This patch series adds support for extra pci roots for ARM.
>>
>> In order to avoid duplicated codes, we introduce a new library
>> PciHostBridgeUtilityLib which extracts common interfaces from
>> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci
>> roots scanning. Using the utility lib, the uefi could scan for extra
>> root buses and recognize multiple roots for ARM.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> 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>
>>
>> Yubo Miao (4):
>> OvmfPkg: Extract functions form PciHostBridgeLib
>> ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
>> OvmfPkg: Extract functions of extra pci roots
>> ArmVirtPkg: Support extra pci roots
>>
>> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>> OvmfPkg/OvmfXen.dsc | 1 +
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 5 +
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 51 ++++
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 98 +++++++
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 221 ++++++++-------
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 234 +---------------
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 283 ++++++++++++++++++++
>> 12 files changed, 563 insertions(+), 335 deletions(-)
>> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>>
>
>
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
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
1 sibling, 2 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-12-04 6:48 UTC (permalink / raw)
To: devel, lersek
Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo,
xuxiaoyang2
Hi Laszlo,
During the modification of this patch series, I got a confusing problem
that the EDK2's Alignment seems not fit with Linux on ARM.
EDK2, in CalculateResourceAperture, aligns the offset of child nodes
in descending order and uses the *Bridge's alignment* to align the
total length of Bridge.
However, Linux, in pbus_size_mem, would align the size of child nodes
and use *the half of max alignment of child nodes* to align the total
length of Bridge.
eg. A Root Bridge with Bus [d2]
-+-[0000:d2]---01.0-[d3]----01.0
where [d2:01.00] is a pcie-pci-bridge with BAR0 (mem, 64-bit, non-pref) [size=256]
[d3:01.00] is a PCI Device with BAR0 (mem, 64-bit, pref) [size=128K]
BAR4 (mem, 64-bit, pref) [size=64M]
In EDK2, the Resource Map would be:
PciBus: Resource Map for Root Bridge PciRoot(0xD2)
Type = Mem64; Base = 0x8004000000; Length = 0x4200000; Alignment = 0x3FFFFFF
Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF; Owner = PPB [D2|01|00:**]; Type = PMem64
Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF; Owner = PPB [D2|01|00:10]
PciBus: Resource Map for Bridge [D2|01|00]
Type = PMem64; Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF
Base = 0x8004000000; Length = 0x4000000; Alignment = 0x3FFFFFF; Owner = PCI [D3|01|00:20]
Base = 0x8008000000; Length = 0x20000; Alignment = 0x1FFFF; Owner = PCI [D3|01|00:10]
Type = Mem64; Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF
It shows that with EDK2's alignment [d2:01.00] only requires
a PMem64 resource range of 0x4100000.
However in Linux, [d2:01.00]'s BAR14 (Prefetchable Memory
Resource behind the Bridge) requires a PMem64 resource range
of 0x06000000, which comes from (0x4000000 + 0x20000) with
alignment=0x1FFFFFF the half of the max alignment 0x3FFFFFF.
Therefore, the assignment for [d2:01.00]'s BAR14 will fail.
The difference could make the resource range allocated in EDK2 smaller
than the resource range needed in Linux, which causes the assignment
failure in Linux.
The same difference also occurs when io resource allocation.
To handle this senario, is it necessary to calculate the resource map
in Linux way?
Furthermore, Linux Kernel will treat pcie-root-port as a hotplugable
bridge and try to require more resource. But EDK2 seems not support
hotplug padding for ARM?
Thanks,
Jiahui
On 2020/11/11 22:33, Laszlo Ersek wrote:
> On 11/09/20 14:05, Jiahui Cen wrote:
>> Changes with v1
>> v1->v2:
>> Separated into four patches.
>> Factor the same logic parts into a new library.
>>
>> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> (1) Each commit message in the series should reference this bugzilla.
>
> But, in itself, that's no reason for a repost; such an update can be
> made by maintainers when they merge the series.
>
> (2) This is a feature addition, so it's merge material for the next
> development cycle
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
> (Also it depends on the QEMU work being merged first.) I'm going to
> proceed with the review now.
>
> Thanks,
> Laszlo
>
>> QEMU: https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>>
>> This patch series adds support for extra pci roots for ARM.
>>
>> In order to avoid duplicated codes, we introduce a new library
>> PciHostBridgeUtilityLib which extracts common interfaces from
>> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci
>> roots scanning. Using the utility lib, the uefi could scan for extra
>> root buses and recognize multiple roots for ARM.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> 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>
>>
>> Yubo Miao (4):
>> OvmfPkg: Extract functions form PciHostBridgeLib
>> ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
>> OvmfPkg: Extract functions of extra pci roots
>> ArmVirtPkg: Support extra pci roots
>>
>> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>> OvmfPkg/OvmfXen.dsc | 1 +
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 5 +
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 51 ++++
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 98 +++++++
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 221 ++++++++-------
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 234 +---------------
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 283 ++++++++++++++++++++
>> 12 files changed, 563 insertions(+), 335 deletions(-)
>> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>>
>
>
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-12-04 6:48 ` Jiahui Cen
@ 2020-12-04 15:08 ` Laszlo Ersek
2020-12-11 10:57 ` Ni, Ray
1 sibling, 0 replies; 23+ messages in thread
From: Laszlo Ersek @ 2020-12-04 15:08 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: jordan.l.justen, ard.biesheuvel, leif, xieyingtai, miaoyubo,
xuxiaoyang2, Alex Williamson, Ray Ni
+Alex, +Ray
comment below
On 12/04/20 07:48, Jiahui Cen wrote:
> Hi Laszlo,
>
> During the modification of this patch series, I got a confusing problem
> that the EDK2's Alignment seems not fit with Linux on ARM.
>
> EDK2, in CalculateResourceAperture, aligns the offset of child nodes
> in descending order and uses the *Bridge's alignment* to align the
> total length of Bridge.
>
> However, Linux, in pbus_size_mem, would align the size of child nodes
> and use *the half of max alignment of child nodes* to align the total
> length of Bridge.
>
> eg. A Root Bridge with Bus [d2]
> -+-[0000:d2]---01.0-[d3]----01.0
> where [d2:01.00] is a pcie-pci-bridge with BAR0 (mem, 64-bit, non-pref) [size=256]
> [d3:01.00] is a PCI Device with BAR0 (mem, 64-bit, pref) [size=128K]
> BAR4 (mem, 64-bit, pref) [size=64M]
>
> In EDK2, the Resource Map would be:
> PciBus: Resource Map for Root Bridge PciRoot(0xD2)
> Type = Mem64; Base = 0x8004000000; Length = 0x4200000; Alignment = 0x3FFFFFF
> Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF; Owner = PPB [D2|01|00:**]; Type = PMem64
> Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF; Owner = PPB [D2|01|00:10]
>
> PciBus: Resource Map for Bridge [D2|01|00]
> Type = PMem64; Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF
> Base = 0x8004000000; Length = 0x4000000; Alignment = 0x3FFFFFF; Owner = PCI [D3|01|00:20]
> Base = 0x8008000000; Length = 0x20000; Alignment = 0x1FFFF; Owner = PCI [D3|01|00:10]
> Type = Mem64; Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF
>
> It shows that with EDK2's alignment [d2:01.00] only requires
> a PMem64 resource range of 0x4100000.
>
> However in Linux, [d2:01.00]'s BAR14 (Prefetchable Memory
> Resource behind the Bridge) requires a PMem64 resource range
> of 0x06000000, which comes from (0x4000000 + 0x20000) with
> alignment=0x1FFFFFF the half of the max alignment 0x3FFFFFF.
>
> Therefore, the assignment for [d2:01.00]'s BAR14 will fail.
>
> The difference could make the resource range allocated in EDK2 smaller
> than the resource range needed in Linux, which causes the assignment
> failure in Linux.
>
> The same difference also occurs when io resource allocation.
>
> To handle this senario, is it necessary to calculate the resource map
> in Linux way?
I don't know why this difference in BAR placement exists between edk2
and Linux, and/or perhaps even between x86_64 Linux and aarch64 Linux. I
don't really remember seeing this issue with OVMF.
It could have something to do with differences in ACPI table composition
as well.
I don't know if we've ever tested hotplug behind pxb.
>
> Furthermore, Linux Kernel will treat pcie-root-port as a hotplugable
> bridge and try to require more resource. But EDK2 seems not support
> hotplug padding for ARM?
Try including "OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf" in
ArmVirtQemu. See commit fe4049471bdf for documentation.
QEMU's pcie-root-port device should expose the same reservation/padding
properties to the user when using the "virt" machine of
qemu-system-aarch64, I think.
Thanks
Laszlo
>
> Thanks,
> Jiahui
>
> On 2020/11/11 22:33, Laszlo Ersek wrote:
>> On 11/09/20 14:05, Jiahui Cen wrote:
>>> Changes with v1
>>> v1->v2:
>>> Separated into four patches.
>>> Factor the same logic parts into a new library.
>>>
>>> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>
>> (1) Each commit message in the series should reference this bugzilla.
>>
>> But, in itself, that's no reason for a repost; such an update can be
>> made by maintainers when they merge the series.
>>
>> (2) This is a feature addition, so it's merge material for the next
>> development cycle
>> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
>> (Also it depends on the QEMU work being merged first.) I'm going to
>> proceed with the review now.
>>
>> Thanks,
>> Laszlo
>>
>>> QEMU: https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>>>
>>> This patch series adds support for extra pci roots for ARM.
>>>
>>> In order to avoid duplicated codes, we introduce a new library
>>> PciHostBridgeUtilityLib which extracts common interfaces from
>>> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci
>>> roots scanning. Using the utility lib, the uefi could scan for extra
>>> root buses and recognize multiple roots for ARM.
>>>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> 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>
>>>
>>> Yubo Miao (4):
>>> OvmfPkg: Extract functions form PciHostBridgeLib
>>> ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
>>> OvmfPkg: Extract functions of extra pci roots
>>> ArmVirtPkg: Support extra pci roots
>>>
>>> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
>>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>>> OvmfPkg/OvmfXen.dsc | 1 +
>>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 5 +
>>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
>>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 51 ++++
>>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 98 +++++++
>>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 221 ++++++++-------
>>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 234 +---------------
>>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 283 ++++++++++++++++++++
>>> 12 files changed, 563 insertions(+), 335 deletions(-)
>>> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>>> create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>>> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>>>
>>
>>
>>
>>
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
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
1 sibling, 1 reply; 23+ messages in thread
From: Ni, Ray @ 2020-12-11 10:57 UTC (permalink / raw)
To: devel@edk2.groups.io, cenjiahui@huawei.com, lersek@redhat.com
Cc: Justen, Jordan L, ard.biesheuvel@arm.com, leif@nuviainc.com,
xieyingtai@huawei.com, miaoyubo@huawei.com,
xuxiaoyang2@huawei.com
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jiahui Cen via groups.io
> Sent: Friday, December 4, 2020 2:49 PM
> To: devel@edk2.groups.io; lersek@redhat.com
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; ard.biesheuvel@arm.com; leif@nuviainc.com; xieyingtai@huawei.com;
> miaoyubo@huawei.com; xuxiaoyang2@huawei.com
> Subject: Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
>
> Hi Laszlo,
>
> During the modification of this patch series, I got a confusing problem
> that the EDK2's Alignment seems not fit with Linux on ARM.
>
> EDK2, in CalculateResourceAperture, aligns the offset of child nodes
> in descending order and uses the *Bridge's alignment* to align the
> total length of Bridge.
>
> However, Linux, in pbus_size_mem, would align the size of child nodes
> and use *the half of max alignment of child nodes* to align the total
> length of Bridge.
>
> eg. A Root Bridge with Bus [d2]
> -+-[0000:d2]---01.0-[d3]----01.0
> where [d2:01.00] is a pcie-pci-bridge with BAR0 (mem, 64-bit, non-pref) [size=256]
> [d3:01.00] is a PCI Device with BAR0 (mem, 64-bit, pref) [size=128K]
> BAR4 (mem, 64-bit, pref) [size=64M]
>
> In EDK2, the Resource Map would be:
> PciBus: Resource Map for Root Bridge PciRoot(0xD2)
> Type = Mem64; Base = 0x8004000000; Length = 0x4200000; Alignment = 0x3FFFFFF
> Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF; Owner = PPB [D2|01|00:**]; Type = PMem64
> Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF; Owner = PPB [D2|01|00:10]
>
> PciBus: Resource Map for Bridge [D2|01|00]
> Type = PMem64; Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF
> Base = 0x8004000000; Length = 0x4000000; Alignment = 0x3FFFFFF; Owner = PCI [D3|01|00:20]
> Base = 0x8008000000; Length = 0x20000; Alignment = 0x1FFFF; Owner = PCI [D3|01|00:10]
> Type = Mem64; Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF
>
> It shows that with EDK2's alignment [d2:01.00] only requires
> a PMem64 resource range of 0x4100000.
>
> However in Linux, [d2:01.00]'s BAR14 (Prefetchable Memory
> Resource behind the Bridge) requires a PMem64 resource range
> of 0x06000000, which comes from (0x4000000 + 0x20000) with
> alignment=0x1FFFFFF the half of the max alignment 0x3FFFFFF.
How is 0x3FFFFFF calculated?
0x3FFFFFF = 0x8000000 / 2 - 1, and 0x8000000 is the minimum value that's
power of 2 and bigger than 0x6000000. Is my understanding correct?
I don't understand why this calculation method is chosen.
It seems a kernel bug to me.
I thought Linux doesn't handle the pci resource assignment but just use what was
assigned by firmware.
>
> Furthermore, Linux Kernel will treat pcie-root-port as a hotplugable
> bridge and try to require more resource. But EDK2 seems not support
> hotplug padding for ARM?
Hotplug padding is supported through HotPlugInit protocol in edk2.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-12-11 10:57 ` Ni, Ray
@ 2020-12-15 12:52 ` Jiahui Cen
2020-12-17 13:23 ` Laszlo Ersek
0 siblings, 1 reply; 23+ messages in thread
From: Jiahui Cen @ 2020-12-15 12:52 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, lersek@redhat.com
Cc: Justen, Jordan L, ard.biesheuvel@arm.com, leif@nuviainc.com,
xieyingtai@huawei.com, miaoyubo@huawei.com,
xuxiaoyang2@huawei.com
On 2020/12/11 18:57, Ni, Ray wrote:
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jiahui Cen via groups.io
>> Sent: Friday, December 4, 2020 2:49 PM
>> To: devel@edk2.groups.io; lersek@redhat.com
>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; ard.biesheuvel@arm.com; leif@nuviainc.com; xieyingtai@huawei.com;
>> miaoyubo@huawei.com; xuxiaoyang2@huawei.com
>> Subject: Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
>>
>> Hi Laszlo,
>>
>> During the modification of this patch series, I got a confusing problem
>> that the EDK2's Alignment seems not fit with Linux on ARM.
>>
>> EDK2, in CalculateResourceAperture, aligns the offset of child nodes
>> in descending order and uses the *Bridge's alignment* to align the
>> total length of Bridge.
>>
>> However, Linux, in pbus_size_mem, would align the size of child nodes
>> and use *the half of max alignment of child nodes* to align the total
>> length of Bridge.
>>
>> eg. A Root Bridge with Bus [d2]
>> -+-[0000:d2]---01.0-[d3]----01.0
>> where [d2:01.00] is a pcie-pci-bridge with BAR0 (mem, 64-bit, non-pref) [size=256]
>> [d3:01.00] is a PCI Device with BAR0 (mem, 64-bit, pref) [size=128K]
>> BAR4 (mem, 64-bit, pref) [size=64M]
>>
>> In EDK2, the Resource Map would be:
>> PciBus: Resource Map for Root Bridge PciRoot(0xD2)
>> Type = Mem64; Base = 0x8004000000; Length = 0x4200000; Alignment = 0x3FFFFFF
>> Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF; Owner = PPB [D2|01|00:**]; Type = PMem64
>> Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF; Owner = PPB [D2|01|00:10]
>>
>> PciBus: Resource Map for Bridge [D2|01|00]
>> Type = PMem64; Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF
>> Base = 0x8004000000; Length = 0x4000000; Alignment = 0x3FFFFFF; Owner = PCI [D3|01|00:20]
>> Base = 0x8008000000; Length = 0x20000; Alignment = 0x1FFFF; Owner = PCI [D3|01|00:10]
>> Type = Mem64; Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF
>>
>> It shows that with EDK2's alignment [d2:01.00] only requires
>> a PMem64 resource range of 0x4100000.
>>
>> However in Linux, [d2:01.00]'s BAR14 (Prefetchable Memory
>> Resource behind the Bridge) requires a PMem64 resource range
>> of 0x06000000, which comes from (0x4000000 + 0x20000) with
>> alignment=0x1FFFFFF the half of the max alignment 0x3FFFFFF.
>
> How is 0x3FFFFFF calculated?
> 0x3FFFFFF = 0x8000000 / 2 - 1, and 0x8000000 is the minimum value that's
> power of 2 and bigger than 0x6000000. Is my understanding correct?
>
The bridge has 2 resource alignments, 0x4000000 with order 26 and 0x20000
with order 20 (the minimum order is 20). Order 26 is the maximum value
whose alignment is larger than the sum of alignments with order below it.
The final alignment is 0x2000000 = 0x4000000 / 2, and the final size is
0x6000000 = ALIGN(0x4000000 + 0x20000, 0x2000000).
It seems that Kernel tries to use a suitable alignment that ensures the
final size can fit all the resource requirements under the bridge, rather
than place each resource and calculate the exact size. Therefore, the size
may be larger than needed.
> I don't understand why this calculation method is chosen.
> It seems a kernel bug to me.
> I thought Linux doesn't handle the pci resource assignment but just use what was
> assigned by firmware.
>
For x86, Linux does not handle the pci resource assignment. But for arm, it
would assign all the pci resources, unless we explicitly let Linux preserve
PCI resource alignment made by firmware, using "PCI Boot Configuration"
_DSM function.
What do you think of adding "PCI Boot Configuration" _DSM function into
dsdt table to make kernel use firmware's resource configuration?
Thanks,
Jiahui
>>
>> Furthermore, Linux Kernel will treat pcie-root-port as a hotplugable
>> bridge and try to require more resource. But EDK2 seems not support
>> hotplug padding for ARM?
>
> Hotplug padding is supported through HotPlugInit protocol in edk2.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-12-15 12:52 ` Jiahui Cen
@ 2020-12-17 13:23 ` Laszlo Ersek
2020-12-17 13:37 ` Ard Biesheuvel
2020-12-17 13:52 ` Jiahui Cen
0 siblings, 2 replies; 23+ messages in thread
From: Laszlo Ersek @ 2020-12-17 13:23 UTC (permalink / raw)
To: Jiahui Cen, Ni, Ray, devel@edk2.groups.io
Cc: Justen, Jordan L, ard.biesheuvel@arm.com, leif@nuviainc.com,
xieyingtai@huawei.com, miaoyubo@huawei.com,
xuxiaoyang2@huawei.com, Alex Williamson
On 12/15/20 13:52, Jiahui Cen wrote:
> For x86, Linux does not handle the pci resource assignment. But for arm, it
> would assign all the pci resources, unless we explicitly let Linux preserve
> PCI resource alignment made by firmware, using "PCI Boot Configuration"
> _DSM function.
This difference between x86 and arm seems inexplicable to me.
> What do you think of adding "PCI Boot Configuration" _DSM function into
> dsdt table to make kernel use firmware's resource configuration?
The relevant kernel commits seem to be:
- 18e94a338436 ("PCI: Make a shareable UUID for PCI firmware ACPI _DSM",
2015-04-08)
- a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM",
2019-06-21)
I've also read now section "4.6.5. _DSM for Ignoring PCI Boot
Configurations" in the "PCI(TM) Firmware Specification,
Revision 3.1, December 13, 2010".
Basically if this _DSM#5 function exists, it tells the OS whether it is
required to honor the firmware-assigned resources (value 0), or if it is
free to reassign (value 1). If the function does not exist at all, then,
"the operating system may continue to use the legacy handling regarding
the boot configuration".
Without knowing more, I consider it a bug that aarch64 Linux uses a
default strategy (in the absence of the _DSM#5 function) that is the
opposite of the x86 default. But, you seem to be right that there is a
specified way to convince arm64 Linux otherwise. Can you indeed add this
_DSM#5 function to the "virt" machine's ACPI generator, returning value
0, and see if it makes a difference?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
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
1 sibling, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-12-17 13:37 UTC (permalink / raw)
To: Laszlo Ersek, Jiahui Cen, Ni, Ray, devel@edk2.groups.io
Cc: Justen, Jordan L, leif@nuviainc.com, xieyingtai@huawei.com,
miaoyubo@huawei.com, xuxiaoyang2@huawei.com, Alex Williamson
On 12/17/20 2:23 PM, Laszlo Ersek wrote:
> On 12/15/20 13:52, Jiahui Cen wrote:
>> For x86, Linux does not handle the pci resource assignment. But for arm, it
>> would assign all the pci resources, unless we explicitly let Linux preserve
>> PCI resource alignment made by firmware, using "PCI Boot Configuration"
>> _DSM function.
>
> This difference between x86 and arm seems inexplicable to me.
>
>> What do you think of adding "PCI Boot Configuration" _DSM function into
>> dsdt table to make kernel use firmware's resource configuration?
>
> The relevant kernel commits seem to be:
>
> - 18e94a338436 ("PCI: Make a shareable UUID for PCI firmware ACPI _DSM",
> 2015-04-08)
>
> - a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM",
> 2019-06-21)
>
> I've also read now section "4.6.5. _DSM for Ignoring PCI Boot
> Configurations" in the "PCI(TM) Firmware Specification,
> Revision 3.1, December 13, 2010".
>
> Basically if this _DSM#5 function exists, it tells the OS whether it is
> required to honor the firmware-assigned resources (value 0), or if it is
> free to reassign (value 1). If the function does not exist at all, then,
> "the operating system may continue to use the legacy handling regarding
> the boot configuration".
>
> Without knowing more, I consider it a bug that aarch64 Linux uses a
> default strategy (in the absence of the _DSM#5 function) that is the
> opposite of the x86 default. But, you seem to be right that there is a
> specified way to convince arm64 Linux otherwise. Can you indeed add this
> _DSM#5 function to the "virt" machine's ACPI generator, returning value
> 0, and see if it makes a difference?
>
That is not going to work, unfortunately. We had a very long discussion
in the PCI SIG firmware subteam about this, and some changes were made
IIRC, but the code does not exist yet in Linux.
For historical reason, Linux/arm64 always reassigns PCI bus and memory
resources: the ARM port on which arm64 is based does not assume the
existence of any firmware, let only a PCI BIOS that carries out all
those things. When ACPI boot on Linux/arm64 came about, this nuance got
missed, and so even though the presence of rich firmware can obviously
be relied upon in this case, the PCI layer still reassigns some of the
resources in many cases. Note that Linux/x86 might do the same, but is
less likely to do so for modern systems. (My personal favorite is the
Linux/x86 quirk that bases this decision on the BIOS year from DMI)
One thing that does seem to help on AArch64 is to ensure that the bus
padding is the same for hotplug capable root ports, because this is the
most common reason for reassigning bus numbers.
HTH,
Ard.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-12-17 13:37 ` Ard Biesheuvel
@ 2020-12-17 14:42 ` Jonathan Cameron
0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2020-12-17 14:42 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Laszlo Ersek, Jiahui Cen, Ni, Ray, Justen, Jordan L,
leif@nuviainc.com, xieyingtai@huawei.com, miaoyubo@huawei.com,
xuxiaoyang2@huawei.com, Alex Williamson
On Thu, 17 Dec 2020 14:37:30 +0100
Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
> On 12/17/20 2:23 PM, Laszlo Ersek wrote:
> > On 12/15/20 13:52, Jiahui Cen wrote:
> >> For x86, Linux does not handle the pci resource assignment. But for arm, it
> >> would assign all the pci resources, unless we explicitly let Linux preserve
> >> PCI resource alignment made by firmware, using "PCI Boot Configuration"
> >> _DSM function.
> >
> > This difference between x86 and arm seems inexplicable to me.
> >
> >> What do you think of adding "PCI Boot Configuration" _DSM function into
> >> dsdt table to make kernel use firmware's resource configuration?
> >
> > The relevant kernel commits seem to be:
> >
> > - 18e94a338436 ("PCI: Make a shareable UUID for PCI firmware ACPI _DSM",
> > 2015-04-08)
> >
> > - a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM",
> > 2019-06-21)
> >
> > I've also read now section "4.6.5. _DSM for Ignoring PCI Boot
> > Configurations" in the "PCI(TM) Firmware Specification,
> > Revision 3.1, December 13, 2010".
> >
> > Basically if this _DSM#5 function exists, it tells the OS whether it is
> > required to honor the firmware-assigned resources (value 0), or if it is
> > free to reassign (value 1). If the function does not exist at all, then,
> > "the operating system may continue to use the legacy handling regarding
> > the boot configuration".
> >
> > Without knowing more, I consider it a bug that aarch64 Linux uses a
> > default strategy (in the absence of the _DSM#5 function) that is the
> > opposite of the x86 default. But, you seem to be right that there is a
> > specified way to convince arm64 Linux otherwise. Can you indeed add this
> > _DSM#5 function to the "virt" machine's ACPI generator, returning value
> > 0, and see if it makes a difference?
> >
>
> That is not going to work, unfortunately. We had a very long discussion
> in the PCI SIG firmware subteam about this, and some changes were made
> IIRC, but the code does not exist yet in Linux.
>
> For historical reason, Linux/arm64 always reassigns PCI bus and memory
> resources: the ARM port on which arm64 is based does not assume the
> existence of any firmware, let only a PCI BIOS that carries out all
> those things. When ACPI boot on Linux/arm64 came about, this nuance got
> missed, and so even though the presence of rich firmware can obviously
> be relied upon in this case, the PCI layer still reassigns some of the
> resources in many cases. Note that Linux/x86 might do the same, but is
> less likely to do so for modern systems. (My personal favorite is the
> Linux/x86 quirk that bases this decision on the BIOS year from DMI)
>
> One thing that does seem to help on AArch64 is to ensure that the bus
> padding is the same for hotplug capable root ports, because this is the
> most common reason for reassigning bus numbers.
Hi Ard,
For an unrelated reason I've been trying to create a setup where bus numbers
get reassigned on aarch64 and been unable to do so.
I'm looking at potential issues around Generic initiators being defined via BDF
and whether we can cache the firmware configured BDF before re enumerating.
All sorts of fun happens if the initial setup is not correct (many of which
result in unusable systems) but for a 'valid' setup I have not managed to
create a case where BDFs move. I can't find a route to reassignment of
bus numbers after the _DSM is evaluated (and host->preserve_config is
first used).
https://elixir.bootlin.com/linux/latest/source/arch/arm64/kernel/pci.c#L194
The padding for bus numbers on root ports seems to be limited to the available
space. Note I'm doing messing around in qemu but have edk2 using PciHotplugInit
from OVMF to allow straight forward root port padding for hp in edk2.
Any pointers would be great!
Jonathan
>
> HTH,
> Ard.
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-12-17 13:23 ` Laszlo Ersek
2020-12-17 13:37 ` Ard Biesheuvel
@ 2020-12-17 13:52 ` Jiahui Cen
1 sibling, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-12-17 13:52 UTC (permalink / raw)
To: devel, lersek, Ni, Ray
Cc: Justen, Jordan L, ard.biesheuvel@arm.com, leif@nuviainc.com,
xieyingtai@huawei.com, miaoyubo@huawei.com,
xuxiaoyang2@huawei.com, Alex Williamson
On 2020/12/17 21:23, Laszlo Ersek wrote:
> On 12/15/20 13:52, Jiahui Cen wrote:
>> For x86, Linux does not handle the pci resource assignment. But for arm, it
>> would assign all the pci resources, unless we explicitly let Linux preserve
>> PCI resource alignment made by firmware, using "PCI Boot Configuration"
>> _DSM function.
> This difference between x86 and arm seems inexplicable to me.
>
>> What do you think of adding "PCI Boot Configuration" _DSM function into
>> dsdt table to make kernel use firmware's resource configuration?
> The relevant kernel commits seem to be:
>
> - 18e94a338436 ("PCI: Make a shareable UUID for PCI firmware ACPI _DSM",
> 2015-04-08)
>
> - a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM",
> 2019-06-21)
>
> I've also read now section "4.6.5. _DSM for Ignoring PCI Boot
> Configurations" in the "PCI(TM) Firmware Specification,
> Revision 3.1, December 13, 2010".
>
> Basically if this _DSM#5 function exists, it tells the OS whether it is
> required to honor the firmware-assigned resources (value 0), or if it is
> free to reassign (value 1). If the function does not exist at all, then,
> "the operating system may continue to use the legacy handling regarding
> the boot configuration".
>
> Without knowing more, I consider it a bug that aarch64 Linux uses a
> default strategy (in the absence of the _DSM#5 function) that is the
> opposite of the x86 default. But, you seem to be right that there is a
> specified way to convince arm64 Linux otherwise. Can you indeed add this
> _DSM#5 function to the "virt" machine's ACPI generator, returning value
> 0, and see if it makes a difference?
OK, I've tested adding _DSM#5 function in QEMU, and it seems to work well for
ARM "virt" machine.
Thanks,
Jiahui
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 0/4] Add extra pci roots support for Arm
2020-11-09 13:05 [PATCH v2 0/4] Add extra pci roots support for Arm Jiahui Cen
` (4 preceding siblings ...)
2020-11-11 14:33 ` [PATCH v2 0/4] Add extra pci roots support for Arm Laszlo Ersek
@ 2020-11-12 8:49 ` Ard Biesheuvel
2020-11-13 19:44 ` Laszlo Ersek
5 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-11-12 8:49 UTC (permalink / raw)
To: Jiahui Cen, devel; +Cc: jordan.l.justen, lersek, leif, xieyingtai, miaoyubo
On 11/9/20 2:05 PM, Jiahui Cen wrote:
> Changes with v1
> v1->v2:
> Separated into four patches.
> Factor the same logic parts into a new library.
>
> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
> QEMU: https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>
> This patch series adds support for extra pci roots for ARM.
>
Why?
> In order to avoid duplicated codes, we introduce a new library
> PciHostBridgeUtilityLib which extracts common interfaces from
> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci
> roots scanning. Using the utility lib, the uefi could scan for extra
> root buses and recognize multiple roots for ARM.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 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>
>
> Yubo Miao (4):
> OvmfPkg: Extract functions form PciHostBridgeLib
> ArmVirtPkg: Use extracted PciHostBridgeUtilityLib
> OvmfPkg: Extract functions of extra pci roots
> ArmVirtPkg: Support extra pci roots
>
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/OvmfXen.dsc | 1 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 5 +
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 51 ++++
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 98 +++++++
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 221 ++++++++-------
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 234 +---------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 283 ++++++++++++++++++++
> 12 files changed, 563 insertions(+), 335 deletions(-)
> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 0/4] Add extra pci roots support for Arm
2020-11-12 8:49 ` Ard Biesheuvel
@ 2020-11-13 19:44 ` Laszlo Ersek
2020-11-16 1:33 ` [edk2-devel] " Jiahui Cen
0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2020-11-13 19:44 UTC (permalink / raw)
To: Ard Biesheuvel, Jiahui Cen, devel
Cc: jordan.l.justen, leif, xieyingtai, miaoyubo
On 11/12/20 09:49, Ard Biesheuvel wrote:
> On 11/9/20 2:05 PM, Jiahui Cen wrote:
>> Changes with v1
>> v1->v2:
>> Separated into four patches.
>> Factor the same logic parts into a new library.
>>
>> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>> QEMU:
>> https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>>
>>
>> This patch series adds support for extra pci roots for ARM.
>>
>
> Why?
I don't know Jiahui Cen's particular reasons; on x86, pxb-pcie is useful
for associating PCIe hierarchies with particular NUMA nodes. See
"docs/pci_expander_bridge.txt" in the QEMU tree.
Also, in Jiahui Cen's QEMU blurb linked above, the following is included:
> Currently pxb-pcie is not supported by arm,
> the reason for it is pxb-pcie is not described in DSDT table
> and only one main host bridge is described in acpi tables,
> which means it is not impossible to present different io numas
> for different devices.
So I guess this work aims to extend the same PCI(e)<->NUMA association
feature, from x86 to ARM.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm
2020-11-13 19:44 ` Laszlo Ersek
@ 2020-11-16 1:33 ` Jiahui Cen
0 siblings, 0 replies; 23+ messages in thread
From: Jiahui Cen @ 2020-11-16 1:33 UTC (permalink / raw)
To: devel, lersek, Ard Biesheuvel; +Cc: jordan.l.justen, leif, xieyingtai, miaoyubo
On 2020/11/14 3:44, Laszlo Ersek wrote:
> On 11/12/20 09:49, Ard Biesheuvel wrote:
>> On 11/9/20 2:05 PM, Jiahui Cen wrote:
>>> Changes with v1
>>> v1->v2:
>>> Separated into four patches.
>>> Factor the same logic parts into a new library.
>>>
>>> v1: https://edk2.groups.io/g/devel/topic/72723351#56901
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>> QEMU:
>>> https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/
>>>
>>>
>>> This patch series adds support for extra pci roots for ARM.
>>>
>>
>> Why?
>
> I don't know Jiahui Cen's particular reasons; on x86, pxb-pcie is useful
> for associating PCIe hierarchies with particular NUMA nodes. See
> "docs/pci_expander_bridge.txt" in the QEMU tree.
>
> Also, in Jiahui Cen's QEMU blurb linked above, the following is included:
>
>> Currently pxb-pcie is not supported by arm,
>> the reason for it is pxb-pcie is not described in DSDT table
>> and only one main host bridge is described in acpi tables,
>> which means it is not impossible to present different io numas
>> for different devices.
>
> So I guess this work aims to extend the same PCI(e)<->NUMA association
> feature, from x86 to ARM.
> > Thanks,
> Laszlo
>
>
> .
>
Right, we'd like to extend the pxb-pcie feature on ARM so that PCI(e)
can be associated with particular NUMA nodes.
Thanks,
Jiahui
^ permalink raw reply [flat|nested] 23+ messages in thread