From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33922) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YWQxc-0002sq-Ke for qemu-devel@nongnu.org; Fri, 13 Mar 2015 10:53:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YWQxb-0005AL-Go for qemu-devel@nongnu.org; Fri, 13 Mar 2015 10:53:12 -0400 Message-ID: <5502F9C2.7040800@redhat.com> Date: Fri, 13 Mar 2015 15:52:50 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <20150313090312.742d135c@bahia.local> <20150313081125.4669.92074.stgit@bahia.local> <5502C49E.2020908@redhat.com> <20150313123212.481ce3f6@bahia.local> In-Reply-To: <20150313123212.481ce3f6@bahia.local> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Michael Roth , Cedric Le Goater , "Michael S. Tsirkin" , qemu-devel@nongnu.org, qemu-stable@nongnu.org On 13/03/2015 12:32, Greg Kurz wrote: > On Fri, 13 Mar 2015 12:06:06 +0100 > Paolo Bonzini wrote: >> >> >> On 13/03/2015 09:11, Greg Kurz wrote: >>> The data argument is a host entity. It is not related to the target >>> endianness. Let's introduce a HOST_WORDS_BIGENDIAN based helper for >>> that. >>> >>> This patch fixes ioeventfd and vhost for a ppc64le host running a ppc= 64le >>> guest (only virtqueue 0 was handled, all others being byteswapped bec= ause >>> of TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixe= d >>> endian architectures (i.e. doesn't break x86). >>> >>> Reported-by: C=C3=A9dric Le Goater >>> Signed-off-by: Greg Kurz >>> --- >>> memory.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/memory.c b/memory.c >>> index 6291cc0..1e29d40 100644 >>> --- a/memory.c >>> +++ b/memory.c >>> @@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(Memor= yRegion *mr) >>> } >>> } >>> =20 >>> +static bool eventfd_wrong_endianness(MemoryRegion *mr) >>> +{ >>> +#ifdef HOST_WORDS_BIGENDIAN >>> + return mr->ops->endianness =3D=3D DEVICE_LITTLE_ENDIAN; >>> +#else >>> + return mr->ops->endianness =3D=3D DEVICE_BIG_ENDIAN; >>> +#endif >>> +} >>> + >>> void memory_region_add_eventfd(MemoryRegion *mr, >>> hwaddr addr, >>> unsigned size, >>> @@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr= , >>> }; >>> unsigned i; >>> =20 >>> - adjust_endianness(&mrfd.data, size, memory_region_wrong_endianne= ss(mr)); >>> + adjust_endianness(&mrfd.data, size, eventfd_wrong_endianness(mr)= ); >> >> Strictly speaking, the place to do this would be kvm_set_ioeventfd_mmi= o. >=20 > FWIW the swap is being done in memory.c since commit: >=20 > commit 28f362be6e7f45ea9b7a57a08555c4c784f36198 > Author: Alexander Graf > Date: Mon Oct 15 20:30:28 2012 +0200 >=20 > memory: Make eventfd adhere to device endianness >=20 > Are you asking to revert this commit and to pass the device endianness = to > kvm_set_ioeventfd_mmio() so it can fix the ordering ? Yes. >> A hypothetical userspace ioeventfd emulation would not need the swap. >=20 > I don't understand why "would not need the swap"... Because the userspace ioeventfd emulation would look at the value as it comes from the target CPU, in target CPU endianness. So it would have a swap done at ioeventfd time, and a swap done at access time. Host endianness doesn't matter. Here, QEMU believes that the target's natural endianness is big-endian, but the kernel believes that the target's natural endianness is little-endian (based on MSR_KERNEL). So there is a different number of swaps in memory_region_add_eventfd and in the kernel's kvmppc_handle_stor= e. Does something like this work? #if defined(HOST_WORDS_BIGENDIAN) !=3D defined(TARGET_WORDS_BIGENDIAN) /* The kernel expects ioeventfd values in HOST_WORDS_BIGENDIAN * endianness, but the memory core hands them in target endianness. * For example, PPC is always treated as big-endian even if running * on KVM and on PPC64LE. Correct here. */ switch(size) { case 2: val =3D bswap16(val); break; case 4: val =3D bswap32(val); break; } #endif in kvm_set_ioeventfd_mmio and kvm_set_ioeventfd_pio? Paolo >=20 >> I can accept the patch, but it's better to add a comment. >> >=20 > ... and so I don't know what to write. :) >=20 > Please enlight ! >=20 > -- > Greg >=20 >> Paolo >> >>> memory_region_transaction_begin(); >>> for (i =3D 0; i < mr->ioeventfd_nb; ++i) { >>> if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i]))= { >>> @@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr= , >>> }; >>> unsigned i; >>> =20 >>> - adjust_endianness(&mrfd.data, size, memory_region_wrong_endianne= ss(mr)); >>> + adjust_endianness(&mrfd.data, size, eventfd_wrong_endianness(mr)= ); >>> memory_region_transaction_begin(); >>> for (i =3D 0; i < mr->ioeventfd_nb; ++i) { >>> if (memory_region_ioeventfd_equal(mrfd, mr->ioeventfds[i])) = { >>> >> >=20