* [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-13 0:41 ` [edk2-devel] " Laszlo Ersek
2021-01-14 8:57 ` Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 2/9] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
` (8 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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/AmdSev/AmdSevX64.dsc | 1 +
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 ++++++++++++++++++++
12 files changed, 164 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/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index bb7697eb324b..33c84ddf2e7f 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -665,6 +665,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/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.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class
2021-01-12 9:45 ` [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
@ 2021-01-13 0:41 ` Laszlo Ersek
2021-01-14 8:57 ` Laszlo Ersek
1 sibling, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 0:41 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 01/12/21 10:45, 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/AmdSev/AmdSevX64.dsc | 1 +
> 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 ++++++++++++++++++++
> 12 files changed, 164 insertions(+), 39 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class
2021-01-12 9:45 ` [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
2021-01-13 0:41 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-14 8:57 ` Laszlo Ersek
1 sibling, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 8:57 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 01/12/21 10:45, 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/AmdSev/AmdSevX64.dsc | 1 +
> 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 ++++++++++++++++++++
> 12 files changed, 164 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/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index bb7697eb324b..33c84ddf2e7f 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -665,6 +665,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/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
> + );
(1) The "Configuration" parameter should be annotated "IN", both in the
declaration of the function, and the definition (implementation) of the
function.
(2) Please update @param to @param[in], accordingly. (Don't forget to
sync the header file and the C file.)
Thanks
Laszlo
> +
> +
> +#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] 35+ messages in thread
* [PATCH v4 2/9] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-13 0:44 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge Jiahui Cen
` (7 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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/ArmVirtKvmTool.dsc | 1 +
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 44 ++------------------
5 files changed, 8 insertions(+), 41 deletions(-)
diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index bf008be50fcb..e027b17bec1b 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -58,6 +58,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
AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.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.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 2/9] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib
2021-01-12 9:45 ` [PATCH v4 2/9] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
@ 2021-01-13 0:44 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 0:44 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 01/12/21 10:45, 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/ArmVirtKvmTool.dsc | 1 +
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 2 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 44 ++------------------
> 5 files changed, 8 insertions(+), 41 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 2/9] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-13 1:28 ` [edk2-devel] " Laszlo Ersek
2021-01-14 8:51 ` Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge Jiahui Cen
` (6 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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 InitRootBridge/UninitRootBridge to PciHostBridgeUtilityLib as
common utility functions. No change of functionality.
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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 7 +
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 75 ++++++++++
OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h | 56 -------
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +-------------------
OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 3 +-
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 155 +++++++++++++++++++
7 files changed, 243 insertions(+), 213 deletions(-)
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index 4c56f3c90b3b..d09429d9f0fb 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -37,7 +37,6 @@ [Packages]
[LibraryClasses]
BaseMemoryLib
DebugLib
- DevicePathLib
MemoryAllocationLib
PciHostBridgeUtilityLib
PciLib
@@ -50,5 +49,4 @@ [Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
- gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
index 1ba8ec3e03c7..32a10e99c3e0 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -30,8 +30,15 @@ [Sources]
PciHostBridgeUtilityLib.c
[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseMemoryLib
DebugLib
+ DevicePathLib
+ MemoryAllocationLib
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
index f932d412aa10..f02cec812a50 100644
--- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -14,6 +14,81 @@
#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
+#include <Library/PciHostBridgeLib.h>
+
+
+/**
+ Utility function to initialize a PCI_ROOT_BRIDGE structure.
+
+ @param[in] Supports Supported attributes.
+
+ @param[in] Attributes Initial attributes.
+
+ @param[in] AllocAttributes Allocation attributes.
+
+ @param[in] RootBusNumber The bus number to store in RootBus.
+
+ @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
+ assigned to any subordinate bus found behind any
+ PCI bridge hanging off this root bus.
+
+ The caller is repsonsible for ensuring that
+ RootBusNumber <= MaxSubBusNumber. If
+ RootBusNumber equals MaxSubBusNumber, then the
+ root bus has no room for subordinate buses.
+
+ @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.
+
+ @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
+ caller) that should be filled in by this
+ function.
+
+ @retval EFI_SUCCESS Initialization successful. A device path
+ consisting of an ACPI device path node, with
+ UID = RootBusNumber, has been allocated and
+ linked into RootBus.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+**/
+EFI_STATUS
+PciHostBridgeUtilityInitRootBridge (
+ 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
+ );
+
+
+/**
+ Utility function to uninitialize a PCI_ROOT_BRIDGE structure set up with
+ PciHostBridgeUtilityInitRootBridge().
+
+ param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
+ initialized with PciHostBridgeUtilityInitRootBridge(),
+ that should be uninitialized. This function doesn't free
+ RootBus.
+**/
+VOID
+PciHostBridgeUtilityUninitRootBridge (
+ IN PCI_ROOT_BRIDGE *RootBus
+ );
+
+
/**
Utility function to inform the platform that the resource conflict happens.
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h
index 134d7411214d..a2e4d8696281 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h
@@ -11,59 +11,3 @@ PCI_ROOT_BRIDGE *
ScanForRootBridges (
UINTN *NumberOfRootBridges
);
-
-/**
- Initialize a PCI_ROOT_BRIDGE structure.
-
- @param[in] Supports Supported attributes.
-
- @param[in] Attributes Initial attributes.
-
- @param[in] AllocAttributes Allocation attributes.
-
- @param[in] RootBusNumber The bus number to store in RootBus.
-
- @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
- assigned to any subordinate bus found behind any
- PCI bridge hanging off this root bus.
-
- The caller is repsonsible for ensuring that
- RootBusNumber <= MaxSubBusNumber. If
- RootBusNumber equals MaxSubBusNumber, then the
- root bus has no room for subordinate buses.
-
- @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.
-
- @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
- caller) that should be filled in by this
- function.
-
- @retval EFI_SUCCESS Initialization successful. A device path
- consisting of an ACPI device path node, with
- UID = RootBusNumber, has been allocated and
- linked into RootBus.
-
- @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
-**/
-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
- );
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index 4a176347fd49..f284244448bb 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -10,14 +10,12 @@
#include <PiDxe.h>
#include <IndustryStandard/Pci.h>
-#include <IndustryStandard/Q35MchIch9.h>
#include <Protocol/PciHostBridgeResourceAllocation.h>
#include <Protocol/PciRootBridgeIo.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
-#include <Library/DevicePathLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
@@ -26,156 +24,8 @@
#include "PciHostBridge.h"
-#pragma pack(1)
-typedef struct {
- ACPI_HID_DEVICE_PATH AcpiDevicePath;
- EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
-} OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH;
-#pragma pack ()
-
-
-STATIC
-CONST
-OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePathTemplate = {
- {
- {
- ACPI_DEVICE_PATH,
- ACPI_DP,
- {
- (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
- (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
- }
- },
- EISA_PNP_ID(0x0A03), // HID
- 0 // UID
- },
-
- {
- END_DEVICE_PATH_TYPE,
- END_ENTIRE_DEVICE_PATH_SUBTYPE,
- {
- END_DEVICE_PATH_LENGTH,
- 0
- }
- }
-};
-
STATIC PCI_ROOT_BRIDGE_APERTURE mNonExistAperture = { MAX_UINT64, 0 };
-/**
- Initialize a PCI_ROOT_BRIDGE structure.
-
- @param[in] Supports Supported attributes.
-
- @param[in] Attributes Initial attributes.
-
- @param[in] AllocAttributes Allocation attributes.
-
- @param[in] RootBusNumber The bus number to store in RootBus.
-
- @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
- assigned to any subordinate bus found behind any
- PCI bridge hanging off this root bus.
-
- The caller is repsonsible for ensuring that
- RootBusNumber <= MaxSubBusNumber. If
- RootBusNumber equals MaxSubBusNumber, then the
- root bus has no room for subordinate buses.
-
- @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.
-
- @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
- caller) that should be filled in by this
- function.
-
- @retval EFI_SUCCESS Initialization successful. A device path
- consisting of an ACPI device path node, with
- UID = RootBusNumber, has been allocated and
- linked into RootBus.
-
- @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
-**/
-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
- )
-{
- OVMF_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 = FALSE;
-
- 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 = (PcdGet16 (PcdOvmfHostBridgePciDevId) !=
- INTEL_Q35_MCH_DEVICE_ID);
-
- DevicePath = AllocateCopyPool (sizeof mRootBridgeDevicePathTemplate,
- &mRootBridgeDevicePathTemplate);
- 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;
-}
-
-
-/**
- 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.
@@ -296,7 +146,7 @@ PciHostBridgeGetRootBridges (
// 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 (
+ Status = PciHostBridgeUtilityInitRootBridge (
Attributes,
Attributes,
AllocationAttributes,
@@ -321,7 +171,7 @@ PciHostBridgeGetRootBridges (
// Install the last root bus (which might be the only, ie. main, root bus, if
// we've found no extra root buses).
//
- Status = InitRootBridge (
+ Status = PciHostBridgeUtilityInitRootBridge (
Attributes,
Attributes,
AllocationAttributes,
@@ -345,7 +195,7 @@ PciHostBridgeGetRootBridges (
FreeBridges:
while (Initialized > 0) {
--Initialized;
- UninitRootBridge (&Bridges[Initialized]);
+ PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]);
}
FreePool (Bridges);
@@ -374,7 +224,7 @@ PciHostBridgeFreeRootBridges (
do {
--Count;
- UninitRootBridge (&Bridges[Count]);
+ PciHostBridgeUtilityUninitRootBridge (&Bridges[Count]);
} while (Count > 0);
FreePool (Bridges);
diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index e161f14375ca..4bd27211ec37 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -18,6 +18,7 @@
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeLib.h>
+#include <Library/PciHostBridgeUtilityLib.h>
#include <Library/PciLib.h>
#include "PciHostBridge.h"
@@ -452,7 +453,7 @@ ScanForRootBridges (
RootBridges
);
ASSERT (RootBridges != NULL);
- InitRootBridge (
+ PciHostBridgeUtilityInitRootBridge (
Attributes, Attributes, 0,
(UINT8) PrimaryBus, (UINT8) SubBus,
&Io, &Mem, &MemAbove4G, &mNonExistAperture, &mNonExistAperture,
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
index bbaa5f830c98..e3db4b766181 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -11,16 +11,171 @@
**/
#include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/Q35MchIch9.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
+#pragma pack(1)
+typedef struct {
+ ACPI_HID_DEVICE_PATH AcpiDevicePath;
+ EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
+} PCI_ROOT_BRIDGE_DEVICE_PATH;
+#pragma pack ()
+
+
GLOBAL_REMOVE_IF_UNREFERENCED
CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = {
L"Mem", L"I/O", L"Bus"
};
+STATIC
+CONST
+PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePath = {
+ {
+ {
+ ACPI_DEVICE_PATH,
+ ACPI_DP,
+ {
+ (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
+ (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
+ }
+ },
+ EISA_PNP_ID(0x0A03), // HID
+ 0 // UID
+ },
+
+ {
+ END_DEVICE_PATH_TYPE,
+ END_ENTIRE_DEVICE_PATH_SUBTYPE,
+ {
+ END_DEVICE_PATH_LENGTH,
+ 0
+ }
+ }
+};
+
+
+/**
+ Utility function to initialize a PCI_ROOT_BRIDGE structure.
+
+ @param[in] Supports Supported attributes.
+
+ @param[in] Attributes Initial attributes.
+
+ @param[in] AllocAttributes Allocation attributes.
+
+ @param[in] RootBusNumber The bus number to store in RootBus.
+
+ @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
+ assigned to any subordinate bus found behind any
+ PCI bridge hanging off this root bus.
+
+ The caller is repsonsible for ensuring that
+ RootBusNumber <= MaxSubBusNumber. If
+ RootBusNumber equals MaxSubBusNumber, then the
+ root bus has no room for subordinate buses.
+
+ @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.
+
+ @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
+ caller) that should be filled in by this
+ function.
+
+ @retval EFI_SUCCESS Initialization successful. A device path
+ consisting of an ACPI device path node, with
+ UID = RootBusNumber, has been allocated and
+ linked into RootBus.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+**/
+EFI_STATUS
+PciHostBridgeUtilityInitRootBridge (
+ 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
+ )
+{
+ 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 = FALSE;
+
+ 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 = (PcdGet16 (PcdOvmfHostBridgePciDevId) !=
+ INTEL_Q35_MCH_DEVICE_ID);
+
+ DevicePath = AllocateCopyPool (sizeof mRootBridgeDevicePath,
+ &mRootBridgeDevicePath);
+ 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;
+}
+
+
+/**
+ Utility function to uninitialize a PCI_ROOT_BRIDGE structure set up with
+ PciHostBridgeUtilityInitRootBridge().
+
+ param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
+ initialized with PciHostBridgeUtilityInitRootBridge(),
+ that should be uninitialized. This function doesn't free
+ RootBus.
+**/
+VOID
+PciHostBridgeUtilityUninitRootBridge (
+ IN PCI_ROOT_BRIDGE *RootBus
+ )
+{
+ FreePool (RootBus->DevicePath);
+}
+
+
/**
Utility function to inform the platform that the resource conflict happens.
--
2.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge
2021-01-12 9:45 ` [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge Jiahui Cen
@ 2021-01-13 1:28 ` Laszlo Ersek
2021-01-13 6:00 ` Jiahui Cen
2021-01-14 8:51 ` Laszlo Ersek
1 sibling, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 1: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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Extract InitRootBridge/UninitRootBridge to PciHostBridgeUtilityLib as
> common utility functions. No change of functionality.
>
> 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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 7 +
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 75 ++++++++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h | 56 -------
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +-------------------
> OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 3 +-
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 155 +++++++++++++++++++
> 7 files changed, 243 insertions(+), 213 deletions(-)
(1) Renaming OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH to
PCI_ROOT_BRIDGE_DEVICE_PATH is not only unnecessary, it carries a risk
of conflicting with something that PciHostBridgeLib in MdeModulePkg
might introduce in the future. Please undo this rename.
(2) Renaming "mRootBridgeDevicePathTemplate" to "mRootBridgeDevicePath"
is not necessary. Although this rename is not risky, it makes the review
(before/after comparison) of this patch more difficult than needed.
Please undo this rename. The word "template" is correct BTW, because we
modify the device path after we allocate & copy it from the template --
see UID.
(3) Unfortunately, the original PciHostBridgeLib instance fails to list
its PcdLib dependency, both between the #include directives, and in the
INF file. I'm not asking you to fix that up, but please do spell out the
PcdLib #include and INF file depencency in the new
PciHostBridgeUtilityLib instance. (See the PcdGet16() call in
PciHostBridgeUtilityInitRootBridge().)
With the above small warts fixed, I'm ready to give R-b.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge
2021-01-13 1:28 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-13 6:00 ` Jiahui Cen
2021-01-13 9:06 ` Laszlo Ersek
0 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-13 6:00 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/13 9:28, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>> Extract InitRootBridge/UninitRootBridge to PciHostBridgeUtilityLib as
>> common utility functions. No change of functionality.
>>
>> 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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 7 +
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 75 ++++++++++
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h | 56 -------
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +-------------------
>> OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 3 +-
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 155 +++++++++++++++++++
>> 7 files changed, 243 insertions(+), 213 deletions(-)
>
> (1) Renaming OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH to
> PCI_ROOT_BRIDGE_DEVICE_PATH is not only unnecessary, it carries a risk
> of conflicting with something that PciHostBridgeLib in MdeModulePkg
> might introduce in the future. Please undo this rename.
>
> (2) Renaming "mRootBridgeDevicePathTemplate" to "mRootBridgeDevicePath"
> is not necessary. Although this rename is not risky, it makes the review
> (before/after comparison) of this patch more difficult than needed.
> Please undo this rename. The word "template" is correct BTW, because we
> modify the device path after we allocate & copy it from the template --
> see UID.
>
Right. I will fix them.
> (3) Unfortunately, the original PciHostBridgeLib instance fails to list
> its PcdLib dependency, both between the #include directives, and in the
> INF file. I'm not asking you to fix that up, but please do spell out the
> PcdLib #include and INF file depencency in the new
> PciHostBridgeUtilityLib instance. (See the PcdGet16() call in
> PciHostBridgeUtilityInitRootBridge().)
>
For the original PciHostBridgeLib, should I fix the dependency
in a new patch? Or just in this patch?
Thanks,
Jiahui
>
> With the above small warts fixed, I'm ready to give R-b.
>
> Thanks
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge
2021-01-13 6:00 ` Jiahui Cen
@ 2021-01-13 9:06 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 9:06 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 01/13/21 07:00, Jiahui Cen wrote:
> Hi Laszlo,
>
> On 2021/1/13 9:28, Laszlo Ersek wrote:
>> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>>> Extract InitRootBridge/UninitRootBridge to PciHostBridgeUtilityLib as
>>> common utility functions. No change of functionality.
>>>
>>> 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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 -
>>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 7 +
>>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 75 ++++++++++
>>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h | 56 -------
>>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +-------------------
>>> OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 3 +-
>>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 155 +++++++++++++++++++
>>> 7 files changed, 243 insertions(+), 213 deletions(-)
>>
>> (1) Renaming OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH to
>> PCI_ROOT_BRIDGE_DEVICE_PATH is not only unnecessary, it carries a risk
>> of conflicting with something that PciHostBridgeLib in MdeModulePkg
>> might introduce in the future. Please undo this rename.
>>
>> (2) Renaming "mRootBridgeDevicePathTemplate" to "mRootBridgeDevicePath"
>> is not necessary. Although this rename is not risky, it makes the review
>> (before/after comparison) of this patch more difficult than needed.
>> Please undo this rename. The word "template" is correct BTW, because we
>> modify the device path after we allocate & copy it from the template --
>> see UID.
>>
>
> Right. I will fix them.
>
>> (3) Unfortunately, the original PciHostBridgeLib instance fails to list
>> its PcdLib dependency, both between the #include directives, and in the
>> INF file. I'm not asking you to fix that up, but please do spell out the
>> PcdLib #include and INF file depencency in the new
>> PciHostBridgeUtilityLib instance. (See the PcdGet16() call in
>> PciHostBridgeUtilityInitRootBridge().)
>>
>
> For the original PciHostBridgeLib, should I fix the dependency
> in a new patch? Or just in this patch?
If you can add a separate patch for that, it's appreciated. It's not
required though -- up to you.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge
2021-01-12 9:45 ` [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge Jiahui Cen
2021-01-13 1:28 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-14 8:51 ` Laszlo Ersek
1 sibling, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> index f932d412aa10..f02cec812a50 100644
> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -14,6 +14,81 @@
> #define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
>
>
> +#include <Library/PciHostBridgeLib.h>
> +
> +
> +/**
> + Utility function to initialize a PCI_ROOT_BRIDGE structure.
> +
> + @param[in] Supports Supported attributes.
> +
> + @param[in] Attributes Initial attributes.
> +
> + @param[in] AllocAttributes Allocation attributes.
> +
> + @param[in] RootBusNumber The bus number to store in RootBus.
> +
> + @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
> + assigned to any subordinate bus found behind any
> + PCI bridge hanging off this root bus.
> +
> + The caller is repsonsible for ensuring that
> + RootBusNumber <= MaxSubBusNumber. If
> + RootBusNumber equals MaxSubBusNumber, then the
> + root bus has no room for subordinate buses.
> +
> + @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.
> +
> + @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
> + caller) that should be filled in by this
> + function.
> +
> + @retval EFI_SUCCESS Initialization successful. A device path
> + consisting of an ACPI device path node, with
> + UID = RootBusNumber, has been allocated and
> + linked into RootBus.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +**/
> +EFI_STATUS
> +PciHostBridgeUtilityInitRootBridge (
> + 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
> + );
> +
> +
> +/**
> + Utility function to uninitialize a PCI_ROOT_BRIDGE structure set up with
> + PciHostBridgeUtilityInitRootBridge().
> +
> + param[in] RootBus The PCI_ROOT_BRIDGE structure, allocated by the caller and
> + initialized with PciHostBridgeUtilityInitRootBridge(),
> + that should be uninitialized. This function doesn't free
> + RootBus.
> +**/
> +VOID
> +PciHostBridgeUtilityUninitRootBridge (
> + IN PCI_ROOT_BRIDGE *RootBus
> + );
> +
> +
> /**
> Utility function to inform the platform that the resource conflict happens.
>
(4) All library functions must be declared EFIAPI.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (2 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-13 1:51 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() Jiahui Cen
` (5 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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
Extend parameter list of PciHostBridgeUtilityInitRootBridge() with
DmaAbove4G and NoExtendedConfigSpace to prepare for support of ArmVirtPkg.
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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 -
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 56 ++++++++++--------
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 5 ++
OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 1 +
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 62 +++++++++++---------
6 files changed, 73 insertions(+), 55 deletions(-)
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index d09429d9f0fb..463c05c94b07 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -49,4 +49,5 @@ [Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
index 32a10e99c3e0..4d6764b702f4 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -39,6 +39,3 @@ [LibraryClasses]
DebugLib
DevicePathLib
MemoryAllocationLib
-
-[Pcd]
- gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
index f02cec812a50..13c591641d7a 100644
--- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -20,49 +20,57 @@
/**
Utility function to initialize a PCI_ROOT_BRIDGE structure.
- @param[in] Supports Supported attributes.
+ @param[in] Supports Supported attributes.
- @param[in] Attributes Initial attributes.
+ @param[in] Attributes Initial attributes.
- @param[in] AllocAttributes Allocation attributes.
+ @param[in] AllocAttributes Allocation attributes.
- @param[in] RootBusNumber The bus number to store in RootBus.
+ @param[in] DmaAbove4G DMA above 4GB memory.
- @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
- assigned to any subordinate bus found behind any
- PCI bridge hanging off this root bus.
+ @param[in] NoExtendedConfigSpace No Extended Config Space.
- The caller is repsonsible for ensuring that
- RootBusNumber <= MaxSubBusNumber. If
- RootBusNumber equals MaxSubBusNumber, then the
- root bus has no room for subordinate buses.
+ @param[in] RootBusNumber The bus number to store in RootBus.
- @param[in] Io IO aperture.
+ @param[in] MaxSubBusNumber The inclusive maximum bus number that can
+ be assigned to any subordinate bus found
+ behind any PCI bridge hanging off this
+ root bus.
- @param[in] Mem MMIO aperture.
+ The caller is repsonsible for ensuring
+ that RootBusNumber <= MaxSubBusNumber. If
+ RootBusNumber equals MaxSubBusNumber, then
+ the root bus has no room for subordinate
+ buses.
- @param[in] MemAbove4G MMIO aperture above 4G.
+ @param[in] Io IO aperture.
- @param[in] PMem Prefetchable MMIO aperture.
+ @param[in] Mem MMIO aperture.
- @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+ @param[in] MemAbove4G MMIO aperture above 4G.
- @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
- caller) that should be filled in by this
- function.
+ @param[in] PMem Prefetchable MMIO aperture.
- @retval EFI_SUCCESS Initialization successful. A device path
- consisting of an ACPI device path node, with
- UID = RootBusNumber, has been allocated and
- linked into RootBus.
+ @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
- @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+ @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated
+ by the caller) that should be filled in by
+ this function.
+
+ @retval EFI_SUCCESS Initialization successful. A device path
+ consisting of an ACPI device path node,
+ with UID = RootBusNumber, has been
+ allocated and linked into RootBus.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
**/
EFI_STATUS
PciHostBridgeUtilityInitRootBridge (
IN UINT64 Supports,
IN UINT64 Attributes,
IN UINT64 AllocAttributes,
+ IN BOOLEAN DmaAbove4G,
+ IN BOOLEAN NoExtendedConfigSpace,
IN UINT8 RootBusNumber,
IN UINT8 MaxSubBusNumber,
IN PCI_ROOT_BRIDGE_APERTURE *Io,
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index f284244448bb..d9c057021598 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -10,6 +10,7 @@
#include <PiDxe.h>
#include <IndustryStandard/Pci.h>
+#include <IndustryStandard/Q35MchIch9.h>
#include <Protocol/PciHostBridgeResourceAllocation.h>
#include <Protocol/PciRootBridgeIo.h>
@@ -150,6 +151,8 @@ PciHostBridgeGetRootBridges (
Attributes,
Attributes,
AllocationAttributes,
+ FALSE,
+ PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
(UINT8) LastRootBridgeNumber,
(UINT8) (RootBridgeNumber - 1),
&Io,
@@ -175,6 +178,8 @@ PciHostBridgeGetRootBridges (
Attributes,
Attributes,
AllocationAttributes,
+ FALSE,
+ PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
(UINT8) LastRootBridgeNumber,
PCI_MAX_BUS,
&Io,
diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 4bd27211ec37..252e30fb06d2 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -455,6 +455,7 @@ ScanForRootBridges (
ASSERT (RootBridges != NULL);
PciHostBridgeUtilityInitRootBridge (
Attributes, Attributes, 0,
+ FALSE, PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
(UINT8) PrimaryBus, (UINT8) SubBus,
&Io, &Mem, &MemAbove4G, &mNonExistAperture, &mNonExistAperture,
&RootBridges[*NumberOfRootBridges]
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
index e3db4b766181..9101aedc8888 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -11,7 +11,6 @@
**/
#include <IndustryStandard/Acpi10.h>
-#include <IndustryStandard/Q35MchIch9.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
@@ -63,49 +62,57 @@ PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePath = {
/**
Utility function to initialize a PCI_ROOT_BRIDGE structure.
- @param[in] Supports Supported attributes.
+ @param[in] Supports Supported attributes.
- @param[in] Attributes Initial attributes.
+ @param[in] Attributes Initial attributes.
- @param[in] AllocAttributes Allocation attributes.
+ @param[in] AllocAttributes Allocation attributes.
- @param[in] RootBusNumber The bus number to store in RootBus.
+ @param[in] DmaAbove4G DMA above 4GB memory.
- @param[in] MaxSubBusNumber The inclusive maximum bus number that can be
- assigned to any subordinate bus found behind any
- PCI bridge hanging off this root bus.
+ @param[in] NoExtendedConfigSpace No Extended Config Space.
- The caller is repsonsible for ensuring that
- RootBusNumber <= MaxSubBusNumber. If
- RootBusNumber equals MaxSubBusNumber, then the
- root bus has no room for subordinate buses.
+ @param[in] RootBusNumber The bus number to store in RootBus.
- @param[in] Io IO aperture.
+ @param[in] MaxSubBusNumber The inclusive maximum bus number that can
+ be assigned to any subordinate bus found
+ behind any PCI bridge hanging off this
+ root bus.
- @param[in] Mem MMIO aperture.
+ The caller is repsonsible for ensuring
+ that RootBusNumber <= MaxSubBusNumber. If
+ RootBusNumber equals MaxSubBusNumber, then
+ the root bus has no room for subordinate
+ buses.
- @param[in] MemAbove4G MMIO aperture above 4G.
+ @param[in] Io IO aperture.
- @param[in] PMem Prefetchable MMIO aperture.
+ @param[in] Mem MMIO aperture.
- @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+ @param[in] MemAbove4G MMIO aperture above 4G.
- @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated by the
- caller) that should be filled in by this
- function.
+ @param[in] PMem Prefetchable MMIO aperture.
- @retval EFI_SUCCESS Initialization successful. A device path
- consisting of an ACPI device path node, with
- UID = RootBusNumber, has been allocated and
- linked into RootBus.
+ @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
- @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+ @param[out] RootBus The PCI_ROOT_BRIDGE structure (allocated
+ by the caller) that should be filled in by
+ this function.
+
+ @retval EFI_SUCCESS Initialization successful. A device path
+ consisting of an ACPI device path node,
+ with UID = RootBusNumber, has been
+ allocated and linked into RootBus.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
**/
EFI_STATUS
PciHostBridgeUtilityInitRootBridge (
IN UINT64 Supports,
IN UINT64 Attributes,
IN UINT64 AllocAttributes,
+ IN BOOLEAN DmaAbove4G,
+ IN BOOLEAN NoExtendedConfigSpace,
IN UINT8 RootBusNumber,
IN UINT8 MaxSubBusNumber,
IN PCI_ROOT_BRIDGE_APERTURE *Io,
@@ -128,7 +135,7 @@ PciHostBridgeUtilityInitRootBridge (
RootBus->Supports = Supports;
RootBus->Attributes = Attributes;
- RootBus->DmaAbove4G = FALSE;
+ RootBus->DmaAbove4G = DmaAbove4G;
RootBus->AllocationAttributes = AllocAttributes;
RootBus->Bus.Base = RootBusNumber;
@@ -139,8 +146,7 @@ PciHostBridgeUtilityInitRootBridge (
CopyMem (&RootBus->PMem, PMem, sizeof (*PMem));
CopyMem (&RootBus->PMemAbove4G, PMemAbove4G, sizeof (*PMemAbove4G));
- RootBus->NoExtendedConfigSpace = (PcdGet16 (PcdOvmfHostBridgePciDevId) !=
- INTEL_Q35_MCH_DEVICE_ID);
+ RootBus->NoExtendedConfigSpace = NoExtendedConfigSpace;
DevicePath = AllocateCopyPool (sizeof mRootBridgeDevicePath,
&mRootBridgeDevicePath);
--
2.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge
2021-01-12 9:45 ` [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge Jiahui Cen
@ 2021-01-13 1:51 ` Laszlo Ersek
2021-01-13 6:01 ` Jiahui Cen
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 1: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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Extend parameter list of PciHostBridgeUtilityInitRootBridge() with
> DmaAbove4G and NoExtendedConfigSpace to prepare for support of ArmVirtPkg.
>
> 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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 -
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 56 ++++++++++--------
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 5 ++
> OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 1 +
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 62 +++++++++++---------
> 6 files changed, 73 insertions(+), 55 deletions(-)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index d09429d9f0fb..463c05c94b07 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -49,4 +49,5 @@ [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> index 32a10e99c3e0..4d6764b702f4 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -39,6 +39,3 @@ [LibraryClasses]
> DebugLib
> DevicePathLib
> MemoryAllocationLib
> -
> -[Pcd]
> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
(1) According to my feedback under patch#3 in this series, I think the
present patch will have to undo the PcdLib dependency as well(both INF
file and #include).
Please extend this patch accordingly; I'm ready to R-b then.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge
2021-01-13 1:51 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-13 6:01 ` Jiahui Cen
0 siblings, 0 replies; 35+ messages in thread
From: Jiahui Cen @ 2021-01-13 6:01 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/13 9:51, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>> Extend parameter list of PciHostBridgeUtilityInitRootBridge() with
>> DmaAbove4G and NoExtendedConfigSpace to prepare for support of ArmVirtPkg.
>>
>> 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: 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/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 -
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 56 ++++++++++--------
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 5 ++
>> OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 1 +
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 62 +++++++++++---------
>> 6 files changed, 73 insertions(+), 55 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> index d09429d9f0fb..463c05c94b07 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> @@ -49,4 +49,5 @@ [Pcd]
>> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
>> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
>> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> index 32a10e99c3e0..4d6764b702f4 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> @@ -39,6 +39,3 @@ [LibraryClasses]
>> DebugLib
>> DevicePathLib
>> MemoryAllocationLib
>> -
>> -[Pcd]
>> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>
> (1) According to my feedback under patch#3 in this series, I think the
> present patch will have to undo the PcdLib dependency as well(both INF
> file and #include).
>
Will fix it.
Thanks,
Jiahui
> Please extend this patch accordingly; I'm ready to R-b then.
>
> Thanks
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge()
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (3 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-13 2:15 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges Jiahui Cen
` (4 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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
Rebase ArmVirtPkg/FdtPciHostBridgeLib to the new
PciHostBridgeUtilityInitRootBridge() / PciHostBridgeUtilityUninitRootBridge()
functions.
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.inf | 1 +
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 124 ++++++++++----------
2 files changed, 61 insertions(+), 64 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
index 01d39626d14c..b813a0851d2a 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
@@ -34,6 +34,7 @@ [Packages]
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseMemoryLib
DebugLib
DevicePathLib
DxeServicesTableLib
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index d554479bf0de..95166d18c82d 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>
@@ -20,37 +21,6 @@
#include <Protocol/PciRootBridgeIo.h>
#include <Protocol/PciHostBridgeResourceAllocation.h>
-#pragma pack(1)
-typedef struct {
- ACPI_HID_DEVICE_PATH AcpiDevicePath;
- EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
-} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
-#pragma pack ()
-
-STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
- {
- {
- ACPI_DEVICE_PATH,
- ACPI_DP,
- {
- (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
- (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
- }
- },
- EISA_PNP_ID(0x0A03),
- 0
- },
-
- {
- END_DEVICE_PATH_TYPE,
- END_ENTIRE_DEVICE_PATH_SUBTYPE,
- {
- END_DEVICE_PATH_LENGTH,
- 0
- }
- }
-};
-
//
// We expect the "ranges" property of "pci-host-ecam-generic" to consist of
// records like this.
@@ -319,11 +289,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 +318,29 @@ PciHostBridgeGetRootBridges (
return NULL;
}
- *Count = 1;
+ ZeroMem (&Io, sizeof (Io));
+ ZeroMem (&Mem, sizeof (Mem));
+ ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
+ ZeroMem (&PMem, sizeof (PMem));
+ ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
- 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,19 +349,41 @@ 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;
+ Status = PciHostBridgeUtilityInitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ TRUE,
+ FALSE,
+ BusMin,
+ BusMax,
+ &Io,
+ &Mem,
+ &MemAbove4G,
+ &PMem,
+ &PMemAbove4G,
+ &mRootBridge
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
+ __FUNCTION__, Status));
+ *Count = 0;
+ return NULL;
+ }
+
+ *Count = 1;
return &mRootBridge;
}
@@ -408,6 +403,7 @@ PciHostBridgeFreeRootBridges (
)
{
ASSERT (Count == 1);
+ PciHostBridgeUtilityUninitRootBridge (Bridges);
}
/**
--
2.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge()
2021-01-12 9:45 ` [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() Jiahui Cen
@ 2021-01-13 2:15 ` Laszlo Ersek
2021-01-13 6:10 ` Jiahui Cen
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 2:15 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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Rebase ArmVirtPkg/FdtPciHostBridgeLib to the new
> PciHostBridgeUtilityInitRootBridge() / PciHostBridgeUtilityUninitRootBridge()
> functions.
>
> 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.inf | 1 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 124 ++++++++++----------
> 2 files changed, 61 insertions(+), 64 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> index 01d39626d14c..b813a0851d2a 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> @@ -34,6 +34,7 @@ [Packages]
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + BaseMemoryLib
> DebugLib
> DevicePathLib
> DxeServicesTableLib
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index d554479bf0de..95166d18c82d 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>
> @@ -20,37 +21,6 @@
> #include <Protocol/PciRootBridgeIo.h>
> #include <Protocol/PciHostBridgeResourceAllocation.h>
>
> -#pragma pack(1)
> -typedef struct {
> - ACPI_HID_DEVICE_PATH AcpiDevicePath;
> - EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
> -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
> -#pragma pack ()
> -
> -STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
> - {
> - {
> - ACPI_DEVICE_PATH,
> - ACPI_DP,
> - {
> - (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
> - (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
> - }
> - },
> - EISA_PNP_ID(0x0A03),
> - 0
> - },
> -
> - {
> - END_DEVICE_PATH_TYPE,
> - END_ENTIRE_DEVICE_PATH_SUBTYPE,
> - {
> - END_DEVICE_PATH_LENGTH,
> - 0
> - }
> - }
> -};
> -
> //
> // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
> // records like this.
> @@ -319,11 +289,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 +318,29 @@ PciHostBridgeGetRootBridges (
> return NULL;
> }
>
> - *Count = 1;
> + ZeroMem (&Io, sizeof (Io));
> + ZeroMem (&Mem, sizeof (Mem));
> + ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
> + ZeroMem (&PMem, sizeof (PMem));
> + ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
>
> - 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,19 +349,41 @@ 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;
> + Status = PciHostBridgeUtilityInitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + TRUE,
> + FALSE,
> + BusMin,
> + BusMax,
> + &Io,
> + &Mem,
> + &MemAbove4G,
> + &PMem,
> + &PMemAbove4G,
> + &mRootBridge
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
> + __FUNCTION__, Status));
(1) EFI_D_ERROR will fail "BaseTools/Scripts/PatchCheck.py":
> Code format is not valid:
> * EFI_D_ERROR was used, but DEBUG_ERROR is now recommended
> File: ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> Line: DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
There are some other syntactic issues with this patch set that
"PatchCheck.py" catches.
It's simplest if you open a pull request with your topic branch, about
to be submitted to the list, on github.com. The PR will be auto-closed,
but it will run CI on your patch series first, and you will get a chance
to fix issues before the on-list review. The CI tasks include
"PatchCheck".
You can also run "BaseTools/Scripts/PatchCheck.py" locally. (You can run
more or less the whole CI content locally, but setting that up is not
quick.)
With the PatchCheck errors addressed, this patch will be OK.
Thanks,
Laszlo
> + *Count = 0;
> + return NULL;
> + }
> +
> + *Count = 1;
>
> return &mRootBridge;
> }
> @@ -408,6 +403,7 @@ PciHostBridgeFreeRootBridges (
> )
> {
> ASSERT (Count == 1);
> + PciHostBridgeUtilityUninitRootBridge (Bridges);
> }
>
> /**
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge()
2021-01-13 2:15 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-13 6:10 ` Jiahui Cen
2021-01-13 9:05 ` Laszlo Ersek
0 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-13 6:10 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/13 10:15, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>> Rebase ArmVirtPkg/FdtPciHostBridgeLib to the new
>> PciHostBridgeUtilityInitRootBridge() / PciHostBridgeUtilityUninitRootBridge()
>> functions.
>>
>> 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.inf | 1 +
>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 124 ++++++++++----------
>> 2 files changed, 61 insertions(+), 64 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> index 01d39626d14c..b813a0851d2a 100644
>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> @@ -34,6 +34,7 @@ [Packages]
>> OvmfPkg/OvmfPkg.dec
>>
>> [LibraryClasses]
>> + BaseMemoryLib
>> DebugLib
>> DevicePathLib
>> DxeServicesTableLib
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> index d554479bf0de..95166d18c82d 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>
>> @@ -20,37 +21,6 @@
>> #include <Protocol/PciRootBridgeIo.h>
>> #include <Protocol/PciHostBridgeResourceAllocation.h>
>>
>> -#pragma pack(1)
>> -typedef struct {
>> - ACPI_HID_DEVICE_PATH AcpiDevicePath;
>> - EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
>> -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
>> -#pragma pack ()
>> -
>> -STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
>> - {
>> - {
>> - ACPI_DEVICE_PATH,
>> - ACPI_DP,
>> - {
>> - (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
>> - (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
>> - }
>> - },
>> - EISA_PNP_ID(0x0A03),
>> - 0
>> - },
>> -
>> - {
>> - END_DEVICE_PATH_TYPE,
>> - END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> - {
>> - END_DEVICE_PATH_LENGTH,
>> - 0
>> - }
>> - }
>> -};
>> -
>> //
>> // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
>> // records like this.
>> @@ -319,11 +289,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 +318,29 @@ PciHostBridgeGetRootBridges (
>> return NULL;
>> }
>>
>> - *Count = 1;
>> + ZeroMem (&Io, sizeof (Io));
>> + ZeroMem (&Mem, sizeof (Mem));
>> + ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
>> + ZeroMem (&PMem, sizeof (PMem));
>> + ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
>>
>> - 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,19 +349,41 @@ 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;
>> + Status = PciHostBridgeUtilityInitRootBridge (
>> + Attributes,
>> + Attributes,
>> + AllocationAttributes,
>> + TRUE,
>> + FALSE,
>> + BusMin,
>> + BusMax,
>> + &Io,
>> + &Mem,
>> + &MemAbove4G,
>> + &PMem,
>> + &PMemAbove4G,
>> + &mRootBridge
>> + );
>> + if (EFI_ERROR (Status)) {
>> + DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
>> + __FUNCTION__, Status));
>
> (1) EFI_D_ERROR will fail "BaseTools/Scripts/PatchCheck.py":
>
>> Code format is not valid:
>> * EFI_D_ERROR was used, but DEBUG_ERROR is now recommended
>> File: ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> Line: DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
>
Sorry for forgetting to check patches. Will fix it.
BTW, EFI_D_ERROR are also used in the other part of the same
function, do I need to fix them too?
> There are some other syntactic issues with this patch set that
> "PatchCheck.py" catches.
Another issue is that the subject is too long. Will try to shorten it.
Seems patch [8/9] has the same issue, will fix too.
Thanks,
Jiahui
>
> It's simplest if you open a pull request with your topic branch, about
> to be submitted to the list, on github.com. The PR will be auto-closed,
> but it will run CI on your patch series first, and you will get a chance
> to fix issues before the on-list review. The CI tasks include
> "PatchCheck".
>
> You can also run "BaseTools/Scripts/PatchCheck.py" locally. (You can run
> more or less the whole CI content locally, but setting that up is not
> quick.)
>
> With the PatchCheck errors addressed, this patch will be OK.
>
> Thanks,
> Laszlo
>
>
>> + *Count = 0;
>> + return NULL;
>> + }
>> +
>> + *Count = 1;
>>
>> return &mRootBridge;
>> }
>> @@ -408,6 +403,7 @@ PciHostBridgeFreeRootBridges (
>> )
>> {
>> ASSERT (Count == 1);
>> + PciHostBridgeUtilityUninitRootBridge (Bridges);
>> }
>>
>> /**
>>
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge()
2021-01-13 6:10 ` Jiahui Cen
@ 2021-01-13 9:05 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-13 9:05 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 01/13/21 07:10, Jiahui Cen wrote:
> Hi Laszlo,
>
> On 2021/1/13 10:15, Laszlo Ersek wrote:
>> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>>> Rebase ArmVirtPkg/FdtPciHostBridgeLib to the new
>>> PciHostBridgeUtilityInitRootBridge() / PciHostBridgeUtilityUninitRootBridge()
>>> functions.
>>>
>>> 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.inf | 1 +
>>> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 124 ++++++++++----------
>>> 2 files changed, 61 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>>> index 01d39626d14c..b813a0851d2a 100644
>>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>>> @@ -34,6 +34,7 @@ [Packages]
>>> OvmfPkg/OvmfPkg.dec
>>>
>>> [LibraryClasses]
>>> + BaseMemoryLib
>>> DebugLib
>>> DevicePathLib
>>> DxeServicesTableLib
>>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> index d554479bf0de..95166d18c82d 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>
>>> @@ -20,37 +21,6 @@
>>> #include <Protocol/PciRootBridgeIo.h>
>>> #include <Protocol/PciHostBridgeResourceAllocation.h>
>>>
>>> -#pragma pack(1)
>>> -typedef struct {
>>> - ACPI_HID_DEVICE_PATH AcpiDevicePath;
>>> - EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
>>> -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
>>> -#pragma pack ()
>>> -
>>> -STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
>>> - {
>>> - {
>>> - ACPI_DEVICE_PATH,
>>> - ACPI_DP,
>>> - {
>>> - (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
>>> - (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
>>> - }
>>> - },
>>> - EISA_PNP_ID(0x0A03),
>>> - 0
>>> - },
>>> -
>>> - {
>>> - END_DEVICE_PATH_TYPE,
>>> - END_ENTIRE_DEVICE_PATH_SUBTYPE,
>>> - {
>>> - END_DEVICE_PATH_LENGTH,
>>> - 0
>>> - }
>>> - }
>>> -};
>>> -
>>> //
>>> // We expect the "ranges" property of "pci-host-ecam-generic" to consist of
>>> // records like this.
>>> @@ -319,11 +289,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 +318,29 @@ PciHostBridgeGetRootBridges (
>>> return NULL;
>>> }
>>>
>>> - *Count = 1;
>>> + ZeroMem (&Io, sizeof (Io));
>>> + ZeroMem (&Mem, sizeof (Mem));
>>> + ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
>>> + ZeroMem (&PMem, sizeof (PMem));
>>> + ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
>>>
>>> - 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,19 +349,41 @@ 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;
>>> + Status = PciHostBridgeUtilityInitRootBridge (
>>> + Attributes,
>>> + Attributes,
>>> + AllocationAttributes,
>>> + TRUE,
>>> + FALSE,
>>> + BusMin,
>>> + BusMax,
>>> + &Io,
>>> + &Mem,
>>> + &MemAbove4G,
>>> + &PMem,
>>> + &PMemAbove4G,
>>> + &mRootBridge
>>> + );
>>> + if (EFI_ERROR (Status)) {
>>> + DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
>>> + __FUNCTION__, Status));
>>
>> (1) EFI_D_ERROR will fail "BaseTools/Scripts/PatchCheck.py":
>>
>>> Code format is not valid:
>>> * EFI_D_ERROR was used, but DEBUG_ERROR is now recommended
>>> File: ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> Line: DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
>>
>
> Sorry for forgetting to check patches. Will fix it.
> BTW, EFI_D_ERROR are also used in the other part of the same
> function, do I need to fix them too?
No, just please don't introduce new instances of EFI_D_*.
Thanks,
Laszlo
>
>> There are some other syntactic issues with this patch set that
>> "PatchCheck.py" catches.
>
> Another issue is that the subject is too long. Will try to shorten it.
> Seems patch [8/9] has the same issue, will fix too.
>
> Thanks,
> Jiahui
>
>>
>> It's simplest if you open a pull request with your topic branch, about
>> to be submitted to the list, on github.com. The PR will be auto-closed,
>> but it will run CI on your patch series first, and you will get a chance
>> to fix issues before the on-list review. The CI tasks include
>> "PatchCheck".
>>
>> You can also run "BaseTools/Scripts/PatchCheck.py" locally. (You can run
>> more or less the whole CI content locally, but setting that up is not
>> quick.)
>>
>> With the PatchCheck errors addressed, this patch will be OK.
>>
>> Thanks,
>> Laszlo
>>
>>
>>> + *Count = 0;
>>> + return NULL;
>>> + }
>>> +
>>> + *Count = 1;
>>>
>>> return &mRootBridge;
>>> }
>>> @@ -408,6 +403,7 @@ PciHostBridgeFreeRootBridges (
>>> )
>>> {
>>> ASSERT (Count == 1);
>>> + PciHostBridgeUtilityUninitRootBridge (Bridges);
>>> }
>>>
>>> /**
>>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (4 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-14 9:12 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges Jiahui Cen
` (3 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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 PciHostBridgeGetRootBridges() / PciHostBridgeFreeRootBridges() to
PciHostBridgeUtilityLib as common utility functions.
No change of functionality.
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 | 1 -
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 5 +
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 50 +++++
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 136 +-------------
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 193 ++++++++++++++++++++
5 files changed, 252 insertions(+), 133 deletions(-)
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index 463c05c94b07..93ba440c9009 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -40,7 +40,6 @@ [LibraryClasses]
MemoryAllocationLib
PciHostBridgeUtilityLib
PciLib
- QemuFwCfgLib
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
index 4d6764b702f4..4cdf367ffee2 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -39,3 +39,8 @@ [LibraryClasses]
DebugLib
DevicePathLib
MemoryAllocationLib
+ PciLib
+ QemuFwCfgLib
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
index 13c591641d7a..29ea19f2d7e5 100644
--- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -97,6 +97,56 @@ PciHostBridgeUtilityUninitRootBridge (
);
+/**
+ Utility function to return all the root bridge instances in an array.
+
+ @param[out] Count The number of root bridge instances.
+
+ @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
+PciHostBridgeUtilityGetRootBridges (
+ UINTN *Count,
+ 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 free root bridge instances array from
+ PciHostBridgeUtilityGetRootBridges().
+
+ @param[in] Bridges The root bridge instances array.
+ @param[in] Count The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeUtilityFreeRootBridges (
+ PCI_ROOT_BRIDGE *Bridges,
+ UINTN Count
+ );
+
+
/**
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 d9c057021598..0a1d94a29bf3 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -9,9 +9,6 @@
**/
#include <PiDxe.h>
-#include <IndustryStandard/Pci.h>
-#include <IndustryStandard/Q35MchIch9.h>
-
#include <Protocol/PciHostBridgeResourceAllocation.h>
#include <Protocol/PciRootBridgeIo.h>
@@ -20,8 +17,6 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
-#include <Library/PciLib.h>
-#include <Library/QemuFwCfgLib.h>
#include "PciHostBridge.h"
@@ -43,14 +38,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;
@@ -90,121 +77,16 @@ 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 = PciHostBridgeUtilityInitRootBridge (
- Attributes,
- Attributes,
- AllocationAttributes,
- FALSE,
- PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
- (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 = PciHostBridgeUtilityInitRootBridge (
- Attributes,
+ return PciHostBridgeUtilityGetRootBridges (
+ Count,
Attributes,
AllocationAttributes,
- FALSE,
- PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
- (UINT8) LastRootBridgeNumber,
- PCI_MAX_BUS,
&Io,
&Mem,
&MemAbove4G,
&mNonExistAperture,
- &mNonExistAperture,
- &Bridges[Initialized]
+ &mNonExistAperture
);
- if (EFI_ERROR (Status)) {
- goto FreeBridges;
- }
- ++Initialized;
-
- *Count = Initialized;
- return Bridges;
-
-FreeBridges:
- while (Initialized > 0) {
- --Initialized;
- PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]);
- }
-
- FreePool (Bridges);
- return NULL;
}
@@ -222,17 +104,7 @@ PciHostBridgeFreeRootBridges (
UINTN Count
)
{
- if (Bridges == NULL && Count == 0) {
- return;
- }
- ASSERT (Bridges != NULL && Count > 0);
-
- do {
- --Count;
- PciHostBridgeUtilityUninitRootBridge (&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 9101aedc8888..629ebcd7a5be 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -11,11 +11,15 @@
**/
#include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/Pci.h>
+#include <IndustryStandard/Q35MchIch9.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PciHostBridgeUtilityLib.h>
+#include <Library/PciLib.h>
+#include <Library/QemuFwCfgLib.h>
#pragma pack(1)
@@ -182,6 +186,195 @@ PciHostBridgeUtilityUninitRootBridge (
}
+/**
+ Utility function to return all the root bridge instances in an array.
+
+ @param[out] Count The number of root bridge instances.
+
+ @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
+PciHostBridgeUtilityGetRootBridges (
+ UINTN *Count,
+ 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;
+ FIRMWARE_CONFIG_ITEM FwCfgItem;
+ UINTN FwCfgSize;
+ UINT64 ExtraRootBridges;
+ PCI_ROOT_BRIDGE *Bridges;
+ 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 > 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 = PciHostBridgeUtilityInitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ FALSE,
+ PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
+ (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 = PciHostBridgeUtilityInitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ FALSE,
+ PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
+ (UINT8) LastRootBridgeNumber,
+ PCI_MAX_BUS,
+ Io,
+ Mem,
+ MemAbove4G,
+ PMem,
+ PMemAbove4G,
+ &Bridges[Initialized]
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBridges;
+ }
+ ++Initialized;
+
+ *Count = Initialized;
+ return Bridges;
+
+FreeBridges:
+ while (Initialized > 0) {
+ --Initialized;
+ PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]);
+ }
+
+ FreePool (Bridges);
+ return NULL;
+}
+
+
+/**
+ Utility function to free root bridge instances array from
+ PciHostBridgeUtilityGetRootBridges().
+
+ @param[in] Bridges The root bridge instances array.
+ @param[in] Count 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;
+ PciHostBridgeUtilityUninitRootBridge (&Bridges[Count]);
+ } while (Count > 0);
+
+ FreePool (Bridges);
+}
+
+
/**
Utility function to inform the platform that the resource conflict happens.
--
2.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges
2021-01-12 9:45 ` [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges Jiahui Cen
@ 2021-01-14 9:12 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Extract PciHostBridgeGetRootBridges() / PciHostBridgeFreeRootBridges() to
> PciHostBridgeUtilityLib as common utility functions.
>
> No change of functionality.
>
> 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 | 1 -
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 5 +
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 50 +++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 136 +-------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 193 ++++++++++++++++++++
> 5 files changed, 252 insertions(+), 133 deletions(-)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index 463c05c94b07..93ba440c9009 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -40,7 +40,6 @@ [LibraryClasses]
> MemoryAllocationLib
> PciHostBridgeUtilityLib
> PciLib
> - QemuFwCfgLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> index 4d6764b702f4..4cdf367ffee2 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -39,3 +39,8 @@ [LibraryClasses]
> DebugLib
> DevicePathLib
> MemoryAllocationLib
> + PciLib
> + QemuFwCfgLib
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
(1) We'll have to re-add the PcdLib dependency then (#include too).
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> index 13c591641d7a..29ea19f2d7e5 100644
> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -97,6 +97,56 @@ PciHostBridgeUtilityUninitRootBridge (
> );
>
>
> +/**
> + Utility function to return all the root bridge instances in an array.
> +
> + @param[out] Count The number of root bridge instances.
> +
> + @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
> +PciHostBridgeUtilityGetRootBridges (
> + UINTN *Count,
> + 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
> + );
(2) You missed annotating the parameters as IN / OUT (should be in sync
with the @param[...] comments above). Don't forget to sync the C file too.
> +
> +
> +/**
> + Utility function to free root bridge instances array from
> + PciHostBridgeUtilityGetRootBridges().
> +
> + @param[in] Bridges The root bridge instances array.
> + @param[in] Count The count of the array.
> +**/
> +VOID
> +EFIAPI
> +PciHostBridgeUtilityFreeRootBridges (
> + PCI_ROOT_BRIDGE *Bridges,
> + UINTN Count
> + );
> +
(3) You missed annotating the parameters as IN / OUT (should be in sync
with the @param[...] comments above). Don't forget to sync the C file too.
> +
> /**
> 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 d9c057021598..0a1d94a29bf3 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -9,9 +9,6 @@
> **/
> #include <PiDxe.h>
>
> -#include <IndustryStandard/Pci.h>
> -#include <IndustryStandard/Q35MchIch9.h>
> -
> #include <Protocol/PciHostBridgeResourceAllocation.h>
> #include <Protocol/PciRootBridgeIo.h>
>
> @@ -20,8 +17,6 @@
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeLib.h>
> #include <Library/PciHostBridgeUtilityLib.h>
> -#include <Library/PciLib.h>
> -#include <Library/QemuFwCfgLib.h>
> #include "PciHostBridge.h"
>
>
> @@ -43,14 +38,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;
> @@ -90,121 +77,16 @@ PciHostBridgeGetRootBridges (
>
> *Count = 0;
(4) Please move this assignment as well into the extracted function.
Thanks
Laszlo
>
> - //
> - // 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 = PciHostBridgeUtilityInitRootBridge (
> - Attributes,
> - Attributes,
> - AllocationAttributes,
> - FALSE,
> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> - (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 = PciHostBridgeUtilityInitRootBridge (
> - Attributes,
> + return PciHostBridgeUtilityGetRootBridges (
> + Count,
> Attributes,
> AllocationAttributes,
> - FALSE,
> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> - (UINT8) LastRootBridgeNumber,
> - PCI_MAX_BUS,
> &Io,
> &Mem,
> &MemAbove4G,
> &mNonExistAperture,
> - &mNonExistAperture,
> - &Bridges[Initialized]
> + &mNonExistAperture
> );
> - if (EFI_ERROR (Status)) {
> - goto FreeBridges;
> - }
> - ++Initialized;
> -
> - *Count = Initialized;
> - return Bridges;
> -
> -FreeBridges:
> - while (Initialized > 0) {
> - --Initialized;
> - PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]);
> - }
> -
> - FreePool (Bridges);
> - return NULL;
> }
>
>
> @@ -222,17 +104,7 @@ PciHostBridgeFreeRootBridges (
> UINTN Count
> )
> {
> - if (Bridges == NULL && Count == 0) {
> - return;
> - }
> - ASSERT (Bridges != NULL && Count > 0);
> -
> - do {
> - --Count;
> - PciHostBridgeUtilityUninitRootBridge (&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 9101aedc8888..629ebcd7a5be 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -11,11 +11,15 @@
> **/
>
> #include <IndustryStandard/Acpi10.h>
> +#include <IndustryStandard/Pci.h>
> +#include <IndustryStandard/Q35MchIch9.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PciHostBridgeUtilityLib.h>
> +#include <Library/PciLib.h>
> +#include <Library/QemuFwCfgLib.h>
>
>
> #pragma pack(1)
> @@ -182,6 +186,195 @@ PciHostBridgeUtilityUninitRootBridge (
> }
>
>
> +/**
> + Utility function to return all the root bridge instances in an array.
> +
> + @param[out] Count The number of root bridge instances.
> +
> + @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
> +PciHostBridgeUtilityGetRootBridges (
> + UINTN *Count,
> + 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;
> + FIRMWARE_CONFIG_ITEM FwCfgItem;
> + UINTN FwCfgSize;
> + UINT64 ExtraRootBridges;
> + PCI_ROOT_BRIDGE *Bridges;
> + 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 > 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 = PciHostBridgeUtilityInitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + FALSE,
> + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + (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 = PciHostBridgeUtilityInitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + FALSE,
> + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + (UINT8) LastRootBridgeNumber,
> + PCI_MAX_BUS,
> + Io,
> + Mem,
> + MemAbove4G,
> + PMem,
> + PMemAbove4G,
> + &Bridges[Initialized]
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBridges;
> + }
> + ++Initialized;
> +
> + *Count = Initialized;
> + return Bridges;
> +
> +FreeBridges:
> + while (Initialized > 0) {
> + --Initialized;
> + PciHostBridgeUtilityUninitRootBridge (&Bridges[Initialized]);
> + }
> +
> + FreePool (Bridges);
> + return NULL;
> +}
> +
> +
> +/**
> + Utility function to free root bridge instances array from
> + PciHostBridgeUtilityGetRootBridges().
> +
> + @param[in] Bridges The root bridge instances array.
> + @param[in] Count 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;
> + PciHostBridgeUtilityUninitRootBridge (&Bridges[Count]);
> + } while (Count > 0);
> +
> + FreePool (Bridges);
> +}
> +
> +
> /**
> Utility function to inform the platform that the resource conflict happens.
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (5 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-14 10:46 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges() Jiahui Cen
` (2 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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
Extend parameter list of PciHostBridgeUtilityGetRootBridges() with
DmaAbove4G, NoExtendedConfigSpace, BusMin and BusMax to support for
ArmVirtPkg.
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/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 --
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 30 +++++++++----
OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 7 +++
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 47 ++++++++++++--------
4 files changed, 57 insertions(+), 30 deletions(-)
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
index 4cdf367ffee2..83a734c1725e 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
@@ -41,6 +41,3 @@ [LibraryClasses]
MemoryAllocationLib
PciLib
QemuFwCfgLib
-
-[Pcd]
- gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
index 29ea19f2d7e5..ed2b386001e1 100644
--- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
+++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
@@ -100,23 +100,31 @@ PciHostBridgeUtilityUninitRootBridge (
/**
Utility function to return all the root bridge instances in an array.
- @param[out] Count The number of root bridge instances.
+ @param[out] Count The number of root bridge instances.
- @param[in] Attributes Initial attributes.
+ @param[in] Attributes Initial attributes.
- @param[in] AllocAttributes Allocation attributes.
+ @param[in] AllocAttributes Allocation attributes.
- @param[in] Io IO aperture.
+ @param[in] DmaAbove4G DMA above 4GB memory.
- @param[in] Mem MMIO aperture.
+ @param[in] NoExtendedConfigSpace No Extended Config Space.
- @param[in] MemAbove4G MMIO aperture above 4G.
+ @param[in] BusMin Minimum Bus number
- @param[in] PMem Prefetchable MMIO aperture.
+ @param[in] BusMax Maximum Bus number
- @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+ @param[in] Io IO aperture.
- @return All the root bridge instances in an array.
+ @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
@@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
UINTN *Count,
UINT64 Attributes,
UINT64 AllocationAttributes,
+ BOOLEAN DmaAbove4G,
+ BOOLEAN NoExtendedConfigSpace,
+ UINT32 BusMin,
+ UINT32 BusMax,
PCI_ROOT_BRIDGE_APERTURE *Io,
PCI_ROOT_BRIDGE_APERTURE *Mem,
PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index 0a1d94a29bf3..28ad32752cab 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -9,6 +9,9 @@
**/
#include <PiDxe.h>
+#include <IndustryStandard/Pci.h>
+#include <IndustryStandard/Q35MchIch9.h>
+
#include <Protocol/PciHostBridgeResourceAllocation.h>
#include <Protocol/PciRootBridgeIo.h>
@@ -81,6 +84,10 @@ PciHostBridgeGetRootBridges (
Count,
Attributes,
AllocationAttributes,
+ FALSE,
+ PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
+ 0,
+ PCI_MAX_BUS,
&Io,
&Mem,
&MemAbove4G,
diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
index 629ebcd7a5be..fd2f54a139e2 100644
--- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
+++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
@@ -12,7 +12,6 @@
#include <IndustryStandard/Acpi10.h>
#include <IndustryStandard/Pci.h>
-#include <IndustryStandard/Q35MchIch9.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
@@ -189,23 +188,31 @@ PciHostBridgeUtilityUninitRootBridge (
/**
Utility function to return all the root bridge instances in an array.
- @param[out] Count The number of root bridge instances.
+ @param[out] Count The number of root bridge instances.
- @param[in] Attributes Initial attributes.
+ @param[in] Attributes Initial attributes.
- @param[in] AllocAttributes Allocation attributes.
+ @param[in] AllocAttributes Allocation attributes.
- @param[in] Io IO aperture.
+ @param[in] DmaAbove4G DMA above 4GB memory.
- @param[in] Mem MMIO aperture.
+ @param[in] NoExtendedConfigSpace No Extended Config Space.
- @param[in] MemAbove4G MMIO aperture above 4G.
+ @param[in] BusMin Minimum Bus number
- @param[in] PMem Prefetchable MMIO aperture.
+ @param[in] BusMax Maximum Bus number
- @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
+ @param[in] Io IO aperture.
- @return All the root bridge instances in an array.
+ @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
@@ -213,6 +220,10 @@ PciHostBridgeUtilityGetRootBridges (
UINTN *Count,
UINT64 Attributes,
UINT64 AllocationAttributes,
+ BOOLEAN DmaAbove4G,
+ BOOLEAN NoExtendedConfigSpace,
+ UINT32 BusMin,
+ UINT32 BusMax,
PCI_ROOT_BRIDGE_APERTURE *Io,
PCI_ROOT_BRIDGE_APERTURE *Mem,
PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
@@ -240,7 +251,7 @@ PciHostBridgeUtilityGetRootBridges (
QemuFwCfgSelectItem (FwCfgItem);
QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
- if (ExtraRootBridges > PCI_MAX_BUS) {
+ if (ExtraRootBridges > BusMax - BusMin) {
DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
"reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
return NULL;
@@ -262,15 +273,15 @@ PciHostBridgeUtilityGetRootBridges (
//
// The "main" root bus is always there.
//
- LastRootBridgeNumber = 0;
+ 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 = 1;
- RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges;
+ for (RootBridgeNumber = BusMin + 1;
+ RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
++RootBridgeNumber) {
UINTN Device;
@@ -290,8 +301,8 @@ PciHostBridgeUtilityGetRootBridges (
Attributes,
Attributes,
AllocationAttributes,
- FALSE,
- PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
+ DmaAbove4G,
+ NoExtendedConfigSpace,
(UINT8) LastRootBridgeNumber,
(UINT8) (RootBridgeNumber - 1),
Io,
@@ -317,8 +328,8 @@ PciHostBridgeUtilityGetRootBridges (
Attributes,
Attributes,
AllocationAttributes,
- FALSE,
- PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
+ DmaAbove4G,
+ NoExtendedConfigSpace,
(UINT8) LastRootBridgeNumber,
PCI_MAX_BUS,
Io,
--
2.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-12 9:45 ` [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges Jiahui Cen
@ 2021-01-14 10:46 ` Laszlo Ersek
2021-01-14 12:44 ` Jiahui Cen
2021-01-15 7:25 ` Jiahui Cen
0 siblings, 2 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 10:46 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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Extend parameter list of PciHostBridgeUtilityGetRootBridges() with
> DmaAbove4G, NoExtendedConfigSpace, BusMin and BusMax to support for
> ArmVirtPkg.
>
> 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/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 --
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 30 +++++++++----
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 7 +++
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 47 ++++++++++++--------
> 4 files changed, 57 insertions(+), 30 deletions(-)
>
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> index 4cdf367ffee2..83a734c1725e 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -41,6 +41,3 @@ [LibraryClasses]
> MemoryAllocationLib
> PciLib
> QemuFwCfgLib
> -
> -[Pcd]
> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> index 29ea19f2d7e5..ed2b386001e1 100644
> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -100,23 +100,31 @@ PciHostBridgeUtilityUninitRootBridge (
> /**
> Utility function to return all the root bridge instances in an array.
>
> - @param[out] Count The number of root bridge instances.
> + @param[out] Count The number of root bridge instances.
>
> - @param[in] Attributes Initial attributes.
> + @param[in] Attributes Initial attributes.
>
> - @param[in] AllocAttributes Allocation attributes.
> + @param[in] AllocAttributes Allocation attributes.
>
> - @param[in] Io IO aperture.
> + @param[in] DmaAbove4G DMA above 4GB memory.
>
> - @param[in] Mem MMIO aperture.
> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>
> - @param[in] MemAbove4G MMIO aperture above 4G.
> + @param[in] BusMin Minimum Bus number
>
> - @param[in] PMem Prefetchable MMIO aperture.
> + @param[in] BusMax Maximum Bus number
>
> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
> + @param[in] Io IO aperture.
>
> - @return All the root bridge instances in an array.
> + @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
> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
> UINTN *Count,
> UINT64 Attributes,
> UINT64 AllocationAttributes,
> + BOOLEAN DmaAbove4G,
> + BOOLEAN NoExtendedConfigSpace,
> + UINT32 BusMin,
> + UINT32 BusMax,
> PCI_ROOT_BRIDGE_APERTURE *Io,
> PCI_ROOT_BRIDGE_APERTURE *Mem,
> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
(1) You forgot to annotate the new params with IN. (Also update the C
file please, in sync.)
(2) The BusMin / BusMax addition must absolutely be a separate patch, so
that we can discuss and review it separately. It's not a simple data
propagation change -- it generalizes the function internally.
(3) BusMax should be documented as an inclusive maximum (but see more on
this below).
(4) I don't understand where the UINT32 type for BusMin / BusMax comes
from. PciHostBridgeUtilityInitRootBridge() takes UINT8 bus numbers
(which makes sense). And the scanning uses UINTN values, see e.g.
RootBridgeNumber, which also makes sense (for convenience). UINT32
matches neither. It's not necessarily wrong, but confusing.
... if you chose UINT32 because ProcessPciHost()
[ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c] outputs
BusMin and BusMax as UINT32, then please use UINTN instead.
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index 0a1d94a29bf3..28ad32752cab 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -9,6 +9,9 @@
> **/
> #include <PiDxe.h>
>
> +#include <IndustryStandard/Pci.h>
> +#include <IndustryStandard/Q35MchIch9.h>
> +
> #include <Protocol/PciHostBridgeResourceAllocation.h>
> #include <Protocol/PciRootBridgeIo.h>
>
> @@ -81,6 +84,10 @@ PciHostBridgeGetRootBridges (
> Count,
> Attributes,
> AllocationAttributes,
> + FALSE,
> + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + 0,
> + PCI_MAX_BUS,
> &Io,
> &Mem,
> &MemAbove4G,
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> index 629ebcd7a5be..fd2f54a139e2 100644
> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -12,7 +12,6 @@
>
> #include <IndustryStandard/Acpi10.h>
> #include <IndustryStandard/Pci.h>
> -#include <IndustryStandard/Q35MchIch9.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> @@ -189,23 +188,31 @@ PciHostBridgeUtilityUninitRootBridge (
> /**
> Utility function to return all the root bridge instances in an array.
>
> - @param[out] Count The number of root bridge instances.
> + @param[out] Count The number of root bridge instances.
>
> - @param[in] Attributes Initial attributes.
> + @param[in] Attributes Initial attributes.
>
> - @param[in] AllocAttributes Allocation attributes.
> + @param[in] AllocAttributes Allocation attributes.
>
> - @param[in] Io IO aperture.
> + @param[in] DmaAbove4G DMA above 4GB memory.
>
> - @param[in] Mem MMIO aperture.
> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>
> - @param[in] MemAbove4G MMIO aperture above 4G.
> + @param[in] BusMin Minimum Bus number
>
> - @param[in] PMem Prefetchable MMIO aperture.
> + @param[in] BusMax Maximum Bus number
>
> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
> + @param[in] Io IO aperture.
>
> - @return All the root bridge instances in an array.
> + @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
> @@ -213,6 +220,10 @@ PciHostBridgeUtilityGetRootBridges (
> UINTN *Count,
> UINT64 Attributes,
> UINT64 AllocationAttributes,
> + BOOLEAN DmaAbove4G,
> + BOOLEAN NoExtendedConfigSpace,
> + UINT32 BusMin,
> + UINT32 BusMax,
> PCI_ROOT_BRIDGE_APERTURE *Io,
> PCI_ROOT_BRIDGE_APERTURE *Mem,
> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
> @@ -240,7 +251,7 @@ PciHostBridgeUtilityGetRootBridges (
> QemuFwCfgSelectItem (FwCfgItem);
> QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
>
> - if (ExtraRootBridges > PCI_MAX_BUS) {
> + if (ExtraRootBridges > BusMax - BusMin) {
> DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
> "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
> return NULL;
(5) The code change looks valid, but please add a comment here (in the
patch dedicated to the bus numbers, that is). Because BusMax is
inclusive, the max bus count is (BusMax - BusMin + 1). From that, the
"main" root bus is is a given, so the max count for the "extra" root
bridges is one less, i.e. (BusMax - BusMin). If the QEMU hint exceeds
that, we have invalid behavior.
(6) In the patch that will deal with the bus numbers exlusively, please
add a sanity check near the top of the function:
BusMin > BusMax || BusMax > PCI_MAX_BUS
If this condition evaluates to TRUE, the function should set (*Count) to
0, and return NULL, at once.
> @@ -262,15 +273,15 @@ PciHostBridgeUtilityGetRootBridges (
> //
> // The "main" root bus is always there.
> //
> - LastRootBridgeNumber = 0;
> + 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 = 1;
> - RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges;
> + for (RootBridgeNumber = BusMin + 1;
> + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
> ++RootBridgeNumber) {
> UINTN Device;
>
> @@ -290,8 +301,8 @@ PciHostBridgeUtilityGetRootBridges (
> Attributes,
> Attributes,
> AllocationAttributes,
> - FALSE,
> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + DmaAbove4G,
> + NoExtendedConfigSpace,
> (UINT8) LastRootBridgeNumber,
> (UINT8) (RootBridgeNumber - 1),
> Io,
> @@ -317,8 +328,8 @@ PciHostBridgeUtilityGetRootBridges (
> Attributes,
> Attributes,
> AllocationAttributes,
> - FALSE,
> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
> + DmaAbove4G,
> + NoExtendedConfigSpace,
> (UINT8) LastRootBridgeNumber,
> PCI_MAX_BUS,
> Io,
>
(7) You missed replacing PCI_MAX_BUS with BusMax here. (But it belongs
in the separate patch that will deal with the bus numbers, and only with
the bus numbers.)
... Which in turn makes me ask you to please test your changes more
carefully. I believe this bug here is actually shown in the firmware
debug log. Namely, the "virt" machine type only supports buses 0x0..0xf,
inclusive (if I remember correctly), because its MMCONFIG space is quite
limited.
Now, assume the common case, with the "virt" machine type, where you
don't add any pxb devices to the QEMU cmdline -- just go with the main
bus (bus#0). With this bug, the "max sub bus number" on bus#0 goes from
0xf to 0xff. I'm fairly sure that change is visible in the messages that
are logged by PciHostBridgeDxe.
You're supposed to regression test the previous use case with the
patched code -- there should be no change in behavior. Comparing the
before-after logs is one of the checks someone should do for this.
... I've found the bus number stuff in this patch so distracting that
I'm actually not capable of reviewing the patch wrt. its "original"
purpose, namely the exposure of the DmaAbove4G and NoExtendedConfigSpace
parameters. I'll do that in v6, once you have split this patch in two.
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-14 10:46 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-14 12:44 ` Jiahui Cen
2021-01-14 16:03 ` Laszlo Ersek
2021-01-15 7:25 ` Jiahui Cen
1 sibling, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-14 12: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/14 18:46, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>> Extend parameter list of PciHostBridgeUtilityGetRootBridges() with
>> DmaAbove4G, NoExtendedConfigSpace, BusMin and BusMax to support for
>> ArmVirtPkg.
>>
>> 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/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 --
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 30 +++++++++----
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 7 +++
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 47 ++++++++++++--------
>> 4 files changed, 57 insertions(+), 30 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> index 4cdf367ffee2..83a734c1725e 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> @@ -41,6 +41,3 @@ [LibraryClasses]
>> MemoryAllocationLib
>> PciLib
>> QemuFwCfgLib
>> -
>> -[Pcd]
>> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> index 29ea19f2d7e5..ed2b386001e1 100644
>> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> @@ -100,23 +100,31 @@ PciHostBridgeUtilityUninitRootBridge (
>> /**
>> Utility function to return all the root bridge instances in an array.
>>
>> - @param[out] Count The number of root bridge instances.
>> + @param[out] Count The number of root bridge instances.
>>
>> - @param[in] Attributes Initial attributes.
>> + @param[in] Attributes Initial attributes.
>>
>> - @param[in] AllocAttributes Allocation attributes.
>> + @param[in] AllocAttributes Allocation attributes.
>>
>> - @param[in] Io IO aperture.
>> + @param[in] DmaAbove4G DMA above 4GB memory.
>>
>> - @param[in] Mem MMIO aperture.
>> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>>
>> - @param[in] MemAbove4G MMIO aperture above 4G.
>> + @param[in] BusMin Minimum Bus number
>>
>> - @param[in] PMem Prefetchable MMIO aperture.
>> + @param[in] BusMax Maximum Bus number
>>
>> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> + @param[in] Io IO aperture.
>>
>> - @return All the root bridge instances in an array.
>> + @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
>> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
>> UINTN *Count,
>> UINT64 Attributes,
>> UINT64 AllocationAttributes,
>> + BOOLEAN DmaAbove4G,
>> + BOOLEAN NoExtendedConfigSpace,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> PCI_ROOT_BRIDGE_APERTURE *Io,
>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>
> (1) You forgot to annotate the new params with IN. (Also update the C
> file please, in sync.)
>
Will add these annotation and also for other patches.
> (2) The BusMin / BusMax addition must absolutely be a separate patch, so
> that we can discuss and review it separately. It's not a simple data
> propagation change -- it generalizes the function internally.
>
> (3) BusMax should be documented as an inclusive maximum (but see more on
> this below).
>
Right, will fix.
> (4) I don't understand where the UINT32 type for BusMin / BusMax comes
> from. PciHostBridgeUtilityInitRootBridge() takes UINT8 bus numbers
> (which makes sense). And the scanning uses UINTN values, see e.g.
> RootBridgeNumber, which also makes sense (for convenience). UINT32
> matches neither. It's not necessarily wrong, but confusing.
>
> ... if you chose UINT32 because ProcessPciHost()
> [ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c] outputs
> BusMin and BusMax as UINT32, then please use UINTN instead.
>
I chose UINT32 because ProcessPciHost() outputs as UINT32 and
PciHostBridgeGetRootBridges() also accepts as UINT32 (in
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c),
and also UINT32 covers UINT8 which comes from OvmfPkg.
Using UINT8 does not fit value passed from ArmVirtPkg, so
as you suggest, UINTN seems a better choice.
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> index 0a1d94a29bf3..28ad32752cab 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> @@ -9,6 +9,9 @@
>> **/
>> #include <PiDxe.h>
>>
>> +#include <IndustryStandard/Pci.h>
>> +#include <IndustryStandard/Q35MchIch9.h>
>> +
>> #include <Protocol/PciHostBridgeResourceAllocation.h>
>> #include <Protocol/PciRootBridgeIo.h>
>>
>> @@ -81,6 +84,10 @@ PciHostBridgeGetRootBridges (
>> Count,
>> Attributes,
>> AllocationAttributes,
>> + FALSE,
>> + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
>> + 0,
>> + PCI_MAX_BUS,
>> &Io,
>> &Mem,
>> &MemAbove4G,
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> index 629ebcd7a5be..fd2f54a139e2 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> @@ -12,7 +12,6 @@
>>
>> #include <IndustryStandard/Acpi10.h>
>> #include <IndustryStandard/Pci.h>
>> -#include <IndustryStandard/Q35MchIch9.h>
>> #include <Library/BaseMemoryLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/DevicePathLib.h>
>> @@ -189,23 +188,31 @@ PciHostBridgeUtilityUninitRootBridge (
>> /**
>> Utility function to return all the root bridge instances in an array.
>>
>> - @param[out] Count The number of root bridge instances.
>> + @param[out] Count The number of root bridge instances.
>>
>> - @param[in] Attributes Initial attributes.
>> + @param[in] Attributes Initial attributes.
>>
>> - @param[in] AllocAttributes Allocation attributes.
>> + @param[in] AllocAttributes Allocation attributes.
>>
>> - @param[in] Io IO aperture.
>> + @param[in] DmaAbove4G DMA above 4GB memory.
>>
>> - @param[in] Mem MMIO aperture.
>> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>>
>> - @param[in] MemAbove4G MMIO aperture above 4G.
>> + @param[in] BusMin Minimum Bus number
>>
>> - @param[in] PMem Prefetchable MMIO aperture.
>> + @param[in] BusMax Maximum Bus number
>>
>> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> + @param[in] Io IO aperture.
>>
>> - @return All the root bridge instances in an array.
>> + @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
>> @@ -213,6 +220,10 @@ PciHostBridgeUtilityGetRootBridges (
>> UINTN *Count,
>> UINT64 Attributes,
>> UINT64 AllocationAttributes,
>> + BOOLEAN DmaAbove4G,
>> + BOOLEAN NoExtendedConfigSpace,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> PCI_ROOT_BRIDGE_APERTURE *Io,
>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>> @@ -240,7 +251,7 @@ PciHostBridgeUtilityGetRootBridges (
>> QemuFwCfgSelectItem (FwCfgItem);
>> QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
>>
>> - if (ExtraRootBridges > PCI_MAX_BUS) {
>> + if (ExtraRootBridges > BusMax - BusMin) {
>> DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
>> "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
>> return NULL;
>
> (5) The code change looks valid, but please add a comment here (in the
> patch dedicated to the bus numbers, that is). Because BusMax is
> inclusive, the max bus count is (BusMax - BusMin + 1). From that, the
> "main" root bus is is a given, so the max count for the "extra" root
> bridges is one less, i.e. (BusMax - BusMin). If the QEMU hint exceeds
> that, we have invalid behavior.
>
> (6) In the patch that will deal with the bus numbers exlusively, please
> add a sanity check near the top of the function:
>
> BusMin > BusMax || BusMax > PCI_MAX_BUS
>
> If this condition evaluates to TRUE, the function should set (*Count) to
> 0, and return NULL, at once.
>
Will add them.
>
>> @@ -262,15 +273,15 @@ PciHostBridgeUtilityGetRootBridges (
>> //
>> // The "main" root bus is always there.
>> //
>> - LastRootBridgeNumber = 0;
>> + 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 = 1;
>> - RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges;
>> + for (RootBridgeNumber = BusMin + 1;
>> + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
>> ++RootBridgeNumber) {
>> UINTN Device;
>>
>> @@ -290,8 +301,8 @@ PciHostBridgeUtilityGetRootBridges (
>> Attributes,
>> Attributes,
>> AllocationAttributes,
>> - FALSE,
>> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
>> + DmaAbove4G,
>> + NoExtendedConfigSpace,
>> (UINT8) LastRootBridgeNumber,
>> (UINT8) (RootBridgeNumber - 1),
>> Io,
>> @@ -317,8 +328,8 @@ PciHostBridgeUtilityGetRootBridges (
>> Attributes,
>> Attributes,
>> AllocationAttributes,
>> - FALSE,
>> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
>> + DmaAbove4G,
>> + NoExtendedConfigSpace,
>> (UINT8) LastRootBridgeNumber,
>> PCI_MAX_BUS,
>> Io,
>>
>
> (7) You missed replacing PCI_MAX_BUS with BusMax here. (But it belongs
> in the separate patch that will deal with the bus numbers, and only with
> the bus numbers.)
>
> ... Which in turn makes me ask you to please test your changes more
> carefully. I believe this bug here is actually shown in the firmware
> debug log. Namely, the "virt" machine type only supports buses 0x0..0xf,
> inclusive (if I remember correctly), because its MMCONFIG space is quite
> limited.
>
I tested on QEMU for x86_64 and aarch64, and it actually worked well so that
I missed this issue. IIUC, for aarch64, the virt machine supports bus range
[0x0, 0xff] by default, and the MMCONFIG space size is 256MB.
> Now, assume the common case, with the "virt" machine type, where you
> don't add any pxb devices to the QEMU cmdline -- just go with the main
> bus (bus#0). With this bug, the "max sub bus number" on bus#0 goes from
> 0xf to 0xff. I'm fairly sure that change is visible in the messages that
> are logged by PciHostBridgeDxe.
>
> You're supposed to regression test the previous use case with the
> patched code -- there should be no change in behavior. Comparing the
> before-after logs is one of the checks someone should do for this.
>
>
> ... I've found the bus number stuff in this patch so distracting that
> I'm actually not capable of reviewing the patch wrt. its "original"
> purpose, namely the exposure of the DmaAbove4G and NoExtendedConfigSpace
> parameters. I'll do that in v6, once you have split this patch in two.
>
Thanks for the review. Will split it.
Thanks,
Jiahui
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-14 12:44 ` Jiahui Cen
@ 2021-01-14 16:03 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 16:03 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 01/14/21 13:44, Jiahui Cen wrote:
> On 2021/1/14 18:46, Laszlo Ersek wrote:
>> (7) You missed replacing PCI_MAX_BUS with BusMax here. (But it belongs
>> in the separate patch that will deal with the bus numbers, and only with
>> the bus numbers.)
>>
>> ... Which in turn makes me ask you to please test your changes more
>> carefully. I believe this bug here is actually shown in the firmware
>> debug log. Namely, the "virt" machine type only supports buses 0x0..0xf,
>> inclusive (if I remember correctly), because its MMCONFIG space is quite
>> limited.
>>
>
> I tested on QEMU for x86_64 and aarch64, and it actually worked well so that
> I missed this issue. IIUC, for aarch64, the virt machine supports bus range
> [0x0, 0xff] by default, and the MMCONFIG space size is 256MB.
Thanks for the reminder -- now I remember this MMCONFIG size depends on
the "highmem" machine type property. Indeed it should be "on" for
aarch64 by default; it's still "off" for 32-bit ARM (and ArmVirtQemu
supports 32-bit ARM as well).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-14 10:46 ` [edk2-devel] " Laszlo Ersek
2021-01-14 12:44 ` Jiahui Cen
@ 2021-01-15 7:25 ` Jiahui Cen
2021-01-15 7:59 ` Laszlo Ersek
1 sibling, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-15 7:25 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/14 18:46, Laszlo Ersek wrote:
>> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
>> UINTN *Count,
>> UINT64 Attributes,
>> UINT64 AllocationAttributes,
>> + BOOLEAN DmaAbove4G,
>> + BOOLEAN NoExtendedConfigSpace,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> PCI_ROOT_BRIDGE_APERTURE *Io,
>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
> (1) You forgot to annotate the new params with IN. (Also update the C
> file please, in sync.)
>
> (2) The BusMin / BusMax addition must absolutely be a separate patch, so
> that we can discuss and review it separately. It's not a simple data
> propagation change -- it generalizes the function internally.
>
> (3) BusMax should be documented as an inclusive maximum (but see more on
> this below).
A little bit confused. IIUC, the original hard-coded bus ranges, from 0 to
PCI_MAX_BUS, are inclusive, as PCI_MAX_BUS = 255. So in my opinion, the
addition of BusMin/BusMax simply extends the parameters, like DmaAbove4G
and NoExtendedConfigSpace, and replaces [0, PCI_MAX_BUS] with [BusMin, BusMax].
Please correct me if I misunderstand.
Does it really generalize the function?
Thanks,
Jiahui
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-15 7:25 ` Jiahui Cen
@ 2021-01-15 7:59 ` Laszlo Ersek
2021-01-15 8:30 ` Jiahui Cen
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-15 7:59 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 01/15/21 08:25, Jiahui Cen wrote:
> Hi Laszlo,
>
> On 2021/1/14 18:46, Laszlo Ersek wrote:
>>> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
>>> UINTN *Count,
>>> UINT64 Attributes,
>>> UINT64 AllocationAttributes,
>>> + BOOLEAN DmaAbove4G,
>>> + BOOLEAN NoExtendedConfigSpace,
>>> + UINT32 BusMin,
>>> + UINT32 BusMax,
>>> PCI_ROOT_BRIDGE_APERTURE *Io,
>>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>> (1) You forgot to annotate the new params with IN. (Also update the C
>> file please, in sync.)
>>
>> (2) The BusMin / BusMax addition must absolutely be a separate patch, so
>> that we can discuss and review it separately. It's not a simple data
>> propagation change -- it generalizes the function internally.
>>
>> (3) BusMax should be documented as an inclusive maximum (but see more on
>> this below).
>
> A little bit confused. IIUC, the original hard-coded bus ranges, from 0 to
> PCI_MAX_BUS, are inclusive, as PCI_MAX_BUS = 255. So in my opinion, the
> addition of BusMin/BusMax simply extends the parameters, like DmaAbove4G
> and NoExtendedConfigSpace, and replaces [0, PCI_MAX_BUS] with [BusMin, BusMax].
> Please correct me if I misunderstand.
>
> Does it really generalize the function?
Yes, I think so. Regarding DmaAbove4G and NoExtendedConfigSpace, you
take those in PciHostBridgeUtilityGetRootBridges(), and simply forward
them to PciHostBridgeUtilityInitRootBridge().
With BusMin / BusMax exposed, you are generalizing the behavior of a
loop, which is tricky, because the actual processing of the ranges is
completed after the loop (with the separate
PciHostBridgeUtilityInitRootBridge() call). In fact, the loop body may
even run zero times (if BusMin==BusMax), and then the only bridge object
is created by the call *after* the loop. All of this requires a lot of
separate thinking.
Before the patch, the hard coded constants are 0, 1, and PCI_MAX_BUS,
and seeing their correctness is a *lot* easier than the parametrized
interval limits. The best evidence for the necessity of separating out
BusMin/BusMax to their own patch is that you missed updating PCI_MAX_BUS
in the PciHostBridgeUtilityInitRootBridge() call after the loop. I think
you failed to realize the path through the code in which the loop body
would be executed zero times. While that is not possible with the
original code, it is definitely possible with the new (parametrized)
code, and it needs additional thought. The code seemed OK there, after
all, but thinking in parameters is always more abstract (hence more
difficult) than thinking in specific constants.
The replacement of [0, PCI_MAX_BUS] with [BusMin, BusMax] is not trivial
at all, as a concept. I had to stop and think for several minutes about
your change in the following condition:
if (ExtraRootBridges > BusMax - BusMin) {
If you think about this BusMin/BusMax change simply as a formal update,
then I believe you are missing part of the picture. I can tell you that
when I originally wrote this code, I absolutely didn't think in terms of
BusMin/BusMax, so this generalization is actually a kind of repurposing,
and it definitely deserves its own 15 minutes of spotlight, so to speak.
This is also why I requested the new sanity checks near the top of the
function:
(BusMin > BusMax || BusMax > PCI_MAX_BUS)
--> FAIL
Obviously this controlling expression evaluates to constant FALSE on the
original code, which is why it doesn't *exist* in the original code
(i.e. why it never occurred to me to express the condition in any shape
or form). You are basically formalizing properties that have
*implicitly* existed in the pre-patch code. This is a bit similar to
theorem proving in mathematics (you recognize emergent properties of a
construct, drag them into the sunlight, and prove them). We need to
concentrate on such thought processes without being disturbed by other
topics.
... Sorry about this wall of text, I really cannot express any better
how big of a difference there is between (a) simple BOOLEAN parameter
forwarding, and (b) turning constants that control a loop, and a
post-loop coda, into caller-controlled parameters.
If it's still hard to accept, please do it just because I'm asking for
it. I'm not asking for it *spuriously*, I promise you that. This review
is difficult for me as well, it's not in my interest to prolong it
needlessly.
Thanks
Laszlo
>
> Thanks,
> Jiahui
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
2021-01-15 7:59 ` Laszlo Ersek
@ 2021-01-15 8:30 ` Jiahui Cen
0 siblings, 0 replies; 35+ messages in thread
From: Jiahui Cen @ 2021-01-15 8:30 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/15 15:59, Laszlo Ersek wrote:
> On 01/15/21 08:25, Jiahui Cen wrote:
>> Hi Laszlo,
>>
>> On 2021/1/14 18:46, Laszlo Ersek wrote:
>>>> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
>>>> UINTN *Count,
>>>> UINT64 Attributes,
>>>> UINT64 AllocationAttributes,
>>>> + BOOLEAN DmaAbove4G,
>>>> + BOOLEAN NoExtendedConfigSpace,
>>>> + UINT32 BusMin,
>>>> + UINT32 BusMax,
>>>> PCI_ROOT_BRIDGE_APERTURE *Io,
>>>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>>>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>>> (1) You forgot to annotate the new params with IN. (Also update the C
>>> file please, in sync.)
>>>
>>> (2) The BusMin / BusMax addition must absolutely be a separate patch, so
>>> that we can discuss and review it separately. It's not a simple data
>>> propagation change -- it generalizes the function internally.
>>>
>>> (3) BusMax should be documented as an inclusive maximum (but see more on
>>> this below).
>>
>> A little bit confused. IIUC, the original hard-coded bus ranges, from 0 to
>> PCI_MAX_BUS, are inclusive, as PCI_MAX_BUS = 255. So in my opinion, the
>> addition of BusMin/BusMax simply extends the parameters, like DmaAbove4G
>> and NoExtendedConfigSpace, and replaces [0, PCI_MAX_BUS] with [BusMin, BusMax].
>> Please correct me if I misunderstand.
>>
>> Does it really generalize the function?
>
> Yes, I think so. Regarding DmaAbove4G and NoExtendedConfigSpace, you
> take those in PciHostBridgeUtilityGetRootBridges(), and simply forward
> them to PciHostBridgeUtilityInitRootBridge().
>
> With BusMin / BusMax exposed, you are generalizing the behavior of a
> loop, which is tricky, because the actual processing of the ranges is
> completed after the loop (with the separate
> PciHostBridgeUtilityInitRootBridge() call). In fact, the loop body may
> even run zero times (if BusMin==BusMax), and then the only bridge object
> is created by the call *after* the loop. All of this requires a lot of
> separate thinking.
>
> Before the patch, the hard coded constants are 0, 1, and PCI_MAX_BUS,
> and seeing their correctness is a *lot* easier than the parametrized
> interval limits. The best evidence for the necessity of separating out
> BusMin/BusMax to their own patch is that you missed updating PCI_MAX_BUS
> in the PciHostBridgeUtilityInitRootBridge() call after the loop. I think
> you failed to realize the path through the code in which the loop body
> would be executed zero times. While that is not possible with the
> original code, it is definitely possible with the new (parametrized)
> code, and it needs additional thought. The code seemed OK there, after
> all, but thinking in parameters is always more abstract (hence more
> difficult) than thinking in specific constants.
>
> The replacement of [0, PCI_MAX_BUS] with [BusMin, BusMax] is not trivial
> at all, as a concept. I had to stop and think for several minutes about
> your change in the following condition:
>
> if (ExtraRootBridges > BusMax - BusMin) {
>
> If you think about this BusMin/BusMax change simply as a formal update,
> then I believe you are missing part of the picture. I can tell you that
> when I originally wrote this code, I absolutely didn't think in terms of
> BusMin/BusMax, so this generalization is actually a kind of repurposing,
> and it definitely deserves its own 15 minutes of spotlight, so to speak.
> This is also why I requested the new sanity checks near the top of the
> function:
>
> (BusMin > BusMax || BusMax > PCI_MAX_BUS)
> --> FAIL
>
> Obviously this controlling expression evaluates to constant FALSE on the
> original code, which is why it doesn't *exist* in the original code
> (i.e. why it never occurred to me to express the condition in any shape
> or form). You are basically formalizing properties that have
> *implicitly* existed in the pre-patch code. This is a bit similar to
> theorem proving in mathematics (you recognize emergent properties of a
> construct, drag them into the sunlight, and prove them). We need to
> concentrate on such thought processes without being disturbed by other
> topics.
>
> ... Sorry about this wall of text, I really cannot express any better
> how big of a difference there is between (a) simple BOOLEAN parameter
> forwarding, and (b) turning constants that control a loop, and a
> post-loop coda, into caller-controlled parameters.
>
Thanks so much for detailed explanation. Now it is much clearer to me.
The addition introduces some new cases that the original hard coded
constants never meet, and makes the patch much more complex. I'm sorry
that I did not notice it before. I'll think more carefully in the future.
> If it's still hard to accept, please do it just because I'm asking for
> it. I'm not asking for it *spuriously*, I promise you that. This review
> is difficult for me as well, it's not in my interest to prolong it
> needlessly.
I will modify the patch as what you point out. Thanks again for the review.
Thanks!
Jiahui
>
> Thanks
> Laszlo
>
>>
>> Thanks,
>> Jiahui
>>
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges()
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (6 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-14 11:01 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 9/9] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
2021-01-14 11:53 ` [PATCH v4 0/9] Add extra pci roots support for Arm Laszlo Ersek
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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
Rebase ArmVirtPkg/FdtPciHostBridgeLib with the new
PciHostBridgeUtilityGetRootBridges() / PciHostBridgeUtilityFreeRootBridges()
functions.
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 | 24 +++++---------------
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 95166d18c82d..89582ef76eb3 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -272,8 +272,6 @@ ProcessPciHost (
return Status;
}
-STATIC PCI_ROOT_BRIDGE mRootBridge;
-
/**
Return all the root bridge instances in an array.
@@ -361,8 +359,10 @@ PciHostBridgeGetRootBridges (
PMemAbove4G.Base = MAX_UINT64;
PMemAbove4G.Limit = 0;
- Status = PciHostBridgeUtilityInitRootBridge (
- Attributes,
+ *Count = 0;
+
+ return PciHostBridgeUtilityGetRootBridges (
+ Count,
Attributes,
AllocationAttributes,
TRUE,
@@ -373,19 +373,8 @@ PciHostBridgeGetRootBridges (
&Mem,
&MemAbove4G,
&PMem,
- &PMemAbove4G,
- &mRootBridge
+ &PMemAbove4G
);
- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
- __FUNCTION__, Status));
- *Count = 0;
- return NULL;
- }
-
- *Count = 1;
-
- return &mRootBridge;
}
/**
@@ -402,8 +391,7 @@ PciHostBridgeFreeRootBridges (
UINTN Count
)
{
- ASSERT (Count == 1);
- PciHostBridgeUtilityUninitRootBridge (Bridges);
+ PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
}
/**
--
2.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges()
2021-01-12 9:45 ` [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges() Jiahui Cen
@ 2021-01-14 11:01 ` Laszlo Ersek
2021-01-14 12:48 ` Jiahui Cen
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 11:01 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 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Rebase ArmVirtPkg/FdtPciHostBridgeLib with the new
> PciHostBridgeUtilityGetRootBridges() / PciHostBridgeUtilityFreeRootBridges()
> functions.
(1) This commit message is utterly useless.
This is the patch where the entire series culminates -- where you
actually bring pxb support to ArmVirtPkg. Do you think you should
perhaps elaborate on the *WHOLE GOAL* of this series, in this commit
message?
>
> 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 | 24 +++++---------------
> 1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 95166d18c82d..89582ef76eb3 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -272,8 +272,6 @@ ProcessPciHost (
> return Status;
> }
>
> -STATIC PCI_ROOT_BRIDGE mRootBridge;
> -
> /**
> Return all the root bridge instances in an array.
>
> @@ -361,8 +359,10 @@ PciHostBridgeGetRootBridges (
> PMemAbove4G.Base = MAX_UINT64;
> PMemAbove4G.Limit = 0;
>
> - Status = PciHostBridgeUtilityInitRootBridge (
> - Attributes,
> + *Count = 0;
(2) As noted earlier, this assignment should be pushed down into
PciHostBridgeUtilityGetRootBridges().
Thanks
Laszlo
> +
> + return PciHostBridgeUtilityGetRootBridges (
> + Count,
> Attributes,
> AllocationAttributes,
> TRUE,
> @@ -373,19 +373,8 @@ PciHostBridgeGetRootBridges (
> &Mem,
> &MemAbove4G,
> &PMem,
> - &PMemAbove4G,
> - &mRootBridge
> + &PMemAbove4G
> );
> - if (EFI_ERROR (Status)) {
> - DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
> - __FUNCTION__, Status));
> - *Count = 0;
> - return NULL;
> - }
> -
> - *Count = 1;
> -
> - return &mRootBridge;
> }
>
> /**
> @@ -402,8 +391,7 @@ PciHostBridgeFreeRootBridges (
> UINTN Count
> )
> {
> - ASSERT (Count == 1);
> - PciHostBridgeUtilityUninitRootBridge (Bridges);
> + PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
> }
>
> /**
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges()
2021-01-14 11:01 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-14 12:48 ` Jiahui Cen
0 siblings, 0 replies; 35+ messages in thread
From: Jiahui Cen @ 2021-01-14 12:48 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/14 19:01, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>> Rebase ArmVirtPkg/FdtPciHostBridgeLib with the new
>> PciHostBridgeUtilityGetRootBridges() / PciHostBridgeUtilityFreeRootBridges()
>> functions.
>
> (1) This commit message is utterly useless.
>
> This is the patch where the entire series culminates -- where you
> actually bring pxb support to ArmVirtPkg. Do you think you should
> perhaps elaborate on the *WHOLE GOAL* of this series, in this commit
> message?
>
Right, I will add some more detailed commit message.
Thanks,
Jiahui
>>
>> 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 | 24 +++++---------------
>> 1 file changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> index 95166d18c82d..89582ef76eb3 100644
>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> @@ -272,8 +272,6 @@ ProcessPciHost (
>> return Status;
>> }
>>
>> -STATIC PCI_ROOT_BRIDGE mRootBridge;
>> -
>> /**
>> Return all the root bridge instances in an array.
>>
>> @@ -361,8 +359,10 @@ PciHostBridgeGetRootBridges (
>> PMemAbove4G.Base = MAX_UINT64;
>> PMemAbove4G.Limit = 0;
>>
>> - Status = PciHostBridgeUtilityInitRootBridge (
>> - Attributes,
>> + *Count = 0;
>
> (2) As noted earlier, this assignment should be pushed down into
> PciHostBridgeUtilityGetRootBridges().
>
> Thanks
> Laszlo
>
>> +
>> + return PciHostBridgeUtilityGetRootBridges (
>> + Count,
>> Attributes,
>> AllocationAttributes,
>> TRUE,
>> @@ -373,19 +373,8 @@ PciHostBridgeGetRootBridges (
>> &Mem,
>> &MemAbove4G,
>> &PMem,
>> - &PMemAbove4G,
>> - &mRootBridge
>> + &PMemAbove4G
>> );
>> - if (EFI_ERROR (Status)) {
>> - DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n",
>> - __FUNCTION__, Status));
>> - *Count = 0;
>> - return NULL;
>> - }
>> -
>> - *Count = 1;
>> -
>> - return &mRootBridge;
>> }
>>
>> /**
>> @@ -402,8 +391,7 @@ PciHostBridgeFreeRootBridges (
>> UINTN Count
>> )
>> {
>> - ASSERT (Count == 1);
>> - PciHostBridgeUtilityUninitRootBridge (Bridges);
>> + PciHostBridgeUtilityFreeRootBridges (Bridges, Count);
>> }
>>
>> /**
>>
>
>
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 9/9] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (7 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges() Jiahui Cen
@ 2021-01-12 9:45 ` Jiahui Cen
2021-01-14 11:04 ` [edk2-devel] " Laszlo Ersek
2021-01-14 11:53 ` [PATCH v4 0/9] Add extra pci roots support for Arm Laszlo Ersek
9 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-12 9:45 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/ArmVirtQemuKernel.dsc | 1 +
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
3 files changed, 3 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/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index c27752b4d5e5..92f327eec4fb 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -424,6 +424,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.29.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH v4 9/9] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug
2021-01-12 9:45 ` [PATCH v4 9/9] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
@ 2021-01-14 11:04 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 11:04 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 01/12/21 10:45, 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/ArmVirtQemuKernel.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
> 3 files changed, 3 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/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index c27752b4d5e5..92f327eec4fb 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -424,6 +424,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
>
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v4 0/9] Add extra pci roots support for Arm
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
` (8 preceding siblings ...)
2021-01-12 9:45 ` [PATCH v4 9/9] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
@ 2021-01-14 11:53 ` Laszlo Ersek
2021-01-14 12:51 ` Jiahui Cen
9 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-14 11:53 UTC (permalink / raw)
To: Jiahui Cen, devel
Cc: Jordan Justen, Ard Biesheuvel, Rebecca Cran, Peter Grehan,
Anthony Perard, Julien Grall, Leif Lindholm, Sami Mujawar,
xieyingtai, wu.wubin, Yubo Miao
On 01/12/21 10:45, Jiahui Cen wrote:
> v3->v4:
> * Refactor InitRootBridges/UninitRootBridges/GetRootBridges/FreeRootBridges.
> * Fix library dependencies.
>
> 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.
>
> v3: https://edk2.groups.io/g/devel/message/69382
> 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>
I've finished reviewing this version; please submit v6.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v4 0/9] Add extra pci roots support for Arm
2021-01-14 11:53 ` [PATCH v4 0/9] Add extra pci roots support for Arm Laszlo Ersek
@ 2021-01-14 12:51 ` Jiahui Cen
2021-01-18 17:26 ` [edk2-devel] " Laszlo Ersek
0 siblings, 1 reply; 35+ messages in thread
From: Jiahui Cen @ 2021-01-14 12:51 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/14 19:53, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen wrote:
>> v3->v4:
>> * Refactor InitRootBridges/UninitRootBridges/GetRootBridges/FreeRootBridges.
>> * Fix library dependencies.
>>
>> 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.
>>
>> v3: https://edk2.groups.io/g/devel/message/69382
>> 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>
>
> I've finished reviewing this version; please submit v6.
>
Thanks for the detailed review. I'll carefully fix them as you points
and submit v6 soon.
Thanks,
Jiahui
> Thanks
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH v4 0/9] Add extra pci roots support for Arm
2021-01-14 12:51 ` Jiahui Cen
@ 2021-01-18 17:26 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-01-18 17:26 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 01/14/21 13:51, Jiahui Cen via groups.io wrote:
> Hi Laszlo,
>
> On 2021/1/14 19:53, Laszlo Ersek wrote:
>> On 01/12/21 10:45, Jiahui Cen wrote:
>>> v3->v4:
>>> * Refactor InitRootBridges/UninitRootBridges/GetRootBridges/FreeRootBridges.
>>> * Fix library dependencies.
>>>
>>> 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.
>>>
>>> v3: https://edk2.groups.io/g/devel/message/69382
>>> 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>
>>
>> I've finished reviewing this version; please submit v6.
>>
>
> Thanks for the detailed review. I'll carefully fix them as you points
> and submit v6 soon.
I'm looking forward to v6; I'd really like to merge that, so I can move
on to reviewing other patches.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread