[v4,16/18] xen: automatically create XenBlockDevice-s
diff mbox series

Message ID 1544543862-9997-17-git-send-email-paul.durrant@citrix.com
State New
Headers show
Series
  • Xen PV backend 'qdevification'
Related show

Commit Message

Paul Durrant Dec. 11, 2018, 3:57 p.m. UTC
This patch adds a creator function for XenBlockDevice-s so that they can
be created automatically when the Xen toolstack instantiates a new
PV backend. When the XenBlockDevice is created this way it is also
necessary to create a drive which matches the configuration that the Xen
toolstack has written into xenstore. This drive is marked 'auto_del' so
that it will be removed when the XenBlockDevice is destroyed. Also, for
compatibility with the legacy 'xen_disk' implementation, an iothread
is automatically created for the new XenBlockDevice. This will also be
removed when the XenBlockDevice is destroyed.

Correspondingly the legacy backend scan for 'qdisk' is removed.

After this patch is applied the legacy 'xen_disk' code is redundant. It
will be removed by a subsequent patch.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Anthony Perard <anthony.perard@citrix.com>
---
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v2:
 - Get rid of error_abort
 - Don't use qdev_init_nofail
 - Explain why file locking needs to be off
---
 hw/block/trace-events       |   1 +
 hw/block/xen-block.c        | 261 +++++++++++++++++++++++++++++++++++++++++++-
 hw/xen/xen-legacy-backend.c |   1 -
 include/hw/xen/xen-block.h  |   1 +
 4 files changed, 262 insertions(+), 2 deletions(-)

Comments

Kevin Wolf Dec. 13, 2018, 11:51 a.m. UTC | #1
Am 11.12.2018 um 16:57 hat Paul Durrant geschrieben:
> This patch adds a creator function for XenBlockDevice-s so that they can
> be created automatically when the Xen toolstack instantiates a new
> PV backend. When the XenBlockDevice is created this way it is also
> necessary to create a drive which matches the configuration that the Xen
> toolstack has written into xenstore. This drive is marked 'auto_del' so
> that it will be removed when the XenBlockDevice is destroyed. Also, for
> compatibility with the legacy 'xen_disk' implementation, an iothread
> is automatically created for the new XenBlockDevice. This will also be
> removed when the XenBlockDevice is destroyed.
> 
> Correspondingly the legacy backend scan for 'qdisk' is removed.
> 
> After this patch is applied the legacy 'xen_disk' code is redundant. It
> will be removed by a subsequent patch.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Anthony Perard <anthony.perard@citrix.com>

So I have two points for this patch.

The first is that devices creating their own backends feels so wrong. I
know that the old xen_disk did the same, and fixing it might neither be
easy nor directly related to the qdevification, so requiring that from
you would probably be unfair. But I still have to make the note, and
hopefully we can get to it eventually (or maybe it is even easy enough
that we can indeed address it in this series).

My problem here is that I don't really understand the Xen mechanisms.
Could you give me a very high-level overview of how adding a disk works
and which component communicates with which other component to get the
information down to QEMU and eventually the newly added
xen_block_device_create()?

Essentially, what I'm wondering is whether we have anything that could
be treated more or less like another monitor besides QMP and HMP, which
would internally work similar to HMP, i.e. map (almost) everything to
QMP commands. I see that there is this XenWatch infrastructure to get
notified about changes (which would be treated like monitor commands),
but I'm not sure if everything would be covered by this mechanism or
whether some things must be fetched explicitly.

Anyway, this is probably for later.

> +static void xen_block_drive_create(const char *id, const char *device_type,
> +                                   QDict *opts, Error **errp)
> +{
> +    const char *params = qdict_get_try_str(opts, "params");
> +    const char *mode = qdict_get_try_str(opts, "mode");
> +    const char *direct_io_safe = qdict_get_try_str(opts, "direct-io-safe");
> +    const char *discard_enable = qdict_get_try_str(opts, "discard-enable");
> +    char *format = NULL;
> +    char *file = NULL;
> +    char *drive_optstr = NULL;
> +    QemuOpts *drive_opts;
> +    Error *local_err = NULL;
> +
> +    if (params) {
> +        char **v = g_strsplit(params, ":", 2);
> +
> +        if (v[1] == NULL) {
> +            file = g_strdup(v[0]);
> +        } else {
> +            if (strcmp(v[0], "aio") == 0) {
> +                format = g_strdup("raw");
> +            } else if (strcmp(v[0], "vhd") == 0) {
> +                format = g_strdup("vpc");
> +            } else {
> +                format = g_strdup(v[0]);
> +            }
> +            file = g_strdup(v[1]);
> +        }
> +
> +        g_strfreev(v);
> +    }
> +
> +    if (!file) {
> +        error_setg(errp, "no file parameter");
> +        return;
> +    }
> +
> +    drive_optstr = g_strdup_printf("id=%s", id);
> +    drive_opts = drive_def(drive_optstr);
> +    if (!drive_opts) {
> +        error_setg(errp, "failed to create drive options");
> +        goto done;
> +    }
> +
> +    qemu_opt_set(drive_opts, "file", file, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err, "failed to set 'file': ");
> +        goto done;
> +    }
> +
> +    qemu_opt_set(drive_opts, "media", device_type, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "failed to set 'media': ");
> +        goto done;
> +    }
> +
> +    if (format) {
> +        qemu_opt_set(drive_opts, "format", format, &local_err);
> +        if (local_err) {
> +            error_propagate_prepend(errp, local_err,
> +                                    "failed to set 'format': ");
> +            goto done;
> +        }
> +    }
> +
> +    if (mode && *mode != 'w') {
> +        qemu_opt_set_bool(drive_opts, BDRV_OPT_READ_ONLY, true, &local_err);
> +        if (local_err) {
> +            error_propagate_prepend(errp, local_err, "failed to set '%s': ",
> +                                    BDRV_OPT_READ_ONLY);
> +            goto done;
> +        }
> +    }
> +
> +    /*
> +     * It is necessary to turn file locking off as an emulated device
> +     * my have already opened the same image file.
> +     */
> +    qemu_opt_set(drive_opts, "file.locking", "off", &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "failed to set 'file.locking': ");
> +        goto done;
> +    }
> +
> +    qemu_opt_set_bool(drive_opts, BDRV_OPT_CACHE_WB, true, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err, "failed to set '%s': ",
> +                                BDRV_OPT_CACHE_WB);
> +        goto done;
> +    }
> +
> +    if (direct_io_safe) {
> +        qemu_opt_set_bool(drive_opts, BDRV_OPT_CACHE_DIRECT, true,
> +                          &local_err);
> +        if (local_err) {
> +            error_propagate_prepend(errp, local_err, "failed to set '%s': ",
> +                                    BDRV_OPT_CACHE_DIRECT);
> +            goto done;
> +        }
> +
> +        qemu_opt_set(drive_opts, "aio", "native", &local_err);
> +        if (local_err) {
> +            error_propagate_prepend(errp, local_err,
> +                                    "failed to set 'aio': ");
> +            goto done;
> +        }
> +    }
> +
> +    if (discard_enable) {
> +        unsigned long value;
> +
> +        if (!qemu_strtoul(discard_enable, NULL, 2, &value)) {
> +            qemu_opt_set_bool(drive_opts, BDRV_OPT_DISCARD, !!value,
> +                              &local_err);
> +            if (local_err) {
> +                error_propagate_prepend(errp, local_err,
> +                                        "failed to set '%s': ",
> +                                        BDRV_OPT_DISCARD);
> +                goto done;
> +            }
> +        }
> +    }
> +
> +    drive_new(drive_opts, IF_NONE, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "failed to create drive: ");
> +        goto done;
> +    }

The other major point is that you're using the legacy drive_*()
infrastructure, which should not only go away as soon as we can, but
which is also full of magic and nasty surprises.

I think the best way would be to create only a block node
(BlockDriverState) here, and get an automatically created anonymous
BlockBackend from the qdev drive property.

There are two ways to achieve this: qmp_blockdev_add() would be optimal
because that's a stable external interface. It would require you to
specify a node-name (you already have the id parameter), and you'd use
this node-name for the qdev drive property.

qmp_blockdev_add() requires a BlockdevOptions object, which you can
either construct manually in C or use a visitor to convert from an
options QDict. Maybe in this case, converting from a QDict is better
because otherwise you need special code for each block driver.

The other way would be calling bdrv_open() directly, which gives you a
BlockDriverState, but it risks using legacy functionality that will be
deprecated soon. Again, you'd take the node-name and pass it to the qdev
drive option below.

> +
> +done:
> +    g_free(drive_optstr);
> +    g_free(format);
> +    g_free(file);
> +}
> +
> +static void xen_block_device_create(BusState *bus, const char *name,
> +                                    QDict *opts, Error **errp)
> +{
> +    unsigned long number;
> +    const char *vdev, *device_type;
> +    BlockBackend *blk = NULL;
> +    IOThread *iothread = NULL;
> +    DeviceState *dev = NULL;
> +    Error *local_err = NULL;
> +    const char *type;
> +    XenBlockDevice *blockdev;
> +
> +    trace_xen_block_device_create(name);
> +
> +    if (qemu_strtoul(name, NULL, 10, &number)) {
> +        error_setg(errp, "failed to parse name '%s'", name);
> +        return;
> +    }
> +
> +    vdev = qdict_get_try_str(opts, "dev");
> +    if (!vdev) {
> +        error_setg(errp, "no dev parameter");
> +        return;
> +    }
> +
> +    device_type = qdict_get_try_str(opts, "device-type");
> +    if (!device_type) {
> +        error_setg(errp, "no device-type parameter");
> +        return;
> +    }
> +
> +    if (!strcmp(device_type, "disk")) {
> +        type = TYPE_XEN_DISK_DEVICE;
> +    } else if (!strcmp(device_type, "cdrom")) {
> +        type = TYPE_XEN_CDROM_DEVICE;
> +    } else {
> +        error_setg(errp, "invalid device-type parameter '%s'", device_type);
> +        return;
> +    }
> +
> +    xen_block_drive_create(vdev, device_type, opts, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    blk = blk_by_name(vdev);
> +    g_assert(blk);
> +
> +    iothread = iothread_create(vdev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto unref;
> +    }
> +
> +    dev = qdev_create(bus, type);
> +    blockdev = XEN_BLOCK_DEVICE(dev);
> +
> +    qdev_prop_set_string(dev, "vdev", vdev);
> +    if (blockdev->vdev.number != number) {
> +        error_setg(errp, "invalid dev parameter '%s'", vdev);
> +        goto unref;
> +    }
> +
> +    qdev_prop_set_drive(dev, "drive", blk, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err, "failed to set 'drive': ");
> +        goto unref;
> +    }

So here you would need to use something like this:

object_property_set_str(OBJECT(dev), vdev, "driver", &local_err);

> +
> +    blockdev->auto_iothread = iothread;
> +
> +    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "initialization of device %s failed: ",
> +                                type);
> +        goto unref;
> +    }
> +
> +    blockdev_mark_auto_del(blk);

You don't need this one any more then (if you look into the details,
it's one of the more confusing parts of the drive_*() magic, so it's
good to get rid of it). When you use the anonymous BlockBackend created
by the qdev drive property (because you passed it a node-name rather
than a BlockBackend name) means that the BlockBackend disappears
together with the drive.

Note that explicitly created block nodes must also be unreferenced
explicitly (bdrv_open() should be paired with bdrv_unref() and
qmp_blockdev_add() with qmp_blockdev_del()). Maybe XenBackendInfo needs
a .destroy callback so we can do destruction symmetrically to device
creation?

> +    return;
> +
> +unref:
> +    if (dev) {
> +        object_unparent(OBJECT(dev));
> +    }
> +
> +    if (iothread) {
> +        iothread_destroy(iothread);
> +    }
> +
> +    if (blk) {
> +        monitor_remove_blk(blk);
> +        blk_unref(blk);
> +    }
> +}

Kevin
Paul Durrant Dec. 13, 2018, 12:44 p.m. UTC | #2
> -----Original Message-----
> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Sent: 13 December 2018 11:52
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-
> devel@lists.xenproject.org; Max Reitz <mreitz@redhat.com>; Stefano
> Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v4 16/18] xen: automatically create XenBlockDevice-s
> 
> Am 11.12.2018 um 16:57 hat Paul Durrant geschrieben:
> > This patch adds a creator function for XenBlockDevice-s so that they can
> > be created automatically when the Xen toolstack instantiates a new
> > PV backend. When the XenBlockDevice is created this way it is also
> > necessary to create a drive which matches the configuration that the Xen
> > toolstack has written into xenstore. This drive is marked 'auto_del' so
> > that it will be removed when the XenBlockDevice is destroyed. Also, for
> > compatibility with the legacy 'xen_disk' implementation, an iothread
> > is automatically created for the new XenBlockDevice. This will also be
> > removed when the XenBlockDevice is destroyed.
> >
> > Correspondingly the legacy backend scan for 'qdisk' is removed.
> >
> > After this patch is applied the legacy 'xen_disk' code is redundant. It
> > will be removed by a subsequent patch.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: Anthony Perard <anthony.perard@citrix.com>
> 
> So I have two points for this patch.
> 
> The first is that devices creating their own backends feels so wrong.

Indeed it does feel wrong, but this is the only way to deal with existing Xen toolstacks. Once toolstacks have been ported to using QMP to instantiate backends then this code can go away.

> I
> know that the old xen_disk did the same, and fixing it might neither be
> easy nor directly related to the qdevification, so requiring that from
> you would probably be unfair. But I still have to make the note, and
> hopefully we can get to it eventually (or maybe it is even easy enough
> that we can indeed address it in this series).
> 

No, it's not easy but once this series is committed the work can be started.

> My problem here is that I don't really understand the Xen mechanisms.
> Could you give me a very high-level overview of how adding a disk works
> and which component communicates with which other component to get the
> information down to QEMU and eventually the newly added
> xen_block_device_create()?

Xen toolstacks instantiate PV backends by just writing values into xenstore. It is up to the entity implementing the backend to set 'watches' so that it gets notified when these values appear. Currently that entity may be QEMU, or it may be a kernel driver such as Linux xen-blkback.ko.

> 
> Essentially, what I'm wondering is whether we have anything that could
> be treated more or less like another monitor besides QMP and HMP, which
> would internally work similar to HMP, i.e. map (almost) everything to
> QMP commands.

Yes, it would be possible to have a separate 'compatibility' daemon to watch xenstore and then formulate the correct sequence of QMP commands to instantiate the backend, but that is more complicated and the right answer of course is to have the toolstack send the QMP commands in the first place.

> I see that there is this XenWatch infrastructure to get
> notified about changes (which would be treated like monitor commands),
> but I'm not sure if everything would be covered by this mechanism or
> whether some things must be fetched explicitly.
> 
> Anyway, this is probably for later.
> 
> > +static void xen_block_drive_create(const char *id, const char
> *device_type,
> > +                                   QDict *opts, Error **errp)
> > +{
> > +    const char *params = qdict_get_try_str(opts, "params");
> > +    const char *mode = qdict_get_try_str(opts, "mode");
> > +    const char *direct_io_safe = qdict_get_try_str(opts, "direct-io-
> safe");
> > +    const char *discard_enable = qdict_get_try_str(opts, "discard-
> enable");
> > +    char *format = NULL;
> > +    char *file = NULL;
> > +    char *drive_optstr = NULL;
> > +    QemuOpts *drive_opts;
> > +    Error *local_err = NULL;
> > +
> > +    if (params) {
> > +        char **v = g_strsplit(params, ":", 2);
> > +
> > +        if (v[1] == NULL) {
> > +            file = g_strdup(v[0]);
> > +        } else {
> > +            if (strcmp(v[0], "aio") == 0) {
> > +                format = g_strdup("raw");
> > +            } else if (strcmp(v[0], "vhd") == 0) {
> > +                format = g_strdup("vpc");
> > +            } else {
> > +                format = g_strdup(v[0]);
> > +            }
> > +            file = g_strdup(v[1]);
> > +        }
> > +
> > +        g_strfreev(v);
> > +    }
> > +
> > +    if (!file) {
> > +        error_setg(errp, "no file parameter");
> > +        return;
> > +    }
> > +
> > +    drive_optstr = g_strdup_printf("id=%s", id);
> > +    drive_opts = drive_def(drive_optstr);
> > +    if (!drive_opts) {
> > +        error_setg(errp, "failed to create drive options");
> > +        goto done;
> > +    }
> > +
> > +    qemu_opt_set(drive_opts, "file", file, &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err, "failed to set 'file':
> ");
> > +        goto done;
> > +    }
> > +
> > +    qemu_opt_set(drive_opts, "media", device_type, &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err,
> > +                                "failed to set 'media': ");
> > +        goto done;
> > +    }
> > +
> > +    if (format) {
> > +        qemu_opt_set(drive_opts, "format", format, &local_err);
> > +        if (local_err) {
> > +            error_propagate_prepend(errp, local_err,
> > +                                    "failed to set 'format': ");
> > +            goto done;
> > +        }
> > +    }
> > +
> > +    if (mode && *mode != 'w') {
> > +        qemu_opt_set_bool(drive_opts, BDRV_OPT_READ_ONLY, true,
> &local_err);
> > +        if (local_err) {
> > +            error_propagate_prepend(errp, local_err, "failed to set
> '%s': ",
> > +                                    BDRV_OPT_READ_ONLY);
> > +            goto done;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * It is necessary to turn file locking off as an emulated device
> > +     * my have already opened the same image file.
> > +     */
> > +    qemu_opt_set(drive_opts, "file.locking", "off", &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err,
> > +                                "failed to set 'file.locking': ");
> > +        goto done;
> > +    }
> > +
> > +    qemu_opt_set_bool(drive_opts, BDRV_OPT_CACHE_WB, true, &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err, "failed to set '%s':
> ",
> > +                                BDRV_OPT_CACHE_WB);
> > +        goto done;
> > +    }
> > +
> > +    if (direct_io_safe) {
> > +        qemu_opt_set_bool(drive_opts, BDRV_OPT_CACHE_DIRECT, true,
> > +                          &local_err);
> > +        if (local_err) {
> > +            error_propagate_prepend(errp, local_err, "failed to set
> '%s': ",
> > +                                    BDRV_OPT_CACHE_DIRECT);
> > +            goto done;
> > +        }
> > +
> > +        qemu_opt_set(drive_opts, "aio", "native", &local_err);
> > +        if (local_err) {
> > +            error_propagate_prepend(errp, local_err,
> > +                                    "failed to set 'aio': ");
> > +            goto done;
> > +        }
> > +    }
> > +
> > +    if (discard_enable) {
> > +        unsigned long value;
> > +
> > +        if (!qemu_strtoul(discard_enable, NULL, 2, &value)) {
> > +            qemu_opt_set_bool(drive_opts, BDRV_OPT_DISCARD, !!value,
> > +                              &local_err);
> > +            if (local_err) {
> > +                error_propagate_prepend(errp, local_err,
> > +                                        "failed to set '%s': ",
> > +                                        BDRV_OPT_DISCARD);
> > +                goto done;
> > +            }
> > +        }
> > +    }
> > +
> > +    drive_new(drive_opts, IF_NONE, &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err,
> > +                                "failed to create drive: ");
> > +        goto done;
> > +    }
> 
> The other major point is that you're using the legacy drive_*()
> infrastructure, which should not only go away as soon as we can, but
> which is also full of magic and nasty surprises.
> 
> I think the best way would be to create only a block node
> (BlockDriverState) here, and get an automatically created anonymous
> BlockBackend from the qdev drive property.
> 
> There are two ways to achieve this: qmp_blockdev_add() would be optimal
> because that's a stable external interface. It would require you to
> specify a node-name (you already have the id parameter), and you'd use
> this node-name for the qdev drive property.
> 
> qmp_blockdev_add() requires a BlockdevOptions object, which you can
> either construct manually in C or use a visitor to convert from an
> options QDict. Maybe in this case, converting from a QDict is better
> because otherwise you need special code for each block driver.
> 

I was using the legacy interfaces because this code is, as I said above, supposed to be a mechanism only required for compatibility with the way toolstacks currently operate (and so is essentially 'legacy') but using the top-level QMP entry point to construct the does sound do-able as long as the underlying file locking can still be avoided with that mechanism. Since BlockdevOptions seems to be an auto-generated structure, figuring out how to fill it in manually is somewhat tricky so the QDict approach is preferable but I'll have to figure out how to use a visitor to do the translation.


> The other way would be calling bdrv_open() directly, which gives you a
> BlockDriverState, but it risks using legacy functionality that will be
> deprecated soon. Again, you'd take the node-name and pass it to the qdev
> drive option below.

Yes, xen_disk does things this way but then we end up with legacy block device and still fall foul of the assertions buried in the code.

> 
> > +
> > +done:
> > +    g_free(drive_optstr);
> > +    g_free(format);
> > +    g_free(file);
> > +}
> > +
> > +static void xen_block_device_create(BusState *bus, const char *name,
> > +                                    QDict *opts, Error **errp)
> > +{
> > +    unsigned long number;
> > +    const char *vdev, *device_type;
> > +    BlockBackend *blk = NULL;
> > +    IOThread *iothread = NULL;
> > +    DeviceState *dev = NULL;
> > +    Error *local_err = NULL;
> > +    const char *type;
> > +    XenBlockDevice *blockdev;
> > +
> > +    trace_xen_block_device_create(name);
> > +
> > +    if (qemu_strtoul(name, NULL, 10, &number)) {
> > +        error_setg(errp, "failed to parse name '%s'", name);
> > +        return;
> > +    }
> > +
> > +    vdev = qdict_get_try_str(opts, "dev");
> > +    if (!vdev) {
> > +        error_setg(errp, "no dev parameter");
> > +        return;
> > +    }
> > +
> > +    device_type = qdict_get_try_str(opts, "device-type");
> > +    if (!device_type) {
> > +        error_setg(errp, "no device-type parameter");
> > +        return;
> > +    }
> > +
> > +    if (!strcmp(device_type, "disk")) {
> > +        type = TYPE_XEN_DISK_DEVICE;
> > +    } else if (!strcmp(device_type, "cdrom")) {
> > +        type = TYPE_XEN_CDROM_DEVICE;
> > +    } else {
> > +        error_setg(errp, "invalid device-type parameter '%s'",
> device_type);
> > +        return;
> > +    }
> > +
> > +    xen_block_drive_create(vdev, device_type, opts, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    blk = blk_by_name(vdev);
> > +    g_assert(blk);
> > +
> > +    iothread = iothread_create(vdev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        goto unref;
> > +    }
> > +
> > +    dev = qdev_create(bus, type);
> > +    blockdev = XEN_BLOCK_DEVICE(dev);
> > +
> > +    qdev_prop_set_string(dev, "vdev", vdev);
> > +    if (blockdev->vdev.number != number) {
> > +        error_setg(errp, "invalid dev parameter '%s'", vdev);
> > +        goto unref;
> > +    }
> > +
> > +    qdev_prop_set_drive(dev, "drive", blk, &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err, "failed to set
> 'drive': ");
> > +        goto unref;
> > +    }
> 
> So here you would need to use something like this:
> 
> object_property_set_str(OBJECT(dev), vdev, "driver", &local_err);
> 
> > +
> > +    blockdev->auto_iothread = iothread;
> > +
> > +    object_property_set_bool(OBJECT(dev), true, "realized",
> &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err,
> > +                                "initialization of device %s failed: ",
> > +                                type);
> > +        goto unref;
> > +    }
> > +
> > +    blockdev_mark_auto_del(blk);
> 
> You don't need this one any more then (if you look into the details,
> it's one of the more confusing parts of the drive_*() magic, so it's
> good to get rid of it). When you use the anonymous BlockBackend created
> by the qdev drive property (because you passed it a node-name rather
> than a BlockBackend name) means that the BlockBackend disappears
> together with the drive.
> 

Ok.

> Note that explicitly created block nodes must also be unreferenced
> explicitly (bdrv_open() should be paired with bdrv_unref() and
> qmp_blockdev_add() with qmp_blockdev_del()). Maybe XenBackendInfo needs
> a .destroy callback so we can do destruction symmetrically to device
> creation?
> 

Yes, I'd probably just add a callback function pointer into XenDevice which only gets set for devices instantiated via this mechanism.

  Paul

> > +    return;
> > +
> > +unref:
> > +    if (dev) {
> > +        object_unparent(OBJECT(dev));
> > +    }
> > +
> > +    if (iothread) {
> > +        iothread_destroy(iothread);
> > +    }
> > +
> > +    if (blk) {
> > +        monitor_remove_blk(blk);
> > +        blk_unref(blk);
> > +    }
> > +}
> 
> Kevin
Kevin Wolf Dec. 13, 2018, 3:08 p.m. UTC | #3
Am 13.12.2018 um 13:44 hat Paul Durrant geschrieben:
> > Essentially, what I'm wondering is whether we have anything that could
> > be treated more or less like another monitor besides QMP and HMP, which
> > would internally work similar to HMP, i.e. map (almost) everything to
> > QMP commands.
> 
> Yes, it would be possible to have a separate 'compatibility' daemon to
> watch xenstore and then formulate the correct sequence of QMP commands
> to instantiate the backend, but that is more complicated and the right
> answer of course is to have the toolstack send the QMP commands in the
> first place.

Okay, if someone is working on actually using QMP instead of xenstore,
that's even better, of course. Disregard this point then.

> > > +    drive_new(drive_opts, IF_NONE, &local_err);
> > > +    if (local_err) {
> > > +        error_propagate_prepend(errp, local_err,
> > > +                                "failed to create drive: ");
> > > +        goto done;
> > > +    }
> > 
> > The other major point is that you're using the legacy drive_*()
> > infrastructure, which should not only go away as soon as we can, but
> > which is also full of magic and nasty surprises.
> > 
> > I think the best way would be to create only a block node
> > (BlockDriverState) here, and get an automatically created anonymous
> > BlockBackend from the qdev drive property.
> > 
> > There are two ways to achieve this: qmp_blockdev_add() would be optimal
> > because that's a stable external interface. It would require you to
> > specify a node-name (you already have the id parameter), and you'd use
> > this node-name for the qdev drive property.
> > 
> > qmp_blockdev_add() requires a BlockdevOptions object, which you can
> > either construct manually in C or use a visitor to convert from an
> > options QDict. Maybe in this case, converting from a QDict is better
> > because otherwise you need special code for each block driver.
> > 
> 
> I was using the legacy interfaces because this code is, as I said
> above, supposed to be a mechanism only required for compatibility with
> the way toolstacks currently operate (and so is essentially 'legacy')
> but using the top-level QMP entry point to construct the does sound
> do-able as long as the underlying file locking can still be avoided
> with that mechanism. Since BlockdevOptions seems to be an
> auto-generated structure, figuring out how to fill it in manually is
> somewhat tricky so the QDict approach is preferable but I'll have to
> figure out how to use a visitor to do the translation.

You can grep for qobject_input_visitor_new() for examples. Some block
drivers use this internally to convert .bdrv_create options to a QAPI
object, it looks like this:

    /* Now get the QAPI type BlockdevCreateOptions */
    v = qobject_input_visitor_new_flat_confused(qdict, errp);
    if (!v) {
        ret = -EINVAL;
        goto finish;
    }

    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
    visit_free(v);

    if (local_err) {
        error_propagate(errp, local_err);
        ret = -EINVAL;
        goto finish;
    }

Obviously, you'd use visit_type_BlockdevOptions instead. You also might
not need the _flat_confused variant, which is about QDicts where we
don't know whether values are stored as their actual type or as strings.
In your code, you have control over the types in the QDict, so this
shouldn't be a problem.

> > The other way would be calling bdrv_open() directly, which gives you a
> > BlockDriverState, but it risks using legacy functionality that will be
> > deprecated soon. Again, you'd take the node-name and pass it to the qdev
> > drive option below.
> 
> Yes, xen_disk does things this way but then we end up with legacy
> block device and still fall foul of the assertions buried in the code.

Yes and no. xen_disk is better in that it avoids the drive_* things
(which internally call bdrv_open() anyway), but it's worse in that it
directly assigns blkdev->blk instead of using the qdev property.

What I meant here is that you create the BDS with bdrv_open(), but then
you wouldn't assign it directly to some field in the device state, but
just put the node-name of the BDS into the qdev property 'drive'. This
would be almost like qmp_blockdev_add(), except without validation
against the QAPI schema.

But if using qmp_blockdev_add() is easy enough, that's preferable.

> > 
> > > +
> > > +done:
> > > +    g_free(drive_optstr);
> > > +    g_free(format);
> > > +    g_free(file);
> > > +}
> > > +
> > > +static void xen_block_device_create(BusState *bus, const char *name,
> > > +                                    QDict *opts, Error **errp)
> > > +{
> > > +    unsigned long number;
> > > +    const char *vdev, *device_type;
> > > +    BlockBackend *blk = NULL;
> > > +    IOThread *iothread = NULL;
> > > +    DeviceState *dev = NULL;
> > > +    Error *local_err = NULL;
> > > +    const char *type;
> > > +    XenBlockDevice *blockdev;
> > > +
> > > +    trace_xen_block_device_create(name);
> > > +
> > > +    if (qemu_strtoul(name, NULL, 10, &number)) {
> > > +        error_setg(errp, "failed to parse name '%s'", name);
> > > +        return;
> > > +    }
> > > +
> > > +    vdev = qdict_get_try_str(opts, "dev");
> > > +    if (!vdev) {
> > > +        error_setg(errp, "no dev parameter");
> > > +        return;
> > > +    }
> > > +
> > > +    device_type = qdict_get_try_str(opts, "device-type");
> > > +    if (!device_type) {
> > > +        error_setg(errp, "no device-type parameter");
> > > +        return;
> > > +    }
> > > +
> > > +    if (!strcmp(device_type, "disk")) {
> > > +        type = TYPE_XEN_DISK_DEVICE;
> > > +    } else if (!strcmp(device_type, "cdrom")) {
> > > +        type = TYPE_XEN_CDROM_DEVICE;
> > > +    } else {
> > > +        error_setg(errp, "invalid device-type parameter '%s'",
> > device_type);
> > > +        return;
> > > +    }
> > > +
> > > +    xen_block_drive_create(vdev, device_type, opts, &local_err);
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +        return;
> > > +    }
> > > +
> > > +    blk = blk_by_name(vdev);
> > > +    g_assert(blk);
> > > +
> > > +    iothread = iothread_create(vdev, &local_err);
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +        goto unref;
> > > +    }
> > > +
> > > +    dev = qdev_create(bus, type);
> > > +    blockdev = XEN_BLOCK_DEVICE(dev);
> > > +
> > > +    qdev_prop_set_string(dev, "vdev", vdev);
> > > +    if (blockdev->vdev.number != number) {
> > > +        error_setg(errp, "invalid dev parameter '%s'", vdev);
> > > +        goto unref;
> > > +    }
> > > +
> > > +    qdev_prop_set_drive(dev, "drive", blk, &local_err);
> > > +    if (local_err) {
> > > +        error_propagate_prepend(errp, local_err, "failed to set
> > 'drive': ");
> > > +        goto unref;
> > > +    }
> > 
> > So here you would need to use something like this:
> > 
> > object_property_set_str(OBJECT(dev), vdev, "driver", &local_err);
> > 
> > > +
> > > +    blockdev->auto_iothread = iothread;
> > > +
> > > +    object_property_set_bool(OBJECT(dev), true, "realized",
> > &local_err);
> > > +    if (local_err) {
> > > +        error_propagate_prepend(errp, local_err,
> > > +                                "initialization of device %s failed: ",
> > > +                                type);
> > > +        goto unref;
> > > +    }
> > > +
> > > +    blockdev_mark_auto_del(blk);
> > 
> > You don't need this one any more then (if you look into the details,
> > it's one of the more confusing parts of the drive_*() magic, so it's
> > good to get rid of it). When you use the anonymous BlockBackend created
> > by the qdev drive property (because you passed it a node-name rather
> > than a BlockBackend name) means that the BlockBackend disappears
> > together with the drive.
> > 
> 
> Ok.
> 
> > Note that explicitly created block nodes must also be unreferenced
> > explicitly (bdrv_open() should be paired with bdrv_unref() and
> > qmp_blockdev_add() with qmp_blockdev_del()). Maybe XenBackendInfo needs
> > a .destroy callback so we can do destruction symmetrically to device
> > creation?
> > 
> 
> Yes, I'd probably just add a callback function pointer into XenDevice
> which only gets set for devices instantiated via this mechanism.

I think it's a bit nicer to have it in XenBackendInfo for symmetry and
because that sits outside the device, so the device doesn't destroy its
own backend, just like it doesn't create it (even if it's the same
source file).

But yes, whatever works best for you.

Kevin
Paul Durrant Dec. 14, 2018, 2:50 p.m. UTC | #4
> -----Original Message-----
[snip]
> > +
> > +    blockdev->auto_iothread = iothread;
> > +
> > +    object_property_set_bool(OBJECT(dev), true, "realized",
> &local_err);
> > +    if (local_err) {
> > +        error_propagate_prepend(errp, local_err,
> > +                                "initialization of device %s failed: ",
> > +                                type);
> > +        goto unref;
> > +    }
> > +
> > +    blockdev_mark_auto_del(blk);
> 
> You don't need this one any more then (if you look into the details,
> it's one of the more confusing parts of the drive_*() magic, so it's
> good to get rid of it). When you use the anonymous BlockBackend created
> by the qdev drive property (because you passed it a node-name rather
> than a BlockBackend name) means that the BlockBackend disappears
> together with the drive.
> 
> Note that explicitly created block nodes must also be unreferenced
> explicitly (bdrv_open() should be paired with bdrv_unref() and
> qmp_blockdev_add() with qmp_blockdev_del()). Maybe XenBackendInfo needs
> a .destroy callback so we can do destruction symmetrically to device
> creation?
> 

I have something implemented using qmp_blockdev_add() now and it seems to work, but when I call into qmp_blockdev_del() (passing in the node-name I used to the set the "drive" parameter) during unrealize then it tells me that the device is in use. Do I need a callback that runs after unrealize of the device?

  Paul 

> > +    return;
> > +
> > +unref:
> > +    if (dev) {
> > +        object_unparent(OBJECT(dev));
> > +    }
> > +
> > +    if (iothread) {
> > +        iothread_destroy(iothread);
> > +    }
> > +
> > +    if (blk) {
> > +        monitor_remove_blk(blk);
> > +        blk_unref(blk);
> > +    }
> > +}
> 
> Kevin
Paul Durrant Dec. 14, 2018, 3:39 p.m. UTC | #5
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 14 December 2018 14:50
> To: 'Kevin Wolf' <kwolf@redhat.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; qemu-devel@nongnu.org; qemu-block@nongnu.org;
> Max Reitz <mreitz@redhat.com>
> Subject: Re: [Xen-devel] [PATCH v4 16/18] xen: automatically create
> XenBlockDevice-s
> 
> > -----Original Message-----
> [snip]
> > > +
> > > +    blockdev->auto_iothread = iothread;
> > > +
> > > +    object_property_set_bool(OBJECT(dev), true, "realized",
> > &local_err);
> > > +    if (local_err) {
> > > +        error_propagate_prepend(errp, local_err,
> > > +                                "initialization of device %s failed:
> ",
> > > +                                type);
> > > +        goto unref;
> > > +    }
> > > +
> > > +    blockdev_mark_auto_del(blk);
> >
> > You don't need this one any more then (if you look into the details,
> > it's one of the more confusing parts of the drive_*() magic, so it's
> > good to get rid of it). When you use the anonymous BlockBackend created
> > by the qdev drive property (because you passed it a node-name rather
> > than a BlockBackend name) means that the BlockBackend disappears
> > together with the drive.
> >
> > Note that explicitly created block nodes must also be unreferenced
> > explicitly (bdrv_open() should be paired with bdrv_unref() and
> > qmp_blockdev_add() with qmp_blockdev_del()). Maybe XenBackendInfo needs
> > a .destroy callback so we can do destruction symmetrically to device
> > creation?
> >
> 
> I have something implemented using qmp_blockdev_add() now and it seems to
> work, but when I call into qmp_blockdev_del() (passing in the node-name I
> used to the set the "drive" parameter) during unrealize then it tells me
> that the device is in use. Do I need a callback that runs after unrealize
> of the device?

I have coded it up now and apparently I do.

  Paul

> 
>   Paul
> 
> > > +    return;
> > > +
> > > +unref:
> > > +    if (dev) {
> > > +        object_unparent(OBJECT(dev));
> > > +    }
> > > +
> > > +    if (iothread) {
> > > +        iothread_destroy(iothread);
> > > +    }
> > > +
> > > +    if (blk) {
> > > +        monitor_remove_blk(blk);
> > > +        blk_unref(blk);
> > > +    }
> > > +}
> >
> > Kevin
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Patch
diff mbox series

diff --git a/hw/block/trace-events b/hw/block/trace-events
index 89e2583..a89c8a6 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -137,3 +137,4 @@  xen_disk_realize(void) ""
 xen_disk_unrealize(void) ""
 xen_cdrom_realize(void) ""
 xen_cdrom_unrealize(void) ""
+xen_block_device_create(const char *name) "name: %s"
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index f3d21c6..0d1616d 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -7,12 +7,15 @@ 
 
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
+#include "qemu/option.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "qapi/qmp/qdict.h"
 #include "hw/hw.h"
 #include "hw/xen/xen_common.h"
 #include "hw/block/xen_blkif.h"
 #include "hw/xen/xen-block.h"
+#include "hw/xen/xen-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/iothread.h"
@@ -131,6 +134,11 @@  static void xen_block_unrealize(XenDevice *xendev, Error **errp)
     xen_block_dataplane_destroy(blockdev->dataplane);
     blockdev->dataplane = NULL;
 
+    if (blockdev->auto_iothread) {
+        iothread_destroy(blockdev->auto_iothread);
+        blockdev->auto_iothread = NULL;
+    }
+
     if (blockdev_class->unrealize) {
         blockdev_class->unrealize(blockdev, errp);
     }
@@ -145,6 +153,8 @@  static void xen_block_realize(XenDevice *xendev, Error **errp)
     XenBlockVdev *vdev = &blockdev->vdev;
     Error *local_err = NULL;
     BlockConf *conf = &blockdev->conf;
+    IOThread *iothread = blockdev->auto_iothread ?
+        blockdev->auto_iothread : blockdev->iothread;
 
     if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
         error_setg(errp, "vdev property not set");
@@ -212,7 +222,7 @@  static void xen_block_realize(XenDevice *xendev, Error **errp)
                               conf->logical_block_size);
 
     blockdev->dataplane = xen_block_dataplane_create(xendev, conf,
-                                                     blockdev->iothread);
+                                                     iothread);
 }
 
 static void xen_block_frontend_changed(XenDevice *xendev,
@@ -474,6 +484,7 @@  static void xen_block_class_init(ObjectClass *class, void *data)
     DeviceClass *dev_class = DEVICE_CLASS(class);
     XenDeviceClass *xendev_class = XEN_DEVICE_CLASS(class);
 
+    xendev_class->backend = "qdisk";
     xendev_class->device = "vbd";
     xendev_class->get_name = xen_block_get_name;
     xendev_class->realize = xen_block_realize;
@@ -586,3 +597,251 @@  static void xen_block_register_types(void)
 }
 
 type_init(xen_block_register_types)
+
+static void xen_block_drive_create(const char *id, const char *device_type,
+                                   QDict *opts, Error **errp)
+{
+    const char *params = qdict_get_try_str(opts, "params");
+    const char *mode = qdict_get_try_str(opts, "mode");
+    const char *direct_io_safe = qdict_get_try_str(opts, "direct-io-safe");
+    const char *discard_enable = qdict_get_try_str(opts, "discard-enable");
+    char *format = NULL;
+    char *file = NULL;
+    char *drive_optstr = NULL;
+    QemuOpts *drive_opts;
+    Error *local_err = NULL;
+
+    if (params) {
+        char **v = g_strsplit(params, ":", 2);
+
+        if (v[1] == NULL) {
+            file = g_strdup(v[0]);
+        } else {
+            if (strcmp(v[0], "aio") == 0) {
+                format = g_strdup("raw");
+            } else if (strcmp(v[0], "vhd") == 0) {
+                format = g_strdup("vpc");
+            } else {
+                format = g_strdup(v[0]);
+            }
+            file = g_strdup(v[1]);
+        }
+
+        g_strfreev(v);
+    }
+
+    if (!file) {
+        error_setg(errp, "no file parameter");
+        return;
+    }
+
+    drive_optstr = g_strdup_printf("id=%s", id);
+    drive_opts = drive_def(drive_optstr);
+    if (!drive_opts) {
+        error_setg(errp, "failed to create drive options");
+        goto done;
+    }
+
+    qemu_opt_set(drive_opts, "file", file, &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err, "failed to set 'file': ");
+        goto done;
+    }
+
+    qemu_opt_set(drive_opts, "media", device_type, &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err,
+                                "failed to set 'media': ");
+        goto done;
+    }
+
+    if (format) {
+        qemu_opt_set(drive_opts, "format", format, &local_err);
+        if (local_err) {
+            error_propagate_prepend(errp, local_err,
+                                    "failed to set 'format': ");
+            goto done;
+        }
+    }
+
+    if (mode && *mode != 'w') {
+        qemu_opt_set_bool(drive_opts, BDRV_OPT_READ_ONLY, true, &local_err);
+        if (local_err) {
+            error_propagate_prepend(errp, local_err, "failed to set '%s': ",
+                                    BDRV_OPT_READ_ONLY);
+            goto done;
+        }
+    }
+
+    /*
+     * It is necessary to turn file locking off as an emulated device
+     * my have already opened the same image file.
+     */
+    qemu_opt_set(drive_opts, "file.locking", "off", &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err,
+                                "failed to set 'file.locking': ");
+        goto done;
+    }
+
+    qemu_opt_set_bool(drive_opts, BDRV_OPT_CACHE_WB, true, &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err, "failed to set '%s': ",
+                                BDRV_OPT_CACHE_WB);
+        goto done;
+    }
+
+    if (direct_io_safe) {
+        qemu_opt_set_bool(drive_opts, BDRV_OPT_CACHE_DIRECT, true,
+                          &local_err);
+        if (local_err) {
+            error_propagate_prepend(errp, local_err, "failed to set '%s': ",
+                                    BDRV_OPT_CACHE_DIRECT);
+            goto done;
+        }
+
+        qemu_opt_set(drive_opts, "aio", "native", &local_err);
+        if (local_err) {
+            error_propagate_prepend(errp, local_err,
+                                    "failed to set 'aio': ");
+            goto done;
+        }
+    }
+
+    if (discard_enable) {
+        unsigned long value;
+
+        if (!qemu_strtoul(discard_enable, NULL, 2, &value)) {
+            qemu_opt_set_bool(drive_opts, BDRV_OPT_DISCARD, !!value,
+                              &local_err);
+            if (local_err) {
+                error_propagate_prepend(errp, local_err,
+                                        "failed to set '%s': ",
+                                        BDRV_OPT_DISCARD);
+                goto done;
+            }
+        }
+    }
+
+    drive_new(drive_opts, IF_NONE, &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err,
+                                "failed to create drive: ");
+        goto done;
+    }
+
+done:
+    g_free(drive_optstr);
+    g_free(format);
+    g_free(file);
+}
+
+static void xen_block_device_create(BusState *bus, const char *name,
+                                    QDict *opts, Error **errp)
+{
+    unsigned long number;
+    const char *vdev, *device_type;
+    BlockBackend *blk = NULL;
+    IOThread *iothread = NULL;
+    DeviceState *dev = NULL;
+    Error *local_err = NULL;
+    const char *type;
+    XenBlockDevice *blockdev;
+
+    trace_xen_block_device_create(name);
+
+    if (qemu_strtoul(name, NULL, 10, &number)) {
+        error_setg(errp, "failed to parse name '%s'", name);
+        return;
+    }
+
+    vdev = qdict_get_try_str(opts, "dev");
+    if (!vdev) {
+        error_setg(errp, "no dev parameter");
+        return;
+    }
+
+    device_type = qdict_get_try_str(opts, "device-type");
+    if (!device_type) {
+        error_setg(errp, "no device-type parameter");
+        return;
+    }
+
+    if (!strcmp(device_type, "disk")) {
+        type = TYPE_XEN_DISK_DEVICE;
+    } else if (!strcmp(device_type, "cdrom")) {
+        type = TYPE_XEN_CDROM_DEVICE;
+    } else {
+        error_setg(errp, "invalid device-type parameter '%s'", device_type);
+        return;
+    }
+
+    xen_block_drive_create(vdev, device_type, opts, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    blk = blk_by_name(vdev);
+    g_assert(blk);
+
+    iothread = iothread_create(vdev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto unref;
+    }
+
+    dev = qdev_create(bus, type);
+    blockdev = XEN_BLOCK_DEVICE(dev);
+
+    qdev_prop_set_string(dev, "vdev", vdev);
+    if (blockdev->vdev.number != number) {
+        error_setg(errp, "invalid dev parameter '%s'", vdev);
+        goto unref;
+    }
+
+    qdev_prop_set_drive(dev, "drive", blk, &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err, "failed to set 'drive': ");
+        goto unref;
+    }
+
+    blockdev->auto_iothread = iothread;
+
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err) {
+        error_propagate_prepend(errp, local_err,
+                                "initialization of device %s failed: ",
+                                type);
+        goto unref;
+    }
+
+    blockdev_mark_auto_del(blk);
+    return;
+
+unref:
+    if (dev) {
+        object_unparent(OBJECT(dev));
+    }
+
+    if (iothread) {
+        iothread_destroy(iothread);
+    }
+
+    if (blk) {
+        monitor_remove_blk(blk);
+        blk_unref(blk);
+    }
+}
+
+static const XenBackendInfo xen_block_backend_info = {
+    .type = "qdisk",
+    .create = xen_block_device_create,
+};
+
+static void xen_block_register_backend(void)
+{
+    xen_backend_register(&xen_block_backend_info);
+}
+
+xen_backend_init(xen_block_register_backend);
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 0c26023..fb227de 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -753,7 +753,6 @@  void xen_be_register_common(void)
 
     xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
-    xen_be_register("qdisk", &xen_blkdev_ops);
 #ifdef CONFIG_VIRTFS
     xen_be_register("9pfs", &xen_9pfs_ops);
 #endif
diff --git a/include/hw/xen/xen-block.h b/include/hw/xen/xen-block.h
index 37ed8a6..5bced60 100644
--- a/include/hw/xen/xen-block.h
+++ b/include/hw/xen/xen-block.h
@@ -37,6 +37,7 @@  typedef struct XenBlockDevice {
     unsigned int info;
     unsigned int max_ring_page_order;
     IOThread *iothread;
+    IOThread *auto_iothread;
     XenBlockDataPlane *dataplane;
 } XenBlockDevice;