[Qemu-devel] [PATCH 0/3] Fix dangling pointers from memory_region_init_*

[Qemu-devel] [PATCH 0/3] Fix dangling pointers from memory_region_init_*

[Paolo Bonzini]

Some devices do not support a simple object_new/object_unref sequence
because they leave dangling pointers under /machine.  This series
fixes this, for the case where the dangling pointers come from the
memory API.

Patch 1 avoids an assertion failure.  Patches 2 and 3 fix the devices
that Markus reported.

Paolo

Paolo Bonzini (3):
  memory: allow destroying a non-empty MemoryRegion
  hw: do not pass NULL to memory_region_init from instance_init
  macio: move DBDMA_init from instance_init to realize

 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          | 14 +++++++-------
 memory.c                       | 17 ++++++++++++++++-
 7 files changed, 29 insertions(+), 14 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/3] memory: allow destroying a non-empty MemoryRegion

[Paolo Bonzini]

This is legal; the MemoryRegion will simply unreference all the
existing subregions and possibly bring them down with it as well.
However, it requires a bit of care to avoid an infinite loop.
Finalizing a memory region cannot trigger an address space update,
but memory_region_del_subregion errs on the side of caution and
might trigger a spurious update: avoid that by resetting mr->enabled
first.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index ef87363..73d28ba 100644
--- a/memory.c
+++ b/memory.c
@@ -1304,7 +1304,22 @@ static void memory_region_finalize(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
 
-    assert(QTAILQ_EMPTY(&mr->subregions));
+    assert(!mr->container);
+
+    /* We know the region is not visible in any address space (it
+     * does not have a container and cannot be a root either because
+     * it has no references, so we can blindly clear mr->enabled.
+     * memory_region_set_enabled instead could trigger a transaction
+     * and cause an infinite loop.
+     */
+    mr->enabled = false;
+    memory_region_transaction_begin();
+    while (!QTAILQ_EMPTY(&mr->subregions)) {
+        MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
+        memory_region_del_subregion(mr, subregion);
+    }
+    memory_region_transaction_commit();
+
     mr->destructor(mr);
     memory_region_clear_coalescing(mr);
     g_free((char *)mr->name);
-- 
2.5.0

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

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 3/3] macio: move DBDMA_init from instance_init to realize

[Paolo Bonzini]

DBDMA_init is not idempotent, and calling it from instance_init
breaks a simple object_new/object_unref pair.  Work around this,
pending qdev-ification of DBDMA, by moving the call to realize.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/macio/macio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 2548d96..c661f86 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -131,6 +131,10 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
     MacIOState *s = MACIO(d);
     SysBusDevice *sysbus_dev;
     Error *err = NULL;
+    MemoryRegion *dbdma_mem;
+
+    s->dbdma = DBDMA_init(&dbdma_mem);
+    memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
 
     object_property_set_bool(OBJECT(&s->cuda), true, "realized", &err);
     if (err) {
@@ -328,16 +332,12 @@ static void macio_newworld_init(Object *obj)
 static void macio_instance_init(Object *obj)
 {
     MacIOState *s = MACIO(obj);
-    MemoryRegion *dbdma_mem;
 
     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());
     object_property_add_child(obj, "cuda", OBJECT(&s->cuda), NULL);
-
-    s->dbdma = DBDMA_init(&dbdma_mem);
-    memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
 }
 
 static const VMStateDescription vmstate_macio_oldworld = {
-- 
2.5.0

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

[Peter Maydell]

On 29 September 2015 at 13:37, Paolo Bonzini <pbonzini@redhat.com> 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(-)

Some of the macio changes are realize function code rather than
instance_init, but they're all bugs either way.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Thomas Huth]

On 29/09/15 14: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>
...
> 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);

Why "OBJECT(s)" and not simply "obj" ?

 Thomas

Re: [Qemu-devel] [PATCH 3/3] macio: move DBDMA_init from instance_init to realize

[Thomas Huth]

On 29/09/15 14:37, Paolo Bonzini wrote:
> DBDMA_init is not idempotent, and calling it from instance_init
> breaks a simple object_new/object_unref pair.  Work around this,
> pending qdev-ification of DBDMA, by moving the call to realize.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/misc/macio/macio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 2548d96..c661f86 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -131,6 +131,10 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>      MacIOState *s = MACIO(d);
>      SysBusDevice *sysbus_dev;
>      Error *err = NULL;
> +    MemoryRegion *dbdma_mem;
> +
> +    s->dbdma = DBDMA_init(&dbdma_mem);
> +    memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
>  
>      object_property_set_bool(OBJECT(&s->cuda), true, "realized", &err);
>      if (err) {
> @@ -328,16 +332,12 @@ static void macio_newworld_init(Object *obj)
>  static void macio_instance_init(Object *obj)
>  {
>      MacIOState *s = MACIO(obj);
> -    MemoryRegion *dbdma_mem;
>  
>      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());
>      object_property_add_child(obj, "cuda", OBJECT(&s->cuda), NULL);
> -
> -    s->dbdma = DBDMA_init(&dbdma_mem);
> -    memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
>  }
>  
>  static const VMStateDescription vmstate_macio_oldworld = {

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

One more: pxa2xx_pcmcia_initfn().

The ones you fix are
Tested-by: Markus Armbruster <armbru@redhat.com>

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

[Paolo Bonzini]

On 30/09/2015 10:57, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> > 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>
> One more: pxa2xx_pcmcia_initfn().
> 
> The ones you fix are
> Tested-by: Markus Armbruster <armbru@redhat.com>

Can you fix it up and take it through your series?

Paolo

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

[Paolo Bonzini]

On 30/09/2015 10:30, Thomas Huth wrote:
>> > @@ -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);
> Why "OBJECT(s)" and not simply "obj" ?

No particular reason, just the way my brain worked. :)

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/09/2015 10:57, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> > 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>
>> One more: pxa2xx_pcmcia_initfn().
>> 
>> The ones you fix are
>> Tested-by: Markus Armbruster <armbru@redhat.com>
>
> Can you fix it up and take it through your series?

Like this?

>From 14ce586f3e8a7ced07ec37ed60ad71ca55f41a08 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Thu, 1 Oct 2015 09:36:39 +0200
Subject: [PATCH] fixup! hw: do not pass NULL to memory_region_init from
 instance_init

---
 hw/pcmcia/pxa2xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index e0de8a6..23649bc 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -163,7 +163,7 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
     sysbus_init_mmio(sbd, &s->container_mem);
 
     /* Socket I/O Memory Space */
-    memory_region_init_io(&s->iomem, NULL, &pxa2xx_pcmcia_io_ops, s,
+    memory_region_init_io(&s->iomem, obj, &pxa2xx_pcmcia_io_ops, s,
                           "pxa2xx-pcmcia-io", 0x04000000);
     memory_region_add_subregion(&s->container_mem, 0x00000000,
                                 &s->iomem);
@@ -171,13 +171,13 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
     /* Then next 64 MB is reserved */
 
     /* Socket Attribute Memory Space */
-    memory_region_init_io(&s->attr_iomem, NULL, &pxa2xx_pcmcia_attr_ops, s,
+    memory_region_init_io(&s->attr_iomem, obj, &pxa2xx_pcmcia_attr_ops, s,
                           "pxa2xx-pcmcia-attribute", 0x04000000);
     memory_region_add_subregion(&s->container_mem, 0x08000000,
                                 &s->attr_iomem);
 
     /* Socket Common Memory Space */
-    memory_region_init_io(&s->common_iomem, NULL, &pxa2xx_pcmcia_common_ops, s,
+    memory_region_init_io(&s->common_iomem, obj, &pxa2xx_pcmcia_common_ops, s,
                           "pxa2xx-pcmcia-common", 0x04000000);
     memory_region_add_subregion(&s->container_mem, 0x0c000000,
                                 &s->common_iomem);
-- 
2.4.3

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/09/2015 10:30, Thomas Huth wrote:
>>> > @@ -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);
>> Why "OBJECT(s)" and not simply "obj" ?
>
> No particular reason, just the way my brain worked. :)

I can touch it up on commit.

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 30/09/2015 10:30, Thomas Huth wrote:
>>>> > @@ -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);
>>> Why "OBJECT(s)" and not simply "obj" ?
>>
>> No particular reason, just the way my brain worked. :)
>
> I can touch it up on commit.

I won't, because the rest of the function uses OBJECT(s).

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

[Peter Maydell]

On 1 October 2015 at 09:26, Markus Armbruster <armbru@redhat.com> wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 30/09/2015 10:30, Thomas Huth wrote:
>>>>> > @@ -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);
>>>> Why "OBJECT(s)" and not simply "obj" ?
>>>
>>> No particular reason, just the way my brain worked. :)
>>
>> I can touch it up on commit.
>
> I won't, because the rest of the function uses OBJECT(s).

That's the result of the code motion in commit 01b91ac2be83
which moved these lines from realize to initfn without noticing
that the OBJECT(s) could be simplified to obj in the process.

thanks
-- PMM

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

[Mark Cave-Ayland]

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.

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

[Paolo Bonzini]

On 01/10/2015 09:39, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 30/09/2015 10:57, Markus Armbruster wrote:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>>> 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>
>>> One more: pxa2xx_pcmcia_initfn().
>>>
>>> The ones you fix are
>>> Tested-by: Markus Armbruster <armbru@redhat.com>
>>
>> Can you fix it up and take it through your series?
> 
> Like this?
> 
> From 14ce586f3e8a7ced07ec37ed60ad71ca55f41a08 Mon Sep 17 00:00:00 2001
> From: Markus Armbruster <armbru@redhat.com>
> Date: Thu, 1 Oct 2015 09:36:39 +0200
> Subject: [PATCH] fixup! hw: do not pass NULL to memory_region_init from
>  instance_init
> 
> ---
>  hw/pcmcia/pxa2xx.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
> index e0de8a6..23649bc 100644
> --- a/hw/pcmcia/pxa2xx.c
> +++ b/hw/pcmcia/pxa2xx.c
> @@ -163,7 +163,7 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
>      sysbus_init_mmio(sbd, &s->container_mem);
>  
>      /* Socket I/O Memory Space */
> -    memory_region_init_io(&s->iomem, NULL, &pxa2xx_pcmcia_io_ops, s,
> +    memory_region_init_io(&s->iomem, obj, &pxa2xx_pcmcia_io_ops, s,
>                            "pxa2xx-pcmcia-io", 0x04000000);
>      memory_region_add_subregion(&s->container_mem, 0x00000000,
>                                  &s->iomem);
> @@ -171,13 +171,13 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
>      /* Then next 64 MB is reserved */
>  
>      /* Socket Attribute Memory Space */
> -    memory_region_init_io(&s->attr_iomem, NULL, &pxa2xx_pcmcia_attr_ops, s,
> +    memory_region_init_io(&s->attr_iomem, obj, &pxa2xx_pcmcia_attr_ops, s,
>                            "pxa2xx-pcmcia-attribute", 0x04000000);
>      memory_region_add_subregion(&s->container_mem, 0x08000000,
>                                  &s->attr_iomem);
>  
>      /* Socket Common Memory Space */
> -    memory_region_init_io(&s->common_iomem, NULL, &pxa2xx_pcmcia_common_ops, s,
> +    memory_region_init_io(&s->common_iomem, obj, &pxa2xx_pcmcia_common_ops, s,
>                            "pxa2xx-pcmcia-common", 0x04000000);
>      memory_region_add_subregion(&s->container_mem, 0x0c000000,
>                                  &s->common_iomem);
> 

Yes, thanks!

Paolo