Re: [Qemu-devel] [PATCH] virtio: verify that all outstanding buffers are flushed (was Re: vmstate conversion for virtio?)

[Anthony Liguori <anthony@codemonkey.ws>]

Rusty Russell <rusty@rustcorp.com.au> writes:

> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> No, because I don't understand it.  Is it true for the case of
> virtio_blk, which has outstanding requests?
>
>>> Currently we dump a massive structure; it's inelegant at the very
>>> least.

Inelegant is a kind word..

There's a couple things to consider though which is why this code hasn't
changed so far.

1) We're writing native endian values to the wire.  This is seriously
   broken.  Just imagine trying to migrate from qemu-system-i386 on an
   big endian box to a little endian box.

2) Fixing (1) either means (a) breaking migration across the board
   gracefully or (b) breaking migration on [big|little] endian hosts in
   an extremely ungraceful way.

3) We send a ton of crap over the wire that is unnecessary, but we need
   to maintain it.

I wrote up a patch series to try to improve the situation that I'll send
out.  I haven't gotten around to testing it with an older version of
QEMU yet.

I went for 2.b and choose to break big endian hosts.

>>> 
>>> Cheers,
>>> Rusty.
>>
>> Hmm not sure what you refer to. I see this per ring:
>>
>>         qemu_put_be32(f, vdev->vq[i].vring.num);
>>         qemu_put_be64(f, vdev->vq[i].pa);
>>         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>>
>> Looks like there's no way around savng these fields.

Correct.

Regards,

Anthony Liguori

>
> Not what I'm referring to.  See here:
>
> virtio.h defines a 48k structure:
>
> #define VIRTQUEUE_MAX_SIZE 1024
>
> 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;
>
> virtio-blk.c uses it in its request struct:
>
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
>     VirtQueueElement elem;
>     struct virtio_blk_inhdr *in;
>     struct virtio_blk_outhdr *out;
>     struct virtio_scsi_inhdr *scsi;
>     QEMUIOVector qiov;
>     struct VirtIOBlockReq *next;
>     BlockAcctCookie acct;
> } VirtIOBlockReq;
>
> ... and saves it in virtio_blk_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));
>         req = req->next;
>     }
>     qemu_put_sbyte(f, 0);
> }
>
> Cheers,
> Rusty.