[Qemu-devel] [PATCH 0/7] qdev patches, batch #1

[Qemu-devel] [PATCH 0/7] qdev patches, batch #1

[Gerd Hoffmann]

  Hi,

This is a respin of my qdev patches.  Rebased after anthony merged tons
of patches last night.  As requested this is the first batch only,
featuring the qdev groundwork.  The pci sound driver conversion to qdev
is included as well as it is pretty straigtforward and should be rather
uncontroversial.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/8] qdev: update pci device registration.

[Gerd Hoffmann]

Makes pci_qdev_register take a PCIDeviceInfo struct instead of a bunch
of parameters.  Also adds config_read and config_write callbacks to
PCIDeviceInfo, so drivers needing these can be converted to the qdev
device API too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/e1000.c         |    8 +++++++-
 hw/eepro100.c      |   25 +++++++++++++++++++------
 hw/lsi53c895a.c    |    8 +++++++-
 hw/ne2000.c        |    8 +++++++-
 hw/pci.c           |   24 ++++++++++--------------
 hw/pci.h           |   10 +++++++++-
 hw/pcnet.c         |    8 +++++++-
 hw/rtl8139.c       |    8 +++++++-
 hw/versatile_pci.c |    9 +++++++--
 hw/virtio-pci.c    |   31 +++++++++++++++++++++++--------
 10 files changed, 103 insertions(+), 36 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index da07c46..4ac8918 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1125,9 +1125,15 @@ static void pci_e1000_init(PCIDevice *pci_dev)
     e1000_reset(d);
 }
 
+static PCIDeviceInfo e1000_info = {
+    .qdev.name = "e1000",
+    .qdev.size = sizeof(E1000State),
+    .init      = pci_e1000_init,
+};
+
 static void e1000_register_devices(void)
 {
-    pci_qdev_register("e1000", sizeof(E1000State), pci_e1000_init);
+    pci_qdev_register(&e1000_info);
 }
 
 device_init(e1000_register_devices)
diff --git a/hw/eepro100.c b/hw/eepro100.c
index 0002140..85446ed 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1792,14 +1792,27 @@ static void pci_i82559er_init(PCIDevice *dev)
     nic_init(dev, i82559ER);
 }
 
+static PCIDeviceInfo eepro100_info[] = {
+    {
+        .qdev.name = "i82551",
+        .qdev.size = sizeof(PCIEEPRO100State),
+        .init      = pci_i82551_init,
+    },{
+        .qdev.name = "i82557b",
+        .qdev.size = sizeof(PCIEEPRO100State),
+        .init      = pci_i82557b_init,
+    },{
+        .qdev.name = "i82559er",
+        .qdev.size = sizeof(PCIEEPRO100State),
+        .init      = pci_i82559er_init,
+    },{
+        /* end of list */
+    }
+};
+
 static void eepro100_register_devices(void)
 {
-    pci_qdev_register("i82551", sizeof(PCIEEPRO100State),
-                      pci_i82551_init);
-    pci_qdev_register("i82557b", sizeof(PCIEEPRO100State),
-                      pci_i82557b_init);
-    pci_qdev_register("i82559er", sizeof(PCIEEPRO100State),
-                      pci_i82559er_init);
+    pci_qdev_register_many(eepro100_info);
 }
 
 device_init(eepro100_register_devices)
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 71f8281..516a468 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -2035,9 +2035,15 @@ static void lsi_scsi_init(PCIDevice *dev)
     scsi_bus_new(&dev->qdev, lsi_scsi_attach);
 }
 
+static PCIDeviceInfo lsi_info = {
+    .qdev.name = "lsi53c895a",
+    .qdev.size = sizeof(LSIState),
+    .init      = lsi_scsi_init,
+};
+
 static void lsi53c895a_register_devices(void)
 {
-    pci_qdev_register("lsi53c895a", sizeof(LSIState), lsi_scsi_init);
+    pci_qdev_register(&lsi_info);
 }
 
 device_init(lsi53c895a_register_devices);
diff --git a/hw/ne2000.c b/hw/ne2000.c
index 66ae9ab..66ff29d 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -832,9 +832,15 @@ static void pci_ne2000_init(PCIDevice *pci_dev)
     register_savevm("ne2000", -1, 3, ne2000_save, ne2000_load, s);
 }
 
+static PCIDeviceInfo ne2000_info = {
+    .qdev.name = "ne2k_pci",
+    .qdev.size = sizeof(PCINE2000State),
+    .init      = pci_ne2000_init,
+};
+
 static void ne2000_register_devices(void)
 {
-    pci_qdev_register("ne2k_pci", sizeof(PCINE2000State), pci_ne2000_init);
+    pci_qdev_register(&ne2000_info);
 }
 
 device_init(ne2000_register_devices)
diff --git a/hw/pci.c b/hw/pci.c
index 4458079..e82965f 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -874,11 +874,6 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
     return s->bus;
 }
 
-typedef struct {
-    DeviceInfo qdev;
-    pci_qdev_initfn init;
-} PCIDeviceInfo;
-
 static void pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     PCIDevice *pci_dev = (PCIDevice *)qdev;
@@ -889,25 +884,26 @@ static void pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
     bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
     devfn = qdev_get_prop_int(qdev, "devfn", -1);
     pci_dev = do_pci_register_device(pci_dev, bus, "FIXME", devfn,
-                                     NULL, NULL);//FIXME:config_read, config_write);
+                                     info->config_read, info->config_write);
     assert(pci_dev);
     info->init(pci_dev);
 }
 
-void pci_qdev_register(const char *name, int size, pci_qdev_initfn init)
+void pci_qdev_register(PCIDeviceInfo *info)
 {
-    PCIDeviceInfo *info;
-
-    info = qemu_mallocz(sizeof(*info));
-    info->qdev.name = qemu_strdup(name);
-    info->qdev.size = size;
-    info->init = init;
     info->qdev.init = pci_qdev_init;
     info->qdev.bus_type = BUS_TYPE_PCI;
-
     qdev_register(&info->qdev);
 }
 
+void pci_qdev_register_many(PCIDeviceInfo *info)
+{
+    while (info->qdev.name) {
+        pci_qdev_register(info);
+        info++;
+    }
+}
+
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 {
     DeviceState *dev;
diff --git a/hw/pci.h b/hw/pci.h
index cb0e382..cbfea6a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -314,7 +314,15 @@ pci_config_set_class(uint8_t *pci_config, uint16_t val)
 }
 
 typedef void (*pci_qdev_initfn)(PCIDevice *dev);
-void pci_qdev_register(const char *name, int size, pci_qdev_initfn init);
+typedef struct {
+    DeviceInfo qdev;
+    pci_qdev_initfn init;
+    PCIConfigReadFunc *config_read;
+    PCIConfigWriteFunc *config_write;
+} PCIDeviceInfo;
+
+void pci_qdev_register(PCIDeviceInfo *info);
+void pci_qdev_register_many(PCIDeviceInfo *info);
 
 PCIDevice *pci_create(const char *name, const char *devaddr);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
diff --git a/hw/pcnet.c b/hw/pcnet.c
index a184146..4519780 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -2143,9 +2143,15 @@ static void lance_init(SysBusDevice *dev)
 }
 #endif /* TARGET_SPARC */
 
+static PCIDeviceInfo pcnet_info = {
+    .qdev.name = "pcnet",
+    .qdev.size = sizeof(PCIPCNetState),
+    .init      = pci_pcnet_init,
+};
+
 static void pcnet_register_devices(void)
 {
-    pci_qdev_register("pcnet", sizeof(PCIPCNetState), pci_pcnet_init);
+    pci_qdev_register(&pcnet_info);
 #if defined (TARGET_SPARC) && !defined(TARGET_SPARC64)
     sysbus_register_dev("lance", sizeof(SysBusPCNetState), lance_init);
 #endif
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 2280018..91165db 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3499,9 +3499,15 @@ static void pci_rtl8139_init(PCIDevice *dev)
 #endif /* RTL8139_ONBOARD_TIMER */
 }
 
+static PCIDeviceInfo rtl8139_info = {
+    .qdev.name = "rtl8139",
+    .qdev.size = sizeof(PCIRTL8139State),
+    .init      = pci_rtl8139_init,
+};
+
 static void rtl8139_register_devices(void)
 {
-    pci_qdev_register("rtl8139", sizeof(PCIRTL8139State), pci_rtl8139_init);
+    pci_qdev_register(&rtl8139_info);
 }
 
 device_init(rtl8139_register_devices)
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index e89add1..5eb2625 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -153,13 +153,18 @@ static void versatile_pci_host_init(PCIDevice *d)
     d->config[0x0D] = 0x10; // latency_timer
 }
 
+static PCIDeviceInfo versatile_pci_host_info = {
+    .qdev.name = "versatile_pci_host",
+    .qdev.size = sizeof(PCIDevice),
+    .init      = versatile_pci_host_init,
+};
+
 static void versatile_pci_register_devices(void)
 {
     sysbus_register_dev("versatile_pci", sizeof(PCIVPBState), pci_vpb_init);
     sysbus_register_dev("realview_pci", sizeof(PCIVPBState),
                         pci_realview_init);
-    pci_qdev_register("versatile_pci_host", sizeof(PCIDevice),
-                      versatile_pci_host_init);
+    pci_qdev_register(&versatile_pci_host_info);
 }
 
 device_init(versatile_pci_register_devices)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f7da503..39e290d 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -466,16 +466,31 @@ static void virtio_balloon_init_pci(PCIDevice *pci_dev)
                     0x00);
 }
 
+static PCIDeviceInfo virtio_info[] = {
+    {
+        .qdev.name = "virtio-blk-pci",
+        .qdev.size = sizeof(VirtIOPCIProxy),
+        .init      = virtio_blk_init_pci,
+    },{
+        .qdev.name = "virtio-net-pci",
+        .qdev.size = sizeof(VirtIOPCIProxy),
+        .init      = virtio_net_init_pci,
+    },{
+        .qdev.name = "virtio-console-pci",
+        .qdev.size = sizeof(VirtIOPCIProxy),
+        .init      = virtio_console_init_pci,
+    },{
+        .qdev.name = "virtio-balloon-pci",
+        .qdev.size = sizeof(VirtIOPCIProxy),
+        .init      = virtio_balloon_init_pci,
+    },{
+        /* end of list */
+    }
+};
+
 static void virtio_pci_register_devices(void)
 {
-    pci_qdev_register("virtio-blk-pci", sizeof(VirtIOPCIProxy),
-                      virtio_blk_init_pci);
-    pci_qdev_register("virtio-net-pci", sizeof(VirtIOPCIProxy),
-                      virtio_net_init_pci);
-    pci_qdev_register("virtio-console-pci", sizeof(VirtIOPCIProxy),
-                      virtio_console_init_pci);
-    pci_qdev_register("virtio-balloon-pci", sizeof(VirtIOPCIProxy),
-                      virtio_balloon_init_pci);
+    pci_qdev_register_many(virtio_info);
 }
 
 device_init(virtio_pci_register_devices)
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/8] qdev: replace bus_type enum with bus_info struct.

[Gerd Hoffmann]

BusInfo is filled with name and size (pretty much like I did for
DeviceInfo as well).  There is also a function pointer to print
bus-specific device information to the monitor.  sysbus is hooked
up there, I've also added a print function for PCI.

Device creation is slightly modified as well:  The device type search
loop now also checks the bus type while scanning the list instead of
complaining thereafter in case of a mismatch.  This effectively gives
each bus a private namespace for device names.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i2c.c    |   10 +++++++---
 hw/pci.c    |   53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/qdev.c   |   58 ++++++++++++++++++++--------------------------------------
 hw/qdev.h   |   23 +++++++++++------------
 hw/ssi.c    |    9 +++++++--
 hw/sysbus.c |   12 ++++++++++--
 6 files changed, 103 insertions(+), 62 deletions(-)

diff --git a/hw/i2c.c b/hw/i2c.c
index 838f40f..98aa7fc 100644
--- a/hw/i2c.c
+++ b/hw/i2c.c
@@ -17,6 +17,11 @@ struct i2c_bus
     int saved_address;
 };
 
+static struct BusInfo i2c_bus_info = {
+    .name = "I2C",
+    .size = sizeof(i2c_bus),
+};
+
 static void i2c_bus_save(QEMUFile *f, void *opaque)
 {
     i2c_bus *bus = (i2c_bus *)opaque;
@@ -44,8 +49,7 @@ i2c_bus *i2c_init_bus(DeviceState *parent, const char *name)
 {
     i2c_bus *bus;
 
-    bus = FROM_QBUS(i2c_bus, qbus_create(BUS_TYPE_I2C, sizeof(i2c_bus),
-                                         parent, name));
+    bus = FROM_QBUS(i2c_bus, qbus_create(&i2c_bus_info, parent, name));
     register_savevm("i2c_bus", -1, 1, i2c_bus_save, i2c_bus_load, bus);
     return bus;
 }
@@ -156,7 +160,7 @@ void i2c_register_slave(I2CSlaveInfo *info)
 {
     assert(info->qdev.size >= sizeof(i2c_slave));
     info->qdev.init = i2c_slave_qdev_init;
-    info->qdev.bus_type = BUS_TYPE_I2C;
+    info->qdev.bus_info = &i2c_bus_info;
     qdev_register(&info->qdev);
 }
 
diff --git a/hw/pci.c b/hw/pci.c
index e82965f..fa618ee 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -45,7 +45,15 @@ struct PCIBus {
     /* The bus IRQ state is the logical OR of the connected devices.
        Keep a count of the number of devices with raised IRQs.  */
     int nirq;
-    int irq_count[];
+    int *irq_count;
+};
+
+static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
+
+static struct BusInfo pci_bus_info = {
+    .name  = "PCI",
+    .size  = sizeof(PCIBus),
+    .print = pcibus_dev_print,
 };
 
 static void pci_update_mappings(PCIDevice *d);
@@ -109,14 +117,13 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     PCIBus *bus;
     static int nbus = 0;
 
-    bus = FROM_QBUS(PCIBus, qbus_create(BUS_TYPE_PCI,
-                                        sizeof(PCIBus) + (nirq * sizeof(int)),
-                                        parent, name));
+    bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, parent, name));
     bus->set_irq = set_irq;
     bus->map_irq = map_irq;
     bus->irq_opaque = pic;
     bus->devfn_min = devfn_min;
     bus->nirq = nirq;
+    bus->irq_count = qemu_malloc(nirq * sizeof(bus->irq_count[0]));
     bus->next = first_bus;
     first_bus = bus;
     register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
@@ -892,7 +899,7 @@ static void pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
 void pci_qdev_register(PCIDeviceInfo *info)
 {
     info->qdev.init = pci_qdev_init;
-    info->qdev.bus_type = BUS_TYPE_PCI;
+    info->qdev.bus_info = &pci_bus_info;
     qdev_register(&info->qdev);
 }
 
@@ -991,3 +998,39 @@ uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
 {
     return pci_find_capability_list(pdev, cap_id, NULL);
 }
+
+static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
+{
+    PCIDevice *d = (PCIDevice *)dev;
+    const pci_class_desc *desc;
+    char ctxt[64];
+    PCIIORegion *r;
+    int i, class;
+
+    class = le16_to_cpu(*((uint16_t *)(d->config + PCI_CLASS_DEVICE)));
+    desc = pci_class_descriptions;
+    while (desc->desc && class != desc->class)
+        desc++;
+    if (desc->desc) {
+        snprintf(ctxt, sizeof(ctxt), "%s", desc->desc);
+    } else {
+        snprintf(ctxt, sizeof(ctxt), "Class %04x", class);
+    }
+
+    monitor_printf(mon, "%*sclass %s, addr %02x:%02x.%x, "
+                   "pci id %04x:%04x (sub %04x:%04x)\n",
+                   indent, "", ctxt,
+                   d->bus->bus_num, d->devfn >> 3, d->devfn & 7,
+                   le16_to_cpu(*((uint16_t *)(d->config + PCI_VENDOR_ID))),
+                   le16_to_cpu(*((uint16_t *)(d->config + PCI_DEVICE_ID))),
+                   le16_to_cpu(*((uint16_t *)(d->config + PCI_SUBSYSTEM_VENDOR_ID))),
+                   le16_to_cpu(*((uint16_t *)(d->config + PCI_SUBSYSTEM_ID))));
+    for (i = 0; i < PCI_NUM_REGIONS; i++) {
+        r = &d->io_regions[i];
+        if (!r->size)
+            continue;
+        monitor_printf(mon, "%*sbar %d: %s at 0x%x [0x%x]\n", indent, "",
+                       i, r->type & PCI_ADDRESS_SPACE_IO ? "i/o" : "mem",
+                       r->addr, r->addr + r->size - 1);
+    }
+}
diff --git a/hw/qdev.c b/hw/qdev.c
index 385e709..93e417d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -48,6 +48,7 @@ struct DeviceType {
 
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
+extern struct BusInfo system_bus_info;
 
 static DeviceType *device_type_list;
 
@@ -72,31 +73,26 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     DeviceType *t;
     DeviceState *dev;
 
-    for (t = device_type_list; t; t = t->next) {
-        if (strcmp(t->info->name, name) == 0) {
-            break;
+    if (!bus) {
+        if (!main_system_bus) {
+            main_system_bus = qbus_create(&system_bus_info, NULL, "main-system-bus");
         }
+        bus = main_system_bus;
+    }
+
+    for (t = device_type_list; t; t = t->next) {
+        if (t->info->bus_info != bus->info)
+            continue;
+        if (strcmp(t->info->name, name) != 0)
+            continue;
+        break;
     }
     if (!t) {
-        hw_error("Unknown device '%s'\n", name);
+        hw_error("Unknown device '%s' for bus '%s'\n", name, bus->info->name);
     }
 
     dev = qemu_mallocz(t->info->size);
     dev->type = t;
-
-    if (!bus) {
-        /* ???: This assumes system busses have no additional state.  */
-        if (!main_system_bus) {
-            main_system_bus = qbus_create(BUS_TYPE_SYSTEM, sizeof(BusState),
-                                          NULL, "main-system-bus");
-        }
-        bus = main_system_bus;
-    }
-    if (t->info->bus_type != bus->type) {
-        /* TODO: Print bus type names.  */
-        hw_error("Device '%s' on wrong bus type (%d/%d)", name,
-                 t->info->bus_type, bus->type);
-    }
     dev->parent_bus = bus;
     LIST_INSERT_HEAD(&bus->children, dev, sibling);
     return dev;
@@ -320,13 +316,12 @@ void scsi_bus_new(DeviceState *host, SCSIAttachFn attach)
    }
 }
 
-BusState *qbus_create(BusType type, size_t size,
-                      DeviceState *parent, const char *name)
+BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
 {
     BusState *bus;
 
-    bus = qemu_mallocz(size);
-    bus->type = type;
+    bus = qemu_mallocz(info->size);
+    bus->info = info;
     bus->parent = parent;
     bus->name = qemu_strdup(name);
     LIST_INIT(&bus->children);
@@ -336,14 +331,6 @@ BusState *qbus_create(BusType type, size_t size,
     return bus;
 }
 
-static const char *bus_type_names[] = {
-    [ BUS_TYPE_SYSTEM ] = "System",
-    [ BUS_TYPE_PCI ]    = "PCI",
-    [ BUS_TYPE_SCSI ]   = "SCSI",
-    [ BUS_TYPE_I2C ]    = "I2C",
-    [ BUS_TYPE_SSI ]    = "SSI",
-};
-
 #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
 static void qbus_print(Monitor *mon, BusState *bus, int indent);
 
@@ -377,13 +364,8 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
             break;
         }
     }
-    switch (dev->parent_bus->type) {
-    case BUS_TYPE_SYSTEM:
-        sysbus_dev_print(mon, dev, indent);
-        break;
-    default:
-        break;
-    }
+    if (dev->parent_bus->info->print)
+        dev->parent_bus->info->print(mon, dev, indent);
     LIST_FOREACH(child, &dev->child_bus, sibling) {
         qbus_print(mon, child, indent);
     }
@@ -395,7 +377,7 @@ static void qbus_print(Monitor *mon, BusState *bus, int indent)
 
     qdev_printf("bus: %s\n", bus->name);
     indent += 2;
-    qdev_printf("type %s\n", bus_type_names[bus->type]);
+    qdev_printf("type %s\n", bus->info->name);
     LIST_FOREACH(dev, &bus->children, sibling) {
         qdev_print(mon, dev, indent);
     }
diff --git a/hw/qdev.h b/hw/qdev.h
index ad10499..8b238d5 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -10,6 +10,8 @@ typedef struct DeviceProperty DeviceProperty;
 
 typedef struct BusState BusState;
 
+typedef struct BusInfo BusInfo;
+
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
@@ -25,18 +27,17 @@ struct DeviceState {
     LIST_ENTRY(DeviceState) sibling;
 };
 
-typedef enum {
-    BUS_TYPE_SYSTEM,
-    BUS_TYPE_PCI,
-    BUS_TYPE_SCSI,
-    BUS_TYPE_I2C,
-    BUS_TYPE_SSI
-} BusType;
+typedef void (*bus_dev_print)(Monitor *mon, DeviceState *dev, int indent);
+struct BusInfo {
+    const char *name;
+    size_t size;
+    bus_dev_print print;
+};
 
 struct BusState {
     DeviceState *parent;
+    BusInfo *info;
     const char *name;
-    BusType type;
     LIST_HEAD(, DeviceState) children;
     LIST_ENTRY(BusState) sibling;
 };
@@ -84,7 +85,7 @@ struct DeviceInfo {
 
     /* Private to qdev / bus.  */
     qdev_initfn init;
-    BusType bus_type;
+    BusInfo *bus_info;
 };
 
 void qdev_register(DeviceInfo *info);
@@ -116,14 +117,12 @@ void *qdev_get_prop_ptr(DeviceState *dev, const char *name);
 
 /*** BUS API. ***/
 
-BusState *qbus_create(BusType type, size_t size,
-                      DeviceState *parent, const char *name);
+BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
 /*** monitor commands ***/
 
 void do_info_qtree(Monitor *mon);
-void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 
 #endif
diff --git a/hw/ssi.c b/hw/ssi.c
index b0bcf97..a5133be 100644
--- a/hw/ssi.c
+++ b/hw/ssi.c
@@ -13,6 +13,11 @@ struct SSIBus {
     BusState qbus;
 };
 
+static struct BusInfo ssi_bus_info = {
+    .name = "SSI",
+    .size = sizeof(SSIBus),
+};
+
 static void ssi_slave_init(DeviceState *dev, DeviceInfo *base_info)
 {
     SSISlaveInfo *info = container_of(base_info, SSISlaveInfo, qdev);
@@ -33,7 +38,7 @@ void ssi_register_slave(SSISlaveInfo *info)
 {
     assert(info->qdev.size >= sizeof(SSISlave));
     info->qdev.init = ssi_slave_init;
-    info->qdev.bus_type = BUS_TYPE_SSI;
+    info->qdev.bus_info = &ssi_bus_info;
     qdev_register(&info->qdev);
 }
 
@@ -48,7 +53,7 @@ DeviceState *ssi_create_slave(SSIBus *bus, const char *name)
 SSIBus *ssi_create_bus(DeviceState *parent, const char *name)
 {
     BusState *bus;
-    bus = qbus_create(BUS_TYPE_SSI, sizeof(SSIBus), parent, name);
+    bus = qbus_create(&ssi_bus_info, parent, name);
     return FROM_QBUS(SSIBus, bus);
 }
 
diff --git a/hw/sysbus.c b/hw/sysbus.c
index ef3a701..13e563b 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -22,6 +22,14 @@
 #include "sysemu.h"
 #include "monitor.h"
 
+static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
+
+struct BusInfo system_bus_info = {
+    .name  = "System",
+    .size  = sizeof(BusState),
+    .print = sysbus_dev_print,
+};
+
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 {
     assert(n >= 0 && n < dev->num_irq);
@@ -108,7 +116,7 @@ static void sysbus_device_init(DeviceState *dev, DeviceInfo *base)
 void sysbus_register_withprop(SysBusDeviceInfo *info)
 {
     info->qdev.init = sysbus_device_init;
-    info->qdev.bus_type = BUS_TYPE_SYSTEM;
+    info->qdev.bus_info = &system_bus_info;
 
     assert(info->qdev.size >= sizeof(SysBusDevice));
     qdev_register(&info->qdev);
@@ -153,7 +161,7 @@ DeviceState *sysbus_create_varargs(const char *name,
     return dev;
 }
 
-void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
+static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     SysBusDevice *s = sysbus_from_qdev(dev);
     int i;
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/8] qdev: remove DeviceType

[Gerd Hoffmann]

The only purpose DeviceType serves is creating a linked list of
DeviceInfo structs.  This removes DeviceType and add a next field to
DeviceInfo instead, so the DeviceInfo structs can be changed that way.
Elimitates a pointless extra level of indirection.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   37 ++++++++++++++-----------------------
 hw/qdev.h |    7 +++----
 2 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 93e417d..3dc7076 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -41,28 +41,19 @@ struct DeviceProperty {
     DeviceProperty *next;
 };
 
-struct DeviceType {
-    DeviceInfo *info;
-    DeviceType *next;
-};
-
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
 extern struct BusInfo system_bus_info;
 
-static DeviceType *device_type_list;
+static DeviceInfo *device_info_list;
 
 /* Register a new device type.  */
 void qdev_register(DeviceInfo *info)
 {
-    DeviceType *t;
-
     assert(info->size >= sizeof(DeviceState));
 
-    t = qemu_mallocz(sizeof(DeviceType));
-    t->next = device_type_list;
-    device_type_list = t;
-    t->info = info;
+    info->next = device_info_list;
+    device_info_list = info;
 }
 
 /* Create a new device.  This only initializes the device state structure
@@ -70,7 +61,7 @@ void qdev_register(DeviceInfo *info)
    initialize the actual device emulation.  */
 DeviceState *qdev_create(BusState *bus, const char *name)
 {
-    DeviceType *t;
+    DeviceInfo *info;
     DeviceState *dev;
 
     if (!bus) {
@@ -80,19 +71,19 @@ DeviceState *qdev_create(BusState *bus, const char *name)
         bus = main_system_bus;
     }
 
-    for (t = device_type_list; t; t = t->next) {
-        if (t->info->bus_info != bus->info)
+    for (info = device_info_list; info != NULL; info = info->next) {
+        if (info->bus_info != bus->info)
             continue;
-        if (strcmp(t->info->name, name) != 0)
+        if (strcmp(info->name, name) != 0)
             continue;
         break;
     }
-    if (!t) {
+    if (!info) {
         hw_error("Unknown device '%s' for bus '%s'\n", name, bus->info->name);
     }
 
-    dev = qemu_mallocz(t->info->size);
-    dev->type = t;
+    dev = qemu_mallocz(info->size);
+    dev->info = info;
     dev->parent_bus = bus;
     LIST_INSERT_HEAD(&bus->children, dev, sibling);
     return dev;
@@ -103,7 +94,7 @@ DeviceState *qdev_create(BusState *bus, const char *name)
    calling this function.  */
 void qdev_init(DeviceState *dev)
 {
-    dev->type->info->init(dev, dev->type->info);
+    dev->info->init(dev, dev->info);
 }
 
 /* Unlink device from bus and free the structure.  */
@@ -165,7 +156,7 @@ CharDriverState *qdev_init_chardev(DeviceState *dev)
     static int next_serial;
     static int next_virtconsole;
     /* FIXME: This is a nasty hack that needs to go away.  */
-    if (strncmp(dev->type->info->name, "virtio", 6) == 0) {
+    if (strncmp(dev->info->name, "virtio", 6) == 0) {
         return virtcon_hds[next_virtconsole++];
     } else {
         return serial_hds[next_serial++];
@@ -338,7 +329,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     DeviceProperty *prop;
     BusState *child;
-    qdev_printf("dev: %s\n", dev->type->info->name);
+    qdev_printf("dev: %s\n", dev->info->name);
     indent += 2;
     if (dev->num_gpio_in) {
         qdev_printf("gpio-in %d\n", dev->num_gpio_in);
@@ -357,7 +348,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
             break;
         case PROP_TYPE_DEV:
             qdev_printf("prop-dev %s %s\n", prop->name,
-                        ((DeviceState *)prop->value.ptr)->type->info->name);
+                        ((DeviceState *)prop->value.ptr)->info->name);
             break;
         default:
             qdev_printf("prop-unknown%d %s\n", prop->type, prop->name);
diff --git a/hw/qdev.h b/hw/qdev.h
index 8b238d5..18321b3 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -4,7 +4,7 @@
 #include "hw.h"
 #include "sys-queue.h"
 
-typedef struct DeviceType DeviceType;
+typedef struct DeviceInfo DeviceInfo;
 
 typedef struct DeviceProperty DeviceProperty;
 
@@ -15,7 +15,7 @@ typedef struct BusInfo BusInfo;
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
-    DeviceType *type;
+    DeviceInfo *info;
     BusState *parent_bus;
     DeviceProperty *props;
     int num_gpio_out;
@@ -72,8 +72,6 @@ typedef struct {
     DevicePropType type;
 } DevicePropList;
 
-typedef struct DeviceInfo DeviceInfo;
-
 typedef void (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
 typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
               int unit);
@@ -86,6 +84,7 @@ struct DeviceInfo {
     /* Private to qdev / bus.  */
     qdev_initfn init;
     BusInfo *bus_info;
+    struct DeviceInfo *next;
 };
 
 void qdev_register(DeviceInfo *info);
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Gerd Hoffmann]

 * maintain a list of busses.
 * maintain bus numbers.
 * add function to find busses by type / name / number.
 * add monitor command to list busses.

Use case for the bus list is hot-plugging devices via monitor, where
qemu needs some way to find the bus specified in the monitor command.
The usb patches in the patch queue make use of this.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   41 ++++++++++++++++++++++++++++++++++++++++-
 hw/qdev.h |    5 +++++
 monitor.c |    2 ++
 3 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 3dc7076..1fb73b1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -306,22 +306,50 @@ void scsi_bus_new(DeviceState *host, SCSIAttachFn attach)
        attach(host, drives_table[index].bdrv, unit);
    }
 }
+static TAILQ_HEAD(, BusState) buslist = TAILQ_HEAD_INITIALIZER(buslist);
 
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
 {
     BusState *bus;
+    char buf[32];
+    int i;
 
     bus = qemu_mallocz(info->size);
     bus->info = info;
+    bus->busnr = info->next_busnr++;
     bus->parent = parent;
+    if (!name) {
+        /* default name: lowercase(type) + busnr, i.e. "scsi0" */
+        snprintf(buf, sizeof(buf), "%s%d", info->name, bus->busnr);
+        for (i = 0; buf[i]; i++)
+            buf[i] = tolower(buf[i]);
+        name = buf;
+    }
     bus->name = qemu_strdup(name);
     LIST_INIT(&bus->children);
     if (parent) {
         LIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
     }
+    TAILQ_INSERT_TAIL(&buslist, bus, next);
     return bus;
 }
 
+BusState *qbus_find(const char *type, const char *name, int busnr)
+{
+    BusState *bus;
+
+    TAILQ_FOREACH(bus, &buslist, next) {
+        if (type && strcmp(type, bus->info->name) != 0)
+            continue;
+        if (name && strcmp(name, bus->name) != 0)
+            continue;
+        if (busnr != -1 && busnr != bus->busnr)
+            continue;
+        return bus;
+    }
+    return NULL;
+}
+
 #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
 static void qbus_print(Monitor *mon, BusState *bus, int indent);
 
@@ -368,7 +396,7 @@ static void qbus_print(Monitor *mon, BusState *bus, int indent)
 
     qdev_printf("bus: %s\n", bus->name);
     indent += 2;
-    qdev_printf("type %s\n", bus->info->name);
+    qdev_printf("type %s, nr %d\n", bus->info->name, bus->busnr);
     LIST_FOREACH(dev, &bus->children, sibling) {
         qdev_print(mon, dev, indent);
     }
@@ -380,3 +408,14 @@ void do_info_qtree(Monitor *mon)
     if (main_system_bus)
         qbus_print(mon, main_system_bus, 0);
 }
+
+void do_info_qbus(Monitor *mon)
+{
+    BusState *bus;
+
+    TAILQ_FOREACH(bus, &buslist, next) {
+        monitor_printf(mon, "%s: type %s, nr %d, parent %s\n",
+                       bus->name, bus->info->name, bus->busnr,
+                       bus->parent ? bus->parent->info->name : "<none>");
+    }
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 18321b3..c4b04c1 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -32,14 +32,17 @@ struct BusInfo {
     const char *name;
     size_t size;
     bus_dev_print print;
+    int next_busnr;
 };
 
 struct BusState {
     DeviceState *parent;
     BusInfo *info;
+    int busnr;
     const char *name;
     LIST_HEAD(, DeviceState) children;
     LIST_ENTRY(BusState) sibling;
+    TAILQ_ENTRY(BusState) next;
 };
 
 /*** Board API.  This should go away once we have a machine config file.  ***/
@@ -117,11 +120,13 @@ void *qdev_get_prop_ptr(DeviceState *dev, const char *name);
 /*** BUS API. ***/
 
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
+BusState *qbus_find(const char *type, const char *name, int busnr);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
 /*** monitor commands ***/
 
 void do_info_qtree(Monitor *mon);
+void do_info_qbus(Monitor *mon);
 
 #endif
diff --git a/monitor.c b/monitor.c
index bad79fe..39ce99f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1759,6 +1759,8 @@ static const mon_cmd_t info_cmds[] = {
       "", "show balloon information" },
     { "qtree", "", do_info_qtree,
       "", "show device tree" },
+    { "qbus", "", do_info_qbus,
+      "", "show qdev bus list" },
     { NULL, NULL, },
 };
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/8] qdev/core: add monitor command to list all drivers

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   10 ++++++++++
 hw/qdev.h |    1 +
 monitor.c |    2 ++
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 1fb73b1..768431c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -419,3 +419,13 @@ void do_info_qbus(Monitor *mon)
                        bus->parent ? bus->parent->info->name : "<none>");
     }
 }
+
+void do_info_qdrv(Monitor *mon)
+{
+    DeviceInfo *info;
+
+    for (info = device_info_list; info != NULL; info = info->next) {
+        monitor_printf(mon, "name \"%s\", bus %s\n",
+                       info->name, info->bus_info->name);
+    }
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index c4b04c1..2f4e4a4 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -128,5 +128,6 @@ BusState *qbus_find(const char *type, const char *name, int busnr);
 
 void do_info_qtree(Monitor *mon);
 void do_info_qbus(Monitor *mon);
+void do_info_qdrv(Monitor *mon);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 39ce99f..5cacfa1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1761,6 +1761,8 @@ static const mon_cmd_t info_cmds[] = {
       "", "show device tree" },
     { "qbus", "", do_info_qbus,
       "", "show qdev bus list" },
+    { "qdrv", "", do_info_qdrv,
+      "", "show qdev driver list" },
     { NULL, NULL, },
 };
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/8] qdev/pci: misc fixes.

[Gerd Hoffmann]

 * use core busnr.
 * fix secondary bus setup.
 * use base->name instead of "FIXME" for device name.
   Yes, the device name is redundant.  Only for drivers converted
   to qdev already though.  Once all drivers are converted we can
   and should kill it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index fa618ee..8edbad8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -115,7 +115,6 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          qemu_irq *pic, int devfn_min, int nirq)
 {
     PCIBus *bus;
-    static int nbus = 0;
 
     bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, parent, name));
     bus->set_irq = set_irq;
@@ -126,7 +125,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     bus->irq_count = qemu_malloc(nirq * sizeof(bus->irq_count[0]));
     bus->next = first_bus;
     first_bus = bus;
-    register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
+    register_savevm("PCIBUS", bus->qbus.busnr, 1, pcibus_save, pcibus_load, bus);
     qemu_register_reset(pci_bus_reset, bus);
     return bus;
 }
@@ -134,7 +133,8 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
 static PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq)
 {
     PCIBus *bus;
-    bus = qemu_mallocz(sizeof(PCIBus));
+
+    bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, &dev->qdev, NULL));
     bus->map_irq = map_irq;
     bus->parent_dev = dev;
     bus->next = dev->bus->next;
@@ -890,7 +890,7 @@ static void pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
 
     bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
     devfn = qdev_get_prop_int(qdev, "devfn", -1);
-    pci_dev = do_pci_register_device(pci_dev, bus, "FIXME", devfn,
+    pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
                                      info->config_read, info->config_write);
     assert(pci_dev);
     info->init(pci_dev);
-- 
1.6.2.5

[Qemu-devel] [PATCH 7/8] qdev: convert es1370.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/es1370.c |   41 ++++++++++++++++++++++-------------------
 1 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/hw/es1370.c b/hw/es1370.c
index f730766..9e860f9 100644
--- a/hw/es1370.c
+++ b/hw/es1370.c
@@ -1005,27 +1005,12 @@ static void es1370_on_reset (void *opaque)
     es1370_reset (s);
 }
 
-int es1370_init (PCIBus *bus)
+static void es1370_initfn(PCIDevice *dev)
 {
-    PCIES1370State *d;
-    ES1370State *s;
-    uint8_t *c;
-
-    if (!bus) {
-        dolog ("No PCI bus\n");
-        return -1;
-    }
-
-    d = (PCIES1370State *) pci_register_device (bus, "ES1370",
-                                                sizeof (PCIES1370State),
-                                                -1, NULL, NULL);
-
-    if (!d) {
-        AUD_log (NULL, "Failed to register PCI device for ES1370\n");
-        return -1;
-    }
+    PCIES1370State *d = DO_UPCAST(PCIES1370State, dev, dev);
+    ES1370State *s = &d->es1370;
+    uint8_t *c = d->dev.config;
 
-    c = d->dev.config;
     pci_config_set_vendor_id (c, PCI_VENDOR_ID_ENSONIQ);
     pci_config_set_device_id (c, PCI_DEVICE_ID_ENSONIQ_ES1370);
     c[0x07] = 2 << 1;
@@ -1059,5 +1044,23 @@ int es1370_init (PCIBus *bus)
 
     AUD_register_card ("es1370", &s->card);
     es1370_reset (s);
+}
+
+int es1370_init (PCIBus *bus)
+{
+    pci_create_simple(bus, -1, "ES1370");
     return 0;
 }
+
+static PCIDeviceInfo es1370_info = {
+    .qdev.name    = "ES1370",
+    .qdev.size    = sizeof(PCIES1370State),
+    .init         = es1370_initfn,
+};
+
+static void es1370_register(void)
+{
+    pci_qdev_register(&es1370_info);
+}
+device_init(es1370_register);
+
-- 
1.6.2.5

[Qemu-devel] [PATCH 8/8] qdev: convert ac97.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ac97.c |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index b9dac3c..37aca9e 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1308,29 +1308,13 @@ static void ac97_on_reset (void *opaque)
     mixer_reset (s);
 }
 
-int ac97_init (PCIBus *bus)
+static void ac97_initfn(PCIDevice *dev)
 {
-    PCIAC97LinkState *d;
-    AC97LinkState *s;
-    uint8_t *c;
-
-    if (!bus) {
-        AUD_log ("ac97", "No PCI bus\n");
-        return -1;
-    }
-
-    d = (PCIAC97LinkState *) pci_register_device (bus, "AC97",
-                                                  sizeof (PCIAC97LinkState),
-                                                  -1, NULL, NULL);
-
-    if (!d) {
-        AUD_log ("ac97", "Failed to register PCI device\n");
-        return -1;
-    }
+    PCIAC97LinkState *d = DO_UPCAST(PCIAC97LinkState, dev, dev);
+    AC97LinkState *s = &d->ac97;
+    uint8_t *c = d->dev.config;
 
-    s = &d->ac97;
     s->pci_dev = &d->dev;
-    c = d->dev.config;
     pci_config_set_vendor_id (c, PCI_VENDOR_ID_INTEL); /* ro */
     pci_config_set_device_id (c, PCI_DEVICE_ID_INTEL_82801AA_5); /* ro */
 
@@ -1372,5 +1356,23 @@ int ac97_init (PCIBus *bus)
     qemu_register_reset (ac97_on_reset, s);
     AUD_register_card ("ac97", &s->card);
     ac97_on_reset (s);
+}
+
+int ac97_init (PCIBus *bus)
+{
+    pci_create_simple(bus, -1, "AC97");
     return 0;
 }
+
+static PCIDeviceInfo ac97_info = {
+    .qdev.name    = "AC97",
+    .qdev.size    = sizeof(PCIAC97LinkState),
+    .init         = ac97_initfn,
+};
+
+static void ac97_register(void)
+{
+    pci_qdev_register(&ac97_info);
+}
+device_init(ac97_register);
+
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Paul Brook]

On Tuesday 30 June 2009, Gerd Hoffmann wrote:
>  * maintain a list of busses.
>  * maintain bus numbers.
>  * add function to find busses by type / name / number.
>  * add monitor command to list busses.

I still object to this patch. Busses should be identified by their location in 
the tree, not by number.

Paul

Re: [Qemu-devel] [PATCH 6/8] qdev/pci: misc fixes.

[Paul Brook]

On Tuesday 30 June 2009, Gerd Hoffmann wrote:
> -    register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
> +    register_savevm("PCIBUS", bus->qbus.busnr, 1, pcibus_save,
> pcibus_load, bus);

This is wrong. In principle there may be many different PCI domains with 
overlapping bus numbers.

Paul

Re: [Qemu-devel] [PATCH 6/8] qdev/pci: misc fixes.

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

> On Tuesday 30 June 2009, Gerd Hoffmann wrote:
>> -    register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
>> +    register_savevm("PCIBUS", bus->qbus.busnr, 1, pcibus_save,
>> pcibus_load, bus);
>
> This is wrong. In principle there may be many different PCI domains with 
> overlapping bus numbers.
>
> Paul

We don't support multiple PCI domains, so this is fine for now.  When we
do, we can simply prepend the 16 bit PCI domain number to the 8 bit PCI
bus number.

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

> On Tuesday 30 June 2009, Gerd Hoffmann wrote:
>>  * maintain a list of busses.
>>  * maintain bus numbers.
>>  * add function to find busses by type / name / number.
>>  * add monitor command to list busses.
>
> I still object to this patch. Busses should be identified by their location in 
> the tree, not by number.
>
> Paul

Location in the tree can be uniquely identified by a number.  Handy when
all you want is enumerate the buses, and you don't really care where
they're hanging out in the tree.  Why should something like that not be
done?

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Paul Brook]

On Tuesday 30 June 2009, Markus Armbruster wrote:
> Paul Brook <paul@codesourcery.com> writes:
> > On Tuesday 30 June 2009, Gerd Hoffmann wrote:
> >>  * maintain a list of busses.
> >>  * maintain bus numbers.
> >>  * add function to find busses by type / name / number.
> >>  * add monitor command to list busses.
> >
> > I still object to this patch. Busses should be identified by their
> > location in the tree, not by number.
> >
> > Paul
>
> Location in the tree can be uniquely identified by a number.  Handy when
> all you want is enumerate the buses, and you don't really care where
> they're hanging out in the tree.  Why should something like that not be
> done?

The address of the BusState is also a locally unique identifier. That doesn't 
mean it's a good thing to expose to the user.

Paul

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

> On Tuesday 30 June 2009, Markus Armbruster wrote:
>> Paul Brook <paul@codesourcery.com> writes:
>> > On Tuesday 30 June 2009, Gerd Hoffmann wrote:
>> >>  * maintain a list of busses.
>> >>  * maintain bus numbers.
>> >>  * add function to find busses by type / name / number.
>> >>  * add monitor command to list busses.
>> >
>> > I still object to this patch. Busses should be identified by their
>> > location in the tree, not by number.
>> >
>> > Paul
>>
>> Location in the tree can be uniquely identified by a number.  Handy when
>> all you want is enumerate the buses, and you don't really care where
>> they're hanging out in the tree.  Why should something like that not be
>> done?
>
> The address of the BusState is also a locally unique identifier. That doesn't 
> mean it's a good thing to expose to the user.
>
> Paul

Red herring.

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Anthony Liguori]

Markus Armbruster wrote:
> Paul Brook <paul@codesourcery.com> writes:
>
>   
>> On Tuesday 30 June 2009, Markus Armbruster wrote:
>>     
>>> Paul Brook <paul@codesourcery.com> writes:
>>>       
>>>> On Tuesday 30 June 2009, Gerd Hoffmann wrote:
>>>>         
>>>>>  * maintain a list of busses.
>>>>>  * maintain bus numbers.
>>>>>  * add function to find busses by type / name / number.
>>>>>  * add monitor command to list busses.
>>>>>           
>>>> I still object to this patch. Busses should be identified by their
>>>> location in the tree, not by number.
>>>>
>>>> Paul
>>>>         
>>> Location in the tree can be uniquely identified by a number.  Handy when
>>> all you want is enumerate the buses, and you don't really care where
>>> they're hanging out in the tree.  Why should something like that not be
>>> done?
>>>       
>> The address of the BusState is also a locally unique identifier. That doesn't 
>> mean it's a good thing to expose to the user.
>>
>> Paul
>>     
>
> Red herring.
>   

I don't think that's a very useful response.

I think it's a perfectly valid suggestion that we should identify buses 
based on the their location in the tree to users verses a number 
generated based on some hashing algorithm.

A tree location has meaning to a user.  A random integer doesn't.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Gerd Hoffmann]

On 06/30/09 21:49, Anthony Liguori wrote:
> I think it's a perfectly valid suggestion that we should identify buses
> based on the their location in the tree to users verses a number
> generated based on some hashing algorithm.
 >
> A tree location has meaning to a user. A random integer doesn't.

Well.  Depends on the bus I think.  About PCI devices the usual user 
probably thinks in terms of "$bus:$slot.$function", which includes a bus 
number.

Speaking of PCI: the PCI bus number (aka PCIBus->bus_num) has nothing to 
do with the more or less random bus number introduced by the (now 
dropped) patch (aka PCIBus->qdev.busnr).  Which indicates that it is 
probably less confusing to have the bus implementation handle the 
enumeration of busses.  If it makes sense for the bus in question of 
course.  sysbus probably doesn't care ;)

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 06/30/09 21:49, Anthony Liguori wrote:
>> I think it's a perfectly valid suggestion that we should identify buses
>> based on the their location in the tree to users verses a number
>> generated based on some hashing algorithm.
> >
>> A tree location has meaning to a user. A random integer doesn't.
>
> Well.  Depends on the bus I think.  About PCI devices the usual user 
> probably thinks in terms of "$bus:$slot.$function", which includes a 
> bus number.

But if you think of SCSI as a bus, then it makes more sense to enumerate 
each SCSI bus by the tree location (which would end up being the PCI 
location) than a made up number.

> Speaking of PCI: the PCI bus number (aka PCIBus->bus_num) has nothing 
> to do with the more or less random bus number introduced by the (now 
> dropped) patch (aka PCIBus->qdev.busnr).

Yup.

> Which indicates that it is probably less confusing to have the bus 
> implementation handle the enumeration of busses.  If it makes sense 
> for the bus in question of course.  sysbus probably doesn't care ;)

That's probably a good point.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster wrote:
>> Paul Brook <paul@codesourcery.com> writes:
>>
>>   
>>> On Tuesday 30 June 2009, Markus Armbruster wrote:
>>>     
>>>> Paul Brook <paul@codesourcery.com> writes:
>>>>       
>>>>> On Tuesday 30 June 2009, Gerd Hoffmann wrote:
>>>>>         
>>>>>>  * maintain a list of busses.
>>>>>>  * maintain bus numbers.
>>>>>>  * add function to find busses by type / name / number.
>>>>>>  * add monitor command to list busses.
>>>>>>           
>>>>> I still object to this patch. Busses should be identified by their
>>>>> location in the tree, not by number.
>>>>>
>>>>> Paul
>>>>>         
>>>> Location in the tree can be uniquely identified by a number.  Handy when
>>>> all you want is enumerate the buses, and you don't really care where
>>>> they're hanging out in the tree.  Why should something like that not be
>>>> done?
>>>>       
>>> The address of the BusState is also a locally unique
>>> identifier. That doesn't mean it's a good thing to expose to the
>>> user.
>>>
>>> Paul
>>>     
>>
>> Red herring.
>>   
>
> I don't think that's a very useful response.
>
> I think it's a perfectly valid suggestion that we should identify
> buses based on the their location in the tree to users verses a number
> generated based on some hashing algorithm.
>
> A tree location has meaning to a user.  A random integer doesn't.

Numbering nodes according to a well-defined tree traversal is not
random.  We can discuss whether using such a number in an interface is a
good idea (nobody suggested to use it *instead* of tree paths).
Rejecting the idea by comparing it to some internal address, however, is
not a very useful contribution to such a discussion.

But I figure I'm wasting my time and yours, so I'll shut up now.

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Paul Brook]

On Tuesday 30 June 2009, Gerd Hoffmann wrote:
> On 06/30/09 21:49, Anthony Liguori wrote:
> > I think it's a perfectly valid suggestion that we should identify buses
> > based on the their location in the tree to users verses a number
> > generated based on some hashing algorithm.
> >
> > A tree location has meaning to a user. A random integer doesn't.
>
> Well.  Depends on the bus I think.  About PCI devices the usual user
> probably thinks in terms of "$bus:$slot.$function", which includes a bus
> number.
>
> Speaking of PCI: the PCI bus number (aka PCIBus->bus_num) has nothing to
> do with the more or less random bus number introduced by the (now
> dropped) patch (aka PCIBus->qdev.busnr).  Which indicates that it is
> probably less confusing to have the bus implementation handle the
> enumeration of busses.  If it makes sense for the bus in question of
> course.  sysbus probably doesn't care ;)

PCI bus numbers should be determined by the guest, and are not fixed values. 
The only reason we get away with it now is because most guests will honor a 
pre-existing bridge device configuration. A guest firmware could choose to 
enumerate the PCI busses in a different order.

Paul

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Paul Brook]

> >>> The address of the BusState is also a locally unique
> >>> identifier. That doesn't mean it's a good thing to expose to the
> >>> user.
> >>
> >> Red herring.
> >
> > I don't think that's a very useful response.
> >
> > I think it's a perfectly valid suggestion that we should identify
> > buses based on the their location in the tree to users verses a number
> > generated based on some hashing algorithm.
> >
> > A tree location has meaning to a user.  A random integer doesn't.
>
> Numbering nodes according to a well-defined tree traversal is not
> random.  We can discuss whether using such a number in an interface is a
> good idea (nobody suggested to use it *instead* of tree paths).

I don't believe the tree traversal order is well defined. While developing the 
qdev patches I went through two or three different traversal algorithms. It 
gets even hairier when you start considering hotplug.

Paul

Re: [Qemu-devel] [PATCH 4/8] qdev/core: bus list

[Gerd Hoffmann]

> PCI bus numbers should be determined by the guest, and are not fixed values.

They are not fixed.  Guest writes to the bridge registers will update 
PCIBus->bus_num, so the guests and qemus view to pci bus numbers should 
match even if the guest changes them.

cheers,
   Gerd