| Message ID | 20181121151211.15997-4-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Xen PV backend 'qdevification' | expand |
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,
> -----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.
> -----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
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 */
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