This flag will be used in device_add to check if
the device needs special allowance from the machine
model.

It will replace the current check based only on the
device being a TYPE_SYB_BUS_DEVICE.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

v2:
 + change the flag name and put it just below user_creatable
---
 include/hw/qdev-core.h | 9 +++++++++
 hw/core/qdev.c         | 1 +
 hw/core/sysbus.c       | 1 +
 3 files changed, 11 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 92c3d65208..6a040fcd3b 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -122,6 +122,15 @@ struct DeviceClass {
      * TODO remove once we're there
      */
     bool user_creatable;
+    /*
+     * Some devices can be user created under certain conditions (eg:
+     * specific machine support for sysbus devices), but it is
+     * preferable to prevent global allowance for the reasons
+     * described above.
+     * This flag is an additional constraint over user_creatable:
+     * user_creatable still needs to be set to true.
+     */
+    bool user_creatable_requires_machine_allowance;
     bool hotpluggable;
 
     /* callbacks */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 84f3019440..0844c85a21 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -833,6 +833,7 @@ static void device_class_init(ObjectClass *class, void *data)
      */
     dc->hotpluggable = true;
     dc->user_creatable = true;
+    dc->user_creatable_requires_machine_allowance = false;
     vc->get_id = device_vmstate_if_get_id;
     rc->get_state = device_get_reset_state;
     rc->child_foreach = device_reset_child_foreach;
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 05c1da3d31..5f771ed1e9 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -325,6 +325,7 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
      * subclass needs to override it and set user_creatable=true.
      */
     k->user_creatable = false;
+    k->user_creatable_requires_machine_allowance = true;
 }
 
 static const TypeInfo sysbus_device_type_info = {
-- 
2.35.1

On Thu, 31 Mar 2022 at 13:19, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This flag will be used in device_add to check if
> the device needs special allowance from the machine
> model.
>
> It will replace the current check based only on the
> device being a TYPE_SYB_BUS_DEVICE.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> v2:
>  + change the flag name and put it just below user_creatable
> ---
>  include/hw/qdev-core.h | 9 +++++++++
>  hw/core/qdev.c         | 1 +
>  hw/core/sysbus.c       | 1 +
>  3 files changed, 11 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..6a040fcd3b 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -122,6 +122,15 @@ struct DeviceClass {
>       * TODO remove once we're there
>       */
>      bool user_creatable;
> +    /*
> +     * Some devices can be user created under certain conditions (eg:
> +     * specific machine support for sysbus devices), but it is
> +     * preferable to prevent global allowance for the reasons
> +     * described above.
> +     * This flag is an additional constraint over user_creatable:
> +     * user_creatable still needs to be set to true.
> +     */
> +    bool user_creatable_requires_machine_allowance;

"allowance" doesn't have the meaning you seem to be trying to give it here.
(It means "the amount of something you're allowed to have", like
a baggage allowance, or "an amount of money you're given for something",
like a travel allowance.)

Do you mean "user creatable only if the machine permits it" ?

More generally, the pluggable-sysbus stuff is an awful hack
that I wish we didn't have to have. I'm not sure I want to see
us expanding the use of it to other device types...

thanks
-- PMM

On 4/21/22 17:59, Peter Maydell wrote:
> On Thu, 31 Mar 2022 at 13:19, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> This flag will be used in device_add to check if
>> the device needs special allowance from the machine
>> model.
>>
>> It will replace the current check based only on the
>> device being a TYPE_SYB_BUS_DEVICE.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> v2:
>>   + change the flag name and put it just below user_creatable
>> ---
>>   include/hw/qdev-core.h | 9 +++++++++
>>   hw/core/qdev.c         | 1 +
>>   hw/core/sysbus.c       | 1 +
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 92c3d65208..6a040fcd3b 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -122,6 +122,15 @@ struct DeviceClass {
>>        * TODO remove once we're there
>>        */
>>       bool user_creatable;
>> +    /*
>> +     * Some devices can be user created under certain conditions (eg:
>> +     * specific machine support for sysbus devices), but it is
>> +     * preferable to prevent global allowance for the reasons
>> +     * described above.
>> +     * This flag is an additional constraint over user_creatable:
>> +     * user_creatable still needs to be set to true.
>> +     */
>> +    bool user_creatable_requires_machine_allowance;
> 
> "allowance" doesn't have the meaning you seem to be trying to give it here.
> (It means "the amount of something you're allowed to have", like
> a baggage allowance, or "an amount of money you're given for something",
> like a travel allowance.)

> 
> Do you mean "user creatable only if the machine permits it" ?
Yes.

> 
> More generally, the pluggable-sysbus stuff is an awful hack
> that I wish we didn't have to have. I'm not sure I want to see
> us expanding the use of it to other device types...

I not really trying to trigger code when adding devices. I'm just trying 
to use the related per-machine allow-list as a way to prevent the user 
to add such devices (specifically cpu group/cluster) on any random 
machine which would most probably not "work".

Thanks,
Damien

On Thu, Mar 31, 2022 at 01:53:08PM +0200, Damien Hedde wrote:
> This flag will be used in device_add to check if
> the device needs special allowance from the machine
> model.
> 
> It will replace the current check based only on the
> device being a TYPE_SYB_BUS_DEVICE.
> 

Looks good to me!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> v2:
>  + change the flag name and put it just below user_creatable
> ---
>  include/hw/qdev-core.h | 9 +++++++++
>  hw/core/qdev.c         | 1 +
>  hw/core/sysbus.c       | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..6a040fcd3b 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -122,6 +122,15 @@ struct DeviceClass {
>       * TODO remove once we're there
>       */
>      bool user_creatable;
> +    /*
> +     * Some devices can be user created under certain conditions (eg:
> +     * specific machine support for sysbus devices), but it is
> +     * preferable to prevent global allowance for the reasons
> +     * described above.
> +     * This flag is an additional constraint over user_creatable:
> +     * user_creatable still needs to be set to true.
> +     */
> +    bool user_creatable_requires_machine_allowance;
>      bool hotpluggable;
>  
>      /* callbacks */
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 84f3019440..0844c85a21 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -833,6 +833,7 @@ static void device_class_init(ObjectClass *class, void *data)
>       */
>      dc->hotpluggable = true;
>      dc->user_creatable = true;
> +    dc->user_creatable_requires_machine_allowance = false;
>      vc->get_id = device_vmstate_if_get_id;
>      rc->get_state = device_get_reset_state;
>      rc->child_foreach = device_reset_child_foreach;
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 05c1da3d31..5f771ed1e9 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -325,6 +325,7 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
>       * subclass needs to override it and set user_creatable=true.
>       */
>      k->user_creatable = false;
> +    k->user_creatable_requires_machine_allowance = true;
>  }
>  
>  static const TypeInfo sysbus_device_type_info = {
> -- 
> 2.35.1
> 
>