[Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"

[Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"

[Michael S. Tsirkin]

This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76.

In case of live migration several queues can be enabled and not only the
first one. So informing backend that only the first queue is enabled is
wrong.

Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/vhost.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1794f0d..de29968 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
     }
 
-    if (hdev->vhost_ops->vhost_set_vring_enable) {
-        /* only enable first vq pair by default */
-        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
-    }
-
     return 0;
 fail_log:
     vhost_log_put(hdev, false);
@@ -1261,10 +1256,6 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
                              hdev->vq_index + i);
     }
 
-    if (hdev->vhost_ops->vhost_set_vring_enable) {
-        hdev->vhost_ops->vhost_set_vring_enable(hdev, 0);
-    }
-
     vhost_log_put(hdev, true);
     hdev->started = false;
     hdev->log = NULL;
-- 
MST

Re: [Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"

[Thibaut Collet]

On Wed, Nov 25, 2015 at 1:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76.
>
> In case of live migration several queues can be enabled and not only the
> first one. So informing backend that only the first queue is enabled is
> wrong.
>
> Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/vhost.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1794f0d..de29968 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>          }
>      }
>
> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> -        /* only enable first vq pair by default */
> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> -    }
> -
>      return 0;
>  fail_log:
>      vhost_log_put(hdev, false);
> @@ -1261,10 +1256,6 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>                               hdev->vq_index + i);
>      }
>
> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, 0);
> -    }
> -
>      vhost_log_put(hdev, true);
>      hdev->started = false;
>      hdev->log = NULL;
> --

Ack

> MST

Re: [Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"

[Yuanhan Liu]

On Wed, Nov 25, 2015 at 02:42:05PM +0200, Michael S. Tsirkin wrote:
> This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76.
> 
> In case of live migration several queues can be enabled and not only the
> first one. So informing backend that only the first queue is enabled is
> wrong.

Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

BTW, we should also update the spec about ring stop, right?

	--yliu
> 
> Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/vhost.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1794f0d..de29968 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>          }
>      }
>  
> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> -        /* only enable first vq pair by default */
> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> -    }
> -
>      return 0;
>  fail_log:
>      vhost_log_put(hdev, false);
> @@ -1261,10 +1256,6 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>                               hdev->vq_index + i);
>      }
>  
> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, 0);
> -    }
> -
>      vhost_log_put(hdev, true);
>      hdev->started = false;
>      hdev->log = NULL;
> -- 
> MST

Re: [Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"

[Michael S. Tsirkin]

On Thu, Nov 26, 2015 at 09:46:08AM +0800, Yuanhan Liu wrote:
> On Wed, Nov 25, 2015 at 02:42:05PM +0200, Michael S. Tsirkin wrote:
> > This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76.
> > 
> > In case of live migration several queues can be enabled and not only the
> > first one. So informing backend that only the first queue is enabled is
> > wrong.
> 
> Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> BTW, we should also update the spec about ring stop, right?
> 
> 	--yliu

Pls take a look at for_upstream in my tree, and tell me
whether there's anything we need to clarify.

> > 
> > Reported-by: Thibaut Collet <thibaut.collet@6wind.com>
> > Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio/vhost.c | 9 ---------
> >  1 file changed, 9 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 1794f0d..de29968 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >          }
> >      }
> >  
> > -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> > -        /* only enable first vq pair by default */
> > -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> > -    }
> > -
> >      return 0;
> >  fail_log:
> >      vhost_log_put(hdev, false);
> > @@ -1261,10 +1256,6 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> >                               hdev->vq_index + i);
> >      }
> >  
> > -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> > -        hdev->vhost_ops->vhost_set_vring_enable(hdev, 0);
> > -    }
> > -
> >      vhost_log_put(hdev, true);
> >      hdev->started = false;
> >      hdev->log = NULL;
> > -- 
> > MST

Re: [Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"

[Yuanhan Liu]

On Thu, Nov 26, 2015 at 06:52:56PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 26, 2015 at 09:46:08AM +0800, Yuanhan Liu wrote:
> > On Wed, Nov 25, 2015 at 02:42:05PM +0200, Michael S. Tsirkin wrote:
> > > This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76.
> > > 
> > > In case of live migration several queues can be enabled and not only the
> > > first one. So informing backend that only the first queue is enabled is
> > > wrong.
> > 
> > Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > 
> > BTW, we should also update the spec about ring stop, right?
> > 
> > 	--yliu
> 
> Pls take a look at for_upstream in my tree, and tell me
> whether there's anything we need to clarify.

I had a quick check, and found it's fine. I was thinking we are
still clarifying that there are two ways for signing a ring stop:
VHOST_USER_GET_VRING_BASE and VHOST_UESR_SET_VRING_ENABLE.

Sorry for the noisy then.

	--yliu