From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzUxl-0002rQ-CN for qemu-devel@nongnu.org; Thu, 19 Nov 2015 14:33:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzUxh-0007vZ-CL for qemu-devel@nongnu.org; Thu, 19 Nov 2015 14:33:45 -0500 References: <11643EA3-BD07-4DD1-8599-1DD91D1CDE4D@gmail.com> <5645B132.2070404@reactos.org> <5645BE00.9070101@redhat.com> <5645D31E.6050205@reactos.org> From: Thomas Huth Message-ID: <564E240E.60508@redhat.com> Date: Thu, 19 Nov 2015 20:33:34 +0100 MIME-Version: 1.0 In-Reply-To: <5645D31E.6050205@reactos.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Herv=c3=a9_Poussineau?= , Programmingkid , qemu-devel qemu-devel Cc: qemu-ppc@nongnu.org, David Gibson On 13/11/15 13:10, Herv=C3=A9 Poussineau wrote: > Le 13/11/2015 11:40, Thomas Huth a =C3=A9crit : >> On 13/11/15 10:45, Herv=C3=A9 Poussineau wrote: >>> Le 13/11/2015 05:09, Programmingkid a =C3=A9crit : >>>> >>>> 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 >>>>> To: qemu-devel@nongnu.org >>>>> Cc: "open list:Old World" , Herv? Poussineau >>>>> >>>>> 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=3DUTF-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 >>>>> --- >>>>> 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 =3D irq; >>>>> - ch->channel =3D nchan; >>>>> ch->rw =3D rw; >>>>> ch->flush =3D flush; >>>>> ch->io.opaque =3D opaque; >>>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem) >>>>> for (i =3D 0; i < DBDMA_CHANNELS; i++) { >>>>> DBDMA_io *io =3D &s->channels[i].io; >>>>> qemu_iovec_init(&io->iov, 1); >>>>> + s->channels[i].channel =3D i; >>>>> } >>>>> >>>>> memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma", >>>>> 0x1000); >>>>> --=20 >>>>> 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 I= DE >>> 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 =3D 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 rea= d >>> the disk/cdrom, QEMU will crash. >> >> Where does it crash? Could you provide a backtrace? ... sounds like th= e >> function where this goes wrong should do some more checking for valid >> channels? >=20 > The structure is: > typedef struct { > MemoryRegion mem; > DBDMA_channel channels[DBDMA_CHANNELS]; > QEMUBH *bh; > } DBDMAState; >=20 > static DBDMAState *dbdma_from_ch(DBDMA_channel *ch) > { > return container_of(ch, DBDMAState, channels[ch->channel]); > } >=20 > 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. >=20 > 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. >=20 > 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 wait= s > for the DMA transfer to complete, which never happens. >=20 > 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=3D0x0) at async.c:130 > 130 ctx =3D bh->ctx; > (gdb) bt > #0 qemu_bh_schedule (bh=3D0x0) at async.c:130 > #1 0x00005555557211b5 in memory_region_write_accessor > (mr=3D0x555556df3f10, addr=3D3328, value=3D, size=3D4, > shift=3D, mask=3D, attrs=3D...) 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