[PATCH v2 0/5] Generalize the sysbus device machine allowance

[PATCH v2 0/5] Generalize the sysbus device machine allowance

[Damien Hedde]

Hi all,

This series transforms the TYPE_SYSBUS_DEVICE allowed list that exists
in machine class model into a TYPE_DEVICE allowed list.

This will allow to add non-sysbus device into this list to prevent
the user to create them on most machines.
Typical use case will be for example cpu related devices like
these developed in the following series:
https://lore.kernel.org/qemu-devel/20220330125639.201937-1-damien.hedde@greensocs.com/

Patches 1 and 3 lack a review.

Thanks,
--
Damien

v2:
 + update the flag name and put it just below user_creatable (Philippe)

Damien Hedde (5):
  qdev: add user_creatable_requires_machine_allowance class flag
  machine: update machine allowed list related functions/fields
  qdev-monitor: use the new user_creatable_requires_machine_allowance
  rename machine_class_allow_dynamic_sysbus_dev
  machine: remove temporary inline functions

 include/hw/boards.h         | 40 ++++++++++++++++++-------------------
 include/hw/qdev-core.h      |  9 +++++++++
 hw/arm/virt.c               | 10 +++++-----
 hw/core/machine.c           | 10 +++++-----
 hw/core/qdev.c              |  1 +
 hw/core/sysbus.c            |  1 +
 hw/i386/microvm.c           |  2 +-
 hw/i386/pc_piix.c           |  4 ++--
 hw/i386/pc_q35.c            |  8 ++++----
 hw/ppc/e500plat.c           |  2 +-
 hw/ppc/spapr.c              |  2 +-
 hw/riscv/virt.c             |  2 +-
 hw/xen/xen-legacy-backend.c |  2 +-
 softmmu/qdev-monitor.c      |  8 ++++----
 14 files changed, 56 insertions(+), 45 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] qdev: add user_creatable_requires_machine_allowance class flag

[Damien Hedde]

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

[PATCH v2 2/5] machine: update machine allowed list related functions/fields

[Damien Hedde]

The list will now accept any device (not only sysbus devices) so
we rename the related code and documentation.

Create some temporary inline functions with old names until
we've udpated callsites as well.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/boards.h | 50 +++++++++++++++++++++++++++------------------
 hw/core/machine.c   | 10 ++++-----
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index c92ac8815c..1814793175 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -38,35 +38,45 @@ void machine_parse_smp_config(MachineState *ms,
                               const SMPConfiguration *config, Error **errp);
 
 /**
- * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
+ * machine_class_allow_dynamic_device: Add type to list of valid devices
  * @mc: Machine class
- * @type: type to allow (should be a subtype of TYPE_SYS_BUS_DEVICE)
+ * @type: type to allow (should be a subtype of TYPE_DEVICE having the
+ *        uc_requires_machine_allowance flag)
  *
  * Add the QOM type @type to the list of devices of which are subtypes
- * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically
- * created (eg by the user on the command line with -device).
- * By default if the user tries to create any devices on the command line
- * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message;
- * for the special cases which are permitted for this machine model, the
- * machine model class init code must call this function to add them
- * to the list of specifically permitted devices.
+ * of TYPE_DEVICE but which are only permitted to be dynamically
+ * created (eg by the user on the command line with -device) if the
+ * machine allowed it.
+ *
+ * Otherwise if the user tries to create such a device on the command line,
+ * it will get an error message.
  */
-void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
+void machine_class_allow_dynamic_device(MachineClass *mc, const char *type);
+static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc,
+                                                          const char *type)
+{
+    machine_class_allow_dynamic_device(mc, type);
+}
 
 /**
- * device_type_is_dynamic_sysbus: Check if type is an allowed sysbus device
+ * device_type_is_dynamic_allowed: Check if type is an allowed device
  * type for the machine class.
  * @mc: Machine class
- * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
+ * @type: type to check (should be a subtype of TYPE_DEVICE)
  *
  * Returns: true if @type is a type in the machine's list of
- * dynamically pluggable sysbus devices; otherwise false.
+ * dynamically pluggable devices; otherwise false.
  *
- * Check if the QOM type @type is in the list of allowed sysbus device
- * types (see machine_class_allowed_dynamic_sysbus_dev()).
+ * Check if the QOM type @type is in the list of allowed device
+ * types (see machine_class_allowed_dynamic_device()).
  * Note that if @type has a parent type in the list, it is allowed too.
  */
-bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type);
+bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type);
+static inline bool device_type_is_dynamic_sysbus(MachineClass *mc,
+                                                 const char *type)
+{
+    return device_type_is_dynamic_allowed(mc, type);
+}
 
 /**
  * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
@@ -74,12 +84,12 @@ bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type);
  * @dev: device to check
  *
  * Returns: true if @dev is a sysbus device on the machine's list
- * of dynamically pluggable sysbus devices; otherwise false.
+ * of dynamically pluggable devices; otherwise false.
  *
  * This function checks whether @dev is a valid dynamic sysbus device,
  * by first confirming that it is a sysbus device and then checking it
- * against the list of permitted dynamic sysbus devices which has been
- * set up by the machine using machine_class_allow_dynamic_sysbus_dev().
+ * against the list of permitted dynamic devices which has been
+ * set up by the machine using machine_class_allow_dynamic_device().
  *
  * It is valid to call this with something that is not a subclass of
  * TYPE_SYS_BUS_DEVICE; the function will return false in this case.
@@ -263,7 +273,7 @@ struct MachineClass {
     bool ignore_memory_transaction_failures;
     int numa_mem_align_shift;
     const char **valid_cpu_types;
-    strList *allowed_dynamic_sysbus_devices;
+    strList *allowed_dynamic_devices;
     bool auto_enable_numa_with_memhp;
     bool auto_enable_numa_with_memdev;
     bool ignore_boot_device_suffixes;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index d856485cb4..fb1f7c8e5a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -545,9 +545,9 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
     nvdimms_state->persistence_string = g_strdup(value);
 }
 
-void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
+void machine_class_allow_dynamic_device(MachineClass *mc, const char *type)
 {
-    QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
+    QAPI_LIST_PREPEND(mc->allowed_dynamic_devices, g_strdup(type));
 }
 
 bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
@@ -558,16 +558,16 @@ bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
         return false;
     }
 
-    return device_type_is_dynamic_sysbus(mc, object_get_typename(obj));
+    return device_type_is_dynamic_allowed(mc, object_get_typename(obj));
 }
 
-bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type)
+bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type)
 {
     bool allowed = false;
     strList *wl;
     ObjectClass *klass = object_class_by_name(type);
 
-    for (wl = mc->allowed_dynamic_sysbus_devices;
+    for (wl = mc->allowed_dynamic_devices;
          !allowed && wl;
          wl = wl->next) {
         allowed |= !!object_class_dynamic_cast(klass, wl->value);
-- 
2.35.1

[PATCH v2 3/5] qdev-monitor: use the new user_creatable_requires_machine_allowance

[Damien Hedde]

Instead of checking if the device is a sysbus device, just check
the newly added flag in device class.

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

v2: update the flag name
---
 softmmu/qdev-monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 12fe60c467..77f468358d 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -258,12 +258,12 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
         return NULL;
     }
 
-    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
-        /* sysbus devices need to be allowed by the machine */
+    if (dc->user_creatable_requires_machine_allowance) {
+        /* some devices need to be allowed by the machine */
         MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
-        if (!device_type_is_dynamic_sysbus(mc, *driver)) {
+        if (!device_type_is_dynamic_allowed(mc, *driver)) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
-                       "a dynamic sysbus device type for the machine");
+                       "the device type is not allowed for this machine");
             return NULL;
         }
     }
-- 
2.35.1

[PATCH v2 4/5] rename machine_class_allow_dynamic_sysbus_dev

[Damien Hedde]

All callsite are updated to the new function name
"machine_class_allow_dynamic_device"

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/virt.c               | 10 +++++-----
 hw/i386/microvm.c           |  2 +-
 hw/i386/pc_piix.c           |  4 ++--
 hw/i386/pc_q35.c            |  8 ++++----
 hw/ppc/e500plat.c           |  2 +-
 hw/ppc/spapr.c              |  2 +-
 hw/riscv/virt.c             |  2 +-
 hw/xen/xen-legacy-backend.c |  2 +-
 8 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d2e5ecd234..1442b8840b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2829,12 +2829,12 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
      * configuration of the particular instance.
      */
     mc->max_cpus = 512;
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
+    machine_class_allow_dynamic_device(mc, TYPE_VFIO_CALXEDA_XGMAC);
+    machine_class_allow_dynamic_device(mc, TYPE_VFIO_AMD_XGBE);
+    machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_device(mc, TYPE_VFIO_PLATFORM);
 #ifdef CONFIG_TPM
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+    machine_class_allow_dynamic_device(mc, TYPE_TPM_TIS_SYSBUS);
 #endif
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 4b3b1dd262..4f8f423d31 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -756,7 +756,7 @@ static void microvm_class_init(ObjectClass *oc, void *data)
         MICROVM_MACHINE_AUTO_KERNEL_CMDLINE,
         "Set off to disable adding virtio-mmio devices to the kernel cmdline");
 
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE);
 }
 
 static const TypeInfo microvm_machine_info = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b72c03d0a6..27373cb16a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -411,8 +411,8 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->desc = "Standard PC (i440FX + PIIX, 1996)";
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
-    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
-    machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
+    machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE);
 }
 
 static void pc_i440fx_7_0_machine_options(MachineClass *m)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1780f79bc1..8221615fa4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -353,10 +353,10 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_display = "std";
     m->default_kernel_irqchip_split = false;
     m->no_floppy = 1;
-    machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
-    machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
-    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
-    machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
+    machine_class_allow_dynamic_device(m, TYPE_AMD_IOMMU_DEVICE);
+    machine_class_allow_dynamic_device(m, TYPE_INTEL_IOMMU_DEVICE);
+    machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE);
     m->max_cpus = 288;
 }
 
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index fc911bbb7b..273cde9d06 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -102,7 +102,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = 32;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
     mc->default_ram_id = "mpc8544ds.ram";
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
+    machine_class_allow_dynamic_device(mc, TYPE_ETSEC_COMMON);
  }
 
 static const TypeInfo e500plat_info = {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a4372ba189..70e12d9037 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4586,7 +4586,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_id = "ppc_spapr.ram";
     mc->default_display = "std";
     mc->kvm_type = spapr_kvm_type;
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
+    machine_class_allow_dynamic_device(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
     mc->pci_allow_0_address = true;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = spapr_get_hotplug_handler;
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index da50cbed43..b6e2b0051b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1513,7 +1513,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_supported = true;
     mc->default_ram_id = "riscv_virt_board.ram";
 
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE);
 
     object_class_property_add_bool(oc, "aclint", virt_get_aclint,
                                    virt_set_aclint);
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 085fd31ef7..7c81c4846a 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -722,7 +722,7 @@ static void xen_set_dynamic_sysbus(void)
     ObjectClass *oc = object_get_class(machine);
     MachineClass *mc = MACHINE_CLASS(oc);
 
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV);
+    machine_class_allow_dynamic_device(mc, TYPE_XENSYSDEV);
 }
 
 int xen_be_register(const char *type, struct XenDevOps *ops)
-- 
2.35.1

[PATCH v2 5/5] machine: remove temporary inline functions

[Damien Hedde]

Now we have renamed all calls to these old functions, we
can delete the temporary inline we've defined.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/boards.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 1814793175..7efba048e9 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -52,11 +52,6 @@ void machine_parse_smp_config(MachineState *ms,
  * it will get an error message.
  */
 void machine_class_allow_dynamic_device(MachineClass *mc, const char *type);
-static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc,
-                                                          const char *type)
-{
-    machine_class_allow_dynamic_device(mc, type);
-}
 
 /**
  * device_type_is_dynamic_allowed: Check if type is an allowed device
@@ -72,11 +67,6 @@ static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc,
  * Note that if @type has a parent type in the list, it is allowed too.
  */
 bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type);
-static inline bool device_type_is_dynamic_sysbus(MachineClass *mc,
-                                                 const char *type)
-{
-    return device_type_is_dynamic_allowed(mc, type);
-}
 
 /**
  * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
-- 
2.35.1

Re: [PATCH v2 1/5] qdev: add user_creatable_requires_machine_allowance class flag

[Edgar E. Iglesias]

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

Re: [PATCH v2 2/5] machine: update machine allowed list related functions/fields

[Edgar E. Iglesias]

On Thu, Mar 31, 2022 at 01:53:09PM +0200, Damien Hedde wrote:
> The list will now accept any device (not only sysbus devices) so
> we rename the related code and documentation.
> 
> Create some temporary inline functions with old names until
> we've udpated callsites as well.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> ---
>  include/hw/boards.h | 50 +++++++++++++++++++++++++++------------------
>  hw/core/machine.c   | 10 ++++-----
>  2 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index c92ac8815c..1814793175 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -38,35 +38,45 @@ void machine_parse_smp_config(MachineState *ms,
>                                const SMPConfiguration *config, Error **errp);
>  
>  /**
> - * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
> + * machine_class_allow_dynamic_device: Add type to list of valid devices
>   * @mc: Machine class
> - * @type: type to allow (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + * @type: type to allow (should be a subtype of TYPE_DEVICE having the
> + *        uc_requires_machine_allowance flag)
>   *
>   * Add the QOM type @type to the list of devices of which are subtypes
> - * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically
> - * created (eg by the user on the command line with -device).
> - * By default if the user tries to create any devices on the command line
> - * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message;
> - * for the special cases which are permitted for this machine model, the
> - * machine model class init code must call this function to add them
> - * to the list of specifically permitted devices.
> + * of TYPE_DEVICE but which are only permitted to be dynamically
> + * created (eg by the user on the command line with -device) if the
> + * machine allowed it.
> + *
> + * Otherwise if the user tries to create such a device on the command line,
> + * it will get an error message.
>   */
> -void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
> +void machine_class_allow_dynamic_device(MachineClass *mc, const char *type);
> +static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc,
> +                                                          const char *type)
> +{
> +    machine_class_allow_dynamic_device(mc, type);
> +}
>  
>  /**
> - * device_type_is_dynamic_sysbus: Check if type is an allowed sysbus device
> + * device_type_is_dynamic_allowed: Check if type is an allowed device
>   * type for the machine class.
>   * @mc: Machine class
> - * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + * @type: type to check (should be a subtype of TYPE_DEVICE)
>   *
>   * Returns: true if @type is a type in the machine's list of
> - * dynamically pluggable sysbus devices; otherwise false.
> + * dynamically pluggable devices; otherwise false.
>   *
> - * Check if the QOM type @type is in the list of allowed sysbus device
> - * types (see machine_class_allowed_dynamic_sysbus_dev()).
> + * Check if the QOM type @type is in the list of allowed device
> + * types (see machine_class_allowed_dynamic_device()).
>   * Note that if @type has a parent type in the list, it is allowed too.
>   */
> -bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type);
> +bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type);
> +static inline bool device_type_is_dynamic_sysbus(MachineClass *mc,
> +                                                 const char *type)
> +{
> +    return device_type_is_dynamic_allowed(mc, type);
> +}
>  
>  /**
>   * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
> @@ -74,12 +84,12 @@ bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type);
>   * @dev: device to check
>   *
>   * Returns: true if @dev is a sysbus device on the machine's list
> - * of dynamically pluggable sysbus devices; otherwise false.
> + * of dynamically pluggable devices; otherwise false.
>   *
>   * This function checks whether @dev is a valid dynamic sysbus device,
>   * by first confirming that it is a sysbus device and then checking it
> - * against the list of permitted dynamic sysbus devices which has been
> - * set up by the machine using machine_class_allow_dynamic_sysbus_dev().
> + * against the list of permitted dynamic devices which has been
> + * set up by the machine using machine_class_allow_dynamic_device().
>   *
>   * It is valid to call this with something that is not a subclass of
>   * TYPE_SYS_BUS_DEVICE; the function will return false in this case.
> @@ -263,7 +273,7 @@ struct MachineClass {
>      bool ignore_memory_transaction_failures;
>      int numa_mem_align_shift;
>      const char **valid_cpu_types;
> -    strList *allowed_dynamic_sysbus_devices;
> +    strList *allowed_dynamic_devices;
>      bool auto_enable_numa_with_memhp;
>      bool auto_enable_numa_with_memdev;
>      bool ignore_boot_device_suffixes;
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..fb1f7c8e5a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -545,9 +545,9 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
>      nvdimms_state->persistence_string = g_strdup(value);
>  }
>  
> -void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
> +void machine_class_allow_dynamic_device(MachineClass *mc, const char *type)
>  {
> -    QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
> +    QAPI_LIST_PREPEND(mc->allowed_dynamic_devices, g_strdup(type));
>  }
>  
>  bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
> @@ -558,16 +558,16 @@ bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>          return false;
>      }
>  
> -    return device_type_is_dynamic_sysbus(mc, object_get_typename(obj));
> +    return device_type_is_dynamic_allowed(mc, object_get_typename(obj));
>  }
>  
> -bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type)
> +bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type)
>  {
>      bool allowed = false;
>      strList *wl;
>      ObjectClass *klass = object_class_by_name(type);
>  
> -    for (wl = mc->allowed_dynamic_sysbus_devices;
> +    for (wl = mc->allowed_dynamic_devices;
>           !allowed && wl;
>           wl = wl->next) {
>          allowed |= !!object_class_dynamic_cast(klass, wl->value);
> -- 
> 2.35.1
> 
> 

Re: [PATCH v2 3/5] qdev-monitor: use the new user_creatable_requires_machine_allowance

[Edgar E. Iglesias]

On Thu, Mar 31, 2022 at 01:53:10PM +0200, Damien Hedde wrote:
> Instead of checking if the device is a sysbus device, just check
> the newly added flag in device class.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> ---
> 
> v2: update the flag name
> ---
>  softmmu/qdev-monitor.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 12fe60c467..77f468358d 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -258,12 +258,12 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>          return NULL;
>      }
>  
> -    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
> -        /* sysbus devices need to be allowed by the machine */
> +    if (dc->user_creatable_requires_machine_allowance) {
> +        /* some devices need to be allowed by the machine */
>          MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
> -        if (!device_type_is_dynamic_sysbus(mc, *driver)) {
> +        if (!device_type_is_dynamic_allowed(mc, *driver)) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> -                       "a dynamic sysbus device type for the machine");
> +                       "the device type is not allowed for this machine");
>              return NULL;
>          }
>      }
> -- 
> 2.35.1
> 
> 

Re: [PATCH v2 4/5] rename machine_class_allow_dynamic_sysbus_dev

[Edgar E. Iglesias]

On Thu, Mar 31, 2022 at 01:53:11PM +0200, Damien Hedde wrote:
> All callsite are updated to the new function name
> "machine_class_allow_dynamic_device"
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> ---
>  hw/arm/virt.c               | 10 +++++-----
>  hw/i386/microvm.c           |  2 +-
>  hw/i386/pc_piix.c           |  4 ++--
>  hw/i386/pc_q35.c            |  8 ++++----
>  hw/ppc/e500plat.c           |  2 +-
>  hw/ppc/spapr.c              |  2 +-
>  hw/riscv/virt.c             |  2 +-
>  hw/xen/xen-legacy-backend.c |  2 +-
>  8 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..1442b8840b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2829,12 +2829,12 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>       * configuration of the particular instance.
>       */
>      mc->max_cpus = 512;
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
> +    machine_class_allow_dynamic_device(mc, TYPE_VFIO_CALXEDA_XGMAC);
> +    machine_class_allow_dynamic_device(mc, TYPE_VFIO_AMD_XGBE);
> +    machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE);
> +    machine_class_allow_dynamic_device(mc, TYPE_VFIO_PLATFORM);
>  #ifdef CONFIG_TPM
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
> +    machine_class_allow_dynamic_device(mc, TYPE_TPM_TIS_SYSBUS);
>  #endif
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 4b3b1dd262..4f8f423d31 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -756,7 +756,7 @@ static void microvm_class_init(ObjectClass *oc, void *data)
>          MICROVM_MACHINE_AUTO_KERNEL_CMDLINE,
>          "Set off to disable adding virtio-mmio devices to the kernel cmdline");
>  
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> +    machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE);
>  }
>  
>  static const TypeInfo microvm_machine_info = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b72c03d0a6..27373cb16a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -411,8 +411,8 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->desc = "Standard PC (i440FX + PIIX, 1996)";
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
> -    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
> -    machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
> +    machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE);
> +    machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE);
>  }
>  
>  static void pc_i440fx_7_0_machine_options(MachineClass *m)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1780f79bc1..8221615fa4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -353,10 +353,10 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->default_display = "std";
>      m->default_kernel_irqchip_split = false;
>      m->no_floppy = 1;
> -    machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
> -    machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
> -    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
> -    machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
> +    machine_class_allow_dynamic_device(m, TYPE_AMD_IOMMU_DEVICE);
> +    machine_class_allow_dynamic_device(m, TYPE_INTEL_IOMMU_DEVICE);
> +    machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE);
> +    machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE);
>      m->max_cpus = 288;
>  }
>  
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index fc911bbb7b..273cde9d06 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -102,7 +102,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = 32;
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>      mc->default_ram_id = "mpc8544ds.ram";
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
> +    machine_class_allow_dynamic_device(mc, TYPE_ETSEC_COMMON);
>   }
>  
>  static const TypeInfo e500plat_info = {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a4372ba189..70e12d9037 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4586,7 +4586,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_id = "ppc_spapr.ram";
>      mc->default_display = "std";
>      mc->kvm_type = spapr_kvm_type;
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
> +    machine_class_allow_dynamic_device(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
>      mc->pci_allow_0_address = true;
>      assert(!mc->get_hotplug_handler);
>      mc->get_hotplug_handler = spapr_get_hotplug_handler;
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index da50cbed43..b6e2b0051b 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1513,7 +1513,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      mc->numa_mem_supported = true;
>      mc->default_ram_id = "riscv_virt_board.ram";
>  
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> +    machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE);
>  
>      object_class_property_add_bool(oc, "aclint", virt_get_aclint,
>                                     virt_set_aclint);
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 085fd31ef7..7c81c4846a 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -722,7 +722,7 @@ static void xen_set_dynamic_sysbus(void)
>      ObjectClass *oc = object_get_class(machine);
>      MachineClass *mc = MACHINE_CLASS(oc);
>  
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV);
> +    machine_class_allow_dynamic_device(mc, TYPE_XENSYSDEV);
>  }
>  
>  int xen_be_register(const char *type, struct XenDevOps *ops)
> -- 
> 2.35.1
> 
> 

Re: [PATCH v2 5/5] machine: remove temporary inline functions

[Edgar E. Iglesias]

On Thu, Mar 31, 2022 at 01:53:12PM +0200, Damien Hedde wrote:
> Now we have renamed all calls to these old functions, we
> can delete the temporary inline we've defined.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> ---
>  include/hw/boards.h | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1814793175..7efba048e9 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -52,11 +52,6 @@ void machine_parse_smp_config(MachineState *ms,
>   * it will get an error message.
>   */
>  void machine_class_allow_dynamic_device(MachineClass *mc, const char *type);
> -static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc,
> -                                                          const char *type)
> -{
> -    machine_class_allow_dynamic_device(mc, type);
> -}
>  
>  /**
>   * device_type_is_dynamic_allowed: Check if type is an allowed device
> @@ -72,11 +67,6 @@ static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc,
>   * Note that if @type has a parent type in the list, it is allowed too.
>   */
>  bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type);
> -static inline bool device_type_is_dynamic_sysbus(MachineClass *mc,
> -                                                 const char *type)
> -{
> -    return device_type_is_dynamic_allowed(mc, type);
> -}
>  
>  /**
>   * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
> -- 
> 2.35.1
> 
> 

Re: [PATCH v2 1/5] qdev: add user_creatable_requires_machine_allowance class flag

[Peter Maydell]

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

Re: [PATCH v2 1/5] qdev: add user_creatable_requires_machine_allowance class flag

[Damien Hedde]

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