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

[Antonios Motakis]

Hello,

On Tue, Mar 4, 2014 at 7:45 PM, Michael S. Tsirkin <address@hidden> 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 <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

--
Antonios Motakis
Virtual Open Systems