Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device

[Liu, Changpeng]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Monday, October 23, 2017 8:55 PM
> To: Liu, Changpeng <address@hidden>
> Cc: Stefan Hajnoczi <address@hidden>; address@hidden;
> address@hidden; address@hidden; address@hidden;
> Harris, James R <address@hidden>
> Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk 
> host
> device
> 
> On Mon, Oct 23, 2017 at 04:26:36AM +0000, Liu, Changpeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Stefan Hajnoczi [mailto:address@hidden
> > > Sent: Friday, October 20, 2017 5:55 PM
> > > To: Liu, Changpeng <address@hidden>
> > > Cc: address@hidden; address@hidden; address@hidden;
> > > address@hidden; address@hidden; Harris, James R
> > > <address@hidden>
> > > Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk
> host
> > > device
> > >
> > > On Fri, Oct 20, 2017 at 01:47:58AM +0000, Liu, Changpeng wrote:
> > > > > > +static Property vhost_user_blk_properties[] = {
> > > > > > +    DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
> > > > > > +    DEFINE_PROP_UINT16("num_queues", VHostUserBlk, num_queues,
> 1),
> > > > > > +    DEFINE_PROP_UINT32("queue_size", VHostUserBlk, queue_size, 
> > > > > > 128),
> > > > > > +    DEFINE_PROP_BIT64("f_size_max", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_SIZE_MAX, true),
> > > > > > +    DEFINE_PROP_BIT64("f_sizemax", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_SIZE_MAX, true),
> > > > > > +    DEFINE_PROP_BIT64("f_segmax", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_SEG_MAX, true),
> > > > > > +    DEFINE_PROP_BIT64("f_geometry", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_GEOMETRY, true),
> > > > > > +    DEFINE_PROP_BIT64("f_readonly", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_RO, false),
> > > > > > +    DEFINE_PROP_BIT64("f_blocksize", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_BLK_SIZE, true),
> > > > > > +    DEFINE_PROP_BIT64("f_topology", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_TOPOLOGY, true),
> > > > > > +    DEFINE_PROP_BIT64("f_multiqueue", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_MQ, true),
> > > > > > +    DEFINE_PROP_BIT64("f_flush", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_FLUSH, true),
> > > > > > +    DEFINE_PROP_BIT64("f_barrier", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_BARRIER, false),
> > > > > > +    DEFINE_PROP_BIT64("f_scsi", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_SCSI, false),
> > > > > > +    DEFINE_PROP_BIT64("f_wce", VHostUserBlk, host_features,
> > > > > > +                      VIRTIO_BLK_F_WCE, false),
> > > > >
> > > > > Please explain how feature negotation works.  The vhost-user slave
> > > > > advertises support features in the return value from
> > > > > VHOST_USER_GET_FEATURES.  How does this additional feature mask
> work
> > > and
> > > > > why is it useful?
> > > > According to Paolo's previous comments, VIRTIO_BLK_F_WCE/
> > > VIRTIO_BLK_F_SCSI/ VIRTIO_BLK_F_BARRIER
> > > > should be removed. Here I added all the feature flags just want to avoid
> the
> > > case that vhost-user slave target
> > > > can support but Qemu vhost block driver cannot support it.
> > >
> > > Please explain a bit more how these options can be used.
> > >
> > > When I looked at the vhost code it seemed like the vhost slave can
> > > report any feature bits it wishes (even things QEMU doesn't know about).
> > > What is the purpose of override some of the feature bits on the QEMU
> > > command-line?
> > Hi Stefan,
> > Here I added a switch which can override vhost-slave's feature bits, for
> example, vhost-slave reported `VIRTIO_BLK_F_RO`,
> > but Qemu vhost-master can disable it through command line when started the
> Qemu. Users don't need to change any
> > vhost-slave's code to disable this feature, and this is also aligned with 
> > vhost-
> scsi and vhost-net's implementation.
> 
> Yes but I don't see these properties in virtio_blk_properties. Please
> make the names consistent at least when virtio-blk has them.
> I am pretty sure you don't want to expose e.g. _F_SCSI.
Yes, I should remove F_SCSI/F_WCE/F_BARRIER features, Virtio-blk hardcoded 4 
features: VIRTIO_BLK_F_SEG_MAX/VIRTIO_BLK_F_GEOMETRY/
VIRTIO_BLK_F_TOPOLOGY/VIRTIO_BLK_F_BLK_SIZE, and extra several configuration 
parameters for VIRTIO_BLK_F_CONFIG_WCE/VIRTIO_BLK_F_MQ,
I can change those feature bits same as virtio-blk.
> 
> 
> > > Stefan