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

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

[Marcel Apfelbaum]

Note:
I took the liberty to CC all the reviewers that took their time
and had a look on the previous version, thanks!!

The PXB host bridge provides a way to have multiple PCI hierarchies (PCI root buses).
This series introduces the pxb-pcie counterpart for PCI Express machines(Currently Q35).

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.

v2 -> v3:
 Addressed Eduardo Habkost comments:
 - Added a bus property to PC machines and use it when querying bus 0.
 Addressed comments from multiple reviewers (Paolo,Markus,Gerd,Michael)
 - The issue was the backport compatibility when the PXB changes.
 - Following all the comments I chose:
   - Leave the PXB intact as it does the job and all its features
     (including the internal pci bridge) makes sense.
   - Add a new device that re-uses all the PXB code but is exposed as
     a different device to guests.
   - Once the functionality of the new device diverges we will have
     no problem to separate the code.

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 (3):
  hw/acpi: merge pxb adjacent memory/IO ranges
  hw/pxb: introduce pxb-pcie expander for PCIe machines
  hw/i386: extend pxb query for all PC machines

 hw/i386/acpi-build.c                | 126 +++++++++++++++++++++---------------
 hw/i386/pc.c                        |   2 +-
 hw/i386/pc_piix.c                   |   1 +
 hw/i386/pc_q35.c                    |   1 +
 hw/pci-bridge/pci_expander_bridge.c |  98 +++++++++++++++++++++++-----
 include/hw/i386/pc.h                |   1 +
 include/hw/pci/pci.h                |   1 +
 7 files changed, 163 insertions(+), 67 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH V3 1/3] hw/acpi: merge pxb adjacent memory/IO ranges

[Marcel Apfelbaum]

A generic PCI Bus Expander doesn't necessary have a built-in PCI bridge.
Int this case 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 V3 2/3] hw/pxb: introduce pxb-pcie expander for PCIe machines

[Marcel Apfelbaum]

The pxb-pcie is the counterpart of pxb for PCI express machines.
The new device re-uses the pxb code, but appears to the guests
as a different device. The pxb-pcie device does not have an internal
pci-pci bridge and exposes a PCIe root bus instead of a PCI one.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 98 +++++++++++++++++++++++++++++++------
 include/hw/pci/pci.h                |  1 +
 2 files changed, 84 insertions(+), 15 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 57f8a37..bb98fb2 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;
@@ -34,6 +37,9 @@ typedef struct PXBBus {
 #define TYPE_PXB_DEVICE "pxb"
 #define PXB_DEV(obj) OBJECT_CHECK(PXBDev, (obj), TYPE_PXB_DEVICE)
 
+#define TYPE_PXB_PCIE_DEVICE "pxb-pcie"
+#define PXB_PCIE_DEV(obj) OBJECT_CHECK(PXBDev, (obj), TYPE_PXB_PCIE_DEVICE)
+
 typedef struct PXBDev {
     /*< private >*/
     PCIDevice parent_obj;
@@ -43,13 +49,18 @@ typedef struct PXBDev {
     uint16_t numa_node;
 } PXBDev;
 
+static PXBDev *convert_to_pxb(PCIDevice *dev)
+{
+    return pci_bus_is_express(dev->bus) ? PXB_PCIE_DEV(dev) : PXB_DEV(dev);
+}
+
 static GList *pxb_dev_list;
 
 #define TYPE_PXB_HOST "pxb-host"
 
 static int pxb_bus_num(PCIBus *bus)
 {
-    PXBDev *pxb = PXB_DEV(bus->parent_dev);
+    PXBDev *pxb = convert_to_pxb(bus->parent_dev);
 
     return pxb->bus_nr;
 }
@@ -61,7 +72,7 @@ static bool pxb_is_root(PCIBus *bus)
 
 static uint16_t pxb_bus_numa_node(PCIBus *bus)
 {
-    PXBDev *pxb = PXB_DEV(bus->parent_dev);
+    PXBDev *pxb = convert_to_pxb(bus->parent_dev);
 
     return pxb->numa_node;
 }
@@ -82,10 +93,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;
@@ -103,7 +122,7 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
 
     pxb_host = PCI_HOST_BRIDGE(dev);
     pxb_bus = pxb_host->bus;
-    pxb_dev = PXB_DEV(pxb_bus->parent_dev);
+    pxb_dev = convert_to_pxb(pxb_bus->parent_dev);
     position = g_list_index(pxb_dev_list, pxb_dev);
     assert(position >= 0);
 
@@ -193,10 +212,10 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
            0;
 }
 
-static int pxb_dev_initfn(PCIDevice *dev)
+static int pxb_dev_init_common(PCIDevice *dev, bool pcie)
 {
-    PXBDev *pxb = PXB_DEV(dev);
-    DeviceState *ds, *bds;
+    PXBDev *pxb = convert_to_pxb(dev);
+    DeviceState *ds, *bds = NULL;
     PCIBus *bus;
     const char *dev_name = NULL;
 
@@ -211,18 +230,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 (pcie) {
+        bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
+    } else {
+        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);
+    }
 
     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 +252,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);
@@ -240,9 +264,19 @@ static int pxb_dev_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int pxb_dev_initfn(PCIDevice *dev)
+{
+    if (pci_bus_is_express(dev->bus)) {
+        error_report("pxb devices cannot reside on a PCIe bus!");
+        return -EINVAL;
+    }
+
+    return pxb_dev_init_common(dev, false);
+}
+
 static void pxb_dev_exitfn(PCIDevice *pci_dev)
 {
-    PXBDev *pxb = PXB_DEV(pci_dev);
+    PXBDev *pxb = convert_to_pxb(pci_dev);
 
     pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
 }
@@ -276,11 +310,45 @@ static const TypeInfo pxb_dev_info = {
     .class_init    = pxb_dev_class_init,
 };
 
+static int pxb_pcie_dev_initfn(PCIDevice *dev)
+{
+    if (!pci_bus_is_express(dev->bus)) {
+        error_report("pxb-pcie devices cannot reside on a PCI bus!");
+        return -EINVAL;
+    }
+
+    return pxb_dev_init_common(dev, true);
+}
+
+static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = pxb_pcie_dev_initfn;
+    k->exit = pxb_dev_exitfn;
+    k->vendor_id = PCI_VENDOR_ID_REDHAT;
+    k->device_id = PCI_DEVICE_ID_REDHAT_PXB_PCIE;
+    k->class_id = PCI_CLASS_BRIDGE_HOST;
+
+    dc->desc = "PCI Express Expander Bridge";
+    dc->props = pxb_dev_properties;
+}
+
+static const TypeInfo pxb_pcie_dev_info = {
+    .name          = TYPE_PXB_PCIE_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PXBDev),
+    .class_init    = pxb_pcie_dev_class_init,
+};
+
 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);
+    type_register_static(&pxb_pcie_dev_info);
 }
 
 type_init(pxb_register_types)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 379b6e1..dedf277 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -93,6 +93,7 @@
 #define PCI_DEVICE_ID_REDHAT_PCIE_HOST   0x0008
 #define PCI_DEVICE_ID_REDHAT_PXB         0x0009
 #define PCI_DEVICE_ID_REDHAT_BRIDGE_SEAT 0x000a
+#define PCI_DEVICE_ID_REDHAT_PXB_PCIE    0x000b
 #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
 
 #define FMT_PCIBUS                      PRIx64
-- 
2.1.0

[Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum]

Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 3 +--
 hw/i386/pc.c         | 2 +-
 hw/i386/pc_piix.c    | 1 +
 hw/i386/pc_q35.c     | 1 +
 include/hw/i386/pc.h | 1 +
 5 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 736b252..bca3f06 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 = PC_MACHINE(machine)->bus;
     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 5e20e07..07697ed 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 = PC_MACHINE(qdev_get_machine())->bus;
 
     if (bus) {
         int extra_hosts = 0;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 07d0baa..ca6ef9a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
                               pcms->below_4g_mem_size,
                               pcms->above_4g_mem_size,
                               pci_memory, ram_memory);
+        pcms->bus = pci_bus;
     } else {
         pci_bus = NULL;
         i440fx_state = NULL;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0fdae09..822b3e5 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
     qdev_init_nofail(DEVICE(q35_host));
     phb = PCI_HOST_BRIDGE(q35_host);
     host_bus = phb->bus;
+    pcms->bus = phb->bus;
     /* create ISA bus */
     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 854c330..e42771c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -41,6 +41,7 @@ struct PCMachineState {
     OnOffAuto smm;
     bool enforce_aligned_dimm;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
+    PCIBus *bus;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.1.0

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

[Laszlo Ersek]

Hello Marcel,

On 11/26/15 17:00, Marcel Apfelbaum wrote:
> Note:
> I took the liberty to CC all the reviewers that took their time
> and had a look on the previous version, thanks!!
> 
> The PXB host bridge provides a way to have multiple PCI hierarchies (PCI root buses).
> This series introduces the pxb-pcie counterpart for PCI Express machines(Currently Q35).
> 
> 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.
> 
> v2 -> v3:
>  Addressed Eduardo Habkost comments:
>  - Added a bus property to PC machines and use it when querying bus 0.
>  Addressed comments from multiple reviewers (Paolo,Markus,Gerd,Michael)
>  - The issue was the backport compatibility when the PXB changes.
>  - Following all the comments I chose:
>    - Leave the PXB intact as it does the job and all its features
>      (including the internal pci bridge) makes sense.
>    - Add a new device that re-uses all the PXB code but is exposed as
>      a different device to guests.
>    - Once the functionality of the new device diverges we will have
>      no problem to separate the code.


I don't think I can productively contribute to the review of this
series, but at least I'll try to follow the comments of others.

Also, your first patch looks like it touches code that is shared by the
i440fx PXB. I think it should cause no change in observable behavior, right?

Should I regression test it with OVMF? If so, that might take... forever. :(

On the other hand, if you have ACPI table dumps from within an i440fx
SeaBIOS Linux guest, both from before and after your QEMU patches, and
those dumps are identical, then that's good evidence against
regressions. (I tend to do such acpidump-based comparisons when messing
with ACPI builder code.)

Thanks (and sorry I can't help more ATM)
Laszlo


> 
> 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 (3):
>   hw/acpi: merge pxb adjacent memory/IO ranges
>   hw/pxb: introduce pxb-pcie expander for PCIe machines
>   hw/i386: extend pxb query for all PC machines
> 
>  hw/i386/acpi-build.c                | 126 +++++++++++++++++++++---------------
>  hw/i386/pc.c                        |   2 +-
>  hw/i386/pc_piix.c                   |   1 +
>  hw/i386/pc_q35.c                    |   1 +
>  hw/pci-bridge/pci_expander_bridge.c |  98 +++++++++++++++++++++++-----
>  include/hw/i386/pc.h                |   1 +
>  include/hw/pci/pci.h                |   1 +
>  7 files changed, 163 insertions(+), 67 deletions(-)
> 

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

[Marcel Apfelbaum]

On 11/26/2015 07:01 PM, Laszlo Ersek wrote:
> Hello Marcel,
>
> On 11/26/15 17:00, Marcel Apfelbaum wrote:
>> Note:
>> I took the liberty to CC all the reviewers that took their time
>> and had a look on the previous version, thanks!!
>>
>> The PXB host bridge provides a way to have multiple PCI hierarchies (PCI root buses).
>> This series introduces the pxb-pcie counterpart for PCI Express machines(Currently Q35).
>>
>> 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.
>>
>> v2 -> v3:
>>   Addressed Eduardo Habkost comments:
>>   - Added a bus property to PC machines and use it when querying bus 0.
>>   Addressed comments from multiple reviewers (Paolo,Markus,Gerd,Michael)
>>   - The issue was the backport compatibility when the PXB changes.
>>   - Following all the comments I chose:
>>     - Leave the PXB intact as it does the job and all its features
>>       (including the internal pci bridge) makes sense.
>>     - Add a new device that re-uses all the PXB code but is exposed as
>>       a different device to guests.
>>     - Once the functionality of the new device diverges we will have
>>       no problem to separate the code.
>
>
> I don't think I can productively contribute to the review of this
> series, but at least I'll try to follow the comments of others.

Hi Laszlo,

Thank you for your comments.

>
> Also, your first patch looks like it touches code that is shared by the
> i440fx PXB. I think it should cause no change in observable behavior, right?

This was the intention. Yes, it should be no change visible to the guest.

>
> Should I regression test it with OVMF? If so, that might take... forever. :(

I am planning to do this myself, I might ping you if I can't compile/run it.
I also think that the new device will work out of the box with Q35 + OVMF.

>
> On the other hand, if you have ACPI table dumps from within an i440fx
> SeaBIOS Linux guest, both from before and after your QEMU patches, and
> those dumps are identical, then that's good evidence against
> regressions. (I tend to do such acpidump-based comparisons when messing
> with ACPI builder code.)

This is a good idea, I am going to compare the dumps and get back to you.

>
> Thanks (and sorry I can't help more ATM)

No problem, thank you for your time!
Marcel

> Laszlo
>
>
>>
>> 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 (3):
>>    hw/acpi: merge pxb adjacent memory/IO ranges
>>    hw/pxb: introduce pxb-pcie expander for PCIe machines
>>    hw/i386: extend pxb query for all PC machines
>>
>>   hw/i386/acpi-build.c                | 126 +++++++++++++++++++++---------------
>>   hw/i386/pc.c                        |   2 +-
>>   hw/i386/pc_piix.c                   |   1 +
>>   hw/i386/pc_q35.c                    |   1 +
>>   hw/pci-bridge/pci_expander_bridge.c |  98 +++++++++++++++++++++++-----
>>   include/hw/i386/pc.h                |   1 +
>>   include/hw/pci/pci.h                |   1 +
>>   7 files changed, 163 insertions(+), 67 deletions(-)
>>
>

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

[Igor Mammedov]

On Thu, 26 Nov 2015 20:35:59 +0200
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 11/26/2015 07:01 PM, Laszlo Ersek wrote:
> > Hello Marcel,
> >
> > On 11/26/15 17:00, Marcel Apfelbaum wrote:
> >> Note:
> >> I took the liberty to CC all the reviewers that took their time
> >> and had a look on the previous version, thanks!!
> >>
> >> The PXB host bridge provides a way to have multiple PCI hierarchies (PCI root buses).
> >> This series introduces the pxb-pcie counterpart for PCI Express machines(Currently Q35).
> >>
> >> 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.
> >>
> >> v2 -> v3:
> >>   Addressed Eduardo Habkost comments:
> >>   - Added a bus property to PC machines and use it when querying bus 0.
> >>   Addressed comments from multiple reviewers (Paolo,Markus,Gerd,Michael)
> >>   - The issue was the backport compatibility when the PXB changes.
> >>   - Following all the comments I chose:
> >>     - Leave the PXB intact as it does the job and all its features
> >>       (including the internal pci bridge) makes sense.
> >>     - Add a new device that re-uses all the PXB code but is exposed as
> >>       a different device to guests.
> >>     - Once the functionality of the new device diverges we will have
> >>       no problem to separate the code.
> >
> >
> > I don't think I can productively contribute to the review of this
> > series, but at least I'll try to follow the comments of others.
> 
> Hi Laszlo,
> 
> Thank you for your comments.
> 
> >
> > Also, your first patch looks like it touches code that is shared by the
> > i440fx PXB. I think it should cause no change in observable behavior, right?
> 
> This was the intention. Yes, it should be no change visible to the guest.
> 
> >
> > Should I regression test it with OVMF? If so, that might take... forever. :(
> 
> I am planning to do this myself, I might ping you if I can't compile/run it.
> I also think that the new device will work out of the box with Q35 + OVMF.
> 
> >
> > On the other hand, if you have ACPI table dumps from within an i440fx
> > SeaBIOS Linux guest, both from before and after your QEMU patches, and
> > those dumps are identical, then that's good evidence against
> > regressions. (I tend to do such acpidump-based comparisons when messing
> > with ACPI builder code.)
> 
> This is a good idea, I am going to compare the dumps and get back to you.
To do it, you can use patch from my drop_ASL_support_v1 branch:
"tests: acpi: print ASL diff in verbose mode"
https://github.com/imammedo/qemu/commit/2df59d77151029554a90ce53c059860babadaf30


> 
> >
> > Thanks (and sorry I can't help more ATM)
> 
> No problem, thank you for your time!
> Marcel
> 
> > Laszlo
> >
> >
> >>
> >> 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 (3):
> >>    hw/acpi: merge pxb adjacent memory/IO ranges
> >>    hw/pxb: introduce pxb-pcie expander for PCIe machines
> >>    hw/i386: extend pxb query for all PC machines
> >>
> >>   hw/i386/acpi-build.c                | 126 +++++++++++++++++++++---------------
> >>   hw/i386/pc.c                        |   2 +-
> >>   hw/i386/pc_piix.c                   |   1 +
> >>   hw/i386/pc_q35.c                    |   1 +
> >>   hw/pci-bridge/pci_expander_bridge.c |  98 +++++++++++++++++++++++-----
> >>   include/hw/i386/pc.h                |   1 +
> >>   include/hw/pci/pci.h                |   1 +
> >>   7 files changed, 163 insertions(+), 67 deletions(-)
> >>
> >
> 

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> Add bus property to PC machines and use it when looking
> for primary PCI root bus (bus 0).
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

I can't pretend I have reviewed the q35 part, but the changes are
an improvement to the existing code that depended on
find_i440fx().

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

BTW, what's missing to allow us to change acpi_set_pci_info() to
use PCMachine::bus instead of find_i440fx(), too? How much of the
PCI hotplug stuff is different in q35?

> ---
>  hw/i386/acpi-build.c | 3 +--
>  hw/i386/pc.c         | 2 +-
>  hw/i386/pc_piix.c    | 1 +
>  hw/i386/pc_q35.c     | 1 +
>  include/hw/i386/pc.h | 1 +
>  5 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 736b252..bca3f06 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 = PC_MACHINE(machine)->bus;
>      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 5e20e07..07697ed 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 = PC_MACHINE(qdev_get_machine())->bus;
>  
>      if (bus) {
>          int extra_hosts = 0;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 07d0baa..ca6ef9a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
>                                pcms->below_4g_mem_size,
>                                pcms->above_4g_mem_size,
>                                pci_memory, ram_memory);
> +        pcms->bus = pci_bus;
>      } else {
>          pci_bus = NULL;
>          i440fx_state = NULL;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0fdae09..822b3e5 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
>      qdev_init_nofail(DEVICE(q35_host));
>      phb = PCI_HOST_BRIDGE(q35_host);
>      host_bus = phb->bus;
> +    pcms->bus = phb->bus;
>      /* create ISA bus */
>      lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>                                            ICH9_LPC_FUNC), true,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 854c330..e42771c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -41,6 +41,7 @@ struct PCMachineState {
>      OnOffAuto smm;
>      bool enforce_aligned_dimm;
>      ram_addr_t below_4g_mem_size, above_4g_mem_size;
> +    PCIBus *bus;
>  };
>  
>  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> -- 
> 2.1.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum]

On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
>> Add bus property to PC machines and use it when looking
>> for primary PCI root bus (bus 0).
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> I can't pretend I have reviewed the q35 part, but the changes are
> an improvement to the existing code that depended on
> find_i440fx().
>
> Acked-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks!

>
> BTW, what's missing to allow us to change acpi_set_pci_info() to
> use PCMachine::bus instead of find_i440fx(), too? How much of the
> PCI hotplug stuff is different in q35?

It is pretty different.
i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
"native", no acpi info is necessary.

Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
cannot be hotplugged/unplugged right now.

Once we decide to add hotplug support for this scenario, maybe we can get rid of
find_i440fx().

Thanks,
Marcel

>
>> ---
>>   hw/i386/acpi-build.c | 3 +--
>>   hw/i386/pc.c         | 2 +-
>>   hw/i386/pc_piix.c    | 1 +
>>   hw/i386/pc_q35.c     | 1 +
>>   include/hw/i386/pc.h | 1 +
>>   5 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 736b252..bca3f06 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 = PC_MACHINE(machine)->bus;
>>       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 5e20e07..07697ed 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 = PC_MACHINE(qdev_get_machine())->bus;
>>
>>       if (bus) {
>>           int extra_hosts = 0;
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 07d0baa..ca6ef9a 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
>>                                 pcms->below_4g_mem_size,
>>                                 pcms->above_4g_mem_size,
>>                                 pci_memory, ram_memory);
>> +        pcms->bus = pci_bus;
>>       } else {
>>           pci_bus = NULL;
>>           i440fx_state = NULL;
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 0fdae09..822b3e5 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
>>       qdev_init_nofail(DEVICE(q35_host));
>>       phb = PCI_HOST_BRIDGE(q35_host);
>>       host_bus = phb->bus;
>> +    pcms->bus = phb->bus;
>>       /* create ISA bus */
>>       lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>>                                             ICH9_LPC_FUNC), true,
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 854c330..e42771c 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -41,6 +41,7 @@ struct PCMachineState {
>>       OnOffAuto smm;
>>       bool enforce_aligned_dimm;
>>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
>> +    PCIBus *bus;
>>   };
>>
>>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>> --
>> 2.1.0
>>
>

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

[Marcel Apfelbaum]

On 11/27/2015 07:04 PM, Igor Mammedov wrote:
> On Thu, 26 Nov 2015 20:35:59 +0200
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 11/26/2015 07:01 PM, Laszlo Ersek wrote:
>>> Hello Marcel,
>>>
>>> On 11/26/15 17:00, Marcel Apfelbaum wrote:
>>>> Note:
>>>> I took the liberty to CC all the reviewers that took their time
>>>> and had a look on the previous version, thanks!!
>>>>
>>>> The PXB host bridge provides a way to have multiple PCI hierarchies (PCI root buses).
>>>> This series introduces the pxb-pcie counterpart for PCI Express machines(Currently Q35).
>>>>
>>>> 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.
>>>>
>>>> v2 -> v3:
>>>>    Addressed Eduardo Habkost comments:
>>>>    - Added a bus property to PC machines and use it when querying bus 0.
>>>>    Addressed comments from multiple reviewers (Paolo,Markus,Gerd,Michael)
>>>>    - The issue was the backport compatibility when the PXB changes.
>>>>    - Following all the comments I chose:
>>>>      - Leave the PXB intact as it does the job and all its features
>>>>        (including the internal pci bridge) makes sense.
>>>>      - Add a new device that re-uses all the PXB code but is exposed as
>>>>        a different device to guests.
>>>>      - Once the functionality of the new device diverges we will have
>>>>        no problem to separate the code.
>>>
>>>
>>> I don't think I can productively contribute to the review of this
>>> series, but at least I'll try to follow the comments of others.
>>
>> Hi Laszlo,
>>
>> Thank you for your comments.
>>
>>>
>>> Also, your first patch looks like it touches code that is shared by the
>>> i440fx PXB. I think it should cause no change in observable behavior, right?
>>
>> This was the intention. Yes, it should be no change visible to the guest.
>>
>>>
>>> Should I regression test it with OVMF? If so, that might take... forever. :(
>>
>> I am planning to do this myself, I might ping you if I can't compile/run it.
>> I also think that the new device will work out of the box with Q35 + OVMF.
>>
>>>
>>> On the other hand, if you have ACPI table dumps from within an i440fx
>>> SeaBIOS Linux guest, both from before and after your QEMU patches, and
>>> those dumps are identical, then that's good evidence against
>>> regressions. (I tend to do such acpidump-based comparisons when messing
>>> with ACPI builder code.)
>>
>> This is a good idea, I am going to compare the dumps and get back to you.
> To do it, you can use patch from my drop_ASL_support_v1 branch:
> "tests: acpi: print ASL diff in verbose mode"
> https://github.com/imammedo/qemu/commit/2df59d77151029554a90ce53c059860babadaf30

Hi Igor,

Thank you for the pointer, goot to know we have this, but in my case in order to do it I need to
change the QEMU command line, re-create the expected asl file, then the expected
binary files, and in the end apply my patches and run the test with "V" environment variable.

But it may took less than loading the guest.

Thanks!
Marcel

>>> Thanks (and sorry I can't help more ATM)
>>
>> No problem, thank you for your time!
>> Marcel
>>
>>> Laszlo
>>>
>>>
>>>>
>>>> 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 (3):
>>>>     hw/acpi: merge pxb adjacent memory/IO ranges
>>>>     hw/pxb: introduce pxb-pcie expander for PCIe machines
>>>>     hw/i386: extend pxb query for all PC machines
>>>>
>>>>    hw/i386/acpi-build.c                | 126 +++++++++++++++++++++---------------
>>>>    hw/i386/pc.c                        |   2 +-
>>>>    hw/i386/pc_piix.c                   |   1 +
>>>>    hw/i386/pc_q35.c                    |   1 +
>>>>    hw/pci-bridge/pci_expander_bridge.c |  98 +++++++++++++++++++++++-----
>>>>    include/hw/i386/pc.h                |   1 +
>>>>    include/hw/pci/pci.h                |   1 +
>>>>    7 files changed, 163 insertions(+), 67 deletions(-)
>>>>
>>>
>>
>

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

[Marcel Apfelbaum]

On 11/26/2015 07:01 PM, Laszlo Ersek wrote:
> Hello Marcel,
>

[...] if you have ACPI table dumps from within an i440fx
> SeaBIOS Linux guest, both from before and after your QEMU patches, and
> those dumps are identical, then that's good evidence against
> regressions. (I tend to do such acpidump-based comparisons when messing
> with ACPI builder code.)
>

Hi,

OK, there are no functional differences between the SSDT before/after,
however the optimization made in patch "1/3 hw/acpi: merge pxb adjacent memory/IO ranges"
for pxb-pcies works also for pxb, which is a good thing.

SSDT before (only PXB differences) :
-----------------------------------

  Device (PC0A)
  {
    ...
     Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
             {
                 ...
                 DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                     0x00000000,         // Granularity
                     0xFE800000,         // Range Minimum
                     0xFE9FFFFF,         // Range Maximum
                     0x00000000,         // Translation Offset
                     0x00200000,         // Length
                     ,, , AddressRangeMemory, TypeStatic)
                 DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                     0x00000000,         // Granularity
                     0xFE000000,         // Range Minimum
                     0xFE7FFFFF,         // Range Maximum
                     0x00000000,         // Translation Offset
                     0x00800000,         // Length
                     ,, , AddressRangeMemory, TypeStatic)
                 ...
             })
  }

SSDT after:
------------

Device (PC0A)
{
   ...
   Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
      {
	...
	DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                     0x00000000,         // Granularity
                     0xFE000000,         // Range Minimum
                     0xFE9FFFFF,         // Range Maximum
                     0x00000000,         // Translation Offset
                     0x00A00000,         // Length
                     ,, , AddressRangeMemory, TypeStatic)
        ...
     })
}

As it can be seen, the optimization works also for PXB by merging the MEM regions.

Thanks,
Marcel

[...]

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

[Laszlo Ersek]

On 11/29/15 13:37, Marcel Apfelbaum wrote:
> On 11/26/2015 07:01 PM, Laszlo Ersek wrote:
>> Hello Marcel,
>>
> 
> [...] if you have ACPI table dumps from within an i440fx
>> SeaBIOS Linux guest, both from before and after your QEMU patches, and
>> those dumps are identical, then that's good evidence against
>> regressions. (I tend to do such acpidump-based comparisons when messing
>> with ACPI builder code.)
>>
> 
> Hi,
> 
> OK, there are no functional differences between the SSDT before/after,
> however the optimization made in patch "1/3 hw/acpi: merge pxb adjacent
> memory/IO ranges"
> for pxb-pcies works also for pxb, which is a good thing.
> 
> SSDT before (only PXB differences) :
> -----------------------------------
> 
>  Device (PC0A)
>  {
>    ...
>     Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>             {
>                 ...
>                 DWordMemory (ResourceProducer, PosDecode, MinFixed,
> MaxFixed, NonCacheable, ReadWrite,
>                     0x00000000,         // Granularity
>                     0xFE800000,         // Range Minimum
>                     0xFE9FFFFF,         // Range Maximum
>                     0x00000000,         // Translation Offset
>                     0x00200000,         // Length
>                     ,, , AddressRangeMemory, TypeStatic)
>                 DWordMemory (ResourceProducer, PosDecode, MinFixed,
> MaxFixed, NonCacheable, ReadWrite,
>                     0x00000000,         // Granularity
>                     0xFE000000,         // Range Minimum
>                     0xFE7FFFFF,         // Range Maximum
>                     0x00000000,         // Translation Offset
>                     0x00800000,         // Length
>                     ,, , AddressRangeMemory, TypeStatic)
>                 ...
>             })
>  }
> 
> SSDT after:
> ------------
> 
> Device (PC0A)
> {
>   ...
>   Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
>      {
>     ...
>     DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed,
> NonCacheable, ReadWrite,
>                     0x00000000,         // Granularity
>                     0xFE000000,         // Range Minimum
>                     0xFE9FFFFF,         // Range Maximum
>                     0x00000000,         // Translation Offset
>                     0x00A00000,         // Length
>                     ,, , AddressRangeMemory, TypeStatic)
>        ...
>     })
> }
> 
> As it can be seen, the optimization works also for PXB by merging the
> MEM regions.
> 
> Thanks,
> Marcel
> 
> [...]

Looks good and makes sense, thanks.

Laszlo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>Add bus property to PC machines and use it when looking
> >>for primary PCI root bus (bus 0).
> >>
> >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >
> >I can't pretend I have reviewed the q35 part, but the changes are
> >an improvement to the existing code that depended on
> >find_i440fx().
> >
> >Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Thanks!
> 
> >
> >BTW, what's missing to allow us to change acpi_set_pci_info() to
> >use PCMachine::bus instead of find_i440fx(), too? How much of the
> >PCI hotplug stuff is different in q35?
> 
> It is pretty different.
> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> "native", no acpi info is necessary.
> 
> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> cannot be hotplugged/unplugged right now.
> 
> Once we decide to add hotplug support for this scenario, maybe we can get rid of
> find_i440fx().

Thanks for the explanation. I wonder if there's a better way to
check if ACPI-based hotplug is needed by looking at
PCMachineState or PCIBus, so we don't couple the ACPI code to
piix.c.

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum]

On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
>> On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
>>> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
>>>> Add bus property to PC machines and use it when looking
>>>> for primary PCI root bus (bus 0).
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>> I can't pretend I have reviewed the q35 part, but the changes are
>>> an improvement to the existing code that depended on
>>> find_i440fx().
>>>
>>> Acked-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>> Thanks!
>>
>>>
>>> BTW, what's missing to allow us to change acpi_set_pci_info() to
>>> use PCMachine::bus instead of find_i440fx(), too? How much of the
>>> PCI hotplug stuff is different in q35?
>>
>> It is pretty different.
>> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
>> "native", no acpi info is necessary.
>>
>> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
>> cannot be hotplugged/unplugged right now.
>>
>> Once we decide to add hotplug support for this scenario, maybe we can get rid of
>> find_i440fx().
>
> Thanks for the explanation. I wonder if there's a better way to
> check if ACPI-based hotplug is needed by looking at
> PCMachineState or PCIBus, so we don't couple the ACPI code to
> piix.c.
>

I suppose we can do something about it, like adding a property to PCMachineState,
lets say bool acpi_hotplug and set it false for Q35.

Then we have:
     pcm = PC_MACHINE(current_machine);
     if(pcm->acpi_hotplug) {
         bus  = pcm->bus;
         ...
     }

Sounds acceptable? If yes, I'll send a patch on top since is not directly related.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> >On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> >>On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >>>On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>>>Add bus property to PC machines and use it when looking
> >>>>for primary PCI root bus (bus 0).
> >>>>
> >>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>
> >>>I can't pretend I have reviewed the q35 part, but the changes are
> >>>an improvement to the existing code that depended on
> >>>find_i440fx().
> >>>
> >>>Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >>Thanks!
> >>
> >>>
> >>>BTW, what's missing to allow us to change acpi_set_pci_info() to
> >>>use PCMachine::bus instead of find_i440fx(), too? How much of the
> >>>PCI hotplug stuff is different in q35?
> >>
> >>It is pretty different.
> >>i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> >>"native", no acpi info is necessary.
> >>
> >>Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> >>cannot be hotplugged/unplugged right now.
> >>
> >>Once we decide to add hotplug support for this scenario, maybe we can get rid of
> >>find_i440fx().
> >
> >Thanks for the explanation. I wonder if there's a better way to
> >check if ACPI-based hotplug is needed by looking at
> >PCMachineState or PCIBus, so we don't couple the ACPI code to
> >piix.c.
> >
> 
> I suppose we can do something about it, like adding a property to PCMachineState,
> lets say bool acpi_hotplug and set it false for Q35.
> 
> Then we have:
>     pcm = PC_MACHINE(current_machine);
>     if(pcm->acpi_hotplug) {
>         bus  = pcm->bus;
>         ...
>     }
> 
> Sounds acceptable? If yes, I'll send a patch on top since is not directly related.S

There's no existing field or method in PCIBus that can be already
used for that? If not, that sounds good to me. But I would move
the field to PCMachineClass instead of PCMachineState.

Would you like me to do it? I am planning to submit other changes
to remove q35/piix dependencies from acpi-build.c.

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum]

On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
> On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
>> On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
>>> On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
>>>> On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
>>>>> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
>>>>>> Add bus property to PC machines and use it when looking
>>>>>> for primary PCI root bus (bus 0).
>>>>>>
>>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>>>
>>>>> I can't pretend I have reviewed the q35 part, but the changes are
>>>>> an improvement to the existing code that depended on
>>>>> find_i440fx().
>>>>>
>>>>> Acked-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> BTW, what's missing to allow us to change acpi_set_pci_info() to
>>>>> use PCMachine::bus instead of find_i440fx(), too? How much of the
>>>>> PCI hotplug stuff is different in q35?
>>>>
>>>> It is pretty different.
>>>> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
>>>> "native", no acpi info is necessary.
>>>>
>>>> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
>>>> cannot be hotplugged/unplugged right now.
>>>>
>>>> Once we decide to add hotplug support for this scenario, maybe we can get rid of
>>>> find_i440fx().
>>>
>>> Thanks for the explanation. I wonder if there's a better way to
>>> check if ACPI-based hotplug is needed by looking at
>>> PCMachineState or PCIBus, so we don't couple the ACPI code to
>>> piix.c.
>>>
>>
>> I suppose we can do something about it, like adding a property to PCMachineState,
>> lets say bool acpi_hotplug and set it false for Q35.
>>
>> Then we have:
>>      pcm = PC_MACHINE(current_machine);
>>      if(pcm->acpi_hotplug) {
>>          bus  = pcm->bus;
>>          ...
>>      }
>>
>> Sounds acceptable? If yes, I'll send a patch on top since is not directly related.S
>
> There's no existing field or method in PCIBus that can be already
> used for that?

Hmm, you can derive the info you need from pci_bus_is_express.
If express-> no acpi_hotplug. This is not 100% true, but since
we don't support acpi hotplug on PCIe machines, it should be OK for now.

  If not, that sounds good to me. But I would move
> the field to PCMachineClass instead of PCMachineState.

Sure.

>
> Would you like me to do it? I am planning to submit other changes
> to remove q35/piix dependencies from acpi-build.c.
>

If you have the time, go for it :)
Is a "one liner", if applied on top of this series.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
> >On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> >>On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> >>>On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> >>>>On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >>>>>On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>>>>>Add bus property to PC machines and use it when looking
> >>>>>>for primary PCI root bus (bus 0).
> >>>>>>
> >>>>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>>>
> >>>>>I can't pretend I have reviewed the q35 part, but the changes are
> >>>>>an improvement to the existing code that depended on
> >>>>>find_i440fx().
> >>>>>
> >>>>>Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> >>>>
> >>>>Thanks!
> >>>>
> >>>>>
> >>>>>BTW, what's missing to allow us to change acpi_set_pci_info() to
> >>>>>use PCMachine::bus instead of find_i440fx(), too? How much of the
> >>>>>PCI hotplug stuff is different in q35?
> >>>>
> >>>>It is pretty different.
> >>>>i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> >>>>"native", no acpi info is necessary.
> >>>>
> >>>>Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> >>>>cannot be hotplugged/unplugged right now.
> >>>>
> >>>>Once we decide to add hotplug support for this scenario, maybe we can get rid of
> >>>>find_i440fx().
> >>>
> >>>Thanks for the explanation. I wonder if there's a better way to
> >>>check if ACPI-based hotplug is needed by looking at
> >>>PCMachineState or PCIBus, so we don't couple the ACPI code to
> >>>piix.c.
> >>>
> >>
> >>I suppose we can do something about it, like adding a property to PCMachineState,
> >>lets say bool acpi_hotplug and set it false for Q35.
> >>
> >>Then we have:
> >>     pcm = PC_MACHINE(current_machine);
> >>     if(pcm->acpi_hotplug) {
> >>         bus  = pcm->bus;
> >>         ...
> >>     }
> >>
> >>Sounds acceptable? If yes, I'll send a patch on top since is not directly related.S
> >
> >There's no existing field or method in PCIBus that can be already
> >used for that?
> 
> Hmm, you can derive the info you need from pci_bus_is_express.
> If express-> no acpi_hotplug. This is not 100% true, but since
> we don't support acpi hotplug on PCIe machines, it should be OK for now.

What about just checking if AcpiPmInfo.pcihp_io_base is set?

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum]

On 12/01/2015 05:09 PM, Eduardo Habkost wrote:
> On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote:
>> On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
>>> On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
>>>> On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
>>>>> On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
>>>>>> On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
>>>>>>> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
>>>>>>>> Add bus property to PC machines and use it when looking
>>>>>>>> for primary PCI root bus (bus 0).
>>>>>>>>
>>>>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>>>>>
>>>>>>> I can't pretend I have reviewed the q35 part, but the changes are
>>>>>>> an improvement to the existing code that depended on
>>>>>>> find_i440fx().
>>>>>>>
>>>>>>> Acked-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>>>
>>>>>>> BTW, what's missing to allow us to change acpi_set_pci_info() to
>>>>>>> use PCMachine::bus instead of find_i440fx(), too? How much of the
>>>>>>> PCI hotplug stuff is different in q35?
>>>>>>
>>>>>> It is pretty different.
>>>>>> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
>>>>>> "native", no acpi info is necessary.
>>>>>>
>>>>>> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
>>>>>> cannot be hotplugged/unplugged right now.
>>>>>>
>>>>>> Once we decide to add hotplug support for this scenario, maybe we can get rid of
>>>>>> find_i440fx().
>>>>>
>>>>> Thanks for the explanation. I wonder if there's a better way to
>>>>> check if ACPI-based hotplug is needed by looking at
>>>>> PCMachineState or PCIBus, so we don't couple the ACPI code to
>>>>> piix.c.
>>>>>
>>>>
>>>> I suppose we can do something about it, like adding a property to PCMachineState,
>>>> lets say bool acpi_hotplug and set it false for Q35.
>>>>
>>>> Then we have:
>>>>      pcm = PC_MACHINE(current_machine);
>>>>      if(pcm->acpi_hotplug) {
>>>>          bus  = pcm->bus;
>>>>          ...
>>>>      }
>>>>
>>>> Sounds acceptable? If yes, I'll send a patch on top since is not directly related.S
>>>
>>> There's no existing field or method in PCIBus that can be already
>>> used for that?
>>
>> Hmm, you can derive the info you need from pci_bus_is_express.
>> If express-> no acpi_hotplug. This is not 100% true, but since
>> we don't support acpi hotplug on PCIe machines, it should be OK for now.
>
> What about just checking if AcpiPmInfo.pcihp_io_base is set?
>

Because this contradicts the "do not probe for piix" requirement.
pcihp_io_base depends on piix query for pm (piix4_pm_find).
So pcihp_io_base is an i440fx only "artifact".

Also, acpi_set_pci_info is called before acpi_build that populates
acpi_get_pm_info. All of that can be taken care of, of course.

At the end of the day, as long as the functionality is preserved,
I personally have no objection in re-factoring.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 06:50:15PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 05:09 PM, Eduardo Habkost wrote:
> >On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote:
> >>On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
> >>>On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> >>>>On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> >>>>>On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> >>>>>>On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >>>>>>>On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>>>>>>>Add bus property to PC machines and use it when looking
> >>>>>>>>for primary PCI root bus (bus 0).
> >>>>>>>>
> >>>>>>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>>>>>
> >>>>>>>I can't pretend I have reviewed the q35 part, but the changes are
> >>>>>>>an improvement to the existing code that depended on
> >>>>>>>find_i440fx().
> >>>>>>>
> >>>>>>>Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> >>>>>>
> >>>>>>Thanks!
> >>>>>>
> >>>>>>>
> >>>>>>>BTW, what's missing to allow us to change acpi_set_pci_info() to
> >>>>>>>use PCMachine::bus instead of find_i440fx(), too? How much of the
> >>>>>>>PCI hotplug stuff is different in q35?
> >>>>>>
> >>>>>>It is pretty different.
> >>>>>>i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> >>>>>>"native", no acpi info is necessary.
> >>>>>>
> >>>>>>Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> >>>>>>cannot be hotplugged/unplugged right now.
> >>>>>>
> >>>>>>Once we decide to add hotplug support for this scenario, maybe we can get rid of
> >>>>>>find_i440fx().
> >>>>>
> >>>>>Thanks for the explanation. I wonder if there's a better way to
> >>>>>check if ACPI-based hotplug is needed by looking at
> >>>>>PCMachineState or PCIBus, so we don't couple the ACPI code to
> >>>>>piix.c.
> >>>>>
> >>>>
> >>>>I suppose we can do something about it, like adding a property to PCMachineState,
> >>>>lets say bool acpi_hotplug and set it false for Q35.
> >>>>
> >>>>Then we have:
> >>>>     pcm = PC_MACHINE(current_machine);
> >>>>     if(pcm->acpi_hotplug) {
> >>>>         bus  = pcm->bus;
> >>>>         ...
> >>>>     }
> >>>>
> >>>>Sounds acceptable? If yes, I'll send a patch on top since is not directly related.S
> >>>
> >>>There's no existing field or method in PCIBus that can be already
> >>>used for that?
> >>
> >>Hmm, you can derive the info you need from pci_bus_is_express.
> >>If express-> no acpi_hotplug. This is not 100% true, but since
> >>we don't support acpi hotplug on PCIe machines, it should be OK for now.
> >
> >What about just checking if AcpiPmInfo.pcihp_io_base is set?
> >
> 
> Because this contradicts the "do not probe for piix" requirement.
> pcihp_io_base depends on piix query for pm (piix4_pm_find).
> So pcihp_io_base is an i440fx only "artifact".
> 

Yes, but at least the piix4-specific code would be contained
inside acpi_get_pm_info(). (And making acpi_get_pm_info() generic
is also part of my plans.)

However, you have a good point:

> Also, acpi_set_pci_info is called before acpi_build that populates
> acpi_get_pm_info. All of that can be taken care of, of course.

So we need to be careful about ordering, there. But it looks
doable without adding yet another PCMachineState field.

> 
> At the end of the day, as long as the functionality is preserved,
> I personally have no objection in re-factoring.

Working on it. :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> Add bus property to PC machines and use it when looking
> for primary PCI root bus (bus 0).
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/i386/acpi-build.c | 3 +--
>  hw/i386/pc.c         | 2 +-
>  hw/i386/pc_piix.c    | 1 +
>  hw/i386/pc_q35.c     | 1 +
>  include/hw/i386/pc.h | 1 +
>  5 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 736b252..bca3f06 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 = PC_MACHINE(machine)->bus;

You can use acpi_get_i386_pci_host()->bus here, so we can reduce
the amount of PC-specific code inside acpi-build.c.

(Making acpi_get_i386_pci_host() more generic and not depend on
piix- and q35-specific checks is also on my plans)

>      if (bus) {
>          QLIST_FOREACH(bus, &bus->child, sibling) {
>              uint8_t bus_num = pci_bus_num(bus);
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum]

On 12/01/2015 08:20 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
>> Add bus property to PC machines and use it when looking
>> for primary PCI root bus (bus 0).
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/i386/acpi-build.c | 3 +--
>>   hw/i386/pc.c         | 2 +-
>>   hw/i386/pc_piix.c    | 1 +
>>   hw/i386/pc_q35.c     | 1 +
>>   include/hw/i386/pc.h | 1 +
>>   5 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 736b252..bca3f06 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 = PC_MACHINE(machine)->bus;
>
> You can use acpi_get_i386_pci_host()->bus here, so we can reduce
> the amount of PC-specific code inside acpi-build.c.
>
> (Making acpi_get_i386_pci_host() more generic and not depend on
> piix- and q35-specific checks is also on my plans)

Well, at this point, looking at the names PC_MACHINE and acpi_get_i386_pci_host,
I don't see much difference :)
I think we can change this later when acpi_get_i386_pci_host will be generic.

Thanks,
Marcel

>
>>       if (bus) {
>>           QLIST_FOREACH(bus, &bus->child, sibling) {
>>               uint8_t bus_num = pci_bus_num(bus);
> [...]
>

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 10:53:07PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 08:20 PM, Eduardo Habkost wrote:
> >On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>Add bus property to PC machines and use it when looking
> >>for primary PCI root bus (bus 0).
> >>
> >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>---
> >>  hw/i386/acpi-build.c | 3 +--
> >>  hw/i386/pc.c         | 2 +-
> >>  hw/i386/pc_piix.c    | 1 +
> >>  hw/i386/pc_q35.c     | 1 +
> >>  include/hw/i386/pc.h | 1 +
> >>  5 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>index 736b252..bca3f06 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 = PC_MACHINE(machine)->bus;
> >
> >You can use acpi_get_i386_pci_host()->bus here, so we can reduce
> >the amount of PC-specific code inside acpi-build.c.
> >
> >(Making acpi_get_i386_pci_host() more generic and not depend on
> >piix- and q35-specific checks is also on my plans)
> 
> Well, at this point, looking at the names PC_MACHINE and acpi_get_i386_pci_host,
> I don't see much difference :)

I'm a bit confused by how generic acpi-build.c is supposed to be.
It has some code that seems to be an attempt to be more generic
(e.g. the "if (pci_host)" check at build_ssdt()), but lots of
other code that only work with PC machines. Maybe we should stop
pretending and simply use PCMachineState everywhere inside
acpi-build.c?

> I think we can change this later when acpi_get_i386_pci_host will be generic.

No problem to me. :)

-- 
Eduardo