On 10/31/2012 08:59 PM, Benjamin Herrenschmidt wrote: > On Wed, 2012-10-31 at 12:32 +0200, Avi Kivity wrote: >> This has nothing to do with device endianness; we're translating from a >> byte buffer (address_space_rw()) to a uint64_t >> (MemoryRegionOps::read/write()) so we need to take host endianess into >> account. >> >> This code cleverly makes use of memory core endian support to do the >> conversion for us. But it's probably too clever and should be replaced >> by an explicit call to a helper. > > Well, the whole idea of providing sized-type accessors (u8/u16/...) is > very fishy for DMA. I understand it comes from the MemoryRegion ops, and > It made somewhat sense in CPU to device (though even then endianness had > always gotten wrong) but for DMA it's going to be a can of worms.
Endianness here has no effect on the result. An address_space_rw() causes a lookup of a memory region, which happens to be an iommu memory region. Because of the way MemoryRegionOps are done, it is converted to a 1/2/4/8 accessor, and then converted immediately back to a byte array. As long as we're consistent there's no change to the data path. However we do have a problem with non-1/2/4/8 byte writes. Right now any mismatched access ends up as an 8 byte write, we need an extra accessor for arbitrary writes, or rather better use of the .impl members of MemoryRegionOps. > > Taking "host endianness" into account means nothing if you don't define > what endianness those are expected to use to write to memory. If you add > yet another case of "guest endianness" in the mix, then you make yet > another mistake akin to the virtio trainwreck since things like powerpc > or ARM can operate in different endianness. > > The best thing to do is to forbid use of those functions :-) And > basically mandate the use of endian explicit accessor that specifically > indicate what endianness the value should have once written in target > memory. The most sensible expectation when using the "raw" Memory ops is > to have the value go "as is" ie in host endianness. -- error compiling committee.c: too many arguments to function