On Tue, 27 May 2014 17:01:38 +0200
Paolo Bonzini <pbonz...@redhat.com> 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 kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
