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

[Philippe Mathieu-Daudé]

On 2/17/20 5:32 PM, Peter Maydell wrote:
> 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?

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