[14/18] xen: add implementations of xen-qdisk connect and disconnect functions...

Commit Message

Paul Durrant Nov. 21, 2018, 3:12 p.m. UTC

...and wire in the dataplane.

This patch adds the remaining code to make the xen-qdisk XenDevice
functional. The parameters that a block frontend expects to find are
populated in the backend xenstore area, and the 'ring-ref' and
'event-channel' values specified in the frontend xenstore area are
mapped/bound and used to set up the dataplane.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Stefano Stabellini <[e-mail]>
Cc: Anthony Perard <[e-mail]>
Cc: Kevin Wolf <[e-mail]>
Cc: Max Reitz <[e-mail]>
---
 hw/block/xen-qdisk.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++
 hw/xen/xen-bus.c           |  12 ++--
 include/hw/xen/xen-bus.h   |   8 +++
 include/hw/xen/xen-qdisk.h |  12 ++++
 4 files changed, 166 insertions(+), 6 deletions(-)

Comments

Kevin Wolf Nov. 28, 2018, 4:34 p.m. UTC | #1

Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> ...and wire in the dataplane.
> 
> This patch adds the remaining code to make the xen-qdisk XenDevice
> functional. The parameters that a block frontend expects to find are
> populated in the backend xenstore area, and the 'ring-ref' and
> 'event-channel' values specified in the frontend xenstore area are
> mapped/bound and used to set up the dataplane.
> 
> Signed-off-by: Paul Durrant <[e-mail]>
> ---
> Cc: Stefano Stabellini <[e-mail]>
> Cc: Anthony Perard <[e-mail]>
> Cc: Kevin Wolf <[e-mail]>
> Cc: Max Reitz <[e-mail]>
> ---
>  hw/block/xen-qdisk.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/xen/xen-bus.c           |  12 ++--
>  include/hw/xen/xen-bus.h   |   8 +++
>  include/hw/xen/xen-qdisk.h |  12 ++++
>  4 files changed, 166 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> index 35f7b70480..8c88393832 100644
> --- a/hw/block/xen-qdisk.c
> +++ b/hw/block/xen-qdisk.c
> @@ -9,6 +9,10 @@
>  #include "qapi/visitor.h"
>  #include "hw/hw.h"
>  #include "hw/xen/xen-qdisk.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
> +#include "dataplane/xen-qdisk.h"
>  #include "trace.h"
>  
>  static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev, Error **errp)
>  {
>      XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
>      XenQdiskVdev *vdev = &qdiskdev->vdev;
> +    BlockConf *conf = &qdiskdev->conf;
> +    DriveInfo *dinfo;
> +    bool is_cdrom;
> +    unsigned int info;
> +    int64_t size;
>  
>      if (!vdev->valid) {
>          error_setg(errp, "vdev property not set");
> @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice *xendev, Error **errp)
>      }
>  
>      trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> +
> +    if (!conf->blk) {
> +        error_setg(errp, "drive property not set");
> +        return;
> +    }
> +
> +    if (!blk_is_inserted(conf->blk)) {
> +        error_setg(errp, "device needs media, but drive is empty");
> +        return;
> +    }

Hm, the code below suggests that you support CD-ROMs. Don't you want to
support media change as well then? Which would mean that you need to
support empty drives.

> +    if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf->blk),
> +                                       false, errp)) {
> +        return;
> +    }
> +
> +    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> +        return;
> +    }
> +
> +    dinfo = blk_legacy_dinfo(conf->blk);
> +    is_cdrom = (dinfo && dinfo->media_cd);

It's called legacy for a reason. Don't use this in new devices.

The proper way is to have two different devices for hard disks and CDs
(like scsi-hd and scsi-cd).

Kevin

Paul Durrant Nov. 28, 2018, 4:40 p.m. UTC | #2

> -----Original Message-----
> From: Kevin Wolf [mailto:[e-mail]]
> Sent: 28 November 2018 16:35
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; [e-mail]; xen-
> [e-mail]; Stefano Stabellini <[e-mail]>;
> Anthony Perard <[e-mail]>; Max Reitz <[e-mail]>
> Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> and disconnect functions...
> 
> Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > ...and wire in the dataplane.
> >
> > This patch adds the remaining code to make the xen-qdisk XenDevice
> > functional. The parameters that a block frontend expects to find are
> > populated in the backend xenstore area, and the 'ring-ref' and
> > 'event-channel' values specified in the frontend xenstore area are
> > mapped/bound and used to set up the dataplane.
> >
> > Signed-off-by: Paul Durrant <[e-mail]>
> > ---
> > Cc: Stefano Stabellini <[e-mail]>
> > Cc: Anthony Perard <[e-mail]>
> > Cc: Kevin Wolf <[e-mail]>
> > Cc: Max Reitz <[e-mail]>
> > ---
> >  hw/block/xen-qdisk.c       | 140
> +++++++++++++++++++++++++++++++++++++++++++++
> >  hw/xen/xen-bus.c           |  12 ++--
> >  include/hw/xen/xen-bus.h   |   8 +++
> >  include/hw/xen/xen-qdisk.h |  12 ++++
> >  4 files changed, 166 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > index 35f7b70480..8c88393832 100644
> > --- a/hw/block/xen-qdisk.c
> > +++ b/hw/block/xen-qdisk.c
> > @@ -9,6 +9,10 @@
> >  #include "qapi/visitor.h"
> >  #include "hw/hw.h"
> >  #include "hw/xen/xen-qdisk.h"
> > +#include "sysemu/blockdev.h"
> > +#include "sysemu/block-backend.h"
> > +#include "sysemu/iothread.h"
> > +#include "dataplane/xen-qdisk.h"
> >  #include "trace.h"
> >
> >  static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev,
> Error **errp)
> >  {
> >      XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> >      XenQdiskVdev *vdev = &qdiskdev->vdev;
> > +    BlockConf *conf = &qdiskdev->conf;
> > +    DriveInfo *dinfo;
> > +    bool is_cdrom;
> > +    unsigned int info;
> > +    int64_t size;
> >
> >      if (!vdev->valid) {
> >          error_setg(errp, "vdev property not set");
> > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice *xendev,
> Error **errp)
> >      }
> >
> >      trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > +
> > +    if (!conf->blk) {
> > +        error_setg(errp, "drive property not set");
> > +        return;
> > +    }
> > +
> > +    if (!blk_is_inserted(conf->blk)) {
> > +        error_setg(errp, "device needs media, but drive is empty");
> > +        return;
> > +    }
> 
> Hm, the code below suggests that you support CD-ROMs. Don't you want to
> support media change as well then? Which would mean that you need to
> support empty drives.

Yes, that's a good point. I should get rid of that check.

> 
> > +    if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf-
> >blk),
> > +                                       false, errp)) {
> > +        return;
> > +    }
> > +
> > +    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > +        return;
> > +    }
> > +
> > +    dinfo = blk_legacy_dinfo(conf->blk);
> > +    is_cdrom = (dinfo && dinfo->media_cd);
> 
> It's called legacy for a reason. Don't use this in new devices.
> 
> The proper way is to have two different devices for hard disks and CDs
> (like scsi-hd and scsi-cd).

...or presumably I could have a property? The legacy init code could then set it based on the drive info.

  Paul

> 
> Kevin

Kevin Wolf Nov. 29, 2018, 9 a.m. UTC | #3

Am 28.11.2018 um 17:40 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Kevin Wolf [mailto:[e-mail]]
> > Sent: 28 November 2018 16:35
> > To: Paul Durrant <[e-mail]>
> > Cc: [e-mail]; [e-mail]; xen-
> > [e-mail]; Stefano Stabellini <[e-mail]>;
> > Anthony Perard <[e-mail]>; Max Reitz <[e-mail]>
> > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> > and disconnect functions...
> > 
> > Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > > ...and wire in the dataplane.
> > >
> > > This patch adds the remaining code to make the xen-qdisk XenDevice
> > > functional. The parameters that a block frontend expects to find are
> > > populated in the backend xenstore area, and the 'ring-ref' and
> > > 'event-channel' values specified in the frontend xenstore area are
> > > mapped/bound and used to set up the dataplane.
> > >
> > > Signed-off-by: Paul Durrant <[e-mail]>
> > > ---
> > > Cc: Stefano Stabellini <[e-mail]>
> > > Cc: Anthony Perard <[e-mail]>
> > > Cc: Kevin Wolf <[e-mail]>
> > > Cc: Max Reitz <[e-mail]>
> > > ---
> > >  hw/block/xen-qdisk.c       | 140
> > +++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/xen/xen-bus.c           |  12 ++--
> > >  include/hw/xen/xen-bus.h   |   8 +++
> > >  include/hw/xen/xen-qdisk.h |  12 ++++
> > >  4 files changed, 166 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > index 35f7b70480..8c88393832 100644
> > > --- a/hw/block/xen-qdisk.c
> > > +++ b/hw/block/xen-qdisk.c
> > > @@ -9,6 +9,10 @@
> > >  #include "qapi/visitor.h"
> > >  #include "hw/hw.h"
> > >  #include "hw/xen/xen-qdisk.h"
> > > +#include "sysemu/blockdev.h"
> > > +#include "sysemu/block-backend.h"
> > > +#include "sysemu/iothread.h"
> > > +#include "dataplane/xen-qdisk.h"
> > >  #include "trace.h"
> > >
> > >  static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> > > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev,
> > Error **errp)
> > >  {
> > >      XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> > >      XenQdiskVdev *vdev = &qdiskdev->vdev;
> > > +    BlockConf *conf = &qdiskdev->conf;
> > > +    DriveInfo *dinfo;
> > > +    bool is_cdrom;
> > > +    unsigned int info;
> > > +    int64_t size;
> > >
> > >      if (!vdev->valid) {
> > >          error_setg(errp, "vdev property not set");
> > > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice *xendev,
> > Error **errp)
> > >      }
> > >
> > >      trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > > +
> > > +    if (!conf->blk) {
> > > +        error_setg(errp, "drive property not set");
> > > +        return;
> > > +    }
> > > +
> > > +    if (!blk_is_inserted(conf->blk)) {
> > > +        error_setg(errp, "device needs media, but drive is empty");
> > > +        return;
> > > +    }
> > 
> > Hm, the code below suggests that you support CD-ROMs. Don't you want to
> > support media change as well then? Which would mean that you need to
> > support empty drives.
> 
> Yes, that's a good point. I should get rid of that check.

Or rather apply it only to hard disks. And for empty CDs, you'll
probably need to create an empty BlockBackend (the !conf->blk case).
Just check the IDE and/or SCSI code for comparison.

> > 
> > > +    if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf-
> > >blk),
> > > +                                       false, errp)) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > > +        return;
> > > +    }
> > > +
> > > +    dinfo = blk_legacy_dinfo(conf->blk);
> > > +    is_cdrom = (dinfo && dinfo->media_cd);
> > 
> > It's called legacy for a reason. Don't use this in new devices.
> > 
> > The proper way is to have two different devices for hard disks and CDs
> > (like scsi-hd and scsi-cd).
> 
> ...or presumably I could have a property? The legacy init code could
> then set it based on the drive info.

Technically yes, but why would that be a good way to model things? I
mean, it's true that xen-qdisk is not real hardware, but I've never seen
any hardware that has a switch to decide whether it should behave as a
CD drive or a hard disk.

Both have very different characteristics (read-only with removable
media, or a single read-write disk), and the existing implementations
use two separate devices. So even if you're not convinced that users
will consider them different concepts (I am; and if they weren't
different concepts, you wouldn't need an is_cdrom variable), consistency
is still a good thing.

Kevin

Paul Durrant Nov. 29, 2018, 9:33 a.m. UTC | #4

> -----Original Message-----
> From: Kevin Wolf [mailto:[e-mail]]
> Sent: 29 November 2018 09:01
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; [e-mail]; xen-
> [e-mail]; Stefano Stabellini <[e-mail]>;
> Anthony Perard <[e-mail]>; Max Reitz <[e-mail]>
> Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> and disconnect functions...
> 
> Am 28.11.2018 um 17:40 hat Paul Durrant geschrieben:
> > > -----Original Message-----
> > > From: Kevin Wolf [mailto:[e-mail]]
> > > Sent: 28 November 2018 16:35
> > > To: Paul Durrant <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; xen-
> > > [e-mail]; Stefano Stabellini
> <[e-mail]>;
> > > Anthony Perard <[e-mail]>; Max Reitz
> <[e-mail]>
> > > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk
> connect
> > > and disconnect functions...
> > >
> > > Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > > > ...and wire in the dataplane.
> > > >
> > > > This patch adds the remaining code to make the xen-qdisk XenDevice
> > > > functional. The parameters that a block frontend expects to find are
> > > > populated in the backend xenstore area, and the 'ring-ref' and
> > > > 'event-channel' values specified in the frontend xenstore area are
> > > > mapped/bound and used to set up the dataplane.
> > > >
> > > > Signed-off-by: Paul Durrant <[e-mail]>
> > > > ---
> > > > Cc: Stefano Stabellini <[e-mail]>
> > > > Cc: Anthony Perard <[e-mail]>
> > > > Cc: Kevin Wolf <[e-mail]>
> > > > Cc: Max Reitz <[e-mail]>
> > > > ---
> > > >  hw/block/xen-qdisk.c       | 140
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/xen/xen-bus.c           |  12 ++--
> > > >  include/hw/xen/xen-bus.h   |   8 +++
> > > >  include/hw/xen/xen-qdisk.h |  12 ++++
> > > >  4 files changed, 166 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > > index 35f7b70480..8c88393832 100644
> > > > --- a/hw/block/xen-qdisk.c
> > > > +++ b/hw/block/xen-qdisk.c
> > > > @@ -9,6 +9,10 @@
> > > >  #include "qapi/visitor.h"
> > > >  #include "hw/hw.h"
> > > >  #include "hw/xen/xen-qdisk.h"
> > > > +#include "sysemu/blockdev.h"
> > > > +#include "sysemu/block-backend.h"
> > > > +#include "sysemu/iothread.h"
> > > > +#include "dataplane/xen-qdisk.h"
> > > >  #include "trace.h"
> > > >
> > > >  static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> > > > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev,
> > > Error **errp)
> > > >  {
> > > >      XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> > > >      XenQdiskVdev *vdev = &qdiskdev->vdev;
> > > > +    BlockConf *conf = &qdiskdev->conf;
> > > > +    DriveInfo *dinfo;
> > > > +    bool is_cdrom;
> > > > +    unsigned int info;
> > > > +    int64_t size;
> > > >
> > > >      if (!vdev->valid) {
> > > >          error_setg(errp, "vdev property not set");
> > > > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice
> *xendev,
> > > Error **errp)
> > > >      }
> > > >
> > > >      trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > > > +
> > > > +    if (!conf->blk) {
> > > > +        error_setg(errp, "drive property not set");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!blk_is_inserted(conf->blk)) {
> > > > +        error_setg(errp, "device needs media, but drive is empty");
> > > > +        return;
> > > > +    }
> > >
> > > Hm, the code below suggests that you support CD-ROMs. Don't you want
> to
> > > support media change as well then? Which would mean that you need to
> > > support empty drives.
> >
> > Yes, that's a good point. I should get rid of that check.
> 
> Or rather apply it only to hard disks. And for empty CDs, you'll
> probably need to create an empty BlockBackend (the !conf->blk case).
> Just check the IDE and/or SCSI code for comparison.
> 
> > >
> > > > +    if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf-
> > > >blk),
> > > > +                                       false, errp)) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    dinfo = blk_legacy_dinfo(conf->blk);
> > > > +    is_cdrom = (dinfo && dinfo->media_cd);
> > >
> > > It's called legacy for a reason. Don't use this in new devices.
> > >
> > > The proper way is to have two different devices for hard disks and CDs
> > > (like scsi-hd and scsi-cd).
> >
> > ...or presumably I could have a property? The legacy init code could
> > then set it based on the drive info.
> 
> Technically yes, but why would that be a good way to model things? I
> mean, it's true that xen-qdisk is not real hardware, but I've never seen
> any hardware that has a switch to decide whether it should behave as a
> CD drive or a hard disk.
> 
> Both have very different characteristics (read-only with removable
> media, or a single read-write disk), and the existing implementations
> use two separate devices. So even if you're not convinced that users
> will consider them different concepts (I am; and if they weren't
> different concepts, you wouldn't need an is_cdrom variable), consistency
> is still a good thing.

Ok. I'll split the device as you suggest... it may mean duplicated code, but the datapath can still be common.

  Paul

> 
> Kevin

Kevin Wolf Nov. 29, 2018, 10:46 a.m. UTC | #5

Am 29.11.2018 um 10:33 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Kevin Wolf [mailto:[e-mail]]
> > Sent: 29 November 2018 09:01
> > To: Paul Durrant <[e-mail]>
> > Cc: [e-mail]; [e-mail]; xen-
> > [e-mail]; Stefano Stabellini <[e-mail]>;
> > Anthony Perard <[e-mail]>; Max Reitz <[e-mail]>
> > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> > and disconnect functions...
> > 
> > Am 28.11.2018 um 17:40 hat Paul Durrant geschrieben:
> > > > -----Original Message-----
> > > > From: Kevin Wolf [mailto:[e-mail]]
> > > > Sent: 28 November 2018 16:35
> > > > To: Paul Durrant <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]; xen-
> > > > [e-mail]; Stefano Stabellini
> > <[e-mail]>;
> > > > Anthony Perard <[e-mail]>; Max Reitz
> > <[e-mail]>
> > > > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk
> > connect
> > > > and disconnect functions...
> > > >
> > > > Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > > > > ...and wire in the dataplane.
> > > > >
> > > > > This patch adds the remaining code to make the xen-qdisk XenDevice
> > > > > functional. The parameters that a block frontend expects to find are
> > > > > populated in the backend xenstore area, and the 'ring-ref' and
> > > > > 'event-channel' values specified in the frontend xenstore area are
> > > > > mapped/bound and used to set up the dataplane.
> > > > >
> > > > > Signed-off-by: Paul Durrant <[e-mail]>
> > > > > ---
> > > > > Cc: Stefano Stabellini <[e-mail]>
> > > > > Cc: Anthony Perard <[e-mail]>
> > > > > Cc: Kevin Wolf <[e-mail]>
> > > > > Cc: Max Reitz <[e-mail]>
> > > > > ---
> > > > >  hw/block/xen-qdisk.c       | 140
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  hw/xen/xen-bus.c           |  12 ++--
> > > > >  include/hw/xen/xen-bus.h   |   8 +++
> > > > >  include/hw/xen/xen-qdisk.h |  12 ++++
> > > > >  4 files changed, 166 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > > > index 35f7b70480..8c88393832 100644
> > > > > --- a/hw/block/xen-qdisk.c
> > > > > +++ b/hw/block/xen-qdisk.c
> > > > > @@ -9,6 +9,10 @@
> > > > >  #include "qapi/visitor.h"
> > > > >  #include "hw/hw.h"
> > > > >  #include "hw/xen/xen-qdisk.h"
> > > > > +#include "sysemu/blockdev.h"
> > > > > +#include "sysemu/block-backend.h"
> > > > > +#include "sysemu/iothread.h"
> > > > > +#include "dataplane/xen-qdisk.h"
> > > > >  #include "trace.h"
> > > > >
> > > > >  static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> > > > > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev,
> > > > Error **errp)
> > > > >  {
> > > > >      XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> > > > >      XenQdiskVdev *vdev = &qdiskdev->vdev;
> > > > > +    BlockConf *conf = &qdiskdev->conf;
> > > > > +    DriveInfo *dinfo;
> > > > > +    bool is_cdrom;
> > > > > +    unsigned int info;
> > > > > +    int64_t size;
> > > > >
> > > > >      if (!vdev->valid) {
> > > > >          error_setg(errp, "vdev property not set");
> > > > > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice
> > *xendev,
> > > > Error **errp)
> > > > >      }
> > > > >
> > > > >      trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > > > > +
> > > > > +    if (!conf->blk) {
> > > > > +        error_setg(errp, "drive property not set");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    if (!blk_is_inserted(conf->blk)) {
> > > > > +        error_setg(errp, "device needs media, but drive is empty");
> > > > > +        return;
> > > > > +    }
> > > >
> > > > Hm, the code below suggests that you support CD-ROMs. Don't you want
> > to
> > > > support media change as well then? Which would mean that you need to
> > > > support empty drives.
> > >
> > > Yes, that's a good point. I should get rid of that check.
> > 
> > Or rather apply it only to hard disks. And for empty CDs, you'll
> > probably need to create an empty BlockBackend (the !conf->blk case).
> > Just check the IDE and/or SCSI code for comparison.
> > 
> > > >
> > > > > +    if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf-
> > > > >blk),
> > > > > +                                       false, errp)) {
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    dinfo = blk_legacy_dinfo(conf->blk);
> > > > > +    is_cdrom = (dinfo && dinfo->media_cd);
> > > >
> > > > It's called legacy for a reason. Don't use this in new devices.
> > > >
> > > > The proper way is to have two different devices for hard disks and CDs
> > > > (like scsi-hd and scsi-cd).
> > >
> > > ...or presumably I could have a property? The legacy init code could
> > > then set it based on the drive info.
> > 
> > Technically yes, but why would that be a good way to model things? I
> > mean, it's true that xen-qdisk is not real hardware, but I've never seen
> > any hardware that has a switch to decide whether it should behave as a
> > CD drive or a hard disk.
> > 
> > Both have very different characteristics (read-only with removable
> > media, or a single read-write disk), and the existing implementations
> > use two separate devices. So even if you're not convinced that users
> > will consider them different concepts (I am; and if they weren't
> > different concepts, you wouldn't need an is_cdrom variable), consistency
> > is still a good thing.
> 
> Ok. I'll split the device as you suggest... it may mean duplicated
> code, but the datapath can still be common.

If you have a look at IDE and SCSI, they don't really duplicate a lot of
code. Basically it's just a second QOM class definition, the rest is
shared. Even the realize functions are essentially shared, with just two
small wrappers for each device type around the common code.

Kevin

Paul Durrant Nov. 29, 2018, 10:47 a.m. UTC | #6

> -----Original Message-----
> From: Kevin Wolf [mailto:[e-mail]]
> Sent: 29 November 2018 10:46
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; [e-mail]; xen-
> [e-mail]; Stefano Stabellini <[e-mail]>;
> Anthony Perard <[e-mail]>; Max Reitz <[e-mail]>
> Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> and disconnect functions...
> 
> Am 29.11.2018 um 10:33 hat Paul Durrant geschrieben:
> > > -----Original Message-----
> > > From: Kevin Wolf [mailto:[e-mail]]
> > > Sent: 29 November 2018 09:01
> > > To: Paul Durrant <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; xen-
> > > [e-mail]; Stefano Stabellini
> <[e-mail]>;
> > > Anthony Perard <[e-mail]>; Max Reitz
> <[e-mail]>
> > > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk
> connect
> > > and disconnect functions...
> > >
> > > Am 28.11.2018 um 17:40 hat Paul Durrant geschrieben:
> > > > > -----Original Message-----
> > > > > From: Kevin Wolf [mailto:[e-mail]]
> > > > > Sent: 28 November 2018 16:35
> > > > > To: Paul Durrant <[e-mail]>
> > > > > Cc: [e-mail]; [e-mail]; xen-
> > > > > [e-mail]; Stefano Stabellini
> > > <[e-mail]>;
> > > > > Anthony Perard <[e-mail]>; Max Reitz
> > > <[e-mail]>
> > > > > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk
> > > connect
> > > > > and disconnect functions...
> > > > >
> > > > > Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > > > > > ...and wire in the dataplane.
> > > > > >
> > > > > > This patch adds the remaining code to make the xen-qdisk
> XenDevice
> > > > > > functional. The parameters that a block frontend expects to find
> are
> > > > > > populated in the backend xenstore area, and the 'ring-ref' and
> > > > > > 'event-channel' values specified in the frontend xenstore area
> are
> > > > > > mapped/bound and used to set up the dataplane.
> > > > > >
> > > > > > Signed-off-by: Paul Durrant <[e-mail]>
> > > > > > ---
> > > > > > Cc: Stefano Stabellini <[e-mail]>
> > > > > > Cc: Anthony Perard <[e-mail]>
> > > > > > Cc: Kevin Wolf <[e-mail]>
> > > > > > Cc: Max Reitz <[e-mail]>
> > > > > > ---
> > > > > >  hw/block/xen-qdisk.c       | 140
> > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  hw/xen/xen-bus.c           |  12 ++--
> > > > > >  include/hw/xen/xen-bus.h   |   8 +++
> > > > > >  include/hw/xen/xen-qdisk.h |  12 ++++
> > > > > >  4 files changed, 166 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > > > > index 35f7b70480..8c88393832 100644
> > > > > > --- a/hw/block/xen-qdisk.c
> > > > > > +++ b/hw/block/xen-qdisk.c
> > > > > > @@ -9,6 +9,10 @@
> > > > > >  #include "qapi/visitor.h"
> > > > > >  #include "hw/hw.h"
> > > > > >  #include "hw/xen/xen-qdisk.h"
> > > > > > +#include "sysemu/blockdev.h"
> > > > > > +#include "sysemu/block-backend.h"
> > > > > > +#include "sysemu/iothread.h"
> > > > > > +#include "dataplane/xen-qdisk.h"
> > > > > >  #include "trace.h"
> > > > > >
> > > > > >  static char *xen_qdisk_get_name(XenDevice *xendev, Error
> **errp)
> > > > > > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice
> *xendev,
> > > > > Error **errp)
> > > > > >  {
> > > > > >      XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> > > > > >      XenQdiskVdev *vdev = &qdiskdev->vdev;
> > > > > > +    BlockConf *conf = &qdiskdev->conf;
> > > > > > +    DriveInfo *dinfo;
> > > > > > +    bool is_cdrom;
> > > > > > +    unsigned int info;
> > > > > > +    int64_t size;
> > > > > >
> > > > > >      if (!vdev->valid) {
> > > > > >          error_setg(errp, "vdev property not set");
> > > > > > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice
> > > *xendev,
> > > > > Error **errp)
> > > > > >      }
> > > > > >
> > > > > >      trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > > > > > +
> > > > > > +    if (!conf->blk) {
> > > > > > +        error_setg(errp, "drive property not set");
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (!blk_is_inserted(conf->blk)) {
> > > > > > +        error_setg(errp, "device needs media, but drive is
> empty");
> > > > > > +        return;
> > > > > > +    }
> > > > >
> > > > > Hm, the code below suggests that you support CD-ROMs. Don't you
> want
> > > to
> > > > > support media change as well then? Which would mean that you need
> to
> > > > > support empty drives.
> > > >
> > > > Yes, that's a good point. I should get rid of that check.
> > >
> > > Or rather apply it only to hard disks. And for empty CDs, you'll
> > > probably need to create an empty BlockBackend (the !conf->blk case).
> > > Just check the IDE and/or SCSI code for comparison.
> > >
> > > > >
> > > > > > +    if (!blkconf_apply_backend_options(conf,
> blk_is_read_only(conf-
> > > > > >blk),
> > > > > > +                                       false, errp)) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    dinfo = blk_legacy_dinfo(conf->blk);
> > > > > > +    is_cdrom = (dinfo && dinfo->media_cd);
> > > > >
> > > > > It's called legacy for a reason. Don't use this in new devices.
> > > > >
> > > > > The proper way is to have two different devices for hard disks and
> CDs
> > > > > (like scsi-hd and scsi-cd).
> > > >
> > > > ...or presumably I could have a property? The legacy init code could
> > > > then set it based on the drive info.
> > >
> > > Technically yes, but why would that be a good way to model things? I
> > > mean, it's true that xen-qdisk is not real hardware, but I've never
> seen
> > > any hardware that has a switch to decide whether it should behave as a
> > > CD drive or a hard disk.
> > >
> > > Both have very different characteristics (read-only with removable
> > > media, or a single read-write disk), and the existing implementations
> > > use two separate devices. So even if you're not convinced that users
> > > will consider them different concepts (I am; and if they weren't
> > > different concepts, you wouldn't need an is_cdrom variable),
> consistency
> > > is still a good thing.
> >
> > Ok. I'll split the device as you suggest... it may mean duplicated
> > code, but the datapath can still be common.
> 
> If you have a look at IDE and SCSI, they don't really duplicate a lot of
> code. Basically it's just a second QOM class definition, the rest is
> shared. Even the realize functions are essentially shared, with just two
> small wrappers for each device type around the common code.

Ok, I was hoping the duplication would be limited to something like that :-) I'll try to follow suit.

  Paul

> 
> Kevin

Anthony PERARD Dec. 4, 2018, 12:33 p.m. UTC | #7

On Wed, Nov 21, 2018 at 03:12:07PM +0000, Paul Durrant wrote:
> diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> index 35f7b70480..8c88393832 100644
> --- a/hw/block/xen-qdisk.c
> +++ b/hw/block/xen-qdisk.c
>  static void xen_qdisk_connect(XenQdiskDevice *qdiskdev, Error **errp)
>  {
>      XenQdiskVdev *vdev = &qdiskdev->vdev;
> +    XenDevice *xendev = XEN_DEVICE(qdiskdev);
> +    unsigned int order, nr_ring_ref, *ring_ref, event_channel, protocol;
> +    char *str;
>  
>      trace_xen_qdisk_connect(vdev->disk, vdev->partition);
> +
> +    if (xen_device_frontend_scanf(xendev, "ring-page-order", "%u",
> +                                  &order) != 1) {
> +        nr_ring_ref = 1;
> +        ring_ref = g_new(unsigned int, nr_ring_ref);
> +
> +        if (xen_device_frontend_scanf(xendev, "ring-ref", "%u",
> +                                      &ring_ref[0]) != 1) {
> +            error_setg(errp, "failed to read ring-ref");

Don't you need to free `ring_ref`?

> +            return;
> +        }
[...]

> diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> index ade0866037..d7dd2bf0ee 100644
> --- a/include/hw/xen/xen-qdisk.h
> +++ b/include/hw/xen/xen-qdisk.h
> @@ -6,7 +6,15 @@
>  #ifndef HW_XEN_QDISK_H
>  #define HW_XEN_QDISK_H
>  
> +#include "hw/xen/xen.h"
>  #include "hw/xen/xen-bus.h"
> +#include "hw/block/block.h"
> +#include "hw/block/xen_blkif.h"
> +#include "hw/block/dataplane/xen-qdisk.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/iothread.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"

You don't need that many includes, especially not iothread.h twice ;-).

I think those new includes would be enough:
#include "hw/block/block.h"; for BlockConf
#include "sysemu/iothread.h"
#include "hw/block/dataplane/xen-qdisk.h"

>  
>  typedef enum XenQdiskVdevType {
>      XEN_QDISK_VDEV_TYPE_DP,
> @@ -33,6 +41,10 @@ typedef struct XenQdiskDevice XenQdiskDevice;
>  struct XenQdiskDevice {
>      XenDevice xendev;
>      XenQdiskVdev vdev;
> +    BlockConf conf;
> +    unsigned int max_ring_page_order;
> +    IOThread *iothread;
> +    XenQdiskDataPlane *dataplane;
>  };
>  
>  #endif /* HW_XEN_QDISK_H */

Paul Durrant Dec. 6, 2018, 12:27 p.m. UTC | #8

> -----Original Message-----
> From: Anthony PERARD [mailto:[e-mail]]
> Sent: 04 December 2018 12:34
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; [e-mail]; xen-
> [e-mail]; Stefano Stabellini <[e-mail]>;
> Kevin Wolf <[e-mail]>; Max Reitz <[e-mail]>
> Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> and disconnect functions...
> 
> On Wed, Nov 21, 2018 at 03:12:07PM +0000, Paul Durrant wrote:
> > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > index 35f7b70480..8c88393832 100644
> > --- a/hw/block/xen-qdisk.c
> > +++ b/hw/block/xen-qdisk.c
> >  static void xen_qdisk_connect(XenQdiskDevice *qdiskdev, Error **errp)
> >  {
> >      XenQdiskVdev *vdev = &qdiskdev->vdev;
> > +    XenDevice *xendev = XEN_DEVICE(qdiskdev);
> > +    unsigned int order, nr_ring_ref, *ring_ref, event_channel,
> protocol;
> > +    char *str;
> >
> >      trace_xen_qdisk_connect(vdev->disk, vdev->partition);
> > +
> > +    if (xen_device_frontend_scanf(xendev, "ring-page-order", "%u",
> > +                                  &order) != 1) {
> > +        nr_ring_ref = 1;
> > +        ring_ref = g_new(unsigned int, nr_ring_ref);
> > +
> > +        if (xen_device_frontend_scanf(xendev, "ring-ref", "%u",
> > +                                      &ring_ref[0]) != 1) {
> > +            error_setg(errp, "failed to read ring-ref");
> 
> Don't you need to free `ring_ref`?

Yes.

> 
> > +            return;
> > +        }
> [...]
> 
> > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > index ade0866037..d7dd2bf0ee 100644
> > --- a/include/hw/xen/xen-qdisk.h
> > +++ b/include/hw/xen/xen-qdisk.h
> > @@ -6,7 +6,15 @@
> >  #ifndef HW_XEN_QDISK_H
> >  #define HW_XEN_QDISK_H
> >
> > +#include "hw/xen/xen.h"
> >  #include "hw/xen/xen-bus.h"
> > +#include "hw/block/block.h"
> > +#include "hw/block/xen_blkif.h"
> > +#include "hw/block/dataplane/xen-qdisk.h"
> > +#include "sysemu/blockdev.h"
> > +#include "sysemu/iothread.h"
> > +#include "sysemu/block-backend.h"
> > +#include "sysemu/iothread.h"
> 
> You don't need that many includes, especially not iothread.h twice ;-).
> 

Oops.

> I think those new includes would be enough:
> #include "hw/block/block.h"; for BlockConf
> #include "sysemu/iothread.h"
> #include "hw/block/dataplane/xen-qdisk.h"
> 

Yes, those seem to be enough.

  Paul

> >
> >  typedef enum XenQdiskVdevType {
> >      XEN_QDISK_VDEV_TYPE_DP,
> > @@ -33,6 +41,10 @@ typedef struct XenQdiskDevice XenQdiskDevice;
> >  struct XenQdiskDevice {
> >      XenDevice xendev;
> >      XenQdiskVdev vdev;
> > +    BlockConf conf;
> > +    unsigned int max_ring_page_order;
> > +    IOThread *iothread;
> > +    XenQdiskDataPlane *dataplane;
> >  };
> >
> >  #endif /* HW_XEN_QDISK_H */
> 
> --
> Anthony PERARD

Patch

diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
index 35f7b70480..8c88393832 100644
--- a/hw/block/xen-qdisk.c
+++ b/hw/block/xen-qdisk.c
@@ -9,6 +9,10 @@ 
 #include "qapi/visitor.h"
 #include "hw/hw.h"
 #include "hw/xen/xen-qdisk.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
+#include "dataplane/xen-qdisk.h"
 #include "trace.h"
 
 static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
@@ -23,6 +27,11 @@  static void xen_qdisk_realize(XenDevice *xendev, Error **errp)
 {
     XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
     XenQdiskVdev *vdev = &qdiskdev->vdev;
+    BlockConf *conf = &qdiskdev->conf;
+    DriveInfo *dinfo;
+    bool is_cdrom;
+    unsigned int info;
+    int64_t size;
 
     if (!vdev->valid) {
         error_setg(errp, "vdev property not set");
@@ -30,13 +39,134 @@  static void xen_qdisk_realize(XenDevice *xendev, Error **errp)
     }
 
     trace_xen_qdisk_realize(vdev->disk, vdev->partition);
+
+    if (!conf->blk) {
+        error_setg(errp, "drive property not set");
+        return;
+    }
+
+    if (!blk_is_inserted(conf->blk)) {
+        error_setg(errp, "device needs media, but drive is empty");
+        return;
+    }
+
+    if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf->blk),
+                                       false, errp)) {
+        return;
+    }
+
+    if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
+        return;
+    }
+
+    dinfo = blk_legacy_dinfo(conf->blk);
+    is_cdrom = (dinfo && dinfo->media_cd);
+
+    blkconf_blocksizes(conf);
+
+    if (conf->logical_block_size > conf->physical_block_size) {
+        error_setg(
+            errp, "logical_block_size > physical_block_size not supported");
+        return;
+    }
+
+    blk_set_guest_block_size(conf->blk, conf->logical_block_size);
+
+    if (conf->discard_granularity > 0) {
+        xen_device_backend_printf(xendev, "feature-discard", "%u", 1);
+    }
+
+    xen_device_backend_printf(xendev, "feature-flush-cache", "%u", 1);
+    xen_device_backend_printf(xendev, "max-ring-page-order", "%u",
+                              qdiskdev->max_ring_page_order);
+
+    info = blk_is_read_only(conf->blk) ? VDISK_READONLY : 0;
+    info |= is_cdrom ? VDISK_CDROM : 0;
+
+    xen_device_backend_printf(xendev, "info", "%u", info);
+
+    xen_device_frontend_printf(xendev, "virtual-device", "%u",
+                               vdev->number);
+    xen_device_frontend_printf(xendev, "device-type", "%s",
+                               is_cdrom ? "cdrom" : "disk");
+
+    size = blk_getlength(conf->blk);
+    xen_device_backend_printf(xendev, "sector-size", "%u",
+                              conf->logical_block_size);
+    xen_device_backend_printf(xendev, "sectors", "%lu",
+                              size / conf->logical_block_size);
+
+    qdiskdev->dataplane = xen_qdisk_dataplane_create(xendev, conf,
+                                                     qdiskdev->iothread);
 }
 
 static void xen_qdisk_connect(XenQdiskDevice *qdiskdev, Error **errp)
 {
     XenQdiskVdev *vdev = &qdiskdev->vdev;
+    XenDevice *xendev = XEN_DEVICE(qdiskdev);
+    unsigned int order, nr_ring_ref, *ring_ref, event_channel, protocol;
+    char *str;
 
     trace_xen_qdisk_connect(vdev->disk, vdev->partition);
+
+    if (xen_device_frontend_scanf(xendev, "ring-page-order", "%u",
+                                  &order) != 1) {
+        nr_ring_ref = 1;
+        ring_ref = g_new(unsigned int, nr_ring_ref);
+
+        if (xen_device_frontend_scanf(xendev, "ring-ref", "%u",
+                                      &ring_ref[0]) != 1) {
+            error_setg(errp, "failed to read ring-ref");
+            return;
+        }
+    } else if (order <= qdiskdev->max_ring_page_order) {
+        unsigned int i;
+
+        nr_ring_ref = 1 << order;
+        ring_ref = g_new(unsigned int, nr_ring_ref);
+
+        for (i = 0; i < nr_ring_ref; i++) {
+            const char *key = g_strdup_printf("ring-ref%u", i);
+
+            if (xen_device_frontend_scanf(xendev, key, "%u",
+                                          &ring_ref[i]) != 1) {
+                error_setg(errp, "failed to read %s", key);
+                g_free((gpointer)key);
+                return;
+            }
+
+            g_free((gpointer)key);
+        }
+    } else {
+        error_setg(errp, "invalid ring-page-order (%d)", order);
+        return;
+    }
+
+    if (xen_device_frontend_scanf(xendev, "event-channel", "%u",
+                                  &event_channel) != 1) {
+        error_setg(errp, "failed to read event-channel");
+        return;
+    }
+
+    if (xen_device_frontend_scanf(xendev, "protocol", "%ms",
+                                  &str) != 1) {
+        protocol = BLKIF_PROTOCOL_NATIVE;
+    } else {
+        if (strcmp(str, XEN_IO_PROTO_ABI_X86_32) == 0) {
+            protocol = BLKIF_PROTOCOL_X86_32;
+        } else if (strcmp(str, XEN_IO_PROTO_ABI_X86_64) == 0) {
+            protocol = BLKIF_PROTOCOL_X86_64;
+        } else {
+            protocol = BLKIF_PROTOCOL_NATIVE;
+        }
+
+        free(str);
+    }
+
+    xen_qdisk_dataplane_start(qdiskdev->dataplane, ring_ref, nr_ring_ref,
+                              event_channel, protocol);
+
+    g_free(ring_ref);
 }
 
 static void xen_qdisk_disconnect(XenQdiskDevice *qdiskdev, Error **errp)
@@ -44,6 +174,8 @@  static void xen_qdisk_disconnect(XenQdiskDevice *qdiskdev, Error **errp)
     XenQdiskVdev *vdev = &qdiskdev->vdev;
 
     trace_xen_qdisk_disconnect(vdev->disk, vdev->partition);
+
+    xen_qdisk_dataplane_stop(qdiskdev->dataplane);
 }
 
 static void xen_qdisk_frontend_changed(XenDevice *xendev,
@@ -93,6 +225,9 @@  static void xen_qdisk_unrealize(XenDevice *xendev, Error **errp)
     trace_xen_qdisk_unrealize(vdev->disk, vdev->partition);
 
     xen_qdisk_disconnect(qdiskdev, &error_fatal);
+
+    xen_qdisk_dataplane_destroy(qdiskdev->dataplane);
+    qdiskdev->dataplane = NULL;
 }
 
 static char *disk_to_vbd_name(unsigned int disk)
@@ -289,6 +424,11 @@  const PropertyInfo xen_qdisk_prop_vdev = {
 static Property xen_qdisk_props[] = {
     DEFINE_PROP("vdev", XenQdiskDevice, vdev,
                 xen_qdisk_prop_vdev, XenQdiskVdev),
+    DEFINE_BLOCK_PROPERTIES(XenQdiskDevice, conf),
+    DEFINE_PROP_UINT32("max-ring-page-order", XenQdiskDevice,
+                       max_ring_page_order, 4),
+    DEFINE_PROP_LINK("iothread", XenQdiskDevice, iothread, TYPE_IOTHREAD,
+                     IOThread *),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 64c8af54b0..c4b253103f 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -217,8 +217,8 @@  static const TypeInfo xen_bus_type_info = {
     .class_init = xen_bus_class_init,
 };
 
-static void xen_device_backend_printf(XenDevice *xendev, const char *key,
-                                      const char *fmt, ...)
+void xen_device_backend_printf(XenDevice *xendev, const char *key,
+                               const char *fmt, ...)
 {
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     va_list ap;
@@ -305,8 +305,8 @@  static void xen_device_backend_destroy(XenDevice *xendev)
     g_free(xendev->backend_path);
 }
 
-static void xen_device_frontend_printf(XenDevice *xendev, const char *key,
-                                       const char *fmt, ...)
+void xen_device_frontend_printf(XenDevice *xendev, const char *key,
+                                const char *fmt, ...)
 {
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     va_list ap;
@@ -318,8 +318,8 @@  static void xen_device_frontend_printf(XenDevice *xendev, const char *key,
     va_end(ap);
 }
 
-static int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
-                                      const char *fmt, ...)
+int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
+                              const char *fmt, ...)
 {
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     va_list ap;
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 386f6bfc93..d931e01268 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -82,6 +82,14 @@  void xen_device_backend_set_state(XenDevice *xendev,
                                   enum xenbus_state state);
 enum xenbus_state xen_device_backend_get_state(XenDevice *xendev);
 
+void xen_device_backend_printf(XenDevice *xendev, const char *key,
+                               const char *fmt, ...);
+
+void xen_device_frontend_printf(XenDevice *xendev, const char *key,
+                                const char *fmt, ...);
+int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
+                              const char *fmt, ...);
+
 void xen_device_set_max_grant_refs(XenDevice *xendev, unsigned int nr_refs,
                                    Error **errp);
 void *xen_device_map_grant_refs(XenDevice *xendev, uint32_t *refs,
diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
index ade0866037..d7dd2bf0ee 100644
--- a/include/hw/xen/xen-qdisk.h
+++ b/include/hw/xen/xen-qdisk.h
@@ -6,7 +6,15 @@ 
 #ifndef HW_XEN_QDISK_H
 #define HW_XEN_QDISK_H
 
+#include "hw/xen/xen.h"
 #include "hw/xen/xen-bus.h"
+#include "hw/block/block.h"
+#include "hw/block/xen_blkif.h"
+#include "hw/block/dataplane/xen-qdisk.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/iothread.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
 
 typedef enum XenQdiskVdevType {
     XEN_QDISK_VDEV_TYPE_DP,
@@ -33,6 +41,10 @@  typedef struct XenQdiskDevice XenQdiskDevice;
 struct XenQdiskDevice {
     XenDevice xendev;
     XenQdiskVdev vdev;
+    BlockConf conf;
+    unsigned int max_ring_page_order;
+    IOThread *iothread;
+    XenQdiskDataPlane *dataplane;
 };
 
 #endif /* HW_XEN_QDISK_H */