| Message ID | 1508390650-19115-3-git-send-email-changpeng.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > attr attr 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
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 <changpeng.liu@intel.com> > --- > 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 > attr attr 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) > libiscsi iscsi 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 0000000..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 <stefanha@linux.vnet.ibm.com> > + * Changpeng Liu <changpeng.liu@intel.com> 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", 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?
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Thursday, October 19, 2017 7:33 PM > To: Liu, Changpeng <changpeng.liu@intel.com>; qemu-devel@nongnu.org > Cc: stefanha@gmail.com; mst@redhat.com; marcandre.lureau@redhat.com; > felipe@nutanix.com; Harris, James R <james.r.harris@intel.com> > 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 > > attr attr 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
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > Sent: Thursday, October 19, 2017 11:18 PM > To: Liu, Changpeng <changpeng.liu@intel.com> > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > <james.r.harris@intel.com> > 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 <changpeng.liu@intel.com> > > --- > > 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 > > attr attr 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) > > libiscsi iscsi 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 0000000..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 <stefanha@linux.vnet.ibm.com> > > + * Changpeng Liu <changpeng.liu@intel.com> > > 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. Ok, will change it, thanks. > > > +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.
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? Stefan
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > Sent: Friday, October 20, 2017 5:55 PM > To: Liu, Changpeng <changpeng.liu@intel.com> > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > <james.r.harris@intel.com> > 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. > > Stefan
On Mon, Oct 23, 2017 at 04:26:36AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > Sent: Friday, October 20, 2017 5:55 PM > > To: Liu, Changpeng <changpeng.liu@intel.com> > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > <james.r.harris@intel.com> > > 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. > > Stefan
On Mon, Oct 23, 2017 at 04:26:36AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > Sent: Friday, October 20, 2017 5:55 PM > > To: Liu, Changpeng <changpeng.liu@intel.com> > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > <james.r.harris@intel.com> > > 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. 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
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: Monday, October 23, 2017 8:55 PM > To: Liu, Changpeng <changpeng.liu@intel.com> > Cc: Stefan Hajnoczi <stefanha@gmail.com>; qemu-devel@nongnu.org; > pbonzini@redhat.com; marcandre.lureau@redhat.com; felipe@nutanix.com; > Harris, James R <james.r.harris@intel.com> > 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:stefanha@gmail.com] > > > Sent: Friday, October 20, 2017 5:55 PM > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > <james.r.harris@intel.com> > > > 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
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > Sent: Tuesday, October 24, 2017 1:12 AM > To: Liu, Changpeng <changpeng.liu@intel.com> > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > <james.r.harris@intel.com> > 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:stefanha@gmail.com] > > > Sent: Friday, October 20, 2017 5:55 PM > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > <james.r.harris@intel.com> > > > 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. > > 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
On Tue, Oct 24, 2017 at 12:44:30AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > Sent: Tuesday, October 24, 2017 1:12 AM > > To: Liu, Changpeng <changpeng.liu@intel.com> > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > <james.r.harris@intel.com> > > 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:stefanha@gmail.com] > > > > Sent: Friday, October 20, 2017 5:55 PM > > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > > <james.r.harris@intel.com> > > > > 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. > > > > 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 the hw/virtio/vhost.c code: uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits, uint64_t features) { const int *bit = feature_bits; while (*bit != VHOST_INVALID_FEATURE_BIT) { uint64_t bit_mask = (1ULL << *bit); if (!(hdev->features & bit_mask)) { features &= ~bit_mask; } bit++; } return features; } If f_readonly=false then features |= s->host_features will not contain f_readonly but it doesn't matter because if hdev->features (this value comes from the vhost slave) has f_readonly we will not clear the bit! So again, what is the purpose of the f_* properties? I don't understand the semantics, it seems useless.
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > Sent: Tuesday, October 24, 2017 8:53 PM > To: Liu, Changpeng <changpeng.liu@intel.com> > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > <james.r.harris@intel.com> > 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 +0000, Liu, Changpeng wrote: > > > > > > > -----Original Message----- > > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > > Sent: Tuesday, October 24, 2017 1:12 AM > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > <james.r.harris@intel.com> > > > 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:stefanha@gmail.com] > > > > > Sent: Friday, October 20, 2017 5:55 PM > > > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > > > <james.r.harris@intel.com> > > > > > 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. > > > > > > 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 the hw/virtio/vhost.c code: > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits, > uint64_t features) > { > const int *bit = feature_bits; > while (*bit != VHOST_INVALID_FEATURE_BIT) { > uint64_t bit_mask = (1ULL << *bit); > if (!(hdev->features & bit_mask)) { > features &= ~bit_mask; > } > bit++; > } > return features; > } > > If f_readonly=false then features |= s->host_features will not contain > f_readonly but it doesn't matter because if hdev->features (this value > comes from the vhost slave) has f_readonly we will not clear the bit! > > So again, what is the purpose of the f_* properties? I don't understand > the semantics, it seems useless. Hi Stefan, I hate this piece of code too, :). There are several feature definitions in VirtIODevice and vhost_dev, I printed the features in the function: vhost_user_blk_get_features, my test vhost-slave also provides RO feature, and I started Qemu with f_readonly=false: static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, uint64_t features, Error **errp) { VHostUserBlk *s = VHOST_USER_BLK(vdev); uint64_t get_features; /* Turn on pre-defined host features */ fprintf(stdout, "Features %"PRIx64"\n", features); features |= s->host_features; fprintf(stdout, "Turn On Predefined Features %"PRIx64"\n", features); fprintf(stdout, "VHOST Features %"PRIx64"\n", s->dev.features); get_features = vhost_get_features(&s->dev, user_feature_bits, features); fprintf(stdout, "Get Features %"PRIx64"\n", get_features); return get_features; } Here is the result, RO feature is bit5: Features 179000000, This is VirtIODevice->host_features. Turn On Predefined Features 179001ed7, RO bit is masked here. VHOST Features 141001466, vhost_dev-> features, you can see that RO bit is set here. Get Features 149001446, this is the final result, RO bit is masked finally. Similar with vhost-scsi and vhost-net, I think this part of logic is correct.
On Wed, Oct 25, 2017 at 01:34:39AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > Sent: Tuesday, October 24, 2017 8:53 PM > > To: Liu, Changpeng <changpeng.liu@intel.com> > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > <james.r.harris@intel.com> > > 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 +0000, Liu, Changpeng wrote: > > > > > > > > > > -----Original Message----- > > > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > > > Sent: Tuesday, October 24, 2017 1:12 AM > > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > > <james.r.harris@intel.com> > > > > 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:stefanha@gmail.com] > > > > > > Sent: Friday, October 20, 2017 5:55 PM > > > > > > To: Liu, Changpeng <changpeng.liu@intel.com> > > > > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > > > > <james.r.harris@intel.com> > > > > > > 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. > > > > > > > > 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 the hw/virtio/vhost.c code: > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits, > > uint64_t features) > > { > > const int *bit = feature_bits; > > while (*bit != VHOST_INVALID_FEATURE_BIT) { > > uint64_t bit_mask = (1ULL << *bit); > > if (!(hdev->features & bit_mask)) { > > features &= ~bit_mask; > > } > > bit++; > > } > > return features; > > } > > > > If f_readonly=false then features |= s->host_features will not contain > > f_readonly but it doesn't matter because if hdev->features (this value > > comes from the vhost slave) has f_readonly we will not clear the bit! > > > > So again, what is the purpose of the f_* properties? I don't understand > > the semantics, it seems useless. > Hi Stefan, > I hate this piece of code too, :). There are several feature definitions in VirtIODevice and vhost_dev, > I printed the features in the function: vhost_user_blk_get_features, my test vhost-slave also provides RO feature, and > I started Qemu with f_readonly=false: > > static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, > uint64_t features, > Error **errp) > { > VHostUserBlk *s = VHOST_USER_BLK(vdev); > uint64_t get_features; > > /* Turn on pre-defined host features */ > fprintf(stdout, "Features %"PRIx64"\n", features); > features |= s->host_features; > fprintf(stdout, "Turn On Predefined Features %"PRIx64"\n", features); > fprintf(stdout, "VHOST Features %"PRIx64"\n", s->dev.features); > > get_features = vhost_get_features(&s->dev, user_feature_bits, features); > fprintf(stdout, "Get Features %"PRIx64"\n", get_features); > > return get_features; > } > > Here is the result, RO feature is bit5: > Features 179000000, This is VirtIODevice->host_features. > Turn On Predefined Features 179001ed7, RO bit is masked here. > VHOST Features 141001466, vhost_dev-> features, you can see that RO bit is set here. > Get Features 149001446, this is the final result, RO bit is masked finally. > > Similar with vhost-scsi and vhost-net, I think this part of logic is correct. I see. I incorrectly thought vhost_dev->features is ORed at some stage. This means that vhost slaves can only influence the feature bits in one way: they can disable feature bits that QEMU knows about. Basically the feature bits are controlled by QEMU, not the slave. Stefan
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 attr attr 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) libiscsi iscsi 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 0000000..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 <stefanha@linux.vnet.ibm.com> + * Changpeng Liu <changpeng.liu@intel.com> + * + * This work is licensed under the terms of the GNU LGPL, version 2 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "qemu/error-report.h" +#include "qemu/typedefs.h" +#include "qemu/cutils.h" +#include "qom/object.h" +#include "hw/qdev-core.h" +#include "hw/virtio/vhost.h" +#include "hw/virtio/vhost-user-blk.h" +#include "hw/virtio/virtio.h" +#include "hw/virtio/virtio-bus.h" +#include "hw/virtio/virtio-access.h" + +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, + VIRTIO_BLK_F_MQ, + VIRTIO_BLK_F_RO, + VIRTIO_BLK_F_FLUSH, + VIRTIO_BLK_F_BARRIER, + VIRTIO_BLK_F_WCE, + VIRTIO_F_VERSION_1, + VIRTIO_RING_F_INDIRECT_DESC, + VIRTIO_RING_F_EVENT_IDX, + VIRTIO_F_NOTIFY_ON_EMPTY, + VHOST_INVALID_FEATURE_BIT +}; + +static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config) +{ + VHostUserBlk *s = VHOST_USER_BLK(vdev); + + memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config)); +} + +static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config) +{ + VHostUserBlk *s = VHOST_USER_BLK(vdev); + struct virtio_blk_config *blkcfg = (struct virtio_blk_config *)config; + int ret; + + if (blkcfg->wce == s->blkcfg.wce) { + return; + } + + ret = vhost_dev_set_config(&s->dev, config, + sizeof(struct virtio_blk_config)); + if (ret) { + error_report("set device config space failed"); + return; + } + + s->blkcfg.wce = blkcfg->wce; +} + +static void vhost_user_blk_handle_config_change(struct vhost_dev *dev) +{ + int ret; + struct virtio_blk_config blkcfg; + VHostUserBlk *s = VHOST_USER_BLK(dev->vdev); + + ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg, + sizeof(struct virtio_blk_config)); + if (ret < 0) { + error_report("get config space failed"); + return; + } + + memcpy(&s->blkcfg, &blkcfg, sizeof(struct virtio_blk_config)); + memcpy(dev->vdev->config, &blkcfg, sizeof(struct virtio_blk_config)); + + virtio_notify_config(dev->vdev); +} + +const VhostDevConfigOps blk_ops = { + .vhost_dev_config_notifier = vhost_user_blk_handle_config_change, +}; + +static void vhost_user_blk_start(VirtIODevice *vdev) +{ + VHostUserBlk *s = VHOST_USER_BLK(vdev); + BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); + int i, ret; + + if (!k->set_guest_notifiers) { + error_report("binding does not support guest notifiers"); + return; + } + + ret = vhost_dev_enable_notifiers(&s->dev, vdev); + if (ret < 0) { + error_report("Error enabling host notifiers: %d", -ret); + return; + } + + ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true); + if (ret < 0) { + error_report("Error binding guest notifier: %d", -ret); + goto err_host_notifiers; + } + + s->dev.acked_features = vdev->guest_features; + ret = vhost_dev_start(&s->dev, vdev); + if (ret < 0) { + error_report("Error starting vhost: %d", -ret); + goto err_guest_notifiers; + } + + /* guest_notifier_mask/pending not used yet, so just unmask + * everything here. virtio-pci will do the right thing by + * enabling/disabling irqfd. + */ + for (i = 0; i < s->dev.nvqs; i++) { + vhost_virtqueue_mask(&s->dev, vdev, i, false); + } + + return; + +err_guest_notifiers: + k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false); +err_host_notifiers: + vhost_dev_disable_notifiers(&s->dev, vdev); +} + +static void vhost_user_blk_stop(VirtIODevice *vdev) +{ + VHostUserBlk *s = VHOST_USER_BLK(vdev); + BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); + int ret; + + if (!k->set_guest_notifiers) { + return; + } + + vhost_dev_stop(&s->dev, vdev); + + ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false); + if (ret < 0) { + error_report("vhost guest notifier cleanup failed: %d", ret); + return; + } + + vhost_dev_disable_notifiers(&s->dev, vdev); +} + +static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) +{ + VHostUserBlk *s = VHOST_USER_BLK(vdev); + bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK; + + if (!vdev->vm_running) { + should_start = false; + } + + if (s->dev.started == should_start) { + return; + } + + if (should_start) { + vhost_user_blk_start(vdev); + } else { + vhost_user_blk_stop(vdev); + } + +} + +static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, + uint64_t features, + Error **errp) +{ + VHostUserBlk *s = VHOST_USER_BLK(vdev); + uint64_t get_features; + + /* Turn on pre-defined features */ + features |= s->host_features; + + get_features = vhost_get_features(&s->dev, user_feature_bits, features); + + return get_features; +} + +static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) +{ + +} + +static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(dev); + VHostUserBlk *s = VHOST_USER_BLK(vdev); + int i, ret; + + if (!s->chardev.chr) { + error_setg(errp, "vhost-user-blk: chardev is mandatory"); + return; + } + + if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) { + error_setg(errp, "vhost-user-blk: invalid number of IO queues"); + return; + } + + if (!s->queue_size) { + error_setg(errp, "vhost-user-blk: queue size must be non-zero"); + return; + } + + virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, + sizeof(struct virtio_blk_config)); + + for (i = 0; i < s->num_queues; i++) { + virtio_add_queue(vdev, s->queue_size, + vhost_user_blk_handle_output); + } + + s->dev.nvqs = s->num_queues; + s->dev.vqs = g_new(struct vhost_virtqueue, s->dev.nvqs); + s->dev.vq_index = 0; + s->dev.backend_features = 0; + + ret = vhost_dev_init(&s->dev, &s->chardev, VHOST_BACKEND_TYPE_USER, 0); + if (ret < 0) { + error_setg(errp, "vhost-user-blk: vhost initialization failed: %s", + strerror(-ret)); + goto virtio_err; + } + + ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, + sizeof(struct virtio_blk_config)); + if (ret < 0) { + error_setg(errp, "vhost-user-blk: get block config failed"); + goto vhost_err; + } + + if (s->blkcfg.num_queues != s->num_queues) { + s->blkcfg.num_queues = s->num_queues; + } + + vhost_dev_set_config_notifier(&s->dev, &blk_ops); + + return; + +vhost_err: + vhost_dev_cleanup(&s->dev); +virtio_err: + g_free(s->dev.vqs); + virtio_cleanup(vdev); +} + +static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(dev); + VHostUserBlk *s = VHOST_USER_BLK(dev); + + vhost_user_blk_set_status(vdev, 0); + vhost_dev_cleanup(&s->dev); + g_free(s->dev.vqs); + virtio_cleanup(vdev); +} + +static void vhost_user_blk_instance_init(Object *obj) +{ + VHostUserBlk *s = VHOST_USER_BLK(obj); + + device_add_bootindex_property(obj, &s->bootindex, "bootindex", + "/disk@0,0", DEVICE(obj), NULL); +} + +static const VMStateDescription vmstate_vhost_user_blk = { + .name = "vhost-user-blk", + .minimum_version_id = 1, + .version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_VIRTIO_DEVICE, + VMSTATE_END_OF_LIST() + }, +}; + +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), + DEFINE_PROP_END_OF_LIST(), +}; + +static void vhost_user_blk_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass); + + dc->props = vhost_user_blk_properties; + dc->vmsd = &vmstate_vhost_user_blk; + set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + vdc->realize = vhost_user_blk_device_realize; + vdc->unrealize = vhost_user_blk_device_unrealize; + vdc->get_config = vhost_user_blk_update_config; + vdc->set_config = vhost_user_blk_set_config; + vdc->get_features = vhost_user_blk_get_features; + vdc->set_status = vhost_user_blk_set_status; +} + +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, +}; + +static void virtio_register_types(void) +{ + type_register_static(&vhost_user_blk_info); +} + +type_init(virtio_register_types) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index e92837c..174939f 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1976,6 +1976,58 @@ static const TypeInfo virtio_blk_pci_info = { .class_init = virtio_blk_pci_class_init, }; +#ifdef CONFIG_VHOST_USER_BLK +/* vhost-user-blk */ + +static Property vhost_user_blk_pci_properties[] = { + DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), + DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), + DEFINE_PROP_END_OF_LIST(), +}; + +static void vhost_user_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) +{ + VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(vpci_dev); + DeviceState *vdev = DEVICE(&dev->vdev); + + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); +} + +static void vhost_user_blk_pci_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass); + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass); + + set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + dc->props = vhost_user_blk_pci_properties; + k->realize = vhost_user_blk_pci_realize; + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET; + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK; + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION; + pcidev_k->class_id = PCI_CLASS_STORAGE_SCSI; +} + +static void vhost_user_blk_pci_instance_init(Object *obj) +{ + VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(obj); + + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev), + TYPE_VHOST_USER_BLK); + object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev), + "bootindex", &error_abort); +} + +static const TypeInfo vhost_user_blk_pci_info = { + .name = TYPE_VHOST_USER_BLK_PCI, + .parent = TYPE_VIRTIO_PCI, + .instance_size = sizeof(VHostUserBlkPCI), + .instance_init = vhost_user_blk_pci_instance_init, + .class_init = vhost_user_blk_pci_class_init, +}; +#endif + /* virtio-scsi-pci */ static Property virtio_scsi_pci_properties[] = { @@ -2622,6 +2674,9 @@ static void virtio_pci_register_types(void) type_register_static(&virtio_9p_pci_info); #endif type_register_static(&virtio_blk_pci_info); +#ifdef CONFIG_VHOST_USER_BLK + type_register_static(&vhost_user_blk_pci_info); +#endif type_register_static(&virtio_scsi_pci_info); type_register_static(&virtio_balloon_pci_info); type_register_static(&virtio_serial_pci_info); diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 12d3a90..a72c1d4 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -27,6 +27,9 @@ #include "hw/virtio/virtio-gpu.h" #include "hw/virtio/virtio-crypto.h" #include "hw/virtio/vhost-user-scsi.h" +#ifdef CONFIG_VHOST_USER_BLK +#include "hw/virtio/vhost-user-blk.h" +#endif #ifdef CONFIG_VIRTFS #include "hw/9pfs/virtio-9p.h" @@ -46,6 +49,7 @@ typedef struct VirtIOSerialPCI VirtIOSerialPCI; typedef struct VirtIONetPCI VirtIONetPCI; typedef struct VHostSCSIPCI VHostSCSIPCI; typedef struct VHostUserSCSIPCI VHostUserSCSIPCI; +typedef struct VHostUserBlkPCI VHostUserBlkPCI; typedef struct VirtIORngPCI VirtIORngPCI; typedef struct VirtIOInputPCI VirtIOInputPCI; typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI; @@ -244,6 +248,20 @@ struct VHostUserSCSIPCI { VHostUserSCSI vdev; }; +#ifdef CONFIG_VHOST_USER_BLK +/* + * vhost-user-blk-pci: This extends VirtioPCIProxy. + */ +#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci" +#define VHOST_USER_BLK_PCI(obj) \ + OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI) + +struct VHostUserBlkPCI { + VirtIOPCIProxy parent_obj; + VHostUserBlk vdev; +}; +#endif + /* * virtio-blk-pci: This extends VirtioPCIProxy. */ diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h new file mode 100644 index 0000000..77d20f0 --- /dev/null +++ b/include/hw/virtio/vhost-user-blk.h @@ -0,0 +1,40 @@ +/* + * vhost-user-blk host device + * Copyright IBM, Corp. 2011 + * Copyright(C) 2017 Intel Corporation. + * + * Authors: + * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> + * Changpeng Liu <changpeng.liu@intel.com> + * + * This work is licensed under the terms of the GNU LGPL, version 2 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#ifndef VHOST_USER_BLK_H +#define VHOST_USER_BLK_H + +#include "standard-headers/linux/virtio_blk.h" +#include "qemu-common.h" +#include "hw/qdev.h" +#include "hw/block/block.h" +#include "chardev/char-fe.h" +#include "hw/virtio/vhost.h" + +#define TYPE_VHOST_USER_BLK "vhost-user-blk" +#define VHOST_USER_BLK(obj) \ + OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK) + +typedef struct VHostUserBlk { + VirtIODevice parent_obj; + CharBackend chardev; + int32_t bootindex; + uint64_t host_features; + struct virtio_blk_config blkcfg; + uint16_t num_queues; + uint32_t queue_size; + struct vhost_dev dev; +} VHostUserBlk; + +#endif
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 <changpeng.liu@intel.com> --- 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