From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39747) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuzgC-000312-MW for qemu-devel@nongnu.org; Thu, 12 Jun 2014 03:44:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wuzg1-0002rZ-6t for qemu-devel@nongnu.org; Thu, 12 Jun 2014 03:44:12 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:59281) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wuzg0-0002rT-UD for qemu-devel@nongnu.org; Thu, 12 Jun 2014 03:44:01 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 12 Jun 2014 08:43:59 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 3D16B1B08041 for ; Thu, 12 Jun 2014 08:44:23 +0100 (BST) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps4074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s5C7huKp29819096 for ; Thu, 12 Jun 2014 07:43:56 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s5C7huJ8009988 for ; Thu, 12 Jun 2014 01:43:56 -0600 Date: Thu, 12 Jun 2014 09:43:51 +0200 From: Greg Kurz Message-ID: <20140612094351.6295fd38@bahia.local> In-Reply-To: <538708FA.4070309@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> 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: Paolo Bonzini Cc: Kevin Wolf , Fam Zheng , Stefan Hajnoczi , "Michael S. Tsirkin" , Juan Quintela , Alexander Graf , qemu-devel@nongnu.org, Anthony Liguori , Amit Shah , Andreas =?UTF-8?B?RsOkcmJlcg==?= 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... :-\ 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. -- 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.