[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix virtio-serial migration on bi-endian target
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH] Fix virtio-serial migration on bi-endian targets |
Date: |
Fri, 12 Dec 2014 16:26:27 +1100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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
pgpi5eHTrBLGh.pgp
Description: PGP signature