Re: [Qemu-devel] [PATCH] Fix virtio-serial migration on bi-endian targets

[David Gibson]

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