From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55021) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv0fW-0005s2-Ib for qemu-devel@nongnu.org; Thu, 12 Jun 2014 04:47:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wv0fN-0000EO-EB for qemu-devel@nongnu.org; Thu, 12 Jun 2014 04:47:34 -0400 Received: from e06smtp13.uk.ibm.com ([195.75.94.109]:49257) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv0fN-0000Dw-1M for qemu-devel@nongnu.org; Thu, 12 Jun 2014 04:47:25 -0400 Received: from /spool/local by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 12 Jun 2014 09:47:23 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 7405F17D804E for ; Thu, 12 Jun 2014 09:48:43 +0100 (BST) Received: from d06av01.portsmouth.uk.ibm.com (d06av01.portsmouth.uk.ibm.com [9.149.37.212]) by b06cxnps4074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s5C8lLUr30343412 for ; Thu, 12 Jun 2014 08:47:21 GMT Received: from d06av01.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av01.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s5C8lKOa019069 for ; Thu, 12 Jun 2014 02:47:21 -0600 Date: Thu, 12 Jun 2014 10:47:17 +0200 From: Greg Kurz Message-ID: <20140612104717.4c902947@bahia.local> In-Reply-To: <20140612075448.GB19354@redhat.com> References: <20140514154130.10746.1412.stgit@bahia.local> <20140514154230.10746.56297.stgit@bahia.local> <5384A8D2.8050104@redhat.com> <20140529111253.4ff55199@bahia.local> <538708FA.4070309@redhat.com> <20140612094351.6295fd38@bahia.local> <20140612075448.GB19354@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Kevin Wolf , Fam Zheng , Stefan Hajnoczi , Juan Quintela , Alexander Graf , qemu-devel@nongnu.org, Anthony Liguori , Amit Shah , Paolo Bonzini , Andreas =?UTF-8?B?RsOkcmJlcg==?= On Thu, 12 Jun 2014 10:54:48 +0300 "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 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.