This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
replace the 'xen_disk' legacy PV backend but it is illustrative to build
up the implementation incrementally, along with the XenBus/XenDevice
framework. Subsequent patches will therefore add to this device's
implementation as new features are added to the framework.

After this patch has been applied it is possible to instantiate a new
'xen-qdisk' device with a single 'vdev' parameter, which accepts values
adhering to the Xen VBD naming scheme [2]. For example, a command-line
instantiation of a xen-qdisk can be done with an argument similar to the
following:

-device xen-qdisk,vdev=hda

The implementation of the vdev parameter formulates the appropriate VBD
number for use in the PV protocol.

[1] The name 'qdisk' as always been the name given to the QEMU
    implementation of the Xen PV block protocol backend implementation
[2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-vbd-interface.markdown.7

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 MAINTAINERS                |   2 +-
 hw/block/Makefile.objs     |   1 +
 hw/block/trace-events      |   4 +
 hw/block/xen-qdisk.c       | 256 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/xen/xen-qdisk.h |  38 +++++++
 5 files changed, 300 insertions(+), 1 deletion(-)
 create mode 100644 hw/block/xen-qdisk.c
 create mode 100644 include/hw/xen/xen-qdisk.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1032406c56..10f048a503 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -389,7 +389,7 @@ F: hw/9pfs/xen-9p-backend.c
 F: hw/char/xen_console.c
 F: hw/display/xenfb.c
 F: hw/net/xen_nic.c
-F: hw/block/xen_*
+F: hw/block/xen*
 F: hw/xen/
 F: hw/xenpv/
 F: hw/i386/xen/
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index 53ce5751ae..bcdd36d318 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_NAND) += nand.o
 common-obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o
 common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
 common-obj-$(CONFIG_XEN) += xen_disk.o
+common-obj-$(CONFIG_XEN) += xen-qdisk.o
 common-obj-$(CONFIG_ECC) += ecc.o
 common-obj-$(CONFIG_ONENAND) += onenand.o
 common-obj-$(CONFIG_NVME_PCI) += nvme.o
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 335c092450..fd3c126ac1 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -127,3 +127,7 @@ xen_disk_init(char *name) "%s"
 xen_disk_connect(char *name) "%s"
 xen_disk_disconnect(char *name) "%s"
 xen_disk_free(char *name) "%s"
+
+# hw/block/xen-qdisk.c
+xen_qdisk_realize(uint32_t disk, uint32_t partition) "d%up%u"
+xen_qdisk_unrealize(uint32_t disk, uint32_t partition) "d%up%u"
diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
new file mode 100644
index 0000000000..72122073f7
--- /dev/null
+++ b/hw/block/xen-qdisk.c
@@ -0,0 +1,256 @@
+/*
+ * Copyright (c) Citrix Systems Inc.
+ * All rights reserved.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "hw/hw.h"
+#include "hw/xen/xen-qdisk.h"
+#include "trace.h"
+
+static void xen_qdisk_realize(XenDevice *xendev, Error **errp)
+{
+    XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
+    XenQdiskVdev *vdev = &qdiskdev->vdev;
+
+    if (!vdev->valid) {
+        error_setg(errp, "vdev property not set");
+        return;
+    }
+
+    trace_xen_qdisk_realize(vdev->disk, vdev->partition);
+}
+
+static void xen_qdisk_unrealize(XenDevice *xendev, Error **errp)
+{
+    XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
+    XenQdiskVdev *vdev = &qdiskdev->vdev;
+
+    trace_xen_qdisk_unrealize(vdev->disk, vdev->partition);
+}
+
+static char *disk_to_vbd_name(unsigned int disk)
+{
+    unsigned int len = DIV_ROUND_UP(disk, 26);
+    char *name = g_malloc0(len + 1);
+
+    do {
+        name[len--] = 'a' + (disk % 26);
+        disk /= 26;
+    } while (disk != 0);
+    assert(len == 0);
+
+    return name;
+}
+
+static void xen_qdisk_get_vdev(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    XenQdiskVdev *vdev = qdev_get_prop_ptr(dev, prop);
+    char *str;
+
+    switch (vdev->type) {
+    case XEN_QDISK_VDEV_TYPE_DP:
+        str = g_strdup_printf("d%lup%lu", vdev->disk, vdev->partition);
+        break;
+
+    case XEN_QDISK_VDEV_TYPE_XVD:
+    case XEN_QDISK_VDEV_TYPE_HD:
+    case XEN_QDISK_VDEV_TYPE_SD: {
+        char *name = disk_to_vbd_name(vdev->disk);
+
+        str = g_strdup_printf("%s%s%lu",
+                              (vdev->type == XEN_QDISK_VDEV_TYPE_XVD) ?
+                              "xvd" :
+                              (vdev->type == XEN_QDISK_VDEV_TYPE_HD) ?
+                              "hd" :
+                              "sd",
+                              name, vdev->partition);
+        g_free(name);
+        break;
+    }
+    default:
+        error_setg(errp, "invalid vdev type");
+        return;
+    }
+
+    visit_type_str(v, name, &str, errp);
+    g_free(str);
+}
+
+static unsigned int vbd_name_to_disk(const char *name, const char **endp)
+{
+    unsigned int disk = 0;
+
+    while (*name != '\0') {
+        if (!g_ascii_isalpha(*name) || !g_ascii_islower(*name)) {
+            break;
+        }
+
+        disk *= 26;
+        disk += *name++ - 'a';
+    }
+    *endp = name;
+
+    return disk;
+}
+
+static void xen_qdisk_set_vdev(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    XenQdiskVdev *vdev = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    char *str, *p;
+    const char *end;
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_str(v, name, &str, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    p = strchr(str, 'd');
+    if (!p) {
+        goto invalid;
+    }
+
+    *p++ = '\0';
+    if (*str == '\0') {
+        vdev->type = XEN_QDISK_VDEV_TYPE_DP;
+    } else if (strcmp(str, "xv") == 0) {
+        vdev->type = XEN_QDISK_VDEV_TYPE_XVD;
+    } else if (strcmp(str, "h") == 0) {
+        vdev->type = XEN_QDISK_VDEV_TYPE_HD;
+    } else if (strcmp(str, "s") == 0) {
+        vdev->type = XEN_QDISK_VDEV_TYPE_SD;
+    } else {
+        goto invalid;
+    }
+
+    if (vdev->type == XEN_QDISK_VDEV_TYPE_DP) {
+        if (qemu_strtoul(p, &end, 10, &vdev->disk)) {
+            goto invalid;
+        }
+
+        if (*end == 'p') {
+            p = (char *) ++end;
+            if (*end == '\0') {
+                goto invalid;
+            }
+        }
+    } else {
+        vdev->disk = vbd_name_to_disk(p, &end);
+    }
+
+    if (*end != '\0') {
+        p = (char *)end;
+
+        if (qemu_strtoul(p, &end, 10, &vdev->partition)) {
+            goto invalid;
+        }
+
+        if (*end != '\0') {
+            goto invalid;
+        }
+    } else {
+        vdev->partition = 0;
+    }
+
+    switch (vdev->type) {
+    case XEN_QDISK_VDEV_TYPE_DP:
+    case XEN_QDISK_VDEV_TYPE_XVD:
+        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
+            vdev->number = (202 << 8) | (vdev->disk << 4) |
+                vdev->partition;
+        } else if (vdev->disk < (1 << 20) && vdev->partition < (1 << 8)) {
+            vdev->number = (1 << 28) | (vdev->disk << 8) |
+                vdev->partition;
+        } else {
+            goto invalid;
+        }
+        break;
+
+    case XEN_QDISK_VDEV_TYPE_HD:
+        if ((vdev->disk == 0 || vdev->disk == 1) &&
+            vdev->partition < (1 << 4)) {
+            vdev->number = (3 << 8) | (vdev->disk << 6) | vdev->partition;
+        } else if ((vdev->disk == 2 || vdev->disk == 3) &&
+                   vdev->partition < (1 << 4)) {
+            vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
+                vdev->partition;
+        } else {
+            goto invalid;
+        }
+        break;
+
+    case XEN_QDISK_VDEV_TYPE_SD:
+        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
+            vdev->number = (8 << 8) | (vdev->disk << 4) | vdev->partition;
+        } else {
+            goto invalid;
+        }
+        break;
+
+    default:
+        goto invalid;
+    }
+
+    g_free(str);
+    vdev->valid = true;
+    return;
+
+invalid:
+    error_setg(errp, "invalid virtual disk specifier");
+    g_free(str);
+}
+
+const PropertyInfo xen_qdisk_prop_vdev = {
+    .name  = "str",
+    .description = "Virtual Disk specifier: d*p*/xvd*/hd*/sd*",
+    .get = xen_qdisk_get_vdev,
+    .set = xen_qdisk_set_vdev,
+};
+
+static Property xen_qdisk_props[] = {
+    DEFINE_PROP("vdev", XenQdiskDevice, vdev,
+                xen_qdisk_prop_vdev, XenQdiskVdev),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void xen_qdisk_class_init(ObjectClass *class, void *data)
+{
+    DeviceClass *dev_class = DEVICE_CLASS(class);
+    XenDeviceClass *xendev_class = XEN_DEVICE_CLASS(class);
+
+    xendev_class->realize = xen_qdisk_realize;
+    xendev_class->unrealize = xen_qdisk_unrealize;
+
+    dev_class->desc = "Xen Qdisk Device";
+    dev_class->props = xen_qdisk_props;
+}
+
+static const TypeInfo xen_qdisk_type_info = {
+    .name = TYPE_XEN_QDISK_DEVICE,
+    .parent = TYPE_XEN_DEVICE,
+    .instance_size = sizeof(XenQdiskDevice),
+    .class_init = xen_qdisk_class_init,
+};
+
+static void xen_qdisk_register_types(void)
+{
+    type_register_static(&xen_qdisk_type_info);
+}
+
+type_init(xen_qdisk_register_types)
diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
new file mode 100644
index 0000000000..ade0866037
--- /dev/null
+++ b/include/hw/xen/xen-qdisk.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) Citrix Systems Inc.
+ * All rights reserved.
+ */
+
+#ifndef HW_XEN_QDISK_H
+#define HW_XEN_QDISK_H
+
+#include "hw/xen/xen-bus.h"
+
+typedef enum XenQdiskVdevType {
+    XEN_QDISK_VDEV_TYPE_DP,
+    XEN_QDISK_VDEV_TYPE_XVD,
+    XEN_QDISK_VDEV_TYPE_HD,
+    XEN_QDISK_VDEV_TYPE_SD,
+    XEN_QDISK_VDEV_TYPE__MAX
+} XenQdiskVdevType;
+
+typedef struct XenQdiskVdev {
+    XenQdiskVdevType type;
+    unsigned long disk;
+    unsigned long partition;
+    unsigned long number;
+    bool valid;
+} XenQdiskVdev;
+
+#define TYPE_XEN_QDISK_DEVICE  "xen-qdisk"
+#define XEN_QDISK_DEVICE(obj) \
+     OBJECT_CHECK(XenQdiskDevice, (obj), TYPE_XEN_QDISK_DEVICE)
+
+typedef struct XenQdiskDevice XenQdiskDevice;
+
+struct XenQdiskDevice {
+    XenDevice xendev;
+    XenQdiskVdev vdev;
+};
+
+#endif /* HW_XEN_QDISK_H */
-- 
2.11.0

On Wed, Nov 21, 2018 at 03:11:56PM +0000, Paul Durrant wrote:
> This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
> replace the 'xen_disk' legacy PV backend but it is illustrative to build
> up the implementation incrementally, along with the XenBus/XenDevice
> framework. Subsequent patches will therefore add to this device's
> implementation as new features are added to the framework.
> 
> After this patch has been applied it is possible to instantiate a new
> 'xen-qdisk' device with a single 'vdev' parameter, which accepts values
> adhering to the Xen VBD naming scheme [2]. For example, a command-line
> instantiation of a xen-qdisk can be done with an argument similar to the
> following:
> 
> -device xen-qdisk,vdev=hda

That works when QEMU boot, but doing the same thing once the guest have
booted, via QMP, doesn't. Here is the result (tested in qmp-shell):

(QEMU) device_add driver=xen-qdisk vdev=hda
{
    "error": {
        "class": "GenericError", 
        "desc": "Bus 'xen-bus.0' does not support hotplugging"
    }
}

That's probably why I've asked about the hotplug capability on the
previous patch.

> The implementation of the vdev parameter formulates the appropriate VBD
> number for use in the PV protocol.
> 
> [1] The name 'qdisk' as always been the name given to the QEMU
>     implementation of the Xen PV block protocol backend implementation
> [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-vbd-interface.markdown.7

Maybe a link to the generated docs would be better:
https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html

Also, it would be useful to have the same link in the source code.

> diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> new file mode 100644
> index 0000000000..72122073f7
> --- /dev/null
> +++ b/hw/block/xen-qdisk.c
[...]
> +static char *disk_to_vbd_name(unsigned int disk)
> +{
> +    unsigned int len = DIV_ROUND_UP(disk, 26);
> +    char *name = g_malloc0(len + 1);
> +
> +    do {
> +        name[len--] = 'a' + (disk % 26);
> +        disk /= 26;
> +    } while (disk != 0);
> +    assert(len == 0);
> +
> +    return name;
> +}

That function doesn't work.

For a simple xvdp, (so disk==15), it return "", I mean "\0p".

For a more complicated 'xvdbhwza', we have len == 22901. And the assert failed.

Maybe the recursing algo in libxl would be fine, with a buffer that is
big enough, and could probably be on the stack (in _get_vdev).

> +
> +    switch (vdev->type) {
> +    case XEN_QDISK_VDEV_TYPE_DP:
> +    case XEN_QDISK_VDEV_TYPE_XVD:
> +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> +            vdev->number = (202 << 8) | (vdev->disk << 4) |
> +                vdev->partition;
> +        } else if (vdev->disk < (1 << 20) && vdev->partition < (1 << 8)) {
> +            vdev->number = (1 << 28) | (vdev->disk << 8) |
> +                vdev->partition;
> +        } else {
> +            goto invalid;
> +        }
> +        break;
> +
> +    case XEN_QDISK_VDEV_TYPE_HD:
> +        if ((vdev->disk == 0 || vdev->disk == 1) &&
> +            vdev->partition < (1 << 4)) {

I think that should be:

    vdev->partition < (1 << 6)

Because hd disk have 0..63 partitions.

> +            vdev->number = (3 << 8) | (vdev->disk << 6) | vdev->partition;
> +        } else if ((vdev->disk == 2 || vdev->disk == 3) &&
> +                   vdev->partition < (1 << 4)) {

same here.

> +            vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
> +                vdev->partition;
> +        } else {
> +            goto invalid;
> +        }
> +        break;
> +
> +    case XEN_QDISK_VDEV_TYPE_SD:
> +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> +            vdev->number = (8 << 8) | (vdev->disk << 4) | vdev->partition;
> +        } else {
> +            goto invalid;
> +        }
> +        break;
> +
> +    default:
> +        goto invalid;
> +    }
> +
> +    g_free(str);
> +    vdev->valid = true;
> +    return;
> +
> +invalid:
> +    error_setg(errp, "invalid virtual disk specifier");
> +    g_free(str);

:(, g_free is called twice.

maybe we could have:
    vdev->valid=true;
    out:
      if (!vdev->valid)
        error_setg(...);
      g_free;

> diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> new file mode 100644
> index 0000000000..ade0866037
> --- /dev/null
> +++ b/include/hw/xen/xen-qdisk.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (c) Citrix Systems Inc.
> + * All rights reserved.
> + */
> +
> +#ifndef HW_XEN_QDISK_H
> +#define HW_XEN_QDISK_H
> +
> +#include "hw/xen/xen-bus.h"
> +
> +typedef enum XenQdiskVdevType {
> +    XEN_QDISK_VDEV_TYPE_DP,

Maybe we could set type_dp value to 1, so that, when vdev->type isn't
set, we can detect it later.


> +    XEN_QDISK_VDEV_TYPE_XVD,
> +    XEN_QDISK_VDEV_TYPE_HD,
> +    XEN_QDISK_VDEV_TYPE_SD,
> +    XEN_QDISK_VDEV_TYPE__MAX
> +} XenQdiskVdevType;

Thanks,

-- 
Anthony PERARD

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 29 November 2018 16:06
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> devel@lists.xenproject.org; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
> 
> On Wed, Nov 21, 2018 at 03:11:56PM +0000, Paul Durrant wrote:
> > This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
> > replace the 'xen_disk' legacy PV backend but it is illustrative to build
> > up the implementation incrementally, along with the XenBus/XenDevice
> > framework. Subsequent patches will therefore add to this device's
> > implementation as new features are added to the framework.
> >
> > After this patch has been applied it is possible to instantiate a new
> > 'xen-qdisk' device with a single 'vdev' parameter, which accepts values
> > adhering to the Xen VBD naming scheme [2]. For example, a command-line
> > instantiation of a xen-qdisk can be done with an argument similar to the
> > following:
> >
> > -device xen-qdisk,vdev=hda
> 
> That works when QEMU boot, but doing the same thing once the guest have
> booted, via QMP, doesn't. Here is the result (tested in qmp-shell):
> 
> (QEMU) device_add driver=xen-qdisk vdev=hda
> {
>     "error": {
>         "class": "GenericError",
>         "desc": "Bus 'xen-bus.0' does not support hotplugging"
>     }
> }
> 
> That's probably why I've asked about the hotplug capability on the
> previous patch.
> 

Ok. I've added the hotplug now so I'll make sure QMP DTRT.

> > The implementation of the vdev parameter formulates the appropriate VBD
> > number for use in the PV protocol.
> >
> > [1] The name 'qdisk' as always been the name given to the QEMU
> >     implementation of the Xen PV block protocol backend implementation
> > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-vbd-
> interface.markdown.7
> 
> Maybe a link to the generated docs would be better:
> https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html
> 

Ok.

> Also, it would be useful to have the same link in the source code.
> 

Yes, I'll add a comment.

> > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > new file mode 100644
> > index 0000000000..72122073f7
> > --- /dev/null
> > +++ b/hw/block/xen-qdisk.c
> [...]
> > +static char *disk_to_vbd_name(unsigned int disk)
> > +{
> > +    unsigned int len = DIV_ROUND_UP(disk, 26);
> > +    char *name = g_malloc0(len + 1);
> > +
> > +    do {
> > +        name[len--] = 'a' + (disk % 26);
> > +        disk /= 26;
> > +    } while (disk != 0);
> > +    assert(len == 0);
> > +
> > +    return name;
> > +}
> 
> That function doesn't work.
> 
> For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> 
> For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> failed.
> 
> Maybe the recursing algo in libxl would be fine, with a buffer that is
> big enough, and could probably be on the stack (in _get_vdev).

I used libxl__device_disk_dev_number() as my model (as well as cross-checking with the spec), but I guess a recursing algorithm would be neater.

> 
> > +
> > +    switch (vdev->type) {
> > +    case XEN_QDISK_VDEV_TYPE_DP:
> > +    case XEN_QDISK_VDEV_TYPE_XVD:
> > +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > +            vdev->number = (202 << 8) | (vdev->disk << 4) |
> > +                vdev->partition;
> > +        } else if (vdev->disk < (1 << 20) && vdev->partition < (1 <<
> 8)) {
> > +            vdev->number = (1 << 28) | (vdev->disk << 8) |
> > +                vdev->partition;
> > +        } else {
> > +            goto invalid;
> > +        }
> > +        break;
> > +
> > +    case XEN_QDISK_VDEV_TYPE_HD:
> > +        if ((vdev->disk == 0 || vdev->disk == 1) &&
> > +            vdev->partition < (1 << 4)) {
> 
> I think that should be:
> 
>     vdev->partition < (1 << 6)
> 
> Because hd disk have 0..63 partitions.

Yes, I must have typo-ed it...

> 
> > +            vdev->number = (3 << 8) | (vdev->disk << 6) | vdev-
> >partition;
> > +        } else if ((vdev->disk == 2 || vdev->disk == 3) &&
> > +                   vdev->partition < (1 << 4)) {
> 
> same here.

...and then cut'n'pasted.

> 
> > +            vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
> > +                vdev->partition;
> > +        } else {
> > +            goto invalid;
> > +        }
> > +        break;
> > +
> > +    case XEN_QDISK_VDEV_TYPE_SD:
> > +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > +            vdev->number = (8 << 8) | (vdev->disk << 4) | vdev-
> >partition;
> > +        } else {
> > +            goto invalid;
> > +        }
> > +        break;
> > +
> > +    default:
> > +        goto invalid;
> > +    }
> > +
> > +    g_free(str);
> > +    vdev->valid = true;
> > +    return;
> > +
> > +invalid:
> > +    error_setg(errp, "invalid virtual disk specifier");
> > +    g_free(str);
> 
> :(, g_free is called twice.

Oops. Good catch.

> 
> maybe we could have:
>     vdev->valid=true;
>     out:
>       if (!vdev->valid)
>         error_setg(...);
>       g_free;
> 
> > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > new file mode 100644
> > index 0000000000..ade0866037
> > --- /dev/null
> > +++ b/include/hw/xen/xen-qdisk.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Copyright (c) Citrix Systems Inc.
> > + * All rights reserved.
> > + */
> > +
> > +#ifndef HW_XEN_QDISK_H
> > +#define HW_XEN_QDISK_H
> > +
> > +#include "hw/xen/xen-bus.h"
> > +
> > +typedef enum XenQdiskVdevType {
> > +    XEN_QDISK_VDEV_TYPE_DP,
> 
> Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> set, we can detect it later.

Rather than having the 'valid' bool? Yes, that would work.

  Paul

> 
> 
> > +    XEN_QDISK_VDEV_TYPE_XVD,
> > +    XEN_QDISK_VDEV_TYPE_HD,
> > +    XEN_QDISK_VDEV_TYPE_SD,
> > +    XEN_QDISK_VDEV_TYPE__MAX
> > +} XenQdiskVdevType;
> 
> Thanks,
> 
> --
> Anthony PERARD

On Tue, Dec 04, 2018 at 03:20:04PM +0000, Paul Durrant wrote:
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +    unsigned int len = DIV_ROUND_UP(disk, 26);
> > > +    char *name = g_malloc0(len + 1);
> > > +
> > > +    do {
> > > +        name[len--] = 'a' + (disk % 26);
> > > +        disk /= 26;
> > > +    } while (disk != 0);
> > > +    assert(len == 0);
> > > +
> > > +    return name;
> > > +}
> > 
> > That function doesn't work.
> > 
> > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> > 
> > For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> > failed.
> > 
> > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > big enough, and could probably be on the stack (in _get_vdev).
> 
> I used libxl__device_disk_dev_number() as my model (as well as cross-checking with the spec), but I guess a recursing algorithm would be neater.

There is libxl__devid_to_vdev() actually just after ;-) which calls
encode_disk_name which is recursing.

> > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > new file mode 100644
> > > index 0000000000..ade0866037
> > > --- /dev/null
> > > +++ b/include/hw/xen/xen-qdisk.h
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > + * Copyright (c) Citrix Systems Inc.
> > > + * All rights reserved.
> > > + */
> > > +
> > > +#ifndef HW_XEN_QDISK_H
> > > +#define HW_XEN_QDISK_H
> > > +
> > > +#include "hw/xen/xen-bus.h"
> > > +
> > > +typedef enum XenQdiskVdevType {
> > > +    XEN_QDISK_VDEV_TYPE_DP,
> > 
> > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > set, we can detect it later.
> 
> Rather than having the 'valid' bool? Yes, that would work.

Well, the 'valid' bool doesn't seems to always be check so it would be
better. e.g. xen_qdisk_get_vdev() doesn't check `valid` before
genereting a string. Then xen_qdisk_set_vdev could set `type` to invalid
when it is invalid.

-- 
Anthony PERARD

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 04 December 2018 15:49
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> devel@lists.xenproject.org; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
> 
> On Tue, Dec 04, 2018 at 03:20:04PM +0000, Paul Durrant wrote:
> > > > +static char *disk_to_vbd_name(unsigned int disk)
> > > > +{
> > > > +    unsigned int len = DIV_ROUND_UP(disk, 26);
> > > > +    char *name = g_malloc0(len + 1);
> > > > +
> > > > +    do {
> > > > +        name[len--] = 'a' + (disk % 26);
> > > > +        disk /= 26;
> > > > +    } while (disk != 0);
> > > > +    assert(len == 0);
> > > > +
> > > > +    return name;
> > > > +}
> > >
> > > That function doesn't work.
> > >
> > > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> > >
> > > For a more complicated 'xvdbhwza', we have len == 22901. And the
> assert
> > > failed.
> > >
> > > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > > big enough, and could probably be on the stack (in _get_vdev).
> >
> > I used libxl__device_disk_dev_number() as my model (as well as cross-
> checking with the spec), but I guess a recursing algorithm would be
> neater.
> 
> There is libxl__devid_to_vdev() actually just after ;-) which calls
> encode_disk_name which is recursing.

Ah, I'll look for that. Currently having trouble reconciling the 'tq' -> 536 mapping in the doc.

  Paul

> 
> > > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > > new file mode 100644
> > > > index 0000000000..ade0866037
> > > > --- /dev/null
> > > > +++ b/include/hw/xen/xen-qdisk.h
> > > > @@ -0,0 +1,38 @@
> > > > +/*
> > > > + * Copyright (c) Citrix Systems Inc.
> > > > + * All rights reserved.
> > > > + */
> > > > +
> > > > +#ifndef HW_XEN_QDISK_H
> > > > +#define HW_XEN_QDISK_H
> > > > +
> > > > +#include "hw/xen/xen-bus.h"
> > > > +
> > > > +typedef enum XenQdiskVdevType {
> > > > +    XEN_QDISK_VDEV_TYPE_DP,
> > >
> > > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > > set, we can detect it later.
> >
> > Rather than having the 'valid' bool? Yes, that would work.
> 
> Well, the 'valid' bool doesn't seems to always be check so it would be
> better. e.g. xen_qdisk_get_vdev() doesn't check `valid` before
> genereting a string. Then xen_qdisk_set_vdev could set `type` to invalid
> when it is invalid.
> 
> --
> Anthony PERARD

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+paul.durrant=citrix.com@nongnu.org] On Behalf Of Paul Durrant
> Sent: 04 December 2018 15:20
> To: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>; Stefano Stabellini
> <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu-devel@nongnu.org;
> Max Reitz <mreitz@redhat.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Qemu-devel] [PATCH 03/18] xen: introduce 'xen-qdisk'
> 
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 29 November 2018 16:06
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> > devel@lists.xenproject.org; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
> >
> > On Wed, Nov 21, 2018 at 03:11:56PM +0000, Paul Durrant wrote:
> > > This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
> > > replace the 'xen_disk' legacy PV backend but it is illustrative to
> build
> > > up the implementation incrementally, along with the XenBus/XenDevice
> > > framework. Subsequent patches will therefore add to this device's
> > > implementation as new features are added to the framework.
> > >
> > > After this patch has been applied it is possible to instantiate a new
> > > 'xen-qdisk' device with a single 'vdev' parameter, which accepts
> values
> > > adhering to the Xen VBD naming scheme [2]. For example, a command-line
> > > instantiation of a xen-qdisk can be done with an argument similar to
> the
> > > following:
> > >
> > > -device xen-qdisk,vdev=hda
> >
> > That works when QEMU boot, but doing the same thing once the guest have
> > booted, via QMP, doesn't. Here is the result (tested in qmp-shell):
> >
> > (QEMU) device_add driver=xen-qdisk vdev=hda
> > {
> >     "error": {
> >         "class": "GenericError",
> >         "desc": "Bus 'xen-bus.0' does not support hotplugging"
> >     }
> > }
> >
> > That's probably why I've asked about the hotplug capability on the
> > previous patch.
> >
> 
> Ok. I've added the hotplug now so I'll make sure QMP DTRT.
> 
> > > The implementation of the vdev parameter formulates the appropriate
> VBD
> > > number for use in the PV protocol.
> > >
> > > [1] The name 'qdisk' as always been the name given to the QEMU
> > >     implementation of the Xen PV block protocol backend implementation
> > > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-
> vbd-
> > interface.markdown.7
> >
> > Maybe a link to the generated docs would be better:
> > https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html
> >
> 
> Ok.
> 
> > Also, it would be useful to have the same link in the source code.
> >
> 
> Yes, I'll add a comment.
> 
> > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > new file mode 100644
> > > index 0000000000..72122073f7
> > > --- /dev/null
> > > +++ b/hw/block/xen-qdisk.c
> > [...]
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +    unsigned int len = DIV_ROUND_UP(disk, 26);
> > > +    char *name = g_malloc0(len + 1);
> > > +
> > > +    do {
> > > +        name[len--] = 'a' + (disk % 26);
> > > +        disk /= 26;
> > > +    } while (disk != 0);
> > > +    assert(len == 0);
> > > +
> > > +    return name;
> > > +}
> >
> > That function doesn't work.
> >
> > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> >
> > For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> > failed.
> >
> > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > big enough, and could probably be on the stack (in _get_vdev).
> 
> I used libxl__device_disk_dev_number() as my model (as well as cross-
> checking with the spec), but I guess a recursing algorithm would be
> neater.
> 
> >
> > > +
> > > +    switch (vdev->type) {
> > > +    case XEN_QDISK_VDEV_TYPE_DP:
> > > +    case XEN_QDISK_VDEV_TYPE_XVD:
> > > +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > > +            vdev->number = (202 << 8) | (vdev->disk << 4) |
> > > +                vdev->partition;
> > > +        } else if (vdev->disk < (1 << 20) && vdev->partition < (1 <<
> > 8)) {
> > > +            vdev->number = (1 << 28) | (vdev->disk << 8) |
> > > +                vdev->partition;
> > > +        } else {
> > > +            goto invalid;
> > > +        }
> > > +        break;
> > > +
> > > +    case XEN_QDISK_VDEV_TYPE_HD:
> > > +        if ((vdev->disk == 0 || vdev->disk == 1) &&
> > > +            vdev->partition < (1 << 4)) {
> >
> > I think that should be:
> >
> >     vdev->partition < (1 << 6)
> >
> > Because hd disk have 0..63 partitions.
> 
> Yes, I must have typo-ed it...
> 
> >
> > > +            vdev->number = (3 << 8) | (vdev->disk << 6) | vdev-
> > >partition;
> > > +        } else if ((vdev->disk == 2 || vdev->disk == 3) &&
> > > +                   vdev->partition < (1 << 4)) {
> >
> > same here.
> 
> ...and then cut'n'pasted.
> 
> >
> > > +            vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
> > > +                vdev->partition;
> > > +        } else {
> > > +            goto invalid;
> > > +        }
> > > +        break;
> > > +
> > > +    case XEN_QDISK_VDEV_TYPE_SD:
> > > +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > > +            vdev->number = (8 << 8) | (vdev->disk << 4) | vdev-
> > >partition;
> > > +        } else {
> > > +            goto invalid;
> > > +        }
> > > +        break;
> > > +
> > > +    default:
> > > +        goto invalid;
> > > +    }
> > > +
> > > +    g_free(str);
> > > +    vdev->valid = true;
> > > +    return;
> > > +
> > > +invalid:
> > > +    error_setg(errp, "invalid virtual disk specifier");
> > > +    g_free(str);
> >
> > :(, g_free is called twice.
> 
> Oops. Good catch.

Oh, I thought you'd found a double free...

> 
> >
> > maybe we could have:
> >     vdev->valid=true;
> >     out:
> >       if (!vdev->valid)
> >         error_setg(...);
> >       g_free;

No, that's quite convoluted. I prefer separate 'fail' and 'success' paths, even if they both need to call g_free().

  Paul

> >
> > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > new file mode 100644
> > > index 0000000000..ade0866037
> > > --- /dev/null
> > > +++ b/include/hw/xen/xen-qdisk.h
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > + * Copyright (c) Citrix Systems Inc.
> > > + * All rights reserved.
> > > + */
> > > +
> > > +#ifndef HW_XEN_QDISK_H
> > > +#define HW_XEN_QDISK_H
> > > +
> > > +#include "hw/xen/xen-bus.h"
> > > +
> > > +typedef enum XenQdiskVdevType {
> > > +    XEN_QDISK_VDEV_TYPE_DP,
> >
> > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > set, we can detect it later.
> 
> Rather than having the 'valid' bool? Yes, that would work.
> 
>   Paul
> 
> >
> >
> > > +    XEN_QDISK_VDEV_TYPE_XVD,
> > > +    XEN_QDISK_VDEV_TYPE_HD,
> > > +    XEN_QDISK_VDEV_TYPE_SD,
> > > +    XEN_QDISK_VDEV_TYPE__MAX
> > > +} XenQdiskVdevType;
> >
> > Thanks,
> >
> > --
> > Anthony PERARD