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

[Thomas Huth <thuth@redhat.com>]

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