On Wed, Sep 09, 2015 at 01:45:50PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
> > From: Changchun Ouyang <changchun.ouy...@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.ouy...@intel.com>
> > Signed-off-by: Yuanhan Liu <yuanhan....@linux.intel.com>
>
> Fine, but I would like to make this dependent on the MQ flag.
Reasonable.
>
>
> > ---
> > 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.
Okay.
>
> > +
> > + 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.
Okay.
>
> > + 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.
Got it, thanks.
>
> > + 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,
Yeah, 81 chars. I thougth that's kind of acceptable. But, anyway, since
you pointed it out, I will fix it next time.
> and should use {}.
TBH, I seldom use {} for one statement, and you could see that a lot from
this patch set. Is it a must in qemu community? If so, I could fix them all.
>
> > + }
> > +
> > + 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.
Agreed. I will make it vhost-usre specific then.
>
>
> > 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.
Let me try.
>
>
> > @@ -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?
To define it as `vhost_backend_enable_vring'?
Thanks.
--yliu
>
> > } 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
