Re: [Qemu-devel] [PATCH v2] Avoid additional GET_FEATURES call on vhost-user

[Michael S. Tsirkin]

On Thu, Sep 22, 2016 at 01:13:41PM +0100, Felipe Franciosi wrote:
> Vhost-user requires an early GET_FEATURES call to determine if the
> slave supports protocol feature negotiation. An extra GET_FEATURES
> call is made after vhost_backend_init() to actually set the device
> features.
> 
> This patch moves the actual setting of the device features to both
> implementations (kernel/user) of vhost_backend_init(), therefore
> eliminating the need for a second call.
> 
> Signed-off-by: Felipe Franciosi <address@hidden>

Maxime, you changes things around feature negotiation
recently, mind reviewing this?

> ---
> v2. Rebase on d1b4259f
> 
>  hw/virtio/vhost-backend.c | 27 ++++++++++++++++++---------
>  hw/virtio/vhost-user.c    |  1 +
>  hw/virtio/vhost.c         |  9 ---------
>  3 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 272a5ec..0ffa4d1 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -25,15 +25,6 @@ static int vhost_kernel_call(struct vhost_dev *dev, 
> unsigned long int request,
>      return ioctl(fd, request, arg);
>  }
>  
> -static int vhost_kernel_init(struct vhost_dev *dev, void *opaque)
> -{
> -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
> -
> -    dev->opaque = opaque;
> -
> -    return 0;
> -}
> -
>  static int vhost_kernel_cleanup(struct vhost_dev *dev)
>  {
>      int fd = (uintptr_t) dev->opaque;
> @@ -185,6 +176,24 @@ static int vhost_kernel_vsock_set_running(struct 
> vhost_dev *dev, int start)
>  }
>  #endif /* CONFIG_VHOST_VSOCK */
>  
> +static int vhost_kernel_init(struct vhost_dev *dev, void *opaque)
> +{
> +    uint64_t features;
> +    int r;
> +
> +    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
> +
> +    dev->opaque = opaque;
> +
> +    r = vhost_kernel_get_features(dev, &features);
> +    if (r < 0) {
> +        return r;
> +    }
> +    dev->features = features;
> +
> +    return 0;
> +}
> +
>  static const VhostOps kernel_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
>          .vhost_backend_init = vhost_kernel_init,
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index b57454a..684f6d7 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -578,6 +578,7 @@ static int vhost_user_init(struct vhost_dev *dev, void 
> *opaque)
>      if (err < 0) {
>          return err;
>      }
> +    dev->features = features;
>  
>      if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>          dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index bd051ab..36fd35f 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1051,7 +1051,6 @@ static void vhost_virtqueue_cleanup(struct 
> vhost_virtqueue *vq)
>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>                     VhostBackendType backend_type, uint32_t busyloop_timeout)
>  {
> -    uint64_t features;
>      int i, r, n_initialized_vqs = 0;
>  
>      hdev->migration_blocker = NULL;
> @@ -1077,12 +1076,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
> *opaque,
>          goto fail;
>      }
>  
> -    r = hdev->vhost_ops->vhost_get_features(hdev, &features);
> -    if (r < 0) {
> -        VHOST_OPS_DEBUG("vhost_get_features failed");
> -        goto fail;
> -    }
> -
>      for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>          r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
>          if (r < 0) {
> @@ -1100,8 +1093,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>          }
>      }
>  
> -    hdev->features = features;
> -
>      hdev->memory_listener = (MemoryListener) {
>          .begin = vhost_begin,
>          .commit = vhost_commit,
> -- 
> 1.9.5