|
From: | Alexander Graf |
Subject: | Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice |
Date: | Thu, 12 Jun 2014 11:08:00 +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 11:07, Michael S. Tsirkin wrote:
On Thu, Jun 12, 2014 at 10:55:40AM +0200, Alexander Graf wrote:On 12.06.14 09:54, Michael S. Tsirkin 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. 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?There are already released and working Linux distributions (Ubuntu, openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our heads into the sand is not an option ;). AlexI don't get it. Does virtio work there at the moment?
With the LE enable patch, yes, sure. Imagine the same would happen for Windows and e1000 - would you still argue the same way?
Alex
[Prev in Thread] | Current Thread | [Next in Thread] |