From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTb3y-0005Yr-My for qemu-devel@nongnu.org; Fri, 28 Mar 2014 13:59:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTb3t-0005GZ-6A for qemu-devel@nongnu.org; Fri, 28 Mar 2014 13:59:30 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59283 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTb3s-0005GN-R8 for qemu-devel@nongnu.org; Fri, 28 Mar 2014 13:59:25 -0400 Message-ID: <5335B87A.3090105@suse.de> Date: Fri, 28 Mar 2014 18:59:22 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <20140328105709.21018.88000.stgit@bahia.local> <20140328105717.21018.17649.stgit@bahia.local> In-Reply-To: <20140328105717.21018.17649.stgit@bahia.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: kwolf@redhat.com, peter.maydell@linaro.org, thuth@linux.vnet.ibm.com, mst@redhat.com, marc.zyngier@arm.com, rusty@rustcorp.com.au, agraf@suse.de, qemu-devel@nongnu.org, Juan Quintela , stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, anthony@codemonkey.ws Am 28.03.2014 11:57, schrieb Greg Kurz: > From: Rusty Russell >=20 > virtio data structures are defined as "target endian", which assumes > that's a fixed value. In fact, that actually means it's platform-speci= fic. > The OASIS virtio 1.0 spec will fix this, by making all little endian. >=20 > We need to support both implementations and we want to share as much co= de > as possible. >=20 > A good way to do it is to introduce a per-device boolean property to te= ll > memory accessors whether they should swap bytes or not. This flag shoul= d > be set at device reset time, because: > - endianness won't change while the device is in use, and if we reboot > into a different endianness, a new device reset will occur > - as suggested by Alexander Graf, we can keep all the logic to set the > property in a single place and share all the virtio memory accessors > between the two implementations >=20 > For legacy devices, we rely on a per-platform hook to set the flag. The > virtio 1.0 implementation will just have to add some more logic in > virtio_reset() instead of calling the hook: >=20 > if (vdev->legacy) { > vdev->needs_byteswap =3D virtio_legacy_get_byteswap(); > } else { > #ifdef HOST_WORDS_BIGENDIAN > vdev->needs_byteswap =3D true; > #else > vdev->needs_byteswap =3D false; > #endif > } >=20 > The needs_byteswap flag is preserved accross migrations. "across" Why? :) For all targets except ppc this field does not change during runtime. Do you intend to support ppc64 <-> ppc64le migration, i.e. protection against changing HOST_WORDS_BIGENDIAN? Since you're setting the field on reset rather than in instance_init or realize, resetting the device on one host with changing ILE may lead to weird endianness settings: Devices are initially reset before the VM starts. ILE will always be Big Endian then IIUC, since all PowerPCCPU models default to Big Endian and SLOF runs Big Endian. SLOF might use a virtio-blk device to boot from. We then boot into SLES12 ppc64le - ILE indicates Little Endian now. As soon as the guest triggers a reset of the device, such as by resetting the whole PCI bus, endianness changes to Little Endian. Now you indeed have an endianness on the source that is different from that of the newly Big Endian reset device on the destination. Is this desired, or did you accidentally initialize the flag in the wrong place? If we do need it, maybe you can place the field into a subsection to avoid imposing it on x86? Regards, Andreas >=20 > Signed-off-by: Rusty Russell > [ fixed checkpatch.pl error with the virtio_byteswap initialisation, > ldq_phys() API change, > relicensed virtio-access.h to GPLv2+ on Rusty's request, > introduce a per-device needs_byteswap flag, > add VirtIODevice * arg to virtio helpers, > rename virtio_get_byteswap to virtio_legacy_get_byteswap, > Greg Kurz ] > Signed-off-by: Greg Kurz --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg