Patchwork [16/24] qdev hotplug: infrastructure and monitor commands.

login
register
mail settings
Submitter Gerd Hoffmann
Date Sept. 25, 2009, 7:42 p.m.
Message ID <1253907769-1067-17-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/34305/
State Superseded
Headers show

Comments

Gerd Hoffmann - Sept. 25, 2009, 7:42 p.m.
Adds device_add and device_del commands.  device_add accepts accepts
the same syntax like the -device command line switch.  device_del
expects a device id.  So you should tag your devices with ids if you
want to remove them later on, like this:

  device_add pci-ohci,id=ohci
  device_del ohci

Unplugging via pci_del or usb_del works too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c       |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h       |   12 +++++++-
 qemu-monitor.hx |   16 +++++++++++
 vl.c            |    2 +
 4 files changed, 107 insertions(+), 2 deletions(-)
Markus Armbruster - Sept. 28, 2009, 8:32 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> From: Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Qemu-devel] [PATCH 16/24] qdev hotplug: infrastructure and monitor
> 	commands.
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Date: Fri, 25 Sep 2009 21:42:41 +0200
>
> Adds device_add and device_del commands.  device_add accepts accepts
> the same syntax like the -device command line switch.  device_del
> expects a device id.  So you should tag your devices with ids if you
> want to remove them later on, like this:
>
>   device_add pci-ohci,id=ohci
>   device_del ohci
>
> Unplugging via pci_del or usb_del works too.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qdev.c       |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h       |   12 +++++++-
>  qemu-monitor.hx |   16 +++++++++++
>  vl.c            |    2 +
>  4 files changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index a25245a..00cb849 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -31,6 +31,8 @@
>  #include "monitor.h"
>  
>  /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
> +static int qdev_hotplug = 0;
> +

I see the "nasty hack" part, but I don't see how qdev_create() accepts a
null bus now:

>  static BusState *main_system_bus;
>  
>  static DeviceInfo *device_info_list;
> @@ -102,6 +104,10 @@ 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);

It still derefences bus.

> +    if (qdev_hotplug) {
> +        assert(bus->allow_hotplug);
> +        dev->hotplugged = 1;
> +    }
>      dev->state = DEV_STATE_CREATED;
>      return dev;
>  }
> @@ -192,6 +198,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>                     path ? path : info->bus_info->name, info->name);
>          return NULL;
>      }
> +    if (qdev_hotplug && !bus->allow_hotplug) {
> +        qemu_error("Bus %s does not support hotplugging\n",
> +                   bus->name);
> +        return NULL;
> +    }
>  
>      /* create device, set properties */
>      qdev = qdev_create(bus, driver);
[...]

As far as I can see, all qdev_hotplug does is telling qdev_device_add()
and qdev_create() that this is a hotplug.

What about something like:

DeviceState *qdev_device_add(QemuOpts *opts, int hotplug)
{
[...]
    if (hotplug && !bus->allow_hotplug) {
        qemu_error("Bus %s does not support hotplugging\n",
                   bus->name);
        return NULL;
    }

    /* create device, set properties */
    qdev = qdev_create(bus, driver);
    if (hotplug) {
        dev->hotplugged = 1;
[...]
}
Gerd Hoffmann - Sept. 29, 2009, 9:08 a.m.
>>   /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
>> +static int qdev_hotplug = 0;
>> +
>
> I see the "nasty hack" part, but I don't see how qdev_create() accepts a
> null bus now:
>
>>   static BusState *main_system_bus;

The (existing) "nasty hack" comment belongs to the main_system_bus 
variable and should stay there of course to avoid confusion ...

>>       /* create device, set properties */
>>       qdev = qdev_create(bus, driver);
> [...]
>
> As far as I can see, all qdev_hotplug does is telling qdev_device_add()
> and qdev_create() that this is a hotplug.

Yes.

> What about something like:
>
> DeviceState *qdev_device_add(QemuOpts *opts, int hotplug)
> {
> [...]
>      if (hotplug&&  !bus->allow_hotplug) {
>          qemu_error("Bus %s does not support hotplugging\n",
>                     bus->name);
>          return NULL;
>      }
>
>      /* create device, set properties */
>      qdev = qdev_create(bus, driver);
>      if (hotplug) {
>          dev->hotplugged = 1;

I started that way.  Doesn't fly.  Not every device creation goes 
through qdev_device_add().  Thus you'll have to do this in 
qdev_create(), which in turn means that you would have to add a hotplug 
parameter to tons of functions just to pass it down to qdev_create ...

cheers,
   Gerd
Markus Armbruster - Sept. 29, 2009, 12:25 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

[...]
>>
>> As far as I can see, all qdev_hotplug does is telling qdev_device_add()
>> and qdev_create() that this is a hotplug.
>
> Yes.
>
>> What about something like:
>>
>> DeviceState *qdev_device_add(QemuOpts *opts, int hotplug)
>> {
>> [...]
>>      if (hotplug&&  !bus->allow_hotplug) {
>>          qemu_error("Bus %s does not support hotplugging\n",
>>                     bus->name);
>>          return NULL;
>>      }
>>
>>      /* create device, set properties */
>>      qdev = qdev_create(bus, driver);
>>      if (hotplug) {
>>          dev->hotplugged = 1;
>
> I started that way.  Doesn't fly.  Not every device creation goes
> through qdev_device_add().  Thus you'll have to do this in
> qdev_create(), which in turn means that you would have to add a
> hotplug parameter to tons of functions just to pass it down to
> qdev_create ...

Isn't it sufficient if every *hotplug* device creation goes through
qdev_device_add()?
Gerd Hoffmann - Sept. 29, 2009, 1:23 p.m.
Hi,

>> I started that way.  Doesn't fly.  Not every device creation goes
>> through qdev_device_add().  Thus you'll have to do this in
>> qdev_create(), which in turn means that you would have to add a
>> hotplug parameter to tons of functions just to pass it down to
>> qdev_create ...
>
> Isn't it sufficient if every *hotplug* device creation goes through
> qdev_device_add()?

That isn't the case too.

cheers,
   Gerd

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index a25245a..00cb849 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -31,6 +31,8 @@ 
 #include "monitor.h"
 
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
+static int qdev_hotplug = 0;
+
 static BusState *main_system_bus;
 
 static DeviceInfo *device_info_list;
@@ -102,6 +104,10 @@  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);
+    if (qdev_hotplug) {
+        assert(bus->allow_hotplug);
+        dev->hotplugged = 1;
+    }
     dev->state = DEV_STATE_CREATED;
     return dev;
 }
@@ -192,6 +198,11 @@  DeviceState *qdev_device_add(QemuOpts *opts)
                    path ? path : info->bus_info->name, info->name);
         return NULL;
     }
+    if (qdev_hotplug && !bus->allow_hotplug) {
+        qemu_error("Bus %s does not support hotplugging\n",
+                   bus->name);
+        return NULL;
+    }
 
     /* create device, set properties */
     qdev = qdev_create(bus, driver);
@@ -229,6 +240,24 @@  int qdev_init(DeviceState *dev)
     return 0;
 }
 
+int qdev_unplug(DeviceState *dev)
+{
+    if (!dev->parent_bus->allow_hotplug) {
+        qemu_error("Bus %s does not support hotplugging\n",
+                   dev->parent_bus->name);
+        return -1;
+    }
+    return dev->info->unplug(dev);
+}
+
+/* can be used as ->unplug() callback for the simple cases */
+int qdev_simple_unplug_cb(DeviceState *dev)
+{
+    /* just zap it */
+    qdev_free(dev);
+    return 0;
+}
+
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
@@ -252,6 +281,15 @@  void qdev_free(DeviceState *dev)
     qemu_free(dev);
 }
 
+void qdev_machine_creation_done(void)
+{
+    /*
+     * ok, initial machine setup is done, starting from now we can
+     * only create hotpluggable devices
+     */
+    qdev_hotplug = 1;
+}
+
 /* Get a character (serial) device interface.  */
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
@@ -370,6 +408,24 @@  static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
+static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
+{
+    DeviceState *dev, *ret;
+    BusState *child;
+
+    QLIST_FOREACH(dev, &bus->children, sibling) {
+        if (dev->id && strcmp(dev->id, id) == 0)
+            return dev;
+        QLIST_FOREACH(child, &dev->child_bus, sibling) {
+            ret = qdev_find_recursive(child, id);
+            if (ret) {
+                return ret;
+            }
+        }
+    }
+    return NULL;
+}
+
 static void qbus_list_bus(DeviceState *dev, char *dest, int len)
 {
     BusState *child;
@@ -647,3 +703,26 @@  void do_info_qdm(Monitor *mon)
         monitor_printf(mon, "%s\n", msg);
     }
 }
+
+void do_device_add(Monitor *mon, const QDict *qdict)
+{
+    QemuOpts *opts;
+
+    opts = qemu_opts_parse(&qemu_device_opts,
+                           qdict_get_str(qdict, "config"), "driver");
+    if (opts)
+        qdev_device_add(opts);
+}
+
+void do_device_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    DeviceState *dev;
+
+    dev = qdev_find_recursive(main_system_bus, id);
+    if (NULL == dev) {
+        qemu_error("Device '%s' not found\n", id);
+        return;
+    }
+    qdev_unplug(dev);
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 0db2d32..8b6e4fe 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -29,6 +29,7 @@  enum DevState {
 struct DeviceState {
     const char *id;
     enum DevState state;
+    int hotplugged;
     DeviceInfo *info;
     BusState *parent_bus;
     int num_gpio_out;
@@ -53,6 +54,7 @@  struct BusState {
     DeviceState *parent;
     BusInfo *info;
     const char *name;
+    int allow_hotplug;
     int qdev_allocated;
     QLIST_HEAD(, DeviceState) children;
     QLIST_ENTRY(BusState) sibling;
@@ -97,7 +99,10 @@  struct CompatProperty {
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_device_add(QemuOpts *opts);
 int qdev_init(DeviceState *dev);
+int qdev_unplug(DeviceState *dev);
 void qdev_free(DeviceState *dev);
+int qdev_simple_unplug_cb(DeviceState *dev);
+void qdev_machine_creation_done(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
@@ -107,7 +112,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);
+typedef int (*qdev_event)(DeviceState *dev);
 
 struct DeviceInfo {
     const char *name;
@@ -125,7 +130,8 @@  struct DeviceInfo {
 
     /* Private to qdev / bus.  */
     qdev_initfn init;
-    qdev_exitfn exit;
+    qdev_event unplug;
+    qdev_event exit;
     BusInfo *bus_info;
     struct DeviceInfo *next;
 };
@@ -164,6 +170,8 @@  void qbus_free(BusState *bus);
 
 void do_info_qtree(Monitor *mon);
 void do_info_qdm(Monitor *mon);
+void do_device_add(Monitor *mon, const QDict *qdict);
+void do_device_del(Monitor *mon, const QDict *qdict);
 
 /*** qdev-properties.c ***/
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9f91873..e4afe62 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -369,6 +369,22 @@  hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
 command @code{info usb} to see the devices you can remove.
 ETEXI
 
+    { "device_add", "config:s", do_device_add,
+      "device", "add device, like -device on the command line" },
+STEXI
+@item device_add @var{config}
+
+Add device.
+ETEXI
+
+    { "device_del", "id:s", do_device_del,
+      "device", "remove device" },
+STEXI
+@item device_del @var{id}
+
+Remove device @var{id}.
+ETEXI
+
     { "cpu", "index:i", do_cpu_set,
       "index", "set the default CPU" },
 STEXI
diff --git a/vl.c b/vl.c
index eb01da7..7df9328 100644
--- a/vl.c
+++ b/vl.c
@@ -5869,6 +5869,8 @@  int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    qdev_machine_creation_done();
+
     if (loadvm) {
         if (load_vmstate(cur_mon, loadvm) < 0) {
             autostart = 0;