From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv0xb-0006fz-Vi for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:06:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wv0xU-0006hB-FG for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:06:15 -0400 Received: from cantor2.suse.de ([195.135.220.15]:57869 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv0xU-0006gz-2a for qemu-devel@nongnu.org; Thu, 12 Jun 2014 05:06:08 -0400 Message-ID: <53996D7D.1070201@suse.de> Date: Thu, 12 Jun 2014 11:06:05 +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> <20140612104717.4c902947@bahia.local> In-Reply-To: <20140612104717.4c902947@bahia.local> 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: Greg Kurz , "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?= On 12.06.14 10:47, Greg Kurz wrote: > 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 ? 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