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

2017-11-03 Thread Stefan Hajnoczi 
On Wed, Oct 25, 2017 at 01:34:39AM +, Liu, Changpeng wrote:
> 
> 
> > -Original Message-
> > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > Sent: Tuesday, October 24, 2017 8:53 PM
> > To: Liu, Changpeng 
> > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > 
> > Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk 
> > host
> > device
> > 
> > On Tue, Oct 24, 2017 at 12:44:30AM +, Liu, Changpeng wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > > Sent: Tuesday, October 24, 2017 1:12 AM
> > > > To: Liu, Changpeng 
> > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > > 
> > > > 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 +, Liu, Changpeng wrote:
> > > > >
> > > > >
> > > > > > -Original Message-
> > > > > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > > > > Sent: Friday, October 20, 2017 5:55 PM
> > > > > > To: Liu, Changpeng 
> > > > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > > > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > > > > 
> > > > > > 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 +, 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

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

2017-10-24 Thread Liu, Changpeng 


> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> Sent: Tuesday, October 24, 2017 8:53 PM
> To: Liu, Changpeng 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> 
> Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk 
> host
> device
> 
> On Tue, Oct 24, 2017 at 12:44:30AM +, Liu, Changpeng wrote:
> >
> >
> > > -Original Message-
> > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > Sent: Tuesday, October 24, 2017 1:12 AM
> > > To: Liu, Changpeng 
> > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > 
> > > 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 +, Liu, Changpeng wrote:
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > > > Sent: Friday, October 20, 2017 5:55 PM
> > > > > To: Liu, Changpeng 
> > > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > > > 
> > > > > 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 +, 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`,
> > >

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

2017-10-24 Thread Stefan Hajnoczi 
On Tue, Oct 24, 2017 at 12:44:30AM +, Liu, Changpeng wrote:
> 
> 
> > -Original Message-
> > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > Sent: Tuesday, October 24, 2017 1:12 AM
> > To: Liu, Changpeng 
> > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > 
> > 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 +, Liu, Changpeng wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > > Sent: Friday, October 20, 2017 5:55 PM
> > > > To: Liu, Changpeng 
> > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > > 
> > > > 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 +, 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.
> > 
> > You said vhost-master can disable features but the code doesn't seem to
> > work that way:
> > 
> > +/* Turn on pre-defined features */
> > +features |= s->host_features;
> User can append parameter when started Qemu: e.g: f_readonly=false to disable 
> it.

No, f_readonly=false has no effect if the vhost slave replies with f_readonly
to the VHOST_USER_GET_FEATURES message.

Take a look at

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

2017-10-23 Thread Liu, Changpeng 


> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> Sent: Tuesday, October 24, 2017 1:12 AM
> To: Liu, Changpeng 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> 
> 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 +, Liu, Changpeng wrote:
> >
> >
> > > -Original Message-
> > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > Sent: Friday, October 20, 2017 5:55 PM
> > > To: Liu, Changpeng 
> > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > 
> > > 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 +, 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.
> 
> You said vhost-master can disable features but the code doesn't seem to
> work that way:
> 
> +/* Turn on pre-defined features */
> +features |= s->host_features;
User can append parameter when started Qemu: e.g: f_readonly=false to disable 
it.
> 
> If the use case isn't clear please remove these properties for now.
I can make it the same with virtio-blk, hardcoded the mandatory features, and 
put 
VIRTIO_BLK_F_MQ/VIRTIO_BLK_F_RO/VIRTIO_BLK_F_CONFIG_WCE configurable. Thoughts?
> 
> Stefan

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

2017-10-23 Thread Liu, Changpeng 


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, October 23, 2017 8:55 PM
> To: Liu, Changpeng 
> Cc: Stefan Hajnoczi ; qemu-devel@nongnu.org;
> pbonz...@redhat.com; marcandre.lur...@redhat.com; fel...@nutanix.com;
> Harris, James R 
> 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 +, Liu, Changpeng wrote:
> >
> >
> > > -Original Message-
> > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > > Sent: Friday, October 20, 2017 5:55 PM
> > > To: Liu, Changpeng 
> > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > > 
> > > 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 +, 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

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

2017-10-23 Thread Stefan Hajnoczi 
On Mon, Oct 23, 2017 at 04:26:36AM +, Liu, Changpeng wrote:
> 
> 
> > -Original Message-
> > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > Sent: Friday, October 20, 2017 5:55 PM
> > To: Liu, Changpeng 
> > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > 
> > 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 +, 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.

You said vhost-master can disable features but the code doesn't seem to
work that way:

+/* Turn on pre-defined features */
+features |= s->host_features;

If the use case isn't clear please remove these properties for now.

Stefan

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

2017-10-23 Thread Michael S. Tsirkin 
On Mon, Oct 23, 2017 at 04:26:36AM +, Liu, Changpeng wrote:
> 
> 
> > -Original Message-
> > From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> > Sent: Friday, October 20, 2017 5:55 PM
> > To: Liu, Changpeng 
> > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> > 
> > 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 +, 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.


> > Stefan

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

2017-10-22 Thread Liu, Changpeng 


> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> Sent: Friday, October 20, 2017 5:55 PM
> To: Liu, Changpeng 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> 
> 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 +, 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.
> 
> Stefan

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

2017-10-20 Thread Stefan Hajnoczi 
On Fri, Oct 20, 2017 at 01:47:58AM +, 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?

Stefan

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

2017-10-19 Thread Liu, Changpeng 


> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> Sent: Thursday, October 19, 2017 11:18 PM
> To: Liu, Changpeng 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com;
> marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R
> 
> Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk 
> host
> device
> 
> On Thu, Oct 19, 2017 at 01:24:08PM +0800, Changpeng Liu wrote:
> > This commit introduces a new vhost-user device for block, it uses a
> > chardev to connect with the backend, same with Qemu virito-blk device,
> > Guest OS still uses the virtio-blk frontend driver.
> >
> > To use it, start Qemu with command line like this:
> >
> > qemu-system-x86_64 \
> > -chardev socket,id=char0,path=/path/vhost.socket \
> > -device vhost-user-blk-pci,chardev=char0,num_queues=1, \
> > bootindex=2... \
> >
> > Users can use different parameters for `num_queues` and `bootindex`.
> >
> > Different with exist Qemu virtio-blk host device, it makes more easy
> > for users to implement their own I/O processing logic, such as all
> > user space I/O stack against hardware block device. It uses the new
> > vhost messages(VHOST_USER_GET_CONFIG) to get block virtio config
> > information from backend process.
> >
> > Signed-off-by: Changpeng Liu 
> > ---
> >  configure  |  11 ++
> >  hw/block/Makefile.objs |   3 +
> >  hw/block/vhost-user-blk.c  | 360
> +
> >  hw/virtio/virtio-pci.c |  55 ++
> >  hw/virtio/virtio-pci.h |  18 ++
> >  include/hw/virtio/vhost-user-blk.h |  40 +
> >  6 files changed, 487 insertions(+)
> >  create mode 100644 hw/block/vhost-user-blk.c
> >  create mode 100644 include/hw/virtio/vhost-user-blk.h
> >
> > diff --git a/configure b/configure
> > index 663e908..f2b348f 100755
> > --- a/configure
> > +++ b/configure
> > @@ -318,6 +318,7 @@ tcg="yes"
> >
> >  vhost_net="no"
> >  vhost_scsi="no"
> > +vhost_user_blk="no"
> >  vhost_vsock="no"
> >  vhost_user=""
> >  kvm="no"
> > @@ -782,6 +783,7 @@ Linux)
> >kvm="yes"
> >vhost_net="yes"
> >vhost_scsi="yes"
> > +  vhost_user_blk="yes"
> >vhost_vsock="yes"
> >QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers
> $QEMU_INCLUDES"
> >supported_os="yes"
> > @@ -1139,6 +1141,10 @@ for opt do
> >;;
> >--enable-vhost-scsi) vhost_scsi="yes"
> >;;
> > +  --disable-vhost-user-blk) vhost_user_blk="no"
> > +  ;;
> > +  --enable-vhost-user-blk) vhost_user_blk="yes"
> > +  ;;
> >--disable-vhost-vsock) vhost_vsock="no"
> >;;
> >--enable-vhost-vsock) vhost_vsock="yes"
> > @@ -1511,6 +1517,7 @@ disabled with --disable-FEATURE, default is enabled if
> available:
> >cap-ng  libcap-ng support
> >attrattr and xattr support
> >vhost-net   vhost-net acceleration support
> > +  vhost-user-blk  VM virtio-blk acceleration in user space
> >spice   spice
> >rbd rados block device (rbd)
> >libiscsiiscsi support
> > @@ -5417,6 +5424,7 @@ echo "posix_madvise $posix_madvise"
> >  echo "libcap-ng support $cap_ng"
> >  echo "vhost-net support $vhost_net"
> >  echo "vhost-scsi support $vhost_scsi"
> > +echo "vhost-user-blk support $vhost_user_blk"
> >  echo "vhost-vsock support $vhost_vsock"
> >  echo "vhost-user support $vhost_user"
> >  echo "Trace backends$trace_backends"
> > @@ -5845,6 +5853,9 @@ fi
> >  if test "$vhost_scsi" = "yes" ; then
> >echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
> >  fi
> > +if test "$vhost_user_blk" = "yes" -a "$vhost_user" = "yes"; then
> > +  echo "CONFIG_VHOST_USER_BLK=y" >> $config_host_mak
> > +fi
> >  if test "$vhost_net" = "yes" -a "$vhost_user" = "yes"; then
> >echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
> >  fi
> > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> > index e0ed980..4c19a58 100644
> > --- a/hw/block/Makefile.objs
> > +++ b/hw/block/Makefile.objs
> > @@ -13,3 +13,6 @@ obj-$(CONFIG_SH4) += tc58128.o
> >
> >  obj-$(CONFIG_VIRTIO) += virtio-blk.o
> >  obj-$(CONFIG_VIRTIO) += dataplane/
> > +ifeq ($(CONFIG_VIRTIO),y)
> > +obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
> > +endif
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > new file mode 100644
> > index 000..8aa9fa9
> > --- /dev/null
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -0,0 +1,360 @@
> > +/*
> > + * vhost-user-blk host device
> > + *
> > + * Copyright IBM, Corp. 2011
> > + * Copyright(C) 2017 Intel Corporation.
> > + *
> > + * Authors:
> > + *  Stefan Hajnoczi 
> > + *  Changpeng Liu 
> 
> This gives the impression that IBM originally authored this code but
> little copied code is actually in this file.  Feel free to put your own
> copyright and

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

2017-10-19 Thread Liu, Changpeng 


> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Thursday, October 19, 2017 7:33 PM
> To: Liu, Changpeng ; qemu-devel@nongnu.org
> Cc: stefa...@gmail.com; m...@redhat.com; marcandre.lur...@redhat.com;
> fel...@nutanix.com; Harris, James R 
> Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk 
> host
> device
> 
> On 19/10/2017 07:24, Changpeng Liu wrote:
> >;;
> >--enable-vhost-scsi) vhost_scsi="yes"
> >;;
> > +  --disable-vhost-user-blk) vhost_user_blk="no"
> > +  ;;
> > +  --enable-vhost-user-blk) vhost_user_blk="yes"
> > +  ;;
> >--disable-vhost-vsock) vhost_vsock="no"
> >;;
> >--enable-vhost-vsock) vhost_vsock="yes"
> > @@ -1511,6 +1517,7 @@ disabled with --disable-FEATURE, default is enabled if
> available:
> >cap-ng  libcap-ng support
> >attrattr and xattr support
> >vhost-net   vhost-net acceleration support
> > +  vhost-user-blk  VM virtio-blk acceleration in user space
> 
> Please use default-configs instead of a new configure switch.  See how
> CONFIG_VHOST_USER_SCSI is used in default-configs/pci.mak and
> default-configs/s390x-softmmu.mak.
Ok, thanks.
> 
> >
> > +static const int user_feature_bits[] = {
> > +VIRTIO_BLK_F_SIZE_MAX,
> > +VIRTIO_BLK_F_SEG_MAX,
> > +VIRTIO_BLK_F_GEOMETRY,
> > +VIRTIO_BLK_F_BLK_SIZE,
> > +VIRTIO_BLK_F_TOPOLOGY,
> > +VIRTIO_BLK_F_SCSI,
> 
> Please omit VIRTIO_BLK_F_SCSI, it's a legacy option that is not anymore
> part of virtio 1.0.
ok
> 
> > +VIRTIO_BLK_F_MQ,
> > +VIRTIO_BLK_F_RO,
> > +VIRTIO_BLK_F_FLUSH,
> > +VIRTIO_BLK_F_BARRIER,
> 
> Same for VIRTIO_BLK_F_BARRIER.
> 
> > +VIRTIO_BLK_F_WCE,
> 
> And VIRTIO_BLK_F_WCE is the same as VIRTIO_BLK_F_FLUSH, so it can be
> removed too.  Please include VIRTIO_BLK_F_CONFIG_WCE instead, since you
> are supporting it in vhost_user_blk_set_config.
Ok.
> 
> > +VIRTIO_F_VERSION_1,
> > +VIRTIO_RING_F_INDIRECT_DESC,
> > +VIRTIO_RING_F_EVENT_IDX,
> > +VIRTIO_F_NOTIFY_ON_EMPTY,
> > +VHOST_INVALID_FEATURE_BIT
> > +};
> 
> >
> > +static const TypeInfo vhost_user_blk_info = {
> > +.name = TYPE_VHOST_USER_BLK,
> > +.parent = TYPE_VIRTIO_DEVICE,
> > +.instance_size = sizeof(VHostUserBlk),
> > +.instance_init = vhost_user_blk_instance_init,
> > +.class_init = vhost_user_blk_class_init,
> > +};
> > +
> 
> There is some code duplication, so maybe it's worth introducing a common
> superclass like TYPE_VIRTIO_SCSI_COMMON.  I'll let others comment on
> whether this is a requirement.
> 
> Paolo

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

2017-10-19 Thread Stefan Hajnoczi 
On Thu, Oct 19, 2017 at 01:24:08PM +0800, Changpeng Liu wrote:
> This commit introduces a new vhost-user device for block, it uses a
> chardev to connect with the backend, same with Qemu virito-blk device,
> Guest OS still uses the virtio-blk frontend driver.
> 
> To use it, start Qemu with command line like this:
> 
> qemu-system-x86_64 \
> -chardev socket,id=char0,path=/path/vhost.socket \
> -device vhost-user-blk-pci,chardev=char0,num_queues=1, \
> bootindex=2... \
> 
> Users can use different parameters for `num_queues` and `bootindex`.
> 
> Different with exist Qemu virtio-blk host device, it makes more easy
> for users to implement their own I/O processing logic, such as all
> user space I/O stack against hardware block device. It uses the new
> vhost messages(VHOST_USER_GET_CONFIG) to get block virtio config
> information from backend process.
> 
> Signed-off-by: Changpeng Liu 
> ---
>  configure  |  11 ++
>  hw/block/Makefile.objs |   3 +
>  hw/block/vhost-user-blk.c  | 360 
> +
>  hw/virtio/virtio-pci.c |  55 ++
>  hw/virtio/virtio-pci.h |  18 ++
>  include/hw/virtio/vhost-user-blk.h |  40 +
>  6 files changed, 487 insertions(+)
>  create mode 100644 hw/block/vhost-user-blk.c
>  create mode 100644 include/hw/virtio/vhost-user-blk.h
> 
> diff --git a/configure b/configure
> index 663e908..f2b348f 100755
> --- a/configure
> +++ b/configure
> @@ -318,6 +318,7 @@ tcg="yes"
>  
>  vhost_net="no"
>  vhost_scsi="no"
> +vhost_user_blk="no"
>  vhost_vsock="no"
>  vhost_user=""
>  kvm="no"
> @@ -782,6 +783,7 @@ Linux)
>kvm="yes"
>vhost_net="yes"
>vhost_scsi="yes"
> +  vhost_user_blk="yes"
>vhost_vsock="yes"
>QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers 
> $QEMU_INCLUDES"
>supported_os="yes"
> @@ -1139,6 +1141,10 @@ for opt do
>;;
>--enable-vhost-scsi) vhost_scsi="yes"
>;;
> +  --disable-vhost-user-blk) vhost_user_blk="no"
> +  ;;
> +  --enable-vhost-user-blk) vhost_user_blk="yes"
> +  ;;
>--disable-vhost-vsock) vhost_vsock="no"
>;;
>--enable-vhost-vsock) vhost_vsock="yes"
> @@ -1511,6 +1517,7 @@ disabled with --disable-FEATURE, default is enabled if 
> available:
>cap-ng  libcap-ng support
>attrattr and xattr support
>vhost-net   vhost-net acceleration support
> +  vhost-user-blk  VM virtio-blk acceleration in user space
>spice   spice
>rbd rados block device (rbd)
>libiscsiiscsi support
> @@ -5417,6 +5424,7 @@ echo "posix_madvise $posix_madvise"
>  echo "libcap-ng support $cap_ng"
>  echo "vhost-net support $vhost_net"
>  echo "vhost-scsi support $vhost_scsi"
> +echo "vhost-user-blk support $vhost_user_blk"
>  echo "vhost-vsock support $vhost_vsock"
>  echo "vhost-user support $vhost_user"
>  echo "Trace backends$trace_backends"
> @@ -5845,6 +5853,9 @@ fi
>  if test "$vhost_scsi" = "yes" ; then
>echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
>  fi
> +if test "$vhost_user_blk" = "yes" -a "$vhost_user" = "yes"; then
> +  echo "CONFIG_VHOST_USER_BLK=y" >> $config_host_mak
> +fi
>  if test "$vhost_net" = "yes" -a "$vhost_user" = "yes"; then
>echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
>  fi
> diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> index e0ed980..4c19a58 100644
> --- a/hw/block/Makefile.objs
> +++ b/hw/block/Makefile.objs
> @@ -13,3 +13,6 @@ obj-$(CONFIG_SH4) += tc58128.o
>  
>  obj-$(CONFIG_VIRTIO) += virtio-blk.o
>  obj-$(CONFIG_VIRTIO) += dataplane/
> +ifeq ($(CONFIG_VIRTIO),y)
> +obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
> +endif
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> new file mode 100644
> index 000..8aa9fa9
> --- /dev/null
> +++ b/hw/block/vhost-user-blk.c
> @@ -0,0 +1,360 @@
> +/*
> + * vhost-user-blk host device
> + *
> + * Copyright IBM, Corp. 2011
> + * Copyright(C) 2017 Intel Corporation.
> + *
> + * Authors:
> + *  Stefan Hajnoczi 
> + *  Changpeng Liu 

This gives the impression that IBM originally authored this code but
little copied code is actually in this file.  Feel free to put your own
copyright and authorship information at the top and then add a statement
like "Based on foo.c, Copyright IBM, Corp. 2011" instead.  That way it's
clear that this is new code that you wrote with reference to the
original file you used as a starting point.

> +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",

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

2017-10-19 Thread Paolo Bonzini 
On 19/10/2017 07:24, Changpeng Liu wrote:
>;;
>--enable-vhost-scsi) vhost_scsi="yes"
>;;
> +  --disable-vhost-user-blk) vhost_user_blk="no"
> +  ;;
> +  --enable-vhost-user-blk) vhost_user_blk="yes"
> +  ;;
>--disable-vhost-vsock) vhost_vsock="no"
>;;
>--enable-vhost-vsock) vhost_vsock="yes"
> @@ -1511,6 +1517,7 @@ disabled with --disable-FEATURE, default is enabled if 
> available:
>cap-ng  libcap-ng support
>attrattr and xattr support
>vhost-net   vhost-net acceleration support
> +  vhost-user-blk  VM virtio-blk acceleration in user space

Please use default-configs instead of a new configure switch.  See how
CONFIG_VHOST_USER_SCSI is used in default-configs/pci.mak and
default-configs/s390x-softmmu.mak.

> 
> +static const int user_feature_bits[] = {
> +VIRTIO_BLK_F_SIZE_MAX,
> +VIRTIO_BLK_F_SEG_MAX,
> +VIRTIO_BLK_F_GEOMETRY,
> +VIRTIO_BLK_F_BLK_SIZE,
> +VIRTIO_BLK_F_TOPOLOGY,
> +VIRTIO_BLK_F_SCSI,

Please omit VIRTIO_BLK_F_SCSI, it's a legacy option that is not anymore
part of virtio 1.0.

> +VIRTIO_BLK_F_MQ,
> +VIRTIO_BLK_F_RO,
> +VIRTIO_BLK_F_FLUSH,
> +VIRTIO_BLK_F_BARRIER,

Same for VIRTIO_BLK_F_BARRIER.

> +VIRTIO_BLK_F_WCE,

And VIRTIO_BLK_F_WCE is the same as VIRTIO_BLK_F_FLUSH, so it can be
removed too.  Please include VIRTIO_BLK_F_CONFIG_WCE instead, since you
are supporting it in vhost_user_blk_set_config.

> +VIRTIO_F_VERSION_1,
> +VIRTIO_RING_F_INDIRECT_DESC,
> +VIRTIO_RING_F_EVENT_IDX,
> +VIRTIO_F_NOTIFY_ON_EMPTY,
> +VHOST_INVALID_FEATURE_BIT
> +};

> 
> +static const TypeInfo vhost_user_blk_info = {
> +.name = TYPE_VHOST_USER_BLK,
> +.parent = TYPE_VIRTIO_DEVICE,
> +.instance_size = sizeof(VHostUserBlk),
> +.instance_init = vhost_user_blk_instance_init,
> +.class_init = vhost_user_blk_class_init,
> +};
> +

There is some code duplication, so maybe it's worth introducing a common
superclass like TYPE_VIRTIO_SCSI_COMMON.  I'll let others comment on
whether this is a requirement.

Paolo