Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <address@hidden> 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