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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Mar 05, 2014 at 02:38:34PM +0100, Antonios Motakis wrote:
> 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?
> 

I think it should be possible to merge after Paolo's
patches go in, yes. I haven't followed them closely
so I don't know when will that be.

I wish someone else would ack the char dev changes - anyone?
But it's not a blocker requirement.


>     >
>     > 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