* [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm.
@ 2020-04-02 12:08 Yubo Miao
2020-04-02 13:52 ` Laszlo Ersek
0 siblings, 1 reply; 5+ messages in thread
From: Yubo Miao @ 2020-04-02 12:08 UTC (permalink / raw)
To: lersek, ard.biesheuvel, leif; +Cc: devel, xiexiangyou, miaoyubo
From: miaoyubo <miaoyubo@huawei.com>
Add support for extra roots for arm, in this patch, we
import the scan for extra root buses therefore the uefi
could recognize multiply roots for arm.
The logic keeps the same with the logic in
"OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c"
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: miaoyubo <miaoyubo@huawei.com>
---
.../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 264 +++++++++++++++---
.../FdtPciHostBridgeLib.inf | 3 +
2 files changed, 230 insertions(+), 37 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 496b192d22..706efeb416 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -14,6 +14,10 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/PciLib.h>
+#include <IndustryStandard/Pci.h>
+#include <Library/BaseMemoryLib.h>
#include <Protocol/FdtClient.h>
#include <Protocol/PciRootBridgeIo.h>
@@ -306,7 +310,70 @@ ProcessPciHost (
return Status;
}
-STATIC PCI_ROOT_BRIDGE mRootBridge;
+EFI_STATUS
+InitRootBridge (
+ IN UINT64 Supports,
+ IN UINT64 Attributes,
+ IN UINT64 AllocAttributes,
+ IN UINT8 RootBusNumber,
+ IN UINT8 MaxSubBusNumber,
+ IN PCI_ROOT_BRIDGE_APERTURE *Io,
+ IN PCI_ROOT_BRIDGE_APERTURE *Mem,
+ IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
+ IN PCI_ROOT_BRIDGE_APERTURE *PMem,
+ IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G,
+ OUT PCI_ROOT_BRIDGE *RootBus
+ )
+{
+ EFI_PCI_ROOT_BRIDGE_DEVICE_PATH *DevicePath;
+
+ //
+ // Be safe if other fields are added to PCI_ROOT_BRIDGE later.
+ //
+ ZeroMem (RootBus, sizeof *RootBus);
+
+ RootBus->Segment = 0;
+ RootBus->ResourceAssigned = FALSE;
+ RootBus->Supports = Supports;
+ RootBus->Attributes = Attributes;
+
+ RootBus->DmaAbove4G = TRUE;
+
+ RootBus->AllocationAttributes = AllocAttributes;
+ RootBus->Bus.Base = RootBusNumber;
+ RootBus->Bus.Limit = MaxSubBusNumber;
+ CopyMem (&RootBus->Io, Io, sizeof (*Io));
+ CopyMem (&RootBus->Mem, Mem, sizeof (*Mem));
+ CopyMem (&RootBus->MemAbove4G, MemAbove4G, sizeof (*MemAbove4G));
+ CopyMem (&RootBus->PMem, PMem, sizeof (*PMem));
+ CopyMem (&RootBus->PMemAbove4G, PMemAbove4G, sizeof (*PMemAbove4G));
+
+ RootBus->NoExtendedConfigSpace = FALSE;
+
+ DevicePath = AllocateCopyPool (sizeof mEfiPciRootBridgeDevicePath,
+ &mEfiPciRootBridgeDevicePath);
+ if (DevicePath == NULL) {
+ DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ DevicePath->AcpiDevicePath.UID = RootBusNumber;
+ RootBus->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)DevicePath;
+
+ DEBUG ((EFI_D_INFO,
+ "%a: populated root bus %d, with room for %d subordinate bus(es)\n",
+ __FUNCTION__, RootBusNumber, MaxSubBusNumber - RootBusNumber));
+ return EFI_SUCCESS;
+}
+
+STATIC
+VOID
+UninitRootBridge (
+ IN PCI_ROOT_BRIDGE *RootBus
+ )
+{
+ FreePool (RootBus->DevicePath);
+}
+
/**
Return all the root bridge instances in an array.
@@ -323,11 +390,25 @@ 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;
+ FIRMWARE_CONFIG_ITEM FwCfgItem;
+ UINTN FwCfgSize;
+ UINT64 ExtraRootBridges;
+ PCI_ROOT_BRIDGE *Bridges;
+ UINTN Initialized;
+ UINTN LastRootBridgeNumber;
+ UINTN RootBridgeNumber;
+ UINT64 Attributes;
+ UINT64 AllocationAttributes;
+ EFI_STATUS Status;
+ PCI_ROOT_BRIDGE_APERTURE Io;
+ PCI_ROOT_BRIDGE_APERTURE PMem;
+ PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
+ PCI_ROOT_BRIDGE_APERTURE Mem;
+ PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
if (PcdGet64 (PcdPciExpressBaseAddress) == 0) {
DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__));
@@ -345,33 +426,27 @@ PciHostBridgeGetRootBridges (
return NULL;
}
- *Count = 1;
+ ZeroMem (&Io, sizeof (Io));
+ ZeroMem (&Mem, sizeof (Mem));
+ ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
- mRootBridge.Segment = 0;
- mRootBridge.Supports = EFI_PCI_ATTRIBUTE_ISA_IO_16 |
- EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
- EFI_PCI_ATTRIBUTE_VGA_IO_16 |
- EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
- mRootBridge.Attributes = mRootBridge.Supports;
+ Attributes = EFI_PCI_ATTRIBUTE_ISA_IO_16 |
+ EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
+ EFI_PCI_ATTRIBUTE_VGA_IO_16 |
+ EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
- mRootBridge.DmaAbove4G = TRUE;
- mRootBridge.NoExtendedConfigSpace = FALSE;
- mRootBridge.ResourceAssigned = FALSE;
+ AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
- mRootBridge.AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
-
- mRootBridge.Bus.Base = BusMin;
- mRootBridge.Bus.Limit = BusMax;
- mRootBridge.Io.Base = IoBase;
- mRootBridge.Io.Limit = IoBase + IoSize - 1;
- mRootBridge.Mem.Base = Mmio32Base;
- mRootBridge.Mem.Limit = Mmio32Base + Mmio32Size - 1;
+ Io.Base = IoBase;
+ Io.Limit = IoBase + IoSize - 1;
+ Mem.Base = Mmio32Base;
+ Mem.Limit = Mmio32Base + Mmio32Size - 1;
if (sizeof (UINTN) == sizeof (UINT64)) {
- mRootBridge.MemAbove4G.Base = Mmio64Base;
- mRootBridge.MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
+ MemAbove4G.Base = Mmio64Base;
+ MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
if (Mmio64Size > 0) {
- mRootBridge.AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
+ AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
}
} else {
//
@@ -380,21 +455,127 @@ 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;
+
+ //
+ // 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);
- mRootBridge.DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath;
+ if (ExtraRootBridges > BusMax) {
+ DEBUG ((EFI_D_ERROR, "%a: invalid count of extra root buses (%Lu) "
+ "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
+ return NULL;
+ }
+ DEBUG ((EFI_D_INFO, "%a: %Lu extra root buses reported by QEMU\n",
+ __FUNCTION__, ExtraRootBridges));
+ }
- return &mRootBridge;
+ //
+ // Allocate the "main" root bridge, and any extra root bridges.
+ //
+ Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges);
+ if (Bridges == NULL) {
+ DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
+ return NULL;
+ }
+ Initialized = 0;
+
+ //
+ // The "main" root bus is always there.
+ //
+ LastRootBridgeNumber = 0;
+ //
+ // Scan all other root buses. If function 0 of any device on a bus returns a
+ // VendorId register value different from all-bits-one, then that bus is
+ // alive.
+ //
+ for (RootBridgeNumber = 1;
+ RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
+ ++RootBridgeNumber) {
+ UINTN Device;
+
+ for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) {
+ if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0,
+ PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) {
+ break;
+ }
+ }
+ if (Device <= PCI_MAX_DEVICE) {
+ //
+ // Found the next root bus. We can now install the *previous* one,
+ // because now we know how big a bus number range *that* one has, for any
+ // subordinate buses that might exist behind PCI bridges hanging off it.
+ //
+ Status = InitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ (UINT8) LastRootBridgeNumber,
+ (UINT8) (RootBridgeNumber - 1),
+ &Io,
+ &Mem,
+ &MemAbove4G,
+ &PMem,
+ &PMemAbove4G,
+ &Bridges[Initialized]
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBridges;
+ }
+ ++Initialized;
+ LastRootBridgeNumber = RootBridgeNumber;
+ }
+ }
+ //
+ // Install the last root bus (which might be the only, ie. main, root bus, if
+ // we've found no extra root buses).
+ //
+ Status = InitRootBridge (
+ Attributes,
+ Attributes,
+ AllocationAttributes,
+ (UINT8) LastRootBridgeNumber,
+ BusMax,
+ &Io,
+ &Mem,
+ &MemAbove4G,
+ &PMem,
+ &PMemAbove4G,
+ &Bridges[Initialized]
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBridges;
+ }
+ ++Initialized;
+
+ *Count = Initialized;
+ return Bridges;
+
+FreeBridges:
+ while (Initialized > 0) {
+ --Initialized;
+ UninitRootBridge (&Bridges[Initialized]);
+ }
+
+ FreePool (Bridges);
+ return NULL;
}
/**
@@ -411,9 +592,18 @@ PciHostBridgeFreeRootBridges (
UINTN Count
)
{
- ASSERT (Count == 1);
-}
+ if (Bridges == NULL && Count == 0) {
+ return;
+ }
+ ASSERT (Bridges != NULL && Count > 0);
+
+ do {
+ --Count;
+ UninitRootBridge (&Bridges[Count]);
+ } while (Count > 0);
+ FreePool (Bridges);
+}
/**
Inform the platform that the resource conflict happens.
diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
index 277ccfd245..3ac58855af 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
@@ -31,11 +31,14 @@
ArmVirtPkg/ArmVirtPkg.dec
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
[LibraryClasses]
DebugLib
DevicePathLib
DxeServicesTableLib
+ BaseMemoryLib
+ QemuFwCfgLib
MemoryAllocationLib
PciPcdProducerLib
--
2.19.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm.
2020-04-02 12:08 [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm Yubo Miao
@ 2020-04-02 13:52 ` Laszlo Ersek
2020-04-03 8:55 ` miaoyubo
2020-04-08 3:58 ` miaoyubo
0 siblings, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-04-02 13:52 UTC (permalink / raw)
To: Yubo Miao, ard.biesheuvel, leif; +Cc: devel, xiexiangyou
Quick review comments only, for now:
On 04/02/20 14:08, Yubo Miao wrote:
> From: miaoyubo <miaoyubo@huawei.com>
>
> Add support for extra roots for arm, in this patch, we
> import the scan for extra root buses therefore the uefi
> could recognize multiply roots for arm.
>
> The logic keeps the same with the logic in
> "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c"
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: miaoyubo <miaoyubo@huawei.com>
> ---
> .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 264 +++++++++++++++---
> .../FdtPciHostBridgeLib.inf | 3 +
> 2 files changed, 230 insertions(+), 37 deletions(-)
(1) The "Contributed-under:" line is no longer necessary (even defined)
in the commit message.
(2) This code is way too large for my taste to duplicate between
ArmVirtPkg and OvmfPkg. I would strongly prefer if we could factor the
logic in OvmfPkg out to a separate library, and use that from both
consumer sites.
(3) Can you please provide a pointer to the QEMU-side work? In
particular, this logic depdens on the "etc/extra-pci-roots" fw_cfg file.
Where is that file being exposed by qemu-system-aarch64 / "virt"? In
general, the firmware code is merged after the QEMU work is merged. Has
the design been accepted for QEMU at least? (So that it make sense for
us to look at the interfacing ArmVirtPkg code.)
(4) Have you tested booting from PCI devices behind the "extra" root
bridges? In particular, have you tested the boot order manipulation via
the "bootindex" device properties? (OvmfPkg/Library/QemuBootOrderLib is
already being used by the ArmVirtQemu platform, so I'd expect no changes
related to boot order filtering / reordering -- but it should be tested.)
(5) I think this feature deserves a TianoCore Feature Request. Can you
please file one at <https://bugzilla.tianocore.org/>? Then the bugzilla
link should be referenced in the commit message.
(Preferably the bugzilla entry should summarize the present QEMU status
too, or provide further links to QEMU discussions etc.)
Thanks,
Laszlo
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 496b192d22..706efeb416 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -14,6 +14,10 @@
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +#include <Library/PciLib.h>
> +#include <IndustryStandard/Pci.h>
> +#include <Library/BaseMemoryLib.h>
>
> #include <Protocol/FdtClient.h>
> #include <Protocol/PciRootBridgeIo.h>
> @@ -306,7 +310,70 @@ ProcessPciHost (
> return Status;
> }
>
> -STATIC PCI_ROOT_BRIDGE mRootBridge;
> +EFI_STATUS
> +InitRootBridge (
> + IN UINT64 Supports,
> + IN UINT64 Attributes,
> + IN UINT64 AllocAttributes,
> + IN UINT8 RootBusNumber,
> + IN UINT8 MaxSubBusNumber,
> + IN PCI_ROOT_BRIDGE_APERTURE *Io,
> + IN PCI_ROOT_BRIDGE_APERTURE *Mem,
> + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
> + IN PCI_ROOT_BRIDGE_APERTURE *PMem,
> + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G,
> + OUT PCI_ROOT_BRIDGE *RootBus
> + )
> +{
> + EFI_PCI_ROOT_BRIDGE_DEVICE_PATH *DevicePath;
> +
> + //
> + // Be safe if other fields are added to PCI_ROOT_BRIDGE later.
> + //
> + ZeroMem (RootBus, sizeof *RootBus);
> +
> + RootBus->Segment = 0;
> + RootBus->ResourceAssigned = FALSE;
> + RootBus->Supports = Supports;
> + RootBus->Attributes = Attributes;
> +
> + RootBus->DmaAbove4G = TRUE;
> +
> + RootBus->AllocationAttributes = AllocAttributes;
> + RootBus->Bus.Base = RootBusNumber;
> + RootBus->Bus.Limit = MaxSubBusNumber;
> + CopyMem (&RootBus->Io, Io, sizeof (*Io));
> + CopyMem (&RootBus->Mem, Mem, sizeof (*Mem));
> + CopyMem (&RootBus->MemAbove4G, MemAbove4G, sizeof (*MemAbove4G));
> + CopyMem (&RootBus->PMem, PMem, sizeof (*PMem));
> + CopyMem (&RootBus->PMemAbove4G, PMemAbove4G, sizeof (*PMemAbove4G));
> +
> + RootBus->NoExtendedConfigSpace = FALSE;
> +
> + DevicePath = AllocateCopyPool (sizeof mEfiPciRootBridgeDevicePath,
> + &mEfiPciRootBridgeDevicePath);
> + if (DevicePath == NULL) {
> + DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
> + return EFI_OUT_OF_RESOURCES;
> + }
> + DevicePath->AcpiDevicePath.UID = RootBusNumber;
> + RootBus->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)DevicePath;
> +
> + DEBUG ((EFI_D_INFO,
> + "%a: populated root bus %d, with room for %d subordinate bus(es)\n",
> + __FUNCTION__, RootBusNumber, MaxSubBusNumber - RootBusNumber));
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +VOID
> +UninitRootBridge (
> + IN PCI_ROOT_BRIDGE *RootBus
> + )
> +{
> + FreePool (RootBus->DevicePath);
> +}
> +
>
> /**
> Return all the root bridge instances in an array.
> @@ -323,11 +390,25 @@ 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;
> + FIRMWARE_CONFIG_ITEM FwCfgItem;
> + UINTN FwCfgSize;
> + UINT64 ExtraRootBridges;
> + PCI_ROOT_BRIDGE *Bridges;
> + UINTN Initialized;
> + UINTN LastRootBridgeNumber;
> + UINTN RootBridgeNumber;
> + UINT64 Attributes;
> + UINT64 AllocationAttributes;
> + EFI_STATUS Status;
> + PCI_ROOT_BRIDGE_APERTURE Io;
> + PCI_ROOT_BRIDGE_APERTURE PMem;
> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
> + PCI_ROOT_BRIDGE_APERTURE Mem;
> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
>
> if (PcdGet64 (PcdPciExpressBaseAddress) == 0) {
> DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__));
> @@ -345,33 +426,27 @@ PciHostBridgeGetRootBridges (
> return NULL;
> }
>
> - *Count = 1;
> + ZeroMem (&Io, sizeof (Io));
> + ZeroMem (&Mem, sizeof (Mem));
> + ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
>
> - mRootBridge.Segment = 0;
> - mRootBridge.Supports = EFI_PCI_ATTRIBUTE_ISA_IO_16 |
> - EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
> - EFI_PCI_ATTRIBUTE_VGA_IO_16 |
> - EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
> - mRootBridge.Attributes = mRootBridge.Supports;
> + Attributes = EFI_PCI_ATTRIBUTE_ISA_IO_16 |
> + EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
> + EFI_PCI_ATTRIBUTE_VGA_IO_16 |
> + EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
>
> - mRootBridge.DmaAbove4G = TRUE;
> - mRootBridge.NoExtendedConfigSpace = FALSE;
> - mRootBridge.ResourceAssigned = FALSE;
> + AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
>
> - mRootBridge.AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM;
> -
> - mRootBridge.Bus.Base = BusMin;
> - mRootBridge.Bus.Limit = BusMax;
> - mRootBridge.Io.Base = IoBase;
> - mRootBridge.Io.Limit = IoBase + IoSize - 1;
> - mRootBridge.Mem.Base = Mmio32Base;
> - mRootBridge.Mem.Limit = Mmio32Base + Mmio32Size - 1;
> + Io.Base = IoBase;
> + Io.Limit = IoBase + IoSize - 1;
> + Mem.Base = Mmio32Base;
> + Mem.Limit = Mmio32Base + Mmio32Size - 1;
>
> if (sizeof (UINTN) == sizeof (UINT64)) {
> - mRootBridge.MemAbove4G.Base = Mmio64Base;
> - mRootBridge.MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
> + MemAbove4G.Base = Mmio64Base;
> + MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1;
> if (Mmio64Size > 0) {
> - mRootBridge.AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
> + AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
> }
> } else {
> //
> @@ -380,21 +455,127 @@ 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;
> +
> + //
> + // 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);
>
> - mRootBridge.DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath;
> + if (ExtraRootBridges > BusMax) {
> + DEBUG ((EFI_D_ERROR, "%a: invalid count of extra root buses (%Lu) "
> + "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
> + return NULL;
> + }
> + DEBUG ((EFI_D_INFO, "%a: %Lu extra root buses reported by QEMU\n",
> + __FUNCTION__, ExtraRootBridges));
> + }
>
> - return &mRootBridge;
> + //
> + // Allocate the "main" root bridge, and any extra root bridges.
> + //
> + Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges);
> + if (Bridges == NULL) {
> + DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES));
> + return NULL;
> + }
> + Initialized = 0;
> +
> + //
> + // The "main" root bus is always there.
> + //
> + LastRootBridgeNumber = 0;
> + //
> + // Scan all other root buses. If function 0 of any device on a bus returns a
> + // VendorId register value different from all-bits-one, then that bus is
> + // alive.
> + //
> + for (RootBridgeNumber = 1;
> + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
> + ++RootBridgeNumber) {
> + UINTN Device;
> +
> + for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) {
> + if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0,
> + PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) {
> + break;
> + }
> + }
> + if (Device <= PCI_MAX_DEVICE) {
> + //
> + // Found the next root bus. We can now install the *previous* one,
> + // because now we know how big a bus number range *that* one has, for any
> + // subordinate buses that might exist behind PCI bridges hanging off it.
> + //
> + Status = InitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + (UINT8) LastRootBridgeNumber,
> + (UINT8) (RootBridgeNumber - 1),
> + &Io,
> + &Mem,
> + &MemAbove4G,
> + &PMem,
> + &PMemAbove4G,
> + &Bridges[Initialized]
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBridges;
> + }
> + ++Initialized;
> + LastRootBridgeNumber = RootBridgeNumber;
> + }
> + }
> + //
> + // Install the last root bus (which might be the only, ie. main, root bus, if
> + // we've found no extra root buses).
> + //
> + Status = InitRootBridge (
> + Attributes,
> + Attributes,
> + AllocationAttributes,
> + (UINT8) LastRootBridgeNumber,
> + BusMax,
> + &Io,
> + &Mem,
> + &MemAbove4G,
> + &PMem,
> + &PMemAbove4G,
> + &Bridges[Initialized]
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBridges;
> + }
> + ++Initialized;
> +
> + *Count = Initialized;
> + return Bridges;
> +
> +FreeBridges:
> + while (Initialized > 0) {
> + --Initialized;
> + UninitRootBridge (&Bridges[Initialized]);
> + }
> +
> + FreePool (Bridges);
> + return NULL;
> }
>
> /**
> @@ -411,9 +592,18 @@ PciHostBridgeFreeRootBridges (
> UINTN Count
> )
> {
> - ASSERT (Count == 1);
> -}
> + if (Bridges == NULL && Count == 0) {
> + return;
> + }
> + ASSERT (Bridges != NULL && Count > 0);
> +
> + do {
> + --Count;
> + UninitRootBridge (&Bridges[Count]);
> + } while (Count > 0);
>
> + FreePool (Bridges);
> +}
> /**
> Inform the platform that the resource conflict happens.
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> index 277ccfd245..3ac58855af 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> @@ -31,11 +31,14 @@
> ArmVirtPkg/ArmVirtPkg.dec
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> DebugLib
> DevicePathLib
> DxeServicesTableLib
> + BaseMemoryLib
> + QemuFwCfgLib
> MemoryAllocationLib
> PciPcdProducerLib
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm.
2020-04-02 13:52 ` Laszlo Ersek
@ 2020-04-03 8:55 ` miaoyubo
2020-04-08 3:58 ` miaoyubo
1 sibling, 0 replies; 5+ messages in thread
From: miaoyubo @ 2020-04-03 8:55 UTC (permalink / raw)
To: Laszlo Ersek, ard.biesheuvel@linaro.org, leif@nuviainc.com
Cc: devel@edk2.groups.io, Xiexiangyou
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, April 2, 2020 9:52 PM
> To: miaoyubo <miaoyubo@huawei.com>; ard.biesheuvel@linaro.org;
> leif@nuviainc.com
> Cc: devel@edk2.groups.io; Xiexiangyou <xiexiangyou@huawei.com>
> Subject: Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for
> Arm.
>
> Quick review comments only, for now:
>
> On 04/02/20 14:08, Yubo Miao wrote:
> > From: miaoyubo <miaoyubo@huawei.com>
> >
> > Add support for extra roots for arm, in this patch, we import the scan
> > for extra root buses therefore the uefi could recognize multiply roots
> > for arm.
> >
> > The logic keeps the same with the logic in
> > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c"
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: miaoyubo <miaoyubo@huawei.com>
> > ---
> > .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 264 +++++++++++++++---
> > .../FdtPciHostBridgeLib.inf | 3 +
> > 2 files changed, 230 insertions(+), 37 deletions(-)
>
> (1) The "Contributed-under:" line is no longer necessary (even defined) in
> the commit message.
>
> (2) This code is way too large for my taste to duplicate between ArmVirtPkg
> and OvmfPkg. I would strongly prefer if we could factor the logic in OvmfPkg
> out to a separate library, and use that from both consumer sites.
>
> (3) Can you please provide a pointer to the QEMU-side work? In particular,
> this logic depdens on the "etc/extra-pci-roots" fw_cfg file.
> Where is that file being exposed by qemu-system-aarch64 / "virt"? In general,
> the firmware code is merged after the QEMU work is merged. Has the design
> been accepted for QEMU at least? (So that it make sense for us to look at the
> interfacing ArmVirtPkg code.)
>
> (4) Have you tested booting from PCI devices behind the "extra" root bridges?
> In particular, have you tested the boot order manipulation via the
> "bootindex" device properties? (OvmfPkg/Library/QemuBootOrderLib is
> already being used by the ArmVirtQemu platform, so I'd expect no changes
> related to boot order filtering / reordering -- but it should be tested.)
>
> (5) I think this feature deserves a TianoCore Feature Request. Can you please
> file one at <https://bugzilla.tianocore.org/>? Then the bugzilla link should be
> referenced in the commit message.
>
> (Preferably the bugzilla entry should summarize the present QEMU status
> too, or provide further links to QEMU discussions etc.)
>
> Thanks,
> Laszlo
>
Thanks for replying!
1. I see, I would not define it in next patch.
2. I would factor the same logic parts into a separate library.
3.The qemu side work is to support pxb-device(which would have extra roots), the patch has been updated to
V4, but "etc/extra-pci-roots" fw_cfg file work would be done in v5. I would soon push the patch v5 and
attach the link in next edk patch.
4. Yes, I have tested to boot from the pci devices behind extra roots with bootindex, it works very well.
5. It would be done in next patch.
> >
> > diff --git
> > a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > index 496b192d22..706efeb416 100644
> > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > @@ -14,6 +14,10 @@
> > #include <Library/MemoryAllocationLib.h> #include <Library/PcdLib.h>
> > #include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/QemuFwCfgLib.h>
> > +#include <Library/PciLib.h>
> > +#include <IndustryStandard/Pci.h>
> > +#include <Library/BaseMemoryLib.h>
> >
> > #include <Protocol/FdtClient.h>
> > #include <Protocol/PciRootBridgeIo.h> @@ -306,7 +310,70 @@
> > ProcessPciHost (
> > return Status;
> > }
> >
> > -STATIC PCI_ROOT_BRIDGE mRootBridge;
> > +EFI_STATUS
> > +InitRootBridge (
> > + IN UINT64 Supports,
> > + IN UINT64 Attributes,
> > + IN UINT64 AllocAttributes,
> > + IN UINT8 RootBusNumber,
> > + IN UINT8 MaxSubBusNumber,
> > + IN PCI_ROOT_BRIDGE_APERTURE *Io,
> > + IN PCI_ROOT_BRIDGE_APERTURE *Mem,
> > + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
> > + IN PCI_ROOT_BRIDGE_APERTURE *PMem,
> > + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G,
> > + OUT PCI_ROOT_BRIDGE *RootBus
> > + )
Regards,
Miao
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm.
2020-04-02 13:52 ` Laszlo Ersek
2020-04-03 8:55 ` miaoyubo
@ 2020-04-08 3:58 ` miaoyubo
2020-04-08 10:15 ` Laszlo Ersek
1 sibling, 1 reply; 5+ messages in thread
From: miaoyubo @ 2020-04-08 3:58 UTC (permalink / raw)
To: Laszlo Ersek, ard.biesheuvel@linaro.org, leif@nuviainc.com
Cc: devel@edk2.groups.io, Xiexiangyou
> -----Original Message-----
> From: miaoyubo
> Sent: Friday, April 3, 2020 4:56 PM
> To: 'Laszlo Ersek' <lersek@redhat.com>; ard.biesheuvel@linaro.org;
> leif@nuviainc.com
> Cc: devel@edk2.groups.io; Xiexiangyou <xiexiangyou@huawei.com>
> Subject: RE: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for
> Arm.
>
>
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Thursday, April 2, 2020 9:52 PM
> > To: miaoyubo <miaoyubo@huawei.com>; ard.biesheuvel@linaro.org;
> > leif@nuviainc.com
> > Cc: devel@edk2.groups.io; Xiexiangyou <xiexiangyou@huawei.com>
> > Subject: Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots
> > for Arm.
> >
> > Quick review comments only, for now:
> >
> > On 04/02/20 14:08, Yubo Miao wrote:
> > > From: miaoyubo <miaoyubo@huawei.com>
> > >
> > > Add support for extra roots for arm, in this patch, we import the
> > > scan for extra root buses therefore the uefi could recognize
> > > multiply roots for arm.
> > >
> > > The logic keeps the same with the logic in
> > > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c"
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: miaoyubo <miaoyubo@huawei.com>
> > > ---
> > > .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 264 +++++++++++++++--
> -
> > > .../FdtPciHostBridgeLib.inf | 3 +
> > > 2 files changed, 230 insertions(+), 37 deletions(-)
> >
> > (1) The "Contributed-under:" line is no longer necessary (even
> > defined) in the commit message.
> >
> > (2) This code is way too large for my taste to duplicate between
> > ArmVirtPkg and OvmfPkg. I would strongly prefer if we could factor the
> > logic in OvmfPkg out to a separate library, and use that from both
> consumer sites.
> >
Where should I put the extracted logic?(Still put in OvmfPkg and direct use it in ArmVirtPkg or
put them in MdeModulePkg/Include/Library/PciHostBridgeLib.h? If in PciHostBridgeLib.h,
should I create a new .c file as a common implementation?)
There are some other codes duplicated between FdtPciHostBridgeLib.c and PciHostBridgeLib.c,
should I extract them?
> > (3) Can you please provide a pointer to the QEMU-side work? In
> > particular, this logic depdens on the "etc/extra-pci-roots" fw_cfg file.
> > Where is that file being exposed by qemu-system-aarch64 / "virt"? In
> > general, the firmware code is merged after the QEMU work is merged.
> > Has the design been accepted for QEMU at least? (So that it make sense
> > for us to look at the interfacing ArmVirtPkg code.)
> >
> > (4) Have you tested booting from PCI devices behind the "extra" root
> bridges?
> > In particular, have you tested the boot order manipulation via the
> > "bootindex" device properties? (OvmfPkg/Library/QemuBootOrderLib is
> > already being used by the ArmVirtQemu platform, so I'd expect no
> > changes related to boot order filtering / reordering -- but it should
> > be tested.)
> >
> > (5) I think this feature deserves a TianoCore Feature Request. Can you
> > please file one at <https://bugzilla.tianocore.org/>? Then the
> > bugzilla link should be referenced in the commit message.
> >
> > (Preferably the bugzilla entry should summarize the present QEMU
> > status too, or provide further links to QEMU discussions etc.)
> >
> > Thanks,
> > Laszlo
> >
>
> Thanks for replying!
> 1. I see, I would not define it in next patch.
> 2. I would factor the same logic parts into a separate library.
> 3.The qemu side work is to support pxb-device(which would have extra
> roots), the patch has been updated to V4, but "etc/extra-pci-roots" fw_cfg
> file work would be done in v5. I would soon push the patch v5 and attach the
> link in next edk patch.
> 4. Yes, I have tested to boot from the pci devices behind extra roots with
> bootindex, it works very well.
> 5. It would be done in next patch.
>
>
> > >
> > > diff --git
> > > a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > > b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > > index 496b192d22..706efeb416 100644
> > > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> > > @@ -14,6 +14,10 @@
> > > #include <Library/MemoryAllocationLib.h> #include
> > > <Library/PcdLib.h> #include <Library/UefiBootServicesTableLib.h>
> > > +#include <Library/QemuFwCfgLib.h>
> > > +#include <Library/PciLib.h>
> > > +#include <IndustryStandard/Pci.h>
> > > +#include <Library/BaseMemoryLib.h>
> > >
> > > #include <Protocol/FdtClient.h>
> > > #include <Protocol/PciRootBridgeIo.h> @@ -306,7 +310,70 @@
> > > ProcessPciHost (
> > > return Status;
> > > }
> > >
> > > -STATIC PCI_ROOT_BRIDGE mRootBridge;
> > > +EFI_STATUS
> > > +InitRootBridge (
> > > + IN UINT64 Supports,
> > > + IN UINT64 Attributes,
> > > + IN UINT64 AllocAttributes,
> > > + IN UINT8 RootBusNumber,
> > > + IN UINT8 MaxSubBusNumber,
> > > + IN PCI_ROOT_BRIDGE_APERTURE *Io,
> > > + IN PCI_ROOT_BRIDGE_APERTURE *Mem,
> > > + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
> > > + IN PCI_ROOT_BRIDGE_APERTURE *PMem,
> > > + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G,
> > > + OUT PCI_ROOT_BRIDGE *RootBus
> > > + )
>
> Regards,
> Miao
Regards，
Miao
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm.
2020-04-08 3:58 ` miaoyubo
@ 2020-04-08 10:15 ` Laszlo Ersek
0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-04-08 10:15 UTC (permalink / raw)
To: miaoyubo, ard.biesheuvel@linaro.org, leif@nuviainc.com
Cc: devel@edk2.groups.io, Xiexiangyou
On 04/08/20 05:58, miaoyubo wrote:
>>> (2) This code is way too large for my taste to duplicate between
>>> ArmVirtPkg and OvmfPkg. I would strongly prefer if we could factor the
>>> logic in OvmfPkg out to a separate library, and use that from both
>> consumer sites.
>>>
>
> Where should I put the extracted logic?(Still put in OvmfPkg and direct use it in ArmVirtPkg or
> put them in MdeModulePkg/Include/Library/PciHostBridgeLib.h? If in PciHostBridgeLib.h,
> should I create a new .c file as a common implementation?)
> There are some other codes duplicated between FdtPciHostBridgeLib.c and PciHostBridgeLib.c,
> should I extract them?
Good questions, I've kind of expected them. Thanks for asking them.
There are at least two ways to approach this.
* One would be to move the ArmVirtPkg/Library/FdtPciHostBridgeLib/ stuff
under OvmfPkg/Library/PciHostBridgeLib/, have two INF files in the same
directory, and separate out exactly those bits of functionality that are
shared, using module-internal header files.
This is usually a good thing if much of the logic is shared. In this
case, I don't like it however, as the ArmVirtPkg lib instance INF file
itself lists some dependencies that come from "ArmPkg.dec" and
"ArmVirtPkg.dec":
- PciPcdProducerLib class
- PcdPciMmio32Translation, PcdPciMmio64Translation, PcdPciIoTranslation
- gFdtClientProtocolGuid
So we'd either have to move those into OvmfPkg, or else make OvmfPkg
content depend on ArmVirtPkg.dec / ArmPkg.dec. None of those are good ideas.
* Therefore, please go the more winding route, and introduce a new lib
class, and lib instance, under OvmfPkg. For the lib class name, I
suggest "PciHostBridgeUtilityLib". Files:
OvmfPkg/OvmfPkg.dec
OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
Then both OvmfPkg's and ArmVirtPkg's PciHostBridgeLib instances can
consume this.
I'd prefer four patches:
- #1: OvmfPkg patch. Introduce the new lib class and lib instance, with
functionality that the ArmVirtPkg and OvmfPkg PciHostBridgeLib instances
already share.
Regarding the OvmfPkg/Library/PciHostBridgeLib instace, you can at once
move code from that lib instance to the new
OvmfPkg/Library/PciHostBridgeUtilityLib, in this patch.
- #2: ArmVirtPkg patch. Elminate the currently duplicated code, and put
the newly extracted code to use, through consuming the new lib class.
- #3: OvmfPkg patch. Move more code from PciHostBridgeLib to
PciHostBridgeUtilityLib: code that you will need to use in the next
patch, for enabling multiple root bridges in ArmVirtPkg.
-#4: ArmVirtPkg patch. Consume the code exposed in patch#3 for enabling
the feature.
In theory, the code extraction / movement described in patches #1 and #3
could be done in a single unified step, at the start of the series.
However, we certainly want to keep #2 and #4 separate (#2 is
refactoring, #4 is feature enablement). And then I prefer keeping #1 and
#3 separate too -- while both move code from the same old lib instance
to the same new lib instance, their motivations are different. The
ultimate goal is of course just the one "share as much code as possible
between ArmVirtPkg and OvmfPkg", but, when I run "git-blame" on the new
lib class header, I'd like to understand why exactly any given function
prototype was declared there.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-08 10:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-02 12:08 [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm Yubo Miao
2020-04-02 13:52 ` Laszlo Ersek
2020-04-03 8:55 ` miaoyubo
2020-04-08 3:58 ` miaoyubo
2020-04-08 10:15 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox