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-requ...@nongnu.org wrote:
>>>
>>>> Message: 3
>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>>> From: Herv? Poussineau <hpous...@reactos.org>
>>>> To: qemu-devel@nongnu.org
>>>> Cc: "open list:Old World" <qemu-...@nongnu.org>, Herv? Poussineau
>>>> <hpous...@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-hpous...@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 <hpous...@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
