fix dma.c MemoryRegion convertion

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.

Fixes WinXP-32 installation.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

On Fri, Dec 14, 2012 at 11:39 PM, Marcelo Tosatti <mtosatti@redhat.com> 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 <mtosatti@redhat.com>
>
> 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;
>      }
>
>
>
>

On Fri, Dec 14, 2012 at 11:39 PM, Marcelo Tosatti <mtosatti@redhat.com> 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
>

I sent a patch on the mailing to resolve the problem yesterday
(http://www.mail-archive.com/qemu-devel@nongnu.org/msg145242.html).
Did it resolve your WinXp-32 installation? I just need to rework the comment.

>
> Also the size of the region is wrong.

I don't think the size of the region is wrong. For the second DMA
controller, the ioports are 0xD0 - 0xDE for the control registers.
So the size is 16 not 8. It's the same for the channel registers.

Thanks,

--
Grall Julien

Am 15.12.2012 14:45, schrieb Julien Grall:
> On Fri, Dec 14, 2012 at 11:39 PM, Marcelo Tosatti <mtosatti@redhat.com> 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
>>
> 
> I sent a patch on the mailing to resolve the problem yesterday
> (http://www.mail-archive.com/qemu-devel@nongnu.org/msg145242.html).
> Did it resolve your WinXp-32 installation? I just need to rework the comment.
> 
>>
>> Also the size of the region is wrong.
> 
> I don't think the size of the region is wrong. For the second DMA
> controller, the ioports are 0xD0 - 0xDE for the control registers.
> So the size is 16 not 8. It's the same for the channel registers.

You did seem to change it in size, if you compare my review questions,
without you mentioning why in the patch.

Andreas

On Sat, Dec 15, 2012 at 01:45:38PM +0000, Julien Grall wrote:
> On Fri, Dec 14, 2012 at 11:39 PM, Marcelo Tosatti <mtosatti@redhat.com> 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
> >
> 
> I sent a patch on the mailing to resolve the problem yesterday
> (http://www.mail-archive.com/qemu-devel@nongnu.org/msg145242.html).
> Did it resolve your WinXp-32 installation? I just need to rework the comment.

Yes it does.

> >
> > Also the size of the region is wrong.
> 
> I don't think the size of the region is wrong. For the second DMA
> controller, the ioports are 0xD0 - 0xDE for the control registers.
> So the size is 16 not 8. It's the same for the channel registers.
> 
> Thanks,

Correct, my patch is broken.

Thanks