[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support t
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice |
|
Date: |
Thu, 12 Jun 2014 12:05:54 +0300 |
On Thu, Jun 12, 2014 at 10:47:17AM +0200, Greg Kurz wrote:
> On Thu, 12 Jun 2014 10:54:48 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
> > > On Thu, 29 May 2014 12:16:26 +0200
> > > Paolo Bonzini <address@hidden> wrote:
> > > > Il 29/05/2014 11:12, Greg Kurz ha scritto:
> > > > > int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > > > > {
> > > > > [...]
> > > > > nheads = vring_avail_idx(&vdev->vq[i]) -
> > > > > vdev->vq[i].last_avail_idx;
> > > > > ^^^^^^^^^^^
> > > > > /* Check it isn't doing very strange things with
> > > > > descriptor numbers. */
> > > > > if (nheads > vdev->vq[i].vring.num) {
> > > > > [...]
> > > > > }
> > > > >
> > > > > and
> > > > >
> > > > > static int virtio_serial_load(QEMUFile *f, void *opaque, int
> > > > > version_id)
> > > > > {
> > > > > [...]
> > > > > /* The config space */
> > > > > qemu_get_be16s(f, &s->config.cols);
> > > > > qemu_get_be16s(f, &s->config.rows);
> > > > >
> > > > > qemu_get_be32s(f, &max_nr_ports);
> > > > > tswap32s(&max_nr_ports);
> > > > > ^^^^^^
> > > > > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > > > [...]
> > > > > }
> > > > >
> > > > > If we stream subsections after the device descriptor as it is done
> > > > > in VMState, these two will break because the device endian is stale.
> > > > >
> > > > > The first one can be easily dealt with: just defer the sanity check
> > > > > to a post_load function.
> > > >
> > > > Good, we're lucky here.
> > > >
> > > > > The second is a bit more tricky: the
> > > > > virtio serial migrates its config space (target endian) and the
> > > > > active ports bitmap. The load code assumes max_nr_ports from the
> > > > > config space tells the size of the ports bitmap... that means the
> > > > > virtio migration protocol is also contaminated by target endianness.
> > > > > :-\
> > > >
> > > > Ouch.
> > > >
> > > > I guess we could break migration in the case of host endianness !=
> > > > target endianness, like this:
> > > >
> > > > /* These three used to be fetched in target endianness and then
> > > > * stored as big endian. It ended up as little endian if host and
> > > > * target endianness doesn't match.
> > > > *
> > > > * Starting with qemu 2.1, we always store as big endian. The
> > > > * version wasn't bumped to avoid breaking backwards compatibility.
> > > > * We check the validity of max_nr_ports, and the incorrect-
> > > > * endianness max_nr_ports will be huge, which will abort migration
> > > > * anyway.
> > > > */
> > > > uint16_t cols = tswap16(s->config.cols);
> > > > uint16_t rows = tswap16(s->config.rows);
> > > > uint32_t max_nr_ports = tswap32(s->config.max_nr_ports);
> > > >
> > > > qemu_put_be16s(f, &cols);
> > > > qemu_put_be16s(f, &rows);
> > > > qemu_put_be32s(f, &max_nr_ports);
> > > >
> > > > ...
> > > >
> > > > uint16_t cols, rows;
> > > >
> > > > qemu_get_be16s(f, &cols);
> > > > qemu_get_be16s(f, &rows);
> > > > qemu_get_be32s(f, &max_nr_ports);
> > > >
> > > > /* Convert back to target endianness when storing into the config
> > > > * space.
> > > > */
> > >
> > > Paolo,
> > >
> > > The patch set to support endian changing targets adds a device_endian
> > > field to the VirtIODevice structure to be used instead of the default
> > > target endianness as it happens with tswap() macros. It also introduces
> > > virtio_tswap() helpers for this purpose, but they can only be used when
> > > the device_endian field has been restored... in a subsection after the
> > > device descriptor... :-\
> >
> > Store it earlier then, using plain put/get.
>
> Not sure I follow... this will break compatibility, no ?
Do it only on the ambialent targets :)
> > You can still add a section conditionally to cause
> > a cleaner failure in broken cross-version scenarios.
> >
> > > If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
> > > and we cannot convert back to LE...
> > >
> > > > s->config.cols = tswap16(cols);
> > > > s->config.rows = tswap16(rows);
> > >
> > > Since cols and rows are not involved in the protocol, we can safely
> > > defer the conversion to post load.
> > >
> > > > if (max_nr_ports > tswap32(s->config.max_nr_ports) {
> > > > ...
> > > > }
> > > >
> > >
> > > Since we know that 0 < max_nr_ports < 32, is it acceptable to guess
> > > the correct endianness with a heuristic ?
> > >
> > > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > max_nr_ports = bswap32(max_nr_ports);
> > > }
> > >
> > > if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> > > return -EINVAL;
> > > }
> > >
> > > > > In the case the answer for above is "legacy virtio really sucks" then,
> > > > > is it acceptable to not honor bug-compatibility with older versions
> > > > > and
> > > > > fix the code ? :)
> > > >
> > > > As long as the common cases don't break, yes. The question is what are
> > > > the common cases. Here I think the only non-obscure case that could
> > > > break is x86-on-PPC, and it's not that common.
> > > >
> > > > Paolo
> > > >
> > >
> > > Thanks.
> >
> > One starts doubting whether all this hackery is worth it. virtio 1.0
> > should be out real soon now, it makes everything LE so the problem goes
> > away. It's not like PPC LE is so popular that we must support old drivers
> > at all costs. Won't time be better spent backporting virtio 1.0 drivers?
> >
>
> Hmmm... AFAIC some QEMU maintainers expressed interest in supporting legacy
> virtio in the case we have endian-changing targets. Patches to run a ppc64le
> guests have been accepted in KVM, Linux and QEMU... the only missing block
> is virtio. I don't especially care in supporting old drivers at all cost: this
> request was expressed on the list. I just want people to be able to run a
> ppc64le
> ubuntu-14.04 (and soon other distros) guest on a ppc64 box and be able to
> migrate.
Sigh. So ubuntu ships known-broken virtio drivers in their LTS release
and now we are trying to make them work.
There's no working qemu configuration at the moment, do we really
have to build new hardware around known-broken drivers
and then maintain it all for years?
Why not build good hardware and backport new drivers?
> Would it be acceptable to break compatibility for ppc64 (and maybe ARM) only
> with
> a target specific hook being called from the virtio code ?
I think so.
Can we just add a function that tells us whether target is ambivalent?
Only migrate new stuff if that's the case.
> >
> > > --
> > > Gregory Kurz address@hidden
> > > address@hidden
> > > Software Engineer @ IBM/Meiosys http://www.ibm.com
> > > Tel +33 (0)562 165 496
> > >
> > > "Anarchy is about taking complete responsibility for yourself."
> > > Alan Moore.
> >
>
> Thanks.
>
> --
> Gregory Kurz address@hidden
> address@hidden
> Software Engineer @ IBM/Meiosys http://www.ibm.com
> Tel +33 (0)562 165 496
>
> "Anarchy is about taking complete responsibility for yourself."
> Alan Moore.