Re: [Qemu-ppc] [Qemu-devel] [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, 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.

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.