Re: [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
> From: Changchun Ouyang <changchun.ouyang@intel.com>
> 
> Add a new message, VHOST_USER_SET_VRING_FLAG, to enable and disable
> a specific virt queue, which is similar to attach/detach queue for
> tap device.
> 
> virtio driver on guest doesn't have to use max virt queue pair, it
> could enable any number of virt queue ranging from 1 to max virt
> queue pair.
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Fine, but I would like to make this dependent on the MQ flag.


> ---
>  docs/specs/vhost-user.txt         | 10 ++++++++++
>  hw/net/vhost_net.c                | 18 ++++++++++++++++++
>  hw/net/virtio-net.c               |  2 ++
>  hw/virtio/vhost-user.c            | 32 ++++++++++++++++++++++++++++----
>  include/hw/virtio/vhost-backend.h |  2 ++
>  include/net/vhost_net.h           |  1 +
>  6 files changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 99d79be..c2c38f3 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -325,3 +325,13 @@ Message types
>        Query how many queues the backend supports. This request should be
>        sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
>        features by VHOST_USER_GET_PROTOCOL_FEATURES.
> +
> + * VHOST_USER_SET_VRING_FLAG


SET_VRIHG_ENABLE would be a better name.

> +
> +      Id: 18
> +      Equivalent ioctl: N/A
> +      Master payload: vring state description
> +
> +      Set the flag(1 for enable and 0 for disable) to signal slave to enable

Space before ( please.

> +      or disable corresponding virt queue. This request should be sent only
> +      when the protocol feature bit VHOST_USER_PROTOCOL_F_VRING_FLAG is set.
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 141b557..7d9cc8d 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -418,6 +418,19 @@ VHostNetState *get_vhost_net(NetClientState *nc)
>  
>      return vhost_net;
>  }
> +
> +int vhost_set_vring_flag(NetClientState *nc, int enable)
> +{
> +    if (nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> +        VHostNetState *net = get_vhost_net(nc);
> +        const VhostOps *vhost_ops = net->dev.vhost_ops;

add an empty line after declaration pls.

> +        if (vhost_ops->vhost_backend_mq_set_vring_flag)
> +            return vhost_ops->vhost_backend_mq_set_vring_flag(&net->dev, enable);

Coding style violation:
	line too long, and should use {}.

> +    }
> +
> +    return 0;
> +}
> +
>  #else
>  struct vhost_net *vhost_net_init(VhostNetOptions *options)
>  {
> @@ -463,4 +476,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
>  {
>      return 0;
>  }
> +
> +int vhost_set_vring_flag(NetClientState *nc, int enable)
> +{
> +    return 0;
> +}
>  #endif
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 8d28e45..53f93b1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -407,6 +407,7 @@ static int peer_attach(VirtIONet *n, int index)
>      }
>  
>      if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> +        vhost_set_vring_flag(nc->peer, 1);
>          return 0;
>      }
>  
> @@ -422,6 +423,7 @@ static int peer_detach(VirtIONet *n, int index)
>      }
>  
>      if (nc->peer->info->type !=  NET_CLIENT_OPTIONS_KIND_TAP) {
> +        vhost_set_vring_flag(nc->peer, 0);
>          return 0;
>      }
>

makes no sense to call it for !== tap only.
Either call this unconditionally, or just for vhost user.

  
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 11e46b5..ca6f7fa 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -25,9 +25,10 @@
>  
>  #define VHOST_MEMORY_MAX_NREGIONS    8
>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
> -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0xfULL
>  
> -#define VHOST_USER_PROTOCOL_F_MQ    0
> +#define VHOST_USER_PROTOCOL_F_MQ            0
> +#define VHOST_USER_PROTOCOL_F_VRING_FLAG    1
>  
>  typedef enum VhostUserRequest {
>      VHOST_USER_NONE = 0,
> @@ -48,6 +49,7 @@ typedef enum VhostUserRequest {
>      VHOST_USER_GET_PROTOCOL_FEATURES = 15,
>      VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>      VHOST_USER_GET_QUEUE_NUM = 17,
> +    VHOST_USER_SET_VRING_FLAG = 18,
>      VHOST_USER_MAX
>  } VhostUserRequest;
>  
> @@ -296,6 +298,11 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          msg.addr.index += dev->vq_index;
>          msg.size = sizeof(m.state);
>          break;
> +    case VHOST_USER_SET_VRING_FLAG:
> +        msg.state.index = dev->vq_index;
> +        msg.state.num = *(int*)arg;
> +        msg.size = sizeof(msg.state);
> +        break;
>  
>      case VHOST_USER_GET_VRING_BASE:
>          memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> @@ -408,6 +415,22 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
>      return 0;
>  }
>  
> +static int vhost_user_set_vring_flag(struct vhost_dev *dev, int enable)
> +{
> +    int err;
> +
> +    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> +
> +    if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_VRING_FLAG)))
> +        return -1;
> +
> +    err = vhost_user_call(dev, VHOST_USER_SET_VRING_FLAG, &enable);
> +    if (err < 0)
> +        return err;
> +
> +    return 0;
> +}
> +
>  static int vhost_user_cleanup(struct vhost_dev *dev)
>  {
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);

In fact, it's a per VQ pair call the way it's coded.
So pls fix it, call it for all VQs.


> @@ -421,5 +444,6 @@ const VhostOps user_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_USER,
>          .vhost_call = vhost_user_call,
>          .vhost_backend_init = vhost_user_init,
> -        .vhost_backend_cleanup = vhost_user_cleanup
> -        };
> +        .vhost_backend_cleanup = vhost_user_cleanup,
> +        .vhost_backend_mq_set_vring_flag = vhost_user_set_vring_flag,
> +};
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index e472f29..6fc76b7 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
>               void *arg);
>  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
>  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> +typedef int (*vhost_backend_mq_set_vring_flag)(struct vhost_dev *dev, int enable);
>  
>  typedef struct VhostOps {
>      VhostBackendType backend_type;
>      vhost_call vhost_call;
>      vhost_backend_init vhost_backend_init;
>      vhost_backend_cleanup vhost_backend_cleanup;
> +    vhost_backend_mq_set_vring_flag vhost_backend_mq_set_vring_flag;

That's quite ugly: mq is a vhost net thing.
Why not enable each VQ?

>  } VhostOps;
>  
>  extern const VhostOps user_ops;
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 8db723e..3ec33ff 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -28,4 +28,5 @@ bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
>  void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
>                                int idx, bool mask);
>  VHostNetState *get_vhost_net(NetClientState *nc);
> +int vhost_set_vring_flag(NetClientState * nc, int enable);
>  #endif
> -- 
> 1.9.0