Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Thu, 12 Jun 2014 10:54:48 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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 <pbonzini@redhat.com> 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 ?

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

Would it be acceptable to break compatibility for ppc64 (and maybe ARM) only with
a target specific hook being called from the virtio code ?

> 
> > -- 
> > Gregory Kurz                                     kurzgreg@fr.ibm.com
> >                                                  gkurz@linux.vnet.ibm.com
> > 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                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.