On Wed, Dec 12, 2012 at 03:33:32PM -0600, Anthony Liguori wrote: > "Michael S. Tsirkin" <m...@redhat.com> 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