Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 16/11/2020 14.25, Philippe Mathieu-Daudé wrote:
>> Hi Gan,
>> 
>> On 11/15/20 7:49 PM, Gan Qixin wrote:
>>> Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
>>> category.
>>>
>>> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
>>> ---
>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/misc/bcm2835_cprman.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>>> index 7e415a017c..c62958a99e 100644
>>> --- a/hw/misc/bcm2835_cprman.c
>>> +++ b/hw/misc/bcm2835_cprman.c
>>> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
>>>  
>>>      dc->reset = pll_reset;
>>>      dc->vmsd = &pll_vmstate;
>>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> 
>> Well, this is not an usable device but a part of a bigger device,
>> so here we want the opposite: not list this device in any category.
>> 
>> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
>> types so management apps can filter them out? (And so we are sure
>> all QOM is classified).
>> 
>> Thomas, you already dealt with categorizing devices in the past,
>> what do you think about this? Who else could help? Maybe add
>> someone from libvirt in the thread?
>
> My 0.02 € : Mark the device as user_creatable = false if it can not really
> be used by the user with the -device CLI parameter. Then it also does not
> need a category. I know Markus will likely have a different opinion, but in

You're hurting my feelings!  ;-P

> my eyes it's just ugly if we present devices to the users that they can not use.

If we believe a device should only ever be used from C, then we should
keep it away from the UI.

However, I'm wary of overloading user_creatable.  Even though it has
shifted shape a number of times (cannot_instantiate_with_device_add_yet,
no_user, and now user_creatable), its purpose has always been focused:
distinguishing devices that can be instantiated by generic code from the
ones that need device-specific code.  See user_creatable's comment in
qdev-core.h.

I don't want to lose that distinction.  That's all.

> (By the way, this device here seems to be a decendant of TYPE_SYS_BUS_DEVICE
> ... shouldn't these show up as user_creatable = false automatically?)

Yes, unless it is a dynamic sysbus device (which I consider a flawed
concept).

But TYPE_CPRMAN_PLL is *not* a descendant of TYPE_SYS_BUS_DEVICE, it's a
bus-less device:

    static const TypeInfo cprman_pll_info = {
        .name = TYPE_CPRMAN_PLL,
--->    .parent = TYPE_DEVICE,
        .instance_size = sizeof(CprmanPllState),
        .class_init = pll_class_init,
        .instance_init = pll_init,
    };

Unless bus-less devices are somehow usable with -device, they should
have user_creatable = false.

qdev_device_add() looks like a bus-less device is usable if the machine
provides a hotplug handler for it.  Commit 03fcbd9dc5 "qdev: Check for
the availability of a hotplug controller before adding a device" seems
to be pertinent.

Are there any hotplug handlers for this device?  If yes, which machines
provide one?