Message ID | 20200215154706.19837-2-philmd@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw: Delay timer_new() from init to realize to avoid memleaks | expand |
On Sat, Feb 15, 2020 at 04:47:05PM +0100, Philippe Mathieu-Daudé wrote: > In commit f3a508eb4e the Euler Robot reported calling timer_new() > in instance_init() can leak heap memory. The easier fix is to > delay the timer creation at instance realize(). Similarly move > timer_del() into a new instance unrealize() method. > > This case was found with the following coccinelle script: > > @ match @ > identifier instance_init; > typedef Object; > identifier obj; > expression val, scale; > identifier clock_type, callback, opaque; > position pos; > @@ > static void instance_init(Object *obj) > { > <... > ( > val = timer_new@pos(clock_type, scale, callback, opaque); > | > val = timer_new_ns@pos(clock_type, callback, opaque); > | > val = timer_new_us@pos(clock_type, callback, opaque); > | > val = timer_new_ms@pos(clock_type, callback, opaque); > ) > ...> > } > > @ script:python @ > f << match.instance_init; > p << match.pos; > @@ > print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f) > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> This looks ok to me: Acked-by: Corey Minyard <cminyard@mvista.com> I can take it into my tree, if you like. -corey > --- > Cc: Pan Nengyuan <pannengyuan@huawei.com> > --- > hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c > index f9a13e0a44..9144ac6c38 100644 > --- a/hw/ipmi/ipmi_bmc_extern.c > +++ b/hw/ipmi/ipmi_bmc_extern.c > @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp) > > qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive, > chr_event, NULL, ibe, NULL, true); > + > + ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); > +} > + > +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp) > +{ > + IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev); > + > + timer_del(ibe->extern_timer); > } > > static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id) > @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj) > { > IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); > > - ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); > vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe); > } > > @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj) > { > IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); > > - timer_del(ibe->extern_timer); > timer_free(ibe->extern_timer); > } > > @@ -528,6 +535,7 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, void *data) > bk->handle_reset = ipmi_bmc_extern_handle_reset; > dc->hotpluggable = false; > dc->realize = ipmi_bmc_extern_realize; > + dc->unrealize = ipmi_bmc_extern_unrealize; > device_class_set_props(dc, ipmi_bmc_extern_properties); > } > > -- > 2.21.1 >
On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > In commit f3a508eb4e the Euler Robot reported calling timer_new() > in instance_init() can leak heap memory. The easier fix is to > delay the timer creation at instance realize(). Similarly move > timer_del() into a new instance unrealize() method. > > This case was found with the following coccinelle script: > > @ match @ > identifier instance_init; > typedef Object; > identifier obj; > expression val, scale; > identifier clock_type, callback, opaque; > position pos; > @@ > static void instance_init(Object *obj) > { > <... > ( > val = timer_new@pos(clock_type, scale, callback, opaque); > | > val = timer_new_ns@pos(clock_type, callback, opaque); > | > val = timer_new_us@pos(clock_type, callback, opaque); > | > val = timer_new_ms@pos(clock_type, callback, opaque); > ) > ...> > } > > @ script:python @ > f << match.instance_init; > p << match.pos; > @@ > print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f) > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Cc: Pan Nengyuan <pannengyuan@huawei.com> > --- > hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c > index f9a13e0a44..9144ac6c38 100644 > --- a/hw/ipmi/ipmi_bmc_extern.c > +++ b/hw/ipmi/ipmi_bmc_extern.c > @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp) > > qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive, > chr_event, NULL, ibe, NULL, true); > + > + ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); > +} > + > +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp) > +{ > + IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev); > + > + timer_del(ibe->extern_timer); > } > > static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id) > @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj) > { > IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); > > - ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); > vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe); > } > > @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj) > { > IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); > > - timer_del(ibe->extern_timer); > timer_free(ibe->extern_timer); > } So we now call timer_new in realize, and timer_del in unrealize, but timer_free in finalize. This seems unbalanced -- why not call timer_free in unrealize too, if we're moving things? Also, this is a case of code that's actually doing things right: we free the memory that we allocated in init in finalize. So we're not fixing any leak here, we're just moving code around (which is reasonable if we're trying to standardize on "call timer_new in realize, not init", but should be noted in the commit message). thanks -- PMM
On 2/17/20 2:25 PM, Peter Maydell wrote: > On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> In commit f3a508eb4e the Euler Robot reported calling timer_new() >> in instance_init() can leak heap memory. The easier fix is to >> delay the timer creation at instance realize(). Similarly move >> timer_del() into a new instance unrealize() method. >> >> This case was found with the following coccinelle script: >> >> @ match @ >> identifier instance_init; >> typedef Object; >> identifier obj; >> expression val, scale; >> identifier clock_type, callback, opaque; >> position pos; >> @@ >> static void instance_init(Object *obj) >> { >> <... >> ( >> val = timer_new@pos(clock_type, scale, callback, opaque); >> | >> val = timer_new_ns@pos(clock_type, callback, opaque); >> | >> val = timer_new_us@pos(clock_type, callback, opaque); >> | >> val = timer_new_ms@pos(clock_type, callback, opaque); >> ) >> ...> >> } >> >> @ script:python @ >> f << match.instance_init; >> p << match.pos; >> @@ >> print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f) >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> Cc: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c >> index f9a13e0a44..9144ac6c38 100644 >> --- a/hw/ipmi/ipmi_bmc_extern.c >> +++ b/hw/ipmi/ipmi_bmc_extern.c >> @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp) >> >> qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive, >> chr_event, NULL, ibe, NULL, true); >> + >> + ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); >> +} >> + >> +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp) >> +{ >> + IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev); >> + >> + timer_del(ibe->extern_timer); >> } >> >> static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id) >> @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj) >> { >> IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); >> >> - ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); >> vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe); >> } >> >> @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj) >> { >> IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); >> >> - timer_del(ibe->extern_timer); >> timer_free(ibe->extern_timer); >> } > > So we now call timer_new in realize, and timer_del in unrealize, > but timer_free in finalize. This seems unbalanced -- why not > call timer_free in unrealize too, if we're moving things? > > Also, this is a case of code that's actually doing things right: > we free the memory that we allocated in init in finalize. So > we're not fixing any leak here, we're just moving code around > (which is reasonable if we're trying to standardize on "call > timer_new in realize, not init", but should be noted in the > commit message). While I understand your point, I am confused because the documentation on unrealize() and finalize() is rather scarce (and not obvious for non-native english speaker). I think I'm not understanding QOM instance lifetime well (in particular hotplug devices) so I will let this series go. Similar confusions: * https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg03079.html Eduardo Habkost wrote: > Philippe Mathieu-Daudé wrote: >> IIUC when we use both init() and realize(), realize() should >> only contains on code that consumes the object properties... >> But maybe the design is not clear. Then why not move all the >> init() code to realize()? > > Normally I would recommend the opposite: delay as much as > possible to realize(), to avoid unwanted side effects when > (e.g.) running qom-list-properties. * https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04143.html David Hildenbrand wrote: >> Philippe Mathieu-Daudé wrote: >> Similarly to your other cleanups, shouldn't we move the >> timer_new_ns() into a realize() function, then do the >> timer_del() in unrealize()? > > include/hw/qdev-core.h > > "Trivial field initializations should go into > #TypeInfo.instance_init. Operations depending on @props > static properties should go into @realize." > > thanks > -- PMM >
On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 2/17/20 2:25 PM, Peter Maydell wrote: > > So we now call timer_new in realize, and timer_del in unrealize, > > but timer_free in finalize. This seems unbalanced -- why not > > call timer_free in unrealize too, if we're moving things? > > > > Also, this is a case of code that's actually doing things right: > > we free the memory that we allocated in init in finalize. So > > we're not fixing any leak here, we're just moving code around > > (which is reasonable if we're trying to standardize on "call > > timer_new in realize, not init", but should be noted in the > > commit message). > > While I understand your point, I am confused because the documentation > on unrealize() and finalize() is rather scarce (and not obvious for > non-native english speaker). I think I'm not understanding QOM instance > lifetime well (in particular hotplug devices) so I will let this series go. Yes, the documentation is really not good at all. The basic structure as I understand it is that we have two-part creation and two-part destruction: * instance_init is creation part 1 * realize is creation part 2 * unrealize is destruction part 1 and is the opposite of realize * instance_finalize is destruction part 2 and is the opposite of instance_init (Base QOM objects only have instance_init/instance_finalize; realize/unrealize exists only for DeviceState objects and their children.) ASCII-art state diagram: [start] --instance_init-> [inited] --realize-> [realized] ^ | ^ | \---instance_finalize---/ \-----unrealize-------/ In practice the only sequences we really care about are: instance_init; realize; unrealize; instance_finalize (a full object creation-use-destruction cycle; even if realize fails, unrealize will be called) instance_init; realize (a subset of the above: for non-hot-pluggable devices we will never try to unrealize them, so this is as far as it goes for most devices unless they returned an error from their realize function) instance_init; instance_finalize (the monitor does this for introspection of an object without actually wanting to create and use it; it's also the basic lifecycle for non-DeviceState objects) The difference between hot-pluggable and not is just whether it's valid to try to unrealize the device. We should definitely be clearer about what belongs in instance_init vs what belongs in realize. But where we have both a "do thing" and a "clean up that thing" task, we should put the cleanup code in the operation that is the pair of the operation we put the "do thing" code in (i.e. do thing in instance_init, clean it up in finalize; or do thing in realize, clean it up in unrealize). thanks -- PMM
On 2/17/20 3:06 PM, Peter Maydell wrote: > On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 2/17/20 2:25 PM, Peter Maydell wrote: > >>> So we now call timer_new in realize, and timer_del in unrealize, >>> but timer_free in finalize. This seems unbalanced -- why not >>> call timer_free in unrealize too, if we're moving things? >>> >>> Also, this is a case of code that's actually doing things right: >>> we free the memory that we allocated in init in finalize. So >>> we're not fixing any leak here, we're just moving code around >>> (which is reasonable if we're trying to standardize on "call >>> timer_new in realize, not init", but should be noted in the >>> commit message). >> >> While I understand your point, I am confused because the documentation >> on unrealize() and finalize() is rather scarce (and not obvious for >> non-native english speaker). I think I'm not understanding QOM instance >> lifetime well (in particular hotplug devices) so I will let this series go. > > Yes, the documentation is really not good at all. The > basic structure as I understand it is that we have two-part > creation and two-part destruction: > * instance_init is creation part 1 > * realize is creation part 2 > * unrealize is destruction part 1 and is the opposite of realize > * instance_finalize is destruction part 2 and is the > opposite of instance_init > > (Base QOM objects only have instance_init/instance_finalize; > realize/unrealize exists only for DeviceState objects > and their children.) > > ASCII-art state diagram: > > [start] --instance_init-> [inited] --realize-> [realized] > ^ | ^ | > \---instance_finalize---/ \-----unrealize-------/ > > In practice the only sequences we really care about are: > instance_init; realize; unrealize; instance_finalize > (a full object creation-use-destruction cycle; > even if realize fails, unrealize will be called) > instance_init; realize > (a subset of the above: for non-hot-pluggable devices > we will never try to unrealize them, so this is > as far as it goes for most devices unless they > returned an error from their realize function) Per this comment in qdev.c, unrealize() is the expected default: /* by default all devices were considered as hotpluggable, * so with intent to check it in generic qdev_unplug() / * device_set_realized() functions make every device * hotpluggable. Devices that shouldn't be hotpluggable, * should override it in their class_init() */ dc->hotpluggable = true; > instance_init; instance_finalize > (the monitor does this for introspection of an object > without actually wanting to create and use it; it's > also the basic lifecycle for non-DeviceState objects) > > The difference between hot-pluggable and not is just > whether it's valid to try to unrealize the device. > > We should definitely be clearer about what belongs in > instance_init vs what belongs in realize. But where we > have both a "do thing" and a "clean up that thing" task, > we should put the cleanup code in the operation that is > the pair of the operation we put the "do thing" code in > (i.e. do thing in instance_init, clean it up in finalize; > or do thing in realize, clean it up in unrealize). With the following code snippet: -- >8 -- diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 3937d1eb1a..00d1e5c0e5 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -859,6 +859,16 @@ static void device_set_realized(Object *obj, bool value, Error **errp) bool unattached_parent = false; static int unattached_count; + if (!dc->hotpluggable && dc->unrealize) { + fprintf(stderr, + "type '%s' is not hotpluggable and implements unrealize()\n", + object_get_typename(obj)); + } + if (dc->hotpluggable && !dc->unrealize) { + fprintf(stderr, "type '%s' is hotpluggable and misses unrealize()\n", + object_get_typename(obj)); + } + if (dev->hotplugged && !dc->hotpluggable) { error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); return; diff --git a/qom/object.c b/qom/object.c index 555c8b9d07..2d8d166cba 100644 --- a/qom/object.c +++ b/qom/object.c @@ -347,6 +347,16 @@ static void type_initialize(TypeImpl *ti) type_initialize_interface(ti, t, t); } + + if (!type_is_ancestor(ti, type_get_by_name(TYPE_DEVICE)) + && !type_is_ancestor(ti, type_get_by_name(TYPE_INTERFACE)) + && !ti->instance_finalize + && !parent->instance_finalize + && !parent->abstract) { + fprintf(stderr, + "type '%s' misses instance_finalize() [parent '%s']\n", + ti->name, parent->name); + } } else { ti->class->properties = g_hash_table_new_full( g_str_hash, g_str_equal, NULL, object_property_free); --- I get for qemu-system-aarch64: - qdev objects missing instance_finalize(): bcm2835-sdhost-bus PCIE pxa2xx-mmci-bus qtest-accel sdhci-bus tcg-accel - non-hotpluggable devices implementing unrealize(): VGA - hotpluggable devices missing unrealize() a15mpcore_priv a9mpcore_priv a9-scu acpi-ged ads7846 aer915 allwinner-a10 allwinner-a10-pic allwinner-A10-timer allwinner-ahci allwinner-emac arm11mpcore_priv arm11-scu ARM,bitband-memory arm.cortex-a9-global-timer arm_gic arm-gicv2m arm-gicv3 arm_mptimer arm-smmuv3 armsse-cpuid armsse-mhu armv7m armv7m_nvic armv7m_systick aspeed.fmc-ast2400 aspeed.fmc-ast2500 aspeed.fmc-ast2600 aspeed.gpio-ast2400 aspeed.gpio-ast2500 aspeed.gpio-ast2600-1_8v aspeed.gpio-ast2600 aspeed.i2c-ast2400 aspeed.i2c-ast2500 aspeed.i2c-ast2600 aspeed-mmi aspeed.rtc aspeed.scu-ast2400 aspeed.scu-ast2500 aspeed.scu-ast2600 aspeed.sdhci aspeed.sdmc-ast2400 aspeed.sdmc-ast2500 aspeed.sdmc-ast2600 aspeed.spi1-ast2400 aspeed.spi1-ast2500 aspeed.spi1-ast2600 aspeed.spi2-ast2500 aspeed.spi2-ast2600 aspeed.timer-ast2400 aspeed.timer-ast2500 aspeed.timer-ast2600 aspeed.vic aspeed.wdt-ast2400 aspeed.wdt-ast2500 aspeed.wdt-ast2600 aspeed.xdma ast2400-a1 ast2500-a1 ast2600-a0 bcm2835-aux bcm2835-dma bcm2835-fb bcm2835_gpio bcm2835-ic bcm2835-mbox bcm2835-peripherals bcm2835-property bcm2835-rng bcm2835-sdhost bcm2835-sys-timer bcm2835-thermal bcm2836-control bcm2836 bcm2837 cadence_gem cadence_ttc cadence_uart cfi.pflash01 cmsdk-apb-dualtimer cmsdk-apb-timer cmsdk-apb-uart cmsdk-apb-watchdog corgi-ssp cpu-cluster designware-pcie-host digic digic-timer digic-uart dpcd ds1338 exynos4210.clk exynos4210.combiner exynos4210-ehci-usb exynos4210.fimd exynos4210.gic exynos4210.i2c exynos4210.irq_gate exynos4210 exynos4210.mct exynos4210.pmu exynos4210.pwm exynos4210.rng exynos4210.rtc exynos4210.uart fsl,imx25 fsl,imx31 fsl,imx6 fsl,imx6ul fsl,imx7 ftgmac100 fw_cfg_mem gpex-pcihost gpio_i2c gpio-key highbank-regs i2c-ddc icp-ctrl-regs imx25.ccm imx25.gpt imx2.wdt imx31.ccm imx31.gpt imx6.ccm imx6.gpt imx6.src imx6ul.ccm imx7.analog imx7.ccm imx7.gpr imx7.gpt imx7.snvs imx.avic imx.enet imx.epit imx.fec imx-gpcv2 imx.gpio imx.i2c imx.rngc imx.serial imx.spi integrator_core integrator_debug integrator_pic integrator_pit iotkit iotkit-secctl iotkit-sysctl iotkit-sysinfo l2x0 lan9118 lm8323 luminary-watchdog mainstone-fpga max1111 max7310 microbit.i2c mps2-fpgaio mps2-scc msf2-soc msf2-sysreg mss-spi mss-timer musicpal_gpio musicpal_key musicpal_lcd musicpal-misc mv88w8618_audio mv88w8618_eth mv88w8618_flashcfg mv88w8618_pic mv88w8618_pit mv88w8618_wlan mx25l25635e mx66l1g45g mx66u51235f n25q128 n25q256a n25q512a11 nand nrf51_soc.gpio nrf51-soc nrf51_soc.nvm nrf51_soc.rng nrf51_soc.timer nrf51_soc.uart omap2-gpio omap2-intc omap-gpio omap_i2c omap-intc onenand or-irq pca9552 pl011 pl011_luminary pl022 pl031 pl041 pl050_keyboard pl050_mouse pl061 pl061_luminary pl080 pl081 pl110 pl110_versatile pl111 pl181 pl190 pl330 platform-bus-device platform-ehci-usb pxa25x-timer pxa27x-timer pxa2xx-dma pxa2xx-gpio pxa2xx_i2c pxa2xx-i2c-slave pxa2xx-pcmcia pxa2xx_pic pxa2xx_rtc pxa2xx-ssp realview_gic realview_mpcore realview_pci realview_sysctl s25sl12801 scoop sd-card serial-mm sii9022 sl-nand smbus-eeprom smc91c111 sp804 spitz-keyboard spitz-lcdtg split-irq ssd0303 ssd0323 sse-200 ssi-sd sst25vf016b sst25wf080 stellaris-adc stellaris_enet stellaris-gptm stellaris-i2c stm32f205-soc stm32f2xx-adc stm32f2xx-spi stm32f2xx-syscfg stm32f2xx-timer stm32f2xx-usart stm32f405-soc stm32f4xx-exti stm32f4xx-syscfg strongarm-gpio strongarm_pic strongarm-ppc strongarm-rtc strongarm-ssp strongarm-uart sysbus-ahci sysbus-ohci tmp105 tmp423 tosa_dac tosa-ssp twl92230 tz-mpc tz-msc tz-ppc unimplemented-device usb-chipidea versatile_i2c versatilepb_sic versatile_pci virtio-mmio w25q256 w25q512jv wm8750 xgmac xilinx,zynq_slcr xlnx.dpdma xlnx.ps7-dev-cfg xlnx.ps7-qspi xlnx.ps7-spi xlnx,ps7-usb xlnx.usmp-gqspi xlnx.v-dp xlnx-versal xlnx.zdma xlnx-zynmp.rtc xlnx.zynqmp_ipi xlnx,zynqmp xlnx,zynq-xadc zipit-lcd Most of these are sysbus devices. The list is big, I probably have something wrong.
On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Per this comment in qdev.c, unrealize() is the expected default: > > /* by default all devices were considered as hotpluggable, > * so with intent to check it in generic qdev_unplug() / > * device_set_realized() functions make every device > * hotpluggable. Devices that shouldn't be hotpluggable, > * should override it in their class_init() > */ > dc->hotpluggable = true; This comment sounds like it's documenting what was the previous default ("were considered") and making a minimal change to avoid breaking existing code where a device does want to be hotpluggable but isn't explicitly saying so. I forget how exactly it works (the mechanism has changed several times) but in practice a sysbus device is generally not hotpluggable, and that's what most devices are. > I get for qemu-system-aarch64: > > > - qdev objects missing instance_finalize(): > > bcm2835-sdhost-bus > PCIE > pxa2xx-mmci-bus > qtest-accel > sdhci-bus > tcg-accel Note that you don't need an instance_finalize() if it would have nothing to do, which may be the case here. > - non-hotpluggable devices implementing unrealize(): > > VGA Not sure which device this is, I couldn't find a TYPE_ define with this name. Is it an abstract or common device type used by the hotpluggable pci VGA card? > - hotpluggable devices missing unrealize() > > a15mpcore_priv > a9mpcore_priv Most of these are not really hotpluggable. What is confusing your test code is that sysbus devices get the default 'hotpluggable = true' setting, but other conditions usually prevent hotplugging. (The reason hotpluggable is true is the default is because of handling of 'dynamic sysbus' devices which live on the 'platform bus'.) In particular, I think that anything with !dc->user_creatable can't be hotplugged unless board code specifically tries it, which would be a bug for most of these devices. Also, if there isn't anything for a device's unrealize method to do, it doesn't need to provide one. thanks -- PMM
On 2/17/20 5:32 PM, Peter Maydell wrote: > On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Per this comment in qdev.c, unrealize() is the expected default: >> >> /* by default all devices were considered as hotpluggable, >> * so with intent to check it in generic qdev_unplug() / >> * device_set_realized() functions make every device >> * hotpluggable. Devices that shouldn't be hotpluggable, >> * should override it in their class_init() >> */ >> dc->hotpluggable = true; > > This comment sounds like it's documenting what was the > previous default ("were considered") and making a minimal > change to avoid breaking existing code where a device > does want to be hotpluggable but isn't explicitly saying so. > I forget how exactly it works (the mechanism has changed > several times) but in practice a sysbus device is generally > not hotpluggable, and that's what most devices are. > >> I get for qemu-system-aarch64: >> >> >> - qdev objects missing instance_finalize(): >> >> bcm2835-sdhost-bus >> PCIE >> pxa2xx-mmci-bus >> qtest-accel >> sdhci-bus >> tcg-accel > > Note that you don't need an instance_finalize() if it > would have nothing to do, which may be the case here. > >> - non-hotpluggable devices implementing unrealize(): >> >> VGA > > Not sure which device this is, I couldn't find a TYPE_ > define with this name. Is it an abstract or common > device type used by the hotpluggable pci VGA card? This is TYPE_PCI_VGA (hw/display/vga-pci.c). > >> - hotpluggable devices missing unrealize() >> >> a15mpcore_priv >> a9mpcore_priv > > Most of these are not really hotpluggable. What is > confusing your test code is that sysbus devices get > the default 'hotpluggable = true' setting, but other > conditions usually prevent hotplugging. (The reason > hotpluggable is true is the default is because of > handling of 'dynamic sysbus' devices which live on > the 'platform bus'.) In particular, I think that > anything with !dc->user_creatable can't be hotplugged > unless board code specifically tries it, which would > be a bug for most of these devices. OK, checking '!dc->user_creatable' removes: ads7846 aer915 corgi-ssp dpcd ds1338 i2c-ddc lm8323 max1111 max7310 mx25l25635e mx66l1g45g mx66u51235f n25q128 n25q256a n25q512a11 nand pca9552 pxa2xx-i2c-slave s25sl12801 sd-card sii9022 spitz-lcdtg ssd0303 ssd0323 sst25vf016b sst25wf080 tmp105 tmp423 tosa_dac tosa-ssp twl92230 w25q256 w25q512jv wm8750 zipit-lcd Previous list only reduced from 300 to 265. I noticed this function, maybe I need to check parent_bus too: static bool device_get_hotpluggable(Object *obj, Error **errp) { DeviceClass *dc = DEVICE_GET_CLASS(obj); DeviceState *dev = DEVICE(obj); return dc->hotpluggable && (dev->parent_bus == NULL || qbus_is_hotpluggable(dev->parent_bus)); } > > Also, if there isn't anything for a device's unrealize > method to do, it doesn't need to provide one. This point is hard to check with simply with qtest. Thanks for your comments, it helped sorting few things out. > > thanks > -- PMM >
On Mon, 17 Feb 2020 at 17:12, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > OK, checking '!dc->user_creatable' removes: [...] > Previous list only reduced from 300 to 265. You could look at what condition we check for that causes this: $ ./build/x86/arm-softmmu/qemu-system-arm -M virt -device a15mpcore_priv qemu-system-arm: -device a15mpcore_priv: Parameter 'driver' expects pluggable device type which will probably let you rule out some more devices. thanks -- PMM
On 2/17/20 6:12 PM, Philippe Mathieu-Daudé wrote: > On 2/17/20 5:32 PM, Peter Maydell wrote: >> On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé [...] >>> - hotpluggable devices missing unrealize() >>> >>> a15mpcore_priv >>> a9mpcore_priv >> >> Most of these are not really hotpluggable. What is >> confusing your test code is that sysbus devices get >> the default 'hotpluggable = true' setting, but other >> conditions usually prevent hotplugging. (The reason >> hotpluggable is true is the default is because of >> handling of 'dynamic sysbus' devices which live on >> the 'platform bus'.) In particular, I think that >> anything with !dc->user_creatable can't be hotplugged >> unless board code specifically tries it, which would >> be a bug for most of these devices. [...] >> Also, if there isn't anything for a device's unrealize >> method to do, it doesn't need to provide one. > > This point is hard to check with simply with qtest. > > Thanks for your comments, it helped sorting few things out. Quick check with TYPE_BITBAND which is a SysBus device, we have: static void bitband_realize(DeviceState *dev, Error **errp) { BitBandState *s = BITBAND(dev); if (!s->source_memory) { error_setg(errp, "source-memory property not set"); return; } address_space_init(&s->source_as, s->source_memory, "bitband-source"); } Do we need the equivalent: static void bitband_unrealize(DeviceState *dev, Error **errp) { BitBandState *s = BITBAND(dev); address_space_destroy(&s->source_as); } Or instead mark the device user_creatable=false because of the link to a TYPE_MEMORY_REGION? > >> >> thanks >> -- PMM >>
On Mon, 17 Feb 2020 at 17:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Quick check with TYPE_BITBAND which is a SysBus device, we have: > > static void bitband_realize(DeviceState *dev, Error **errp) > { > BitBandState *s = BITBAND(dev); > > if (!s->source_memory) { > error_setg(errp, "source-memory property not set"); > return; > } > > address_space_init(&s->source_as, s->source_memory, "bitband-source"); > } > > Do we need the equivalent: > > static void bitband_unrealize(DeviceState *dev, Error **errp) > { > BitBandState *s = BITBAND(dev); > > address_space_destroy(&s->source_as); > } > > Or instead mark the device user_creatable=false because of the link to a > TYPE_MEMORY_REGION? I don't believe that this device is user-creatable. The base class sysbus_device_class_init() sets user_creatable to false by default for all sysbus devices, and a sysbus device which wants to opt into being user-created has to set it to true. Also the device's type name string is "ARM,bitband-memory" and the -device option at least does not like the comma in the middle of the name, so I don't know how you'd create it on the command line even if it wasn't marked not user-creatable. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 2/17/20 2:25 PM, Peter Maydell wrote: > >> > So we now call timer_new in realize, and timer_del in unrealize, >> > but timer_free in finalize. This seems unbalanced -- why not >> > call timer_free in unrealize too, if we're moving things? >> > >> > Also, this is a case of code that's actually doing things right: >> > we free the memory that we allocated in init in finalize. So >> > we're not fixing any leak here, we're just moving code around >> > (which is reasonable if we're trying to standardize on "call >> > timer_new in realize, not init", but should be noted in the >> > commit message). >> >> While I understand your point, I am confused because the documentation >> on unrealize() and finalize() is rather scarce (and not obvious for >> non-native english speaker). I think I'm not understanding QOM instance >> lifetime well (in particular hotplug devices) so I will let this series go. > > Yes, the documentation is really not good at all. The > basic structure as I understand it is that we have two-part > creation and two-part destruction: > * instance_init is creation part 1 > * realize is creation part 2 > * unrealize is destruction part 1 and is the opposite of realize > * instance_finalize is destruction part 2 and is the > opposite of instance_init > > (Base QOM objects only have instance_init/instance_finalize; > realize/unrealize exists only for DeviceState objects > and their children.) The split exists so you can set property values between instance_init() and realize(). It's how qdev has always worked. It permits setting properties one by one even when this results in intermediate states where invariants involving multiple property values are violated: delay checking them until realize(), rely on them only while the device is realized. Note that both realize() and unrealize() can fail. instance_init() and instance_finalize() can't. > ASCII-art state diagram: > > [start] --instance_init-> [inited] --realize-> [realized] > ^ | ^ | > \---instance_finalize---/ \-----unrealize-------/ > > In practice the only sequences we really care about are: > instance_init; realize; unrealize; instance_finalize > (a full object creation-use-destruction cycle; > even if realize fails, unrealize will be called) > instance_init; realize > (a subset of the above: for non-hot-pluggable devices > we will never try to unrealize them, so this is > as far as it goes for most devices unless they > returned an error from their realize function) > instance_init; instance_finalize > (the monitor does this for introspection of an object > without actually wanting to create and use it; it's > also the basic lifecycle for non-DeviceState objects) In theory, you can realize + unrealize multiple times. It might even work in practice sometimes. > The difference between hot-pluggable and not is just > whether it's valid to try to unrealize the device. > > We should definitely be clearer about what belongs in > instance_init vs what belongs in realize. But where we > have both a "do thing" and a "clean up that thing" task, > we should put the cleanup code in the operation that is > the pair of the operation we put the "do thing" code in > (i.e. do thing in instance_init, clean it up in finalize; > or do thing in realize, clean it up in unrealize). Not doing so risks introspection leaks or double-frees.
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Per this comment in qdev.c, unrealize() is the expected default: >> >> /* by default all devices were considered as hotpluggable, >> * so with intent to check it in generic qdev_unplug() / >> * device_set_realized() functions make every device >> * hotpluggable. Devices that shouldn't be hotpluggable, >> * should override it in their class_init() >> */ >> dc->hotpluggable = true; Please note that "hot-pluggable" is *not* required for "need unrealize() to work", at least in theory. Cold plug exists, and cold unplug could exist. > This comment sounds like it's documenting what was the > previous default ("were considered") and making a minimal > change to avoid breaking existing code where a device > does want to be hotpluggable but isn't explicitly saying so. Commit 1a37eca107, six years ago: qdev: add "hotpluggable" property to Device Currently it's possible to make PCIDevice not hotpluggable by using no_hotplug field of PCIDeviceClass. However it limits this only to PCI devices and prevents from generalizing hotplug code. So add similar field to DeviceClass so it could be reused with other Devices and would allow to replace PCI specific hotplug callbacks with generic implementation. Following patches will replace PCIDeviceClass.no_hotplug with this new property. In addition expose field as "hotpluggable" readonly property, to make it possible to read its value via QOM interface. Make DeviceClass hotpluggable by default as it was assumed before. > I forget how exactly it works (the mechanism has changed > several times) but in practice a sysbus device is generally > not hotpluggable, and that's what most devices are. A device's "hot-pluggability" comes into play only when both bus and machine support hot-plug. And before we even get there, the device needs to be "pluggable", i.e. dc->user_creatable. Bus types supporting hot plug include PCI, SCSI, USB, virtio-serial-bus. Grep for qbus_set_hotplug_handler().
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 17 Feb 2020 at 17:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Quick check with TYPE_BITBAND which is a SysBus device, we have: >> >> static void bitband_realize(DeviceState *dev, Error **errp) >> { >> BitBandState *s = BITBAND(dev); >> >> if (!s->source_memory) { >> error_setg(errp, "source-memory property not set"); >> return; >> } >> >> address_space_init(&s->source_as, s->source_memory, "bitband-source"); >> } >> >> Do we need the equivalent: >> >> static void bitband_unrealize(DeviceState *dev, Error **errp) >> { >> BitBandState *s = BITBAND(dev); >> >> address_space_destroy(&s->source_as); >> } >> >> Or instead mark the device user_creatable=false because of the link to a >> TYPE_MEMORY_REGION? > > I don't believe that this device is user-creatable. The > base class sysbus_device_class_init() sets user_creatable > to false by default for all sysbus devices, and a sysbus > device which wants to opt into being user-created has to > set it to true. As far as I can tell, you additionally have to machine_class_allow_dynamic_sysbus_dev(). Sysbus is special. > Also the device's type name string is "ARM,bitband-memory" > and the -device option at least does not like the comma > in the middle of the name, so I don't know how you'd > create it on the command line even if it wasn't marked > not user-creatable. Double the comma. If I remember correctly, the use of comma in type comes from IEEE-1275. It's quite inappropriate for QEMU.
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c index f9a13e0a44..9144ac6c38 100644 --- a/hw/ipmi/ipmi_bmc_extern.c +++ b/hw/ipmi/ipmi_bmc_extern.c @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp) qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive, chr_event, NULL, ibe, NULL, true); + + ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); +} + +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp) +{ + IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev); + + timer_del(ibe->extern_timer); } static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id) @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj) { IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); - ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe); } @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj) { IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); - timer_del(ibe->extern_timer); timer_free(ibe->extern_timer); } @@ -528,6 +535,7 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, void *data) bk->handle_reset = ipmi_bmc_extern_handle_reset; dc->hotpluggable = false; dc->realize = ipmi_bmc_extern_realize; + dc->unrealize = ipmi_bmc_extern_unrealize; device_class_set_props(dc, ipmi_bmc_extern_properties); }
In commit f3a508eb4e the Euler Robot reported calling timer_new() in instance_init() can leak heap memory. The easier fix is to delay the timer creation at instance realize(). Similarly move timer_del() into a new instance unrealize() method. This case was found with the following coccinelle script: @ match @ identifier instance_init; typedef Object; identifier obj; expression val, scale; identifier clock_type, callback, opaque; position pos; @@ static void instance_init(Object *obj) { <... ( val = timer_new@pos(clock_type, scale, callback, opaque); | val = timer_new_ns@pos(clock_type, callback, opaque); | val = timer_new_us@pos(clock_type, callback, opaque); | val = timer_new_ms@pos(clock_type, callback, opaque); ) ...> } @ script:python @ f << match.instance_init; p << match.pos; @@ print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f) Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- Cc: Pan Nengyuan <pannengyuan@huawei.com> --- hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)