* [PATCH v3 0/5] Add extra pci roots support for Arm
@ 2020-12-22 9:59 Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Jiahui Cen @ 2020-12-22 9:59 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Rebecca Cran,
Peter Grehan, Anthony Perard, Julien Grall, Leif Lindholm,
Sami Mujawar, xieyingtai, wu.wubin, Jiahui Cen, Yubo Miao
v2->v3:
* Rename utility functions under the PciHostBridgeUtilityLib namespace.
* Remove some unused Library dependencies.
* Sort the Include headers.
v1->v2:
* Separated into four patches.
* Factor the same logic parts into a new library.
v2: https://edk2.groups.io/g/devel/topic/78135572#67173
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
QEMU: https://lore.kernel.org/qemu-devel/20201119014841.7298-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: Rebecca Cran <rebecca@bsdio.com>
Cc: Peter Grehan <grehan@freebsd.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
Jiahui Cen (5):
OvmfPkg: Introduce PciHostBridgeUtilityLib class
ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
OvmfPkg: Extract functions for extra pci roots
ArmVirtPkg: Add support for extra pci roots
ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
OvmfPkg/OvmfPkg.dec | 4 +
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
ArmVirtPkg/ArmVirtKvmTool.dsc | 1 +
ArmVirtPkg/ArmVirtQemu.dsc | 2 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
OvmfPkg/Bhyve/BhyveX64.dsc | 1 +
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 +
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 3 +-
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 41 +++
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 95 +++++++
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 182 ++++++------
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 199 +-------------
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 289 ++++++++++++++++++++
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
18 files changed, 560 insertions(+), 266 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
--
2.28.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
@ 2020-12-22 9:59 ` Jiahui Cen
2021-01-06 8:51 ` [edk2-devel] " Laszlo Ersek
2020-12-22 9:59 ` [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Jiahui Cen @ 2020-12-22 9:59 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Rebecca Cran,
Peter Grehan, Anthony Perard, Julien Grall, Leif Lindholm,
Sami Mujawar, xieyingtai, wu.wubin, Jiahui Cen, Yubo Miao
Introduce a new PciHostBridgeUtilityLib class to share duplicate code
between OvmfPkg and ArmVirtPkg.
Extract function PciHostBridgeUtilityResourceConflict from
PciHostBridgeResourceConflict in OvmfPkg/PciHostBridgeLib.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Peter Grehan <grehan@freebsd.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
---
OvmfPkg/OvmfPkg.dec | 4 +
OvmfPkg/Bhyve/BhyveX64.dsc | 1 +
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 | 37 ++++++++++
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 37 ++++++++++
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 41 +----------
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 77 ++++++++++++++++++++
11 files changed, 163 insertions(+), 39 deletions(-)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 50d7b27d941c..e39097a253a1 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.
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index b93fe30ae4e0..f1fdd85d1911 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -660,6 +660,7 @@ [Components]
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 26a013ec353e..6eef5e0cfa9c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -745,6 +745,7 @@ [Components]
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 10579fe46c5b..4b2f48406543 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -759,6 +759,7 @@ [Components.X64]
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c9235e48ad62..8577ccaa35af 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -755,6 +755,7 @@ [Components]
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 12b7a87ee877..fa35d122cf3e 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -550,6 +550,7 @@ [Components]
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
<LibraryClasses>
PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
}
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index 6ec9ec751478..4c56f3c90b3b 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -39,6 +39,7 @@ [LibraryClasses]
DebugLib
DevicePathLib
MemoryAllocationLib
+ PciHostBridgeUtilityLib
PciLib
QemuFwCfgLib
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
new file mode 100644
index 000000000000..1ba8ec3e03c7
--- /dev/null
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -0,0 +1,37 @@
+## @file
+# 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>
+# Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+ 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 AARCH64 ARM
+#
+
+[Sources]
+ PciHostBridgeUtilityLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ DebugLib
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
new file mode 100644
index 000000000000..f932d412aa10
--- /dev/null
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -0,0 +1,37 @@
+/** @file
+ Provide common utility functions to PciHostBridgeLib instances in
+ ArmVirtPkg and OvmfPkg.
+
+ Copyright (C) 2016, Red Hat, Inc.
+ Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2020, Huawei 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__
+
+
+/**
+ Utility function to inform the platform that the resource conflict happens.
+
+ @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
+PciHostBridgeUtilityResourceConflict (
+ VOID *Configuration
+ );
+
+
+#endif // __PCI_HOST_BRIDGE_UTILITY_LIB_H__
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index e850f7d183ee..4a176347fd49 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -20,6 +20,7 @@
#include <Library/DevicePathLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeLib.h>
+#include <Library/PciHostBridgeUtilityLib.h>
#include <Library/PciLib.h>
#include <Library/QemuFwCfgLib.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 = {
@@ -407,37 +402,5 @@ PciHostBridgeResourceConflict (
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
- );
- }
+ PciHostBridgeUtilityResourceConflict (Configuration);
}
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
new file mode 100644
index 000000000000..bbaa5f830c98
--- /dev/null
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -0,0 +1,77 @@
+/** @file
+ Provide common utility functions to PciHostBridgeLib instances in
+ ArmVirtPkg and OvmfPkg.
+
+ Copyright (C) 2016, Red Hat, Inc.
+ Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <IndustryStandard/Acpi10.h>
+#include <Library/DebugLib.h>
+#include <Library/PciHostBridgeUtilityLib.h>
+
+
+GLOBAL_REMOVE_IF_UNREFERENCED
+CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
+ L"Mem", L"I/O", L"Bus"
+};
+
+
+/**
+ Utility function to inform the platform that the resource conflict happens.
+
+ @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
+PciHostBridgeUtilityResourceConflict (
+ 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 (mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr)
+ );
+ DEBUG ((DEBUG_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
+ mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[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] 15+ messages in thread
* [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
@ 2020-12-22 9:59 ` Jiahui Cen
2021-01-06 9:12 ` [edk2-devel] " Laszlo Ersek
2020-12-22 9:59 ` [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots Jiahui Cen
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Jiahui Cen @ 2020-12-22 9:59 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Rebecca Cran,
Peter Grehan, Anthony Perard, Julien Grall, Leif Lindholm,
Sami Mujawar, xieyingtai, wu.wubin, Jiahui Cen, Yubo Miao
Eliminate currently duplicated code in ArmVirtPkg with the common utility
class PciHostBridgeUtilityLib.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
---
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
ArmVirtPkg/ArmVirtKvmTool.dsc | 1 +
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 44 ++------------------
6 files changed, 9 insertions(+), 41 deletions(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 9ec92930472d..b9a0cd362416 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -145,6 +145,7 @@ [LibraryClasses.common]
PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
# USB Libraries
UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index bf008be50fcb..f817132ba7b0 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -65,6 +65,7 @@ [LibraryClasses.common]
PlatformPeiLib|ArmVirtPkg/Library/KvmtoolPlatformPeiLib/KvmtoolPlatformPeiLib.inf
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index ef5d6dbeaddc..a11ffd9ba553 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -78,6 +78,7 @@ [LibraryClasses.common]
PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
!if $(TPM2_ENABLE) == TRUE
Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index f8f5f7f4b94b..c27752b4d5e5 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -76,6 +76,7 @@ [LibraryClasses.common]
PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
[LibraryClasses.common.DXE_DRIVER]
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
index 277ccfd24546..01d39626d14c 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
@@ -31,12 +31,14 @@ [Packages]
ArmVirtPkg/ArmVirtPkg.dec
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
[LibraryClasses]
DebugLib
DevicePathLib
DxeServicesTableLib
MemoryAllocationLib
+ PciHostBridgeUtilityLib
PciPcdProducerLib
[FixedPcd]
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 496b192d2291..d554479bf0de 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -7,12 +7,13 @@
**/
#include <PiDxe.h>
-#include <Library/PciHostBridgeLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/DxeServicesTableLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
+#include <Library/PciHostBridgeLib.h>
+#include <Library/PciHostBridgeUtilityLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Protocol/FdtClient.h>
@@ -50,11 +51,6 @@ STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
}
};
-GLOBAL_REMOVE_IF_UNREFERENCED
-CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
- L"Mem", L"I/O", L"Bus"
-};
-
//
// We expect the "ranges" property of "pci-host-ecam-generic" to consist of
// records like this.
@@ -435,39 +431,5 @@ PciHostBridgeResourceConflict (
VOID *Configuration
)
{
- EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
- UINTN RootBridgeIndex;
- DEBUG ((EFI_D_ERROR, "PciHostBridge: Resource conflict happens!\n"));
-
- RootBridgeIndex = 0;
- Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
- while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
- DEBUG ((EFI_D_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
- for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
- ASSERT (Descriptor->ResType <
- (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) /
- sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0])
- )
- );
- DEBUG ((EFI_D_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
- mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
- Descriptor->AddrLen, Descriptor->AddrRangeMax
- ));
- if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
- DEBUG ((EFI_D_ERROR, " Granularity/SpecificFlag = %ld / %02x%s\n",
- Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
- ((Descriptor->SpecificFlag &
- EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE
- ) != 0) ? L" (Prefetchable)" : L""
- ));
- }
- }
- //
- // Skip the END descriptor for root bridge
- //
- ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
- Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
- (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1
- );
- }
+ PciHostBridgeUtilityResourceConflict (Configuration);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
@ 2020-12-22 9:59 ` Jiahui Cen
2021-01-06 10:27 ` [edk2-devel] " Laszlo Ersek
2020-12-22 9:59 ` [PATCH v3 4/5] ArmVirtPkg: Add support " Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
4 siblings, 1 reply; 15+ messages in thread
From: Jiahui Cen @ 2020-12-22 9:59 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Rebecca Cran,
Peter Grehan, Anthony Perard, Julien Grall, Leif Lindholm,
Sami Mujawar, xieyingtai, wu.wubin, Jiahui Cen, Yubo Miao
Extract functions that support extra pci roots from
OvmfPkg/PciHostBridgeLib to share this feature with other packages.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
---
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 4 +
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 58 ++++++
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +--------------
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 212 ++++++++++++++++++++
5 files changed, 285 insertions(+), 149 deletions(-)
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index 4c56f3c90b3b..8bdb76899111 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -40,8 +40,6 @@ [LibraryClasses]
DevicePathLib
MemoryAllocationLib
PciHostBridgeUtilityLib
- PciLib
- QemuFwCfgLib
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
index 1ba8ec3e03c7..a10afbe30c6b 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -30,8 +30,12 @@ [Sources]
PciHostBridgeUtilityLib.c
[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
DebugLib
+ MemoryAllocationLib
+ PciLib
+ QemuFwCfgLib
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
index f932d412aa10..1d1c86c69064 100644
--- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -14,6 +14,64 @@
#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
+#include <Library/PciHostBridgeLib.h>
+
+
+/**
+ Utility function to free root bridge instances array.
+
+ @param The root bridge instances array.
+ @param The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeUtilityFreeRootBridges (
+ PCI_ROOT_BRIDGE *Bridges,
+ UINTN Count
+ );
+
+
+/**
+ Utility function to return all the root bridge instances in an array.
+
+ @param Count The number of root bridge instances.
+
+ @param[in] BusMin The min bus number.
+
+ @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.
+
+ @param[in] PMem Prefetchable MMIO aperture.
+
+ @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+
+ @return All the root bridge instances in an array.
+**/
+PCI_ROOT_BRIDGE *
+EFIAPI
+PciHostBridgeUtilityExtraRoots (
+ UINTN *Count,
+ UINT32 BusMin,
+ UINT32 BusMax,
+ UINT64 Attributes,
+ UINT64 AllocationAttributes,
+ PCI_ROOT_BRIDGE_APERTURE Io,
+ PCI_ROOT_BRIDGE_APERTURE Mem,
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G,
+ PCI_ROOT_BRIDGE_APERTURE PMem,
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G
+ );
+
+
/**
Utility function to inform the platform that the resource conflict happens.
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index 4a176347fd49..19c6e9fa421a 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -21,8 +21,6 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
-#include <Library/PciLib.h>
-#include <Library/QemuFwCfgLib.h>
#include "PciHostBridge.h"
@@ -160,23 +158,6 @@ InitRootBridge (
}
-/**
- 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 +173,6 @@ 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,117 +212,18 @@ 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,
+ return PciHostBridgeUtilityExtraRoots (
+ Count,
+ 0,
PCI_MAX_BUS,
- &Io,
- &Mem,
- &MemAbove4G,
- &mNonExistAperture,
- &mNonExistAperture,
- &Bridges[Initialized]
+ Attributes,
+ AllocationAttributes,
+ Io,
+ Mem,
+ MemAbove4G,
+ mNonExistAperture,
+ mNonExistAperture
);
- if (EFI_ERROR (Status)) {
- goto FreeBridges;
- }
- ++Initialized;
-
- *Count = Initialized;
- return Bridges;
-
-FreeBridges:
- while (Initialized > 0) {
- --Initialized;
- UninitRootBridge (&Bridges[Initialized]);
- }
-
- FreePool (Bridges);
- return NULL;
}
@@ -367,17 +241,7 @@ PciHostBridgeFreeRootBridges (
UINTN Count
)
{
- if (Bridges == NULL && Count == 0) {
- return;
- }
- ASSERT (Bridges != NULL && Count > 0);
-
- do {
- --Count;
- UninitRootBridge (&Bridges[Count]);
- } while (Count > 0);
-
- FreePool (Bridges);
+ PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
}
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
index bbaa5f830c98..a0cfd24ce477 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -11,8 +11,13 @@
**/
#include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/Pci.h>
#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
+#include <Library/PciLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include "Library/PciHostBridgeLib/PciHostBridge.h"
GLOBAL_REMOVE_IF_UNREFERENCED
@@ -21,6 +26,213 @@ CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
};
+/**
+ 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);
+}
+
+
+/**
+ Utility function to free root bridge instances array.
+
+ @param The root bridge instances array.
+ @param The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeUtilityFreeRootBridges (
+ 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);
+}
+
+
+/**
+ Utility function to return all the root bridge instances in an array.
+
+ @param Count The number of root bridge instances.
+
+ @param[in] BusMin The min bus number.
+
+ @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.
+
+ @param[in] PMem Prefetchable MMIO aperture.
+
+ @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+
+ @return All the root bridge instances in an array.
+**/
+PCI_ROOT_BRIDGE *
+EFIAPI
+PciHostBridgeUtilityExtraRoots (
+ UINTN *Count,
+ UINT32 BusMin,
+ UINT32 BusMax,
+ UINT64 Attributes,
+ UINT64 AllocationAttributes,
+ PCI_ROOT_BRIDGE_APERTURE Io,
+ PCI_ROOT_BRIDGE_APERTURE Mem,
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G,
+ PCI_ROOT_BRIDGE_APERTURE PMem,
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G
+ )
+{
+ 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 - BusMin) {
+ 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 = BusMin;
+
+ //
+ // 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 = BusMin + 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;
+}
+
+
/**
Utility function to inform the platform that the resource conflict happens.
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 4/5] ArmVirtPkg: Add support for extra pci roots
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
` (2 preceding siblings ...)
2020-12-22 9:59 ` [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots Jiahui Cen
@ 2020-12-22 9:59 ` Jiahui Cen
2021-01-06 10:28 ` [edk2-devel] " Laszlo Ersek
2020-12-22 9:59 ` [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
4 siblings, 1 reply; 15+ messages in thread
From: Jiahui Cen @ 2020-12-22 9:59 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Rebecca Cran,
Peter Grehan, Anthony Perard, Julien Grall, Leif Lindholm,
Sami Mujawar, xieyingtai, wu.wubin, Jiahui Cen, Yubo Miao
Use utility functions in PciHostBridgeUtilityLib and some platform specific
functions to add support for extra pci roots in ArmVirtPkg.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
---
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 138 ++++++++++++++------
1 file changed, 101 insertions(+), 37 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index d554479bf0de..a29dcecf7044 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -7,6 +7,7 @@
**/
#include <PiDxe.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/DxeServicesTableLib.h>
@@ -302,7 +303,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->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 ((DEBUG_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 ((DEBUG_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.
@@ -319,11 +373,18 @@ 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;
+ EFI_STATUS Status;
+ UINT64 Attributes;
+ UINT64 AllocationAttributes;
+ PCI_ROOT_BRIDGE_APERTURE Io;
+ PCI_ROOT_BRIDGE_APERTURE Mem;
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
+ PCI_ROOT_BRIDGE_APERTURE PMem;
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
if (PcdGet64 (PcdPciExpressBaseAddress) == 0) {
DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__));
@@ -341,33 +402,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 {
//
@@ -376,21 +431,30 @@ 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;
+ return PciHostBridgeUtilityExtraRoots (
+ Count,
+ BusMin,
+ BusMax,
+ Attributes,
+ AllocationAttributes,
+ Io,
+ Mem,
+ MemAbove4G,
+ PMem,
+ PMemAbove4G
+ );
}
/**
@@ -407,7 +471,7 @@ PciHostBridgeFreeRootBridges (
UINTN Count
)
{
- ASSERT (Count == 1);
+ PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
}
/**
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
` (3 preceding siblings ...)
2020-12-22 9:59 ` [PATCH v3 4/5] ArmVirtPkg: Add support " Jiahui Cen
@ 2020-12-22 9:59 ` Jiahui Cen
2021-01-06 10:31 ` [edk2-devel] " Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Jiahui Cen @ 2020-12-22 9:59 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Rebecca Cran,
Peter Grehan, Anthony Perard, Julien Grall, Leif Lindholm,
Sami Mujawar, xieyingtai, wu.wubin, Jiahui Cen, Yubo Miao
It is necessary to add padding for hotplugable PCI Devices like
pcie-root-port.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
---
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
2 files changed, 2 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index a11ffd9ba553..d77c226d80ee 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -488,6 +488,7 @@ [Components.common]
<LibraryClasses>
NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
}
+ OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
OvmfPkg/Virtio10Dxe/Virtio10.inf
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 1752fee12b79..5b1d10057545 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -162,6 +162,7 @@ [FV.FvMain]
INF ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
INF MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+ INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
INF OvmfPkg/Virtio10Dxe/Virtio10.inf
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class
2020-12-22 9:59 ` [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
@ 2021-01-06 8:51 ` Laszlo Ersek
2021-01-07 5:44 ` Jiahui Cen
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2021-01-06 8:51 UTC (permalink / raw)
To: devel, cenjiahui
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> Introduce a new PciHostBridgeUtilityLib class to share duplicate code
> between OvmfPkg and ArmVirtPkg.
>
> Extract function PciHostBridgeUtilityResourceConflict from
> PciHostBridgeResourceConflict in OvmfPkg/PciHostBridgeLib.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Peter Grehan <grehan@freebsd.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> ---
> OvmfPkg/OvmfPkg.dec | 4 +
> OvmfPkg/Bhyve/BhyveX64.dsc | 1 +
> 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 | 37 ++++++++++
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 37 ++++++++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 41 +----------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 77 ++++++++++++++++++++
> 11 files changed, 163 insertions(+), 39 deletions(-)
(1) Since you posted v2, OvmfPkg gained a new platform DSC file. The
full set of DSC files in OvmfPkg is, at this point:
OvmfPkg/AmdSev/AmdSevX64.dsc
OvmfPkg/Bhyve/BhyveX64.dsc
OvmfPkg/OvmfPkgIa32.dsc
OvmfPkg/OvmfPkgIa32X64.dsc
OvmfPkg/OvmfPkgX64.dsc
OvmfPkg/OvmfXen.dsc
"OvmfPkg/AmdSev/AmdSevX64.dsc" consumes
"OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf" as well; please
add the new PciHostBridgeUtilityLib resolution to that DSC file too, in v4.
(You should check for any and all DSC files with such a dependency,
every time you rebase.)
With that update, I'm ready to grant an R-b for this patch.
Thanks!
Laszlo
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 50d7b27d941c..e39097a253a1 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.
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index b93fe30ae4e0..f1fdd85d1911 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -660,6 +660,7 @@ [Components]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 26a013ec353e..6eef5e0cfa9c 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -745,6 +745,7 @@ [Components]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 10579fe46c5b..4b2f48406543 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -759,6 +759,7 @@ [Components.X64]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index c9235e48ad62..8577ccaa35af 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -755,6 +755,7 @@ [Components]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 12b7a87ee877..fa35d122cf3e 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -550,6 +550,7 @@ [Components]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index 6ec9ec751478..4c56f3c90b3b 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -39,6 +39,7 @@ [LibraryClasses]
> DebugLib
> DevicePathLib
> MemoryAllocationLib
> + PciHostBridgeUtilityLib
> PciLib
> QemuFwCfgLib
>
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> new file mode 100644
> index 000000000000..1ba8ec3e03c7
> --- /dev/null
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -0,0 +1,37 @@
> +## @file
> +# 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>
> +# Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> + 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 AARCH64 ARM
> +#
> +
> +[Sources]
> + PciHostBridgeUtilityLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + DebugLib
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> new file mode 100644
> index 000000000000..f932d412aa10
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -0,0 +1,37 @@
> +/** @file
> + Provide common utility functions to PciHostBridgeLib instances in
> + ArmVirtPkg and OvmfPkg.
> +
> + Copyright (C) 2016, Red Hat, Inc.
> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2020, Huawei 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__
> +
> +
> +/**
> + Utility function to inform the platform that the resource conflict happens.
> +
> + @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
> +PciHostBridgeUtilityResourceConflict (
> + VOID *Configuration
> + );
> +
> +
> +#endif // __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index e850f7d183ee..4a176347fd49 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -20,6 +20,7 @@
> #include <Library/DevicePathLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
> #include <Library/PciLib.h>
> #include <Library/QemuFwCfgLib.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 = {
> @@ -407,37 +402,5 @@ PciHostBridgeResourceConflict (
> 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
> - );
> - }
> + PciHostBridgeUtilityResourceConflict (Configuration);
> }
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> new file mode 100644
> index 000000000000..bbaa5f830c98
> --- /dev/null
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -0,0 +1,77 @@
> +/** @file
> + Provide common utility functions to PciHostBridgeLib instances in
> + ArmVirtPkg and OvmfPkg.
> +
> + Copyright (C) 2016, Red Hat, Inc.
> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <IndustryStandard/Acpi10.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
> +
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED
> +CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
> + L"Mem", L"I/O", L"Bus"
> +};
> +
> +
> +/**
> + Utility function to inform the platform that the resource conflict happens.
> +
> + @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
> +PciHostBridgeUtilityResourceConflict (
> + 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 (mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr)
> + );
> + DEBUG ((DEBUG_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
> + mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[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
> + );
> + }
> +}
> +
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
2020-12-22 9:59 ` [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
@ 2021-01-06 9:12 ` Laszlo Ersek
2021-01-07 5:44 ` Jiahui Cen
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2021-01-06 9:12 UTC (permalink / raw)
To: devel, cenjiahui
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> Eliminate currently duplicated code in ArmVirtPkg with the common utility
> class PciHostBridgeUtilityLib.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> ArmVirtPkg/ArmVirtKvmTool.dsc | 1 +
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 44 ++------------------
> 6 files changed, 9 insertions(+), 41 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 9ec92930472d..b9a0cd362416 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -145,6 +145,7 @@ [LibraryClasses.common]
> PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>
> # USB Libraries
> UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index bf008be50fcb..f817132ba7b0 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -65,6 +65,7 @@ [LibraryClasses.common]
> PlatformPeiLib|ArmVirtPkg/Library/KvmtoolPlatformPeiLib/KvmtoolPlatformPeiLib.inf
>
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index ef5d6dbeaddc..a11ffd9ba553 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -78,6 +78,7 @@ [LibraryClasses.common]
> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>
> !if $(TPM2_ENABLE) == TRUE
> Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index f8f5f7f4b94b..c27752b4d5e5 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -76,6 +76,7 @@ [LibraryClasses.common]
> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>
> [LibraryClasses.common.DXE_DRIVER]
Please pay more attention to review feedback. In v2, I requested two
things, and those have not been resolved precisely:
(1) The files
- ArmVirtPkg/ArmVirtKvmTool.dsc
- ArmVirtPkg/ArmVirtQemu.dsc
- ArmVirtPkg/ArmVirtQemuKernel.dsc
should be updated *instead of* "ArmVirtPkg/ArmVirt.dsc.inc" -- not "in
addition to" the latter.
So please drop the "ArmVirtPkg/ArmVirt.dsc.inc" hunk.
(2) In "ArmVirtPkg/ArmVirtKvmTool.dsc", you didn't place the
PciHostBridgeUtilityLib class resolution right after the
PciHostBridgeLib one.
So please move the new lib class resolution into said (more intuitive) spot.
With the above updates, the patch is going to be OK.
Thanks,
Laszlo
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> index 277ccfd24546..01d39626d14c 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> @@ -31,12 +31,14 @@ [Packages]
> ArmVirtPkg/ArmVirtPkg.dec
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> DebugLib
> DevicePathLib
> DxeServicesTableLib
> MemoryAllocationLib
> + PciHostBridgeUtilityLib
> PciPcdProducerLib
>
> [FixedPcd]
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 496b192d2291..d554479bf0de 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -7,12 +7,13 @@
>
> **/
> #include <PiDxe.h>
> -#include <Library/PciHostBridgeLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/DxeServicesTableLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
> +#include <Library/PciHostBridgeLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
> #include <Protocol/FdtClient.h>
> @@ -50,11 +51,6 @@ STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
> }
> };
>
> -GLOBAL_REMOVE_IF_UNREFERENCED
> -CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> - L"Mem", L"I/O", L"Bus"
> -};
> -
> //
> // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
> // records like this.
> @@ -435,39 +431,5 @@ PciHostBridgeResourceConflict (
> VOID *Configuration
> )
> {
> - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> - UINTN RootBridgeIndex;
> - DEBUG ((EFI_D_ERROR, "PciHostBridge: Resource conflict happens!\n"));
> -
> - RootBridgeIndex = 0;
> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
> - while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> - DEBUG ((EFI_D_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
> - for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
> - ASSERT (Descriptor->ResType <
> - (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) /
> - sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0])
> - )
> - );
> - DEBUG ((EFI_D_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
> - mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
> - Descriptor->AddrLen, Descriptor->AddrRangeMax
> - ));
> - if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> - DEBUG ((EFI_D_ERROR, " Granularity/SpecificFlag = %ld / %02x%s\n",
> - Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
> - ((Descriptor->SpecificFlag &
> - EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE
> - ) != 0) ? L" (Prefetchable)" : L""
> - ));
> - }
> - }
> - //
> - // Skip the END descriptor for root bridge
> - //
> - ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
> - (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1
> - );
> - }
> + PciHostBridgeUtilityResourceConflict (Configuration);
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots
2020-12-22 9:59 ` [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots Jiahui Cen
@ 2021-01-06 10:27 ` Laszlo Ersek
2021-01-07 5:47 ` Jiahui Cen
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2021-01-06 10:27 UTC (permalink / raw)
To: devel, cenjiahui
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> Extract functions that support extra pci roots from
> OvmfPkg/PciHostBridgeLib to share this feature with other packages.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> ---
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 4 +
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 58 ++++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +--------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 212 ++++++++++++++++++++
> 5 files changed, 285 insertions(+), 149 deletions(-)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index 4c56f3c90b3b..8bdb76899111 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -40,8 +40,6 @@ [LibraryClasses]
> DevicePathLib
> MemoryAllocationLib
> PciHostBridgeUtilityLib
> - PciLib
(1) Removing the PciLib class dependency is incorrect. The
"XenSupport.c" file continues using PciRead32(), for example.
> - QemuFwCfgLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> index 1ba8ec3e03c7..a10afbe30c6b 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -30,8 +30,12 @@ [Sources]
> PciHostBridgeUtilityLib.c
>
> [Packages]
> + MdeModulePkg/MdeModulePkg.dec
(2) Why is this needed? Both the MemoryAllocationLib and PciLib class
headers are in MdePkg:
MdePkg/Include/Library/MemoryAllocationLib.h
MdePkg/Include/Library/PciLib.h
... Ah, wait, I see it. This is needed because, unfortunately, we cannot
avoid a dependency on the PCI_ROOT_BRIDGE type, from file
MdeModulePkg/Include/Library/PciHostBridgeLib.h
OK then.
> MdePkg/MdePkg.dec
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> DebugLib
> + MemoryAllocationLib
> + PciLib
> + QemuFwCfgLib
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> index f932d412aa10..1d1c86c69064 100644
> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -14,6 +14,64 @@
> #define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>
>
> +#include <Library/PciHostBridgeLib.h>
> +
> +
> +/**
> + Utility function to free root bridge instances array.
> +
> + @param The root bridge instances array.
> + @param The count of the array.
> +**/
> +VOID
> +EFIAPI
> +PciHostBridgeUtilityFreeRootBridges (
> + PCI_ROOT_BRIDGE *Bridges,
> + UINTN Count
> + );
> +
> +
> +/**
> + Utility function to return all the root bridge instances in an array.
> +
> + @param Count The number of root bridge instances.
> +
> + @param[in] BusMin The min bus number.
> +
> + @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.
> +
> + @param[in] PMem Prefetchable MMIO aperture.
> +
> + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
> +
> + @return All the root bridge instances in an array.
> +**/
> +PCI_ROOT_BRIDGE *
> +EFIAPI
> +PciHostBridgeUtilityExtraRoots (
> + UINTN *Count,
> + UINT32 BusMin,
> + UINT32 BusMax,
> + UINT64 Attributes,
> + UINT64 AllocationAttributes,
> + PCI_ROOT_BRIDGE_APERTURE Io,
> + PCI_ROOT_BRIDGE_APERTURE Mem,
> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G,
> + PCI_ROOT_BRIDGE_APERTURE PMem,
> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G
> + );
> +
> +
> /**
> Utility function to inform the platform that the resource conflict happens.
>
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index 4a176347fd49..19c6e9fa421a 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -21,8 +21,6 @@
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeLib.h>
> #include <Library/PciHostBridgeUtilityLib.h>
> -#include <Library/PciLib.h>
> -#include <Library/QemuFwCfgLib.h>
> #include "PciHostBridge.h"
>
>
> @@ -160,23 +158,6 @@ InitRootBridge (
> }
>
>
> -/**
> - 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 +173,6 @@ 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,117 +212,18 @@ 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,
> + return PciHostBridgeUtilityExtraRoots (
> + Count,
> + 0,
> PCI_MAX_BUS,
> - &Io,
> - &Mem,
> - &MemAbove4G,
> - &mNonExistAperture,
> - &mNonExistAperture,
> - &Bridges[Initialized]
> + Attributes,
> + AllocationAttributes,
> + Io,
> + Mem,
> + MemAbove4G,
> + mNonExistAperture,
> + mNonExistAperture
> );
> - if (EFI_ERROR (Status)) {
> - goto FreeBridges;
> - }
> - ++Initialized;
> -
> - *Count = Initialized;
> - return Bridges;
> -
> -FreeBridges:
> - while (Initialized > 0) {
> - --Initialized;
> - UninitRootBridge (&Bridges[Initialized]);
> - }
> -
> - FreePool (Bridges);
> - return NULL;
> }
>
>
> @@ -367,17 +241,7 @@ PciHostBridgeFreeRootBridges (
> UINTN Count
> )
> {
> - if (Bridges == NULL && Count == 0) {
> - return;
> - }
> - ASSERT (Bridges != NULL && Count > 0);
> -
> - do {
> - --Count;
> - UninitRootBridge (&Bridges[Count]);
> - } while (Count > 0);
> -
> - FreePool (Bridges);
> + PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
> }
>
>
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> index bbaa5f830c98..a0cfd24ce477 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -11,8 +11,13 @@
> **/
>
> #include <IndustryStandard/Acpi10.h>
> +#include <IndustryStandard/Pci.h>
> #include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeUtilityLib.h>
> +#include <Library/PciLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +#include "Library/PciHostBridgeLib/PciHostBridge.h"
(3) This approach is wrong.
Library instances in edk2 are supposed to be self-contained. It is
non-idiomatic in edk2 for a consumed library class to expect a
*dependent* (= consumer) library instance to provide a *callback*, with
a particular funcion name. Whenever such generality is necessary, such
that a callback cannot be avoided, then a typedef (for a function
pointer type) must be introduced in the consumed library class, and the
consumer (= dependent) library instance passes in the concrete callback
as a pointer-to-function parameter -- wherever necessary.
However, we do not need that level of generality here -- we don't need
any callbacks.
(3a) You first need to extract the InitRootBridge() and
UninitRootBridge() functions to PciHostBridgeUtilityLib(), in a
separate patch.
In the same patch, move the OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH
typedef, and the "mRootBridgeDevicePathTemplate" global variable
too. (Notice that the latter is identical to
"mEfiPciRootBridgeDevicePath" in
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c").
No change of functionality; only rename the functions, in addition
to moving them.
Note that his code extraction / rename will affect the
"XenSupport.c" file too!
(3b) Then, in another patch, you need to extend the parameter list of
PciHostBridgeUtilityInitRootBridge() with the following parameters:
- DmaAbove4G
- NoExtendedConfigSpace
(3c) Then, in another patch, rebase ArmVirtPkg's PciHostBridgeLib
instance to the new PciHostBridgeUtilityInitRootBridge() /
PciHostBridgeUtilityUninitRootBridge() functions. Note that this
will affect the *single* root bus, at this point; namely the
PciHostBridgeGetRootBridges() function in
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c".
This will allow you to verify whether the new (extended)
PciHostBridgeUtilityInitRootBridge() interface is flexible enough.
The EFI_PCI_ROOT_BRIDGE_DEVICE_PATH typedef and the
"mEfiPciRootBridgeDevicePath" variable should disappear from
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c", at
this point.
(3d) Then you can proceed to extracting the fw_cfg-parsing logic from
OvmfPkg's PciHostBridgeLib instance to PciHostBridgeUtilityLib.
At first, there should be no change in functionality; in
particular, the DmaAbove4G and NoExtendedConfigSpace parameters
should remain hard-coded, in the call(s) to
PciHostBridgeUtilityInitRootBridge().
The name of the new functions should be
PciHostBridgeUtilityGetRootBridges() -- not
PciHostBridgeUtilityExtraRoots() --, and
PciHostBridgeUtilityFreeRootBridges() -- already named correctly in
your patch, apparently.
(3e) Extend PciHostBridgeUtilityGetRootBridges() with the DmaAbove4G and
NoExtendedConfigSpace parameters, so they are propagated from the
outside into PciHostBridgeUtilityInitRootBridge().
(3f) Call PciHostBridgeUtilityGetRootBridges() in ArmVirtPkg's
FdtPciHostBridgeLib.
>
>
> GLOBAL_REMOVE_IF_UNREFERENCED
> @@ -21,6 +26,213 @@ CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
> };
>
>
> +/**
> + 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);
> +}
> +
> +
> +/**
> + Utility function to free root bridge instances array.
> +
> + @param The root bridge instances array.
> + @param The count of the array.
> +**/
> +VOID
> +EFIAPI
> +PciHostBridgeUtilityFreeRootBridges (
> + 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);
> +}
> +
> +
> +/**
> + Utility function to return all the root bridge instances in an array.
> +
> + @param Count The number of root bridge instances.
(4) This should be @param[out].
Thanks
Laszlo
> +
> + @param[in] BusMin The min bus number.
> +
> + @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.
> +
> + @param[in] PMem Prefetchable MMIO aperture.
> +
> + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
> +
> + @return All the root bridge instances in an array.
> +**/
> +PCI_ROOT_BRIDGE *
> +EFIAPI
> +PciHostBridgeUtilityExtraRoots (
> + UINTN *Count,
> + UINT32 BusMin,
> + UINT32 BusMax,
> + UINT64 Attributes,
> + UINT64 AllocationAttributes,
> + PCI_ROOT_BRIDGE_APERTURE Io,
> + PCI_ROOT_BRIDGE_APERTURE Mem,
> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G,
> + PCI_ROOT_BRIDGE_APERTURE PMem,
> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G
> + )
> +{
> + 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 - BusMin) {
> + 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 = BusMin;
> +
> + //
> + // 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 = BusMin + 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;
> +}
> +
> +
> /**
> Utility function to inform the platform that the resource conflict happens.
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 4/5] ArmVirtPkg: Add support for extra pci roots
2020-12-22 9:59 ` [PATCH v3 4/5] ArmVirtPkg: Add support " Jiahui Cen
@ 2021-01-06 10:28 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-01-06 10:28 UTC (permalink / raw)
To: devel, cenjiahui
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> Use utility functions in PciHostBridgeUtilityLib and some platform specific
> functions to add support for extra pci roots in ArmVirtPkg.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> ---
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 138 ++++++++++++++------
> 1 file changed, 101 insertions(+), 37 deletions(-)
Skipping this patch now, due to the required restructuring I outlined
under patch v3 3/5.
Thanks
Laszlo
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index d554479bf0de..a29dcecf7044 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -7,6 +7,7 @@
>
> **/
> #include <PiDxe.h>
> +#include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/DxeServicesTableLib.h>
> @@ -302,7 +303,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->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 ((DEBUG_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 ((DEBUG_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.
> @@ -319,11 +373,18 @@ 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;
> + EFI_STATUS Status;
> + UINT64 Attributes;
> + UINT64 AllocationAttributes;
> + PCI_ROOT_BRIDGE_APERTURE Io;
> + PCI_ROOT_BRIDGE_APERTURE Mem;
> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
> + PCI_ROOT_BRIDGE_APERTURE PMem;
> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
>
> if (PcdGet64 (PcdPciExpressBaseAddress) == 0) {
> DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__));
> @@ -341,33 +402,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 {
> //
> @@ -376,21 +431,30 @@ 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;
> + return PciHostBridgeUtilityExtraRoots (
> + Count,
> + BusMin,
> + BusMax,
> + Attributes,
> + AllocationAttributes,
> + Io,
> + Mem,
> + MemAbove4G,
> + PMem,
> + PMemAbove4G
> + );
> }
>
> /**
> @@ -407,7 +471,7 @@ PciHostBridgeFreeRootBridges (
> UINTN Count
> )
> {
> - ASSERT (Count == 1);
> + PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
> }
>
> /**
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
2020-12-22 9:59 ` [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
@ 2021-01-06 10:31 ` Laszlo Ersek
2021-01-07 5:47 ` Jiahui Cen
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2021-01-06 10:31 UTC (permalink / raw)
To: devel, cenjiahui
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
> It is necessary to add padding for hotplugable PCI Devices like
> pcie-root-port.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index a11ffd9ba553..d77c226d80ee 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -488,6 +488,7 @@ [Components.common]
> <LibraryClasses>
> NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> }
> + OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
> OvmfPkg/Virtio10Dxe/Virtio10.inf
>
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index 1752fee12b79..5b1d10057545 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -162,6 +162,7 @@ [FV.FvMain]
> INF ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
> INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> INF MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> + INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
> INF OvmfPkg/Virtio10Dxe/Virtio10.inf
>
>
(1) Please be more diligent / careful. You are modifying an FDF
*include* file. Obviously, you want to check what includes the file, no?
ArmVirtPkg/ArmVirtQemu.fdf:!include ArmVirtQemuFvMain.fdf.inc
ArmVirtPkg/ArmVirtQemuKernel.fdf:!include ArmVirtQemuFvMain.fdf.inc
Thus, you need to modify "ArmVirtPkg/ArmVirtQemuKernel.dsc" as well;
otherwise the ArmVirtQemuKernel platform build will break.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class
2021-01-06 8:51 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-07 5:44 ` Jiahui Cen
0 siblings, 0 replies; 15+ messages in thread
From: Jiahui Cen @ 2021-01-07 5:44 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
Hi Laszlo,
On 2021/1/6 16:51, Laszlo Ersek wrote:
> On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
>> Introduce a new PciHostBridgeUtilityLib class to share duplicate code
>> between OvmfPkg and ArmVirtPkg.
>>
>> Extract function PciHostBridgeUtilityResourceConflict from
>> PciHostBridgeResourceConflict in OvmfPkg/PciHostBridgeLib.
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Cc: Peter Grehan <grehan@freebsd.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Julien Grall <julien@xen.org>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>> ---
>> OvmfPkg/OvmfPkg.dec | 4 +
>> OvmfPkg/Bhyve/BhyveX64.dsc | 1 +
>> 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 | 37 ++++++++++
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 37 ++++++++++
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 41 +----------
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 77 ++++++++++++++++++++
>> 11 files changed, 163 insertions(+), 39 deletions(-)
>
> (1) Since you posted v2, OvmfPkg gained a new platform DSC file. The
> full set of DSC files in OvmfPkg is, at this point:
>
> OvmfPkg/AmdSev/AmdSevX64.dsc
> OvmfPkg/Bhyve/BhyveX64.dsc
> OvmfPkg/OvmfPkgIa32.dsc
> OvmfPkg/OvmfPkgIa32X64.dsc
> OvmfPkg/OvmfPkgX64.dsc
> OvmfPkg/OvmfXen.dsc
>
> "OvmfPkg/AmdSev/AmdSevX64.dsc" consumes
> "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf" as well; please
> add the new PciHostBridgeUtilityLib resolution to that DSC file too, in v4.
>
> (You should check for any and all DSC files with such a dependency,
> every time you rebase.)
Thanks for your review. I'll fix it in v4, and will be careful about
the dependencies in the future.
Thanks,
Jiahui
>
> With that update, I'm ready to grant an R-b for this patch.
>
> Thanks!
> Laszlo
>
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 50d7b27d941c..e39097a253a1 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.
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>> index b93fe30ae4e0..f1fdd85d1911 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>> @@ -660,6 +660,7 @@ [Components]
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 26a013ec353e..6eef5e0cfa9c 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -745,6 +745,7 @@ [Components]
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 10579fe46c5b..4b2f48406543 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -759,6 +759,7 @@ [Components.X64]
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index c9235e48ad62..8577ccaa35af 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -755,6 +755,7 @@ [Components]
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>> index 12b7a87ee877..fa35d122cf3e 100644
>> --- a/OvmfPkg/OvmfXen.dsc
>> +++ b/OvmfPkg/OvmfXen.dsc
>> @@ -550,6 +550,7 @@ [Components]
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>> <LibraryClasses>
>> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
>> }
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> index 6ec9ec751478..4c56f3c90b3b 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> @@ -39,6 +39,7 @@ [LibraryClasses]
>> DebugLib
>> DevicePathLib
>> MemoryAllocationLib
>> + PciHostBridgeUtilityLib
>> PciLib
>> QemuFwCfgLib
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> new file mode 100644
>> index 000000000000..1ba8ec3e03c7
>> --- /dev/null
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> @@ -0,0 +1,37 @@
>> +## @file
>> +# 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>
>> +# Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +#
>> +##
>> +
>> +[Defines]
>> + 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 AARCH64 ARM
>> +#
>> +
>> +[Sources]
>> + PciHostBridgeUtilityLib.c
>> +
>> +[Packages]
>> + MdePkg/MdePkg.dec
>> + OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> + DebugLib
>> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> new file mode 100644
>> index 000000000000..f932d412aa10
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> @@ -0,0 +1,37 @@
>> +/** @file
>> + Provide common utility functions to PciHostBridgeLib instances in
>> + ArmVirtPkg and OvmfPkg.
>> +
>> + Copyright (C) 2016, Red Hat, Inc.
>> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>> + Copyright (c) 2020, Huawei 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__
>> +
>> +
>> +/**
>> + Utility function to inform the platform that the resource conflict happens.
>> +
>> + @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
>> +PciHostBridgeUtilityResourceConflict (
>> + VOID *Configuration
>> + );
>> +
>> +
>> +#endif // __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> index e850f7d183ee..4a176347fd49 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> @@ -20,6 +20,7 @@
>> #include <Library/DevicePathLib.h>
>> #include <Library/MemoryAllocationLib.h>
>> #include <Library/PciHostBridgeLib.h>
>> +#include <Library/PciHostBridgeUtilityLib.h>
>> #include <Library/PciLib.h>
>> #include <Library/QemuFwCfgLib.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 = {
>> @@ -407,37 +402,5 @@ PciHostBridgeResourceConflict (
>> 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
>> - );
>> - }
>> + PciHostBridgeUtilityResourceConflict (Configuration);
>> }
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> new file mode 100644
>> index 000000000000..bbaa5f830c98
>> --- /dev/null
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> @@ -0,0 +1,77 @@
>> +/** @file
>> + Provide common utility functions to PciHostBridgeLib instances in
>> + ArmVirtPkg and OvmfPkg.
>> +
>> + Copyright (C) 2016, Red Hat, Inc.
>> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>> + Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +
>> +#include <IndustryStandard/Acpi10.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/PciHostBridgeUtilityLib.h>
>> +
>> +
>> +GLOBAL_REMOVE_IF_UNREFERENCED
>> +CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
>> + L"Mem", L"I/O", L"Bus"
>> +};
>> +
>> +
>> +/**
>> + Utility function to inform the platform that the resource conflict happens.
>> +
>> + @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
>> +PciHostBridgeUtilityResourceConflict (
>> + 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 (mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr)
>> + );
>> + DEBUG ((DEBUG_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
>> + mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[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
>> + );
>> + }
>> +}
>> +
>>
>
> .
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
2021-01-06 9:12 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-07 5:44 ` Jiahui Cen
0 siblings, 0 replies; 15+ messages in thread
From: Jiahui Cen @ 2021-01-07 5:44 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
Hi Laszlo,
On 2021/1/6 17:12, Laszlo Ersek wrote:
> On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
>> Eliminate currently duplicated code in ArmVirtPkg with the common utility
>> class PciHostBridgeUtilityLib.
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>> ---
>> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
>> ArmVirtPkg/ArmVirtKvmTool.dsc | 1 +
>> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
>> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 +
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 44 ++------------------
>> 6 files changed, 9 insertions(+), 41 deletions(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index 9ec92930472d..b9a0cd362416 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -145,6 +145,7 @@ [LibraryClasses.common]
>> PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
>> PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
>> PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>>
>> # USB Libraries
>> UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
>> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
>> index bf008be50fcb..f817132ba7b0 100644
>> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
>> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
>> @@ -65,6 +65,7 @@ [LibraryClasses.common]
>> PlatformPeiLib|ArmVirtPkg/Library/KvmtoolPlatformPeiLib/KvmtoolPlatformPeiLib.inf
>>
>> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
>> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
>>
>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>> index ef5d6dbeaddc..a11ffd9ba553 100644
>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>> @@ -78,6 +78,7 @@ [LibraryClasses.common]
>> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
>> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>> PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>>
>> !if $(TPM2_ENABLE) == TRUE
>> Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> index f8f5f7f4b94b..c27752b4d5e5 100644
>> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> @@ -76,6 +76,7 @@ [LibraryClasses.common]
>> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
>> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>> PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>>
>> [LibraryClasses.common.DXE_DRIVER]
>
> Please pay more attention to review feedback. In v2, I requested two
> things, and those have not been resolved precisely:
>
> (1) The files
>
> - ArmVirtPkg/ArmVirtKvmTool.dsc
> - ArmVirtPkg/ArmVirtQemu.dsc
> - ArmVirtPkg/ArmVirtQemuKernel.dsc
>
> should be updated *instead of* "ArmVirtPkg/ArmVirt.dsc.inc" -- not "in
> addition to" the latter.
>
> So please drop the "ArmVirtPkg/ArmVirt.dsc.inc" hunk.
>
> (2) In "ArmVirtPkg/ArmVirtKvmTool.dsc", you didn't place the
> PciHostBridgeUtilityLib class resolution right after the
> PciHostBridgeLib one.
>
> So please move the new lib class resolution into said (more intuitive) spot.
>
Sorry for missing these review points. Will fix them in v4.
Thanks,
Jiahui
>
> With the above updates, the patch is going to be OK.
>
> Thanks,
> Laszlo
>
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> index 277ccfd24546..01d39626d14c 100644
>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> @@ -31,12 +31,14 @@ [Packages]
>> ArmVirtPkg/ArmVirtPkg.dec
>> MdeModulePkg/MdeModulePkg.dec
>> MdePkg/MdePkg.dec
>> + OvmfPkg/OvmfPkg.dec
>>
>> [LibraryClasses]
>> DebugLib
>> DevicePathLib
>> DxeServicesTableLib
>> MemoryAllocationLib
>> + PciHostBridgeUtilityLib
>> PciPcdProducerLib
>>
>> [FixedPcd]
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> index 496b192d2291..d554479bf0de 100644
>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> @@ -7,12 +7,13 @@
>>
>> **/
>> #include <PiDxe.h>
>> -#include <Library/PciHostBridgeLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/DevicePathLib.h>
>> #include <Library/DxeServicesTableLib.h>
>> #include <Library/MemoryAllocationLib.h>
>> #include <Library/PcdLib.h>
>> +#include <Library/PciHostBridgeLib.h>
>> +#include <Library/PciHostBridgeUtilityLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>>
>> #include <Protocol/FdtClient.h>
>> @@ -50,11 +51,6 @@ STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
>> }
>> };
>>
>> -GLOBAL_REMOVE_IF_UNREFERENCED
>> -CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
>> - L"Mem", L"I/O", L"Bus"
>> -};
>> -
>> //
>> // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
>> // records like this.
>> @@ -435,39 +431,5 @@ PciHostBridgeResourceConflict (
>> VOID *Configuration
>> )
>> {
>> - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
>> - UINTN RootBridgeIndex;
>> - DEBUG ((EFI_D_ERROR, "PciHostBridge: Resource conflict happens!\n"));
>> -
>> - RootBridgeIndex = 0;
>> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
>> - while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
>> - DEBUG ((EFI_D_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
>> - for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
>> - ASSERT (Descriptor->ResType <
>> - (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) /
>> - sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0])
>> - )
>> - );
>> - DEBUG ((EFI_D_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
>> - mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
>> - Descriptor->AddrLen, Descriptor->AddrRangeMax
>> - ));
>> - if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>> - DEBUG ((EFI_D_ERROR, " Granularity/SpecificFlag = %ld / %02x%s\n",
>> - Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
>> - ((Descriptor->SpecificFlag &
>> - EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE
>> - ) != 0) ? L" (Prefetchable)" : L""
>> - ));
>> - }
>> - }
>> - //
>> - // Skip the END descriptor for root bridge
>> - //
>> - ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
>> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
>> - (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1
>> - );
>> - }
>> + PciHostBridgeUtilityResourceConflict (Configuration);
>> }
>>
>
> .
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots
2021-01-06 10:27 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-07 5:47 ` Jiahui Cen
0 siblings, 0 replies; 15+ messages in thread
From: Jiahui Cen @ 2021-01-07 5:47 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
Hi Laszlo,
Thanks for your detailed review!
On 2021/1/6 18:27, Laszlo Ersek wrote:
> On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
>> Extract functions that support extra pci roots from
>> OvmfPkg/PciHostBridgeLib to share this feature with other packages.
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>> ---
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 4 +
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 58 ++++++
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +--------------
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 212 ++++++++++++++++++++
>> 5 files changed, 285 insertions(+), 149 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> index 4c56f3c90b3b..8bdb76899111 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> @@ -40,8 +40,6 @@ [LibraryClasses]
>> DevicePathLib
>> MemoryAllocationLib
>> PciHostBridgeUtilityLib
>> - PciLib
>
> (1) Removing the PciLib class dependency is incorrect. The
> "XenSupport.c" file continues using PciRead32(), for example.
Will fix it.
>
>> - QemuFwCfgLib
>>
>> [Pcd]
>> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> index 1ba8ec3e03c7..a10afbe30c6b 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> @@ -30,8 +30,12 @@ [Sources]
>> PciHostBridgeUtilityLib.c
>>
>> [Packages]
>> + MdeModulePkg/MdeModulePkg.dec
>
> (2) Why is this needed? Both the MemoryAllocationLib and PciLib class
> headers are in MdePkg:
>
> MdePkg/Include/Library/MemoryAllocationLib.h
> MdePkg/Include/Library/PciLib.h
>
> ... Ah, wait, I see it. This is needed because, unfortunately, we cannot
> avoid a dependency on the PCI_ROOT_BRIDGE type, from file
>
> MdeModulePkg/Include/Library/PciHostBridgeLib.h
>
> OK then.
>
>> MdePkg/MdePkg.dec
>> OvmfPkg/OvmfPkg.dec
>>
>> [LibraryClasses]
>> DebugLib
>> + MemoryAllocationLib
>> + PciLib
>> + QemuFwCfgLib
>> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> index f932d412aa10..1d1c86c69064 100644
>> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> @@ -14,6 +14,64 @@
>> #define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>>
>>
>> +#include <Library/PciHostBridgeLib.h>
>> +
>> +
>> +/**
>> + Utility function to free root bridge instances array.
>> +
>> + @param The root bridge instances array.
>> + @param The count of the array.
>> +**/
>> +VOID
>> +EFIAPI
>> +PciHostBridgeUtilityFreeRootBridges (
>> + PCI_ROOT_BRIDGE *Bridges,
>> + UINTN Count
>> + );
>> +
>> +
>> +/**
>> + Utility function to return all the root bridge instances in an array.
>> +
>> + @param Count The number of root bridge instances.
>> +
>> + @param[in] BusMin The min bus number.
>> +
>> + @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.
>> +
>> + @param[in] PMem Prefetchable MMIO aperture.
>> +
>> + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> +
>> + @return All the root bridge instances in an array.
>> +**/
>> +PCI_ROOT_BRIDGE *
>> +EFIAPI
>> +PciHostBridgeUtilityExtraRoots (
>> + UINTN *Count,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> + UINT64 Attributes,
>> + UINT64 AllocationAttributes,
>> + PCI_ROOT_BRIDGE_APERTURE Io,
>> + PCI_ROOT_BRIDGE_APERTURE Mem,
>> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G,
>> + PCI_ROOT_BRIDGE_APERTURE PMem,
>> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G
>> + );
>> +
>> +
>> /**
>> Utility function to inform the platform that the resource conflict happens.
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> index 4a176347fd49..19c6e9fa421a 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> @@ -21,8 +21,6 @@
>> #include <Library/MemoryAllocationLib.h>
>> #include <Library/PciHostBridgeLib.h>
>> #include <Library/PciHostBridgeUtilityLib.h>
>> -#include <Library/PciLib.h>
>> -#include <Library/QemuFwCfgLib.h>
>> #include "PciHostBridge.h"
>>
>>
>> @@ -160,23 +158,6 @@ InitRootBridge (
>> }
>>
>>
>> -/**
>> - 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 +173,6 @@ 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,117 +212,18 @@ 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,
>> + return PciHostBridgeUtilityExtraRoots (
>> + Count,
>> + 0,
>> PCI_MAX_BUS,
>> - &Io,
>> - &Mem,
>> - &MemAbove4G,
>> - &mNonExistAperture,
>> - &mNonExistAperture,
>> - &Bridges[Initialized]
>> + Attributes,
>> + AllocationAttributes,
>> + Io,
>> + Mem,
>> + MemAbove4G,
>> + mNonExistAperture,
>> + mNonExistAperture
>> );
>> - if (EFI_ERROR (Status)) {
>> - goto FreeBridges;
>> - }
>> - ++Initialized;
>> -
>> - *Count = Initialized;
>> - return Bridges;
>> -
>> -FreeBridges:
>> - while (Initialized > 0) {
>> - --Initialized;
>> - UninitRootBridge (&Bridges[Initialized]);
>> - }
>> -
>> - FreePool (Bridges);
>> - return NULL;
>> }
>>
>>
>> @@ -367,17 +241,7 @@ PciHostBridgeFreeRootBridges (
>> UINTN Count
>> )
>> {
>> - if (Bridges == NULL && Count == 0) {
>> - return;
>> - }
>> - ASSERT (Bridges != NULL && Count > 0);
>> -
>> - do {
>> - --Count;
>> - UninitRootBridge (&Bridges[Count]);
>> - } while (Count > 0);
>> -
>> - FreePool (Bridges);
>> + PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
>> }
>>
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> index bbaa5f830c98..a0cfd24ce477 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> @@ -11,8 +11,13 @@
>> **/
>>
>> #include <IndustryStandard/Acpi10.h>
>> +#include <IndustryStandard/Pci.h>
>> #include <Library/DebugLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> #include <Library/PciHostBridgeUtilityLib.h>
>> +#include <Library/PciLib.h>
>> +#include <Library/QemuFwCfgLib.h>
>> +#include "Library/PciHostBridgeLib/PciHostBridge.h"
>
> (3) This approach is wrong.
>
> Library instances in edk2 are supposed to be self-contained. It is
> non-idiomatic in edk2 for a consumed library class to expect a
> *dependent* (= consumer) library instance to provide a *callback*, with
> a particular funcion name. Whenever such generality is necessary, such
> that a callback cannot be avoided, then a typedef (for a function
> pointer type) must be introduced in the consumed library class, and the
> consumer (= dependent) library instance passes in the concrete callback
> as a pointer-to-function parameter -- wherever necessary.
Thanks for the explanation.
>
> However, we do not need that level of generality here -- we don't need
> any callbacks.
>
> (3a) You first need to extract the InitRootBridge() and
> UninitRootBridge() functions to PciHostBridgeUtilityLib(), in a
> separate patch.
>
> In the same patch, move the OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH
> typedef, and the "mRootBridgeDevicePathTemplate" global variable
> too. (Notice that the latter is identical to
> "mEfiPciRootBridgeDevicePath" in
> "ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c").
>
> No change of functionality; only rename the functions, in addition
> to moving them.
>
> Note that his code extraction / rename will affect the
> "XenSupport.c" file too!
>
> (3b) Then, in another patch, you need to extend the parameter list of
> PciHostBridgeUtilityInitRootBridge() with the following parameters:
>
> - DmaAbove4G
> - NoExtendedConfigSpace
>
> (3c) Then, in another patch, rebase ArmVirtPkg's PciHostBridgeLib
> instance to the new PciHostBridgeUtilityInitRootBridge() /
> PciHostBridgeUtilityUninitRootBridge() functions. Note that this
> will affect the *single* root bus, at this point; namely the
> PciHostBridgeGetRootBridges() function in
> "ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c".
>
> This will allow you to verify whether the new (extended)
> PciHostBridgeUtilityInitRootBridge() interface is flexible enough.
>
> The EFI_PCI_ROOT_BRIDGE_DEVICE_PATH typedef and the
> "mEfiPciRootBridgeDevicePath" variable should disappear from
> "ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c", at
> this point.
>
> (3d) Then you can proceed to extracting the fw_cfg-parsing logic from
> OvmfPkg's PciHostBridgeLib instance to PciHostBridgeUtilityLib.
>
> At first, there should be no change in functionality; in
> particular, the DmaAbove4G and NoExtendedConfigSpace parameters
> should remain hard-coded, in the call(s) to
> PciHostBridgeUtilityInitRootBridge().
>
> The name of the new functions should be
> PciHostBridgeUtilityGetRootBridges() -- not
> PciHostBridgeUtilityExtraRoots() --, and
> PciHostBridgeUtilityFreeRootBridges() -- already named correctly in
> your patch, apparently.
>
> (3e) Extend PciHostBridgeUtilityGetRootBridges() with the DmaAbove4G and
> NoExtendedConfigSpace parameters, so they are propagated from the
> outside into PciHostBridgeUtilityInitRootBridge().
>
> (3f) Call PciHostBridgeUtilityGetRootBridges() in ArmVirtPkg's
> FdtPciHostBridgeLib.
>
Got it. I will refactor the patches as you points.
>
>>
>>
>> GLOBAL_REMOVE_IF_UNREFERENCED
>> @@ -21,6 +26,213 @@ CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
>> };
>>
>>
>> +/**
>> + 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);
>> +}
>> +
>> +
>> +/**
>> + Utility function to free root bridge instances array.
>> +
>> + @param The root bridge instances array.
>> + @param The count of the array.
>> +**/
>> +VOID
>> +EFIAPI
>> +PciHostBridgeUtilityFreeRootBridges (
>> + 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);
>> +}
>> +
>> +
>> +/**
>> + Utility function to return all the root bridge instances in an array.
>> +
>> + @param Count The number of root bridge instances.
>
> (4) This should be @param[out].
Yes. Will fix.
Thanks,
Jiahui
>
> Thanks
> Laszlo
>
>> +
>> + @param[in] BusMin The min bus number.
>> +
>> + @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.
>> +
>> + @param[in] PMem Prefetchable MMIO aperture.
>> +
>> + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> +
>> + @return All the root bridge instances in an array.
>> +**/
>> +PCI_ROOT_BRIDGE *
>> +EFIAPI
>> +PciHostBridgeUtilityExtraRoots (
>> + UINTN *Count,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> + UINT64 Attributes,
>> + UINT64 AllocationAttributes,
>> + PCI_ROOT_BRIDGE_APERTURE Io,
>> + PCI_ROOT_BRIDGE_APERTURE Mem,
>> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G,
>> + PCI_ROOT_BRIDGE_APERTURE PMem,
>> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G
>> + )
>> +{
>> + 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 - BusMin) {
>> + 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 = BusMin;
>> +
>> + //
>> + // 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 = BusMin + 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;
>> +}
>> +
>> +
>> /**
>> Utility function to inform the platform that the resource conflict happens.
>>
>>
>
> .
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
2021-01-06 10:31 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-07 5:47 ` Jiahui Cen
0 siblings, 0 replies; 15+ messages in thread
From: Jiahui Cen @ 2021-01-07 5:47 UTC (permalink / raw)
To: devel, lersek
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
Hi Laszlo,
On 2021/1/6 18:31, Laszlo Ersek wrote:
> On 12/22/20 10:59, Jiahui Cen via groups.io wrote:
>> It is necessary to add padding for hotplugable PCI Devices like
>> pcie-root-port.
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
>> ---
>> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
>> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>> index a11ffd9ba553..d77c226d80ee 100644
>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>> @@ -488,6 +488,7 @@ [Components.common]
>> <LibraryClasses>
>> NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
>> }
>> + OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>> OvmfPkg/Virtio10Dxe/Virtio10.inf
>>
>> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
>> index 1752fee12b79..5b1d10057545 100644
>> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
>> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
>> @@ -162,6 +162,7 @@ [FV.FvMain]
>> INF ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
>> INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> INF MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> + INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>> INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>> INF OvmfPkg/Virtio10Dxe/Virtio10.inf
>>
>>
>
> (1) Please be more diligent / careful. You are modifying an FDF
> *include* file. Obviously, you want to check what includes the file, no?
>
> ArmVirtPkg/ArmVirtQemu.fdf:!include ArmVirtQemuFvMain.fdf.inc
> ArmVirtPkg/ArmVirtQemuKernel.fdf:!include ArmVirtQemuFvMain.fdf.inc
>
> Thus, you need to modify "ArmVirtPkg/ArmVirtQemuKernel.dsc" as well;
> otherwise the ArmVirtQemuKernel platform build will break.
Will add it and check more carefully.
Thanks,
Jiahui
>
> Thanks
> Laszlo
>
>
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-01-07 5:47 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-22 9:59 [PATCH v3 0/5] Add extra pci roots support for Arm Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
2021-01-06 8:51 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:44 ` Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 2/5] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
2021-01-06 9:12 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:44 ` Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 3/5] OvmfPkg: Extract functions for extra pci roots Jiahui Cen
2021-01-06 10:27 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:47 ` Jiahui Cen
2020-12-22 9:59 ` [PATCH v3 4/5] ArmVirtPkg: Add support " Jiahui Cen
2021-01-06 10:28 ` [edk2-devel] " Laszlo Ersek
2020-12-22 9:59 ` [PATCH v3 5/5] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
2021-01-06 10:31 ` [edk2-devel] " Laszlo Ersek
2021-01-07 5:47 ` Jiahui Cen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox