From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41628) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YWRRv-0004zN-Me for qemu-devel@nongnu.org; Fri, 13 Mar 2015 11:24:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YWRRr-00012l-Ln for qemu-devel@nongnu.org; Fri, 13 Mar 2015 11:24:31 -0400 Date: Fri, 13 Mar 2015 16:24:03 +0100 From: "Michael S. Tsirkin" Message-ID: <20150313162346-mutt-send-email-mst@redhat.com> References: <20150313090312.742d135c@bahia.local> <20150313081125.4669.92074.stgit@bahia.local> <5502C49E.2020908@redhat.com> <20150313123212.481ce3f6@bahia.local> <5502F9C2.7040800@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5502F9C2.7040800@redhat.com> 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: Paolo Bonzini Cc: Michael Roth , qemu-stable@nongnu.org, Cedric Le Goater , qemu-devel@nongnu.org On Fri, Mar 13, 2015 at 03:52:50PM +0100, Paolo Bonzini wrote: >=20 >=20 > 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 p= pc64le > >>> guest (only virtqueue 0 was handled, all others being byteswapped b= ecause > >>> of TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fi= xed > >>> endian architectures (i.e. doesn't break x86). > >>> > >>> Reported-by: C=E9dric 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(Mem= oryRegion *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_endian= ness(mr)); > >>> + adjust_endianness(&mrfd.data, size, eventfd_wrong_endianness(m= r)); > >> > >> Strictly speaking, the place to do this would be kvm_set_ioeventfd_m= mio. > >=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 endiannes= s to > > kvm_set_ioeventfd_mmio() so it can fix the ordering ? >=20 > Yes. >=20 > >> A hypothetical userspace ioeventfd emulation would not need the swa= p. > >=20 > > I don't understand why "would not need the swap"... >=20 > 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. >=20 > 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_st= ore. >=20 > Does something like this work? >=20 > #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 >=20 > in kvm_set_ioeventfd_mmio and kvm_set_ioeventfd_pio? >=20 > Paolo Maybe you can post a patch? Would be easier ... > >=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_endian= ness(mr)); > >>> + adjust_endianness(&mrfd.data, size, eventfd_wrong_endianness(m= r)); > >>> memory_region_transaction_begin(); > >>> for (i =3D 0; i < mr->ioeventfd_nb; ++i) { > >>> if (memory_region_ioeventfd_equal(mrfd, mr->ioeventfds[i])= ) { > >>> > >> > >=20