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

From: Thomas Huth <thuth@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: zhang.zhanghailiang@huawei.com, qemu-trivial@nongnu.org,
	qemu-devel@nongnu.org, "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Gan Qixin" <ganqixin@huawei.com>,
	kuhn.chenqun@huawei.com
Subject: Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category
Date: Tue, 17 Nov 2020 21:12:49 +0100	[thread overview]
Message-ID: <b5c76a27-551f-5b79-a47b-0d4a283a2d37@redhat.com> (raw)
In-Reply-To: <877dqldyoh.fsf@dusky.pond.sub.org>

On 16/11/2020 18.00, Markus Armbruster wrote:
> 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.

Well, currently we have the user_creatable flag and the hotpluggable flag. I
guess that's simply not enough.

I think in the long run, we should maybe replace the two flags with a
"creatable" type instead that could take the following values:

 CREATABLE_AS_SUBDEVICE  /* Device is part of another device and
                            can only by added by code */
 CREATABLE_BY_QOM        /* Some fancy new QOM function can be
                            used to e.g. create this as part of
                            a machine */
 CREATABLE_BY_COLDPLUG   /* For cold-plugging via -device */
 CREATABLE_BY_HOTPLUG    /* For hot-plugging via device_add */

... but that's likely something for the distant future...

>> (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:

Oops, I obviously looked at the wrong device in that file
(TYPE_BCM2835_CPRMAN instead of TYPE_CPRMAN_PLL) - thanks for the clarification!

 Thomas