| Message ID | 5645B132.2070404@reactos.org |
|---|---|
| State | New |
| Headers | show |
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-request@nongnu.org wrote: >> >>> Message: 3 >>> Date: Thu, 12 Nov 2015 22:24:08 +0100 >>> From: Herv? Poussineau <hpoussin@reactos.org> >>> To: qemu-devel@nongnu.org >>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau >>> <hpoussin@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-hpoussin@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 <hpoussin@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? Thomas
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-request@nongnu.org wrote: >> >>> Message: 3 >>> Date: Thu, 12 Nov 2015 22:24:08 +0100 >>> From: Herv? Poussineau <hpoussin@reactos.org> >>> To: qemu-devel@nongnu.org >>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau >>> <hpoussin@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-hpoussin@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 <hpoussin@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. 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.
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-request@nongnu.org wrote: >>> >>>> Message: 3 >>>> Date: Thu, 12 Nov 2015 22:24:08 +0100 >>>> From: Herv? Poussineau <hpoussin@reactos.org> >>>> To: qemu-devel@nongnu.org >>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau >>>> <hpoussin@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-hpoussin@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 <hpoussin@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 #2 0x0000555555720635 in access_with_adjusted_size (addr=0, addr@entry=3328, value=0x0, value@entry=0x7fffdaf79498, size=4, access_size_min=0, access_size_max=0, access=0x555555721100 <memory_region_write_accessor>, mr=0x555556df3f10, attrs=...) at memory.c:506 #3 0x00005555557225eb in memory_region_dispatch_write (mr=0x555556df3f10, addr=3328, data=2415955968, size=4, attrs=...) at memory.c:1172 #4 0x00007fffde735ab9 in code_gen_buffer () #5 0x00005555556e8510 in cpu_tb_exec (tb_ptr=0x7fffde735a30 <code_gen_buffer+6617608> "A\213n\360\205\355\017\205\246", cpu=0x555556ae6bc0) at cpu-exec.c:157 #6 cpu_ppc_exec (cpu=cpu@entry=0x555556ae6bc0) at cpu-exec.c:520 #7 0x0000555555711d98 in tcg_cpu_exec (cpu=0x555556ae6bc0) at cpus.c:1470 #8 tcg_exec_all () at cpus.c:1503 #9 qemu_tcg_cpu_thread_fn (arg=<optimized out>) at cpus.c:1138 Hervé
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-request@nongnu.org wrote: >>>> >>>>> Message: 3 >>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100 >>>>> From: Herv? Poussineau <hpoussin@reactos.org> >>>>> To: qemu-devel@nongnu.org >>>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau >>>>> <hpoussin@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-hpoussin@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 <hpoussin@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
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-request@nongnu.org wrote: >>> >>>> Message: 3 >>>> Date: Thu, 12 Nov 2015 22:24:08 +0100 >>>> From: Herv? Poussineau <hpoussin@reactos.org> >>>> To: qemu-devel@nongnu.org >>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau >>>> <hpoussin@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-hpoussin@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 <hpoussin@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
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); }