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, address@hidden wrote:
>>>
>>>> Message: 3
>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>>> From: Herv? Poussineau <address@hidden>
>>>> To: address@hidden
>>>> Cc: "open list:Old World" <address@hidden>, Herv? Poussineau
>>>>     <address@hidden>
>>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>>     channel    field in DBDMA_channel
>>>> Message-ID: <address@hidden>
>>>> 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 <address@hidden>
>>>> ---
>>>> 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