Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Michael S. Tsirkin]

On Wed, Nov 25, 2015 at 01:17:53PM +0100, Thibaut Collet wrote:
> On Wed, Nov 25, 2015 at 12:04 PM, Michael S. Tsirkin <address@hidden> wrote:
> > On Wed, Nov 25, 2015 at 09:32:15AM +0800, Yuanhan Liu wrote:
> >> On Tue, Nov 24, 2015 at 11:23:34PM +0200, Michael S. Tsirkin wrote:
> >> > On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote:
> >> > > On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <address@hidden> 
> >> > > wrote:
> >> > > > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
> >> > > >> This patch reverts partially commit 3a12f32229a.
> >> > > >>
> >> > > >> In case of live migration several queues can be enabled and not 
> >> > > >> only the first
> >> > > >> one. So inform backend that only the first queue is enabled is 
> >> > > >> wrong.
> >> > > >>
> >> > > >> Since commit 7263a0ad7899 backend is already notified of the state 
> >> > > >> of the vring
> >> > > >> through the vring attach operation. This function, called during 
> >> > > >> the startup
> >> > > >> sequence, provides the correct state of the vring, even in case of 
> >> > > >> live
> >> > > >> migration.
> >> > > >>
> >> > > >> So nothing has to be added to give the vring state to the backend 
> >> > > >> at the startup.
> >> > > >>
> >> > > >> Signed-off-by: Thibaut Collet <address@hidden>
> >> > > >> ---
> >> > > >>  hw/virtio/vhost.c | 5 -----
> >> > > >>  1 file changed, 5 deletions(-)
> >> > > >>
> >> > > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> > > >> index 1794f0d..870cd12 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);
> >> > > >> --
> >> > > >> 2.1.4
> >> > > >
> >> > > > Yes - and I'm beginning to think that maybe we should revert
> >> > > > all of 3a12f32229a then, for symmetry.
> >> > > >
> >> > >
> >> > > Keep the disable vring on the stop can be useful. For example if the
> >> > > VM is rebooted all the vring will be disabled and backend will avoid
> >> > > to send packet to the VM in this case (I am not sure the virtio ring
> >> > > address is always valid during a reboot and writingg data in this
> >> > > memory can cause unexpected behaviour in this case).
> >> >
> >> > I think there's still some confusion:
> >> > writing memory can still happen even if you disable the ring
> >> > since the TX ring is still processed so we write into the used ring.
> >> >
> >> > We call GET_VRING_BASE on stop and that ensures rings are
> >> > stopped.
> >>
> >> Yes, that's what I suggested first, which also makes the logic quite
> >> simple: we use GET_VRING_BASE as the sign of vring stop. Intead of
> >> GET_VRING_BASE when protocol not negotiated, and SET_VRING_ENABLE
> >> when protocol is negotiated.
> >>
> >> Michael, should I submit a revert patch, or you could do it directly?
> >>
> >>       --yliu
> >
> > I can handle it.
> >
> 
> The patch must be completly reverted. There are too an issue with
> suspend/resume operations.
> On the suspend the dev_stop is called that will disable all the vrings.
> On the resume the dev_start is called but not the peer_attach (vring
> is already attached) and state of the vring is not provided to the
> backend.

OK, I queued up a revert. Thanks!

> >> >
> >> >
> >> > > > Yunnan, Victor - what do you think?
> >> > > >
> >> > > > --
> >> > > > MST