[Qemu-devel] [PATCH 0/4] hw/pcie: Multi-root support for Q35

[Qemu-devel] [PATCH 0/4] hw/pcie: Multi-root support for Q35

[Marcel Apfelbaum]

The PXB host bridge provides a way to have multiple PCI hierarchies (PCI root buses).
This series is porting the PXB device to Q35 machines by:
 - removing the internal pci-bridge
 - make the PXB bus PCIe for Q35, while it remains PCI for i440fx.

This approach works because the Root Complexes are exposed to guest as regular
(legacy) opaque PCI host bridges.

Tested on Fedora and Windows guests with both Root Ports and PCIe Switches.

Thanks,
Marcel

Marcel Apfelbaum (4):
  hw/pxb: remove the built-in pci bridge
  hw/acpi: merge pxb adjacent memory/IO ranges
  hw/pc: query both q35 and i440fx bus
  hw/pxb: add support for PCIe

 hw/i386/acpi-build.c                | 126 +++++++++++++++++++++---------------
 hw/i386/pc.c                        |   2 +-
 hw/pci-bridge/pci_expander_bridge.c |  28 +++++---
 hw/pci-host/q35.c                   |   8 +++
 include/hw/i386/pc.h                |   9 +++
 5 files changed, 112 insertions(+), 61 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/4] hw/pxb: remove the built-in pci bridge

[Marcel Apfelbaum]

As part of porting the pxb device to Q35 remove the internal pci-2-pci
bridge. The only way to hot-pug devices on the extra PCI root buses
is by adding a pci-2-pci to the pxb before the firmware assign the
IO/mem resources.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 57f8a37..0464dec 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -196,7 +196,7 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
 static int pxb_dev_initfn(PCIDevice *dev)
 {
     PXBDev *pxb = PXB_DEV(dev);
-    DeviceState *ds, *bds;
+    DeviceState *ds;
     PCIBus *bus;
     const char *dev_name = NULL;
 
@@ -211,18 +211,13 @@ static int pxb_dev_initfn(PCIDevice *dev)
     }
 
     ds = qdev_create(NULL, TYPE_PXB_HOST);
-    bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
+    bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_BUS);
 
     bus->parent_dev = dev;
     bus->address_space_mem = dev->bus->address_space_mem;
     bus->address_space_io = dev->bus->address_space_io;
     bus->map_irq = pxb_map_irq_fn;
 
-    bds = qdev_create(BUS(bus), "pci-bridge");
-    bds->id = dev_name;
-    qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
-    qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
-
     PCI_HOST_BRIDGE(ds)->bus = bus;
 
     if (pxb_register_bus(dev, bus)) {
@@ -230,7 +225,6 @@ static int pxb_dev_initfn(PCIDevice *dev)
     }
 
     qdev_init_nofail(ds);
-    qdev_init_nofail(bds);
 
     pci_word_test_and_set_mask(dev->config + PCI_STATUS,
                                PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/4] hw/pxb: remove the built-in pci bridge

[Gerd Hoffmann]

On Do, 2015-11-12 at 17:11 +0200, Marcel Apfelbaum wrote:
> As part of porting the pxb device to Q35 remove the internal pci-2-pci
> bridge. The only way to hot-pug devices on the extra PCI root buses
> is by adding a pci-2-pci to the pxb before the firmware assign the
> IO/mem resources.

I think this breaks live migration and is guest-visible too.  So not
going to fly without backward-compatibility mode for old machine
types ...

Possibly it is easier to just have have pci and pcie pxb devices look
differenty rather than adding all the compatibility fluff.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] hw/pxb: remove the built-in pci bridge

[Marcel Apfelbaum]

On 11/13/2015 11:13 AM, Gerd Hoffmann wrote:
> On Do, 2015-11-12 at 17:11 +0200, Marcel Apfelbaum wrote:
>> As part of porting the pxb device to Q35 remove the internal pci-2-pci
>> bridge. The only way to hot-pug devices on the extra PCI root buses
>> is by adding a pci-2-pci to the pxb before the firmware assign the
>> IO/mem resources.
>
> I think this breaks live migration and is guest-visible too.  So not
> going to fly without backward-compatibility mode for old machine
> types ...
>
> Possibly it is easier to just have have pci and pcie pxb devices look
> differenty rather than adding all the compatibility fluff.
>

Hi Gerd,

Thank you for the review.

Yes, you are right about migration, but I prefer to add a compat-prop rather
than different devices because I do want to remove the internal pci-bridge
even for pxb PCI.
The reason is the feedback I got from libvirt that "composed" device are not recommended.

In conclusion, since I am going to get rid of the pci-bridge, the devices will be exactly
the same (except for the internal bus).

I want to use the same concept as virtio-pci devices that get to be "express" on PCIe,
but remain PCI on PCI buses.

Thanks,
Marcel


> cheers,
>    Gerd
>
>

[Qemu-devel] [PATCH 2/4] hw/acpi: merge pxb adjacent memory/IO ranges

[Marcel Apfelbaum]

Since the PXB has no longer a built-in PCI bridge, the
ACPI will include IO/MEM ranges per device. Try to merge
adjacent resources to reduce the ACPI tables length.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 123 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 74 insertions(+), 49 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..736b252 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -762,16 +762,59 @@ static void crs_replace_with_free_ranges(GPtrArray *ranges,
     g_ptr_array_free(free_ranges, false);
 }
 
+/*
+ * crs_range_merge - merges adjacent ranges in the given array.
+ * Array elements are deleted and replaced with the merged ranges.
+ */
+static void crs_range_merge(GPtrArray *range)
+{
+    GPtrArray *tmp =  g_ptr_array_new_with_free_func(crs_range_free);
+    CrsRangeEntry *entry;
+    uint64_t range_base, range_limit;
+    int i;
+
+    if (!range->len) {
+        return;
+    }
+
+    g_ptr_array_sort(range, crs_range_compare);
+
+    entry = g_ptr_array_index(range, 0);
+    range_base = entry->base;
+    range_limit = entry->limit;
+    for (i = 1; i < range->len; i++) {
+        entry = g_ptr_array_index(range, i);
+        if (entry->base - 1 == range_limit) {
+            range_limit = entry->limit;
+        } else {
+            crs_range_insert(tmp, range_base, range_limit);
+            range_base = entry->base;
+            range_limit = entry->limit;
+        }
+    }
+    crs_range_insert(tmp, range_base, range_limit);
+
+    g_ptr_array_set_size(range, 0);
+    for (i = 0; i < tmp->len; i++) {
+        entry = g_ptr_array_index(tmp, i);
+        crs_range_insert(range, entry->base, entry->limit);
+    }
+    g_ptr_array_free(tmp, true);
+}
+
 static Aml *build_crs(PCIHostState *host,
                       GPtrArray *io_ranges, GPtrArray *mem_ranges)
 {
     Aml *crs = aml_resource_template();
+    GPtrArray *host_io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    GPtrArray *host_mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    CrsRangeEntry *entry;
     uint8_t max_bus = pci_bus_num(host->bus);
     uint8_t type;
     int devfn;
+    int i;
 
     for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
-        int i;
         uint64_t range_base, range_limit;
         PCIDevice *dev = host->bus->devices[devfn];
 
@@ -794,26 +837,9 @@ static Aml *build_crs(PCIHostState *host,
             }
 
             if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
-                aml_append(crs,
-                    aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
-                                AML_POS_DECODE, AML_ENTIRE_RANGE,
-                                0,
-                                range_base,
-                                range_limit,
-                                0,
-                                range_limit - range_base + 1));
-                crs_range_insert(io_ranges, range_base, range_limit);
+                crs_range_insert(host_io_ranges, range_base, range_limit);
             } else { /* "memory" */
-                aml_append(crs,
-                    aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-                                     AML_MAX_FIXED, AML_NON_CACHEABLE,
-                                     AML_READ_WRITE,
-                                     0,
-                                     range_base,
-                                     range_limit,
-                                     0,
-                                     range_limit - range_base + 1));
-                crs_range_insert(mem_ranges, range_base, range_limit);
+                crs_range_insert(host_mem_ranges, range_base, range_limit);
             }
         }
 
@@ -832,15 +858,7 @@ static Aml *build_crs(PCIHostState *host,
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                aml_append(crs,
-                           aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
-                                       AML_POS_DECODE, AML_ENTIRE_RANGE,
-                                       0,
-                                       range_base,
-                                       range_limit,
-                                       0,
-                                       range_limit - range_base + 1));
-                crs_range_insert(io_ranges, range_base, range_limit);
+                crs_range_insert(host_io_ranges, range_base, range_limit);
             }
 
             range_base =
@@ -853,16 +871,7 @@ static Aml *build_crs(PCIHostState *host,
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                aml_append(crs,
-                           aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-                                            AML_MAX_FIXED, AML_NON_CACHEABLE,
-                                            AML_READ_WRITE,
-                                            0,
-                                            range_base,
-                                            range_limit,
-                                            0,
-                                            range_limit - range_base + 1));
-                crs_range_insert(mem_ranges, range_base, range_limit);
+                crs_range_insert(host_mem_ranges, range_base, range_limit);
             }
 
             range_base =
@@ -875,20 +884,36 @@ static Aml *build_crs(PCIHostState *host,
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                aml_append(crs,
-                           aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-                                            AML_MAX_FIXED, AML_NON_CACHEABLE,
-                                            AML_READ_WRITE,
-                                            0,
-                                            range_base,
-                                            range_limit,
-                                            0,
-                                            range_limit - range_base + 1));
-                crs_range_insert(mem_ranges, range_base, range_limit);
+                crs_range_insert(host_mem_ranges, range_base, range_limit);
             }
         }
     }
 
+    crs_range_merge(host_io_ranges);
+    for (i = 0; i < host_io_ranges->len; i++) {
+        entry = g_ptr_array_index(host_io_ranges, i);
+        aml_append(crs,
+                   aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
+                               AML_POS_DECODE, AML_ENTIRE_RANGE,
+                               0, entry->base, entry->limit, 0,
+                               entry->limit - entry->base + 1));
+        crs_range_insert(io_ranges, entry->base, entry->limit);
+    }
+    g_ptr_array_free(host_io_ranges, true);
+
+    crs_range_merge(host_mem_ranges);
+    for (i = 0; i < host_mem_ranges->len; i++) {
+        entry = g_ptr_array_index(host_mem_ranges, i);
+        aml_append(crs,
+                   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
+                                    AML_READ_WRITE,
+                                    0, entry->base, entry->limit, 0,
+                                    entry->limit - entry->base + 1));
+        crs_range_insert(mem_ranges, entry->base, entry->limit);
+    }
+    g_ptr_array_free(host_mem_ranges, true);
+
     aml_append(crs,
         aml_word_bus_number(AML_MIN_FIXED, AML_MAX_FIXED, AML_POS_DECODE,
                             0,
-- 
2.1.0

[Qemu-devel] [PATCH 3/4] hw/pc: query both q35 and i440fx bus

[Marcel Apfelbaum]

Look for pxb devices on both i386 machines.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 3 +--
 hw/i386/pc.c         | 2 +-
 hw/pci-host/q35.c    | 8 ++++++++
 include/hw/i386/pc.h | 9 +++++++++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 736b252..80e9d47 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
     /* Reserve space for header */
     acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
 
-    /* Extra PCI root buses are implemented  only for i440fx */
-    bus = find_i440fx();
+    bus = find_pc();
     if (bus) {
         QLIST_FOREACH(bus, &bus->child, sibling) {
             uint8_t bus_num = pci_bus_num(bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0cb8afd..c027782 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1174,7 +1174,7 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
     PcGuestInfoState *guest_info_state = container_of(notifier,
                                                       PcGuestInfoState,
                                                       machine_done);
-    PCIBus *bus = find_i440fx();
+    PCIBus *bus = find_pc();
 
     if (bus) {
         int extra_hosts = 0;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index c81507d..2fbbcd3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -511,6 +511,14 @@ static void mch_realize(PCIDevice *d, Error **errp)
     }
 }
 
+PCIBus *find_q35(void)
+{
+    PCIHostState *s = OBJECT_CHECK(PCIHostState,
+                                   object_resolve_path("/machine/q35", NULL),
+                                   TYPE_PCI_HOST_BRIDGE);
+    return s ? s->bus : NULL;
+}
+
 uint64_t mch_mcfg_base(void)
 {
     bool ambiguous;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4bbc0ff..95d7610 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -242,6 +242,15 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
                     MemoryRegion *ram_memory);
 
 PCIBus *find_i440fx(void);
+PCIBus *find_q35(void);
+
+static inline PCIBus *find_pc(void)
+{
+    PCIBus *bus = find_i440fx();
+
+    return bus ? bus : find_q35();
+}
+
 /* piix4.c */
 extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
-- 
2.1.0

[Qemu-devel] [PATCH 4/4] hw/pxb: add support for PCIe

[Marcel Apfelbaum]

The PXB internal bus type is created in  conformance
with the primary root bus (bus 0).

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 0464dec..dac28f0 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -23,6 +23,9 @@
 #define TYPE_PXB_BUS "pxb-bus"
 #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS)
 
+#define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"
+#define PXB_PCIE_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_PCIE_BUS)
+
 typedef struct PXBBus {
     /*< private >*/
     PCIBus parent_obj;
@@ -82,10 +85,18 @@ static const TypeInfo pxb_bus_info = {
     .class_init    = pxb_bus_class_init,
 };
 
+static const TypeInfo pxb_pcie_bus_info = {
+    .name          = TYPE_PXB_PCIE_BUS,
+    .parent        = TYPE_PCIE_BUS,
+    .instance_size = sizeof(PXBBus),
+    .class_init    = pxb_bus_class_init,
+};
+
 static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
                                           PCIBus *rootbus)
 {
-    PXBBus *bus = PXB_BUS(rootbus);
+    PXBBus *bus = pci_bus_is_express(rootbus) ?
+                  PXB_PCIE_BUS(rootbus) : PXB_BUS(rootbus);
 
     snprintf(bus->bus_path, 8, "0000:%02x", pxb_bus_num(rootbus));
     return bus->bus_path;
@@ -211,7 +222,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
     }
 
     ds = qdev_create(NULL, TYPE_PXB_HOST);
-    bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_BUS);
+    if (pci_bus_is_express(dev->bus)) {
+        bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
+    } else {
+        bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_BUS);
+    }
 
     bus->parent_dev = dev;
     bus->address_space_mem = dev->bus->address_space_mem;
@@ -273,6 +288,7 @@ static const TypeInfo pxb_dev_info = {
 static void pxb_register_types(void)
 {
     type_register_static(&pxb_bus_info);
+    type_register_static(&pxb_pcie_bus_info);
     type_register_static(&pxb_host_info);
     type_register_static(&pxb_dev_info);
 }
-- 
2.1.0