diff mbox series

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

Message ID 20201115184903.1292715-14-ganqixin@huawei.com (mailing list archive)
State New, archived
Headers show
Series Categorize some uncategorized devices | expand

Commit Message

Gan Qixin Nov. 15, 2020, 6:49 p.m. UTC
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(+)

Comments

Philippe Mathieu-Daudé Nov. 16, 2020, 1:25 p.m. UTC | #1
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?

>  }
>  
>  static const TypeInfo cprman_pll_info = {
> @@ -239,6 +240,7 @@ static void pll_channel_class_init(ObjectClass *klass, void *data)
>  
>      dc->reset = pll_channel_reset;
>      dc->vmsd = &pll_channel_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo cprman_pll_channel_info = {
> @@ -359,6 +361,7 @@ static void clock_mux_class_init(ObjectClass *klass, void *data)
>  
>      dc->reset = clock_mux_reset;
>      dc->vmsd = &clock_mux_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo cprman_clock_mux_info = {
> @@ -411,6 +414,7 @@ static void dsi0hsck_mux_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->vmsd = &dsi0hsck_mux_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo cprman_dsi0hsck_mux_info = {
>
Peter Maydell Nov. 16, 2020, 1:31 p.m. UTC | #2
On Mon, 16 Nov 2020 at 13:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 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?

If we could get to the point where we can assert() that
dc->categories is non-zero in class init, we would be able
to avoid further "forgot to categorize device" bugs getting
into the tree in future, which seems like an argument for
having some way of marking "really just an implementation
detail" devices I guess?

thanks
-- PMM
Thomas Huth Nov. 16, 2020, 2:30 p.m. UTC | #3
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
my eyes it's just ugly if we present devices to the users that they can not use.
(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?)

 Thomas
Thomas Huth Nov. 16, 2020, 2:33 p.m. UTC | #4
On 16/11/2020 14.31, Peter Maydell wrote:
> On Mon, 16 Nov 2020 at 13:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> 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?
> 
> If we could get to the point where we can assert() that
> dc->categories is non-zero in class init, we would be able
> to avoid further "forgot to categorize device" bugs getting
> into the tree in future, which seems like an argument for
> having some way of marking "really just an implementation
> detail" devices I guess?

IMHO we need:

  assert(dc->user_creatable == false ||  categories != 0)

then we don't need something like DEVICE_CATEGORY_COMPOSITE.

 Thomas
Markus Armbruster Nov. 16, 2020, 5 p.m. UTC | #5
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?
Peter Maydell Nov. 16, 2020, 5:15 p.m. UTC | #6
On Mon, 16 Nov 2020 at 17:09, Markus Armbruster <armbru@redhat.com> wrote:
> 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,
>     };

I'm really dubious of devices that directly inherit from
TYPE_DEVICE, because their reset method won't be automatically
called. In this case it looks like the TYPE_BCM2835_CPRMAN
device's reset method manually calls reset on these devices,
though, so it isn't actually buggy, just confusing.

thanks
-- PMM
Markus Armbruster Nov. 17, 2020, 5:41 a.m. UTC | #7
Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 16 Nov 2020 at 17:09, Markus Armbruster <armbru@redhat.com> wrote:
>> 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,
>>     };
>
> I'm really dubious of devices that directly inherit from
> TYPE_DEVICE, because their reset method won't be automatically
> called. In this case it looks like the TYPE_BCM2835_CPRMAN
> device's reset method manually calls reset on these devices,
> though, so it isn't actually buggy, just confusing.

I guess this is a trap for unwary implementers, aggravated by our usual
dearth of qdev documentation.  I can see ~100 .parent = TYPE_DEVICE
lines.  Checking them all manually won't be fun.  Any automation ideas?

The concept "bus-less device" is sane.  We used not to have it, and the
resulting need for pseudo-busses was annoying enough to make us add
bus-less devices.
Peter Maydell Nov. 17, 2020, 11:08 a.m. UTC | #8
On Tue, 17 Nov 2020 at 05:41, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > I'm really dubious of devices that directly inherit from
> > TYPE_DEVICE, because their reset method won't be automatically
> > called. In this case it looks like the TYPE_BCM2835_CPRMAN
> > device's reset method manually calls reset on these devices,
> > though, so it isn't actually buggy, just confusing.
>
> I guess this is a trap for unwary implementers, aggravated by our usual
> dearth of qdev documentation.  I can see ~100 .parent = TYPE_DEVICE
> lines.  Checking them all manually won't be fun.  Any automation ideas?
>
> The concept "bus-less device" is sane.  We used not to have it, and the
> resulting need for pseudo-busses was annoying enough to make us add
> bus-less devices.

Yeah, the problem really is that our reset handling remains a
mess. I'm not sure (a) what the right model for reset is and
(b) what would be a feasible transition plan to get from
here to there...

thanks
-- PMM
Gan Qixin Nov. 17, 2020, 2:02 p.m. UTC | #9
> -----Original Message-----
> From: Thomas Huth [mailto:thuth@redhat.com]
> Sent: Monday, November 16, 2020 10:30 PM
> To: Philippe Mathieu-Daudé <f4bug@amsat.org>; ganqixin
> <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Markus Armbruster
> <armbru@redhat.com>
> Subject: Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of
> bcm2835 cprman into the 'misc' category
> 
> 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 my eyes it's
> just ugly if we present devices to the users that they can not use.
> (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?)

I agree, but marking user_creatable as false seems to make it deviate from its own meaning (like markus said).
Is there any other way to avoid presenting devices to the users in "-device help"?

Thanks,
Gan Qixin
Thomas Huth Nov. 17, 2020, 8:12 p.m. UTC | #10
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
Markus Armbruster Nov. 18, 2020, 8:41 a.m. UTC | #11
Thomas Huth <thuth@redhat.com> writes:

> 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 */

For hot-plug to actually work, both the plug / device and the socket /
bus need to support it.  CREATABLE_BY_HOTPLUG would be about the former.
The latter could depend on bus or machine state.

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

The future arrives when the need for it overcomes inertia :)

[...]
Markus Armbruster Nov. 18, 2020, 8:50 a.m. UTC | #12
Markus Armbruster <armbru@redhat.com> writes:

[...]
> 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.

Nope.  A hotplug handler is only required for hot plug (d'oh!), not for
cold plug.  

I wonder whether bus-less devices should default to .user_creatable =
false like sysbus devices, and for the same reasons.

To actually *do* something, a physical device requires some connection
to the rest of the world.  Same for a virtual device (at least the ones
that model physical devices).

sysbus_device_class_init():

    /*
     * device_add plugs devices into a suitable bus.  For "real" buses,
     * that actually connects the device.  For sysbus, the connections
     * need to be made separately, and device_add can't do that.  The
     * device would be left unconnected, and will probably not work
     *
     * However, a few machines can handle device_add/-device with
     * a few specific sysbus devices. In those cases, the device
     * subclass needs to override it and set user_creatable=true.
     */
    k->user_creatable = false;

The reasoning applies to bus-less devices just as well, doesn't it?

[...]
Thomas Huth Nov. 18, 2020, 9:08 a.m. UTC | #13
On 18/11/2020 09.50, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> [...]
>> 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.
> 
> Nope.  A hotplug handler is only required for hot plug (d'oh!), not for
> cold plug.  
> 
> I wonder whether bus-less devices should default to .user_creatable =
> false like sysbus devices, and for the same reasons.
> 
> To actually *do* something, a physical device requires some connection
> to the rest of the world.  Same for a virtual device (at least the ones
> that model physical devices).

I know at least two virtual devices that are bus-less and cold-pluggable:
hw/ppc/spapr_rng.c and hw/watchdog/wdt_diag288.c ... but we could certainly
mark them with user_creatable = true manually if we decide that bus-less
devices should be handled differently by default.

 Thomas
diff mbox series

Patch

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);
 }
 
 static const TypeInfo cprman_pll_info = {
@@ -239,6 +240,7 @@  static void pll_channel_class_init(ObjectClass *klass, void *data)
 
     dc->reset = pll_channel_reset;
     dc->vmsd = &pll_channel_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_pll_channel_info = {
@@ -359,6 +361,7 @@  static void clock_mux_class_init(ObjectClass *klass, void *data)
 
     dc->reset = clock_mux_reset;
     dc->vmsd = &clock_mux_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_clock_mux_info = {
@@ -411,6 +414,7 @@  static void dsi0hsck_mux_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &dsi0hsck_mux_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_dsi0hsck_mux_info = {