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

[Qemu-devel] [PATCH V2 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.

v1 -> v2:
 Addressed Gerd Hoffmann comments:
 - Added x-enable-internal-bridge compat property to keep the PCI
   bridge for older machine to avoid breaking migration.

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 |  45 ++++++++++---
 hw/pci-host/q35.c                   |   8 +++
 include/hw/compat.h                 |   4 ++
 include/hw/i386/pc.h                |   9 +++
 6 files changed, 133 insertions(+), 61 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH V2 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.

Keep the internal bridge for existing machines.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 27 +++++++++++++++++++--------
 include/hw/compat.h                 |  4 ++++
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 57f8a37..541414c 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -34,6 +34,9 @@ typedef struct PXBBus {
 #define TYPE_PXB_DEVICE "pxb"
 #define PXB_DEV(obj) OBJECT_CHECK(PXBDev, (obj), TYPE_PXB_DEVICE)
 
+#define PXB_FLAG_ENABLE_BRIDGE_BIT 0
+#define PXB_FLAG_ENABLE_BRIDGE (1 << PXB_FLAG_ENABLE_BRIDGE_BIT)
+
 typedef struct PXBDev {
     /*< private >*/
     PCIDevice parent_obj;
@@ -41,6 +44,7 @@ typedef struct PXBDev {
 
     uint8_t bus_nr;
     uint16_t numa_node;
+    uint32_t flags;
 } PXBDev;
 
 static GList *pxb_dev_list;
@@ -196,7 +200,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, *bds = NULL;
     PCIBus *bus;
     const char *dev_name = NULL;
 
@@ -211,18 +215,21 @@ 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);
+    if (pxb->flags & PXB_FLAG_ENABLE_BRIDGE) {
+        bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
+        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);
+    } 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;
     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 +237,9 @@ static int pxb_dev_initfn(PCIDevice *dev)
     }
 
     qdev_init_nofail(ds);
-    qdev_init_nofail(bds);
+    if (bds) {
+        qdev_init_nofail(bds);
+    }
 
     pci_word_test_and_set_mask(dev->config + PCI_STATUS,
                                PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
@@ -251,6 +260,8 @@ static Property pxb_dev_properties[] = {
     /* Note: 0 is not a legal a PXB bus number. */
     DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
     DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED),
+    DEFINE_PROP_BIT("x-enable-internal-bridge", PXBDev, flags,
+                    PXB_FLAG_ENABLE_BRIDGE_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 97feb75..cbf65ee 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -10,6 +10,10 @@
             .driver   = "virtio-pci",\
             .property = "disable-pcie",\
             .value    = "on",\
+        },{\
+            .driver   = "pxb",\
+            .property = "x-enable-internal-bridge",\
+            .value    = "on",\
         },
 
 #define HW_COMPAT_2_3 \
-- 
2.1.0

[Qemu-devel] [PATCH V2 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 V2 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

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

[Eduardo Habkost]

On Sun, Nov 15, 2015 at 05:39:05PM +0200, Marcel Apfelbaum wrote:
> Look for pxb devices on both i386 machines.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
[...]
> 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);

Why not use the PCI_HOST_BRIDGE macro?

> +    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();

Have you considered a /machine/pci link, or (in case you don't
want to add something new to the QOM tree) a simple
PCMachineState::pci_bus struct field?

-- 
Eduardo

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

[Marcel Apfelbaum]

On 11/16/2015 08:26 PM, Eduardo Habkost wrote:
> On Sun, Nov 15, 2015 at 05:39:05PM +0200, Marcel Apfelbaum wrote:
>> Look for pxb devices on both i386 machines.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> [...]
>> 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);
>

Hi Eduardo,

Thanks for the review.

> Why not use the PCI_HOST_BRIDGE macro?

Do you mean ?
	PCIHostState *s = PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));

I can use it, thanks -- I was doing the usual copy/paste of the i440fx counterpart :)

>
>> +    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();
>
> Have you considered a /machine/pci link, or (in case you don't
> want to add something new to the QOM tree) a simple
> PCMachineState::pci_bus struct field?

This is a nice idea, I like the PC machine state, I'll look into it.

Thanks,
Marcel

>

[Qemu-devel] [PATCH V2 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 541414c..0a04297 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;
@@ -86,10 +89,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;
@@ -222,7 +233,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
         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);
     } else {
-        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;
@@ -290,6 +305,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

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

[Paolo Bonzini]

On 15/11/2015 16:39, Marcel Apfelbaum wrote:
>  Addressed Gerd Hoffmann comments:
>  - Added x-enable-internal-bridge compat property to keep the PCI
>    bridge for older machine to avoid breaking migration.

This will break PXB command lines written for "-M pc" in QEMU <= 2.5.
Technically there's no prohibition against doing so, but we've never
done it; I'm not sure why we should start doing so now.

Can we have two devices, on for a PCI expander bridge and one for a PCIe
expander bridge?

Paolo

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

[Marcel Apfelbaum]

On 11/16/2015 10:40 AM, Paolo Bonzini wrote:
>
>
> On 15/11/2015 16:39, Marcel Apfelbaum wrote:
>>   Addressed Gerd Hoffmann comments:
>>   - Added x-enable-internal-bridge compat property to keep the PCI
>>     bridge for older machine to avoid breaking migration.
>
> This will break PXB command lines written for "-M pc" in QEMU <= 2.5.
> Technically there's no prohibition against doing so, but we've never
> done it; I'm not sure why we should start doing so now.

Hi Paolo,

Can you please explain how does it break the command line?

For QEMU < 2.5 we automatically have the  x-enable-internal-bridge compat property "on"
so the command line remains the same.

For QEMU >= 2.5 is "off" by default and the bridge disappear, without command line changes.

>
> Can we have two devices, on for a PCI expander bridge and one for a PCIe
> expander bridge?

Gerd also asked for this, OK, I am going for it.

Thanks,
Marcel

>
> Paolo
>

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

[Paolo Bonzini]

On 16/11/2015 10:52, Marcel Apfelbaum wrote:
> On 11/16/2015 10:40 AM, Paolo Bonzini wrote:
>>
>>
>> On 15/11/2015 16:39, Marcel Apfelbaum wrote:
>>>   Addressed Gerd Hoffmann comments:
>>>   - Added x-enable-internal-bridge compat property to keep the PCI
>>>     bridge for older machine to avoid breaking migration.
>>
>> This will break PXB command lines written for "-M pc" in QEMU <= 2.5.
>> Technically there's no prohibition against doing so, but we've never
>> done it; I'm not sure why we should start doing so now.
> 
> Hi Paolo,
> 
> Can you please explain how does it break the command line?
> 
> For QEMU < 2.5 we automatically have the  x-enable-internal-bridge
> compat property "on"
> so the command line remains the same.
> 
> For QEMU >= 2.5 is "off" by default and the bridge disappear, without
> command line changes.

If you were using "-M pc", and you expected "-device" to place things
under the PCI-to-PCI bridge, that command line will not work anymore I
think.

Management tools can work around it.  They can always set the new
property to off---which by the way means it should not be "x"
prefixed---and assume the PCI-to-PCI bridge is there if the property
doesn't exist.  But it's inconvenient, and we try to avoid lockstep
changes to QEMU and management.

Paolo

>> Can we have two devices, on for a PCI expander bridge and one for a PCIe
>> expander bridge?
> 
> Gerd also asked for this, OK, I am going for it.
> 
> Thanks,
> Marcel
> 
>>
>> Paolo
>>
> 

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

[Marcel Apfelbaum]

On 11/16/2015 11:56 AM, Paolo Bonzini wrote:
>
>
> On 16/11/2015 10:52, Marcel Apfelbaum wrote:
>> On 11/16/2015 10:40 AM, Paolo Bonzini wrote:
>>>
>>>
>>> On 15/11/2015 16:39, Marcel Apfelbaum wrote:
>>>>    Addressed Gerd Hoffmann comments:
>>>>    - Added x-enable-internal-bridge compat property to keep the PCI
>>>>      bridge for older machine to avoid breaking migration.
>>>
>>> This will break PXB command lines written for "-M pc" in QEMU <= 2.5.
>>> Technically there's no prohibition against doing so, but we've never
>>> done it; I'm not sure why we should start doing so now.
>>
>> Hi Paolo,
>>
>> Can you please explain how does it break the command line?
>>
>> For QEMU < 2.5 we automatically have the  x-enable-internal-bridge
>> compat property "on"
>> so the command line remains the same.
>>
>> For QEMU >= 2.5 is "off" by default and the bridge disappear, without
>> command line changes.
>
> If you were using "-M pc", and you expected "-device" to place things
> under the PCI-to-PCI bridge, that command line will not work anymore I
> think.

Actually the command line would work. The devices will be placed on the root bus
instead of the pci bridge bus. The trick here is that we name the bus the devices
will be attached to by the pxb id.
  - When we have the bridge -> the pxb id will be used for the secondary bus
  - when don't have the bridge -> the pxb id wil be used for the root bus.
Both times the command line remains -device pxb,id=my_root -device e1000,bus=my_root


>
> Management tools can work around it.  They can always set the new
> property to off---which by the way means it should not be "x"
> prefixed---and assume the PCI-to-PCI bridge is there if the property
> doesn't exist.  But it's inconvenient, and we try to avoid lockstep
> changes to QEMU and management.
>

I understand, I'll implement this as a new device "pxb-pcie"

Thanks,
Marcel

> Paolo
>
>>> Can we have two devices, on for a PCI expander bridge and one for a PCIe
>>> expander bridge?
>>
>> Gerd also asked for this, OK, I am going for it.
>>
>> Thanks,
>> Marcel
>>
>>>
>>> Paolo
>>>
>>

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

[Paolo Bonzini]

On 16/11/2015 11:02, Marcel Apfelbaum wrote:
> Actually the command line would work. The devices will be placed on the
> root bus
> instead of the pci bridge bus. The trick here is that we name the bus
> the devices
> will be attached to by the pxb id.
>  - When we have the bridge -> the pxb id will be used for the secondary bus
>  - when don't have the bridge -> the pxb id wil be used for the root bus.
> Both times the command line remains -device pxb,id=my_root -device
> e1000,bus=my_root

What would you lose?  Hotplug?

Paolo

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

[Marcel Apfelbaum]

On 11/16/2015 12:03 PM, Paolo Bonzini wrote:
>
>
> On 16/11/2015 11:02, Marcel Apfelbaum wrote:
>> Actually the command line would work. The devices will be placed on the
>> root bus
>> instead of the pci bridge bus. The trick here is that we name the bus
>> the devices
>> will be attached to by the pxb id.
>>   - When we have the bridge -> the pxb id will be used for the secondary bus
>>   - when don't have the bridge -> the pxb id wil be used for the root bus.
>> Both times the command line remains -device pxb,id=my_root -device
>> e1000,bus=my_root
>
> What would you lose?  Hotplug?
>

Without the bridge? Yes.
However the user can add it manually the pci-bridge and have it anyway.

I wanted to get rid of the internal pci-bridge as a default, and this is why
pxb and pxb-pcie are he same device now (except bus type)



> Paolo
>

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

[Paolo Bonzini]

On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>> What would you lose?  Hotplug?
> 
> Without the bridge? Yes. However the user can add it manually the
> pci-bridge and have it anyway.

Ok, I guess that's more or less acceptable.  It's still ugly however, to
the point that I wonder if we should rename the device and call the old
one a failed experiment.

Paolo

> I wanted to get rid of the internal pci-bridge as a default, and this
> is why pxb and pxb-pcie are he same device now (except bus type)

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

[Marcel Apfelbaum]

On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>
>
> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>> What would you lose?  Hotplug?
>>
>> Without the bridge? Yes. However the user can add it manually the
>> pci-bridge and have it anyway.
>
> Ok, I guess that's more or less acceptable.  It's still ugly however, to
> the point that I wonder if we should rename the device and call the old
> one a failed experiment.
>

I guess we can rename the pxb to extra-root or something, but in this way
will have a deprecated/duplicated device to support and kill in the future.

Why not use the compat property as it is?
Again, the command line *remains* the same, the difference is where the
devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
or on the root bus itself (QEMU >= 2.5).

I know is guest visible, but the guest will see one of them depending on the machine type.

Regarding the splitting of pxb into 2 devices (pci/pcie), I have nothing against it,
but because the implementation is *exactly* the same I think we should gain more
by maintaining one device.


Thanks,
Marcel


> Paolo
>
>> I wanted to get rid of the internal pci-bridge as a default, and this
>> is why pxb and pxb-pcie are he same device now (except bus type)

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

[Michael S. Tsirkin]

On Mon, Nov 16, 2015 at 12:34:11PM +0200, Marcel Apfelbaum wrote:
> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
> >
> >
> >On 16/11/2015 11:10, Marcel Apfelbaum wrote:
> >>>What would you lose?  Hotplug?
> >>
> >>Without the bridge? Yes. However the user can add it manually the
> >>pci-bridge and have it anyway.
> >
> >Ok, I guess that's more or less acceptable.  It's still ugly however, to
> >the point that I wonder if we should rename the device and call the old
> >one a failed experiment.
> >
> 
> I guess we can rename the pxb to extra-root or something, but in this way
> will have a deprecated/duplicated device to support and kill in the future.
> 
> Why not use the compat property as it is?
> Again, the command line *remains* the same, the difference is where the
> devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
> or on the root bus itself (QEMU >= 2.5).
> 
> I know is guest visible, but the guest will see one of them depending on the machine type.
> 
> Regarding the splitting of pxb into 2 devices (pci/pcie), I have nothing against it,
> but because the implementation is *exactly* the same I think we should gain more
> by maintaining one device.
> 
> 
> Thanks,
> Marcel

Yes, I think you want a new "pci-extender" device which is just the extender.
Then existing pxb will create both it and the bridge behind it.
Maybe creating pxb which is extender+bridge was a mistake, I don't know,
but we shipped it in QEMU so we support it.


> 
> >Paolo
> >
> >>I wanted to get rid of the internal pci-bridge as a default, and this
> >>is why pxb and pxb-pcie are he same device now (except bus type)

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

[Marcel Apfelbaum]

On 11/16/2015 12:37 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2015 at 12:34:11PM +0200, Marcel Apfelbaum wrote:
>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>> What would you lose?  Hotplug?
>>>>
>>>> Without the bridge? Yes. However the user can add it manually the
>>>> pci-bridge and have it anyway.
>>>
>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>> the point that I wonder if we should rename the device and call the old
>>> one a failed experiment.
>>>
>>
>> I guess we can rename the pxb to extra-root or something, but in this way
>> will have a deprecated/duplicated device to support and kill in the future.
>>
>> Why not use the compat property as it is?
>> Again, the command line *remains* the same, the difference is where the
>> devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
>> or on the root bus itself (QEMU >= 2.5).
>>
>> I know is guest visible, but the guest will see one of them depending on the machine type.
>>
>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have nothing against it,
>> but because the implementation is *exactly* the same I think we should gain more
>> by maintaining one device.
>>
>>
>> Thanks,
>> Marcel
>
> Yes, I think you want a new "pci-extender" device which is just the extender.
> Then existing pxb will create both it and the bridge behind it.
> Maybe creating pxb which is extender+bridge was a mistake, I don't know,
> but we shipped it in QEMU so we support it.

OK, but this device will be both pci/pcie,  depending on the machine type right?
No need to split it too?

Thanks,
Marcel


>
>>
>>> Paolo
>>>
>>>> I wanted to get rid of the internal pci-bridge as a default, and this
>>>> is why pxb and pxb-pcie are he same device now (except bus type)

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

[Michael S. Tsirkin]

On Mon, Nov 16, 2015 at 12:39:45PM +0200, Marcel Apfelbaum wrote:
> On 11/16/2015 12:37 PM, Michael S. Tsirkin wrote:
> >On Mon, Nov 16, 2015 at 12:34:11PM +0200, Marcel Apfelbaum wrote:
> >>On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
> >>>
> >>>
> >>>On 16/11/2015 11:10, Marcel Apfelbaum wrote:
> >>>>>What would you lose?  Hotplug?
> >>>>
> >>>>Without the bridge? Yes. However the user can add it manually the
> >>>>pci-bridge and have it anyway.
> >>>
> >>>Ok, I guess that's more or less acceptable.  It's still ugly however, to
> >>>the point that I wonder if we should rename the device and call the old
> >>>one a failed experiment.
> >>>
> >>
> >>I guess we can rename the pxb to extra-root or something, but in this way
> >>will have a deprecated/duplicated device to support and kill in the future.
> >>
> >>Why not use the compat property as it is?
> >>Again, the command line *remains* the same, the difference is where the
> >>devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
> >>or on the root bus itself (QEMU >= 2.5).
> >>
> >>I know is guest visible, but the guest will see one of them depending on the machine type.
> >>
> >>Regarding the splitting of pxb into 2 devices (pci/pcie), I have nothing against it,
> >>but because the implementation is *exactly* the same I think we should gain more
> >>by maintaining one device.
> >>
> >>
> >>Thanks,
> >>Marcel
> >
> >Yes, I think you want a new "pci-extender" device which is just the extender.
> >Then existing pxb will create both it and the bridge behind it.
> >Maybe creating pxb which is extender+bridge was a mistake, I don't know,
> >but we shipped it in QEMU so we support it.
> 
> OK, but this device will be both pci/pcie,  depending on the machine type right?
> No need to split it too?
> 
> Thanks,
> Marcel

It's ok to have a single device but I don't like
tying it to a machine type, much.
It's always integrated within a RC, right?
Can you go by the type of the parent device?

> 
> >
> >>
> >>>Paolo
> >>>
> >>>>I wanted to get rid of the internal pci-bridge as a default, and this
> >>>>is why pxb and pxb-pcie are he same device now (except bus type)

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

[Marcel Apfelbaum]

On 11/16/2015 12:59 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2015 at 12:39:45PM +0200, Marcel Apfelbaum wrote:
>> On 11/16/2015 12:37 PM, Michael S. Tsirkin wrote:
>>> On Mon, Nov 16, 2015 at 12:34:11PM +0200, Marcel Apfelbaum wrote:
>>>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>>>> What would you lose?  Hotplug?
>>>>>>
>>>>>> Without the bridge? Yes. However the user can add it manually the
>>>>>> pci-bridge and have it anyway.
>>>>>
>>>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>>>> the point that I wonder if we should rename the device and call the old
>>>>> one a failed experiment.
>>>>>
>>>>
>>>> I guess we can rename the pxb to extra-root or something, but in this way
>>>> will have a deprecated/duplicated device to support and kill in the future.
>>>>
>>>> Why not use the compat property as it is?
>>>> Again, the command line *remains* the same, the difference is where the
>>>> devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
>>>> or on the root bus itself (QEMU >= 2.5).
>>>>
>>>> I know is guest visible, but the guest will see one of them depending on the machine type.
>>>>
>>>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have nothing against it,
>>>> but because the implementation is *exactly* the same I think we should gain more
>>>> by maintaining one device.
>>>>
>>>>
>>>> Thanks,
>>>> Marcel
>>>
>>> Yes, I think you want a new "pci-extender" device which is just the extender.
>>> Then existing pxb will create both it and the bridge behind it.
>>> Maybe creating pxb which is extender+bridge was a mistake, I don't know,
>>> but we shipped it in QEMU so we support it.
>>
>> OK, but this device will be both pci/pcie,  depending on the machine type right?
>> No need to split it too?
>>
>> Thanks,
>> Marcel
>
> It's ok to have a single device but I don't like
> tying it to a machine type, much.
> It's always integrated within a RC, right?
> Can you go by the type of the parent device?

Yes and this is what I am doing, looking for the bus the pxb is on and not for the machine type.

I'll start working on it,
Thanks,
Marcel

>
>>
>>>
>>>>
>>>>> Paolo
>>>>>
>>>>>> I wanted to get rid of the internal pci-bridge as a default, and this
>>>>>> is why pxb and pxb-pcie are he same device now (except bus type)

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

[Paolo Bonzini]

On 16/11/2015 11:39, Marcel Apfelbaum wrote:
>> Yes, I think you want a new "pci-extender" device which is just the
>> extender.
>> Then existing pxb will create both it and the bridge behind it.
>> Maybe creating pxb which is extender+bridge was a mistake, I don't know,
>> but we shipped it in QEMU so we support it.
> 
> OK, but this device will be both pci/pcie,  depending on the machine
> type right?
> No need to split it too?

No, I think there's no need to split it.  It can be a property.

Paolo

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

[Laine Stump]

On 11/16/2015 05:37 AM, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2015 at 12:34:11PM +0200, Marcel Apfelbaum wrote:
>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>> What would you lose?  Hotplug?
>>>>
>>>> Without the bridge? Yes. However the user can add it manually the
>>>> pci-bridge and have it anyway.
>>>
>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>> the point that I wonder if we should rename the device and call the old
>>> one a failed experiment.
>>>
>>
>> I guess we can rename the pxb to extra-root or something, but in this way
>> will have a deprecated/duplicated device to support and kill in the future.
>>
>> Why not use the compat property as it is?
>> Again, the command line *remains* the same, the difference is where the
>> devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
>> or on the root bus itself (QEMU >= 2.5).
>>
>> I know is guest visible, but the guest will see one of them depending on the machine type.
>>
>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have nothing against it,
>> but because the implementation is *exactly* the same I think we should gain more
>> by maintaining one device.
>>
>>
>> Thanks,
>> Marcel
>
> Yes, I think you want a new "pci-extender" device which is just the extender.
> Then existing pxb will create both it and the bridge behind it.
> Maybe creating pxb which is extender+bridge was a mistake, I don't know,
> but we shipped it in QEMU so we support it.

At least from libvirt's POV, pxb support hasn't yet been added, so even 
though qemu has shipped it, libvirt (and thus ovirt and openstack at 
least) hasn't used it, and from the sounds of this discussion, it 
probably won't; we'll just directly support the new model for 440fx and q35.

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

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>
>>
>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>> What would you lose?  Hotplug?
>>>
>>> Without the bridge? Yes. However the user can add it manually the
>>> pci-bridge and have it anyway.
>>
>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>> the point that I wonder if we should rename the device and call the old
>> one a failed experiment.
>>
>
> I guess we can rename the pxb to extra-root or something, but in this way
> will have a deprecated/duplicated device to support and kill in the future.
>
> Why not use the compat property as it is?
> Again, the command line *remains* the same, the difference is where the
> devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
> or on the root bus itself (QEMU >= 2.5).
>
> I know is guest visible, but the guest will see one of them depending
> on the machine type.
>
> Regarding the splitting of pxb into 2 devices (pci/pcie), I have
> nothing against it,
> but because the implementation is *exactly* the same I think we should gain more
> by maintaining one device.

I have no opinion on two devices vs. one device + property in this
particular case, I just want to interject that I'd expect the difference
in maintaince to be negligible.

A second device basically takes a copy of the TypeInfo with some
(trivial) init function to make it different.  Might be a few more lines
of code than adding a property, but in complexity, it's a wash.

In case you plan to get rid of the old variant: with two devices, you
deprecate and later delete the old device.  With device + property, you
deprecate setting the property, and later delete it.  The former might
be a bit easier to document.

>>> I wanted to get rid of the internal pci-bridge as a default, and this
>>> is why pxb and pxb-pcie are he same device now (except bus type)

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

[Marcel Apfelbaum]

On 11/17/2015 10:15 AM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>> What would you lose?  Hotplug?
>>>>
>>>> Without the bridge? Yes. However the user can add it manually the
>>>> pci-bridge and have it anyway.
>>>
>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>> the point that I wonder if we should rename the device and call the old
>>> one a failed experiment.
>>>
>>
>> I guess we can rename the pxb to extra-root or something, but in this way
>> will have a deprecated/duplicated device to support and kill in the future.
>>
>> Why not use the compat property as it is?
>> Again, the command line *remains* the same, the difference is where the
>> devices associated with the pxb will land: on the secondary bus (for QEMU < 2.5)
>> or on the root bus itself (QEMU >= 2.5).
>>
>> I know is guest visible, but the guest will see one of them depending
>> on the machine type.
>>
>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have
>> nothing against it,
>> but because the implementation is *exactly* the same I think we should gain more
>> by maintaining one device.
>
> I have no opinion on two devices vs. one device + property in this
> particular case, I just want to interject that I'd expect the difference
> in maintaince to be negligible.
>
> A second device basically takes a copy of the TypeInfo with some
> (trivial) init function to make it different.  Might be a few more lines
> of code than adding a property, but in complexity, it's a wash.
>
> In case you plan to get rid of the old variant: with two devices, you
> deprecate and later delete the old device.  With device + property, you
> deprecate setting the property, and later delete it.  The former might
> be a bit easier to document.

Hi Markus,
Thank you for the review.

Following all the comments I have received, I am going to:

1. Leave the old device (pxb) as is and mark it as deprecated. Maybe
you can point me on the cleanest way to mark a device as deprecated?

2. Create a new device (pci-expander) that will behave as a:
    - PCI root, if the machine's bus 0 is legacy PCI.
    - PCI express root (Root Complex), if machine's bus 0 is PCI Express.

This way I will not need two device and not even a property for (pci/pcie).
We don't really need an extra PCI root on a PCIe machine and vice versa.

By the way, following the same concept I converted the virtio devices to PCIe devices.
I didn't want a new set of devices.

Thanks,
Marcel

>
>>>> I wanted to get rid of the internal pci-bridge as a default, and this
>>>> is why pxb and pxb-pcie are he same device now (except bus type)

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

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 11/17/2015 10:15 AM, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>
>>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>>> What would you lose?  Hotplug?
>>>>>
>>>>> Without the bridge? Yes. However the user can add it manually the
>>>>> pci-bridge and have it anyway.
>>>>
>>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>>> the point that I wonder if we should rename the device and call the old
>>>> one a failed experiment.
>>>>
>>>
>>> I guess we can rename the pxb to extra-root or something, but in this way
>>> will have a deprecated/duplicated device to support and kill in the future.
>>>
>>> Why not use the compat property as it is?
>>> Again, the command line *remains* the same, the difference is where the
>>> devices associated with the pxb will land: on the secondary bus
>>> (for QEMU < 2.5)
>>> or on the root bus itself (QEMU >= 2.5).
>>>
>>> I know is guest visible, but the guest will see one of them depending
>>> on the machine type.
>>>
>>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have
>>> nothing against it,
>>> but because the implementation is *exactly* the same I think we
>>> should gain more
>>> by maintaining one device.
>>
>> I have no opinion on two devices vs. one device + property in this
>> particular case, I just want to interject that I'd expect the difference
>> in maintaince to be negligible.
>>
>> A second device basically takes a copy of the TypeInfo with some
>> (trivial) init function to make it different.  Might be a few more lines
>> of code than adding a property, but in complexity, it's a wash.
>>
>> In case you plan to get rid of the old variant: with two devices, you
>> deprecate and later delete the old device.  With device + property, you
>> deprecate setting the property, and later delete it.  The former might
>> be a bit easier to document.
>
> Hi Markus,
> Thank you for the review.
>
> Following all the comments I have received, I am going to:
>
> 1. Leave the old device (pxb) as is and mark it as deprecated. Maybe
> you can point me on the cleanest way to mark a device as deprecated?

First, we need to decide whether they're deprecated or legacy.

Deprecated means their use is discouraged and they may go away
eventually.  Users are advised to switch to the replacement.

Legacy means we keep them around just for compatibility.  Existing uses
are just fine, but new use is discouraged.

We usually remove deprecated interfaces from help and documentation.
Sometimes we mark them deprecated for a while before we remove them.

Legacy interfaces stay documented, but should be clearly marked.

Right now, -device help shows all pluggable devices, i.e. the devices
that don't have cannot_instantiate_with_device_add_yet set.  In other
words, there's no way to suppress deprecated devices, yet.

Two ways to mark them:

1. Set DeviceClass member desc to a string containing "(deprecated)" or
   "(legacy)", respectively.

2. Create a new DeviceCategory, and move the device there.

I prefer 2.  You can easily turn it into a way to suppress deprecated
devices: just skip the "deprecated" category.

> 2. Create a new device (pci-expander) that will behave as a:
>    - PCI root, if the machine's bus 0 is legacy PCI.
>    - PCI express root (Root Complex), if machine's bus 0 is PCI Express.
>
> This way I will not need two device and not even a property for (pci/pcie).
> We don't really need an extra PCI root on a PCIe machine and vice versa.
>
> By the way, following the same concept I converted the virtio devices
> to PCIe devices.
> I didn't want a new set of devices.

Trust your judgement :)

[...]

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

[Marcel Apfelbaum]

On 11/17/2015 02:26 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 11/17/2015 10:15 AM, Markus Armbruster wrote:
>>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>>
>>>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>>>> What would you lose?  Hotplug?
>>>>>>
>>>>>> Without the bridge? Yes. However the user can add it manually the
>>>>>> pci-bridge and have it anyway.
>>>>>
>>>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>>>> the point that I wonder if we should rename the device and call the old
>>>>> one a failed experiment.
>>>>>
>>>>
>>>> I guess we can rename the pxb to extra-root or something, but in this way
>>>> will have a deprecated/duplicated device to support and kill in the future.
>>>>
>>>> Why not use the compat property as it is?
>>>> Again, the command line *remains* the same, the difference is where the
>>>> devices associated with the pxb will land: on the secondary bus
>>>> (for QEMU < 2.5)
>>>> or on the root bus itself (QEMU >= 2.5).
>>>>
>>>> I know is guest visible, but the guest will see one of them depending
>>>> on the machine type.
>>>>
>>>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have
>>>> nothing against it,
>>>> but because the implementation is *exactly* the same I think we
>>>> should gain more
>>>> by maintaining one device.
>>>
>>> I have no opinion on two devices vs. one device + property in this
>>> particular case, I just want to interject that I'd expect the difference
>>> in maintaince to be negligible.
>>>
>>> A second device basically takes a copy of the TypeInfo with some
>>> (trivial) init function to make it different.  Might be a few more lines
>>> of code than adding a property, but in complexity, it's a wash.
>>>
>>> In case you plan to get rid of the old variant: with two devices, you
>>> deprecate and later delete the old device.  With device + property, you
>>> deprecate setting the property, and later delete it.  The former might
>>> be a bit easier to document.
>>
>> Hi Markus,
>> Thank you for the review.
>>
>> Following all the comments I have received, I am going to:
>>
>> 1. Leave the old device (pxb) as is and mark it as deprecated. Maybe
>> you can point me on the cleanest way to mark a device as deprecated?
>
> First, we need to decide whether they're deprecated or legacy.
>
> Deprecated means their use is discouraged and they may go away
> eventually.  Users are advised to switch to the replacement.
>
> Legacy means we keep them around just for compatibility.  Existing uses
> are just fine, but new use is discouraged.
>
> We usually remove deprecated interfaces from help and documentation.
> Sometimes we mark them deprecated for a while before we remove them.
>
> Legacy interfaces stay documented, but should be clearly marked.
>
> Right now, -device help shows all pluggable devices, i.e. the devices
> that don't have cannot_instantiate_with_device_add_yet set.  In other
> words, there's no way to suppress deprecated devices, yet.
>
> Two ways to mark them:
>
> 1. Set DeviceClass member desc to a string containing "(deprecated)" or
>     "(legacy)", respectively.
>
> 2. Create a new DeviceCategory, and move the device there.
>
> I prefer 2.  You can easily turn it into a way to suppress deprecated
> devices: just skip the "deprecated" category.

Thanks Markus for the detailed information!

>
>> 2. Create a new device (pci-expander) that will behave as a:
>>     - PCI root, if the machine's bus 0 is legacy PCI.
>>     - PCI express root (Root Complex), if machine's bus 0 is PCI Express.
>>
>> This way I will not need two device and not even a property for (pci/pcie).
>> We don't really need an extra PCI root on a PCIe machine and vice versa.
>>
>> By the way, following the same concept I converted the virtio devices
>> to PCIe devices.
>> I didn't want a new set of devices.
>
> Trust your judgement :)

Re-thinking this whole thing, I have a new plan :)

We have the pxb device which is working just fine. Having the internal pci-bridge
by default is a good idea (hot-plug support, smaller ACPI tables).

Adding a new device, say "root-complex", that re-uses some of the pxb code
is the best way to go, and it will be easier to maintain when those devices
will diverge.

Thanks,
Marcel

>
> [...]
>