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

[Marcel Apfelbaum <marcel@redhat.com>]

On 09/23/2015 07:19 AM, Yuanhan Liu wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> 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.
>
> [ leverage vhost_user_call for request handling -- Yuanhan Liu ]
>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>   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

VHOST_USER_GET_PROTOCOL_FEATURES does not have an equivalent ioctl

> +      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

Same here

> +      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 1d76b94..9d32d76 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;
>       }

Maybe protocol_features assignment should be outside the if clause.
(assigned to 0 in both cases)

>       net->nc = options->net_backend;
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 13677ac..7fe35c6 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)) {
> +        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> +
> +        err = vhost_user_call(dev, VHOST_USER_GET_PROTOCOL_FEATURES, &features);
> +        if (err < 0) {
> +            return err;
> +        }
> +
> +        dev->protocol_features = features & VHOST_USER_PROTOCOL_FEATURE_MASK;
> +        err = vhost_user_call(dev, VHOST_USER_SET_PROTOCOL_FEATURES,
> +                              &dev->protocol_features);
> +        if (err < 0) {
> +            return err;
> +        }
> +    }
> +
>       return 0;
>   }
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index dd51050..6467c73 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -47,6 +47,7 @@ struct vhost_dev {
>       unsigned long long features;
>       unsigned long long acked_features;
>       unsigned long long backend_features;
> +    unsigned long long protocol_features;
>       bool started;
>       bool log_enabled;
>       unsigned long long log_size;
>

The above comments can be addressed on top of this series because
does not affect the correctness of the patch.

-- 
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>