From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47952) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8Xxb-0007tF-Ps for qemu-devel@nongnu.org; Mon, 14 Dec 2015 13:35:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8XxY-00043u-J2 for qemu-devel@nongnu.org; Mon, 14 Dec 2015 13:34:59 -0500 References: <11643EA3-BD07-4DD1-8599-1DD91D1CDE4D@gmail.com> <5645B132.2070404@reactos.org> <5645C840.9020705@ilande.co.uk> From: Laurent Vivier Message-ID: <566F0BCD.2070409@redhat.com> Date: Mon, 14 Dec 2015 19:34:53 +0100 MIME-Version: 1.0 In-Reply-To: <5645C840.9020705@ilande.co.uk> Content-Type: text/plain; charset=windows-1252 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: Mark Cave-Ayland , =?UTF-8?Q?Herv=c3=a9_Poussineau?= , Programmingkid , qemu-devel qemu-devel Cc: "qemu-ppc@nongnu.org list:PowerPC" On 13/11/2015 12:23, Mark Cave-Ayland wrote: > On 13/11/15 09:45, Herv=E9 Poussineau wrote: >=20 >> Le 13/11/2015 05:09, Programmingkid a =E9crit : >>> >>> 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 ID= E >> 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 channe= l) >> { >> 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 read >> the disk/cdrom, QEMU will crash. >=20 > 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