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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>