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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Kevin Wolf [mailto:kwolf@xxxxxxxxxx]
> Sent: 29 November 2018 10:46
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: qemu-block@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Anthony Perard <anthony.perard@xxxxxxxxxx>; Max Reitz <mreitz@xxxxxxxxxx>
> 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:kwolf@xxxxxxxxxx]
> > > Sent: 29 November 2018 09:01
> > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > Cc: qemu-block@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; xen-
> > > devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>;
> > > Anthony Perard <anthony.perard@xxxxxxxxxx>; Max Reitz
> <mreitz@xxxxxxxxxx>
> > > 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:kwolf@xxxxxxxxxx]
> > > > > Sent: 28 November 2018 16:35
> > > > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > > > Cc: qemu-block@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; xen-
> > > > > devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini
> > > <sstabellini@xxxxxxxxxx>;
> > > > > Anthony Perard <anthony.perard@xxxxxxxxxx>; Max Reitz
> > > <mreitz@xxxxxxxxxx>
> > > > > 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 <paul.durrant@xxxxxxxxxx>
> > > > > > ---
> > > > > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > > Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
> > > > > > Cc: Kevin Wolf <kwolf@xxxxxxxxxx>
> > > > > > Cc: Max Reitz <mreitz@xxxxxxxxxx>
> > > > > > ---
> > > > > >  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel