From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51778) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7SKL-00012n-TY for qemu-devel@nongnu.org; Thu, 08 Aug 2013 11:40:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7SKG-0002tj-9h for qemu-devel@nongnu.org; Thu, 08 Aug 2013 11:40:37 -0400 Received: from mail-ie0-f169.google.com ([209.85.223.169]:34612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7SKG-0002tY-4s for qemu-devel@nongnu.org; Thu, 08 Aug 2013 11:40:32 -0400 Received: by mail-ie0-f169.google.com with SMTP id qd12so2171060ieb.0 for ; Thu, 08 Aug 2013 08:40:31 -0700 (PDT) From: Anthony Liguori In-Reply-To: <5203AB19.9070505@suse.de> References: <1375938949-22622-1-git-send-email-rusty@rustcorp.com.au> <1375938949-22622-2-git-send-email-rusty@rustcorp.com.au> <87li4cgvh1.fsf@codemonkey.ws> <5203AB19.9070505@suse.de> Date: Thu, 08 Aug 2013 10:40:28 -0500 Message-ID: <87r4e4p4wj.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= , Rusty Russell Cc: Peter Maydell , qemu-devel@nongnu.org Andreas F=C3=A4rber writes: > Am 08.08.2013 15:31, schrieb Anthony Liguori: >> Rusty Russell writes: >>=20 >>> Virtio is currently defined to work as "guest endian", but this is a >>> problem if the guest can change endian. As most targets can't change >>> endian, we make it a per-target option to avoid pessimising. >>> >>> This is based on a simpler patch by Anthony Liguouri, which only handled >>> the vring accesses. We also need some drivers to access these helpers, >>> eg. for data which contains headers. >>> >>> Signed-off-by: Rusty Russell >>> --- >>> hw/virtio/virtio.c | 46 +++++++++---- >>> include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++= ++++++++ >>> 2 files changed, 170 insertions(+), 14 deletions(-) >>> create mode 100644 include/hw/virtio/virtio-access.h >>> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 8176c14..2887f17 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -18,6 +18,7 @@ >>> #include "hw/virtio/virtio.h" >>> #include "qemu/atomic.h" >>> #include "hw/virtio/virtio-bus.h" >>> +#include "hw/virtio/virtio-access.h" >>>=20=20 >>> /* The alignment to use between consumer and producer parts of vring. >>> * x86 pagesize again. */ >>> @@ -84,6 +85,20 @@ struct VirtQueue >>> EventNotifier host_notifier; >>> }; >>>=20=20 >>> +#ifdef TARGET_VIRTIO_SWAPENDIAN >>> +bool virtio_byteswap; >>> + >>> +/* Ask target code if we should swap endian for all vring and config a= ccess. */ >>> +static void mark_endian(void) >>> +{ >>> + virtio_byteswap =3D virtio_swap_endian(); >>> +} >>> +#else >>> +static void mark_endian(void) >>> +{ >>> +} >>> +#endif >>> + >>=20 >> It would be very good to avoid a target specific define here. We would >> like to move to only building a single copy of the virtio code. > > +1 > >> We have a mechanism to do weak functions via stubs/. I think it would >> be better to do cpu_get_byteswap() as a stub function and then overload >> it in the ppc64 code. > > If this as your name indicates is a per-CPU function then it should go > into CPUClass. Interesting question is, what is virtio supposed to do if > we have two ppc CPUs, one is Big Endian, the other is Little Endian. PPC64 is big endian. AFAIK, there is no such thing as a little endian PPC64 processor. This is just a processor mode where loads/stores are byte lane swapped. Hence the name 'cpu_get_byteswap'. It's just asking whether the load/stores are being swapped or not. At least for PPC64, it's not possible to enable/disable byte lane swapping for individual CPUs. It's done through a system-wide hcall. FWIW, I think most bi-endian architectures are this way too so I think this is equally applicable to ARM. Peter, is that right? Regards, Anthony Liguori > We'd need to check current_cpu then, which for Xen is always NULL. > > Andreas > > --=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= =BCrnberg