On Tue, 21 Feb 2017 18:09:04 +0000 Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 21 February 2017 at 16:34, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > > > > On 21/02/2017 17:21, Alex Williamson wrote: > >> On Tue, 21 Feb 2017 14:46:55 +0800 > >> Yongji Xie <xyj...@linux.vnet.ibm.com> wrote: > >> > >>> At the moment ram device's memory regions are NATIVE_ENDIAN. This does > >>> not work on PPC64 because VFIO PCI device is little endian but PPC64 > >>> always defines static macro TARGET_WORDS_BIGENDIAN. > >>> > >>> This fixes endianness for ram device the same way as it is done > >>> for VFIO region in commit 6758008e2c4e79fb6bd04fe8e7a41665fa583965. > >> > >> The referenced commit was to vfio code where the endianness is fixed, > >> here you're modifying shared generic code to assume the same > >> endianness as vfio. That seems wrong. > > > > Is the goal to have the same endianness as VFIO? Or is it just a trick > > to ensure the number of swaps is always 0 or 2, so that they cancel away? > > > > In other words, would Yongji's patch just work if it used > > DEVICE_BIG_ENDIAN and beNN_to_cpu/cpu_to_beNN? If so, then I think the > > patch is okay. > > I think any patch that proposes adding or removing one or more > endianness related swaps should come with a commit message that > states very clearly why this exact point in the stack is the > correct place to do these swaps, from a design point of view. > (This is so we can avoid the pitfall of putting in enough swaps > to cancel each other out but at the wrong point in the design.) > > In this instance I don't understand the patch. The ram_device > mem-ops are there to deal with memory regions backed by a > lump of RAM, right? Lumps of memory are always the endianness > of the host CPU by definition, so DEVICE_NATIVE_ENDIAN and > no swapping in the accessors seems like it ought to be the right > thing... I'm glad I'm not the only one confused. The clarity I can add is that for vfio, we define that any read/write to the vfio file descriptor is little endian. The kernel does cpu_to_leXX/leXX_to_cpu around the ioreadXX/iowriteXX calls. For better or worse, this gets us around the implicit CPU endianness of the data there and gives us a standard endianness. mmaps are of course device native endian, but the point of the ram device was that some devices (realtek NICs) don't respond well to using memcpy through the mmap and we're better off using explicit reads and writes. The part where I get lost is that if PPC64 always sets the target to big endian, then adjust_endianness() we will always bswap after a read and before a write. I don't follow how that bswap necessarily gets us to QEMU CPU endian such that the cpu_to_leXX/leXX_to_cpu in the access functions always do the right thing. I know we do this elsewhere in vfio, perhaps I've deferred to someone convincing me it was the right thing at the time, but I'm not able to derive it myself currently. To answer Paolo's question, if PPC64 sets TARGET_WORDS_BIGENDIAN, which is a compile time setting, then using DEVICE_BIG_ENDIAN would deterministically remove the bswap from adjust_endianness(), but I don't know how anything is deterministic about CPU-relative byte swaps since (I think) PPC64 could actually be running in either. +1 to being very explicit about which swaps are occurring where. Thanks, Alex
