From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLCAk-0007Pv-Oe for qemu-devel@nongnu.org; Wed, 05 Mar 2014 08:47:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLCAe-00089Y-Vl for qemu-devel@nongnu.org; Wed, 05 Mar 2014 08:47:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:65452) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLCAe-00088p-BN for qemu-devel@nongnu.org; Wed, 05 Mar 2014 08:47:40 -0500 Date: Wed, 5 Mar 2014 15:47:31 +0200 From: "Michael S. Tsirkin" Message-ID: <20140305134731.GA25484@redhat.com> References: <1393957383-16685-1-git-send-email-a.motakis@virtualopensystems.com> <1393957383-16685-11-git-send-email-a.motakis@virtualopensystems.com> <20140304184507.GC21171@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 10/20] Gracefully handle ioctl failure in vhost_virtqueue_stop List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Antonios Motakis Cc: Luke Gorrie , "snabb-devel@googlegroups.com" , Nikolay Nikolaev , qemu-devel qemu-devel , VirtualOpenSystems Technical Team On Wed, Mar 05, 2014 at 02:38:34PM +0100, Antonios Motakis wrote: > Hello, >=20 >=20 > On Tue, Mar 4, 2014 at 7:45 PM, Michael S. Tsirkin wro= te: >=20 > 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 thi= s call > > and use the current avail_idx. Some packets from the avail ring m= ay be > > omitted but still we keep a sane value and can continue on reconn= ect. >=20 > omitted how? > some guests crash if we never complete handling buffers, > or networking breaks, etc ... >=20 > 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. >=20 >=20 >=20 > Reconnect would be a really useful feature for us, so we tried to keep = it in a > reasonable way. >=20 > 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 altog= ether > 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. >=20 > Thanks for pointing this out. >=20 > 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? >=20 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 > > Signed-off-by: Nikolay Nikolaev >=20 > 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. >=20 >=20 > > --- > > =A0hw/virtio/vhost.c | 3 ++- > > =A01 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 vho= st_dev > *dev, > > =A0 =A0 =A0assert(idx >=3D dev->vq_index && idx < dev->vq_index += dev->nvqs); > > =A0 =A0 =A0r =3D ioctl(dev->control, VHOST_GET_VRING_BASE, &state= ); > > =A0 =A0 =A0if (r < 0) { > > + =A0 =A0 =A0 =A0state.num =3D virtio_queue_get_avail_idx(vdev, i= dx); > > =A0 =A0 =A0 =A0 =A0fprintf(stderr, "vhost VQ %d ring restore fail= ed: %d\n", idx, > r); > > =A0 =A0 =A0 =A0 =A0fflush(stderr); > > =A0 =A0 =A0} > > =A0 =A0 =A0virtio_queue_set_last_avail_idx(vdev, idx, state.num); > > =A0 =A0 =A0virtio_queue_invalidate_signalled_used(vdev, idx); > > - =A0 =A0assert (r >=3D 0); > > + > > =A0 =A0 =A0cpu_physical_memory_unmap(vq->ring, virtio_queue_get_r= ing_size(vdev, > idx), > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00,= virtio_queue_get_ring_size(vdev, idx)); > > =A0 =A0 =A0cpu_physical_memory_unmap(vq->used, virtio_queue_get_u= sed_size(vdev, > idx), > > -- > > 1.8.3.2 >=20 >=20 >=20 >=20 > -- > Antonios Motakis > Virtual Open Systems