[Qemu-devel] [PATCH 0/4] SCSI-hotdel: Implement removal of SCSI hard disks

[Qemu-devel] [PATCH 0/4] SCSI-hotdel: Implement removal of SCSI hard disks

[Wolfgang Mauerer]

Hi,

since Qemu has support for adding disk drives to SCSI controllers
at runtime, it should also have support for the inverse operation.
This patch series introduces support for drive hot-removal by
introducing a new monitor command: drive_del.

Notice that support for libvirt is also available and was 
cross-posted to qemu-devel for those who are interested.

Thanks, Wolfgang

 hw/lsi53c895a.c  |   16 +++++++++
 hw/pci-hotplug.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/pci.h         |    1 +
 qemu-monitor.hx  |   14 +++++++-
 sysemu.h         |    2 +
 vl.c             |   69 ++++++++++++++++++++++-----------------
 6 files changed, 164 insertions(+), 33 deletions(-)

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 1/4] SCSI-hotdel: Add detach method to LSI SCSI driver

[Wolfgang Mauerer]

Introduce lsi_scsi_detach, which destroys and de-registers
a device from the LSI SCSI host

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/lsi53c895a.c |   16 ++++++++++++++++
 hw/pci.h        |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 62bdca8..7e975b4 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1959,6 +1959,22 @@ static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
     cpu_register_physical_memory(addr + 0, 0x400, s->mmio_io_addr);
 }
 
+void lsi_scsi_detach(DeviceState *host, BlockDriverState *bd, int id)
+{
+    LSIState *s = (LSIState *)host;
+
+    if (id >= LSI_MAX_DEVS || id < 0) {
+        BADF("Bad Device ID %d\n", id);
+        return;
+    }
+
+    if (s->bus->devs[id]) {
+        DPRINTF("Destroying device %d\n", id);
+        s->bus->devs[id]->info->destroy(s->bus->devs[id]);
+    }
+    s->bus->devs[id] = NULL;
+}
+
 static void lsi_scsi_save(QEMUFile *f, void *opaque)
 {
     LSIState *s = opaque;
diff --git a/hw/pci.h b/hw/pci.h
index 4e901be..1127c87 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -382,6 +382,7 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
 /* lsi53c895a.c */
 #define LSI_MAX_DEVS 7
+void lsi_scsi_detach(DeviceState *host, BlockDriverState *bd, int id);
 
 /* vmware_vga.c */
 void pci_vmsvga_init(PCIBus *bus);
-- 
1.6.4

[Qemu-devel] [PATCH 2/4] SCSI-hotdel: Factor out IF argument parsing

[Wolfgang Mauerer]

This avoids some small code duplication later on.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 sysemu.h |    1 +
 vl.c     |   69 +++++++++++++++++++++++++++++++++++---------------------------
 2 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index e79e4ac..4fabb2a 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -194,6 +194,7 @@ extern int drive_get_max_bus(BlockInterfaceType type);
 extern void drive_uninit(BlockDriverState *bdrv);
 extern const char *drive_get_serial(BlockDriverState *bdrv);
 extern BlockInterfaceErrorAction drive_get_onerror(BlockDriverState *bdrv);
+BlockInterfaceType qemu_opt_get_if(const char* buf);
 
 BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type);
 
diff --git a/vl.c b/vl.c
index 88bd3eb..bd0c2be 100644
--- a/vl.c
+++ b/vl.c
@@ -1903,6 +1903,31 @@ void drive_uninit(BlockDriverState *bdrv)
     }
 }
 
+BlockInterfaceType qemu_opt_get_if(const char* buf) {
+    BlockInterfaceType type;
+    
+    type = IF_COUNT;
+    if (!strcmp(buf, "ide")) {
+        type = IF_IDE;
+    } else if (!strcmp(buf, "scsi")) {
+        type = IF_SCSI;
+    } else if (!strcmp(buf, "floppy")) {
+        type = IF_FLOPPY;
+    } else if (!strcmp(buf, "pflash")) {
+        type = IF_PFLASH;
+    } else if (!strcmp(buf, "mtd")) {
+        type = IF_MTD;
+    } else if (!strcmp(buf, "sd")) {
+        type = IF_SD;
+    } else if (!strcmp(buf, "virtio")) {
+        type = IF_VIRTIO;
+    } else if (!strcmp(buf, "xen")) {
+        type = IF_XEN;
+    } 
+
+    return type;
+}
+
 DriveInfo *drive_init(QemuOpts *opts, void *opaque,
                       int *fatal_error)
 {
@@ -1959,39 +1984,23 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 
     if ((buf = qemu_opt_get(opts, "if")) != NULL) {
         pstrcpy(devname, sizeof(devname), buf);
-        if (!strcmp(buf, "ide")) {
-	    type = IF_IDE;
-            max_devs = MAX_IDE_DEVS;
-        } else if (!strcmp(buf, "scsi")) {
-	    type = IF_SCSI;
-            max_devs = MAX_SCSI_DEVS;
-        } else if (!strcmp(buf, "floppy")) {
-	    type = IF_FLOPPY;
-            max_devs = 0;
-        } else if (!strcmp(buf, "pflash")) {
-	    type = IF_PFLASH;
-            max_devs = 0;
-	} else if (!strcmp(buf, "mtd")) {
-	    type = IF_MTD;
-            max_devs = 0;
-	} else if (!strcmp(buf, "sd")) {
-	    type = IF_SD;
-            max_devs = 0;
-        } else if (!strcmp(buf, "virtio")) {
-            type = IF_VIRTIO;
-            max_devs = 0;
-	} else if (!strcmp(buf, "xen")) {
-	    type = IF_XEN;
-            max_devs = 0;
-	} else if (!strcmp(buf, "none")) {
-	    type = IF_NONE;
+	type = qemu_opt_get_if(buf);
+	switch (type) {
+	case IF_IDE:
+	    max_devs = MAX_IDE_DEVS;
+	    break;
+	case IF_SCSI:
+	    max_devs = MAX_SCSI_DEVS;
+	    break;
+	case IF_COUNT:
             max_devs = 0;
-	} else {
-            fprintf(stderr, "qemu: unsupported bus type '%s'\n", buf);
-            return NULL;
+	    fprintf(stderr, "qemu: unsupported bus type '%s'\n", buf);
+	    return NULL; 
+	default:
+	    max_devs = 0;
 	}
     }
-
+	
     if (cyls || heads || secs) {
         if (cyls < 1 || cyls > 16383) {
             fprintf(stderr, "qemu: '%s' invalid physical cyls number\n", buf);
-- 
1.6.4

[Qemu-devel] [PATCH 3/4] SCSI-Hotdel: Implement drive_hot_del

[Wolfgang Mauerer]

This commit implements the method drive_hot_del() that allows
for hot-removing drives. It also introduces a new
monitor command drive_del, which acts as counterpart
to drive_add.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pci-hotplug.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-monitor.hx  |   14 +++++++-
 sysemu.h         |    1 +
 3 files changed, 106 insertions(+), 1 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index b8ea9ae..4455f17 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -50,6 +50,98 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
     return pci_nic_init(&nd_table[ret], "rtl8139", devaddr);
 }
 
+void drive_hot_del(Monitor *mon, const QDict *qdict)
+{
+    int dom, pci_bus;
+    unsigned slot;
+    unsigned unit, bus; 
+    int type;
+    int success = 0;
+    PCIDevice *dev;
+    DriveInfo *dinfo;
+    BlockDriverState *bs;
+    char buf[128];
+    const char *strbuf;
+    QemuOpts *opts;
+
+    static const char * const params[] = { "bus", "unit", "if", NULL };
+    const char *pci_addr = qdict_get_str(qdict, "pci_addr");
+    const char *optstring = qdict_get_try_str(qdict, "opts");
+
+     /* strip legacy tag */
+    if (!strncmp(pci_addr, "pci_addr=", 9)) {
+        pci_addr += 9;
+    }
+
+    if (check_params(buf, sizeof(buf), params, optstring) < 0) {
+        monitor_printf(mon, "qemu: unknown parameter '%s' in '%s'\n",
+                         buf, optstring);
+	return;
+    }
+    opts = drive_add(NULL, "%s", optstring); // Just parses optstring
+
+    if (pci_read_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) {
+        monitor_printf(mon, "Invalid pci address\n");
+        return;
+    }
+
+    dev = pci_find_device(pci_bus, slot, 0);
+    if (!dev) {
+        monitor_printf(mon, "No pci device with address %s\n", pci_addr);
+        return;
+    }
+
+    bus = qemu_opt_get_number(opts, "bus", -1);
+    if (bus == -1) {
+        monitor_printf(mon, "Need to specify bus\n");
+        return;
+    }
+
+    unit = qemu_opt_get_number(opts, "unit", -1);
+    if (unit == -1) {
+        monitor_printf(mon, "Need to specify unit\n");
+        return;
+    }
+
+    if ((strbuf = qemu_opt_get(opts, "if")) != NULL) {
+        type = qemu_opt_get_if(strbuf);
+
+	if (type == IF_COUNT) {
+	    monitor_printf(mon, "Trying to remove device on invalid bus\n");
+	    return;
+	}
+     } else {
+         /* Assume SCSI for hot-remove per default */
+         type = IF_SCSI;
+     }
+
+     dinfo = drive_get(type, bus, unit);
+     if (!dinfo) {
+         monitor_printf(mon, "Trying to remove non-existent device\n");
+	 return;
+     }
+
+    switch (type) {
+    case IF_SCSI:
+        success = 1;
+        lsi_scsi_detach(&dev->qdev, dinfo->bdrv, dinfo->unit);
+
+        bs = dinfo->bdrv;
+        drive_uninit(bs);
+        bdrv_delete(bs);
+        break;
+    default:
+        monitor_printf(mon, "Hot-remove unsupported for interface type %d\n", 
+		       type);
+    }
+
+    if (success) 
+        monitor_printf(mon, "OK bus %d, unit %d\n", bus, unit);
+
+    return;
+}
+
+
 void drive_hot_add(Monitor *mon, const QDict *qdict)
 {
     int dom, pci_bus;
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index a889fed..aee5ff9 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -510,7 +510,19 @@ Add drive to PCI storage controller.
 ETEXI
 
 #if defined(TARGET_I386)
-    { "pci_add", "pci_addr:s,type:s,opts:s?", pci_device_hot_add, "auto|[[<domain>:]<bus>:]<slot> nic|storage|host [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]... [host=02:00.0[,name=string][,dma=none]", "hot-add PCI device" },
+    { "drive_del", "ss", drive_hot_del, "[[<domain>:]<bus>:]<slot>\n"
+                                         "bus=n,unit=m[,if=type]\n",
+                                         "Delete drive from PCI storage controller" },
+#endif
+STEXI
+@item drive_del
+Delete drive from PCI storage controller. The device must be identified
+by the bus and unit parameters. Currently, only the LSI SCSI driver
+supports drive removal.
+ETEXI
+
+#if defined(TARGET_I386)
+    { "pci_add", "sss?", pci_device_hot_add, "auto|[[<domain>:]<bus>:]<slot> nic|storage|host [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]... [host=02:00.0[,name=string][,dma=none]", "hot-add PCI device" },
 #endif
 STEXI
 @item pci_add
diff --git a/sysemu.h b/sysemu.h
index 4fabb2a..0c08b48 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -218,6 +218,7 @@ void destroy_bdrvs(dev_match_fn *match_fn, void *arg);
 /* pci-hotplug */
 void pci_device_hot_add(Monitor *mon, const QDict *qdict);
 void drive_hot_add(Monitor *mon, const QDict *qdict);
+void drive_hot_del(Monitor *mon, const QDict *qdict);
 void pci_device_hot_remove(Monitor *mon, const char *pci_addr);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
 void pci_device_hot_remove_success(int pcibus, int slot);
-- 
1.6.4

[Qemu-devel] [PATCH 4/4] Small cleanup for drive_hot_add

[Wolfgang Mauerer]

Remove an unused variable.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pci-hotplug.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 4455f17..e62a2b0 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -146,7 +146,7 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
 {
     int dom, pci_bus;
     unsigned slot;
-    int type, bus;
+    int type;
     int success = 0;
     PCIDevice *dev;
     DriveInfo *dinfo;
@@ -172,7 +172,6 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
         return;
     }
     type = dinfo->type;
-    bus = drive_get_max_bus (type);
 
     switch (type) {
     case IF_SCSI:
-- 
1.6.4

Re: [Qemu-devel] [PATCH 3/4] SCSI-Hotdel: Implement drive_hot_del

[Gerd Hoffmann]

   Hi,

> +     dinfo = drive_get(type, bus, unit);
> +     if (!dinfo) {
> +         monitor_printf(mon, "Trying to remove non-existent device\n");
> +	 return;
> +     }

No.  Just don't do this silly if/bus/unit parsing.  At very minimum use 
drive_get_by_id() here, then have something like 'drive_del $id'.

IMHO much better would be to go qdev instead though.  We should have 
generic device_add + device_del monitor commands which work for any 
device, pretty much like the -device command line switch.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/4] SCSI-Hotdel: Implement drive_hot_del

[Wolfgang Mauerer]

Gerd Hoffmann wrote:
>    Hi,
> 
>> +     dinfo = drive_get(type, bus, unit);
>> +     if (!dinfo) {
>> +         monitor_printf(mon, "Trying to remove non-existent device\n");
>> +	 return;
>> +     }
> 
> No.  Just don't do this silly if/bus/unit parsing.  At very minimum use 
> drive_get_by_id() here, then have something like 'drive_del $id'.
> 
> IMHO much better would be to go qdev instead though.  We should have 
> generic device_add + device_del monitor commands which work for any 
> device, pretty much like the -device command line switch.

That makes sense, but I'd nevertheless prefer to stick with the
more traditional approach right now, replacing the
bus/unit parsing with an ID-based variant. However, is there any
standard way to get from an instance of DriveInfo to
the corresponding instance of SCSIDevice respectively
SCSIDeviceInfo? It's a bit unclear to me if there is
no such connection, of if I'm just overlooking something.

Thanks, Wolfgang

Re: [Qemu-devel] [PATCH 3/4] SCSI-Hotdel: Implement drive_hot_del

[Gerd Hoffmann]

On 09/30/09 17:45, Wolfgang Mauerer wrote:
> Gerd Hoffmann wrote:
>>     Hi,
>>
>>> +     dinfo = drive_get(type, bus, unit);
>>> +     if (!dinfo) {
>>> +         monitor_printf(mon, "Trying to remove non-existent device\n");
>>> +	 return;
>>> +     }
>>
>> No.  Just don't do this silly if/bus/unit parsing.  At very minimum use
>> drive_get_by_id() here, then have something like 'drive_del $id'.
>>
>> IMHO much better would be to go qdev instead though.  We should have
>> generic device_add + device_del monitor commands which work for any
>> device, pretty much like the -device command line switch.
>
> That makes sense, but I'd nevertheless prefer to stick with the
> more traditional approach right now, replacing the
> bus/unit parsing with an ID-based variant.

Have a look at the "qdev: bus management updates." patch series posted a 
few days ago.  It adds device_add+device_del.

> However, is there any
> standard way to get from an instance of DriveInfo to
> the corresponding instance of SCSIDevice respectively
> SCSIDeviceInfo?

No.  It is linked the other way around: The device has a reference to 
the DriveInfo.  Which is one of the reasons why I think drive_del isn't 
that a great idea.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/4] SCSI-Hotdel: Implement drive_hot_del

[Wolfgang Mauerer]

Gerd Hoffmann wrote:
> On 09/30/09 17:45, Wolfgang Mauerer wrote:
>> Gerd Hoffmann wrote:
>>>     Hi,
>>>
>>>> +     dinfo = drive_get(type, bus, unit);
>>>> +     if (!dinfo) {
>>>> +         monitor_printf(mon, "Trying to remove non-existent device\n");
>>>> +	 return;
>>>> +     }
>>> No.  Just don't do this silly if/bus/unit parsing.  At very minimum use
>>> drive_get_by_id() here, then have something like 'drive_del $id'.
>>>
>>> IMHO much better would be to go qdev instead though.  We should have
>>> generic device_add + device_del monitor commands which work for any
>>> device, pretty much like the -device command line switch.
>> That makes sense, but I'd nevertheless prefer to stick with the
>> more traditional approach right now, replacing the
>> bus/unit parsing with an ID-based variant.
> 
> Have a look at the "qdev: bus management updates." patch series posted a 
> few days ago.  It adds device_add+device_del.

Thanks for the pointer. The reason why I'd (still :-)) like
to stick to the non-qdev variant is because I would want
the patch to be applicable to older qemu releases for various
reasons, and backporting all the required qdev  changes does
not seem to be a reasonable option (but please tell me if you
think this would make sense)

>> However, is there any
>> standard way to get from an instance of DriveInfo to
>> the corresponding instance of SCSIDevice respectively
>> SCSIDeviceInfo?
> 
> No.  It is linked the other way around: The device has a reference to 
> the DriveInfo.  Which is one of the reasons why I think drive_del isn't 
> that a great idea.

Cheers, Wolfgang

Re: [Qemu-devel] [PATCH 3/4] SCSI-Hotdel: Implement drive_hot_del

[Gerd Hoffmann]

On 10/01/09 09:54, Wolfgang Mauerer wrote:
> Thanks for the pointer. The reason why I'd (still :-)) like
> to stick to the non-qdev variant is because I would want
> the patch to be applicable to older qemu releases for various
> reasons, and backporting all the required qdev  changes does
> not seem to be a reasonable option (but please tell me if you
> think this would make sense)

Yes, backporting all qdev isn't going to fly.

Note that support for looking up drives by name is available in 0.11+ 
only too, so you might have to stick with the lookup by pci address, 
depending on which versions you plan to backport it ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/4] SCSI-hotdel: Add detach method to LSI SCSI driver

[Gerd Hoffmann]

> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index 62bdca8..7e975b4 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -1959,6 +1959,22 @@ static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
>       cpu_register_physical_memory(addr + 0, 0x400, s->mmio_io_addr);
>   }
>
> +void lsi_scsi_detach(DeviceState *host, BlockDriverState *bd, int id)
> +{
> +    LSIState *s = (LSIState *)host;
> +
> +    if (id>= LSI_MAX_DEVS || id<  0) {
> +        BADF("Bad Device ID %d\n", id);
> +        return;
> +    }
> +
> +    if (s->bus->devs[id]) {
> +        DPRINTF("Destroying device %d\n", id);
> +        s->bus->devs[id]->info->destroy(s->bus->devs[id]);
> +    }
> +    s->bus->devs[id] = NULL;
> +}
> +

Wrong place.  It should go to scsi-bus.c, it is the reverse of the 
scsi_qdev_init function.

cheers,
   Gerd