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

[Michael S. Tsirkin]

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.

> 
> Signed-off-by: Antonios Motakis <address@hidden>
> Signed-off-by: Nikolay Nikolaev <address@hidden>

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