Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

[Anthony Liguori]

"Michael S. Tsirkin" <address@hidden> 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;
>     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.

We technically should save the addresses and sizes too.  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.

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