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-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.
>>
>> 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
