| Message ID | 1443530263-32340-3-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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>
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
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
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.
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).
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
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.
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());
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(-)