Patchwork Always swap endianness in DBDMA

login
register
mail settings
Submitter Alexander Graf
Date Dec. 22, 2009, 10:24 a.m.
Message ID <1261477458-26222-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/41605/
State New
Headers show

Comments

Alexander Graf - Dec. 22, 2009, 10:24 a.m.
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.

Signed-off-by: Alexander Graf <agraf@suse.de>
CC: Laurent Vivier <Laurent@vivier.eu>
---
 hw/mac_dbdma.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Dec. 22, 2009, 11:36 a.m.
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?

> Signed-off-by: Alexander Graf <agraf@suse.de>
> CC: Laurent Vivier <Laurent@vivier.eu>
> ---
>  hw/mac_dbdma.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/mac_dbdma.c b/hw/mac_dbdma.c
> index 98dccfd..4dbfc16 100644
> --- a/hw/mac_dbdma.c
> +++ b/hw/mac_dbdma.c
> @@ -40,6 +40,14 @@
>  #include "isa.h"
>  #include "mac_dbdma.h"
>  
> +/*
> + * XXX This is just plain wrong. Apparently we don't want to have big endian
> + *     values, but reversed endian ones. The code as is doesn't work on big
> + *     endian hosts. With these defines it does.
> + */
> +#define cpu_to_be32 bswap32
> +#define be32_to_cpu bswap32
> +
>  /* debug DBDMA */
>  //#define DEBUG_DBDMA
>  
> -- 
> 1.6.0.2
> 
>
Alexander Graf - Dec. 22, 2009, 12:07 p.m.
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

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
Michael S. Tsirkin - Dec. 22, 2009, 12:47 p.m.
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?
Paul Brook - Dec. 23, 2009, 10:55 a.m.
> 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.

The real problem is that devices shouldn't be doing byteswapping at all. This 
should be determined by the (currently non-existant) bus layers and 
implemented in generic code before the device callback.

The current code[1] is a nasty hack that sort-of works for most of the current 
machines because all devices happen to be connected the same way.  However 
there are other machines (e.g. ixp4xx) some peripherals are connected 
natively, whereas others are cross-wired.

On a related note, I'm not sure what the author of mac_bdbma.c was smoking[2]. 
It appears to keep register values in big-endian form for no good reason. Much 
easier would be to store them in native form, and just do the byteswapping 
when accessed by the CPU.

Paul

[1] e.g. vga.c:vga_mem_readw
[2] My guess is that the code is cribbed from elsewhere, and the original 
source gave the CPU direct access to the ch->regs[] array.
Laurent Vivier - Dec. 23, 2009, 11:39 p.m.
Le mercredi 23 décembre 2009 à 10:55 +0000, Paul Brook a écrit :
> > 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.
> 
> The real problem is that devices shouldn't be doing byteswapping at all. This 
> should be determined by the (currently non-existant) bus layers and 
> implemented in generic code before the device callback.
> 
> The current code[1] is a nasty hack that sort-of works for most of the current 
> machines because all devices happen to be connected the same way.  However 
> there are other machines (e.g. ixp4xx) some peripherals are connected 
> natively, whereas others are cross-wired.
> 
> On a related note, I'm not sure what the author of mac_bdbma.c was smoking[2]. 

I didn't smoke... just need some sleep.

> It appears to keep register values in big-endian form for no good reason. Much 
> easier would be to store them in native form, and just do the byteswapping 
> when accessed by the CPU.
> 
> Paul
> 
> [1] e.g. vga.c:vga_mem_readw
> [2] My guess is that the code is cribbed from elsewhere, and the original 
> source gave the CPU direct access to the ch->regs[] array.

>From pearpc io/macio/macio.cc.

Regards,
Laurent
Laurent Vivier - Dec. 23, 2009, 11:58 p.m.
Le mercredi 23 décembre 2009 à 10:55 +0000, Paul Brook a écrit :
> [2] My guess is that the code is cribbed from elsewhere, and the original 
> source gave the CPU direct access to the ch->regs[] array.

In fact it comes from Mac-On-Linux, src/drivers/dbdma.c

Laurent
Aurelien Jarno - Dec. 24, 2009, 12:08 a.m.
On Thu, Dec 24, 2009 at 12:39:36AM +0100, Laurent Vivier wrote:
> Le mercredi 23 décembre 2009 à 10:55 +0000, Paul Brook a écrit :
> > > 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.
> > 
> > The real problem is that devices shouldn't be doing byteswapping at all. This 
> > should be determined by the (currently non-existant) bus layers and 
> > implemented in generic code before the device callback.
> > 
> > The current code[1] is a nasty hack that sort-of works for most of the current 
> > machines because all devices happen to be connected the same way.  However 
> > there are other machines (e.g. ixp4xx) some peripherals are connected 
> > natively, whereas others are cross-wired.
> > 
> > On a related note, I'm not sure what the author of mac_bdbma.c was smoking[2]. 
> 
> I didn't smoke... just need some sleep.
> 
> > It appears to keep register values in big-endian form for no good reason. Much 
> > easier would be to store them in native form, and just do the byteswapping 
> > when accessed by the CPU.
> > 

I have just posted a patch that does that, and also simulate the bus is
connected backward, using the same trick as in other devices. Alexander,
could you please try if it works for you on a big endian host?
Alexander Graf - Dec. 24, 2009, 10:01 a.m.
On 24.12.2009, at 01:08, Aurelien Jarno wrote:

> On Thu, Dec 24, 2009 at 12:39:36AM +0100, Laurent Vivier wrote:
>> Le mercredi 23 décembre 2009 à 10:55 +0000, Paul Brook a écrit :
>>>> 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.
>>> 
>>> The real problem is that devices shouldn't be doing byteswapping at all. This 
>>> should be determined by the (currently non-existant) bus layers and 
>>> implemented in generic code before the device callback.
>>> 
>>> The current code[1] is a nasty hack that sort-of works for most of the current 
>>> machines because all devices happen to be connected the same way.  However 
>>> there are other machines (e.g. ixp4xx) some peripherals are connected 
>>> natively, whereas others are cross-wired.
>>> 
>>> On a related note, I'm not sure what the author of mac_bdbma.c was smoking[2]. 
>> 
>> I didn't smoke... just need some sleep.
>> 
>>> It appears to keep register values in big-endian form for no good reason. Much 
>>> easier would be to store them in native form, and just do the byteswapping 
>>> when accessed by the CPU.
>>> 
> 
> I have just posted a patch that does that, and also simulate the bus is
> connected backward, using the same trick as in other devices. Alexander,
> could you please try if it works for you on a big endian host?

I just did and it works on PPC guest, PPC64 host.

Alex

Patch

diff --git a/hw/mac_dbdma.c b/hw/mac_dbdma.c
index 98dccfd..4dbfc16 100644
--- a/hw/mac_dbdma.c
+++ b/hw/mac_dbdma.c
@@ -40,6 +40,14 @@ 
 #include "isa.h"
 #include "mac_dbdma.h"
 
+/*
+ * XXX This is just plain wrong. Apparently we don't want to have big endian
+ *     values, but reversed endian ones. The code as is doesn't work on big
+ *     endian hosts. With these defines it does.
+ */
+#define cpu_to_be32 bswap32
+#define be32_to_cpu bswap32
+
 /* debug DBDMA */
 //#define DEBUG_DBDMA