[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targ
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targets |
Date: |
Tue, 16 Dec 2014 16:38:15 +1100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, Dec 16, 2014 at 10:08:49AM +0530, Amit Shah wrote:
> On (Tue) 16 Dec 2014 [15:33:54], David Gibson wrote:
> > 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.
>
> Yes, the submitter didn't follow up after review.
>
> > > 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
>
> For this patch, I'm just suggesting to only touch cols and rows, and
> lines that touch the max_nr_ports can be put in 2/2. Eliminating
> config entirely may not be desirable, but if you want to do that, and
> mark all fields as 'unused' in the savevm/loadvm functions, go for
> it :-)
That division really doesn't make sense to me. If rows and cols are
removed, but max_nr_ports stays, then get_config has to become a
weird hybrid where it copies some stuff directly from ->config and
other bits from elsewhere.
I really don't see any reason keeping config would be a desirable
thing: it's a cache of information that doesn't need to be cached, and
just adds complexity because of the endian issues.
--
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
pgpZ2sQWNPjMF.pgp
Description: PGP signature