[Qemu-devel] [PATCH 00/13] qdev: bus management updates.

[Qemu-devel] [PATCH 00/13] qdev: bus management updates.

[Gerd Hoffmann]

  Hi,

This patch series does a bunch of improvements for qdev busses.
The former name was "allow embedded bus structs", but that title
doesn't cut it any more as there are lots of followup improvements
and fixes.  Changes:

  * allow embedded bus structs (yes, still there).
  * add bus release function.
  * added device destruction callback to qdev devices.
  * make busses use the new features.

End result is that you can use qdev_free() to zap a device.  Using the
destruction callback everything will be released properly.  qdev will
walk the child bus(es) if present and zap all connected devices too.

You can pci_del the scsi adapter and all scsi disks will be deleted too.
You can pci_del the usb controller and all usb devices connected will be
deleted too.  You can unplug usb hubs, all connected devices will go away
too.

cheers,
  Gerd

[Qemu-devel] [PATCH 01/13] allow qdev busses allocations be inplace

[Gerd Hoffmann]

A bus usually is instanciated by the parent device.  The accociated bus
state can usually be embeeded into the parent device state struct, there
is no need for separate allocation and pointer indirection.

This patch adds a non-allocating qbus_create variant.  We keep track of
whenever qdev allocated the struct or not.  Will be needed later in this
patch series when we'll fix bus unplugging.

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

diff --git a/hw/qdev.c b/hw/qdev.c
index 43b1beb..43372c1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -502,13 +502,12 @@ static BusState *qbus_find(const char *path)
     }
 }
 
-BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
+void qbus_create_inplace(BusState *bus, BusInfo *info,
+                         DeviceState *parent, const char *name)
 {
-    BusState *bus;
     char *buf;
     int i,len;
 
-    bus = qemu_mallocz(info->size);
     bus->info = info;
     bus->parent = parent;
 
@@ -537,6 +536,16 @@ BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
         QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
         parent->num_child_bus++;
     }
+
+}
+
+BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
+{
+    BusState *bus;
+
+    bus = qemu_mallocz(info->size);
+    bus->qdev_allocated = 1;
+    qbus_create_inplace(bus, info, parent, name);
     return bus;
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 623ded5..ccc45b9 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -47,6 +47,7 @@ struct BusState {
     DeviceState *parent;
     BusInfo *info;
     const char *name;
+    int qdev_allocated;
     QLIST_HEAD(, DeviceState) children;
     QLIST_ENTRY(BusState) sibling;
 };
@@ -144,6 +145,8 @@ BusState *qdev_get_parent_bus(DeviceState *dev);
 
 /*** BUS API. ***/
 
+void qbus_create_inplace(BusState *bus, BusInfo *info,
+                         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)
-- 
1.6.2.5

[Qemu-devel] [PATCH 02/13] switch scsi bus to inplace allocation.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/esp.c        |   10 +++++-----
 hw/lsi53c895a.c |   12 ++++++------
 hw/scsi-bus.c   |    9 +++------
 hw/scsi-disk.h  |    4 ++--
 hw/usb-msd.c    |    6 +++---
 5 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/hw/esp.c b/hw/esp.c
index 9a5a8fb..5d86020 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -63,7 +63,7 @@ struct ESPState {
     uint8_t ti_buf[TI_BUFSZ];
     uint32_t sense;
     uint32_t dma;
-    SCSIBus *bus;
+    SCSIBus bus;
     SCSIDevice *current_dev;
     uint8_t cmdbuf[TI_BUFSZ];
     uint32_t cmdlen;
@@ -191,7 +191,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf)
         s->async_len = 0;
     }
 
-    if (target >= ESP_MAX_DEVS || !s->bus->devs[target]) {
+    if (target >= ESP_MAX_DEVS || !s->bus.devs[target]) {
         // No such drive
         s->rregs[ESP_RSTAT] = 0;
         s->rregs[ESP_RINTR] = INTR_DC;
@@ -199,7 +199,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf)
         esp_raise_irq(s);
         return 0;
     }
-    s->current_dev = s->bus->devs[target];
+    s->current_dev = s->bus.devs[target];
     return dmalen;
 }
 
@@ -672,8 +672,8 @@ static int esp_init1(SysBusDevice *dev)
 
     qdev_init_gpio_in(&dev->qdev, parent_esp_reset, 1);
 
-    s->bus = scsi_bus_new(&dev->qdev, 0, ESP_MAX_DEVS, esp_command_complete);
-    scsi_bus_legacy_handle_cmdline(s->bus);
+    scsi_bus_new(&s->bus, &dev->qdev, 0, ESP_MAX_DEVS, esp_command_complete);
+    scsi_bus_legacy_handle_cmdline(&s->bus);
     return 0;
 }
 
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 62bdca8..7c45391 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -193,7 +193,7 @@ typedef struct {
      * 2 if processing DMA from lsi_execute_script.
      * 3 if a DMA operation is in progress.  */
     int waiting;
-    SCSIBus *bus;
+    SCSIBus bus;
     SCSIDevice *current_dev;
     int current_lun;
     /* The tag is a combination of the device ID and the SCSI tag.  */
@@ -585,7 +585,7 @@ static void lsi_reselect(LSIState *s, uint32_t tag)
     id = (tag >> 8) & 0xf;
     s->ssid = id | 0x80;
     DPRINTF("Reselected target %d\n", id);
-    s->current_dev = s->bus->devs[id];
+    s->current_dev = s->bus.devs[id];
     s->current_tag = tag;
     s->scntl1 |= LSI_SCNTL1_CON;
     lsi_set_phase(s, PHASE_MI);
@@ -1041,7 +1041,7 @@ again:
                 }
                 s->sstat0 |= LSI_SSTAT0_WOA;
                 s->scntl1 &= ~LSI_SCNTL1_IARB;
-                if (id >= LSI_MAX_DEVS || !s->bus->devs[id]) {
+                if (id >= LSI_MAX_DEVS || !s->bus.devs[id]) {
                     DPRINTF("Selected absent target %d\n", id);
                     lsi_script_scsi_interrupt(s, 0, LSI_SIST1_STO);
                     lsi_disconnect(s);
@@ -1052,7 +1052,7 @@ again:
                 /* ??? Linux drivers compain when this is set.  Maybe
                    it only applies in low-level mode (unimplemented).
                 lsi_script_scsi_interrupt(s, LSI_SIST0_CMP, 0); */
-                s->current_dev = s->bus->devs[id];
+                s->current_dev = s->bus.devs[id];
                 s->current_tag = id << 8;
                 s->scntl1 |= LSI_SCNTL1_CON;
                 if (insn & (1 << 3)) {
@@ -2178,8 +2178,8 @@ static int lsi_scsi_init(PCIDevice *dev)
 
     lsi_soft_reset(s);
 
-    s->bus = scsi_bus_new(&dev->qdev, 1, LSI_MAX_DEVS, lsi_command_complete);
-    scsi_bus_legacy_handle_cmdline(s->bus);
+    scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, lsi_command_complete);
+    scsi_bus_legacy_handle_cmdline(&s->bus);
     register_savevm("lsiscsi", -1, 0, lsi_scsi_save, lsi_scsi_load, s);
     return 0;
 }
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 16afa05..881e363 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -15,17 +15,14 @@ static struct BusInfo scsi_bus_info = {
 static int next_scsi_bus;
 
 /* Create a scsi bus, and attach devices to it.  */
-SCSIBus *scsi_bus_new(DeviceState *host, int tcq, int ndev,
-                      scsi_completionfn complete)
+void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,
+                  scsi_completionfn complete)
 {
-    SCSIBus *bus;
-
-    bus = FROM_QBUS(SCSIBus, qbus_create(&scsi_bus_info, host, NULL));
+    qbus_create_inplace(&bus->qbus, &scsi_bus_info, host, NULL);
     bus->busnr = next_scsi_bus++;
     bus->tcq = tcq;
     bus->ndev = ndev;
     bus->complete = complete;
-    return bus;
 }
 
 static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
diff --git a/hw/scsi-disk.h b/hw/scsi-disk.h
index febde44..b6b6c12 100644
--- a/hw/scsi-disk.h
+++ b/hw/scsi-disk.h
@@ -52,8 +52,8 @@ struct SCSIBus {
     SCSIDevice *devs[8];
 };
 
-SCSIBus *scsi_bus_new(DeviceState *host, int tcq, int ndev,
-                      scsi_completionfn complete);
+void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,
+                  scsi_completionfn complete);
 void scsi_qdev_register(SCSIDeviceInfo *info);
 
 static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index aa0ce6a..6b9c8a5 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -45,7 +45,7 @@ typedef struct {
     uint32_t data_len;
     uint32_t residue;
     uint32_t tag;
-    SCSIBus *bus;
+    SCSIBus bus;
     DriveInfo *dinfo;
     SCSIDevice *scsi_dev;
     int result;
@@ -527,8 +527,8 @@ static int usb_msd_initfn(USBDevice *dev)
     }
 
     s->dev.speed = USB_SPEED_FULL;
-    s->bus = scsi_bus_new(&s->dev.qdev, 0, 1, usb_msd_command_complete);
-    s->scsi_dev = scsi_bus_legacy_add_drive(s->bus, s->dinfo, 0);
+    scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete);
+    s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, s->dinfo, 0);
     usb_msd_handle_reset(dev);
     return 0;
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 03/13] switch usb bus to inplace allocation.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-bus.c  |    7 ++-----
 hw/usb-musb.c |    6 +++---
 hw/usb-ohci.c |    6 +++---
 hw/usb-uhci.c |    6 +++---
 hw/usb.h      |    2 +-
 5 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 03933f1..2cac1e8 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -14,16 +14,13 @@ static struct BusInfo usb_bus_info = {
 static int next_usb_bus = 0;
 static QTAILQ_HEAD(, USBBus) busses = QTAILQ_HEAD_INITIALIZER(busses);
 
-USBBus *usb_bus_new(DeviceState *host)
+void usb_bus_new(USBBus *bus, DeviceState *host)
 {
-    USBBus *bus;
-
-    bus = FROM_QBUS(USBBus, qbus_create(&usb_bus_info, host, NULL));
+    qbus_create_inplace(&bus->qbus, &usb_bus_info, host, NULL);
     bus->busnr = next_usb_bus++;
     QTAILQ_INIT(&bus->free);
     QTAILQ_INIT(&bus->used);
     QTAILQ_INSERT_TAIL(&busses, bus, next);
-    return bus;
 }
 
 USBBus *usb_bus_find(int busnr)
diff --git a/hw/usb-musb.c b/hw/usb-musb.c
index 9eb0d63..09ec5a1 100644
--- a/hw/usb-musb.c
+++ b/hw/usb-musb.c
@@ -281,7 +281,7 @@ typedef struct {
 
 struct MUSBState {
     qemu_irq *irqs;
-    USBBus *bus;
+    USBBus bus;
     USBPort port;
 
     int idx;
@@ -331,8 +331,8 @@ struct MUSBState {
         s->ep[i].epnum = i;
     }
 
-    s->bus = usb_bus_new(NULL /* FIXME */);
-    usb_register_port(s->bus, &s->port, s, 0, musb_attach);
+    usb_bus_new(&s->bus, NULL /* FIXME */);
+    usb_register_port(&s->bus, &s->port, s, 0, musb_attach);
 
     return s;
 }
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 6e428c4..48ccd49 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -65,7 +65,7 @@ enum ohci_type {
 };
 
 typedef struct {
-    USBBus *bus;
+    USBBus bus;
     qemu_irq irq;
     enum ohci_type type;
     int mem;
@@ -1690,10 +1690,10 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
     ohci->irq = irq;
     ohci->type = type;
 
-    ohci->bus = usb_bus_new(dev);
+    usb_bus_new(&ohci->bus, dev);
     ohci->num_ports = num_ports;
     for (i = 0; i < num_ports; i++) {
-        usb_register_port(ohci->bus, &ohci->rhport[i].port, ohci, i, ohci_attach);
+        usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, ohci_attach);
     }
 
     ohci->async_td = 0;
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 6807413..a3ed9b2 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -122,7 +122,7 @@ typedef struct UHCIPort {
 
 typedef struct UHCIState {
     PCIDevice dev;
-    USBBus *bus;
+    USBBus bus;
     uint16_t cmd; /* cmd register */
     uint16_t status;
     uint16_t intr; /* interrupt enable register */
@@ -1083,9 +1083,9 @@ static int usb_uhci_common_initfn(UHCIState *s)
     pci_conf[0x3d] = 4; // interrupt pin 3
     pci_conf[0x60] = 0x10; // release number
 
-    s->bus = usb_bus_new(&s->dev.qdev);
+    usb_bus_new(&s->bus, &s->dev.qdev);
     for(i = 0; i < NB_PORTS; i++) {
-        usb_register_port(s->bus, &s->ports[i].port, s, i, uhci_attach);
+        usb_register_port(&s->bus, &s->ports[i].port, s, i, uhci_attach);
     }
     s->frame_timer = qemu_new_timer(vm_clock, uhci_frame_timer, s);
 
diff --git a/hw/usb.h b/hw/usb.h
index 7c5cf83..467cddb 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -303,7 +303,7 @@ struct USBBus {
     QTAILQ_ENTRY(USBBus) next;
 };
 
-USBBus *usb_bus_new(DeviceState *host);
+void usb_bus_new(USBBus *bus, DeviceState *host);
 USBBus *usb_bus_find(int busnr);
 void usb_qdev_register(USBDeviceInfo *info);
 void usb_qdev_register_many(USBDeviceInfo *info);
-- 
1.6.2.5

[Qemu-devel] [PATCH 04/13] switch ide bus to inplace allocation.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ide/internal.h |    2 +-
 hw/ide/isa.c      |   24 ++++++++++++------------
 hw/ide/pci.c      |   42 +++++++++++++++++++++---------------------
 hw/ide/qdev.c     |    7 ++-----
 4 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 9df759d..029bf80 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -551,7 +551,7 @@ void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
 
 /* hw/ide/qdev.c */
-IDEBus *ide_bus_new(DeviceState *dev);
+void ide_bus_new(IDEBus *idebus, DeviceState *dev);
 IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
 
 #endif /* HW_IDE_INTERNAL_H */
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index d2fe0c0..3205f40 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -37,7 +37,7 @@
 
 typedef struct ISAIDEState {
     ISADevice dev;
-    IDEBus    *bus;
+    IDEBus    bus;
     uint32_t  iobase;
     uint32_t  iobase2;
     uint32_t  isairq;
@@ -48,18 +48,18 @@ static void isa_ide_save(QEMUFile* f, void *opaque)
 {
     ISAIDEState *s = opaque;
 
-    idebus_save(f, s->bus);
-    ide_save(f, &s->bus->ifs[0]);
-    ide_save(f, &s->bus->ifs[1]);
+    idebus_save(f, &s->bus);
+    ide_save(f, &s->bus.ifs[0]);
+    ide_save(f, &s->bus.ifs[1]);
 }
 
 static int isa_ide_load(QEMUFile* f, void *opaque, int version_id)
 {
     ISAIDEState *s = opaque;
 
-    idebus_load(f, s->bus, version_id);
-    ide_load(f, &s->bus->ifs[0], version_id);
-    ide_load(f, &s->bus->ifs[1], version_id);
+    idebus_load(f, &s->bus, version_id);
+    ide_load(f, &s->bus.ifs[0], version_id);
+    ide_load(f, &s->bus.ifs[1], version_id);
     return 0;
 }
 
@@ -67,10 +67,10 @@ static int isa_ide_initfn(ISADevice *dev)
 {
     ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
 
-    s->bus = ide_bus_new(&s->dev.qdev);
-    ide_init_ioport(s->bus, s->iobase, s->iobase2);
+    ide_bus_new(&s->bus, &s->dev.qdev);
+    ide_init_ioport(&s->bus, s->iobase, s->iobase2);
     isa_init_irq(dev, &s->irq, s->isairq);
-    ide_init2(s->bus, NULL, NULL, s->irq);
+    ide_init2(&s->bus, NULL, NULL, s->irq);
     register_savevm("isa-ide", 0, 3, isa_ide_save, isa_ide_load, s);
     return 0;
 };
@@ -90,9 +90,9 @@ int isa_ide_init(int iobase, int iobase2, int isairq,
 
     s = DO_UPCAST(ISAIDEState, dev, dev);
     if (hd0)
-        ide_create_drive(s->bus, 0, hd0);
+        ide_create_drive(&s->bus, 0, hd0);
     if (hd1)
-        ide_create_drive(s->bus, 1, hd1);
+        ide_create_drive(&s->bus, 1, hd1);
     return 0;
 }
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 89ecd44..bddf541 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -51,7 +51,7 @@
 
 typedef struct PCIIDEState {
     PCIDevice dev;
-    IDEBus *bus[2];
+    IDEBus bus[2];
     BMDMAState bmdma[2];
     int type; /* see IDE_TYPE_xxx */
     uint32_t secondary;
@@ -66,7 +66,7 @@ static void ide_map(PCIDevice *pci_dev, int region_num,
     IDEBus *bus;
 
     if (region_num <= 3) {
-        bus = d->bus[(region_num >> 1)];
+        bus = &d->bus[(region_num >> 1)];
         if (region_num & 1) {
             register_ioport_read(addr + 2, 1, 1, ide_status_read, bus);
             register_ioport_write(addr + 2, 1, 1, ide_cmd_write, bus);
@@ -252,9 +252,9 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        d->bus[i]->bmdma = bm;
+        d->bus[i].bmdma = bm;
         bm->pci_dev = DO_UPCAST(PCIIDEState, dev, pci_dev);
-        bm->bus = d->bus[i];
+        bm->bus = d->bus+i;
         qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
@@ -294,13 +294,13 @@ static void pci_ide_save(QEMUFile* f, void *opaque)
 
     /* per IDE interface data */
     for(i = 0; i < 2; i++) {
-        idebus_save(f, d->bus[i]);
+        idebus_save(f, d->bus+i);
     }
 
     /* per IDE drive data */
     for(i = 0; i < 2; i++) {
-        ide_save(f, &d->bus[i]->ifs[0]);
-        ide_save(f, &d->bus[i]->ifs[1]);
+        ide_save(f, &d->bus[i].ifs[0]);
+        ide_save(f, &d->bus[i].ifs[1]);
     }
 }
 
@@ -330,13 +330,13 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
 
     /* per IDE interface data */
     for(i = 0; i < 2; i++) {
-        idebus_load(f, d->bus[i], version_id);
+        idebus_load(f, d->bus+i, version_id);
     }
 
     /* per IDE drive data */
     for(i = 0; i < 2; i++) {
-        ide_load(f, &d->bus[i]->ifs[0], version_id);
-        ide_load(f, &d->bus[i]->ifs[1], version_id);
+        ide_load(f, &d->bus[i].ifs[0], version_id);
+        ide_load(f, &d->bus[i].ifs[1], version_id);
     }
     return 0;
 }
@@ -351,7 +351,7 @@ static void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
     for (i = 0; i < 4; i++) {
         if (hd_table[i] == NULL)
             continue;
-        ide_create_drive(d->bus[bus[i]], unit[i], hd_table[i]);
+        ide_create_drive(d->bus+bus[i], unit[i], hd_table[i]);
     }
 }
 
@@ -427,10 +427,10 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     pci_conf[0x3d] = 0x01; // interrupt on pin 1
 
     irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
-    d->bus[0] = ide_bus_new(&d->dev.qdev);
-    d->bus[1] = ide_bus_new(&d->dev.qdev);
-    ide_init2(d->bus[0], NULL, NULL, irq[0]);
-    ide_init2(d->bus[1], NULL, NULL, irq[1]);
+    ide_bus_new(&d->bus[0], &d->dev.qdev);
+    ide_bus_new(&d->bus[1], &d->dev.qdev);
+    ide_init2(&d->bus[0], NULL, NULL, irq[0]);
+    ide_init2(&d->bus[1], NULL, NULL, irq[1]);
 
     register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
     qemu_register_reset(cmd646_reset, d);
@@ -482,13 +482,13 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
 
     register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
 
-    d->bus[0] = ide_bus_new(&d->dev.qdev);
-    d->bus[1] = ide_bus_new(&d->dev.qdev);
-    ide_init_ioport(d->bus[0], 0x1f0, 0x3f6);
-    ide_init_ioport(d->bus[1], 0x170, 0x376);
+    ide_bus_new(&d->bus[0], &d->dev.qdev);
+    ide_bus_new(&d->bus[1], &d->dev.qdev);
+    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
+    ide_init_ioport(&d->bus[1], 0x170, 0x376);
 
-    ide_init2(d->bus[0], NULL, NULL, isa_reserve_irq(14));
-    ide_init2(d->bus[1], NULL, NULL, isa_reserve_irq(15));
+    ide_init2(&d->bus[0], NULL, NULL, isa_reserve_irq(14));
+    ide_init2(&d->bus[1], NULL, NULL, isa_reserve_irq(15));
     return 0;
 }
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index d467e3a..c562bc6 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -29,12 +29,9 @@ static struct BusInfo ide_bus_info = {
     .size  = sizeof(IDEBus),
 };
 
-IDEBus *ide_bus_new(DeviceState *dev)
+void ide_bus_new(IDEBus *idebus, DeviceState *dev)
 {
-    IDEBus *idebus;
-
-    idebus = FROM_QBUS(IDEBus, qbus_create(&ide_bus_info, dev, NULL));
-    return idebus;
+    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
 }
 
 static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
-- 
1.6.2.5

[Qemu-devel] [PATCH 05/13] inplace allocation for pci, split irq init.

[Gerd Hoffmann]

This patch allows carrying the pcibus struct inplace.  Separate
allocation is possible as well.  It also splits pci_register_bus into
two parts (irq setup is separate now).

Followup patches will use this.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c |   43 +++++++++++++++++++++++++++++++++----------
 hw/pci.h |    5 +++++
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 64d70ed..ad508a5 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -98,24 +98,47 @@ static void pci_bus_reset(void *opaque)
     }
 }
 
-PCIBus *pci_register_bus(DeviceState *parent, const char *name,
-                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque, int devfn_min, int nirq)
+void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
+                         const char *name, int devfn_min)
 {
-    PCIBus *bus;
     static int nbus = 0;
 
-    bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, parent, name));
-    bus->set_irq = set_irq;
-    bus->map_irq = map_irq;
-    bus->irq_opaque = irq_opaque;
+    qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
     bus->devfn_min = devfn_min;
-    bus->nirq = nirq;
-    bus->irq_count = qemu_mallocz(nirq * sizeof(bus->irq_count[0]));
     bus->next = first_bus;
     first_bus = bus;
     vmstate_register(nbus++, &vmstate_pcibus, bus);
     qemu_register_reset(pci_bus_reset, bus);
+}
+
+PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min)
+{
+    PCIBus *bus;
+
+    bus = qemu_mallocz(sizeof(*bus));
+    bus->qbus.qdev_allocated = 1;
+    pci_bus_new_inplace(bus, parent, name, devfn_min);
+    return bus;
+}
+
+void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+                  void *irq_opaque, int nirq)
+{
+    bus->set_irq = set_irq;
+    bus->map_irq = map_irq;
+    bus->irq_opaque = irq_opaque;
+    bus->nirq = nirq;
+    bus->irq_count = qemu_mallocz(nirq * sizeof(bus->irq_count[0]));
+}
+
+PCIBus *pci_register_bus(DeviceState *parent, const char *name,
+                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+                         void *irq_opaque, int devfn_min, int nirq)
+{
+    PCIBus *bus;
+
+    bus = pci_bus_new(parent, name, devfn_min);
+    pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
     return bus;
 }
 
diff --git a/hw/pci.h b/hw/pci.h
index caba5c8..2b6a0f2 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -248,6 +248,11 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
 
 typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
+void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
+                         const char *name, int devfn_min);
+PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
+void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+                  void *irq_opaque, int nirq);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque, int devfn_min, int nirq);
-- 
1.6.2.5

[Qemu-devel] [PATCH 06/13] convert pci bridge to qdev

[Gerd Hoffmann]

Also switch secondary pci busses to inplace allocation.

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

diff --git a/hw/pci.c b/hw/pci.c
index ad508a5..608792a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -142,18 +142,16 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     return bus;
 }
 
-static PCIBus *pci_register_secondary_bus(PCIDevice *dev,
-                                          pci_map_irq_fn map_irq,
-                                          const char *name)
+static void pci_register_secondary_bus(PCIBus *bus,
+                                       PCIDevice *dev,
+                                       pci_map_irq_fn map_irq,
+                                       const char *name)
 {
-    PCIBus *bus;
-
-    bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, &dev->qdev, name));
+    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
     bus->map_irq = map_irq;
     bus->parent_dev = dev;
     bus->next = dev->bus->next;
     dev->bus->next = bus;
-    return bus;
 }
 
 int pci_bus_num(PCIBus *s)
@@ -858,7 +856,9 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
 
 typedef struct {
     PCIDevice dev;
-    PCIBus *bus;
+    PCIBus bus;
+    uint32_t vid;
+    uint32_t did;
 } PCIBridge;
 
 static void pci_bridge_write_config(PCIDevice *d,
@@ -867,7 +867,7 @@ static void pci_bridge_write_config(PCIDevice *d,
     PCIBridge *s = (PCIBridge *)d;
 
     pci_default_write_config(d, address, val, len);
-    s->bus->bus_num = d->config[PCI_SECONDARY_BUS];
+    s->bus.bus_num = d->config[PCI_SECONDARY_BUS];
 }
 
 PCIBus *pci_find_bus(int bus_num)
@@ -890,15 +890,12 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
     return bus->devices[PCI_DEVFN(slot, function)];
 }
 
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name)
+static int pci_bridge_initfn(PCIDevice *dev)
 {
-    PCIBridge *s;
-    s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge),
-                                         devfn, NULL, pci_bridge_write_config);
+    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
 
-    pci_config_set_vendor_id(s->dev.config, vid);
-    pci_config_set_device_id(s->dev.config, did);
+    pci_config_set_vendor_id(s->dev.config, s->vid);
+    pci_config_set_device_id(s->dev.config, s->did);
 
     s->dev.config[0x04] = 0x06; // command = bus master, pci mem
     s->dev.config[0x05] = 0x00;
@@ -911,9 +908,23 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
     s->dev.config[PCI_HEADER_TYPE] =
         PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type
     s->dev.config[0x1E] = 0xa0; // secondary status
+    return 0;
+}
 
-    s->bus = pci_register_secondary_bus(&s->dev, map_irq, name);
-    return s->bus;
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
+                        pci_map_irq_fn map_irq, const char *name)
+{
+    PCIDevice *dev;
+    PCIBridge *s;
+
+    dev = pci_create_noinit(bus, devfn, "pci-bridge");
+    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
+    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
+    qdev_init(&dev->qdev);
+
+    s = DO_UPCAST(PCIBridge, dev, dev);
+    pci_register_secondary_bus(&s->bus, &s->dev, map_irq, name);
+    return &s->bus;
 }
 
 static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
@@ -1074,3 +1085,22 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
                        r->addr, r->addr + r->size - 1);
     }
 }
+
+static PCIDeviceInfo bridge_info = {
+    .qdev.name    = "pci-bridge",
+    .qdev.size    = sizeof(PCIBridge),
+    .init         = pci_bridge_initfn,
+    .config_write = pci_bridge_write_config,
+    .qdev.props   = (Property[]) {
+        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
+        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void pci_register_devices(void)
+{
+    pci_qdev_register(&bridge_info);
+}
+
+device_init(pci_register_devices)
-- 
1.6.2.5

[Qemu-devel] [PATCH 07/13] piix_pci: kill PIIX3IrqState

[Gerd Hoffmann]

The splitted init + irq setup for the pci bus untangles a initialization
order issue and allows to remove the PIIX3IrqState struct.  We can
carry the state directly in PIIX3State insted.

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

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index edd6df0..0be3b02 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -36,18 +36,14 @@ typedef PCIHostState I440FXState;
 typedef struct PIIX3State {
     PCIDevice dev;
     int pci_irq_levels[4];
-} PIIX3State;
-
-typedef struct PIIX3IrqState {
-    PIIX3State *piix3;
     qemu_irq *pic;
-} PIIX3IrqState;
+} PIIX3State;
 
 struct PCII440FXState {
     PCIDevice dev;
     target_phys_addr_t isa_page_descs[384 / 4];
     uint8_t smm_enabled;
-    PIIX3IrqState *irq_state;
+    PIIX3State *piix3;
 };
 
 static void i440fx_addr_writel(void* opaque, uint32_t addr, uint32_t val)
@@ -167,7 +163,7 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
 
     if (version_id == 2)
         for (i = 0; i < 4; i++)
-            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
+            d->piix3->pci_irq_levels[i] = qemu_get_be32(f);
 
     return 0;
 }
@@ -232,23 +228,24 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
     PCIBus *b;
     PCIDevice *d;
     I440FXState *s;
-    PIIX3IrqState *irq_state = qemu_malloc(sizeof(*irq_state));
+    PIIX3State *piix3;
 
-    irq_state->pic = pic;
     dev = qdev_create(NULL, "i440FX-pcihost");
     s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
-    b = pci_register_bus(&s->busdev.qdev, "pci.0",
-                         piix3_set_irq, pci_slot_get_pirq, irq_state, 0, 4);
+    b = pci_bus_new(&s->busdev.qdev, NULL, 0);
     s->bus = b;
     qdev_init(dev);
 
     d = pci_create_simple(b, 0, "i440FX");
     *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
-    (*pi440fx_state)->irq_state = irq_state;
 
-    irq_state->piix3 = DO_UPCAST(PIIX3State, dev,
+    piix3 = DO_UPCAST(PIIX3State, dev,
                                  pci_create_simple(b, -1, "PIIX3"));
-    *piix3_devfn = irq_state->piix3->dev.devfn;
+    piix3->pic = pic;
+    pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3, 4);
+    (*pi440fx_state)->piix3 = piix3;
+
+    *piix3_devfn = piix3->dev.devfn;
 
     return b;
 }
@@ -258,22 +255,22 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
 static void piix3_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
-    PIIX3IrqState *irq_state = opaque;
+    PIIX3State *piix3 = opaque;
 
-    irq_state->piix3->pci_irq_levels[irq_num] = level;
+    piix3->pci_irq_levels[irq_num] = level;
 
     /* now we change the pic irq level according to the piix irq mappings */
     /* XXX: optimize */
-    pic_irq = irq_state->piix3->dev.config[0x60 + irq_num];
+    pic_irq = piix3->dev.config[0x60 + irq_num];
     if (pic_irq < 16) {
         /* The pic level is the logical OR of all the PCI irqs mapped
            to it */
         pic_level = 0;
         for (i = 0; i < 4; i++) {
-            if (pic_irq == irq_state->piix3->dev.config[0x60 + i])
-                pic_level |= irq_state->piix3->pci_irq_levels[i];
+            if (pic_irq == piix3->dev.config[0x60 + i])
+                pic_level |= piix3->pci_irq_levels[i];
         }
-        qemu_set_irq(irq_state->pic[pic_irq], pic_level);
+        qemu_set_irq(piix3->pic[pic_irq], pic_level);
     }
 }
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 08/13] qdev: device free fixups.

[Gerd Hoffmann]

Two bug fixes:
 * When freeing a device we unregister stuff unconditionally,
   even in case we didn't register in the first place because
   the ->init() callback failed.
 * When freeing a device with child busses attached we don't
   zap the child bus (and the devices attached to it).

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

diff --git a/hw/qdev.c b/hw/qdev.c
index 43372c1..4931da1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -102,6 +102,7 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
     qdev_prop_set_compat(dev);
     QLIST_INSERT_HEAD(&bus->children, dev, sibling);
+    dev->state = DEV_STATE_CREATED;
     return dev;
 }
 
@@ -216,6 +217,7 @@ int qdev_init(DeviceState *dev)
 {
     int rc;
 
+    assert(dev->state == DEV_STATE_CREATED);
     rc = dev->info->init(dev, dev->info);
     if (rc < 0)
         return rc;
@@ -223,18 +225,27 @@ int qdev_init(DeviceState *dev)
         qemu_register_reset(dev->info->reset, dev);
     if (dev->info->vmsd)
         vmstate_register(-1, dev->info->vmsd, dev);
+    dev->state = DEV_STATE_INITIALIZED;
     return 0;
 }
 
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
+    BusState *bus;
+
+    if (dev->state == DEV_STATE_INITIALIZED) {
+        while (dev->num_child_bus) {
+            bus = QLIST_FIRST(&dev->child_bus);
+            qbus_free(bus);
+        }
 #if 0 /* FIXME: need sane vmstate_unregister function */
-    if (dev->info->vmsd)
-        vmstate_unregister(dev->info->vmsd, dev);
+        if (dev->info->vmsd)
+            vmstate_unregister(dev->info->vmsd, dev);
 #endif
-    if (dev->info->reset)
-        qemu_unregister_reset(dev->info->reset, dev);
+        if (dev->info->reset)
+            qemu_unregister_reset(dev->info->reset, dev);
+    }
     QLIST_REMOVE(dev, sibling);
     qemu_free(dev);
 }
@@ -549,6 +560,22 @@ BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
     return bus;
 }
 
+void qbus_free(BusState *bus)
+{
+    DeviceState *dev;
+
+    while ((dev = QLIST_FIRST(&bus->children)) != NULL) {
+        qdev_free(dev);
+    }
+    if (bus->parent) {
+        QLIST_REMOVE(bus, sibling);
+        bus->parent->num_child_bus--;
+    }
+    if (bus->qdev_allocated) {
+        qemu_free(bus);
+    }
+}
+
 #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
 static void qbus_print(Monitor *mon, BusState *bus, int indent);
 
diff --git a/hw/qdev.h b/hw/qdev.h
index ccc45b9..c036aff 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -19,10 +19,16 @@ typedef struct BusState BusState;
 
 typedef struct BusInfo BusInfo;
 
+enum DevState {
+    DEV_STATE_CREATED = 1,
+    DEV_STATE_INITIALIZED,
+};
+
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
     const char *id;
+    enum DevState state;
     DeviceInfo *info;
     BusState *parent_bus;
     int num_gpio_out;
@@ -148,6 +154,7 @@ BusState *qdev_get_parent_bus(DeviceState *dev);
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
+void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 09/13] Add exit callback to DeviceInfo.

[Gerd Hoffmann]

This adds a exit callback for device destruction to DeviceInfo, so
we can hook cleanups into qdev device destruction.

Followup patches will put that into use.

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

diff --git a/hw/qdev.c b/hw/qdev.c
index 4931da1..a25245a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -245,6 +245,8 @@ void qdev_free(DeviceState *dev)
 #endif
         if (dev->info->reset)
             qemu_unregister_reset(dev->info->reset, dev);
+        if (dev->info->exit)
+            dev->info->exit(dev);
     }
     QLIST_REMOVE(dev, sibling);
     qemu_free(dev);
diff --git a/hw/qdev.h b/hw/qdev.h
index c036aff..0db2d32 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -107,6 +107,7 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 /*** Device API.  ***/
 
 typedef int (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
+typedef int (*qdev_exitfn)(DeviceState *dev);
 
 struct DeviceInfo {
     const char *name;
@@ -124,6 +125,7 @@ struct DeviceInfo {
 
     /* Private to qdev / bus.  */
     qdev_initfn init;
+    qdev_exitfn exit;
     BusInfo *bus_info;
     struct DeviceInfo *next;
 };
-- 
1.6.2.5

[Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/scsi-bus.c     |   24 +++++++++++++++++++++---
 hw/scsi-disk.c    |    6 ------
 hw/scsi-generic.c |    2 --
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 881e363..27defc4 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -30,6 +30,7 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
     SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
     SCSIDeviceInfo *info = DO_UPCAST(SCSIDeviceInfo, qdev, base);
     SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    int rc = -1;
 
     if (dev->id == -1) {
         for (dev->id = 0; dev->id < bus->ndev; dev->id++) {
@@ -43,21 +44,38 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
     }
 
     if (bus->devs[dev->id]) {
-        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
+        qdev_free(&bus->devs[dev->id]->qdev);
     }
     bus->devs[dev->id] = dev;
 
     dev->info = info;
-    return dev->info->init(dev);
+    rc = dev->info->init(dev);
+    if (rc != 0) {
+        bus->devs[dev->id] = NULL;
+    }
 
 err:
-    return -1;
+    return rc;
+}
+
+static int scsi_qdev_exit(DeviceState *qdev)
+{
+    SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+
+    assert(bus->devs[dev->id] != NULL);
+    if (bus->devs[dev->id]->info->destroy) {
+        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
+    }
+    bus->devs[dev->id] = NULL;
+    return 0;
 }
 
 void scsi_qdev_register(SCSIDeviceInfo *info)
 {
     info->qdev.bus_info = &scsi_bus_info;
     info->qdev.init     = scsi_qdev_init;
+    info->qdev.exit     = scsi_qdev_exit;
     qdev_register(&info->qdev);
 }
 
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index de8e314..0f029f8 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -936,11 +936,6 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     }
 }
 
-static void scsi_destroy(SCSIDevice *d)
-{
-    qemu_free(d);
-}
-
 static int scsi_disk_initfn(SCSIDevice *dev)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
@@ -974,7 +969,6 @@ static SCSIDeviceInfo scsi_disk_info = {
     .qdev.desc    = "virtual scsi disk or cdrom",
     .qdev.size    = sizeof(SCSIDiskState),
     .init         = scsi_disk_initfn,
-    .destroy      = scsi_destroy,
     .send_command = scsi_send_command,
     .read_data    = scsi_read_data,
     .write_data   = scsi_write_data,
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index f8c010b..86d1e54 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -668,8 +668,6 @@ static void scsi_destroy(SCSIDevice *d)
         qemu_free(r);
         r = n;
     }
-
-    qemu_free(d);
 }
 
 static int scsi_generic_initfn(SCSIDevice *dev)
-- 
1.6.2.5

[Qemu-devel] [PATCH 11/13] pci: use qdev for device destruction.

[Gerd Hoffmann]

pci_unregister_device is static now and hooked into Devicestate->exit.
qdev_free(pci_device) works now.

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

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index f3dc421..255decd 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -247,6 +247,6 @@ void pci_device_hot_remove_success(int pcibus, int slot)
         break;
     }
 
-    pci_unregister_device(d);
+    qdev_free(&d->qdev);
 }
 
diff --git a/hw/pci.c b/hw/pci.c
index 608792a..a6cd630 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -399,8 +399,9 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
     }
 }
 
-int pci_unregister_device(PCIDevice *pci_dev)
+static int pci_unregister_device(DeviceState *dev)
 {
+    PCIDevice *pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
     int ret = 0;
 
     if (pci_dev->unregister)
@@ -412,7 +413,6 @@ int pci_unregister_device(PCIDevice *pci_dev)
 
     qemu_free_irqs(pci_dev->irq);
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
-    qdev_free(&pci_dev->qdev);
     return 0;
 }
 
@@ -945,6 +945,7 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
 void pci_qdev_register(PCIDeviceInfo *info)
 {
     info->qdev.init = pci_qdev_init;
+    info->qdev.exit = pci_unregister_device;
     info->qdev.bus_info = &pci_bus_info;
     qdev_register(&info->qdev);
 }
diff --git a/hw/pci.h b/hw/pci.h
index 2b6a0f2..0ba7e6c 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -185,7 +185,6 @@ struct PCIDevice {
     /* do not access the following fields */
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;
-    PCIUnregisterFunc *unregister;
 
     /* IRQ objects for the INTA-INTD pins.  */
     qemu_irq *irq;
@@ -224,7 +223,6 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
                                int instance_size, int devfn,
                                PCIConfigReadFunc *config_read,
                                PCIConfigWriteFunc *config_write);
-int pci_unregister_device(PCIDevice *pci_dev);
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
                             uint32_t size, int type,
-- 
1.6.2.5

[Qemu-devel] [PATCH 12/13] pci: move unregister from PCIDevice to PCIDeviceInfo

[Gerd Hoffmann]

One more cleanup while being at it ;)

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/e1000.c      |    2 +-
 hw/eepro100.c   |    5 +++--
 hw/lsi53c895a.c |    2 +-
 hw/pci.c        |    7 +++++--
 hw/pci.h        |    1 +
 hw/pcnet.c      |    3 +--
 hw/rtl8139.c    |    3 +--
 hw/virtio-pci.c |    1 -
 8 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 95c471c..f123bda 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1129,7 +1129,6 @@ static int pci_e1000_init(PCIDevice *pci_dev)
     qemu_format_nic_info_str(d->vc, macaddr);
 
     register_savevm(info_str, -1, 2, nic_save, nic_load, d);
-    d->dev.unregister = pci_e1000_uninit;
     qemu_register_reset(e1000_reset, d);
     e1000_reset(d);
     return 0;
@@ -1139,6 +1138,7 @@ static PCIDeviceInfo e1000_info = {
     .qdev.name = "e1000",
     .qdev.size = sizeof(E1000State),
     .init      = pci_e1000_init,
+    .exit      = pci_e1000_uninit,
 };
 
 static void e1000_register_devices(void)
diff --git a/hw/eepro100.c b/hw/eepro100.c
index d3b7c3d..ea9c338 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1733,8 +1733,6 @@ static int nic_init(PCIDevice *pci_dev, uint32_t device)
 
     logout("\n");
 
-    s->dev.unregister = pci_nic_uninit;
-
     s->device = device;
 
     pci_reset(s);
@@ -1793,14 +1791,17 @@ static PCIDeviceInfo eepro100_info[] = {
         .qdev.name = "i82551",
         .qdev.size = sizeof(EEPRO100State),
         .init      = pci_i82551_init,
+        .exit      = pci_nic_uninit,
     },{
         .qdev.name = "i82557b",
         .qdev.size = sizeof(EEPRO100State),
         .init      = pci_i82557b_init,
+        .exit      = pci_nic_uninit,
     },{
         .qdev.name = "i82559er",
         .qdev.size = sizeof(EEPRO100State),
         .init      = pci_i82559er_init,
+        .exit      = pci_nic_uninit,
     },{
         /* end of list */
     }
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 7c45391..a4d3a57 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -2174,7 +2174,6 @@ static int lsi_scsi_init(PCIDevice *dev)
     s->queue = qemu_malloc(sizeof(lsi_queue));
     s->queue_len = 1;
     s->active_commands = 0;
-    s->dev.unregister = lsi_scsi_uninit;
 
     lsi_soft_reset(s);
 
@@ -2189,6 +2188,7 @@ static PCIDeviceInfo lsi_info = {
     .qdev.alias = "lsi",
     .qdev.size  = sizeof(LSIState),
     .init       = lsi_scsi_init,
+    .exit       = lsi_scsi_uninit,
 };
 
 static void lsi53c895a_register_devices(void)
diff --git a/hw/pci.c b/hw/pci.c
index a6cd630..3221a6c 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -26,6 +26,7 @@
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
+#include "msix.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -402,13 +403,15 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
 static int pci_unregister_device(DeviceState *dev)
 {
     PCIDevice *pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
+    PCIDeviceInfo *info = DO_UPCAST(PCIDeviceInfo, qdev, dev->info);
     int ret = 0;
 
-    if (pci_dev->unregister)
-        ret = pci_dev->unregister(pci_dev);
+    if (info->exit)
+        ret = info->exit(pci_dev);
     if (ret)
         return ret;
 
+    msix_uninit(pci_dev);
     pci_unregister_io_regions(pci_dev);
 
     qemu_free_irqs(pci_dev->irq);
diff --git a/hw/pci.h b/hw/pci.h
index 0ba7e6c..5e781d8 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -329,6 +329,7 @@ typedef int (*pci_qdev_initfn)(PCIDevice *dev);
 typedef struct {
     DeviceInfo qdev;
     pci_qdev_initfn init;
+    PCIUnregisterFunc *exit;
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;
 } PCIDeviceInfo;
diff --git a/hw/pcnet.c b/hw/pcnet.c
index ae98a20..ee0c98c 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -2039,8 +2039,6 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
         sizeof(struct pcnet_RMD), sizeof(struct pcnet_TMD));
 #endif
 
-    pci_dev->unregister = pci_pcnet_uninit;
-
     pci_conf = pci_dev->config;
 
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_AMD);
@@ -2170,6 +2168,7 @@ static PCIDeviceInfo pcnet_info = {
     .qdev.name = "pcnet",
     .qdev.size = sizeof(PCIPCNetState),
     .init      = pci_pcnet_init,
+    .exit      = pci_pcnet_uninit,
 };
 
 static void pcnet_register_devices(void)
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 83cb1ff..db9cb5a 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3443,8 +3443,6 @@ static int pci_rtl8139_init(PCIDevice *dev)
     RTL8139State * s = DO_UPCAST(RTL8139State, dev, dev);
     uint8_t *pci_conf;
 
-    s->dev.unregister = pci_rtl8139_uninit;
-
     pci_conf = s->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8139);
@@ -3493,6 +3491,7 @@ static PCIDeviceInfo rtl8139_info = {
     .qdev.size  = sizeof(RTL8139State),
     .qdev.reset = rtl8139_reset,
     .init       = pci_rtl8139_init,
+    .exit       = pci_rtl8139_uninit,
 };
 
 static void rtl8139_register_devices(void)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index bd5a7c4..c6fbaac 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -417,7 +417,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
                          msix_bar_size(&proxy->pci_dev),
                          PCI_ADDRESS_SPACE_MEM,
                          msix_mmio_map);
-        proxy->pci_dev.unregister = msix_uninit;
     } else
         vdev->nvectors = 0;
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 13/13] usb: hook unplug into qdev, cleanups + fixes.

[Gerd Hoffmann]

Hook into DeviceInfo->exit().

handle_destroy() must not free the state struct, this is handled
by the new usb_qdev_exit() function now.

qdev_free(usb_device) works now.

Fix usb hub to qdev_free() all connected devices on unplug.
Unplugging a usb hub works now.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-bt.c     |    1 -
 hw/usb-bus.c    |   57 +++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/usb-hid.c    |    1 -
 hw/usb-hub.c    |    6 ++++-
 hw/usb-msd.c    |    2 -
 hw/usb-serial.c |    1 -
 hw/usb-wacom.c  |    1 -
 hw/usb.h        |    2 +
 8 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/hw/usb-bt.c b/hw/usb-bt.c
index 4c60d42..3b085cc 100644
--- a/hw/usb-bt.c
+++ b/hw/usb-bt.c
@@ -614,7 +614,6 @@ static void usb_bt_handle_destroy(USBDevice *dev)
     s->hci->opaque = NULL;
     s->hci->evt_recv = NULL;
     s->hci->acl_recv = NULL;
-    qemu_free(s);
 }
 
 static int usb_bt_initfn(USBDevice *dev)
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 2cac1e8..d0b59dd 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -50,10 +50,22 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
     return rc;
 }
 
+static int usb_qdev_exit(DeviceState *qdev)
+{
+    USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
+
+    usb_device_detach(dev);
+    if (dev->info->handle_destroy) {
+        dev->info->handle_destroy(dev);
+    }
+    return 0;
+}
+
 void usb_qdev_register(USBDeviceInfo *info)
 {
     info->qdev.bus_info = &usb_bus_info;
     info->qdev.init     = usb_qdev_init;
+    info->qdev.exit     = usb_qdev_exit;
     qdev_register(&info->qdev);
 }
 
@@ -101,6 +113,14 @@ void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index,
     bus->nfree++;
 }
 
+void usb_unregister_port(USBBus *bus, USBPort *port)
+{
+    if (port->dev)
+        qdev_free(&port->dev->qdev);
+    QTAILQ_REMOVE(&bus->free, port, next);
+    bus->nfree--;
+}
+
 static void do_attach(USBDevice *dev)
 {
     USBBus *bus = usb_bus_from_device(dev);
@@ -136,6 +156,34 @@ int usb_device_attach(USBDevice *dev)
     return 0;
 }
 
+int usb_device_detach(USBDevice *dev)
+{
+    USBBus *bus = usb_bus_from_device(dev);
+    USBPort *port;
+
+    if (!dev->attached) {
+        fprintf(stderr, "Warning: tried to detach unattached usb device %s\n",
+                dev->devname);
+        return -1;
+    }
+    dev->attached--;
+
+    QTAILQ_FOREACH(port, &bus->used, next) {
+        if (port->dev == dev)
+            break;
+    }
+    assert(port != NULL);
+
+    QTAILQ_REMOVE(&bus->used, port, next);
+    bus->nused--;
+
+    usb_attach(port, NULL);
+
+    QTAILQ_INSERT_TAIL(&bus->free, port, next);
+    bus->nfree++;
+    return 0;
+}
+
 int usb_device_delete_addr(int busnr, int addr)
 {
     USBBus *bus;
@@ -152,16 +200,9 @@ int usb_device_delete_addr(int busnr, int addr)
     }
     if (!port)
         return -1;
-
     dev = port->dev;
-    QTAILQ_REMOVE(&bus->used, port, next);
-    bus->nused--;
-
-    usb_attach(port, NULL);
-    dev->info->handle_destroy(dev);
 
-    QTAILQ_INSERT_TAIL(&bus->free, port, next);
-    bus->nfree++;
+    qdev_free(&dev->qdev);
     return 0;
 }
 
diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index 3bf06fa..d1cc45e 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -843,7 +843,6 @@ static void usb_hid_handle_destroy(USBDevice *dev)
     if (s->kind != USB_KEYBOARD)
         qemu_remove_mouse_event_handler(s->ptr.eh_entry);
     /* TODO: else */
-    qemu_free(s);
 }
 
 static int usb_hid_initfn(USBDevice *dev, int kind)
diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index 0a39986..e5a0938 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -517,8 +517,12 @@ static int usb_hub_handle_packet(USBDevice *dev, USBPacket *p)
 static void usb_hub_handle_destroy(USBDevice *dev)
 {
     USBHubState *s = (USBHubState *)dev;
+    int i;
 
-    qemu_free(s);
+    for (i = 0; i < s->nb_ports; i++) {
+        usb_unregister_port(usb_bus_from_device(dev),
+                            &s->ports[i].port);
+    }
 }
 
 static int usb_hub_initfn(USBDevice *dev)
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 6b9c8a5..843a22f 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -512,9 +512,7 @@ static void usb_msd_handle_destroy(USBDevice *dev)
 {
     MSDState *s = (MSDState *)dev;
 
-    s->scsi_dev->info->destroy(s->scsi_dev);
     drive_uninit(s->dinfo->bdrv);
-    qemu_free(s);
 }
 
 static int usb_msd_initfn(USBDevice *dev)
diff --git a/hw/usb-serial.c b/hw/usb-serial.c
index 091ab2c..e2379c4 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -486,7 +486,6 @@ static void usb_serial_handle_destroy(USBDevice *dev)
     USBSerialState *s = (USBSerialState *)dev;
 
     qemu_chr_close(s->cs);
-    qemu_free(s);
 }
 
 static int usb_serial_can_read(void *opaque)
diff --git a/hw/usb-wacom.c b/hw/usb-wacom.c
index 55f06bf..3ea7241 100644
--- a/hw/usb-wacom.c
+++ b/hw/usb-wacom.c
@@ -389,7 +389,6 @@ static void usb_wacom_handle_destroy(USBDevice *dev)
     USBWacomState *s = (USBWacomState *) dev;
 
     qemu_remove_mouse_event_handler(s->eh_entry);
-    qemu_free(s);
 }
 
 static int usb_wacom_initfn(USBDevice *dev)
diff --git a/hw/usb.h b/hw/usb.h
index 467cddb..be4fcf6 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -311,7 +311,9 @@ USBDevice *usb_create(USBBus *bus, const char *name);
 USBDevice *usb_create_simple(USBBus *bus, const char *name);
 void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index,
                        usb_attachfn attach);
+void usb_unregister_port(USBBus *bus, USBPort *port);
 int usb_device_attach(USBDevice *dev);
+int usb_device_detach(USBDevice *dev);
 int usb_device_delete_addr(int busnr, int addr);
 
 static inline USBBus *usb_bus_from_device(USBDevice *d)
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 09/13] Add exit callback to DeviceInfo.

[Christoph Egger]

On Tuesday 22 September 2009 11:29:23 Gerd Hoffmann wrote:
> This adds a exit callback for device destruction to DeviceInfo, so
> we can hook cleanups into qdev device destruction.

This is not enough.
You need a childexit to allow devices to cleanup their children
in the device tree first.

Christoph

>
> Followup patches will put that into use.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qdev.c |    2 ++
>  hw/qdev.h |    2 ++
>  2 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 4931da1..a25245a 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -245,6 +245,8 @@ void qdev_free(DeviceState *dev)
>  #endif
>          if (dev->info->reset)
>              qemu_unregister_reset(dev->info->reset, dev);
> +        if (dev->info->exit)
> +            dev->info->exit(dev);
>      }
>      QLIST_REMOVE(dev, sibling);
>      qemu_free(dev);
> diff --git a/hw/qdev.h b/hw/qdev.h
> index c036aff..0db2d32 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -107,6 +107,7 @@ BusState *qdev_get_child_bus(DeviceState *dev, const
> char *name); /*** Device API.  ***/
>
>  typedef int (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
> +typedef int (*qdev_exitfn)(DeviceState *dev);
>
>  struct DeviceInfo {
>      const char *name;
> @@ -124,6 +125,7 @@ struct DeviceInfo {
>
>      /* Private to qdev / bus.  */
>      qdev_initfn init;
> +    qdev_exitfn exit;
>      BusInfo *bus_info;
>      struct DeviceInfo *next;
>  };



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 09/13] Add exit callback to DeviceInfo.

[Gerd Hoffmann]

On 09/22/09 12:06, Christoph Egger wrote:
> On Tuesday 22 September 2009 11:29:23 Gerd Hoffmann wrote:
>> This adds a exit callback for device destruction to DeviceInfo, so
>> we can hook cleanups into qdev device destruction.
>
> This is not enough.
> You need a childexit to allow devices to cleanup their children
> in the device tree first.

qdev will do that for you.  Check out patch #8 and the new qbus_free() 
function added there.

Of course the bus implementations must make use of the exit hook so it 
actually works, the following patches do that for pci, scsi and usb.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 12/13] pci: move unregister from PCIDevice to PCIDeviceInfo

[Michael S. Tsirkin]

On Tue, Sep 22, 2009 at 11:29:26AM +0200, Gerd Hoffmann wrote:
> diff --git a/hw/pci.c b/hw/pci.c
> index a6cd630..3221a6c 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -26,6 +26,7 @@
>  #include "monitor.h"
>  #include "net.h"
>  #include "sysemu.h"
> +#include "msix.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -402,13 +403,15 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
>  static int pci_unregister_device(DeviceState *dev)
>  {
>      PCIDevice *pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
> +    PCIDeviceInfo *info = DO_UPCAST(PCIDeviceInfo, qdev, dev->info);
>      int ret = 0;
>  
> -    if (pci_dev->unregister)
> -        ret = pci_dev->unregister(pci_dev);
> +    if (info->exit)
> +        ret = info->exit(pci_dev);
>      if (ret)
>          return ret;
>  
> +    msix_uninit(pci_dev);
>      pci_unregister_io_regions(pci_dev);
>  
>      qemu_free_irqs(pci_dev->irq);

Since devices call msix_add, I think it is cleaner to have them
uninit it as well in their exit routines.

-- 
MST

Re: [Qemu-devel] [PATCH 06/13] convert pci bridge to qdev

[Michael S. Tsirkin]

On Tue, Sep 22, 2009 at 11:29:20AM +0200, Gerd Hoffmann wrote:
> @@ -890,15 +890,12 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
>      return bus->devices[PCI_DEVFN(slot, function)];
>  }
>  
> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
> -                        pci_map_irq_fn map_irq, const char *name)
> +static int pci_bridge_initfn(PCIDevice *dev)

pci_bridge_dev_init a better name?

>  {
> -    PCIBridge *s;
> -    s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge),
> -                                         devfn, NULL, pci_bridge_write_config);
> +    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
>  
> -    pci_config_set_vendor_id(s->dev.config, vid);
> -    pci_config_set_device_id(s->dev.config, did);
> +    pci_config_set_vendor_id(s->dev.config, s->vid);
> +    pci_config_set_device_id(s->dev.config, s->did);
>  
>      s->dev.config[0x04] = 0x06; // command = bus master, pci mem
>      s->dev.config[0x05] = 0x00;
> @@ -911,9 +908,23 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
>      s->dev.config[PCI_HEADER_TYPE] =
>          PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type
>      s->dev.config[0x1E] = 0xa0; // secondary status
> +    return 0;
> +}
>  
> -    s->bus = pci_register_secondary_bus(&s->dev, map_irq, name);
> -    return s->bus;
> +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
> +                        pci_map_irq_fn map_irq, const char *name)
> +{
> +    PCIDevice *dev;
> +    PCIBridge *s;
> +
> +    dev = pci_create_noinit(bus, devfn, "pci-bridge");
> +    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
> +    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
> +    qdev_init(&dev->qdev);
> +
> +    s = DO_UPCAST(PCIBridge, dev, dev);
> +    pci_register_secondary_bus(&s->bus, &s->dev, map_irq, name);
> +    return &s->bus;
>  }
>  
>  static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
> @@ -1074,3 +1085,22 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>                         r->addr, r->addr + r->size - 1);
>      }
>  }
> +
> +static PCIDeviceInfo bridge_info = {
> +    .qdev.name    = "pci-bridge",
> +    .qdev.size    = sizeof(PCIBridge),
> +    .init         = pci_bridge_initfn,
> +    .config_write = pci_bridge_write_config,
> +    .qdev.props   = (Property[]) {
> +        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
> +        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void pci_register_devices(void)
> +{
> +    pci_qdev_register(&bridge_info);
> +}
> +
> +device_init(pci_register_devices)
> -- 
> 1.6.2.5
> 
> 

Re: [Qemu-devel] [PATCH 06/13] convert pci bridge to qdev

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Tue, Sep 22, 2009 at 11:29:20AM +0200, Gerd Hoffmann wrote:
>> @@ -890,15 +890,12 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
>>      return bus->devices[PCI_DEVFN(slot, function)];
>>  }
>>  
>> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
>> -                        pci_map_irq_fn map_irq, const char *name)
>> +static int pci_bridge_initfn(PCIDevice *dev)
>
> pci_bridge_dev_init a better name?

There's precedence for _initfn, _init1, and _init.  I wish we'd pick one
convention and stick to it.

[...]

Re: [Qemu-devel] [PATCH 07/13] piix_pci: kill PIIX3IrqState

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> The splitted init + irq setup for the pci bus untangles a initialization
> order issue and allows to remove the PIIX3IrqState struct.  We can
> carry the state directly in PIIX3State insted.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/piix_pci.c |   37 +++++++++++++++++--------------------
>  1 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index edd6df0..0be3b02 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
[...]
> @@ -232,23 +228,24 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
>      PCIBus *b;
>      PCIDevice *d;
>      I440FXState *s;
> -    PIIX3IrqState *irq_state = qemu_malloc(sizeof(*irq_state));
> +    PIIX3State *piix3;
>  
> -    irq_state->pic = pic;
>      dev = qdev_create(NULL, "i440FX-pcihost");
>      s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
> -    b = pci_register_bus(&s->busdev.qdev, "pci.0",
> -                         piix3_set_irq, pci_slot_get_pirq, irq_state, 0, 4);
> +    b = pci_bus_new(&s->busdev.qdev, NULL, 0);

Passes null name to pci_bus_new().  Intentional?  Before, we passed
"pci.0" to pci_register_bus().

>      s->bus = b;
>      qdev_init(dev);
>  
>      d = pci_create_simple(b, 0, "i440FX");
>      *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
> -    (*pi440fx_state)->irq_state = irq_state;
>  
> -    irq_state->piix3 = DO_UPCAST(PIIX3State, dev,
> +    piix3 = DO_UPCAST(PIIX3State, dev,
>                                   pci_create_simple(b, -1, "PIIX3"));
> -    *piix3_devfn = irq_state->piix3->dev.devfn;
> +    piix3->pic = pic;
> +    pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3, 4);
> +    (*pi440fx_state)->piix3 = piix3;
> +
> +    *piix3_devfn = piix3->dev.devfn;
>  
>      return b;
>  }
[...]

Re: [Qemu-devel] [PATCH 06/13] convert pci bridge to qdev

[Michael S. Tsirkin]

On Thu, Sep 24, 2009 at 08:29:56PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, Sep 22, 2009 at 11:29:20AM +0200, Gerd Hoffmann wrote:
> >> @@ -890,15 +890,12 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
> >>      return bus->devices[PCI_DEVFN(slot, function)];
> >>  }
> >>  
> >> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
> >> -                        pci_map_irq_fn map_irq, const char *name)
> >> +static int pci_bridge_initfn(PCIDevice *dev)
> >
> > pci_bridge_dev_init a better name?
> 
> There's precedence for _initfn, _init1, and _init.  I wish we'd pick one
> convention and stick to it.

The problem here is that we now have *both* pci_bridge_init and
pci_bridge_initfn.

> [...]

Re: [Qemu-devel] [PATCH 08/13] qdev: device free fixups.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Two bug fixes:
>  * When freeing a device we unregister stuff unconditionally,
>    even in case we didn't register in the first place because
>    the ->init() callback failed.
>  * When freeing a device with child busses attached we don't
>    zap the child bus (and the devices attached to it).
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Maybe I'm just having a particularly dense day, but I misread this as
"this is what the fix does".  In reality, it's "this is what we did
wrong".  Careless readers like me can be kept on track by emphasizing
the "wrongness" of what's being done, so that it's (more) obvious we're
talking about the bug and not the fix:

  * When freeing a device we unregister even stuff we didn't register in
    the first place because the ->init() callback failed.
  * When freeing a device with child busses attached, we fail to zap the
    child bus (and the devices attached to it).

Another language trick is to use past tense for the bug and present
tense for the fix.

Re: [Qemu-devel] [PATCH 09/13] Add exit callback to DeviceInfo.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> This adds a exit callback for device destruction to DeviceInfo, so
> we can hook cleanups into qdev device destruction.
>
> Followup patches will put that into use.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qdev.c |    2 ++
>  hw/qdev.h |    2 ++
>  2 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 4931da1..a25245a 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -245,6 +245,8 @@ void qdev_free(DeviceState *dev)
>  #endif
>          if (dev->info->reset)
>              qemu_unregister_reset(dev->info->reset, dev);
> +        if (dev->info->exit)
> +            dev->info->exit(dev);
>      }
>      QLIST_REMOVE(dev, sibling);
>      qemu_free(dev);
> diff --git a/hw/qdev.h b/hw/qdev.h
> index c036aff..0db2d32 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -107,6 +107,7 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
>  /*** Device API.  ***/
>  
>  typedef int (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
> +typedef int (*qdev_exitfn)(DeviceState *dev);
>  
>  struct DeviceInfo {
>      const char *name;
> @@ -124,6 +125,7 @@ struct DeviceInfo {
>  
>      /* Private to qdev / bus.  */
>      qdev_initfn init;
> +    qdev_exitfn exit;
>      BusInfo *bus_info;
>      struct DeviceInfo *next;
>  };

I don't like the name "exit".  The buddy of init() is often called
fini().

Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/scsi-bus.c     |   24 +++++++++++++++++++++---
>  hw/scsi-disk.c    |    6 ------
>  hw/scsi-generic.c |    2 --
>  3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 881e363..27defc4 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -30,6 +30,7 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
>      SCSIDeviceInfo *info = DO_UPCAST(SCSIDeviceInfo, qdev, base);
>      SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +    int rc = -1;
>  
>      if (dev->id == -1) {
>          for (dev->id = 0; dev->id < bus->ndev; dev->id++) {
> @@ -43,21 +44,38 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      }
>  
>      if (bus->devs[dev->id]) {
> -        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
> +        qdev_free(&bus->devs[dev->id]->qdev);
>      }

If I understand this correctly, SCSI devices "overwrite": if you add a
new one with an existing SCSI ID, the old one gets disconnected
automatically.  Isn't that inconsistent with other buses?  PCI,
specifically.  Question applies before your patch already.

>      bus->devs[dev->id] = dev;
>  
>      dev->info = info;
> -    return dev->info->init(dev);
> +    rc = dev->info->init(dev);
> +    if (rc != 0) {
> +        bus->devs[dev->id] = NULL;
> +    }
>  
>  err:
> -    return -1;
> +    return rc;
> +}
> +
> +static int scsi_qdev_exit(DeviceState *qdev)
> +{
> +    SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
> +    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +
> +    assert(bus->devs[dev->id] != NULL);
> +    if (bus->devs[dev->id]->info->destroy) {
> +        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
> +    }
> +    bus->devs[dev->id] = NULL;
> +    return 0;
>  }

You have scsi_qdev_exit() as qdev callback exit().  It does the generic
stuff, then runs the SCSIDevice callback destroy() for the specific
stuff.  Shouldn't the two callbacks be named the same, to make their
relation more obvious?

>  
>  void scsi_qdev_register(SCSIDeviceInfo *info)
>  {
>      info->qdev.bus_info = &scsi_bus_info;
>      info->qdev.init     = scsi_qdev_init;
> +    info->qdev.exit     = scsi_qdev_exit;
>      qdev_register(&info->qdev);
>  }
>  
[...]

Re: [Qemu-devel] [PATCH 00/13] qdev: bus management updates.

[Markus Armbruster]

Your improved commit messages are much appreciated.  Run them through a
spellchecker, and they're perfect ;)

Re: [Qemu-devel] [PATCH 06/13] convert pci bridge to qdev

[Gerd Hoffmann]

On 09/24/09 20:51, Michael S. Tsirkin wrote:
> On Thu, Sep 24, 2009 at 08:29:56PM +0200, Markus Armbruster wrote:
>> "Michael S. Tsirkin"<mst@redhat.com>  writes:
>>
>>> On Tue, Sep 22, 2009 at 11:29:20AM +0200, Gerd Hoffmann wrote:
>>>> @@ -890,15 +890,12 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
>>>>       return bus->devices[PCI_DEVFN(slot, function)];
>>>>   }
>>>>
>>>> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
>>>> -                        pci_map_irq_fn map_irq, const char *name)
>>>> +static int pci_bridge_initfn(PCIDevice *dev)
>>>
>>> pci_bridge_dev_init a better name?
>>
>> There's precedence for _initfn, _init1, and _init.  I wish we'd pick one
>> convention and stick to it.
>
> The problem here is that we now have *both* pci_bridge_init and
> pci_bridge_initfn.
>
>> [...]

There are quite a few places which are simliar named.  *_initfn is the 
qdev init callback, *_init is the convenience wrapper to keep the old 
code paths working, which ideally just does qdev_create() + set 
properties + qdev_init().  We are not there (yet) in all cases though.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 07/13] piix_pci: kill PIIX3IrqState

[Gerd Hoffmann]

   Hi,

>> -    b = pci_register_bus(&s->busdev.qdev, "pci.0",
>> -                         piix3_set_irq, pci_slot_get_pirq, irq_state, 0, 4);
>> +    b = pci_bus_new(&s->busdev.qdev, NULL, 0);
>
> Passes null name to pci_bus_new().  Intentional?  Before, we passed
> "pci.0" to pci_register_bus().

Yes.  qbus_create() will construct a name for you, which happens to be 
"pci.0".  So we don't need to pass it in.  Having "pci.0" in there was 
just temporary needed due to the order patches where merged.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 09/13] Add exit callback to DeviceInfo.

[Gerd Hoffmann]

>>       qdev_initfn init;
>> +    qdev_exitfn exit;
>>       BusInfo *bus_info;
>>       struct DeviceInfo *next;
>>   };
>
> I don't like the name "exit".  The buddy of init() is often called
> fini().

Picked that name convention up from the linux kernel which has 
module_init() and module_exit() ...

I can change it, but I'd like to have some kind of agreement on the list 
so I don't have to go through all the patches changing this twice. 
Naming variants I can thing of are:

   * exit
   * fini
   * free
   * uninit
   * cleanup
   * release

Comments?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Gerd Hoffmann]

   Hi,

> If I understand this correctly, SCSI devices "overwrite": if you add a
> new one with an existing SCSI ID, the old one gets disconnected
> automatically.  Isn't that inconsistent with other buses?  PCI,
> specifically.  Question applies before your patch already.

Yes, this is correct.
I've just maintained current behavior.

To extend that question:  While playing with that I've noticed linux 
does not automagically find the a scsi disk hot-plugged in.  After 
reboot (and the scsi bus rescan triggered by that) it finds the disk. 
Reloading the driver module probably would have worked too.  We don't 
signal the guest in any way it got a new disk, so this isn't exactly 
surprising.  Is this just a emulation limitation?  Or a limitation of 
the emulated scsi host adapters?

>> +static int scsi_qdev_exit(DeviceState *qdev)
>> +{
>> +    SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
>> +    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
>> +
>> +    assert(bus->devs[dev->id] != NULL);
>> +    if (bus->devs[dev->id]->info->destroy) {
>> +        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
>> +    }
>> +    bus->devs[dev->id] = NULL;
>> +    return 0;
>>   }
>
> You have scsi_qdev_exit() as qdev callback exit().  It does the generic
> stuff, then runs the SCSIDevice callback destroy() for the specific
> stuff.  Shouldn't the two callbacks be named the same, to make their
> relation more obvious?

I'm just using the existing callbacks and windup stuff correctly in the 
qdev_free() paths.  Make the naming more consistent would be useful 
indeed, but that is IMHO a job for a separate patch.  And we should 
agree on a naming convention first ;)

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 12/13] pci: move unregister from PCIDevice to PCIDeviceInfo

[Gerd Hoffmann]

   Hi,

>>   static int pci_unregister_device(DeviceState *dev)

>> +    msix_uninit(pci_dev);

> Since devices call msix_add, I think it is cleaner to have them
> uninit it as well in their exit routines.

Would work too.  But this way you can't miss the msix_uninit() call by 
accident.  It also saves a few lines of code.  msix_uninit() carefully 
checks whenever msix is actually enabled and it is slow path, so calling 
it on non-msix devices isn't a big deal IMHO.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> If I understand this correctly, SCSI devices "overwrite": if you add a
>> new one with an existing SCSI ID, the old one gets disconnected
>> automatically.  Isn't that inconsistent with other buses?  PCI,
>> specifically.  Question applies before your patch already.
>
> Yes, this is correct.
> I've just maintained current behavior.
>
> To extend that question:  While playing with that I've noticed linux
> does not automagically find the a scsi disk hot-plugged in.  After
> reboot (and the scsi bus rescan triggered by that) it finds the
> disk. Reloading the driver module probably would have worked too.  We
> don't signal the guest in any way it got a new disk, so this isn't
> exactly surprising.  Is this just a emulation limitation?  Or a
> limitation of the emulated scsi host adapters?

What appens when you hotplug or just switch on a real SCSI device?  Does
Linux pick it up automatically?  Long ago when I last tried that, I
think I had to do a magic write to sysfs to make it look for the device.

>>> +static int scsi_qdev_exit(DeviceState *qdev)
>>> +{
>>> +    SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
>>> +    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
>>> +
>>> +    assert(bus->devs[dev->id] != NULL);
>>> +    if (bus->devs[dev->id]->info->destroy) {
>>> +        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
>>> +    }
>>> +    bus->devs[dev->id] = NULL;
>>> +    return 0;
>>>   }
>>
>> You have scsi_qdev_exit() as qdev callback exit().  It does the generic
>> stuff, then runs the SCSIDevice callback destroy() for the specific
>> stuff.  Shouldn't the two callbacks be named the same, to make their
>> relation more obvious?
>
> I'm just using the existing callbacks and windup stuff correctly in
> the qdev_free() paths.  Make the naming more consistent would be
> useful indeed, but that is IMHO a job for a separate patch.  And we
> should agree on a naming convention first ;)

Fair enough.

Tasteful naming *is* important.

Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Gerd Hoffmann]

   Hi,

>> To extend that question:  While playing with that I've noticed linux
>> does not automagically find the a scsi disk hot-plugged in.  After
>> reboot (and the scsi bus rescan triggered by that) it finds the
>> disk. Reloading the driver module probably would have worked too.  We
>> don't signal the guest in any way it got a new disk, so this isn't
>> exactly surprising.  Is this just a emulation limitation?  Or a
>> limitation of the emulated scsi host adapters?
>
> What appens when you hotplug or just switch on a real SCSI device?  Does
> Linux pick it up automatically?  Long ago when I last tried that, I
> think I had to do a magic write to sysfs to make it look for the device.

Old, classic, parallel scsi (with the 50wire cable) works that way.
But IIRC there are also more modern variants which can handle hotplug.

Dunno how that works though and whenever we could do that with the lsi 
adapter we are emulating ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Artyom Tarasenko]

2009/9/25 Markus Armbruster <armbru@redhat.com>:
> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>>   Hi,
>>
>>> If I understand this correctly, SCSI devices "overwrite": if you add a
>>> new one with an existing SCSI ID, the old one gets disconnected
>>> automatically.  Isn't that inconsistent with other buses?  PCI,
>>> specifically.  Question applies before your patch already.
>>
>> Yes, this is correct.
>> I've just maintained current behavior.
>>
>> To extend that question:  While playing with that I've noticed linux
>> does not automagically find the a scsi disk hot-plugged in.  After
>> reboot (and the scsi bus rescan triggered by that) it finds the
>> disk. Reloading the driver module probably would have worked too.  We
>> don't signal the guest in any way it got a new disk, so this isn't
>> exactly surprising.  Is this just a emulation limitation?  Or a
>> limitation of the emulated scsi host adapters?
>
> What appens when you hotplug or just switch on a real SCSI device?  Does
> Linux pick it up automatically?  Long ago when I last tried that, I
> think I had to do a magic write to sysfs to make it look for the device.

I guess it wasn't magic, but an official way of doing it. At least
SCSI-2 controllers didn't get any notification signal on a hot-plug.
I used to do it with
echo "scsi add-single-device 0 0 3 0" > /proc/scsi/scsi

Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Markus Armbruster]

Artyom Tarasenko <atar4qemu@googlemail.com> writes:

> 2009/9/25 Markus Armbruster <armbru@redhat.com>:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
[...]
>>> To extend that question:  While playing with that I've noticed linux
>>> does not automagically find the a scsi disk hot-plugged in.  After
>>> reboot (and the scsi bus rescan triggered by that) it finds the
>>> disk. Reloading the driver module probably would have worked too.  We
>>> don't signal the guest in any way it got a new disk, so this isn't
>>> exactly surprising.  Is this just a emulation limitation?  Or a
>>> limitation of the emulated scsi host adapters?
>>
>> What appens when you hotplug or just switch on a real SCSI device?  Does
>> Linux pick it up automatically?  Long ago when I last tried that, I
>> think I had to do a magic write to sysfs to make it look for the device.
>
> I guess it wasn't magic, but an official way of doing it. At least
> SCSI-2 controllers didn't get any notification signal on a hot-plug.
> I used to do it with
> echo "scsi add-single-device 0 0 3 0" > /proc/scsi/scsi

Call it "official magic" then ;)

Re: [Qemu-devel] [PATCH 12/13] pci: move unregister from PCIDevice to PCIDeviceInfo

[Michael S. Tsirkin]

On Fri, Sep 25, 2009 at 09:49:16AM +0200, Gerd Hoffmann wrote:
>   Hi,
>
>>>   static int pci_unregister_device(DeviceState *dev)
>
>>> +    msix_uninit(pci_dev);
>
>> Since devices call msix_add, I think it is cleaner to have them
>> uninit it as well in their exit routines.
>
> Would work too.  But this way you can't miss the msix_uninit() call by  
> accident.

What kind of accident? We don't want a garbage collector in here, do we?

>  It also saves a few lines of code.  msix_uninit() carefully  
> checks whenever msix is actually enabled and it is slow path, so calling  
> it on non-msix devices isn't a big deal IMHO.
>
> cheers,
>   Gerd

Yes, it works, but I find it confusing: IMO init and uninit being in
separate modules just makes code impossible to figure out.  For example,
if there's a failure adding the device, device has to call msix_uninit
itself, and IMO it's better to have error handling and cleanup in sync.

-- 
MST

Re: [Qemu-devel] [PATCH 12/13] pci: move unregister from PCIDevice to PCIDeviceInfo

[Gerd Hoffmann]

   Hi,

> Yes, it works, but I find it confusing: IMO init and uninit being in
> separate modules just makes code impossible to figure out.

Ok, I'll change it for the next respin.

cheers
   Gerd