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

[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 <address@hidden>
> ---
>  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 <address@hidden>
> + *  Changpeng Liu <address@hidden>

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?