Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Tue, 27 May 2014 17:01:38 +0200
Paolo Bonzini <address@hidden> wrote:

> Il 14/05/2014 17:42, Greg Kurz ha scritto:
> > +    { .name = "virtio/is_big_endian",
> > +      .version_id = 1,
> > +      .save = virtio_save_device_endian,
> > +      .load = virtio_load_device_endian,
> > +    },
> >      { .name = NULL }
> 
> I think this is okay, but you need to add support for a "needed" 
> function like you have in normal vmstate subsections.  You only need the 
> subsection if
> 
>      if (target_words_bigendian()) {
>          return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_BIG;
>      } else {
>          return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
>      }
> 
> Paolo
> 

Hi Paolo,

As suggested by Andreas, I have reworked the patch set to use
VMState directly instead of mimicking... and of course, I end
up with a virtio_device_endian_needed() function that does just
that ! :)

I'm on leave now, I'll try to send an update next week.

BTW, I have spotted two locations where the migration code is
affected by the device endianness at load time:

int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
                       ^^^^^^^^^^^
            /* Check it isn't doing very strange things with descriptor 
numbers. */
            if (nheads > vdev->vq[i].vring.num) {
[...]
}

and

static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
    /* The config space */
    qemu_get_be16s(f, &s->config.cols);
    qemu_get_be16s(f, &s->config.rows);

    qemu_get_be32s(f, &max_nr_ports);
    tswap32s(&max_nr_ports);
     ^^^^^^
    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
[...]
}

If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.

The first one can be easily dealt with: just defer the sanity check
to a post_load function. The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\

I have questions about virtio serial:
- what is exactly the point at migrating the virtio device config space ?
- what is the scenario that calls for the active ports bitmap checking
  at load time ?
- we currently have 0 < max_nr_ports < VIRTIO_PCI_QUEUE_MAX / 2 hardcoded.
  With the current code, that means the ports bitmap is always a single
  32-bit fixed size value... are there plans to support virtio serial
  with 0 port ? with more than 32 ports ?

In the case the answer for above is "legacy virtio really sucks" then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)

A solution could be to simply remove all that "crap" and bump versions,
or at least send zeroes on save and skip them on load.

Another solution I haven't tried yet would be to stream subsections before
the device descriptor...

Any suggestions ?

Cheers.

-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.