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

[Paolo Bonzini]

Il 29/05/2014 11:12, Greg Kurz ha scritto:
> 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.

Good, we're lucky here.

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

Ouch.

I guess we could break migration in the case of host endianness != 
target endianness, like this:

    /* These three used to be fetched in target endianness and then
     * stored as big endian.  It ended up as little endian if host and
     * target endianness doesn't match.
     *
     * Starting with qemu 2.1, we always store as big endian.  The
     * version wasn't bumped to avoid breaking backwards compatibility.
     * We check the validity of max_nr_ports, and the incorrect-
     * endianness max_nr_ports will be huge, which will abort migration
     * anyway.
     */
    uint16_t cols = tswap16(s->config.cols);
    uint16_t rows = tswap16(s->config.rows);
    uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);

    qemu_put_be16s(f, &cols);
    qemu_put_be16s(f, &rows);
    qemu_put_be32s(f, &max_nr_ports);

...

    uint16_t cols, rows;

    qemu_get_be16s(f, &cols);
    qemu_get_be16s(f, &rows);
    qemu_get_be32s(f, &max_nr_ports);

    /* Convert back to target endianness when storing into the config
     * space.
     */
    s->config.cols = tswap16(cols);
    s->config.rows = tswap16(rows);
    if (max_nr_ports > tswap32(s->config.max_nr_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 ? :)

As long as the common cases don't break, yes.  The question is what are 
the common cases.  Here I think the only non-obscure case that could 
break is x86-on-PPC, and it's not that common.

Paolo