On Fri, Dec 12, 2014 at 04:04:35PM +1100, David Gibson wrote: > On Fri, Dec 12, 2014 at 03:52:45PM +1100, David Gibson wrote: > > On a bi-endian target, with a guest in the non-default endian mode, > > attempting to migrate twice in a row with a virtio-serial device wil > > cause a qemu SEGV on the second outgoing migration. > > > > The problem is that virtio_serial_save_device() (and other places) expect > > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > > virtio_serial_device_realize() will initialize VirtIOSerial->config in > > default endianness. It's assumed the guest OS will make its true > > endianness known before the device is reset and initialized, then > > vser_reset adjusts VirtIOSerial->config into the new endianness. > > > > But on an incoming migration, the device isn't reset (after all the guest > > has a running driver as far as it's concerned), which means that > > VirtIOSerial->config retains its default endianness value from > > virtio_serial_device_realize(). > > > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > > its actually in default endianness and then runs off the end of the > > ports_map array in the loop immediately afterwards. > > > > We could fix this by adjusting VirtIOSerial->config into the correct > > current endianness after an incoming migration. But a better fix is just > > to get rid of VirtIOSerial->config entirely: > > * The virtio-serial config space is not settable, it always contains the > > values set at initialization > > * AFAICT "rows" and "cols" have never actually been used for anything and > > are always zero. > > * "max_nr_ports" is initialized from > > VirtIOSerial->serial.max_virtserial_ports (host endian) > > > > So instead of maintaining this pointless guest-endian cache of the config > > data, we can just construct it directly into the correct current guest > > endian in the get_config hook. Current users of ->config can instead use > > the sources from which the config values were derived, which means they > > don't have to mess about with converting from guest endian at all. > > [snip] > > @@ -715,13 +714,14 @@ static int virtio_serial_load_device(VirtIODevice > > *vdev, QEMUFile *f, > > qemu_get_be16s(f, (uint16_t *) &tmp); > > qemu_get_be32s(f, &tmp); > > > > - /* Note: this is the only location where we use tswap32() instead of > > - * virtio_tswap32() because: > > - * - virtio_tswap32() only makes sense when the device is fully > > restored > > - * - the target endianness that was used to populate s->config is > > - * necessarly the default one > > + /* Note: Usually we get the maximum number of ports from config > > + * space. Unfortunately there it's in guest endian, and we don't > > + * yet know what that is, because it hasn't been loaded from the > > + * migration stream. We use the host endian copy in the > > + * virtio_serial_conf structure (in fact, config space is > > + * initially populated from there) > > */ > > Realised too late that this comment is no longer correct for the final > version and should just be dropped. I'll resend without it, if people > think the patch is basically sane.can resend with it removed if > people think the patch is basically sane.
Ugh. Found some other problems. Thought I'd compiled and tested it,
but apparently not :(. I'll send a v2.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpOfogqgLbA8.pgp
Description: PGP signature
