|
From: | Alexander Graf |
Subject: | Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice |
Date: | Thu, 12 Jun 2014 11:06:05 +0200 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 12.06.14 10:47, 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 ?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. PaoloThanks.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 ?
I don't see the problem. Let's just make the config fields UNUSED in vmstate and keep the virtio-serial config space in good endian locally. Then you don't care about ordering of sections anymore.
Then we only need the subsection code for "endian is not target endian" which makes migration backwards compatible with non-switchable QEMU versions and everyone's happy. End of story :).
Alex
[Prev in Thread] | Current Thread | [Next in Thread] |