On 06/02/15 17:04, Marcel Apfelbaum wrote:
> Hi,
>
> The following series:
>    - [Qemu-devel] [PATCH V8 00/17] hw/pc: implement multiple primary
>      busses for pc machines
>    - https://www.mail-archive.com/qemu-devel@nongnu.org/msg300089.html
>      adds a PCI Expander Device to QEMU that exposes a new PCI root
>      bus.

(Let's tie this thread to the v7 question too:

http://thread.gmane.org/gmane.comp.emulators.qemu/338583/focus=338599
)

> The PXB is a "light-weight" host bridge whose purpose is to enable
> the main host bridge to support multiple PCI root buses.
>
> It does not have its own registers for configuration cycles, but is
> snoops on main host bridge registers and it lives on the same PCI
> segment.
>
> The device receives from the command line the bus number and expects
> the firmware (bios/UEFI) to probe the bus for devices behind it and
> configure them.
>
> My question is how can it be supported in edk2? Are there any
> architecture limitations that will prevent it to work?
>
> My edk2/UEFI knowledge is rather limited, but I did see in the spec
> that there is support for this kind of device:
>
>      13.1.1 PCI Root Bridge I/O Overview
>      ...
>      Depending on the chipset, a single
>      EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL may abstract a portion of a PCI
>      Segment, or an entire PCI Segment. A PCI Host Bridge may produce
>      one or more PCI Root Bridges. When a PCI Host Bridge produces
>      multiple PCI Root Bridges, it is possible to have more than one
>      PCI Segment.
>      ...
>
> It seems that multiple EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances for
> the same PCI Host Bridge mapped into the same PCI Segment is the
> answer. First instance belongs to the "main" host bridge and the other
> to the PXBs.
>
> The open questions are of course how to assign resources (bus
> numbers/IO/MEM) to the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances.
>
> For the bus numbers I think that the PCI Host Bridge can scan the 0x0
> - 0xff range and build incrementally the bus ranges.
>
> Regarding IO/MEM ranges I am still not sure. The way it is done in
> SeaBIOS is that all devices behind PXB root bus are "considered" as
> being behind bus 0 for resources allocation. Once the resources
> allocation is done, each EFI_PCI_ROOT_BRIDGE gets the list of MEM/IO
> ranges corresponding with the devices behind them.
>
> Any comments and suggestions would be greatly appreciated.
> Thank you in advance,
> Marcel

I'm attaching a horrible patch (applies on top of edk2 SVN r17543, aka
git commit d4848bb9df) that allows OVMF to recognize the e1000 NIC with
the following QEMU command line:

ISO=/mnt/data/isos/Fedora-Live-Xfce-x86_64-20-1.iso
CODE=/home/virt-images/OVMF_CODE.fd
TMPL=/home/virt-images/OVMF_VARS.fd

cp $TMPL vars.fd

qemu-system-x86_64 \
  -m 2048 \
  -M pc \
  -enable-kvm \
  -device qxl-vga \
  -drive if=pflash,readonly,format=raw,file=$CODE \
  -drive if=pflash,format=raw,file=vars.fd \
  -drive id=cdrom,if=none,readonly,format=raw,file=$ISO \
  -device virtio-scsi-pci,id=scsi0 \
  -device scsi-cd,bus=scsi0.0,drive=cdrom,bootindex=0 \
  -debugcon file:debug.log \
  -global isa-debugcon.iobase=0x402 \
  -device pxb,id=bridge1,bus_nr=128 \
  -netdev user,id=netdev0,hostfwd=tcp:127.0.0.1:2222-:22 \
  -device e1000,netdev=netdev0,bus=bridge1,addr=1 \
  -monitor stdio

With this hack in place, and using the above QEMU command line,
"debug.log" bears witness to the PCI enumeration succeeding, and the
"PCI" command in the UEFI shell lists the e1000 NIC.

I agree with your analysis that the way to support this QEMU feature in
OVMF is to produce several EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances.
Beyond that agreement, I must say that invalidating the assumption that
"there is only one root bridge" breaks about everything in OVMF.

Just skimming my hack-patch identifies the problems (or in some cases,
questions):

* The PCI host bridge driver under PcAtChipsetPkg needs to be cloned
under OvmfPkg, and (as you say) the bus ranges need to be determined
dynamically. On IRC you said that probing for device 0 on a bus is
sufficient to see if the bus lives, but for now I'm unsure if this would
be a layering violation or not for the UEFI protocols in question. Maybe
not.

* The bus ranges assigned to each "pxb" device (ie. root bridge) would
have to be able to accommodate any subordinate buses enumerated off that
root bridge. At least this is what PciRootBridgeEnumerator() in
"MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c" seems to require. I've
got no clue how to size the bus ranges properly for the root bridges to
satisfy this.

* In fact, the bus range presented over the
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Configuration() function cannot be just
a range. As far as I tested the PCI bus driver (see path above), it
doesn't find anything if the range retrieved from
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Configuration() doesn't start *exactly*
with a live bus. In other words, it doesn't look for sibling buses, or
independent buses in the bus range exposed by the root bridge protocol.
It looks for *one* bus *at* the start of the range (and subordinate
buses hanging off of that). I have absolutely no clue why this is so,
but it means that for each pxb found, one root bridge io protocol would
have to be produced, and that proto should expose one bus range, with
the low end matching exactly the bus number, and the high end enabling
child buses to be enumerated.

* In the attached hack, I'm splitting the pre-patch, static, IO & MMIO
apertures in the middle. Maybe they could be the same shared ranges, as
you say. I don't know.

* In the OVMF BDS (boot device selection) code, we manually connect the
only one root bridge. This would have to be made dynamic, to connect all
of them. This connection basically amounts to "starting the enumeration".

* The OVMF boot order processing code hardcodes PciRoot(0x0) in a bunch
of device path matching logic. That would not be appropriate any longer.
In fact the above command line should boot the fedora live CD, but it
doesn't, and in the UEFI setup utility I cannot even browse the CD
filesystem.

* Gabriel wrote earlier some code for setting the INTx interrupt pin
registers of all PCI devices in OVMF's BDS. That code breaks now, an
assert is triggered ("PCI host bridge (00:00.0) should have no
interrupts"). Not sure why this happens.

* The UEFI device paths for the PCI root bridges (textually,
PciRoot(0x0), PciRoot(0x1) etc) actually start with ACPI device path
nodes. They consist of a PNP0A03 _HID and a numeric _UID. If my reading
of the UEFI spec is correct, the _UIDs that OVMF would assign to these
device path notes would have to match the *actual* ACPI payload that
QEMU exports. The _UID assignment is now static (just a 0), and my
hack-patch assigns a static 1 to the "other" root bridge's device path.
This is not good. OVMF would either have to parse ACPI payload
(horrible) or get the _UID<->pxb assignment via fw_cfg.

That's all the carnage I can think of right now, but I'm sure this is
just the tip of the iceberg. This would be a very large project, and
QEMU might have to expose a lot more info over fw_cfg than it does now.

In any case, the device model itself could be digestible for OVMF, based
on the results of this hack.

Thanks
Laszlo
From fe852d09bc15ba4dacc792b73dff3a05f2027db3 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <ler...@redhat.com>
Date: Tue, 2 Jun 2015 17:32:35 +0200
Subject: [PATCH] hack

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---
 OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h    | 12 +++----
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c  |  9 +++++
 OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c    | 17 +++++++++
 OvmfPkg/Library/PlatformBdsLib/PlatformData.c   |  7 +++-
 PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridge.c | 47 ++++++++++++++++++++++---
 5 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h
index 7006fb3..5374313 100644
--- a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h
@@ -93,7 +93,7 @@ extern VENDOR_DEVICE_PATH         gTerminalTypeDeviceNode;
     (Dev) \
   }
 
-#define PNPID_DEVICE_PATH_NODE(PnpId) \
+#define PNPID_DEVICE_PATH_NODE(PnpId, Uid) \
   { \
     { \
       ACPI_DEVICE_PATH, \
@@ -104,11 +104,11 @@ extern VENDOR_DEVICE_PATH         gTerminalTypeDeviceNode;
       }, \
     }, \
     EISA_PNP_ID((PnpId)), \
-    0 \
+    (Uid) \
   }
 
-#define gPciRootBridge \
-  PNPID_DEVICE_PATH_NODE(0x0A03)
+#define gPciRootBridge(Uid) \
+  PNPID_DEVICE_PATH_NODE(0x0A03, (Uid))
 
 #define gPciIsaBridge \
   PCI_DEVICE_PATH_NODE(0, 0x1f)
@@ -117,10 +117,10 @@ extern VENDOR_DEVICE_PATH         gTerminalTypeDeviceNode;
   PCI_DEVICE_PATH_NODE(0, 0x1e)
 
 #define gPnpPs2Keyboard \
-  PNPID_DEVICE_PATH_NODE(0x0303)
+  PNPID_DEVICE_PATH_NODE(0x0303, 0)
 
 #define gPnp16550ComPort \
-  PNPID_DEVICE_PATH_NODE(0x0501)
+  PNPID_DEVICE_PATH_NODE(0x0501, 0)
 
 #define gUart \
   { \
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
index 7329143..54a27e9 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
@@ -237,6 +237,8 @@ PciRootBridgeEnumerator (
   // Get the bus number to start with
   //
   StartBusNumber = (UINT8) (Configuration->AddrRangeMin);
+  DEBUG ((EFI_D_VERBOSE, "%a: %d: StartBusNumber=%d\n", __FUNCTION__, __LINE__,
+    StartBusNumber));
 
   //
   // Initialize the subordinate bus number
@@ -260,6 +262,9 @@ PciRootBridgeEnumerator (
             &SubBusNumber,
             &PaddedBusRange
             );
+  DEBUG ((EFI_D_VERBOSE, "%a: %d: Status=%r, SubBusNumber=%d, "
+    "PaddedBusRange=%d\n", __FUNCTION__, __LINE__, Status, SubBusNumber,
+    PaddedBusRange));
 
   if (EFI_ERROR (Status)) {
     return Status;
@@ -271,6 +276,10 @@ PciRootBridgeEnumerator (
   //
 
   Status = PciAllocateBusNumber (RootBridgeDev, SubBusNumber, PaddedBusRange, &SubBusNumber);
+
+  DEBUG ((EFI_D_VERBOSE, "%a: %d: Status=%r, SubBusNumber=%d\n", __FUNCTION__,
+    __LINE__, Status, SubBusNumber));
+
   if (EFI_ERROR (Status)) {
     return Status;
   }  
diff --git a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
index 1eb2a8b..7fc64b7 100644
--- a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
@@ -158,6 +158,22 @@ Returns:
     return Status;
   }
 
+  BdsLibConnectDevicePath (gPlatformRootBridges[1]);
+
+  Status = gBS->LocateDevicePath (
+                  &gEfiDevicePathProtocolGuid,
+                  &gPlatformRootBridges[1],
+                  &RootHandle
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = gBS->ConnectController (RootHandle, NULL, NULL, FALSE);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   return EFI_SUCCESS;
 }
 
@@ -798,6 +814,7 @@ SetPciIntLine (
         "%a: PCI host bridge (00:00.0) should have no interrupts!\n",
         __FUNCTION__
         ));
+      return Status;
       ASSERT (FALSE);
     }
 
diff --git a/OvmfPkg/Library/PlatformBdsLib/PlatformData.c b/OvmfPkg/Library/PlatformBdsLib/PlatformData.c
index 1a30531..969f9f8 100644
--- a/OvmfPkg/Library/PlatformBdsLib/PlatformData.c
+++ b/OvmfPkg/Library/PlatformBdsLib/PlatformData.c
@@ -29,12 +29,17 @@ VENDOR_DEVICE_PATH         gTerminalTypeDeviceNode    = gPcAnsiTerminal;
 // Predefined platform root bridge
 //
 PLATFORM_ROOT_BRIDGE_DEVICE_PATH  gPlatformRootBridge0 = {
-  gPciRootBridge,
+  gPciRootBridge(0),
+  gEndEntire
+};
+PLATFORM_ROOT_BRIDGE_DEVICE_PATH  gPlatformRootBridge1 = {
+  gPciRootBridge(1),
   gEndEntire
 };
 
 EFI_DEVICE_PATH_PROTOCOL          *gPlatformRootBridges[] = {
   (EFI_DEVICE_PATH_PROTOCOL *) &gPlatformRootBridge0,
+  (EFI_DEVICE_PATH_PROTOCOL *) &gPlatformRootBridge1,
   NULL
 };
 
diff --git a/PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridge.c b/PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridge.c
index 2f6ef68..2bb3773 100644
--- a/PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridge.c
+++ b/PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridge.c
@@ -20,11 +20,16 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 //            Root Bridge's device path
 //            Root Bridge's resource aperture
 //
-UINTN RootBridgeNumber[1] = { 1 };
+UINTN RootBridgeNumber[1] = { 2 };
 
-UINT64 RootBridgeAttribute[1][1] = { { EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM } };
+UINT64 RootBridgeAttribute[1][2] = {
+  {
+    EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM,
+    EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM 
+  }
+};
 
-EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath[1][1] = {
+EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath[1][2] = {
   {
     {
       {
@@ -48,12 +53,38 @@ EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath[1][1] = {
           0
         }
       }
+    },
+    {
+      {
+        {
+          ACPI_DEVICE_PATH,
+          ACPI_DP,
+          {
+            (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
+            (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
+          }
+        },
+        EISA_PNP_ID(0x0A03),
+        1
+      },
+  
+      {
+        END_DEVICE_PATH_TYPE,
+        END_ENTIRE_DEVICE_PATH_SUBTYPE,
+        {
+          END_DEVICE_PATH_LENGTH,
+          0
+        }
+      }
     }
   }
 };
 
-PCI_ROOT_BRIDGE_RESOURCE_APERTURE  mResAperture[1][1] = {
-  {{0, 0xff, 0x80000000, 0xffffffff, 0, 0xffff}}
+PCI_ROOT_BRIDGE_RESOURCE_APERTURE  mResAperture[1][2] = {
+  {
+    {0x00, 0x7f, 0x80000000, 0xbfffffff, 0x0000, 0x7fff},
+    {0x80, 0xff, 0xc0000000, 0xffffffff, 0x8000, 0xffff},
+  }
 };
 
 EFI_HANDLE mDriverImageHandle;
@@ -695,6 +726,9 @@ StartBusEnumeration(
       ((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Temp)->AddrTranslationOffset = 0;       
       ((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Temp)->AddrLen = BusEnd - BusStart + 1;
       
+      DEBUG ((EFI_D_VERBOSE, "%a: %d: AddrLen=%Ld\n", __FUNCTION__, __LINE__, 
+        (INT64)(BusEnd - BusStart + 1)));
+
       Temp = Temp + sizeof(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR);
       ((EFI_ACPI_END_TAG_DESCRIPTOR *)Temp)->Desc = 0x79;      
       ((EFI_ACPI_END_TAG_DESCRIPTOR *)Temp)->Checksum = 0x0;
@@ -781,6 +815,9 @@ SetBusNumbers(
       BusStart = (UINTN)((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Ptr)->AddrRangeMin;
       BusLen = (UINTN)((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Ptr)->AddrLen;
       BusEnd = BusStart + BusLen - 1;
+
+      DEBUG ((EFI_D_VERBOSE, "%a: %d: BusEnd=%Ld\n", __FUNCTION__, __LINE__,
+        (INT64)BusEnd));
       
       if (BusStart > BusEnd) {
         return EFI_INVALID_PARAMETER;
-- 
1.8.3.1

------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to