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

[Felipe Franciosi]

> On 19 Sep 2016, at 18:42, Michael S. Tsirkin <m...@redhat.com> wrote:
> 
> Thanks for the reminder. 2.8 is open now so I can integrate this.
> I would appreciate it if you could rebase this top of
> 
>   commit d1b4259f1ab18af24e6a297edb6a8f71691f3256
>   Author: Maxime Coquelin <maxime.coque...@redhat.com>
>   Date:   Tue Sep 13 15:30:30 2016 +0200
>   virtio-bus: Plug devices after features are negotiated
> 
> test and repost if it still works.
> 
> Thanks!

No problem! Done! :)

http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05582.html

Felipe

> 
> 
> On Mon, Sep 19, 2016 at 05:25:49PM +, Felipe Franciosi wrote:
>> Gentle ping.
>> 
>> 
>> 
>> Thanks,
>> 
>> Felipe
>> 
>> 
>> 
>> From: Marc-André Lureau <marcandre.lur...@gmail.com>
>> Date: Thursday, 25 August 2016 at 12:14
>> To: Felipe Franciosi <fel...@nutanix.com>, "qemu-devel@nongnu.org"
>> <qemu-devel@nongnu.org>, "Michael S. Tsirkin" <m...@redhat.com>
>> Subject: Re: [Qemu-devel] [PATCH] Avoid additional GET_FEATURES call on
>> vhost-user
>> 
>> 
>> 
>>Hi
>> 
>> 
>> 
>>On Sat, Aug 20, 2016 at 3:28 AM Felipe Franciosi <fel...@nutanix.com>
>>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 <fel...@nutanix.com>
>> 
>> 
>> 
>>Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> 
>> 
>> 
>>---
>> 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 7681f15..a519fe2 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;
>>@@ -172,6 +163,24 @@ static int vhost_kernel_get_vq_index(struct
>>vhost_dev *dev, int idx)
>> return idx - dev->vq_index;
>> }
>> 
>>+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, );
>>+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_featur

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

[Felipe Franciosi]

Gentle ping.

Thanks,
Felipe

From: Marc-André Lureau <marcandre.lur...@gmail.com>
Date: Thursday, 25 August 2016 at 12:14
To: Felipe Franciosi <fel...@nutanix.com>, "qemu-devel@nongnu.org" 
<qemu-devel@nongnu.org>, "Michael S. Tsirkin" <m...@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] Avoid additional GET_FEATURES call on 
vhost-user

Hi

On Sat, Aug 20, 2016 at 3:28 AM Felipe Franciosi 
<fel...@nutanix.com<mailto:fel...@nutanix.com>> 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 <fel...@nutanix.com<mailto:fel...@nutanix.com>>


Reviewed-by: Marc-André Lureau 
<marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com>>

---
 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 7681f15..a519fe2 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;
@@ -172,6 +163,24 @@ static int vhost_kernel_get_vq_index(struct vhost_dev 
*dev, int idx)
 return idx - dev->vq_index;
 }

+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, );
+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 3d0c807..cb9870a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1037,7 +1037,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;
@@ -1063,12 +1062,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 goto fail;
 }

-r = hdev->vhost_ops->vhost_get_features(hdev, );
-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) {
@@ -1086,8 +1079,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

--
Marc-André Lureau

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

[Michael S. Tsirkin]

Thanks for the reminder. 2.8 is open now so I can integrate this.
I would appreciate it if you could rebase this top of

commit d1b4259f1ab18af24e6a297edb6a8f71691f3256
Author: Maxime Coquelin <maxime.coque...@redhat.com>
Date:   Tue Sep 13 15:30:30 2016 +0200
virtio-bus: Plug devices after features are negotiated

test and repost if it still works.

Thanks!


On Mon, Sep 19, 2016 at 05:25:49PM +, Felipe Franciosi wrote:
> Gentle ping.
> 
>  
> 
> Thanks,
> 
> Felipe
> 
>  
> 
> From: Marc-André Lureau <marcandre.lur...@gmail.com>
> Date: Thursday, 25 August 2016 at 12:14
> To: Felipe Franciosi <fel...@nutanix.com>, "qemu-devel@nongnu.org"
> <qemu-devel@nongnu.org>, "Michael S. Tsirkin" <m...@redhat.com>
> Subject: Re: [Qemu-devel] [PATCH] Avoid additional GET_FEATURES call on
> vhost-user
> 
>  
> 
> Hi
> 
>  
> 
> On Sat, Aug 20, 2016 at 3:28 AM Felipe Franciosi <fel...@nutanix.com>
> 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 <fel...@nutanix.com>
> 
> 
> 
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> 
>  
> 
> ---
>  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 7681f15..a519fe2 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;
> @@ -172,6 +163,24 @@ static int vhost_kernel_get_vq_index(struct
> vhost_dev *dev, int idx)
>  return idx - dev->vq_index;
>  }
> 
> +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, );
> +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 3d0c807..cb9870a 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1037,7 +1037,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

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

[Marc-André Lureau]

Hi

On Sat, Aug 20, 2016 at 3:28 AM 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 
>


Reviewed-by: Marc-André Lureau 


> ---
>  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 7681f15..a519fe2 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;
> @@ -172,6 +163,24 @@ static int vhost_kernel_get_vq_index(struct vhost_dev
> *dev, int idx)
>  return idx - dev->vq_index;
>  }
>
> +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, );
> +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 3d0c807..cb9870a 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1037,7 +1037,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;
> @@ -1063,12 +1062,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> *opaque,
>  goto fail;
>  }
>
> -r = hdev->vhost_ops->vhost_get_features(hdev, );
> -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) {
> @@ -1086,8 +1079,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
>
>
> --
Marc-André Lureau

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

[Felipe Franciosi]

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 
---
 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 7681f15..a519fe2 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;
@@ -172,6 +163,24 @@ static int vhost_kernel_get_vq_index(struct vhost_dev 
*dev, int idx)
 return idx - dev->vq_index;
 }
 
+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, );
+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 3d0c807..cb9870a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1037,7 +1037,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;
@@ -1063,12 +1062,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 goto fail;
 }
 
-r = hdev->vhost_ops->vhost_get_features(hdev, );
-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) {
@@ -1086,8 +1079,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