[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk'
From: |
Paul Durrant |
Subject: |
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk' |
Date: |
Tue, 4 Dec 2018 15:20:04 +0000 |
> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 29 November 2018 16:06
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; xen-
> address@hidden; Kevin Wolf <address@hidden>; Max Reitz
> <address@hidden>; Stefano Stabellini <address@hidden>
> 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
- Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk',
Paul Durrant <=