Re: [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion

[Blue Swirl]

On Fri, Dec 14, 2012 at 11:39 PM, Marcelo Tosatti <address@hidden> wrote:
>
> The high byte of the ioport address is necessary to compute
> the register address, see "82371AB PCI ISA IDE Xcelerator (PIIX4)"
> document, eg:
>
> 4.2.1.1. DCOM—DMA Command Register (IO)
> I/O Address: Channels 0–3—08h; Channels 4–7—0D0h
>
> Also the size of the region is wrong.

Is this true for other users, like MIPS and PPC?

>
> Fixes WinXP-32 installation.
>
> Signed-off-by: Marcelo Tosatti <address@hidden>
>
> diff --git a/hw/dma.c b/hw/dma.c
> index c2d7b21..e7de522 100644
> --- a/hw/dma.c
> +++ b/hw/dma.c
> @@ -56,6 +56,7 @@ static struct dma_cont {
>      uint8_t mask;
>      uint8_t flip_flop;
>      int dshift;
> +    int iobase;

Devices shouldn't care about their address. An address agnostic
approach would be to specify whether the device uses high channels or
low channels when instantiating it, based obviously on the address.

>      struct dma_regs regs[4];
>      qemu_irq *cpu_request_exit;
>      MemoryRegion channel_io;
> @@ -192,7 +193,8 @@ static void write_chan(void *opaque, hwaddr nport, 
> uint64_t data,
>      }
>  }
>
> -static void write_cont(void *opaque, hwaddr nport, uint64_t data,
> +
> +static void __write_cont(void *opaque, hwaddr nport, uint64_t data,
>                         unsigned size)

Please don't use identifiers with leading underscores, as explained in HACKING.

>  {
>      struct dma_cont *d = opaque;
> @@ -200,7 +202,7 @@ static void write_cont(void *opaque, hwaddr nport, 
> uint64_t data,
>
>      iport = (nport >> d->dshift) & 0x0f;
>      switch (iport) {
> -    case 0x01:                  /* command */
> +    case 0x08:                  /* command */
>          if ((data != 0) && (data & CMD_NOT_SUPPORTED)) {
>              dolog("command %"PRIx64" not supported\n", data);
>              return;
> @@ -208,7 +210,7 @@ static void write_cont(void *opaque, hwaddr nport, 
> uint64_t data,
>          d->command = data;
>          break;
>
> -    case 0x02:
> +    case 0x09:
>          ichan = data & 3;
>          if (data & 4) {
>              d->status |= 1 << (ichan + 4);
> @@ -220,7 +222,7 @@ static void write_cont(void *opaque, hwaddr nport, 
> uint64_t data,
>          DMA_run();
>          break;
>
> -    case 0x03:                  /* single mask */
> +    case 0x0a:                  /* single mask */
>          if (data & 4)
>              d->mask |= 1 << (data & 3);
>          else
> @@ -228,7 +230,7 @@ static void write_cont(void *opaque, hwaddr nport, 
> uint64_t data,
>          DMA_run();
>          break;
>
> -    case 0x04:                  /* mode */
> +    case 0x0b:                  /* mode */
>          {
>              ichan = data & 3;
>  #ifdef DEBUG_DMA
> @@ -247,23 +249,23 @@ static void write_cont(void *opaque, hwaddr nport, 
> uint64_t data,
>              break;
>          }
>
> -    case 0x05:                  /* clear flip flop */
> +    case 0x0c:                  /* clear flip flop */
>          d->flip_flop = 0;
>          break;
>
> -    case 0x06:                  /* reset */
> +    case 0x0d:                  /* reset */
>          d->flip_flop = 0;
>          d->mask = ~0;
>          d->status = 0;
>          d->command = 0;
>          break;
>
> -    case 0x07:                  /* clear mask for all channels */
> +    case 0x0e:                  /* clear mask for all channels */
>          d->mask = 0;
>          DMA_run();
>          break;
>
> -    case 0x08:                  /* write mask for all channels */
> +    case 0x0f:                  /* write mask for all channels */
>          d->mask = data;
>          DMA_run();
>          break;
> @@ -281,11 +283,22 @@ static void write_cont(void *opaque, hwaddr nport, 
> uint64_t data,
>  #endif
>  }
>
> +static void write_cont(void *opaque, hwaddr nport, uint64_t data,
> +                       unsigned size)
> +{
> +    struct dma_cont *d = opaque;
> +    int add = d->iobase + (8 << d->dshift);
> +
> +    __write_cont(opaque, nport+add, data, size);

Spaces around '+'.

> +}
> +
>  static uint64_t read_cont(void *opaque, hwaddr nport, unsigned size)
>  {
>      struct dma_cont *d = opaque;
>      int iport, val;
>
> +    nport += d->iobase + (8 << d->dshift);
> +
>      iport = (nport >> d->dshift) & 0x0f;
>      switch (iport) {
>      case 0x08:                  /* status */
> @@ -467,7 +480,7 @@ void DMA_schedule(int nchan)
>  static void dma_reset(void *opaque)
>  {
>      struct dma_cont *d = opaque;
> -    write_cont(d, (0x06 << d->dshift), 0, 1);
> +    __write_cont(d, (0xd << d->dshift), 0, 1);
>  }
>
>  static int dma_phony_handler (void *opaque, int nchan, int dma_pos, int 
> dma_len)
> @@ -523,7 +536,7 @@ static void dma_init2(struct dma_cont *d, int base, int 
> dshift,
>      d->cpu_request_exit = cpu_request_exit;
>
>      memory_region_init_io(&d->channel_io, &channel_io_ops, d,
> -                          "dma-chan", 8 << d->dshift);
> +                          "dma-chan", 8);
>      memory_region_add_subregion(isa_address_space_io(NULL),
>                                  base, &d->channel_io);
>
> @@ -535,12 +548,13 @@ static void dma_init2(struct dma_cont *d, int base, int 
> dshift,
>      }
>
>      memory_region_init_io(&d->cont_io, &cont_io_ops, d, "dma-cont",
> -                          8 << d->dshift);
> +                          8);
>      memory_region_add_subregion(isa_address_space_io(NULL),
>                                  base + (8 << d->dshift), &d->cont_io);
>
>      qemu_register_reset(dma_reset, d);
>      dma_reset(d);
> +    d->iobase = base;
>      for (i = 0; i < ARRAY_SIZE (d->regs); ++i) {
>          d->regs[i].transfer_handler = dma_phony_handler;
>      }
>
>
>
>