[Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Programmingkid]

On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:

> Message: 3
> Date: Thu, 12 Nov 2015 22:24:08 +0100
> From: Herv? Poussineau <hpoussin@reactos.org>
> To: qemu-devel@nongnu.org
> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
> 	<hpoussin@reactos.org>
> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
> 	channel	field in DBDMA_channel
> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
> Content-Type: text/plain; charset=UTF-8
> 
> dbdma_from_ch() uses channel field to return the right DBDMA object.
> Previous code was working if guest OS was only using registered DMA channels.
> However, it lead to QEMU crashes if guest OS was using unregistered DMA channels.
> 
> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
> ---
> hw/misc/macio/mac_dbdma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 779683c..5ee8f02 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
>     DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
> 
>     ch->irq = irq;
> -    ch->channel = nchan;
>     ch->rw = rw;
>     ch->flush = flush;
>     ch->io.opaque = opaque;
> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>     for (i = 0; i < DBDMA_CHANNELS; i++) {
>         DBDMA_io *io = &s->channels[i].io;
>         qemu_iovec_init(&io->iov, 1);
> +        s->channels[i].channel = i;
>     }
> 
>     memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma", 0x1000);
> -- 
> 2.1.4

What operating system(s) did you use to test this patch out?

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Hervé Poussineau]

Le 13/11/2015 05:09, Programmingkid a écrit :
>
> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>
>> Message: 3
>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>> From: Herv? Poussineau <hpoussin@reactos.org>
>> To: qemu-devel@nongnu.org
>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>> 	<hpoussin@reactos.org>
>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>> 	channel	field in DBDMA_channel
>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>> Content-Type: text/plain; charset=UTF-8
>>
>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>> Previous code was working if guest OS was only using registered DMA channels.
>> However, it lead to QEMU crashes if guest OS was using unregistered DMA channels.
>>
>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>> ---
>> hw/misc/macio/mac_dbdma.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>> index 779683c..5ee8f02 100644
>> --- a/hw/misc/macio/mac_dbdma.c
>> +++ b/hw/misc/macio/mac_dbdma.c
>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
>>      DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>
>>      ch->irq = irq;
>> -    ch->channel = nchan;
>>      ch->rw = rw;
>>      ch->flush = flush;
>>      ch->io.opaque = opaque;
>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>      for (i = 0; i < DBDMA_CHANNELS; i++) {
>>          DBDMA_io *io = &s->channels[i].io;
>>          qemu_iovec_init(&io->iov, 1);
>> +        s->channels[i].channel = i;
>>      }
>>
>>      memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma", 0x1000);
>> --
>> 2.1.4
>
> What operating system(s) did you use to test this patch out?
>

It was during some custom tests with OpenBIOS, where i miswrote the IDE DMA channel.

However, you can see the problem by using this "patch":
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 3ee962f..73dfec0 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
  void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
  {
      s->dbdma = dbdma;
-    DBDMA_register_channel(dbdma, channel, s->dma_irq,
+    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
                             pmac_ide_transfer, pmac_ide_flush, s);
  }

And starting whatever operating system. As soon as DMA is used to read the disk/cdrom, QEMU will crash.

Regards,

Hervé

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Thomas Huth]

On 13/11/15 10:45, Hervé Poussineau wrote:
> Le 13/11/2015 05:09, Programmingkid a écrit :
>>
>> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>>
>>> Message: 3
>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>> From: Herv? Poussineau <hpoussin@reactos.org>
>>> To: qemu-devel@nongnu.org
>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>>>     <hpoussin@reactos.org>
>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>     channel    field in DBDMA_channel
>>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>>> Content-Type: text/plain; charset=UTF-8
>>>
>>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>>> Previous code was working if guest OS was only using registered DMA
>>> channels.
>>> However, it lead to QEMU crashes if guest OS was using unregistered
>>> DMA channels.
>>>
>>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>>> ---
>>> hw/misc/macio/mac_dbdma.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>>> index 779683c..5ee8f02 100644
>>> --- a/hw/misc/macio/mac_dbdma.c
>>> +++ b/hw/misc/macio/mac_dbdma.c
>>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int
>>> nchan, qemu_irq irq,
>>>      DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>>
>>>      ch->irq = irq;
>>> -    ch->channel = nchan;
>>>      ch->rw = rw;
>>>      ch->flush = flush;
>>>      ch->io.opaque = opaque;
>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>>      for (i = 0; i < DBDMA_CHANNELS; i++) {
>>>          DBDMA_io *io = &s->channels[i].io;
>>>          qemu_iovec_init(&io->iov, 1);
>>> +        s->channels[i].channel = i;
>>>      }
>>>
>>>      memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma",
>>> 0x1000);
>>> -- 
>>> 2.1.4
>>
>> What operating system(s) did you use to test this patch out?
>>
> 
> It was during some custom tests with OpenBIOS, where i miswrote the IDE
> DMA channel.
> 
> However, you can see the problem by using this "patch":
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 3ee962f..73dfec0 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s,
> DriveInfo **hd_table)
>  void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
>  {
>      s->dbdma = dbdma;
> -    DBDMA_register_channel(dbdma, channel, s->dma_irq,
> +    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
>                             pmac_ide_transfer, pmac_ide_flush, s);
>  }
> 
> And starting whatever operating system. As soon as DMA is used to read
> the disk/cdrom, QEMU will crash.

Where does it crash? Could you provide a backtrace? ... sounds like the
function where this goes wrong should do some more checking for valid
channels?

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Mark Cave-Ayland]

On 13/11/15 09:45, Hervé Poussineau wrote:

> Le 13/11/2015 05:09, Programmingkid a écrit :
>>
>> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>>
>>> Message: 3
>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>> From: Herv? Poussineau <hpoussin@reactos.org>
>>> To: qemu-devel@nongnu.org
>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>>>     <hpoussin@reactos.org>
>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>     channel    field in DBDMA_channel
>>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>>> Content-Type: text/plain; charset=UTF-8
>>>
>>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>>> Previous code was working if guest OS was only using registered DMA
>>> channels.
>>> However, it lead to QEMU crashes if guest OS was using unregistered
>>> DMA channels.
>>>
>>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>>> ---
>>> hw/misc/macio/mac_dbdma.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>>> index 779683c..5ee8f02 100644
>>> --- a/hw/misc/macio/mac_dbdma.c
>>> +++ b/hw/misc/macio/mac_dbdma.c
>>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int
>>> nchan, qemu_irq irq,
>>>      DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>>
>>>      ch->irq = irq;
>>> -    ch->channel = nchan;
>>>      ch->rw = rw;
>>>      ch->flush = flush;
>>>      ch->io.opaque = opaque;
>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>>      for (i = 0; i < DBDMA_CHANNELS; i++) {
>>>          DBDMA_io *io = &s->channels[i].io;
>>>          qemu_iovec_init(&io->iov, 1);
>>> +        s->channels[i].channel = i;
>>>      }
>>>
>>>      memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma",
>>> 0x1000);
>>> -- 
>>> 2.1.4
>>
>> What operating system(s) did you use to test this patch out?
>>
> 
> It was during some custom tests with OpenBIOS, where i miswrote the IDE
> DMA channel.
> 
> However, you can see the problem by using this "patch":
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 3ee962f..73dfec0 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s,
> DriveInfo **hd_table)
>  void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
>  {
>      s->dbdma = dbdma;
> -    DBDMA_register_channel(dbdma, channel, s->dma_irq,
> +    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
>                             pmac_ide_transfer, pmac_ide_flush, s);
>  }
> 
> And starting whatever operating system. As soon as DMA is used to read
> the disk/cdrom, QEMU will crash.

Not sure if this is accidental fallout from the macio realize patch,
however the expectation is that DBDMA channels should be able to run
without crashing if not registered - they should instead NOOP because
ch->io isn't defined.

I'll have a look at this in a bit more detail later, but at first glance
I'd say this patch is correct and it should prevent a host crash.


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Hervé Poussineau]

Le 13/11/2015 11:40, Thomas Huth a écrit :
> On 13/11/15 10:45, Hervé Poussineau wrote:
>> Le 13/11/2015 05:09, Programmingkid a écrit :
>>>
>>> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>>>
>>>> Message: 3
>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>>> From: Herv? Poussineau <hpoussin@reactos.org>
>>>> To: qemu-devel@nongnu.org
>>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>>>>      <hpoussin@reactos.org>
>>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>>      channel    field in DBDMA_channel
>>>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>>>> Content-Type: text/plain; charset=UTF-8
>>>>
>>>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>>>> Previous code was working if guest OS was only using registered DMA
>>>> channels.
>>>> However, it lead to QEMU crashes if guest OS was using unregistered
>>>> DMA channels.
>>>>
>>>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>>>> ---
>>>> hw/misc/macio/mac_dbdma.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>>>> index 779683c..5ee8f02 100644
>>>> --- a/hw/misc/macio/mac_dbdma.c
>>>> +++ b/hw/misc/macio/mac_dbdma.c
>>>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int
>>>> nchan, qemu_irq irq,
>>>>       DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>>>
>>>>       ch->irq = irq;
>>>> -    ch->channel = nchan;
>>>>       ch->rw = rw;
>>>>       ch->flush = flush;
>>>>       ch->io.opaque = opaque;
>>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>>>       for (i = 0; i < DBDMA_CHANNELS; i++) {
>>>>           DBDMA_io *io = &s->channels[i].io;
>>>>           qemu_iovec_init(&io->iov, 1);
>>>> +        s->channels[i].channel = i;
>>>>       }
>>>>
>>>>       memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma",
>>>> 0x1000);
>>>> --
>>>> 2.1.4
>>>
>>> What operating system(s) did you use to test this patch out?
>>>
>>
>> It was during some custom tests with OpenBIOS, where i miswrote the IDE
>> DMA channel.
>>
>> However, you can see the problem by using this "patch":
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index 3ee962f..73dfec0 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s,
>> DriveInfo **hd_table)
>>   void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
>>   {
>>       s->dbdma = dbdma;
>> -    DBDMA_register_channel(dbdma, channel, s->dma_irq,
>> +    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
>>                              pmac_ide_transfer, pmac_ide_flush, s);
>>   }
>>
>> And starting whatever operating system. As soon as DMA is used to read
>> the disk/cdrom, QEMU will crash.
>
> Where does it crash? Could you provide a backtrace? ... sounds like the
> function where this goes wrong should do some more checking for valid
> channels?

The structure is:
typedef struct {
     MemoryRegion mem;
     DBDMA_channel channels[DBDMA_CHANNELS];
     QEMUBH *bh;
} DBDMAState;

static DBDMAState *dbdma_from_ch(DBDMA_channel *ch)
{
     return container_of(ch, DBDMAState, channels[ch->channel]);
}

Guest can deal with whatever DMA channel it wants (< DBDMA_CHANNELS). Some work will then be done with s->channels["guest_provided_channel"]. Note that DMA channel can be registered to some device, or 
not.

Later, dbdma_from_ch(DBDMA_channel) method is called to get back the DBDMAState structure from the channel. This method uses the ch->channel field. If this field is not initialized (ie is 0), a wrong 
DBDMAState pointer is returned, and memory corruption starts. QEMU crashes later, without any useful hint.

I just tried to register a wrong DMA channel for IDE and start MacOS 9. Without my patch, QEMU crashes. With my patch, MacOS 9 freezes and waits for the DMA transfer to complete, which never happens.

As you want a stack trace, here it is:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffdaf7a700 (LWP 32484)]
qemu_bh_schedule (bh=0x0) at async.c:130
130	    ctx = bh->ctx;
(gdb) bt
#0  qemu_bh_schedule (bh=0x0) at async.c:130
#1  0x00005555557211b5 in memory_region_write_accessor (mr=0x555556df3f10, addr=3328, value=<optimized out>, size=4, shift=<optimized out>, mask=<optimized out>, attrs=...) at memory.c:450
#2  0x0000555555720635 in access_with_adjusted_size (addr=0, addr@entry=3328, value=0x0, value@entry=0x7fffdaf79498, size=4, access_size_min=0, access_size_max=0, access=0x555555721100 
<memory_region_write_accessor>,
     mr=0x555556df3f10, attrs=...) at memory.c:506
#3  0x00005555557225eb in memory_region_dispatch_write (mr=0x555556df3f10, addr=3328, data=2415955968, size=4, attrs=...) at memory.c:1172
#4  0x00007fffde735ab9 in code_gen_buffer ()
#5  0x00005555556e8510 in cpu_tb_exec (tb_ptr=0x7fffde735a30 <code_gen_buffer+6617608> "A\213n\360\205\355\017\205\246", cpu=0x555556ae6bc0) at cpu-exec.c:157
#6  cpu_ppc_exec (cpu=cpu@entry=0x555556ae6bc0) at cpu-exec.c:520
#7  0x0000555555711d98 in tcg_cpu_exec (cpu=0x555556ae6bc0) at cpus.c:1470
#8  tcg_exec_all () at cpus.c:1503
#9  qemu_tcg_cpu_thread_fn (arg=<optimized out>) at cpus.c:1138

Hervé

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Thomas Huth]

On 13/11/15 13:10, Hervé Poussineau wrote:
> Le 13/11/2015 11:40, Thomas Huth a écrit :
>> On 13/11/15 10:45, Hervé Poussineau wrote:
>>> Le 13/11/2015 05:09, Programmingkid a écrit :
>>>>
>>>> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>>>>
>>>>> Message: 3
>>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>>>> From: Herv? Poussineau <hpoussin@reactos.org>
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>>>>>      <hpoussin@reactos.org>
>>>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>>>      channel    field in DBDMA_channel
>>>>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>>>>> Content-Type: text/plain; charset=UTF-8
>>>>>
>>>>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>>>>> Previous code was working if guest OS was only using registered DMA
>>>>> channels.
>>>>> However, it lead to QEMU crashes if guest OS was using unregistered
>>>>> DMA channels.
>>>>>
>>>>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>>>>> ---
>>>>> hw/misc/macio/mac_dbdma.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>>>>> index 779683c..5ee8f02 100644
>>>>> --- a/hw/misc/macio/mac_dbdma.c
>>>>> +++ b/hw/misc/macio/mac_dbdma.c
>>>>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int
>>>>> nchan, qemu_irq irq,
>>>>>       DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>>>>
>>>>>       ch->irq = irq;
>>>>> -    ch->channel = nchan;
>>>>>       ch->rw = rw;
>>>>>       ch->flush = flush;
>>>>>       ch->io.opaque = opaque;
>>>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>>>>       for (i = 0; i < DBDMA_CHANNELS; i++) {
>>>>>           DBDMA_io *io = &s->channels[i].io;
>>>>>           qemu_iovec_init(&io->iov, 1);
>>>>> +        s->channels[i].channel = i;
>>>>>       }
>>>>>
>>>>>       memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma",
>>>>> 0x1000);
>>>>> -- 
>>>>> 2.1.4
>>>>
>>>> What operating system(s) did you use to test this patch out?
>>>>
>>>
>>> It was during some custom tests with OpenBIOS, where i miswrote the IDE
>>> DMA channel.
>>>
>>> However, you can see the problem by using this "patch":
>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>> index 3ee962f..73dfec0 100644
>>> --- a/hw/ide/macio.c
>>> +++ b/hw/ide/macio.c
>>> @@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s,
>>> DriveInfo **hd_table)
>>>   void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int
>>> channel)
>>>   {
>>>       s->dbdma = dbdma;
>>> -    DBDMA_register_channel(dbdma, channel, s->dma_irq,
>>> +    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
>>>                              pmac_ide_transfer, pmac_ide_flush, s);
>>>   }
>>>
>>> And starting whatever operating system. As soon as DMA is used to read
>>> the disk/cdrom, QEMU will crash.
>>
>> Where does it crash? Could you provide a backtrace? ... sounds like the
>> function where this goes wrong should do some more checking for valid
>> channels?
> 
> The structure is:
> typedef struct {
>     MemoryRegion mem;
>     DBDMA_channel channels[DBDMA_CHANNELS];
>     QEMUBH *bh;
> } DBDMAState;
> 
> static DBDMAState *dbdma_from_ch(DBDMA_channel *ch)
> {
>     return container_of(ch, DBDMAState, channels[ch->channel]);
> }
> 
> Guest can deal with whatever DMA channel it wants (< DBDMA_CHANNELS).
> Some work will then be done with s->channels["guest_provided_channel"].
> Note that DMA channel can be registered to some device, or not.
> 
> Later, dbdma_from_ch(DBDMA_channel) method is called to get back the
> DBDMAState structure from the channel. This method uses the ch->channel
> field. If this field is not initialized (ie is 0), a wrong DBDMAState
> pointer is returned, and memory corruption starts. QEMU crashes later,
> without any useful hint.
> 
> I just tried to register a wrong DMA channel for IDE and start MacOS 9.
> Without my patch, QEMU crashes. With my patch, MacOS 9 freezes and waits
> for the DMA transfer to complete, which never happens.
> 
> As you want a stack trace, here it is:
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffdaf7a700 (LWP 32484)]
> qemu_bh_schedule (bh=0x0) at async.c:130
> 130        ctx = bh->ctx;
> (gdb) bt
> #0  qemu_bh_schedule (bh=0x0) at async.c:130
> #1  0x00005555557211b5 in memory_region_write_accessor
> (mr=0x555556df3f10, addr=3328, value=<optimized out>, size=4,
> shift=<optimized out>, mask=<optimized out>, attrs=...) at memory.c:450

Ok, thanks a lot for the backtrace - I'd hoped that there would be
something more obvious visible in it, e.g. a hint to a function that's
lacking some sanity checks for valid channel IDs or so ... but the
backtrace looks rather non-related to the Mac code, so this was a dead
end :-(

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Laurent Vivier]

On 13/11/2015 12:23, Mark Cave-Ayland wrote:
> On 13/11/15 09:45, Hervé Poussineau wrote:
> 
>> Le 13/11/2015 05:09, Programmingkid a écrit :
>>>
>>> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>>>
>>>> Message: 3
>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>>> From: Herv? Poussineau <hpoussin@reactos.org>
>>>> To: qemu-devel@nongnu.org
>>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>>>>     <hpoussin@reactos.org>
>>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>>     channel    field in DBDMA_channel
>>>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>>>> Content-Type: text/plain; charset=UTF-8
>>>>
>>>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>>>> Previous code was working if guest OS was only using registered DMA
>>>> channels.
>>>> However, it lead to QEMU crashes if guest OS was using unregistered
>>>> DMA channels.
>>>>
>>>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>>>> ---
>>>> hw/misc/macio/mac_dbdma.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>>>> index 779683c..5ee8f02 100644
>>>> --- a/hw/misc/macio/mac_dbdma.c
>>>> +++ b/hw/misc/macio/mac_dbdma.c
>>>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int
>>>> nchan, qemu_irq irq,
>>>>      DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>>>
>>>>      ch->irq = irq;
>>>> -    ch->channel = nchan;
>>>>      ch->rw = rw;
>>>>      ch->flush = flush;
>>>>      ch->io.opaque = opaque;
>>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>>>      for (i = 0; i < DBDMA_CHANNELS; i++) {
>>>>          DBDMA_io *io = &s->channels[i].io;
>>>>          qemu_iovec_init(&io->iov, 1);
>>>> +        s->channels[i].channel = i;
>>>>      }
>>>>
>>>>      memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma",
>>>> 0x1000);
>>>> -- 
>>>> 2.1.4
>>>
>>> What operating system(s) did you use to test this patch out?
>>>
>>
>> It was during some custom tests with OpenBIOS, where i miswrote the IDE
>> DMA channel.
>>
>> However, you can see the problem by using this "patch":
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index 3ee962f..73dfec0 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s,
>> DriveInfo **hd_table)
>>  void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
>>  {
>>      s->dbdma = dbdma;
>> -    DBDMA_register_channel(dbdma, channel, s->dma_irq,
>> +    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
>>                             pmac_ide_transfer, pmac_ide_flush, s);
>>  }
>>
>> And starting whatever operating system. As soon as DMA is used to read
>> the disk/cdrom, QEMU will crash.
> 
> Not sure if this is accidental fallout from the macio realize patch,
> however the expectation is that DBDMA channels should be able to run
> without crashing if not registered - they should instead NOOP because
> ch->io isn't defined.

In fact, the problem has been introduced by:

d2f0ce2 PPC: dbdma: Move static bh variable to device struct

That replaces the static variable to the QEMUBH for the DMA by a
container_of based() on the channel number.

Laurent