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

[David Gibson]

On Mon, Dec 15, 2014 at 03:59:56PM +0100, Alexander Graf wrote:
> 
> 
> On 12.12.14 06:26, 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.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> 
> In general I agree with the patch, but ...

[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);
> 
> ... are you 100% sure these are always unused and will stay unused? I
> think we'd be better off just treating them the same way as max_nr_ports
> and migrate them over - or at least add a comment here what the
> variables mean so that whenever someone adds support for rows/cols they
> know where to put them.

Well, so.  Here's what I know:

  * The rows and cols fields certainly aren't used at present (and we
    don't advertise the VIRTIO_CONSOLE_F_SIZE feature bit which says
    they are valid)

  * I've looked through the git history and I'm confident they've
    never been used.

  * The current incoming side migration code will ignore these
    fields.  Prior to e38e943a it would load them, but then not do
    anything with the cols and rows fields as before.

Thinking about it, even if rows/cols are used in future, it doesn't
actually make sense to migrate them.  They'll change in response to
host events, not guest events, and the suitable values might change on
migration.  So it seems like it would be more sensible for the
migration destination to generate correct values for itself and
advertise them to the guest with an unconditional config_changed
interrupt.

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

pgp5IzwB_xzI9.pgp
Description: PGP signature