From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58003) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTc1E-0002ko-7j for qemu-devel@nongnu.org; Fri, 28 Mar 2014 15:00:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTc16-00006I-VB for qemu-devel@nongnu.org; Fri, 28 Mar 2014 15:00:44 -0400 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:37345) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTc16-00006C-Iv for qemu-devel@nongnu.org; Fri, 28 Mar 2014 15:00:36 -0400 Received: from /spool/local by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Mar 2014 19:00:35 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id D05742190041 for ; Fri, 28 Mar 2014 19:00:28 +0000 (GMT) Received: from d06av12.portsmouth.uk.ibm.com (d06av12.portsmouth.uk.ibm.com [9.149.37.247]) by b06cxnps4075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2SJ0M7G19464280 for ; Fri, 28 Mar 2014 19:00:22 GMT Received: from d06av12.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av12.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2SJ0Vm4005045 for ; Fri, 28 Mar 2014 13:00:33 -0600 Date: Fri, 28 Mar 2014 20:00:27 +0100 From: Greg Kurz Message-ID: <20140328200027.23ea3977@bahia.local> In-Reply-To: <5335B87A.3090105@suse.de> References: <20140328105709.21018.88000.stgit@bahia.local> <20140328105717.21018.17649.stgit@bahia.local> <5335B87A.3090105@suse.de> MIME-Version: 1.0 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: Andreas =?UTF-8?B?RsOkcmJlcg==?= 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 On Fri, 28 Mar 2014 18:59:22 +0100 Andreas F=C3=A4rber wrote: > 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 should > > 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. >=20 > "across" >=20 > 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? >=20 Because we only set this property at virtio_reset() time... how can we ensure it still has the correct value after a migration then ? And no, I don't intend to support cross-endian migration... The only endianness change that we can support is rebooting into a different endianness. > 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=20 > 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? >=20 Hmmm... the assumption is that ALL virtio devices get reset after the guest kernel switches to LE. Are you saying this is not the case if SLOF uses a virtio-blk device to boot from ? This device would be handed over to the guest kernel to be used as is ? If yes, then I don't see how we can cope with that... The way legacy virtio is implemented, we cannot reasonably support an endianness change without fully reseting the device. I guess this is the main motivation behind virtio 1.0 :) > If we do need it, maybe you can place the field into a subsection to > avoid imposing it on x86? >=20 I think it is needed anyway as long as we want to support a ppc64 guest that can change endianness and uses legacy virtio devices, even with a x86 host. > Regards, > Andreas >=20 > >=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 Cheers. --=20 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.