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

[Michael S. Tsirkin]

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


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