* [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion
@ 2012-12-14 23:39 Marcelo Tosatti
2012-12-15 9:02 ` Blue Swirl
2012-12-15 13:45 ` Julien Grall
0 siblings, 2 replies; 5+ messages in thread
From: Marcelo Tosatti @ 2012-12-14 23:39 UTC (permalink / raw)
To: Julien Grall; +Cc: qemu-devel
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>
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;
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;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion
2012-12-14 23:39 [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion Marcelo Tosatti
@ 2012-12-15 9:02 ` Blue Swirl
2012-12-15 13:45 ` Julien Grall
1 sibling, 0 replies; 5+ messages in thread
From: Blue Swirl @ 2012-12-15 9:02 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Julien Grall, qemu-devel
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;
> }
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion
2012-12-14 23:39 [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion Marcelo Tosatti
2012-12-15 9:02 ` Blue Swirl
@ 2012-12-15 13:45 ` Julien Grall
2012-12-15 17:08 ` Andreas Färber
2012-12-17 0:01 ` Marcelo Tosatti
1 sibling, 2 replies; 5+ messages in thread
From: Julien Grall @ 2012-12-15 13:45 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Julien Grall, qemu-devel@nongnu.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion
2012-12-15 13:45 ` Julien Grall
@ 2012-12-15 17:08 ` Andreas Färber
2012-12-17 0:01 ` Marcelo Tosatti
1 sibling, 0 replies; 5+ messages in thread
From: Andreas Färber @ 2012-12-15 17:08 UTC (permalink / raw)
To: Julien Grall; +Cc: Julien Grall, Marcelo Tosatti, qemu-devel@nongnu.org
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion
2012-12-15 13:45 ` Julien Grall
2012-12-15 17:08 ` Andreas Färber
@ 2012-12-17 0:01 ` Marcelo Tosatti
1 sibling, 0 replies; 5+ messages in thread
From: Marcelo Tosatti @ 2012-12-17 0:01 UTC (permalink / raw)
To: Julien Grall; +Cc: Julien Grall, qemu-devel@nongnu.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-12-17 0:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-14 23:39 [Qemu-devel] [PATCH] fix dma.c MemoryRegion convertion Marcelo Tosatti
2012-12-15 9:02 ` Blue Swirl
2012-12-15 13:45 ` Julien Grall
2012-12-15 17:08 ` Andreas Färber
2012-12-17 0:01 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).