Skip to contentSkip to conversation

fix dma.c MemoryRegion convertion

PATCH In review Marcelo Tosatti 4 participants 5 messages Dec 14, 2012 – Dec 17, 2012

lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
hw/dma.c +26 -12
index c2d7b21..e7de522 100644
@@ -56,6 +56,7 @@ static struct dma_cont {
     uint8_t mask;
     uint8_t flip_flop;
     int dshift;
+    int iobase;
     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)
 {
     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);
+}
+
 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;
     }
#1 lore raw
Quoted 9 lines · Marcelo Tosatti

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?

Quoted 10 lines

Fixes WinXP-32 installation.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/dma.c +0 -0
index c2d7b21..e7de522 100644

hw/dma.c static struct dma_cont { · line 59

Quoted 4 lines
hw/dma.c +1 -0
@@ -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.

hw/dma.c static void write_chan(void *opaque, hwaddr nport, uint64_t data, · line 198

Quoted 11 lines
hw/dma.c +2 -1
     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.

Quoted 67 lines
hw/dma.c +8 -8
 {
     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;

hw/dma.c static void write_cont(void *opaque, hwaddr nport, uint64_t data, · line 292

Quoted 10 lines
hw/dma.c +7 -0
@@ -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 '+'.

Quoted 49 lines
hw/dma.c +8 -3
+}
+
 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;
     }
#2 lore raw
In reply to Marcelo Tosatti
Quoted 9 lines · Marcelo Tosatti

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/[email protected]/msg145242.html).
Did it resolve your WinXp-32 installation? I just need to rework the comment.

Quoted 2 lines

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

#3 lore raw

Am 15.12.2012 14:45, schrieb Julien Grall:

Quoted 16 lines · Marcelo Tosatti
Quoted 9 lines

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/[email protected]/msg145242.html).
Did it resolve your WinXp-32 installation? I just need to rework the comment.

Quoted 1 line
Quoted 5 lines
Quoted 1 line

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

Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
author
#4 lore raw
In reply to Julien Grall
Quoted 7 lines · Julien Grall
Quoted 6 lines · Marcelo Tosatti

The high byte of the ioport address is necessary to compute the register address, see "82371AB PCI ISA IDE Xcelerator (PIIX4)" document, eg:

Quoted 7 lines
Quoted 3 lines

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/[email protected]/msg145242.html).
Did it resolve your WinXp-32 installation? I just need to rework the comment.

Yes it does.

Quoted 8 lines
Quoted 2 lines

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