Re: [Qemu-devel] [PATCH v2] qdev: Mark devices as non-hotpluggable by default

[Thomas Huth]

On 25.09.2017 19:59, Eduardo Habkost wrote:
> On Mon, Sep 25, 2017 at 07:42:13PM +0200, Thomas Huth wrote:
>> On 25.09.2017 17:26, Peter Maydell wrote:
>>> On 25 September 2017 at 16:19, Thomas Huth <address@hidden> wrote:
>>>> Not sure whether this works for the virtio-xxx-device devices,
>>>> though, since they are marked as user_creatable = true currently...
>>>
>>> That's deliberate -- for the arm boards with virtio-mmio
>>> the board creates a bunch of virtio-mmio transports and the
>>> virtio-foo-device can be user created to plug into those.
>>
>> Yes, I know ... I'm just wondering whether the virtio-xxx-device devices
>> should be non-user_creatable on the non-ARM targets, since they
>> apparently can't be used with "-device" there...?
> 
> I wouldn't like to make DeviceClass fields depend on the target.
> Being user-creatable doesn't mean they will work on all machines,
> anyway.  If user/management need more specific info, they need
> something like the query-device-slots command I've proposed some
> time ago.
> 
>>
>>> If overall trying to do the 'right thing' is tricky here
>>> then perhaps we can start by flipping the default to
>>> not-hotpluggable and marking some devices hotpluggable
>>> that in theory shouldn't be with a comment about why.
>>
>> Yes, if Eduardo's idea to move the test to qmp_device_add() does not
>> work out (I'll try that next), your suggestion is certainly the best
>> thing we can do right now.
> 
> I think it would work, but we would lose the feature Peter
> mentions below:
> 
>>
>>> Incidentally I think there's still some advantage in the
>>> "created as part of some other device" devices having
>>> to be explicitly marked hotpluggable, because those
>>> devices do still need some specific code in them to
>>> handle it (ie code to release the resources that are
>>> created in the device's realize method).
>>
>> Ok ... by the way, does anybody know more devices like
>> virtio-xxx-device, i.e. devices that are indirectly plugged when adding
>> other devices?
> 
> "make check" found a few candidates:
> https://travis-ci.org/ehabkost/qemu/jobs/278743999
> 
>   Initialization of device dpcd failed: Device 'dpcd' does not support 
> hotplugging
> [...]
>   Initialization of device nand failed: Device 'nand' does not support 
> hotplugging

I've now had a look at those, and I now feel like the whole
"hotpluggable = false by default" idea was either just wrong or there
are other smart ideas necessary to solve this issue: These devices are
created during the instance_init() function of another device, e.g.
"dpcd" is created in the xlnx_dp_init() function, which is the
instance_init of TYPE_XLNX_DP. Now, instance_init() can be called at any
time during runtime, even without really adding the device, e.g. for
parameter introspection (try "device_add xlnx.v-dp,help" at the HMP
prompt for example).

But just setting "hotpluggable = true" for a device (e.g. "dpcd") which
could be created during instance_init also does not sound very
attractive to me... Not sure about any good alternative way to tackle
this right now, maybe I've eventually got to check user_creatable in
device_set_realized, too, or move the hotpluggable checks to
qmp_device_add() instead... I'll think about that for a while...
suggestions welcome!

 Thomas