[Qemu-devel] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Qemu-devel] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Hervé Poussineau]

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

Re: [Qemu-devel] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Mark Cave-Ayland]

On 12/11/15 21:24, Hervé Poussineau wrote:

> 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);
> 

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

This definitely prevents a host crash here, although it is possible to
argue whether or not an assert is more useful than a hang for developers.

David, can you queue this for ppc-next? It's not urgent but it would be
good not to have a potential host crash in 2.5.

Incidentally the QEMU iov can disappear from the io struct now since it
was only used for Alex's original alignment fixes for the macio code,
and it is probably more correct to move head_remainder and
tail_remainder into MACIOIDEState since DBDMA doesn't always involve
disk devices - but that can wait for another day...


ATB,

Mark.