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

[Aurelien Jarno]

On Tue, Dec 22, 2009 at 02:45:17PM +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.

I have to say I found it too hacky to be included in QEMU. I would
prefer if someone can provide a real fix.

> Signed-off-by: Alexander Graf <address@hidden>
> CC: Laurent Vivier <address@hidden>
> 
> ---
> 
> V1 -> V2:
> 
>   - s/cpu_to_be32/dbdma_cpu_to_be32/g
>   - s/be32_to_cpu/dbdma_be32_to_cpu/g
> ---
>  hw/mac_dbdma.c |   86 ++++++++++++++++++++++++++++++-------------------------
>  1 files changed, 47 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/mac_dbdma.c b/hw/mac_dbdma.c
> index 98dccfd..555983a 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 dbdma_cpu_to_be32 bswap32
> +#define dbdma_be32_to_cpu bswap32
> +
>  /* debug DBDMA */
>  //#define DEBUG_DBDMA
>  
> @@ -184,19 +192,19 @@ static void dump_dbdma_cmd(dbdma_cmd *cmd)
>  static void dbdma_cmdptr_load(DBDMA_channel *ch)
>  {
>      DBDMA_DPRINTF("dbdma_cmdptr_load 0x%08x\n",
> -                  be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]));
> -    cpu_physical_memory_read(be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]),
> +                  dbdma_be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]));
> +    cpu_physical_memory_read(dbdma_be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]),
>                               (uint8_t*)&ch->current, sizeof(dbdma_cmd));
>  }
>  
>  static void dbdma_cmdptr_save(DBDMA_channel *ch)
>  {
>      DBDMA_DPRINTF("dbdma_cmdptr_save 0x%08x\n",
> -                  be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]));
> +                  dbdma_be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]));
>      DBDMA_DPRINTF("xfer_status 0x%08x res_count 0x%04x\n",
>                    le16_to_cpu(ch->current.xfer_status),
>                    le16_to_cpu(ch->current.res_count));
> -    cpu_physical_memory_write(be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]),
> +    cpu_physical_memory_write(dbdma_be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]),
>                                (uint8_t*)&ch->current, sizeof(dbdma_cmd));
>  }
>  
> @@ -204,8 +212,8 @@ static void kill_channel(DBDMA_channel *ch)
>  {
>      DBDMA_DPRINTF("kill_channel\n");
>  
> -    ch->regs[DBDMA_STATUS] |= cpu_to_be32(DEAD);
> -    ch->regs[DBDMA_STATUS] &= cpu_to_be32(~ACTIVE);
> +    ch->regs[DBDMA_STATUS] |= dbdma_cpu_to_be32(DEAD);
> +    ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~ACTIVE);
>  
>      qemu_irq_raise(ch->irq);
>  }
> @@ -230,10 +238,10 @@ static void conditional_interrupt(DBDMA_channel *ch)
>          return;
>      }
>  
> -    status = be32_to_cpu(ch->regs[DBDMA_STATUS]) & DEVSTAT;
> +    status = dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]) & DEVSTAT;
>  
> -    sel_mask = (be32_to_cpu(ch->regs[DBDMA_INTR_SEL]) >> 16) & 0x0f;
> -    sel_value = be32_to_cpu(ch->regs[DBDMA_INTR_SEL]) & 0x0f;
> +    sel_mask = (dbdma_be32_to_cpu(ch->regs[DBDMA_INTR_SEL]) >> 16) & 0x0f;
> +    sel_value = dbdma_be32_to_cpu(ch->regs[DBDMA_INTR_SEL]) & 0x0f;
>  
>      cond = (status & sel_mask) == (sel_value & sel_mask);
>  
> @@ -268,10 +276,10 @@ static int conditional_wait(DBDMA_channel *ch)
>          return 1;
>      }
>  
> -    status = be32_to_cpu(ch->regs[DBDMA_STATUS]) & DEVSTAT;
> +    status = dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]) & DEVSTAT;
>  
> -    sel_mask = (be32_to_cpu(ch->regs[DBDMA_WAIT_SEL]) >> 16) & 0x0f;
> -    sel_value = be32_to_cpu(ch->regs[DBDMA_WAIT_SEL]) & 0x0f;
> +    sel_mask = (dbdma_be32_to_cpu(ch->regs[DBDMA_WAIT_SEL]) >> 16) & 0x0f;
> +    sel_value = dbdma_be32_to_cpu(ch->regs[DBDMA_WAIT_SEL]) & 0x0f;
>  
>      cond = (status & sel_mask) == (sel_value & sel_mask);
>  
> @@ -292,10 +300,10 @@ static void next(DBDMA_channel *ch)
>  {
>      uint32_t cp;
>  
> -    ch->regs[DBDMA_STATUS] &= cpu_to_be32(~BT);
> +    ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~BT);
>  
> -    cp = be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]);
> -    ch->regs[DBDMA_CMDPTR_LO] = cpu_to_be32(cp + sizeof(dbdma_cmd));
> +    cp = dbdma_be32_to_cpu(ch->regs[DBDMA_CMDPTR_LO]);
> +    ch->regs[DBDMA_CMDPTR_LO] = dbdma_cpu_to_be32(cp + sizeof(dbdma_cmd));
>      dbdma_cmdptr_load(ch);
>  }
>  
> @@ -304,7 +312,7 @@ static void branch(DBDMA_channel *ch)
>      dbdma_cmd *current = &ch->current;
>  
>      ch->regs[DBDMA_CMDPTR_LO] = current->cmd_dep;
> -    ch->regs[DBDMA_STATUS] |= cpu_to_be32(BT);
> +    ch->regs[DBDMA_STATUS] |= dbdma_cpu_to_be32(BT);
>      dbdma_cmdptr_load(ch);
>  }
>  
> @@ -331,10 +339,10 @@ static void conditional_branch(DBDMA_channel *ch)
>          return;
>      }
>  
> -    status = be32_to_cpu(ch->regs[DBDMA_STATUS]) & DEVSTAT;
> +    status = dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]) & DEVSTAT;
>  
> -    sel_mask = (be32_to_cpu(ch->regs[DBDMA_BRANCH_SEL]) >> 16) & 0x0f;
> -    sel_value = be32_to_cpu(ch->regs[DBDMA_BRANCH_SEL]) & 0x0f;
> +    sel_mask = (dbdma_be32_to_cpu(ch->regs[DBDMA_BRANCH_SEL]) >> 16) & 0x0f;
> +    sel_value = dbdma_be32_to_cpu(ch->regs[DBDMA_BRANCH_SEL]) & 0x0f;
>  
>      cond = (status & sel_mask) == (sel_value & sel_mask);
>  
> @@ -365,19 +373,19 @@ static void dbdma_end(DBDMA_io *io)
>      if (conditional_wait(ch))
>          goto wait;
>  
> -    current->xfer_status = cpu_to_le16(be32_to_cpu(ch->regs[DBDMA_STATUS]));
> -    current->res_count = cpu_to_le16(be32_to_cpu(io->len));
> +    current->xfer_status = 
> cpu_to_le16(dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]));
> +    current->res_count = cpu_to_le16(dbdma_be32_to_cpu(io->len));
>      dbdma_cmdptr_save(ch);
>      if (io->is_last)
> -        ch->regs[DBDMA_STATUS] &= cpu_to_be32(~FLUSH);
> +        ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~FLUSH);
>  
>      conditional_interrupt(ch);
>      conditional_branch(ch);
>  
>  wait:
>      ch->processing = 0;
> -    if ((ch->regs[DBDMA_STATUS] & cpu_to_be32(RUN)) &&
> -        (ch->regs[DBDMA_STATUS] & cpu_to_be32(ACTIVE)))
> +    if ((ch->regs[DBDMA_STATUS] & dbdma_cpu_to_be32(RUN)) &&
> +        (ch->regs[DBDMA_STATUS] & dbdma_cpu_to_be32(ACTIVE)))
>          channel_run(ch);
>  }
>  
> @@ -456,9 +464,9 @@ static void load_word(DBDMA_channel *ch, int key, 
> uint32_t addr,
>      if (conditional_wait(ch))
>          goto wait;
>  
> -    current->xfer_status = cpu_to_le16(be32_to_cpu(ch->regs[DBDMA_STATUS]));
> +    current->xfer_status = 
> cpu_to_le16(dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]));
>      dbdma_cmdptr_save(ch);
> -    ch->regs[DBDMA_STATUS] &= cpu_to_be32(~FLUSH);
> +    ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~FLUSH);
>  
>      conditional_interrupt(ch);
>      next(ch);
> @@ -494,9 +502,9 @@ static void store_word(DBDMA_channel *ch, int key, 
> uint32_t addr,
>      if (conditional_wait(ch))
>          goto wait;
>  
> -    current->xfer_status = cpu_to_le16(be32_to_cpu(ch->regs[DBDMA_STATUS]));
> +    current->xfer_status = 
> cpu_to_le16(dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]));
>      dbdma_cmdptr_save(ch);
> -    ch->regs[DBDMA_STATUS] &= cpu_to_be32(~FLUSH);
> +    ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~FLUSH);
>  
>      conditional_interrupt(ch);
>      next(ch);
> @@ -512,7 +520,7 @@ static void nop(DBDMA_channel *ch)
>      if (conditional_wait(ch))
>          goto wait;
>  
> -    current->xfer_status = cpu_to_le16(be32_to_cpu(ch->regs[DBDMA_STATUS]));
> +    current->xfer_status = 
> cpu_to_le16(dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]));
>      dbdma_cmdptr_save(ch);
>  
>      conditional_interrupt(ch);
> @@ -524,7 +532,7 @@ wait:
>  
>  static void stop(DBDMA_channel *ch)
>  {
> -    ch->regs[DBDMA_STATUS] &= cpu_to_be32(~(ACTIVE|DEAD|FLUSH));
> +    ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~(ACTIVE|DEAD|FLUSH));
>  
>      /* the stop command does not increment command pointer */
>  }
> @@ -541,7 +549,7 @@ static void channel_run(DBDMA_channel *ch)
>  
>      /* clear WAKE flag at command fetch */
>  
> -    ch->regs[DBDMA_STATUS] &= cpu_to_be32(~WAKE);
> +    ch->regs[DBDMA_STATUS] &= dbdma_cpu_to_be32(~WAKE);
>  
>      cmd = le16_to_cpu(current->command) & COMMAND_MASK;
>  
> @@ -618,7 +626,7 @@ static void DBDMA_run (DBDMA_channel *ch)
>      int channel;
>  
>      for (channel = 0; channel < DBDMA_CHANNELS; channel++, ch++) {
> -            uint32_t status = be32_to_cpu(ch->regs[DBDMA_STATUS]);
> +            uint32_t status = dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]);
>              if (!ch->processing && (status & RUN) && (status & ACTIVE))
>                  channel_run(ch);
>      }
> @@ -660,12 +668,12 @@ dbdma_control_write(DBDMA_channel *ch)
>      uint16_t mask, value;
>      uint32_t status;
>  
> -    mask = (be32_to_cpu(ch->regs[DBDMA_CONTROL]) >> 16) & 0xffff;
> -    value = be32_to_cpu(ch->regs[DBDMA_CONTROL]) & 0xffff;
> +    mask = (dbdma_be32_to_cpu(ch->regs[DBDMA_CONTROL]) >> 16) & 0xffff;
> +    value = dbdma_be32_to_cpu(ch->regs[DBDMA_CONTROL]) & 0xffff;
>  
>      value &= (RUN | PAUSE | FLUSH | WAKE | DEVSTAT);
>  
> -    status = be32_to_cpu(ch->regs[DBDMA_STATUS]);
> +    status = dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]);
>  
>      status = (value & mask) | (status & ~mask);
>  
> @@ -677,14 +685,14 @@ dbdma_control_write(DBDMA_channel *ch)
>      }
>      if (status & PAUSE)
>          status &= ~ACTIVE;
> -    if ((be32_to_cpu(ch->regs[DBDMA_STATUS]) & RUN) && !(status & RUN)) {
> +    if ((dbdma_be32_to_cpu(ch->regs[DBDMA_STATUS]) & RUN) && !(status & 
> RUN)) {
>          /* RUN is cleared */
>          status &= ~(ACTIVE|DEAD);
>      }
>  
>      DBDMA_DPRINTF("    status 0x%08x\n", status);
>  
> -    ch->regs[DBDMA_STATUS] = cpu_to_be32(status);
> +    ch->regs[DBDMA_STATUS] = dbdma_cpu_to_be32(status);
>  
>      if (status & ACTIVE)
>          qemu_bh_schedule(dbdma_bh);
> @@ -706,7 +714,7 @@ static void dbdma_writel (void *opaque,
>      /* cmdptr cannot be modified if channel is RUN or ACTIVE */
>  
>      if (reg == DBDMA_CMDPTR_LO &&
> -        (ch->regs[DBDMA_STATUS] & cpu_to_be32(RUN | ACTIVE)))
> +        (ch->regs[DBDMA_STATUS] & dbdma_cpu_to_be32(RUN | ACTIVE)))
>       return;
>  
>      ch->regs[reg] = value;
> @@ -717,7 +725,7 @@ static void dbdma_writel (void *opaque,
>          break;
>      case DBDMA_CMDPTR_LO:
>          /* 16-byte aligned */
> -        ch->regs[DBDMA_CMDPTR_LO] &= cpu_to_be32(~0xf);
> +        ch->regs[DBDMA_CMDPTR_LO] &= dbdma_cpu_to_be32(~0xf);
>          dbdma_cmdptr_load(ch);
>          break;
>      case DBDMA_STATUS:
> -- 
> 1.6.0.2
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net