From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59741) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv0zS-0000pt-Pd for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:08:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wv0zL-0007ZH-9K for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:08:10 -0400 Received: from cantor2.suse.de ([195.135.220.15]:57921 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv0zK-0007Z3-Vy for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:08:03 -0400 Message-ID: <53996DF0.5070106@suse.de> Date: Thu, 12 Jun 2014 11:08:00 +0200 From: Alexander Graf MIME-Version: 1.0 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> <53996B0C.9090409@suse.de> <20140612090718.GC22230@redhat.com> In-Reply-To: <20140612090718.GC22230@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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 , qemu-devel@nongnu.org, Anthony Liguori , Amit Shah , Paolo Bonzini , =?ISO-8859-1?Q?Andreas_F=E4rber?= , Greg Kurz 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 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. >>>>> >>>>> 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? >> 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 ;). >> >> >> Alex > I 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