qemu-devel
[Top][All Lists]
Advanced

[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:04:35 +1100
User-agent: Mutt/1.5.23 (2014-03-12)

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.

-- 
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

Attachment: pgpDu6w3q_mEN.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]