From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiuHV-0004aH-II for qemu-devel@nongnu.org; Wed, 12 Dec 2012 16:56:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tiu9v-0003ff-Fr for qemu-devel@nongnu.org; Wed, 12 Dec 2012 16:48:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35746) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tiu9v-0003fW-8A for qemu-devel@nongnu.org; Wed, 12 Dec 2012 16:48:07 -0500 Date: Wed, 12 Dec 2012 23:51:15 +0200 From: "Michael S. Tsirkin" Message-ID: <20121212215115.GA27912@redhat.com> References: <20121212152508.GB16750@redhat.com> <50C8A855.4050607@redhat.com> <20121212163713.GD17446@redhat.com> <50C8B627.606@redhat.com> <20121212171423.GB18597@redhat.com> <50C8C143.2010503@redhat.com> <20121212192336.GA20201@redhat.com> <50C8F075.4000206@redhat.com> <20121212211926.GA23087@redhat.com> <87bodzrm4z.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87bodzrm4z.fsf@codemonkey.ws> Subject: Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Paolo Bonzini , Rusty Russell , qemu-devel@nongnu.org, Stefan Hajnoczi , Stefan Hajnoczi On Wed, Dec 12, 2012 at 03:33:32PM -0600, Anthony Liguori wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Dec 12, 2012 at 10:00:37PM +0100, Paolo Bonzini wrote: > >> Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto: > >> >>> Saving inuse counter is useless. We need to know which requests > >> >>> are outstanding if we want to retry them on remote. > >> >> > >> >> And that's what virtio-blk and virtio-scsi have been doing for years. > >> > > >> > I don't see it - all I see in save is virtio_save. > >> > >> static void virtio_blk_save(QEMUFile *f, void *opaque) > >> { > >> VirtIOBlock *s = opaque; > >> VirtIOBlockReq *req = s->rq; > >> > >> virtio_save(&s->vdev, f); > >> > >> while (req) { > >> qemu_put_sbyte(f, 1); > >> qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); > > > > Ow. Does it really save VirtQueueElement? > > > > typedef struct VirtQueueElement > > { > > unsigned int index; > > unsigned int out_num; > > unsigned int in_num; BTW there's a hole after in_num which is uninitialized, that's also a nasty thing to send on the wire. > > hwaddr in_addr[VIRTQUEUE_MAX_SIZE]; > > hwaddr out_addr[VIRTQUEUE_MAX_SIZE]; > > struct iovec in_sg[VIRTQUEUE_MAX_SIZE]; > > struct iovec out_sg[VIRTQUEUE_MAX_SIZE]; > > } VirtQueueElement; > > > > Complete with pointers into qemu memory and all? > > That's got to hurt. > > > > All we really need is the index. > > > > Yes, take a look at the series I sent out that scrubs all of this to > just send the index and the addresses of the element. Will do. > We technically should save the addresses and sizes too. I guess as long as these are guest addresses, not ther qemu ones. > It makes it a > heck of a lot safer then re-reading guest memory since we do some > validation on the size of the sg elements. > But we could get away with only saving the index if we really wanted to. I guess re-validating it is needed anyway: we should not trust remote more than we trust the guest. > Regards, > > Anthony Liguori > > >> req = req->next; > >> } > >> qemu_put_sbyte(f, 0); > >> } > >> > >> > >> virtio-scsi does it in virtio_scsi_save_request. > >> > >> > You need to retry A1 on remote. How do you do that? There's > >> > no way to find out it has not been completed > >> > from the ring itself. > >> > >> virtio_blk_dma_restart_bh and scsi_dma_restart_bh do it. > >> > >> Paolo > > > > Okay, so the only bug is inuse getting negative right? > > So all we need to do is fix up the inuse value > > after restoring the outstanding requests - basically > > count the restored buffers and set inuse accordingly. > > > > -- > > MST