Re: [Qemu-devel] [PATCH 2/3] hw: do not pass NULL to memory_region_init from instance_init

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 29/09/15 13:37, Paolo Bonzini wrote:

> This causes the region to outlive the object, because it attaches the
> region to /machine.  This is not nice for the "realize" method, but
> much worse for "instance_init" because it can cause dangling pointers
> after a simple object_new/object_unref pair.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/arm/pxa2xx.c                | 2 +-
>  hw/display/cg3.c               | 4 ++--
>  hw/display/tcx.c               | 2 +-
>  hw/misc/arm_integrator_debug.c | 2 +-
>  hw/misc/macio/cuda.c           | 2 +-
>  hw/misc/macio/macio.c          | 6 +++---
>  6 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 164260a..79d22d9 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1958,7 +1958,7 @@ static void pxa2xx_fir_instance_init(Object *obj)
>      PXA2xxFIrState *s = PXA2XX_FIR(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    memory_region_init_io(&s->iomem, NULL, &pxa2xx_fir_ops, s,
> +    memory_region_init_io(&s->iomem, obj, &pxa2xx_fir_ops, s,
>                            "pxa2xx-fir", 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
>      sysbus_init_irq(sbd, &s->irq);
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index d2a0d97..e309fbe 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -280,12 +280,12 @@ static void cg3_initfn(Object *obj)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      CG3State *s = CG3(obj);
>  
> -    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE,
> +    memory_region_init_ram(&s->rom, obj, "cg3.prom", FCODE_MAX_ROM_SIZE,
>                             &error_fatal);
>      memory_region_set_readonly(&s->rom, true);
>      sysbus_init_mmio(sbd, &s->rom);
>  
> -    memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg",
> +    memory_region_init_io(&s->reg, obj, &cg3_reg_ops, s, "cg3.reg",
>                            CG3_REG_SIZE);
>      sysbus_init_mmio(sbd, &s->reg);
>  }
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 4635800..bf119bc 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -944,7 +944,7 @@ static void tcx_initfn(Object *obj)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      TCXState *s = TCX(obj);
>  
> -    memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE,
> +    memory_region_init_ram(&s->rom, OBJECT(s), "tcx.prom", FCODE_MAX_ROM_SIZE,
>                             &error_fatal);
>      memory_region_set_readonly(&s->rom, true);
>      sysbus_init_mmio(sbd, &s->rom);
> diff --git a/hw/misc/arm_integrator_debug.c b/hw/misc/arm_integrator_debug.c
> index 99b720f..6d9dd74 100644
> --- a/hw/misc/arm_integrator_debug.c
> +++ b/hw/misc/arm_integrator_debug.c
> @@ -79,7 +79,7 @@ static void intdbg_control_init(Object *obj)
>      SysBusDevice *sd = SYS_BUS_DEVICE(obj);
>      IntegratorDebugState *s = INTEGRATOR_DEBUG(obj);
>  
> -    memory_region_init_io(&s->iomem, NULL, &intdbg_control_ops,
> +    memory_region_init_io(&s->iomem, obj, &intdbg_control_ops,
>                            NULL, "dbg-leds", 0x1000000);
>      sysbus_init_mmio(sd, &s->iomem);
>  }
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index f3984e3..5d7043e 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -713,7 +713,7 @@ static void cuda_initfn(Object *obj)
>      CUDAState *s = CUDA(obj);
>      int i;
>  
> -    memory_region_init_io(&s->mem, NULL, &cuda_ops, s, "cuda", 0x2000);
> +    memory_region_init_io(&s->mem, obj, &cuda_ops, s, "cuda", 0x2000);
>      sysbus_init_mmio(d, &s->mem);
>      sysbus_init_irq(d, &s->irq);
>  
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index e3c0242..2548d96 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -105,10 +105,10 @@ static void macio_escc_legacy_setup(MacIOState *macio_state)
>          0xF0, 0xE0,
>      };
>  
> -    memory_region_init(escc_legacy, NULL, "escc-legacy", 256);
> +    memory_region_init(escc_legacy, OBJECT(macio_state), "escc-legacy", 256);
>      for (i = 0; i < ARRAY_SIZE(maps); i += 2) {
>          MemoryRegion *port = g_new(MemoryRegion, 1);
> -        memory_region_init_alias(port, NULL, "escc-legacy-port",
> +        memory_region_init_alias(port, OBJECT(macio_state), "escc-legacy-port",
>                                   macio_state->escc_mem, maps[i+1], 0x2);
>          memory_region_add_subregion(escc_legacy, maps[i], port);
>      }
> @@ -330,7 +330,7 @@ static void macio_instance_init(Object *obj)
>      MacIOState *s = MACIO(obj);
>      MemoryRegion *dbdma_mem;
>  
> -    memory_region_init(&s->bar, NULL, "macio", 0x80000);
> +    memory_region_init(&s->bar, obj, "macio", 0x80000);
>  
>      object_initialize(&s->cuda, sizeof(s->cuda), TYPE_CUDA);
>      qdev_set_parent_bus(DEVICE(&s->cuda), sysbus_get_default());

I'm not able to test this right now, but for the TCX/CG3 changes:

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


ATB,

Mark.