[Qemu-devel] [PATCH 0/4] S390 virtio hotplug v3

[Qemu-devel] [PATCH 0/4] S390 virtio hotplug v3

[Alexander Graf]

Hey guys,

this patch set adds support for hotplug add on S390. Apparently it's the first
non-x86 platform receiving so much love in Qemu, so I've stumbled over some
very basic #if defined(TARGET_I386) cases that just shouldn't be there.

It's trying to make things as generic as possible. I've talked to Markus about
this vs blockdev and he's fine with pulling in a "dirty" solution in case the
right one takes too long. But we haven't decided on which one is more clean :).

Alex

Alexander Graf (4):
  [S390] Add hotplug support
  Compile device-hotplug on all targets
  Add generic drive hotplugging
  Expose drive_add on all architectures

 Makefile.target      |    5 ++++-
 hmp-commands.hx      |    2 --
 hw/device-hotplug.c  |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci-hotplug.c     |   24 ++++--------------------
 hw/s390-virtio-bus.c |   24 +++++++++++++++++++-----
 hw/s390-virtio-bus.h |    5 +++++
 sysemu.h             |    6 +++++-
 7 files changed, 84 insertions(+), 29 deletions(-)

[Qemu-devel] [PATCH 1/4] [S390] Add hotplug support

[Alexander Graf]

I just submitted a few patches that enable the s390 virtio bus to receive
a hotplug add event. This patch implements the qemu side of it, so that new
hotplug events can be submitted to the guest.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - make s390 virtio hoplug code emulate-capable
---
 hw/s390-virtio-bus.c |   24 +++++++++++++++++++-----
 hw/s390-virtio-bus.h |    5 +++++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index e2f3e32..0adbddb 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -82,12 +82,24 @@ VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
     bus->dev_offs = bus->dev_page;
     bus->next_ring = bus->dev_page + TARGET_PAGE_SIZE;
 
+    /* Enable hotplugging */
+    _bus->allow_hotplug = 1;
+
     /* Allocate RAM for VirtIO device pages (descriptors, queues, rings) */
     *ram_size += S390_DEVICE_PAGES * TARGET_PAGE_SIZE;
 
     return bus;
 }
 
+static void s390_virtio_irq(CPUState *env, int config_change, uint64_t token)
+{
+    if (kvm_enabled()) {
+        kvm_s390_virtio_irq(env, config_change, token);
+    } else {
+        cpu_inject_ext(env, VIRTIO_EXT_CODE, config_change, token);
+    }
+}
+
 static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
 {
     VirtIOS390Bus *bus;
@@ -109,6 +121,11 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
     dev->host_features = vdev->get_features(vdev, dev->host_features);
     s390_virtio_device_sync(dev);
 
+    if (dev->qdev.hotplugged) {
+        CPUState *env = s390_cpu_addr2state(0);
+        s390_virtio_irq(env, VIRTIO_PARAM_DEV_ADD, dev->dev_offs);
+    }
+
     return 0;
 }
 
@@ -313,11 +330,7 @@ static void virtio_s390_notify(void *opaque, uint16_t vector)
     uint64_t token = s390_virtio_device_vq_token(dev, vector);
     CPUState *env = s390_cpu_addr2state(0);
 
-    if (kvm_enabled()) {
-        kvm_s390_virtio_irq(env, 0, token);
-    } else {
-        cpu_inject_ext(env, VIRTIO_EXT_CODE, 0, token);
-    }
+    s390_virtio_irq(env, 0, token);
 }
 
 static unsigned virtio_s390_get_features(void *opaque)
@@ -385,6 +398,7 @@ static void s390_virtio_bus_register_withprop(VirtIOS390DeviceInfo *info)
 {
     info->qdev.init = s390_virtio_busdev_init;
     info->qdev.bus_info = &s390_virtio_bus_info;
+    info->qdev.unplug = qdev_simple_unplug_cb;
 
     assert(info->qdev.size >= sizeof(VirtIOS390Device));
     qdev_register(&info->qdev);
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index f1bece7..d02a907 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -35,6 +35,11 @@
 #define VIRTIO_RING_LEN			(TARGET_PAGE_SIZE * 3)
 #define S390_DEVICE_PAGES		512
 
+#define VIRTIO_PARAM_MASK               0xff
+#define VIRTIO_PARAM_VRING_INTERRUPT    0x0
+#define VIRTIO_PARAM_CONFIG_CHANGED     0x1
+#define VIRTIO_PARAM_DEV_ADD            0x2
+
 typedef struct VirtIOS390Device {
     DeviceState qdev;
     ram_addr_t dev_offs;
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/4] Compile device-hotplug on all targets

[Alexander Graf]

All guest targets could potentially implement hotplugging. With the next
patches in this set I will also reflect this in the monitor interface.

So let's always compile it in. It shouldn't hurt.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Makefile.target |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index a53a2ff..84fd700 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -230,12 +230,15 @@ ifeq ($(CONFIG_KVM), y)
 endif
 obj-$(CONFIG_IVSHMEM) += ivshmem.o
 
+# Generic hotplugging
+obj-y += device-hotplug.o
+
 # Hardware support
 obj-i386-y += vga.o
 obj-i386-y += mc146818rtc.o i8259.o pc.o
 obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
 obj-i386-y += vmport.o
-obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
+obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += debugcon.o multiboot.o
 obj-i386-y += pc_piix.o
 obj-i386-$(CONFIG_KVM) += kvmclock.o
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/4] Add generic drive hotplugging

[Alexander Graf]

The monitor command for hotplugging is in i386 specific code. This is just
plain wrong, as S390 just learned how to do hotplugging too and needs to
get drives for that.

So let's add a generic copy to generic code that handles drive_add in a
way that doesn't have pci dependencies. All pci specific code can then
be handled in a pci specific function.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - align generic drive_add to pci specific one
  - rework to split between generic and pci code

v2 -> v3:

  - remove comment
---
 hw/device-hotplug.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci-hotplug.c    |   24 ++++--------------------
 sysemu.h            |    6 +++++-
 3 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 8b2ed7a..2bdc615 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -26,6 +26,9 @@
 #include "boards.h"
 #include "net.h"
 #include "blockdev.h"
+#include "qemu-config.h"
+#include "sysemu.h"
+#include "monitor.h"
 
 DriveInfo *add_init_drive(const char *optstr)
 {
@@ -44,3 +47,47 @@ DriveInfo *add_init_drive(const char *optstr)
 
     return dinfo;
 }
+
+#if !defined(TARGET_I386)
+int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
+                      DriveInfo *dinfo, int type)
+{
+    /* On non-x86 we don't do PCI hotplug */
+    monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
+    return -1;
+}
+#endif
+
+void drive_hot_add(Monitor *mon, const QDict *qdict)
+{
+    int type;
+    DriveInfo *dinfo = NULL;
+    const char *opts = qdict_get_str(qdict, "opts");
+
+    dinfo = add_init_drive(opts);
+    if (!dinfo) {
+        goto err;
+    }
+    if (dinfo->devaddr) {
+        monitor_printf(mon, "Parameter addr not supported\n");
+        goto err;
+    }
+    type = dinfo->type;
+
+    switch (type) {
+    case IF_NONE:
+        monitor_printf(mon, "OK\n");
+        break;
+    default:
+        if (pci_drive_hot_add(mon, qdict, dinfo, type)) {
+            goto err;
+        }
+    }
+    return;
+
+err:
+    if (dinfo) {
+        drive_put_ref(dinfo);
+    }
+    return;
+}
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index b59be2a..f08d367 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -103,24 +103,13 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
     return 0;
 }
 
-void drive_hot_add(Monitor *mon, const QDict *qdict)
+int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
+                      DriveInfo *dinfo, int type)
 {
     int dom, pci_bus;
     unsigned slot;
-    int type;
     PCIDevice *dev;
-    DriveInfo *dinfo = NULL;
     const char *pci_addr = qdict_get_str(qdict, "pci_addr");
-    const char *opts = qdict_get_str(qdict, "opts");
-
-    dinfo = add_init_drive(opts);
-    if (!dinfo)
-        goto err;
-    if (dinfo->devaddr) {
-        monitor_printf(mon, "Parameter addr not supported\n");
-        goto err;
-    }
-    type = dinfo->type;
 
     switch (type) {
     case IF_SCSI:
@@ -137,19 +126,14 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
             goto err;
         }
         break;
-    case IF_NONE:
-        monitor_printf(mon, "OK\n");
-        break;
     default:
         monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
         goto err;
     }
-    return;
 
+    return 0;
 err:
-    if (dinfo)
-        drive_put_ref(dinfo);
-    return;
+    return -1;
 }
 
 static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
diff --git a/sysemu.h b/sysemu.h
index d3013f5..9c8e50f 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -144,9 +144,13 @@ extern unsigned int nb_prom_envs;
 
 /* pci-hotplug */
 void pci_device_hot_add(Monitor *mon, const QDict *qdict);
-void drive_hot_add(Monitor *mon, const QDict *qdict);
+int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
+                      DriveInfo *dinfo, int type);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
 
+/* generic hotplug */
+void drive_hot_add(Monitor *mon, const QDict *qdict);
+
 /* pcie aer error injection */
 void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
 int do_pcie_aer_inejct_error(Monitor *mon,
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/4] Expose drive_add on all architectures

[Alexander Graf]

All architectures can now use drive_add on the monitor. This of course
does not mean that there is hotplug support for the specific platform,
so in order to actually make use of the new drives you still need to
have a hotplug capable device.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hmp-commands.hx |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6ad8806..e275f88 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -857,7 +857,6 @@ STEXI
 Snapshot device, using snapshot file as target if provided
 ETEXI
 
-#if defined(TARGET_I386)
     {
         .name       = "drive_add",
         .args_type  = "pci_addr:s,opts:s",
@@ -869,7 +868,6 @@ ETEXI
         .help       = "add drive to PCI storage controller",
         .mhandler.cmd = drive_hot_add,
     },
-#endif
 
 STEXI
 @item drive_add
-- 
1.6.0.2

[Qemu-devel] [PATCH 0/4] S390 virtio hotplug v2

[Alexander Graf]

Hey guys,

this patch set adds support for hotplug add on S390. Apparently it's the first
non-x86 platform receiving so much love in Qemu, so I've stumbled over some
very basic #if defined(TARGET_I386) cases that just shouldn't be there.

It's trying to make things as generic as possible. I've talked to Markus about
this vs blockdev and he's fine with pulling in a "dirty" solution in case the
right one takes too long. But we haven't decided on which one is more clean :).

Alex

Alexander Graf (4):
  [S390] Add hotplug support
  Compile device-hotplug on all targets
  Add generic drive hotplugging
  Expose drive_add on all architectures

 Makefile.target      |    5 +++-
 hmp-commands.hx      |    2 -
 hw/device-hotplug.c  |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci-hotplug.c     |   24 +++-------------------
 hw/s390-virtio-bus.c |   24 ++++++++++++++++++----
 hw/s390-virtio-bus.h |    5 ++++
 sysemu.h             |    6 ++++-
 7 files changed, 88 insertions(+), 29 deletions(-)

[Qemu-devel] [PATCH 3/4] Add generic drive hotplugging

[Alexander Graf]

The monitor command for hotplugging is in i386 specific code. This is just
plain wrong, as S390 just learned how to do hotplugging too and needs to
get drives for that.

So let's add a generic copy to generic code that handles drive_add in a
way that doesn't have pci dependencies. All pci specific code can then
be handled in a pci specific function.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - align generic drive_add to pci specific one
  - rework to split between generic and pci code
---
 hw/device-hotplug.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci-hotplug.c    |   24 ++++--------------------
 sysemu.h            |    6 +++++-
 3 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 8b2ed7a..eb6dd0e 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -26,6 +26,9 @@
 #include "boards.h"
 #include "net.h"
 #include "blockdev.h"
+#include "qemu-config.h"
+#include "sysemu.h"
+#include "monitor.h"
 
 DriveInfo *add_init_drive(const char *optstr)
 {
@@ -44,3 +47,51 @@ DriveInfo *add_init_drive(const char *optstr)
 
     return dinfo;
 }
+
+#if !defined(TARGET_I386)
+int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
+                      DriveInfo *dinfo, int type)
+{
+    /* On non-x86 we don't do PCI hotplug */
+    monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
+    return -1;
+}
+#endif
+
+/*
+ * This version of drive_hot_add does not know anything about PCI specifics.
+ * It is used as fallback on architectures that don't implement pci-hotplug.
+ */
+void drive_hot_add(Monitor *mon, const QDict *qdict)
+{
+    int type;
+    DriveInfo *dinfo = NULL;
+    const char *opts = qdict_get_str(qdict, "opts");
+
+    dinfo = add_init_drive(opts);
+    if (!dinfo) {
+        goto err;
+    }
+    if (dinfo->devaddr) {
+        monitor_printf(mon, "Parameter addr not supported\n");
+        goto err;
+    }
+    type = dinfo->type;
+
+    switch (type) {
+    case IF_NONE:
+        monitor_printf(mon, "OK\n");
+        break;
+    default:
+        if (pci_drive_hot_add(mon, qdict, dinfo, type)) {
+            goto err;
+        }
+    }
+    return;
+
+err:
+    if (dinfo) {
+        drive_put_ref(dinfo);
+    }
+    return;
+}
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index b59be2a..f08d367 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -103,24 +103,13 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
     return 0;
 }
 
-void drive_hot_add(Monitor *mon, const QDict *qdict)
+int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
+                      DriveInfo *dinfo, int type)
 {
     int dom, pci_bus;
     unsigned slot;
-    int type;
     PCIDevice *dev;
-    DriveInfo *dinfo = NULL;
     const char *pci_addr = qdict_get_str(qdict, "pci_addr");
-    const char *opts = qdict_get_str(qdict, "opts");
-
-    dinfo = add_init_drive(opts);
-    if (!dinfo)
-        goto err;
-    if (dinfo->devaddr) {
-        monitor_printf(mon, "Parameter addr not supported\n");
-        goto err;
-    }
-    type = dinfo->type;
 
     switch (type) {
     case IF_SCSI:
@@ -137,19 +126,14 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
             goto err;
         }
         break;
-    case IF_NONE:
-        monitor_printf(mon, "OK\n");
-        break;
     default:
         monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
         goto err;
     }
-    return;
 
+    return 0;
 err:
-    if (dinfo)
-        drive_put_ref(dinfo);
-    return;
+    return -1;
 }
 
 static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
diff --git a/sysemu.h b/sysemu.h
index d3013f5..9c8e50f 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -144,9 +144,13 @@ extern unsigned int nb_prom_envs;
 
 /* pci-hotplug */
 void pci_device_hot_add(Monitor *mon, const QDict *qdict);
-void drive_hot_add(Monitor *mon, const QDict *qdict);
+int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
+                      DriveInfo *dinfo, int type);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
 
+/* generic hotplug */
+void drive_hot_add(Monitor *mon, const QDict *qdict);
+
 /* pcie aer error injection */
 void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
 int do_pcie_aer_inejct_error(Monitor *mon,
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging

[Kevin Wolf]

Am 12.07.2011 09:21, schrieb Alexander Graf:
> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
> 
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies. All pci specific code can then
> be handled in a pci specific function.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v1 -> v2:
> 
>   - align generic drive_add to pci specific one
>   - rework to split between generic and pci code
> ---
>  hw/device-hotplug.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci-hotplug.c    |   24 ++++--------------------
>  sysemu.h            |    6 +++++-
>  3 files changed, 60 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index 8b2ed7a..eb6dd0e 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -26,6 +26,9 @@
>  #include "boards.h"
>  #include "net.h"
>  #include "blockdev.h"
> +#include "qemu-config.h"
> +#include "sysemu.h"
> +#include "monitor.h"
>  
>  DriveInfo *add_init_drive(const char *optstr)
>  {
> @@ -44,3 +47,51 @@ DriveInfo *add_init_drive(const char *optstr)
>  
>      return dinfo;
>  }
> +
> +#if !defined(TARGET_I386)
> +int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
> +                      DriveInfo *dinfo, int type)
> +{
> +    /* On non-x86 we don't do PCI hotplug */
> +    monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
> +    return -1;
> +}
> +#endif

This assumes that only x86 can do PCI hotplug. If someone added it to
another target, the error should be obvious enough, so I guess it's okay.

> +
> +/*
> + * This version of drive_hot_add does not know anything about PCI specifics.
> + * It is used as fallback on architectures that don't implement pci-hotplug.
> + */

Maybe it was true in v1, don't know, but now it's not a fallback, but a
common implementation that is used by both x86 and s390 and calls a more
specific one in some cases.

It also doesn't make too much sense to have a comment that says what a
function is _not_. Without knowing the context of this patch, I probably
wouldn't understand what the comment is trying to say.

> +void drive_hot_add(Monitor *mon, const QDict *qdict)
> +{
> +    int type;
> +    DriveInfo *dinfo = NULL;
> +    const char *opts = qdict_get_str(qdict, "opts");
> +
> +    dinfo = add_init_drive(opts);
> +    if (!dinfo) {
> +        goto err;
> +    }
> +    if (dinfo->devaddr) {
> +        monitor_printf(mon, "Parameter addr not supported\n");
> +        goto err;
> +    }
> +    type = dinfo->type;
> +
> +    switch (type) {
> +    case IF_NONE:
> +        monitor_printf(mon, "OK\n");
> +        break;
> +    default:
> +        if (pci_drive_hot_add(mon, qdict, dinfo, type)) {
> +            goto err;
> +        }
> +    }
> +    return;
> +
> +err:
> +    if (dinfo) {
> +        drive_put_ref(dinfo);
> +    }
> +    return;
> +}
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index b59be2a..f08d367 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -103,24 +103,13 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
>      return 0;
>  }
>  
> -void drive_hot_add(Monitor *mon, const QDict *qdict)
> +int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
> +                      DriveInfo *dinfo, int type)
>  {
>      int dom, pci_bus;
>      unsigned slot;
> -    int type;
>      PCIDevice *dev;
> -    DriveInfo *dinfo = NULL;
>      const char *pci_addr = qdict_get_str(qdict, "pci_addr");
> -    const char *opts = qdict_get_str(qdict, "opts");
> -
> -    dinfo = add_init_drive(opts);
> -    if (!dinfo)
> -        goto err;
> -    if (dinfo->devaddr) {
> -        monitor_printf(mon, "Parameter addr not supported\n");
> -        goto err;
> -    }
> -    type = dinfo->type;
>  
>      switch (type) {
>      case IF_SCSI:
> @@ -137,19 +126,14 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
>              goto err;
>          }
>          break;
> -    case IF_NONE:
> -        monitor_printf(mon, "OK\n");
> -        break;
>      default:
>          monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
>          goto err;
>      }
> -    return;
>  
> +    return 0;
>  err:
> -    if (dinfo)
> -        drive_put_ref(dinfo);
> -    return;
> +    return -1;
>  }

Now that there isn't any error handling any more, "goto err;" could be
replaced by "return -1;".

The general approach looks okay.

Kevin

Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging

[Alexander Graf]

On 14.07.2011, at 14:13, Kevin Wolf wrote:

> Am 12.07.2011 09:21, schrieb Alexander Graf:
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>> 
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies. All pci specific code can then
>> be handled in a pci specific function.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> 
>> ---
>> 
>> v1 -> v2:
>> 
>>  - align generic drive_add to pci specific one
>>  - rework to split between generic and pci code
>> ---
>> hw/device-hotplug.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/pci-hotplug.c    |   24 ++++--------------------
>> sysemu.h            |    6 +++++-
>> 3 files changed, 60 insertions(+), 21 deletions(-)
>> 
>> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
>> index 8b2ed7a..eb6dd0e 100644
>> --- a/hw/device-hotplug.c
>> +++ b/hw/device-hotplug.c
>> @@ -26,6 +26,9 @@
>> #include "boards.h"
>> #include "net.h"
>> #include "blockdev.h"
>> +#include "qemu-config.h"
>> +#include "sysemu.h"
>> +#include "monitor.h"
>> 
>> DriveInfo *add_init_drive(const char *optstr)
>> {
>> @@ -44,3 +47,51 @@ DriveInfo *add_init_drive(const char *optstr)
>> 
>>     return dinfo;
>> }
>> +
>> +#if !defined(TARGET_I386)
>> +int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
>> +                      DriveInfo *dinfo, int type)
>> +{
>> +    /* On non-x86 we don't do PCI hotplug */
>> +    monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
>> +    return -1;
>> +}
>> +#endif
> 
> This assumes that only x86 can do PCI hotplug. If someone added it to
> another target, the error should be obvious enough, so I guess it's okay.

Yes, I tried to keep it functionally the same. If any other targets want to add PCI hotplug support, it's as simple as an #ifdef change. For now, none does :). Next time someone adds PCI hotplug support for another arch, we might want to consider cleaning this whole thing up a bit though.

> 
>> +
>> +/*
>> + * This version of drive_hot_add does not know anything about PCI specifics.
>> + * It is used as fallback on architectures that don't implement pci-hotplug.
>> + */
> 
> Maybe it was true in v1, don't know, but now it's not a fallback, but a
> common implementation that is used by both x86 and s390 and calls a more
> specific one in some cases.
> 
> It also doesn't make too much sense to have a comment that says what a
> function is _not_. Without knowing the context of this patch, I probably
> wouldn't understand what the comment is trying to say.

Yep. Will just remove the comment.

> 
>> +void drive_hot_add(Monitor *mon, const QDict *qdict)
>> +{
>> +    int type;
>> +    DriveInfo *dinfo = NULL;
>> +    const char *opts = qdict_get_str(qdict, "opts");
>> +
>> +    dinfo = add_init_drive(opts);
>> +    if (!dinfo) {
>> +        goto err;
>> +    }
>> +    if (dinfo->devaddr) {
>> +        monitor_printf(mon, "Parameter addr not supported\n");
>> +        goto err;
>> +    }
>> +    type = dinfo->type;
>> +
>> +    switch (type) {
>> +    case IF_NONE:
>> +        monitor_printf(mon, "OK\n");
>> +        break;
>> +    default:
>> +        if (pci_drive_hot_add(mon, qdict, dinfo, type)) {
>> +            goto err;
>> +        }
>> +    }
>> +    return;
>> +
>> +err:
>> +    if (dinfo) {
>> +        drive_put_ref(dinfo);
>> +    }
>> +    return;
>> +}
>> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
>> index b59be2a..f08d367 100644
>> --- a/hw/pci-hotplug.c
>> +++ b/hw/pci-hotplug.c
>> @@ -103,24 +103,13 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
>>     return 0;
>> }
>> 
>> -void drive_hot_add(Monitor *mon, const QDict *qdict)
>> +int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
>> +                      DriveInfo *dinfo, int type)
>> {
>>     int dom, pci_bus;
>>     unsigned slot;
>> -    int type;
>>     PCIDevice *dev;
>> -    DriveInfo *dinfo = NULL;
>>     const char *pci_addr = qdict_get_str(qdict, "pci_addr");
>> -    const char *opts = qdict_get_str(qdict, "opts");
>> -
>> -    dinfo = add_init_drive(opts);
>> -    if (!dinfo)
>> -        goto err;
>> -    if (dinfo->devaddr) {
>> -        monitor_printf(mon, "Parameter addr not supported\n");
>> -        goto err;
>> -    }
>> -    type = dinfo->type;
>> 
>>     switch (type) {
>>     case IF_SCSI:
>> @@ -137,19 +126,14 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
>>             goto err;
>>         }
>>         break;
>> -    case IF_NONE:
>> -        monitor_printf(mon, "OK\n");
>> -        break;
>>     default:
>>         monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
>>         goto err;
>>     }
>> -    return;
>> 
>> +    return 0;
>> err:
>> -    if (dinfo)
>> -        drive_put_ref(dinfo);
>> -    return;
>> +    return -1;
>> }
> 
> Now that there isn't any error handling any more, "goto err;" could be
> replaced by "return -1;".

I actually changed it at first and then changed it back to the goto err :). I find this more readable tbh as "err" somehow jumps into the eye more easily than "return -1". And with such a huge amount of error cases, I really wanted to stress them :).


Alex