Re: [Qemu-devel] [PATCH v2 3/7] memory: iommu support

[Avi Kivity <avi@redhat.com>]

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