Re: [Qemu-block] [PATCH 16/18] xen: automatically create XenQdiskDevice-s
> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: 04 December 2018 16:41
> To: Paul Durrant
> Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen-
> de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> ; Stefano Stabellini
> Subject: Re: [PATCH 16/18] xen: automatically create XenQdiskDevice-s
>
> On Wed, Nov 21, 2018 at 03:12:09PM +, Paul Durrant wrote:
> > This patch adds a creator function for XenQdiskDevice-s so that they can
> > be created automatically when the Xen toolstack instantiates a new
> > PV backend. When the XenQdiskDevice 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 XenQdiskDevice is destroyed. Also, for
> > compatibilitye with the legacy 'xen_disk' implementation, an iothread
> > is automatically created for the new XenQdiskDevice. This will also be
> > removed when he XenQdiskDevice is destroyed.
>
> "the XenQdiskDevice"
>
> [...]
> > +qemu_opt_set(drive_opts, "file.locking", "off", _err);
>
> That looks new, compared to the xen_disk.c implementation. Maybe that
> should be mention in the commit message.
>
It's necessary to avoid problems when an emulated device is also present. The
xen_disk code avoided the issue by basically bypassing the checks and stooging
into the middle of the block code. I'll add a comment to the code saying why
locking needs to be off.
>
> [..]
>
> > +static void xen_qdisk_device_create(BusState *bus, const char *name,
> > +QDict *opts, Error **errp)
> > +{
> [...]
> > +iothread = iothread_create(vdev, _abort);
>
> I would just propagate the error, since iothread could fail for external
> reason. No need to crash qemu while a VM is running.
True.
>
> > +
> > +dev = qdev_create(bus, TYPE_XEN_QDISK_DEVICE);
> > +
> > +qdev_prop_set_string(dev, "vdev", vdev);
> > +
> > +if (XEN_QDISK_DEVICE(dev)->vdev.number != number) {
> > +error_setg(errp, "invalid dev parameter '%s'", vdev);
> > +goto unref;
> > +}
> > +
> > +qdev_prop_set_drive(dev, "drive", blk, _err);
> > +if (local_err) {
> > +error_propagate(errp, local_err);
> > +error_prepend(errp, "failed to set 'drive': ");
> > +goto unref;
> > +}
> > +
> > +XEN_QDISK_DEVICE(dev)->auto_iothread = iothread;
> > +
> > +qdev_init_nofail(dev);
>
> That function shouldn't be use during hotplug. But I'm not sure what
> should be done instead, probably object_property_set_bool(..., true
> "realized") and check for error.
Ok, I'll do that.
Paul
>
>
> Thanks,
>
> --
> Anthony PERARD
Re: [Qemu-block] [PATCH 16/18] xen: automatically create XenQdiskDevice-s
On Wed, Nov 21, 2018 at 03:12:09PM +, Paul Durrant wrote:
> This patch adds a creator function for XenQdiskDevice-s so that they can
> be created automatically when the Xen toolstack instantiates a new
> PV backend. When the XenQdiskDevice 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 XenQdiskDevice is destroyed. Also, for
> compatibilitye with the legacy 'xen_disk' implementation, an iothread
> is automatically created for the new XenQdiskDevice. This will also be
> removed when he XenQdiskDevice is destroyed.
"the XenQdiskDevice"
[...]
> +qemu_opt_set(drive_opts, "file.locking", "off", _err);
That looks new, compared to the xen_disk.c implementation. Maybe that
should be mention in the commit message.
[..]
> +static void xen_qdisk_device_create(BusState *bus, const char *name,
> +QDict *opts, Error **errp)
> +{
[...]
> +iothread = iothread_create(vdev, _abort);
I would just propagate the error, since iothread could fail for external
reason. No need to crash qemu while a VM is running.
> +
> +dev = qdev_create(bus, TYPE_XEN_QDISK_DEVICE);
> +
> +qdev_prop_set_string(dev, "vdev", vdev);
> +
> +if (XEN_QDISK_DEVICE(dev)->vdev.number != number) {
> +error_setg(errp, "invalid dev parameter '%s'", vdev);
> +goto unref;
> +}
> +
> +qdev_prop_set_drive(dev, "drive", blk, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to set 'drive': ");
> +goto unref;
> +}
> +
> +XEN_QDISK_DEVICE(dev)->auto_iothread = iothread;
> +
> +qdev_init_nofail(dev);
That function shouldn't be use during hotplug. But I'm not sure what
should be done instead, probably object_property_set_bool(..., true
"realized") and check for error.
Thanks,
--
Anthony PERARD
[Qemu-block] [PATCH 16/18] xen: automatically create XenQdiskDevice-s
This patch adds a creator function for XenQdiskDevice-s so that they can
be created automatically when the Xen toolstack instantiates a new
PV backend. When the XenQdiskDevice 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 XenQdiskDevice is destroyed. Also, for
compatibilitye with the legacy 'xen_disk' implementation, an iothread
is automatically created for the new XenQdiskDevice. This will also be
removed when he XenQdiskDevice 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
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Stefano Stabellini
Cc: Anthony Perard
---
hw/block/trace-events | 1 +
hw/block/xen-qdisk.c| 237 +++-
hw/xen/xen-legacy-backend.c | 1 -
include/hw/xen/xen-qdisk.h | 1 +
4 files changed, 238 insertions(+), 2 deletions(-)
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 8b95567560..ee2ac756cf 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -133,3 +133,4 @@ xen_qdisk_realize(uint32_t disk, uint32_t partition)
"d%up%u"
xen_qdisk_connect(uint32_t disk, uint32_t partition) "d%up%u"
xen_qdisk_disconnect(uint32_t disk, uint32_t partition) "d%up%u"
xen_qdisk_unrealize(uint32_t disk, uint32_t partition) "d%up%u"
+xen_qdisk_device_create(const char *name) "name: %s"
diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
index 8c88393832..00cdd8181b 100644
--- a/hw/block/xen-qdisk.c
+++ b/hw/block/xen-qdisk.c
@@ -5,9 +5,12 @@
#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-backend.h"
#include "hw/xen/xen-qdisk.h"
#include "sysemu/blockdev.h"
#include "sysemu/block-backend.h"
@@ -28,6 +31,8 @@ static void xen_qdisk_realize(XenDevice *xendev, Error **errp)
XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
XenQdiskVdev *vdev = >vdev;
BlockConf *conf = >conf;
+IOThread *iothread = qdiskdev->auto_iothread ?
+qdiskdev->auto_iothread : qdiskdev->iothread;
DriveInfo *dinfo;
bool is_cdrom;
unsigned int info;
@@ -97,7 +102,7 @@ static void xen_qdisk_realize(XenDevice *xendev, Error
**errp)
size / conf->logical_block_size);
qdiskdev->dataplane = xen_qdisk_dataplane_create(xendev, conf,
- qdiskdev->iothread);
+ iothread);
}
static void xen_qdisk_connect(XenQdiskDevice *qdiskdev, Error **errp)
@@ -228,6 +233,11 @@ static void xen_qdisk_unrealize(XenDevice *xendev, Error
**errp)
xen_qdisk_dataplane_destroy(qdiskdev->dataplane);
qdiskdev->dataplane = NULL;
+
+if (qdiskdev->auto_iothread) {
+iothread_destroy(qdiskdev->auto_iothread);
+qdiskdev->auto_iothread = NULL;
+}
}
static char *disk_to_vbd_name(unsigned int disk)
@@ -461,3 +471,228 @@ static void xen_qdisk_register_types(void)
}
type_init(xen_qdisk_register_types)
+
+static void xen_qdisk_drive_create(const char *id, QDict *opts,
+ Error **errp)
+{
+const char *params, *device_type, *mode, *direct_io_safe,
+*discard_enable;
+char *format = NULL;
+char *file = NULL;
+char *drive_optstr = NULL;
+QemuOpts *drive_opts;
+Error *local_err = NULL;
+
+params = qdict_get_try_str(opts, "params");
+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, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+error_prepend(errp, "failed to set 'file': ");
+goto done;
+}
+
+if (format) {
+qemu_opt_set(drive_opts, "format", format, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+error_prepend(errp, "failed to set 'format': ");
+
