On Thu, 12 Jun 2014 10:54:48 +0300 "Michael S. Tsirkin" <m...@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 <pbonz...@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 kurzg...@fr.ibm.com > > gk...@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 kurzg...@fr.ibm.com gk...@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.