Patchwork [06/40] qdev-ify: xen backends

login
register
mail settings
Submitter Alexander Graf
Date Nov. 1, 2010, 3:01 p.m.
Message ID <1288623713-28062-7-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/69786/
State New
Headers show

Comments

Alexander Graf - Nov. 1, 2010, 3:01 p.m.
From: Gerd Hoffmann <kraxel@redhat.com>

This patch converts the xen backend code to qdev.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/xen_backend.c    |  176 ++++++++++++++++++++++++++++++++-------------------
 hw/xen_backend.h    |    9 ++-
 hw/xen_console.c    |   10 +++-
 hw/xen_disk.c       |   10 +++-
 hw/xen_machine_pv.c |    6 +--
 hw/xen_nic.c        |   10 +++-
 hw/xenfb.c          |   14 ++++-
 7 files changed, 158 insertions(+), 77 deletions(-)
Markus Armbruster - Nov. 2, 2010, 10:08 a.m.
Alexander Graf <agraf@suse.de> writes:

> From: Gerd Hoffmann <kraxel@redhat.com>
>
> This patch converts the xen backend code to qdev.

qdev conversions are always welcome.  This one's not complete (search
for #if 0).  The commit message should state that.

Two questions inline.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/xen_backend.c    |  176 ++++++++++++++++++++++++++++++++-------------------
>  hw/xen_backend.h    |    9 ++-
>  hw/xen_console.c    |   10 +++-
>  hw/xen_disk.c       |   10 +++-
>  hw/xen_machine_pv.c |    6 +--
>  hw/xen_nic.c        |   10 +++-
>  hw/xenfb.c          |   14 ++++-
>  7 files changed, 158 insertions(+), 77 deletions(-)
>
> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
> index a2e408f..0d6a96b 100644
> --- a/hw/xen_backend.c
> +++ b/hw/xen_backend.c
> @@ -42,13 +42,21 @@
>  
>  /* ------------------------------------------------------------- */
>  
> +typedef struct XenBus {
> +    BusState qbus;
> +} XenBus;
> +
>  /* public */
>  int xen_xc;
>  struct xs_handle *xenstore = NULL;
>  const char *xen_protocol;
>  
>  /* private */
> -static QTAILQ_HEAD(XenDeviceHead, XenDevice) xendevs = QTAILQ_HEAD_INITIALIZER(xendevs);
> +static struct BusInfo xen_bus_info = {
> +    .name       = "Xen",
> +    .size       = sizeof(XenBus),
> +};
> +static XenBus *xenbus;
>  static int debug = 0;
>  
>  /* ------------------------------------------------------------- */
> @@ -163,14 +171,16 @@ int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
>  
>  struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev)
>  {
> +    struct DeviceState *qdev;
>      struct XenDevice *xendev;
>  
> -    QTAILQ_FOREACH(xendev, &xendevs, next) {
> +    QLIST_FOREACH(qdev, &xenbus->qbus.children, sibling) {
> +        xendev = container_of(qdev, struct XenDevice, qdev);
>  	if (xendev->dom != dom)
>  	    continue;
>  	if (xendev->dev != dev)
>  	    continue;
> -	if (strcmp(xendev->type, type) != 0)
> +	if (strcmp(xendev->ops->type, type) != 0)
>  	    continue;
>  	return xendev;
>      }
> @@ -180,28 +190,34 @@ struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev)
>  /*
>   * get xen backend device, allocate a new one if it doesn't exist.
>   */
> -static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
> +static struct XenDevice *xen_be_get_xendev(int dom, int dev,
>                                             struct XenDevOps *ops)
>  {
> +    struct DeviceState *qdev;
>      struct XenDevice *xendev;
> +    char name[64];
>      char *dom0;
>  
> -    xendev = xen_be_find_xendev(type, dom, dev);
> +    xendev = xen_be_find_xendev(ops->type, dom, dev);
>      if (xendev)
>  	return xendev;
>  
> +    /* create new xendev */
> +    snprintf(name, sizeof(name), "xen-%s", ops->type);
> +    qdev = qdev_create(&xenbus->qbus, name);
> +    qdev_init_nofail(qdev);
> +    xendev = container_of(qdev, struct XenDevice, qdev);
> +
>      /* init new xendev */
> -    xendev = qemu_mallocz(ops->size);
> -    xendev->type  = type;
>      xendev->dom   = dom;
>      xendev->dev   = dev;
>      xendev->ops   = ops;
>  
>      dom0 = xs_get_domain_path(xenstore, 0);
>      snprintf(xendev->be, sizeof(xendev->be), "%s/backend/%s/%d/%d",
> -	     dom0, xendev->type, xendev->dom, xendev->dev);
> +	     dom0, xendev->ops->type, xendev->dom, xendev->dev);
>      snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
> -	     xendev->type, xendev->dev);
> +	     xendev->ops->type, xendev->dev);
>      free(dom0);
>  
>      xendev->debug      = debug;
> @@ -210,7 +226,7 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>      xendev->evtchndev = xc_evtchn_open();
>      if (xendev->evtchndev < 0) {
>  	xen_be_printf(NULL, 0, "can't open evtchn device\n");
> -	qemu_free(xendev);
> +	qdev_free(&xendev->qdev);
>  	return NULL;
>      }
>      fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> @@ -220,15 +236,13 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>  	if (xendev->gnttabdev < 0) {
>  	    xen_be_printf(NULL, 0, "can't open gnttab device\n");
>  	    xc_evtchn_close(xendev->evtchndev);
> -	    qemu_free(xendev);
> +	    qdev_free(&xendev->qdev);
>  	    return NULL;
>  	}
>      } else {
>  	xendev->gnttabdev = -1;
>      }
>  
> -    QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
> -
>      if (xendev->ops->alloc)
>  	xendev->ops->alloc(xendev);
>  
> @@ -238,43 +252,44 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>  /*
>   * release xen backend device.
>   */
> -static struct XenDevice *xen_be_del_xendev(int dom, int dev)
> +static void xen_be_del_xendev(int dom, int dev, struct XenDevOps *ops)
>  {
> -    struct XenDevice *xendev, *xnext;
> -
> -    /*
> -     * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
> -     * we save the next pointer in xnext because we might free xendev.
> -     */
> -    xnext = xendevs.tqh_first;
> -    while (xnext) {
> -        xendev = xnext;
> -        xnext = xendev->next.tqe_next;
> -
> -	if (xendev->dom != dom)
> -	    continue;
> -	if (xendev->dev != dev && dev != -1)
> -	    continue;
> -
> -	if (xendev->ops->free)
> -	    xendev->ops->free(xendev);
> -
> -	if (xendev->fe) {
> -	    char token[XEN_BUFSIZE];
> -	    snprintf(token, sizeof(token), "fe:%p", xendev);
> -	    xs_unwatch(xenstore, xendev->fe, token);
> -	    qemu_free(xendev->fe);
> -	}
> -
> -	if (xendev->evtchndev >= 0)
> -	    xc_evtchn_close(xendev->evtchndev);
> -	if (xendev->gnttabdev >= 0)
> -	    xc_gnttab_close(xendev->gnttabdev);
> -
> -	QTAILQ_REMOVE(&xendevs, xendev, next);
> -	qemu_free(xendev);
> -    }
> -    return NULL;
> +    struct DeviceState *qdev;
> +    struct XenDevice *xendev;
> +    int done;
> +
> +    do {
> +        done = 1;
> +        QLIST_FOREACH(qdev, &xenbus->qbus.children, sibling) {
> +            xendev = container_of(qdev, struct XenDevice, qdev);
> +            if (xendev->dom != dom)
> +                continue;
> +            if (xendev->dev != dev && dev != -1)
> +                continue;
> +            if (xendev->ops != ops)
> +                continue;
> +
> +            if (xendev->ops->free)
> +                xendev->ops->free(xendev);
> +
> +            if (xendev->fe) {
> +                char token[XEN_BUFSIZE];
> +                snprintf(token, sizeof(token), "fe:%p", xendev);
> +                xs_unwatch(xenstore, xendev->fe, token);
> +                qemu_free(xendev->fe);
> +            }
> +
> +            if (xendev->evtchndev >= 0)
> +                xc_evtchn_close(xendev->evtchndev);
> +            if (xendev->gnttabdev >= 0)
> +                xc_gnttab_close(xendev->gnttabdev);
> +
> +            qdev_free(&xendev->qdev);
> +
> +            done = 0;
> +            break;
> +        }
> +    } while (!done);

This loop nest confuses me.  Why can't we just QLIST_FOREACH_SAFE()?

>  }
>  
>  /*
> @@ -498,7 +513,7 @@ void xen_be_check_state(struct XenDevice *xendev)
>  
>  /* ------------------------------------------------------------- */
>  
> -static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
> +static int xenstore_scan(int dom, struct XenDevOps *ops)
>  {
>      struct XenDevice *xendev;
>      char path[XEN_BUFSIZE], token[XEN_BUFSIZE];
> @@ -507,8 +522,8 @@ static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
>  
>      /* setup watch */
>      dom0 = xs_get_domain_path(xenstore, 0);
> -    snprintf(token, sizeof(token), "be:%p:%d:%p", type, dom, ops);
> -    snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
> +    snprintf(token, sizeof(token), "be:%d:%p", dom, ops);

Why drop %p, type from token?

> +    snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, ops->type, dom);
>      free(dom0);
>      if (!xs_watch(xenstore, path, token)) {
>  	xen_be_printf(NULL, 0, "xen be: watching backend path (%s) failed\n", path);
> @@ -520,7 +535,7 @@ static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
>      if (!dev)
>  	return 0;
>      for (j = 0; j < cdev; j++) {
> -	xendev = xen_be_get_xendev(type, dom, atoi(dev[j]), ops);
> +	xendev = xen_be_get_xendev(dom, atoi(dev[j]), ops);
>  	if (xendev == NULL)
>  	    continue;
>  	xen_be_check_state(xendev);
> @@ -529,15 +544,14 @@ static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
>      return 0;
>  }
>  
> -static void xenstore_update_be(char *watch, char *type, int dom,
> -			       struct XenDevOps *ops)
> +static void xenstore_update_be(char *watch, int dom, struct XenDevOps *ops)
>  {
>      struct XenDevice *xendev;
>      char path[XEN_BUFSIZE], *dom0;
>      unsigned int len, dev;
>  
>      dom0 = xs_get_domain_path(xenstore, 0);
> -    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
> +    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, ops->type, dom);
>      free(dom0);
>      if (strncmp(path, watch, len) != 0)
>  	return;
> @@ -551,10 +565,10 @@ static void xenstore_update_be(char *watch, char *type, int dom,
>  
>      if (0) {
>  	/* FIXME: detect devices being deleted from xenstore ... */
> -	xen_be_del_xendev(dom, dev);
> +	xen_be_del_xendev(dom, dev, ops);
>      }
>  
> -    xendev = xen_be_get_xendev(type, dom, dev, ops);
> +    xendev = xen_be_get_xendev(dom, dev, ops);
>      if (xendev != NULL) {
>  	xen_be_backend_changed(xendev, path);
>  	xen_be_check_state(xendev);
> @@ -580,16 +594,16 @@ static void xenstore_update_fe(char *watch, struct XenDevice *xendev)
>  static void xenstore_update(void *unused)
>  {
>      char **vec = NULL;
> -    intptr_t type, ops, ptr;
> +    intptr_t ops, ptr;
>      unsigned int dom, count;
>  
>      vec = xs_read_watch(xenstore, &count);
>      if (vec == NULL)
>  	goto cleanup;
>  
> -    if (sscanf(vec[XS_WATCH_TOKEN], "be:%" PRIxPTR ":%d:%" PRIxPTR,
> -               &type, &dom, &ops) == 3)
> -	xenstore_update_be(vec[XS_WATCH_PATH], (void*)type, dom, (void*)ops);
> +    if (sscanf(vec[XS_WATCH_TOKEN], "be:%d:%" PRIxPTR,
> +               &dom, &ops) == 2)
> +	xenstore_update_be(vec[XS_WATCH_PATH], dom, (void*)ops);
>      if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1)
>  	xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr);
>  
> @@ -642,9 +656,43 @@ err:
>      return -1;
>  }
>  
> -int xen_be_register(const char *type, struct XenDevOps *ops)
> +void xen_create_bus(DeviceState *parent)
>  {
> -    return xenstore_scan(type, xen_domid, ops);
> +    DeviceInfo *info;
> +    BusState *qbus;
> +
> +    qbus = qbus_create(&xen_bus_info, parent, NULL);
> +    xenbus = DO_UPCAST(XenBus, qbus, qbus);
> +    for (info = device_info_list; info != NULL; info = info->next) {
> +        if (info->bus_info != &xen_bus_info)
> +            continue;
> +        xenstore_scan(xen_domid, DO_UPCAST(struct XenDevOps, qinfo, info));
> +    }
> +
> +    qbus->allow_hotplug = 1;
> +}
> +
> +static int xen_be_initfn(DeviceState *dev, DeviceInfo *info)
> +{
> +#if 0
> +    struct XenDevOps *ops = container_of(info, struct XenDevOps, qinfo);
> +    struct XenDevice *xendev = container_of(dev, struct XenDevice, qdev);
> +
> +    /* nothing to do as create + init isn't really splitted. */
> +#endif
> +    return 0;
> +}
> +
> +void xen_qdev_register(struct XenDevOps *ops)
> +{
> +    char name[64];
> +
> +    snprintf(name, sizeof(name), "xen-%s", ops->type);
> +    ops->qinfo.name = qemu_strdup(name);
> +    ops->qinfo.init = xen_be_initfn;
> +    ops->qinfo.bus_info = &xen_bus_info;
> +    ops->qinfo.no_user = 1,
> +    qdev_register(&ops->qinfo);
>  }
>  
>  int xen_be_bind_evtchn(struct XenDevice *xendev)
> diff --git a/hw/xen_backend.h b/hw/xen_backend.h
> index 1b428e3..f53a742 100644
> --- a/hw/xen_backend.h
> +++ b/hw/xen_backend.h
> @@ -4,6 +4,7 @@
>  #include "xen_common.h"
>  #include "sysemu.h"
>  #include "net.h"
> +#include "qdev.h"
>  
>  /* ------------------------------------------------------------- */
>  
> @@ -17,7 +18,8 @@ struct XenDevice;
>  #define DEVOPS_FLAG_IGNORE_STATE  2
>  
>  struct XenDevOps {
> -    size_t    size;
> +    DeviceInfo qinfo;
> +    char      type[64];
>      uint32_t  flags;
>      void      (*alloc)(struct XenDevice *xendev);
>      int       (*init)(struct XenDevice *xendev);
> @@ -30,7 +32,7 @@ struct XenDevOps {
>  };
>  
>  struct XenDevice {
> -    const char         *type;
> +    DeviceState        qdev;
>      int                dom;
>      int                dev;
>      char               name[64];
> @@ -78,7 +80,8 @@ void xen_be_check_state(struct XenDevice *xendev);
>  
>  /* xen backend driver bits */
>  int xen_be_init(void);
> -int xen_be_register(const char *type, struct XenDevOps *ops);
> +void xen_create_bus(DeviceState *parent);
> +void xen_qdev_register(struct XenDevOps *ops);
>  int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state);
>  int xen_be_bind_evtchn(struct XenDevice *xendev);
>  void xen_be_unbind_evtchn(struct XenDevice *xendev);
> diff --git a/hw/xen_console.c b/hw/xen_console.c
> index d2261f4..a980dc8 100644
> --- a/hw/xen_console.c
> +++ b/hw/xen_console.c
> @@ -260,10 +260,18 @@ static void con_event(struct XenDevice *xendev)
>  /* -------------------------------------------------------------------- */
>  
>  struct XenDevOps xen_console_ops = {
> -    .size       = sizeof(struct XenConsole),
> +    .qinfo.size = sizeof(struct XenConsole),
> +    .type       = "console",
>      .flags      = DEVOPS_FLAG_IGNORE_STATE,
>      .init       = con_init,
>      .connect    = con_connect,
>      .event      = con_event,
>      .disconnect = con_disconnect,
>  };
> +
> +static void xen_console_register_devices(void)
> +{
> +    xen_qdev_register(&xen_console_ops);
> +}
> +
> +device_init(xen_console_register_devices)
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 06752de..5392f58 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -774,7 +774,8 @@ static void blk_event(struct XenDevice *xendev)
>  }
>  
>  struct XenDevOps xen_blkdev_ops = {
> -    .size       = sizeof(struct XenBlkDev),
> +    .qinfo.size = sizeof(struct XenBlkDev),
> +    .type       = "qdisk",
>      .flags      = DEVOPS_FLAG_NEED_GNTDEV,
>      .alloc      = blk_alloc,
>      .init       = blk_init,
> @@ -783,3 +784,10 @@ struct XenDevOps xen_blkdev_ops = {
>      .event      = blk_event,
>      .free       = blk_free,
>  };
> +
> +static void xen_blkdev_register_devices(void)
> +{
> +    xen_qdev_register(&xen_blkdev_ops);
> +}
> +
> +device_init(xen_blkdev_register_devices)
> diff --git a/hw/xen_machine_pv.c b/hw/xen_machine_pv.c
> index 77a34bf..b94d6e9 100644
> --- a/hw/xen_machine_pv.c
> +++ b/hw/xen_machine_pv.c
> @@ -75,11 +75,7 @@ static void xen_init_pv(ram_addr_t ram_size,
>          break;
>      }
>  
> -    xen_be_register("console", &xen_console_ops);
> -    xen_be_register("vkbd", &xen_kbdmouse_ops);
> -    xen_be_register("vfb", &xen_framebuffer_ops);
> -    xen_be_register("qdisk", &xen_blkdev_ops);
> -    xen_be_register("qnic", &xen_netdev_ops);
> +    xen_create_bus(NULL);
>  
>      /* configure framebuffer */
>      if (xenfb_enabled) {
> diff --git a/hw/xen_nic.c b/hw/xen_nic.c
> index 08055b8..02d3c4e 100644
> --- a/hw/xen_nic.c
> +++ b/hw/xen_nic.c
> @@ -411,7 +411,8 @@ static int net_free(struct XenDevice *xendev)
>  /* ------------------------------------------------------------- */
>  
>  struct XenDevOps xen_netdev_ops = {
> -    .size       = sizeof(struct XenNetDev),
> +    .qinfo.size = sizeof(struct XenNetDev),
> +    .type       = "qnic",
>      .flags      = DEVOPS_FLAG_NEED_GNTDEV,
>      .init       = net_init,
>      .connect    = net_connect,
> @@ -419,3 +420,10 @@ struct XenDevOps xen_netdev_ops = {
>      .disconnect = net_disconnect,
>      .free       = net_free,
>  };
> +
> +static void xen_netdev_register_devices(void)
> +{
> +    xen_qdev_register(&xen_netdev_ops);
> +}
> +
> +device_init(xen_netdev_register_devices)
> diff --git a/hw/xenfb.c b/hw/xenfb.c
> index da5297b..293210f 100644
> --- a/hw/xenfb.c
> +++ b/hw/xenfb.c
> @@ -953,7 +953,8 @@ static void fb_event(struct XenDevice *xendev)
>  /* -------------------------------------------------------------------- */
>  
>  struct XenDevOps xen_kbdmouse_ops = {
> -    .size       = sizeof(struct XenInput),
> +    .qinfo.size = sizeof(struct XenInput),
> +    .type       = "vkbd",
>      .init       = input_init,
>      .connect    = input_connect,
>      .disconnect = input_disconnect,
> @@ -961,7 +962,8 @@ struct XenDevOps xen_kbdmouse_ops = {
>  };
>  
>  struct XenDevOps xen_framebuffer_ops = {
> -    .size       = sizeof(struct XenFB),
> +    .qinfo.size = sizeof(struct XenFB),
> +    .type       = "vfb",
>      .init       = fb_init,
>      .connect    = fb_connect,
>      .disconnect = fb_disconnect,
> @@ -1011,3 +1013,11 @@ wait_more:
>      xen_be_check_state(xin);
>      xen_be_check_state(xfb);
>  }
> +
> +static void xenfb_register_devices(void)
> +{
> +    xen_qdev_register(&xen_kbdmouse_ops);
> +    xen_qdev_register(&xen_framebuffer_ops);
> +}
> +
> +device_init(xenfb_register_devices)
Gerd Hoffmann - Nov. 2, 2010, 10:43 a.m.
On 11/02/10 11:08, Markus Armbruster wrote:
> Alexander Graf<agraf@suse.de>  writes:
>
>> From: Gerd Hoffmann<kraxel@redhat.com>
>>
>> This patch converts the xen backend code to qdev.
>
> qdev conversions are always welcome.  This one's not complete (search
> for #if 0).

It is a tricky one too.

Creating the xen backend device instances is controlled via xenstore 
(either emulated in case of xenner or xenstored when running on Xen). 
When creating block/net backends via qemu command line switches all qemu 
does is creating the xenstore entries.  Having a external entity (i.e. 
xend) creating the xenstore entries works too.

This workflow is a bit hard to fit into the qdev model ...

>> +    do {
>> +        done = 1;
>> +        QLIST_FOREACH(qdev,&xenbus->qbus.children, sibling) {
>> +            xendev = container_of(qdev, struct XenDevice, qdev);

[ ... ]

>> +            done = 0;
>> +            break;
>> +        }
>> +    } while (!done);
>
> This loop nest confuses me.  Why can't we just QLIST_FOREACH_SAFE()?

Just historical reasons I guess.  QLIST_FOREACH_SAFE() wasn't there from 
the start but got added later.

cheers,
   Gerd
Markus Armbruster - Nov. 2, 2010, 1:26 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> On 11/02/10 11:08, Markus Armbruster wrote:
>> Alexander Graf<agraf@suse.de>  writes:
>>
>>> From: Gerd Hoffmann<kraxel@redhat.com>
>>>
>>> This patch converts the xen backend code to qdev.
>>
>> qdev conversions are always welcome.  This one's not complete (search
>> for #if 0).
>
> It is a tricky one too.
>
> Creating the xen backend device instances is controlled via xenstore
> (either emulated in case of xenner or xenstored when running on
> Xen). When creating block/net backends via qemu command line switches
> all qemu does is creating the xenstore entries.  Having a external
> entity (i.e. xend) creating the xenstore entries works too.
>
> This workflow is a bit hard to fit into the qdev model ...

I'm fine with imperfect qdev conversions, as long as the issues make
things no worse then they were before (check, I think), they're clearly
documented in the source (check, although the comment could be
improved), and in the commit message (fail, but easy enough to fix).

>>> +    do {
>>> +        done = 1;
>>> +        QLIST_FOREACH(qdev,&xenbus->qbus.children, sibling) {
>>> +            xendev = container_of(qdev, struct XenDevice, qdev);
>
> [ ... ]
>
>>> +            done = 0;
>>> +            break;
>>> +        }
>>> +    } while (!done);
>>
>> This loop nest confuses me.  Why can't we just QLIST_FOREACH_SAFE()?
>
> Just historical reasons I guess.  QLIST_FOREACH_SAFE() wasn't there
> from the start but got added later.

I'd like that to be cleaned up then.

Patch

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index a2e408f..0d6a96b 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -42,13 +42,21 @@ 
 
 /* ------------------------------------------------------------- */
 
+typedef struct XenBus {
+    BusState qbus;
+} XenBus;
+
 /* public */
 int xen_xc;
 struct xs_handle *xenstore = NULL;
 const char *xen_protocol;
 
 /* private */
-static QTAILQ_HEAD(XenDeviceHead, XenDevice) xendevs = QTAILQ_HEAD_INITIALIZER(xendevs);
+static struct BusInfo xen_bus_info = {
+    .name       = "Xen",
+    .size       = sizeof(XenBus),
+};
+static XenBus *xenbus;
 static int debug = 0;
 
 /* ------------------------------------------------------------- */
@@ -163,14 +171,16 @@  int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
 
 struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev)
 {
+    struct DeviceState *qdev;
     struct XenDevice *xendev;
 
-    QTAILQ_FOREACH(xendev, &xendevs, next) {
+    QLIST_FOREACH(qdev, &xenbus->qbus.children, sibling) {
+        xendev = container_of(qdev, struct XenDevice, qdev);
 	if (xendev->dom != dom)
 	    continue;
 	if (xendev->dev != dev)
 	    continue;
-	if (strcmp(xendev->type, type) != 0)
+	if (strcmp(xendev->ops->type, type) != 0)
 	    continue;
 	return xendev;
     }
@@ -180,28 +190,34 @@  struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev)
 /*
  * get xen backend device, allocate a new one if it doesn't exist.
  */
-static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
+static struct XenDevice *xen_be_get_xendev(int dom, int dev,
                                            struct XenDevOps *ops)
 {
+    struct DeviceState *qdev;
     struct XenDevice *xendev;
+    char name[64];
     char *dom0;
 
-    xendev = xen_be_find_xendev(type, dom, dev);
+    xendev = xen_be_find_xendev(ops->type, dom, dev);
     if (xendev)
 	return xendev;
 
+    /* create new xendev */
+    snprintf(name, sizeof(name), "xen-%s", ops->type);
+    qdev = qdev_create(&xenbus->qbus, name);
+    qdev_init_nofail(qdev);
+    xendev = container_of(qdev, struct XenDevice, qdev);
+
     /* init new xendev */
-    xendev = qemu_mallocz(ops->size);
-    xendev->type  = type;
     xendev->dom   = dom;
     xendev->dev   = dev;
     xendev->ops   = ops;
 
     dom0 = xs_get_domain_path(xenstore, 0);
     snprintf(xendev->be, sizeof(xendev->be), "%s/backend/%s/%d/%d",
-	     dom0, xendev->type, xendev->dom, xendev->dev);
+	     dom0, xendev->ops->type, xendev->dom, xendev->dev);
     snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
-	     xendev->type, xendev->dev);
+	     xendev->ops->type, xendev->dev);
     free(dom0);
 
     xendev->debug      = debug;
@@ -210,7 +226,7 @@  static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
     xendev->evtchndev = xc_evtchn_open();
     if (xendev->evtchndev < 0) {
 	xen_be_printf(NULL, 0, "can't open evtchn device\n");
-	qemu_free(xendev);
+	qdev_free(&xendev->qdev);
 	return NULL;
     }
     fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
@@ -220,15 +236,13 @@  static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
 	if (xendev->gnttabdev < 0) {
 	    xen_be_printf(NULL, 0, "can't open gnttab device\n");
 	    xc_evtchn_close(xendev->evtchndev);
-	    qemu_free(xendev);
+	    qdev_free(&xendev->qdev);
 	    return NULL;
 	}
     } else {
 	xendev->gnttabdev = -1;
     }
 
-    QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
-
     if (xendev->ops->alloc)
 	xendev->ops->alloc(xendev);
 
@@ -238,43 +252,44 @@  static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
 /*
  * release xen backend device.
  */
-static struct XenDevice *xen_be_del_xendev(int dom, int dev)
+static void xen_be_del_xendev(int dom, int dev, struct XenDevOps *ops)
 {
-    struct XenDevice *xendev, *xnext;
-
-    /*
-     * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
-     * we save the next pointer in xnext because we might free xendev.
-     */
-    xnext = xendevs.tqh_first;
-    while (xnext) {
-        xendev = xnext;
-        xnext = xendev->next.tqe_next;
-
-	if (xendev->dom != dom)
-	    continue;
-	if (xendev->dev != dev && dev != -1)
-	    continue;
-
-	if (xendev->ops->free)
-	    xendev->ops->free(xendev);
-
-	if (xendev->fe) {
-	    char token[XEN_BUFSIZE];
-	    snprintf(token, sizeof(token), "fe:%p", xendev);
-	    xs_unwatch(xenstore, xendev->fe, token);
-	    qemu_free(xendev->fe);
-	}
-
-	if (xendev->evtchndev >= 0)
-	    xc_evtchn_close(xendev->evtchndev);
-	if (xendev->gnttabdev >= 0)
-	    xc_gnttab_close(xendev->gnttabdev);
-
-	QTAILQ_REMOVE(&xendevs, xendev, next);
-	qemu_free(xendev);
-    }
-    return NULL;
+    struct DeviceState *qdev;
+    struct XenDevice *xendev;
+    int done;
+
+    do {
+        done = 1;
+        QLIST_FOREACH(qdev, &xenbus->qbus.children, sibling) {
+            xendev = container_of(qdev, struct XenDevice, qdev);
+            if (xendev->dom != dom)
+                continue;
+            if (xendev->dev != dev && dev != -1)
+                continue;
+            if (xendev->ops != ops)
+                continue;
+
+            if (xendev->ops->free)
+                xendev->ops->free(xendev);
+
+            if (xendev->fe) {
+                char token[XEN_BUFSIZE];
+                snprintf(token, sizeof(token), "fe:%p", xendev);
+                xs_unwatch(xenstore, xendev->fe, token);
+                qemu_free(xendev->fe);
+            }
+
+            if (xendev->evtchndev >= 0)
+                xc_evtchn_close(xendev->evtchndev);
+            if (xendev->gnttabdev >= 0)
+                xc_gnttab_close(xendev->gnttabdev);
+
+            qdev_free(&xendev->qdev);
+
+            done = 0;
+            break;
+        }
+    } while (!done);
 }
 
 /*
@@ -498,7 +513,7 @@  void xen_be_check_state(struct XenDevice *xendev)
 
 /* ------------------------------------------------------------- */
 
-static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
+static int xenstore_scan(int dom, struct XenDevOps *ops)
 {
     struct XenDevice *xendev;
     char path[XEN_BUFSIZE], token[XEN_BUFSIZE];
@@ -507,8 +522,8 @@  static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
 
     /* setup watch */
     dom0 = xs_get_domain_path(xenstore, 0);
-    snprintf(token, sizeof(token), "be:%p:%d:%p", type, dom, ops);
-    snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
+    snprintf(token, sizeof(token), "be:%d:%p", dom, ops);
+    snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, ops->type, dom);
     free(dom0);
     if (!xs_watch(xenstore, path, token)) {
 	xen_be_printf(NULL, 0, "xen be: watching backend path (%s) failed\n", path);
@@ -520,7 +535,7 @@  static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
     if (!dev)
 	return 0;
     for (j = 0; j < cdev; j++) {
-	xendev = xen_be_get_xendev(type, dom, atoi(dev[j]), ops);
+	xendev = xen_be_get_xendev(dom, atoi(dev[j]), ops);
 	if (xendev == NULL)
 	    continue;
 	xen_be_check_state(xendev);
@@ -529,15 +544,14 @@  static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
     return 0;
 }
 
-static void xenstore_update_be(char *watch, char *type, int dom,
-			       struct XenDevOps *ops)
+static void xenstore_update_be(char *watch, int dom, struct XenDevOps *ops)
 {
     struct XenDevice *xendev;
     char path[XEN_BUFSIZE], *dom0;
     unsigned int len, dev;
 
     dom0 = xs_get_domain_path(xenstore, 0);
-    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
+    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, ops->type, dom);
     free(dom0);
     if (strncmp(path, watch, len) != 0)
 	return;
@@ -551,10 +565,10 @@  static void xenstore_update_be(char *watch, char *type, int dom,
 
     if (0) {
 	/* FIXME: detect devices being deleted from xenstore ... */
-	xen_be_del_xendev(dom, dev);
+	xen_be_del_xendev(dom, dev, ops);
     }
 
-    xendev = xen_be_get_xendev(type, dom, dev, ops);
+    xendev = xen_be_get_xendev(dom, dev, ops);
     if (xendev != NULL) {
 	xen_be_backend_changed(xendev, path);
 	xen_be_check_state(xendev);
@@ -580,16 +594,16 @@  static void xenstore_update_fe(char *watch, struct XenDevice *xendev)
 static void xenstore_update(void *unused)
 {
     char **vec = NULL;
-    intptr_t type, ops, ptr;
+    intptr_t ops, ptr;
     unsigned int dom, count;
 
     vec = xs_read_watch(xenstore, &count);
     if (vec == NULL)
 	goto cleanup;
 
-    if (sscanf(vec[XS_WATCH_TOKEN], "be:%" PRIxPTR ":%d:%" PRIxPTR,
-               &type, &dom, &ops) == 3)
-	xenstore_update_be(vec[XS_WATCH_PATH], (void*)type, dom, (void*)ops);
+    if (sscanf(vec[XS_WATCH_TOKEN], "be:%d:%" PRIxPTR,
+               &dom, &ops) == 2)
+	xenstore_update_be(vec[XS_WATCH_PATH], dom, (void*)ops);
     if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1)
 	xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr);
 
@@ -642,9 +656,43 @@  err:
     return -1;
 }
 
-int xen_be_register(const char *type, struct XenDevOps *ops)
+void xen_create_bus(DeviceState *parent)
 {
-    return xenstore_scan(type, xen_domid, ops);
+    DeviceInfo *info;
+    BusState *qbus;
+
+    qbus = qbus_create(&xen_bus_info, parent, NULL);
+    xenbus = DO_UPCAST(XenBus, qbus, qbus);
+    for (info = device_info_list; info != NULL; info = info->next) {
+        if (info->bus_info != &xen_bus_info)
+            continue;
+        xenstore_scan(xen_domid, DO_UPCAST(struct XenDevOps, qinfo, info));
+    }
+
+    qbus->allow_hotplug = 1;
+}
+
+static int xen_be_initfn(DeviceState *dev, DeviceInfo *info)
+{
+#if 0
+    struct XenDevOps *ops = container_of(info, struct XenDevOps, qinfo);
+    struct XenDevice *xendev = container_of(dev, struct XenDevice, qdev);
+
+    /* nothing to do as create + init isn't really splitted. */
+#endif
+    return 0;
+}
+
+void xen_qdev_register(struct XenDevOps *ops)
+{
+    char name[64];
+
+    snprintf(name, sizeof(name), "xen-%s", ops->type);
+    ops->qinfo.name = qemu_strdup(name);
+    ops->qinfo.init = xen_be_initfn;
+    ops->qinfo.bus_info = &xen_bus_info;
+    ops->qinfo.no_user = 1,
+    qdev_register(&ops->qinfo);
 }
 
 int xen_be_bind_evtchn(struct XenDevice *xendev)
diff --git a/hw/xen_backend.h b/hw/xen_backend.h
index 1b428e3..f53a742 100644
--- a/hw/xen_backend.h
+++ b/hw/xen_backend.h
@@ -4,6 +4,7 @@ 
 #include "xen_common.h"
 #include "sysemu.h"
 #include "net.h"
+#include "qdev.h"
 
 /* ------------------------------------------------------------- */
 
@@ -17,7 +18,8 @@  struct XenDevice;
 #define DEVOPS_FLAG_IGNORE_STATE  2
 
 struct XenDevOps {
-    size_t    size;
+    DeviceInfo qinfo;
+    char      type[64];
     uint32_t  flags;
     void      (*alloc)(struct XenDevice *xendev);
     int       (*init)(struct XenDevice *xendev);
@@ -30,7 +32,7 @@  struct XenDevOps {
 };
 
 struct XenDevice {
-    const char         *type;
+    DeviceState        qdev;
     int                dom;
     int                dev;
     char               name[64];
@@ -78,7 +80,8 @@  void xen_be_check_state(struct XenDevice *xendev);
 
 /* xen backend driver bits */
 int xen_be_init(void);
-int xen_be_register(const char *type, struct XenDevOps *ops);
+void xen_create_bus(DeviceState *parent);
+void xen_qdev_register(struct XenDevOps *ops);
 int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state);
 int xen_be_bind_evtchn(struct XenDevice *xendev);
 void xen_be_unbind_evtchn(struct XenDevice *xendev);
diff --git a/hw/xen_console.c b/hw/xen_console.c
index d2261f4..a980dc8 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -260,10 +260,18 @@  static void con_event(struct XenDevice *xendev)
 /* -------------------------------------------------------------------- */
 
 struct XenDevOps xen_console_ops = {
-    .size       = sizeof(struct XenConsole),
+    .qinfo.size = sizeof(struct XenConsole),
+    .type       = "console",
     .flags      = DEVOPS_FLAG_IGNORE_STATE,
     .init       = con_init,
     .connect    = con_connect,
     .event      = con_event,
     .disconnect = con_disconnect,
 };
+
+static void xen_console_register_devices(void)
+{
+    xen_qdev_register(&xen_console_ops);
+}
+
+device_init(xen_console_register_devices)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 06752de..5392f58 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -774,7 +774,8 @@  static void blk_event(struct XenDevice *xendev)
 }
 
 struct XenDevOps xen_blkdev_ops = {
-    .size       = sizeof(struct XenBlkDev),
+    .qinfo.size = sizeof(struct XenBlkDev),
+    .type       = "qdisk",
     .flags      = DEVOPS_FLAG_NEED_GNTDEV,
     .alloc      = blk_alloc,
     .init       = blk_init,
@@ -783,3 +784,10 @@  struct XenDevOps xen_blkdev_ops = {
     .event      = blk_event,
     .free       = blk_free,
 };
+
+static void xen_blkdev_register_devices(void)
+{
+    xen_qdev_register(&xen_blkdev_ops);
+}
+
+device_init(xen_blkdev_register_devices)
diff --git a/hw/xen_machine_pv.c b/hw/xen_machine_pv.c
index 77a34bf..b94d6e9 100644
--- a/hw/xen_machine_pv.c
+++ b/hw/xen_machine_pv.c
@@ -75,11 +75,7 @@  static void xen_init_pv(ram_addr_t ram_size,
         break;
     }
 
-    xen_be_register("console", &xen_console_ops);
-    xen_be_register("vkbd", &xen_kbdmouse_ops);
-    xen_be_register("vfb", &xen_framebuffer_ops);
-    xen_be_register("qdisk", &xen_blkdev_ops);
-    xen_be_register("qnic", &xen_netdev_ops);
+    xen_create_bus(NULL);
 
     /* configure framebuffer */
     if (xenfb_enabled) {
diff --git a/hw/xen_nic.c b/hw/xen_nic.c
index 08055b8..02d3c4e 100644
--- a/hw/xen_nic.c
+++ b/hw/xen_nic.c
@@ -411,7 +411,8 @@  static int net_free(struct XenDevice *xendev)
 /* ------------------------------------------------------------- */
 
 struct XenDevOps xen_netdev_ops = {
-    .size       = sizeof(struct XenNetDev),
+    .qinfo.size = sizeof(struct XenNetDev),
+    .type       = "qnic",
     .flags      = DEVOPS_FLAG_NEED_GNTDEV,
     .init       = net_init,
     .connect    = net_connect,
@@ -419,3 +420,10 @@  struct XenDevOps xen_netdev_ops = {
     .disconnect = net_disconnect,
     .free       = net_free,
 };
+
+static void xen_netdev_register_devices(void)
+{
+    xen_qdev_register(&xen_netdev_ops);
+}
+
+device_init(xen_netdev_register_devices)
diff --git a/hw/xenfb.c b/hw/xenfb.c
index da5297b..293210f 100644
--- a/hw/xenfb.c
+++ b/hw/xenfb.c
@@ -953,7 +953,8 @@  static void fb_event(struct XenDevice *xendev)
 /* -------------------------------------------------------------------- */
 
 struct XenDevOps xen_kbdmouse_ops = {
-    .size       = sizeof(struct XenInput),
+    .qinfo.size = sizeof(struct XenInput),
+    .type       = "vkbd",
     .init       = input_init,
     .connect    = input_connect,
     .disconnect = input_disconnect,
@@ -961,7 +962,8 @@  struct XenDevOps xen_kbdmouse_ops = {
 };
 
 struct XenDevOps xen_framebuffer_ops = {
-    .size       = sizeof(struct XenFB),
+    .qinfo.size = sizeof(struct XenFB),
+    .type       = "vfb",
     .init       = fb_init,
     .connect    = fb_connect,
     .disconnect = fb_disconnect,
@@ -1011,3 +1013,11 @@  wait_more:
     xen_be_check_state(xin);
     xen_be_check_state(xfb);
 }
+
+static void xenfb_register_devices(void)
+{
+    xen_qdev_register(&xen_kbdmouse_ops);
+    xen_qdev_register(&xen_framebuffer_ops);
+}
+
+device_init(xenfb_register_devices)