Re: [Qemu-devel] [PATCH v9 10/20] Gracefully handle ioctl failure in vhost_virtqueue_stop

[Antonios Motakis <a.motakis@virtualopensystems.com>]

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

Hello,


On Tue, Mar 4, 2014 at 7:45 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Mar 04, 2014 at 07:22:53PM +0100, Antonios Motakis wrote:
> > On stopping the vhost, a call to VHOST_GET_VRING_BASE is issued. The
> > received value is stored as last_avail_idx, so the virtqueue can continue
> > operating if the connection is resumed. Handle the failure of this call
> > and use the current avail_idx. Some packets from the avail ring may be
> > omitted but still we keep a sane value and can continue on reconnect.
>
> omitted how?
> some guests crash if we never complete handling buffers,
> or networking breaks, etc ...
>
> This would be a big problem for reconnect, some robust way to
> communicate avail ring state would need to be found.
> Is reconnect really a mandatory feature for you?
> I'd suggest you drop it from v1, focus on basic functionality.
>
>
Reconnect would be a really useful feature for us, so we tried to keep it
in a reasonable way.

However we didn't take into account that some guests might crash under
those assumptions. Looks like we have no option but to remove reconnect
altogether for now; maybe a future extension to the virtio-net spec will
allow us to do it cleanly, but I don't see an obvious workaround to keep
this in now.

Thanks for pointing this out.

Btw, since it looks like we are closing a final version of the patches,
what kind of timeframe should we aim for inclusion? Should we already
rebase on top of Paolo's NUMA patch series?

>
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>
> Problem is, a bunch of stuff breaks if vhost keeps
> going when we ask it to stop.
> In particular it will keep looking at the ring
> state when guest asked it to stop doing this,
> this will corrupt guest memory.
>
>
> > ---
> >  hw/virtio/vhost.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 9e336ad..322e2c0 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -758,12 +758,13 @@ static void vhost_virtqueue_stop(struct vhost_dev
> *dev,
> >      assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >      r = ioctl(dev->control, VHOST_GET_VRING_BASE, &state);
> >      if (r < 0) {
> > +        state.num = virtio_queue_get_avail_idx(vdev, idx);
> >          fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx,
> r);
> >          fflush(stderr);
> >      }
> >      virtio_queue_set_last_avail_idx(vdev, idx, state.num);
> >      virtio_queue_invalidate_signalled_used(vdev, idx);
> > -    assert (r >= 0);
> > +
> >      cpu_physical_memory_unmap(vq->ring,
> virtio_queue_get_ring_size(vdev, idx),
> >                                0, virtio_queue_get_ring_size(vdev, idx));
> >      cpu_physical_memory_unmap(vq->used,
> virtio_queue_get_used_size(vdev, idx),
> > --
> > 1.8.3.2
>



-- 
Antonios Motakis
Virtual Open Systems

[-- Attachment #2: Type: text/html, Size: 4072 bytes --]