[Qemu-devel] Re: [PATCH] Always swap endianness in DBDMA

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 01:07:20PM +0100, Alexander Graf wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Dec 22, 2009 at 11:24:18AM +0100, Alexander Graf wrote:
> >   
> >> When we get an MMIO request, we always get variables in host endianness. 
> >> The
> >> only time we need to actually reverse byte order is when we read bytes from
> >> guest memory.
> >>
> >> Apparently the DBDMA implementation is different there. A lot of the logic
> >> in there depends on values being big endian. Now, qemu does all the 
> >> conversion
> >> in the MMIO handlers for us already though, so it turns out that we're in
> >> the same byte order from a C point of view, but cpu_to_be32 and be32_to_cpu
> >> end up being nops.
> >>
> >> This makes the code work differently on x86 (little endian) than on ppc 
> >> (big
> >> endian). On x86 it works, on ppc it doesn't.
> >>
> >> This patch (while being seriously hacky and ugly) makes dbdma emulation 
> >> work
> >> on ppc hosts. I'll leave the real fixing to someone else.
> >>     
> >
> > Come on, 
> >
> > #define cpu_to_dbdma32 bswap32
> > #define dbdma_to_cpu32 bswap32
> >
> > and then
> >
> > s/cpu_to_be32/cpu_to_dbdma32/g
> > s/be32_to_cpu/dbdma32_to_cpu/g
> >
> > is not too hard, is it?
> >   
> 
> Well, if I'd want to do a sed'ish approach I'd just
> 
> s/cpu_to_be32/bswap32/g
> s/be32_to_cpu/bswap32/g

This would throw away some information: it is better to know whether you
are converting from or to cpu mode.  Hopefully at some point we will add
sparce annotations which will make this even more important.

> The problem is that the whole define is just plain wrong which tells me
> that the code is using the bswap functions incorrectly. This really
> needs to be fixed by someone who knows the dbdma device. I don't see how
> calling incorrect calls even more incorrect makes any difference.
> 
> Alex

At least build will not break in strange ways e.g. when someone changes
cpu_to_be32 to a macro.  I don't really know about this hardware either,
but why make it more fragile than it already is?

-- 
MST