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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4769 bytes --]

On Tue, Dec 16, 2014 at 09:43:30AM +0530, Amit Shah wrote:
> On (Fri) 12 Dec 2014 [16:26:35], 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.
> 
> There were patches on the list a few years back to add resizing
> support.

Well, apparently they were never merged, because I can't see a thing
in the git history.

> Also, ppc and s390 people were using this feature (why else would it
> have been implemented?) -- since you're saying they're not in use, I
> suppose ppc doesn't use it.  CC'ing s390 people for comment.
> 
> >  * "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.
> 
> I'd agree with this approach when I have confirmation no one actually
> uses the {rows,cols}.

I've grepped the tree and searched through git history.  I'm pretty
sure they're not used.

And even if we do want to use them, this approach would still be sound
- it would make more sense to hold the rows and cols info in host
endian (which is well defined and static, at least) and just byteswap
it on get_config() where necessary.

> Since qemu doesn't use the rows and cols, it doesn't matter what
> settings the dest host has; otherwise the dest host would have had to
> adjust the guest to use the dest's settings for rows and cols after a
> migration.  Also, for "new" guests, they should use the control vq
> command to adjust the rows and cols -- and they're not migrated since
> they're not guest state.

Right, I came to the same conclusion (see my reply to agraf).

[snip]
> > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f)
> >      uint32_t nr_active_ports;
> >      unsigned int i, max_nr_ports;
> >  
> > -    /* The config space */
> > -    qemu_put_be16s(f, &s->config.cols);
> > -    qemu_put_be16s(f, &s->config.rows);
> > +    max_nr_ports = s->serial.max_virtserial_ports;
> >  
> > -    qemu_put_be32s(f, &s->config.max_nr_ports);
> > +    /* Used to be config space, now redundant */
> > +    qemu_put_be16(f, 0);
> > +    qemu_put_be16(f, 0);
> > +    qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports));
> 
> Can you split this patch so the config change and the max_nr_ports
> change are separate?  The max_nr_ports could similarly be ignored by
> dest, right?

Um.. I'm not exactly sure where you're drawing the distinction between
the two parts.  Are you thinking
    patch 1) make config.max_nr_ports unused, by using
             max_virtserial_ports instead
    patch 2) eliminate the config field entirely

-- 
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 #2: Type: application/pgp-signature, Size: 819 bytes --]