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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 12/12/2012 22:19, Michael S. Tsirkin ha scritto:
> 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?

Yes, that sucks (also the endianness is totally broken), but the
pointers into QEMU memory are restored afterwards by remapping the
address.  Like Anthony, I'm not sure whether reloading the sglist from
guest memory is safe.  It would require re-validation of everything.

We _can_ trust remote.  Things like races on connecting to the
incoming-migration socket are best handled outside QEMU with a firewall
or a separate network that is not guest-accessible.  I'm pretty sure
that virtio-blk is the latest of our worries here.

> 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.

Yes, I agree.

Paolo