Re: [Qemu-devel] [PATCH 2/7] vhost-user: add protocol feature negotiation

[Yuanhan Liu]

On Thu, Sep 17, 2015 at 03:12:55PM +0800, Jason Wang wrote:
> 
> 
> On 09/15/2015 03:10 PM, Yuanhan Liu wrote:
> > From: "Michael S. Tsirkin" <address@hidden>
> >
> > Support a separate bitmask for vhost-user protocol features,
> > and messages to get/set protocol features.
> >
> > Invoke them at init.
> >
> > No features are defined yet.
> >
> > v2: leverage vhost_user_call for request handling -- Yuanhan Liu
> >
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Yuanhan Liu <address@hidden>
> > ---
> >  docs/specs/vhost-user.txt | 37 +++++++++++++++++++++++++++++++++++++
> >  hw/net/vhost_net.c        |  2 ++
> >  hw/virtio/vhost-user.c    | 31 +++++++++++++++++++++++++++++++
> >  include/hw/virtio/vhost.h |  1 +
> >  4 files changed, 71 insertions(+)
> >
> > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > index 650bb18..70da3b1 100644
> > --- a/docs/specs/vhost-user.txt
> > +++ b/docs/specs/vhost-user.txt
> > @@ -113,6 +113,7 @@ message replies. Most of the requests don't require 
> > replies. Here is a list of
> >  the ones that do:
> >  
> >   * VHOST_GET_FEATURES
> > + * VHOST_GET_PROTOCOL_FEATURES
> >   * VHOST_GET_VRING_BASE
> >  
> >  There are several messages that the master sends with file descriptors 
> > passed
> > @@ -127,6 +128,13 @@ in the ancillary data:
> >  If Master is unable to send the full message or receives a wrong reply it 
> > will
> >  close the connection. An optional reconnection mechanism can be 
> > implemented.
> >  
> > +Any protocol extensions are gated by protocol feature bits,
> > +which allows full backwards compatibility on both master
> > +and slave.
> > +As older slaves don't support negotiating protocol features,
> > +a feature bit was dedicated for this purpose:
> > +#define VHOST_USER_F_PROTOCOL_FEATURES 30
> > +
> >  Message types
> >  -------------
> >  
> > @@ -138,6 +146,8 @@ Message types
> >        Slave payload: u64
> >  
> >        Get from the underlying vhost implementation the features bitmask.
> > +      Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals slave support for
> > +      VHOST_USER_GET_PROTOCOL_FEATURES and 
> > VHOST_USER_SET_PROTOCOL_FEATURES.
> >  
> >   * VHOST_USER_SET_FEATURES
> >  
> > @@ -146,6 +156,33 @@ Message types
> >        Master payload: u64
> >  
> >        Enable features in the underlying vhost implementation using a 
> > bitmask.
> > +      Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals slave support for
> > +      VHOST_USER_GET_PROTOCOL_FEATURES and 
> > VHOST_USER_SET_PROTOCOL_FEATURES.
> > +
> > + * VHOST_USER_GET_PROTOCOL_FEATURES
> > +
> > +      Id: 15
> > +      Equivalent ioctl: VHOST_GET_FEATURES
> > +      Master payload: N/A
> > +      Slave payload: u64
> > +
> > +      Get the protocol feature bitmask from the underlying vhost 
> > implementation.
> > +      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present 
> > in
> > +      VHOST_USER_GET_FEATURES.
> > +      Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
> > +      this message even before VHOST_USER_SET_FEATURES was called.
> > +
> > + * VHOST_USER_SET_PROTOCOL_FEATURES
> > +
> > +      Id: 16
> > +      Ioctl: VHOST_SET_FEATURES
> > +      Master payload: u64
> > +
> > +      Enable protocol features in the underlying vhost implementation.
> > +      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present 
> > in
> > +      VHOST_USER_GET_FEATURES.
> > +      Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
> > +      this message even before VHOST_USER_SET_FEATURES was called.
> >  
> >   * VHOST_USER_SET_OWNER
> >  
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 5c1d11f..c864237 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -152,8 +152,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions 
> > *options)
> >          net->dev.backend_features = qemu_has_vnet_hdr(options->net_backend)
> >              ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
> >          net->backend = r;
> > +        net->dev.protocol_features = 0;
> >      } else {
> >          net->dev.backend_features = 0;
> > +        net->dev.protocol_features = 0;
> >          net->backend = -1;
> >      }
> >      net->nc = options->net_backend;
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 13677ac..43169a1 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -24,6 +24,8 @@
> >  #include <linux/vhost.h>
> >  
> >  #define VHOST_MEMORY_MAX_NREGIONS    8
> > +#define VHOST_USER_F_PROTOCOL_FEATURES 30
> > +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
> >  
> >  typedef enum VhostUserRequest {
> >      VHOST_USER_NONE = 0,
> > @@ -41,6 +43,8 @@ typedef enum VhostUserRequest {
> >      VHOST_USER_SET_VRING_KICK = 12,
> >      VHOST_USER_SET_VRING_CALL = 13,
> >      VHOST_USER_SET_VRING_ERR = 14,
> > +    VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> > +    VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> >      VHOST_USER_MAX
> >  } VhostUserRequest;
> >  
> > @@ -206,11 +210,13 @@ static int vhost_user_call(struct vhost_dev *dev, 
> > unsigned long int request,
> >  
> >      switch (msg_request) {
> >      case VHOST_USER_GET_FEATURES:
> > +    case VHOST_USER_GET_PROTOCOL_FEATURES:
> >          need_reply = 1;
> >          break;
> >  
> >      case VHOST_USER_SET_FEATURES:
> >      case VHOST_USER_SET_LOG_BASE:
> > +    case VHOST_USER_SET_PROTOCOL_FEATURES:
> >          msg.u64 = *((__u64 *) arg);
> >          msg.size = sizeof(m.u64);
> >          break;
> > @@ -308,6 +314,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
> > unsigned long int request,
> >  
> >          switch (msg_request) {
> >          case VHOST_USER_GET_FEATURES:
> > +        case VHOST_USER_GET_PROTOCOL_FEATURES:
> >              if (msg.size != sizeof(m.u64)) {
> >                  error_report("Received bad msg size.");
> >                  return -1;
> > @@ -333,10 +340,34 @@ static int vhost_user_call(struct vhost_dev *dev, 
> > unsigned long int request,
> >  
> >  static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> >  {
> > +    unsigned long long features;
> > +    int err;
> > +
> >      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >  
> >      dev->opaque = opaque;
> >  
> > +    err = vhost_user_call(dev, VHOST_USER_GET_FEATURES, &features);
> > +    if (err < 0) {
> > +        return err;
> > +    }
> > +
> > +    if (__virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> 
> Need rebase to master. (And use virtio_has_feature() here).

Thanks. Will fix them in next version.

        --yliu