[Qemu-devel] vmstate conversion for virtio?

[Qemu-devel] vmstate conversion for virtio?

[Rusty Russell]

Hi all,

        I want to rework the qemu virtio subsystem, but various
structures are currently blatted to disk in save/load.  So I looked at
altering that, only to discover that it needs conversion to vmstate, and
2009 patches in patchwork which have never been applied.

Has there been any progress on this?  A git tree I should be using?

Thanks,
Rusty.

Re: [Qemu-devel] vmstate conversion for virtio?

[Michael S. Tsirkin]

On Tue, Dec 04, 2012 at 01:39:35PM +1030, Rusty Russell wrote:
> Hi all,
> 
>         I want to rework the qemu virtio subsystem, but various
> structures are currently blatted to disk in save/load.  So I looked at
> altering that, only to discover that it needs conversion to vmstate, and
> 2009 patches in patchwork which have never been applied.
> 
> Has there been any progress on this?  A git tree I should be using?
> 
> Thanks,
> Rusty.

Not that I know - Juan might have one.
But if anything, changing datastructures with old vmstate
is easier.

-- 
MST

Re: [Qemu-devel] vmstate conversion for virtio?

[Juan Quintela]

Rusty Russell <rusty@rustcorp.com.au> wrote:
> Hi all,
>
>         I want to rework the qemu virtio subsystem, but various
> structures are currently blatted to disk in save/load.  So I looked at
> altering that, only to discover that it needs conversion to vmstate, and
> 2009 patches in patchwork which have never been applied.
>
> Has there been any progress on this?  A git tree I should be using?

My trees are more than 1 year old, and unfinished (basic trouble is how
to express the lists of pending requests for block and now serial).  I
haven't yet looked at virtio-scsi.

Later, Juan.

Re: [Qemu-devel] vmstate conversion for virtio?

[Rusty Russell]

Juan Quintela <quintela@redhat.com> writes:

> Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Hi all,
>>
>>         I want to rework the qemu virtio subsystem, but various
>> structures are currently blatted to disk in save/load.  So I looked at
>> altering that, only to discover that it needs conversion to vmstate, and
>> 2009 patches in patchwork which have never been applied.
>>
>> Has there been any progress on this?  A git tree I should be using?
>
> My trees are more than 1 year old, and unfinished (basic trouble is how
> to express the lists of pending requests for block and now serial).  I
> haven't yet looked at virtio-scsi.

That's the easy part though, and part of what I want to change.

All we need is the index of the request; the rest can be re-read from
the ring.  So create the array in pre_save, and restore on post_load.

Is there something more recent than
http://repo.or.cz/w/qemu/quintela.git/shortlog/refs/heads/vmstate/virtio
or should I cherry-pick from there?

Thanks,
Rusty.

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

[Michael S. Tsirkin]

Add sanity check to address the following concern:

On Wed, Dec 05, 2012 at 09:47:22AM +1030, Rusty Russell wrote:
> All we need is the index of the request; the rest can be re-read from
> the ring.

I'd like to point out that this is not generally
true if any available requests are outstanding.
Imagine a ring of size 4.
Below A means available U means used.

A 1
A 2
U 2
A 2
U 2
A 2
U 2
A 2
U 2

At this point available ring has wrapped around, the only
way to know head 1 is outstanding is because backend
has stored this info somewhere.

The reason we manage to migrate without tracking this in migration
state is because we flush outstanding requests before
migration.
This flush is device-specific though, let's add
a safeguard in virtio core to ensure it's done properly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/virtio.c b/hw/virtio.c
index f40a8c5..b80a5a9 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -788,6 +788,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
         if (vdev->vq[i].vring.num == 0)
             break;
 
+        assert(!vq->inuse);
+
         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);

-- 
MST

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> Add sanity check to address the following concern:
>
> On Wed, Dec 05, 2012 at 09:47:22AM +1030, Rusty Russell wrote:
>> All we need is the index of the request; the rest can be re-read from
>> the ring.

The terminology I used here was loose, indeed.

We need the head of the chained descriptor, which we already read from
the ring when we gathered the request.

Currently we dump a massive structure; it's inelegant at the very least.

Cheers,
Rusty.

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

[Michael S. Tsirkin]

On Thu, Dec 06, 2012 at 04:33:06PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > Add sanity check to address the following concern:
> >
> > On Wed, Dec 05, 2012 at 09:47:22AM +1030, Rusty Russell wrote:
> >> All we need is the index of the request; the rest can be re-read from
> >> the ring.
> 
> The terminology I used here was loose, indeed.
> 
> We need the head of the chained descriptor, which we already read from
> the ring when we gathered the request.

So ack that patch?

> Currently we dump a massive structure; it's inelegant at the very least.
> 
> 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.

-- 
MST

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Dec 06, 2012 at 04:33:06PM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > Add sanity check to address the following concern:
>> >
>> > On Wed, Dec 05, 2012 at 09:47:22AM +1030, Rusty Russell wrote:
>> >> All we need is the index of the request; the rest can be re-read from
>> >> the ring.
>> 
>> The terminology I used here was loose, indeed.
>> 
>> We need the head of the chained descriptor, which we already read from
>> the ring when we gathered the request.
>
> So ack that patch?

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

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.

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

[Anthony Liguori]

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.

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

[Michael S. Tsirkin]

On Wed, Dec 05, 2012 at 01:08:07PM +0200, Michael S. Tsirkin wrote:
> Add sanity check to address the following concern:
> 
> On Wed, Dec 05, 2012 at 09:47:22AM +1030, Rusty Russell wrote:
> > All we need is the index of the request; the rest can be re-read from
> > the ring.
> 
> I'd like to point out that this is not generally
> true if any available requests are outstanding.
> Imagine a ring of size 4.
> Below A means available U means used.
> 
> A 1
> A 2
> U 2
> A 2
> U 2
> A 2
> U 2
> A 2
> U 2
> 
> At this point available ring has wrapped around, the only
> way to know head 1 is outstanding is because backend
> has stored this info somewhere.
> 
> The reason we manage to migrate without tracking this in migration
> state is because we flush outstanding requests before
> migration.
> This flush is device-specific though, let's add
> a safeguard in virtio core to ensure it's done properly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Doh sent a wrong patch sorry. I'll resend the right one shortly.
Pls disregard.

> ---
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index f40a8c5..b80a5a9 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -788,6 +788,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>          if (vdev->vq[i].vring.num == 0)
>              break;
>  
> +        assert(!vq->inuse);
> +
>          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);
> 
> -- 
> MST

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

[Rusty Russell]

Anthony Liguori <anthony@codemonkey.ws> writes:

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

Since we only actually want to save the descriptor head, I was planning
on a new format version.  That will fix both...

Look forward to your patch,
Rusty.