[PATCH v3 0/3] Dynamic sysbus device check error report

[PATCH v3 0/3] Dynamic sysbus device check error report

[Damien Hedde]

Hi,

Dynamic sysbus devices are allowed by a per-machine basis.
Right now, the allowance check is done during an machine_init_done
notifier, well after such devices are created.

This series move the check at the right place (during the handling
of a QMP device_add command or -device CLI option) so that we can
report the error right away.

This was initially part of my RFC (hence the v3) about allowing to
create devices during the machine initialized phase (link is below).
But it seems to me these patches make sense already as a standalone
cleanup.

Only patch 1 miss a review.

Thanks,
Damien

v3:
 + standalone series
 + minor tweaks

v2 was part of:
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05683.html

Damien Hedde (3):
  machine: add device_type_is_dynamic_sysbus function
  qdev-monitor: Check sysbus device type before creating it
  machine: remove the done notifier for dynamic sysbus device type check

 include/hw/boards.h    | 16 +++++++++++++++-
 hw/core/machine.c      | 40 ++++++++++------------------------------
 softmmu/qdev-monitor.c | 11 +++++++++++
 3 files changed, 36 insertions(+), 31 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] machine: add device_type_is_dynamic_sysbus function

[Damien Hedde]

Right now the allowance check for adding a sysbus device using
-device cli option (or device_add qmp command) is done well after
the device has been created. It is done during the machine init done
notifier: machine_init_notify() in hw/core/machine.c

This new function will allow us to do the check at the right time and
issue an error if it fails.

Also make device_is_dynamic_sysbus() use the new function.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Cc: Ani Sinha <ani@anisinha.ca>

v3: change the function name (Ani)
---
 include/hw/boards.h | 15 +++++++++++++++
 hw/core/machine.c   | 13 ++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 5adbcbb99b..85adf660c1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -51,6 +51,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
  */
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
 
+/**
+ * device_type_is_dynamic_sysbus: Check if type is an allowed sysbus device
+ * type for the machine class.
+ * @mc: Machine class
+ * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
+ *
+ * Returns: true if @type is a type in the machine's list of
+ * dynamically pluggable sysbus 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()).
+ * 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);
+
 /**
  * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
  * @mc: Machine class
diff --git a/hw/core/machine.c b/hw/core/machine.c
index b8d95eec32..0d20104796 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -548,18 +548,25 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
 
 bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
 {
-    bool allowed = false;
-    strList *wl;
     Object *obj = OBJECT(dev);
 
     if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
         return false;
     }
 
+    return device_type_is_dynamic_sysbus(mc, object_get_typename(obj));
+}
+
+bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type)
+{
+    bool allowed = false;
+    strList *wl;
+    ObjectClass *klass = object_class_by_name(type);
+
     for (wl = mc->allowed_dynamic_sysbus_devices;
          !allowed && wl;
          wl = wl->next) {
-        allowed |= !!object_dynamic_cast(obj, wl->value);
+        allowed |= !!object_class_dynamic_cast(klass, wl->value);
     }
 
     return allowed;
-- 
2.33.0

Re: [PATCH v3 1/3] machine: add device_type_is_dynamic_sysbus function

[Philippe Mathieu-Daudé]

On 10/29/21 16:22, Damien Hedde wrote:
> Right now the allowance check for adding a sysbus device using
> -device cli option (or device_add qmp command) is done well after
> the device has been created. It is done during the machine init done
> notifier: machine_init_notify() in hw/core/machine.c
> 
> This new function will allow us to do the check at the right time and
> issue an error if it fails.
> 
> Also make device_is_dynamic_sysbus() use the new function.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Ani Sinha <ani@anisinha.ca>
> 
> v3: change the function name (Ani)
> ---
>  include/hw/boards.h | 15 +++++++++++++++
>  hw/core/machine.c   | 13 ++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 1/3] machine: add device_type_is_dynamic_sysbus function

[Alistair Francis]

On Sat, Oct 30, 2021 at 12:26 AM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
> Right now the allowance check for adding a sysbus device using
> -device cli option (or device_add qmp command) is done well after
> the device has been created. It is done during the machine init done
> notifier: machine_init_notify() in hw/core/machine.c
>
> This new function will allow us to do the check at the right time and
> issue an error if it fails.
>
> Also make device_is_dynamic_sysbus() use the new function.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: Ani Sinha <ani@anisinha.ca>
>
> v3: change the function name (Ani)
> ---
>  include/hw/boards.h | 15 +++++++++++++++
>  hw/core/machine.c   | 13 ++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 5adbcbb99b..85adf660c1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -51,6 +51,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
>   */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>
> +/**
> + * device_type_is_dynamic_sysbus: Check if type is an allowed sysbus device
> + * type for the machine class.
> + * @mc: Machine class
> + * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + *
> + * Returns: true if @type is a type in the machine's list of
> + * dynamically pluggable sysbus 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()).
> + * 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);
> +
>  /**
>   * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
>   * @mc: Machine class
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index b8d95eec32..0d20104796 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -548,18 +548,25 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>
>  bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>  {
> -    bool allowed = false;
> -    strList *wl;
>      Object *obj = OBJECT(dev);
>
>      if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
>          return false;
>      }
>
> +    return device_type_is_dynamic_sysbus(mc, object_get_typename(obj));
> +}
> +
> +bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type)
> +{
> +    bool allowed = false;
> +    strList *wl;
> +    ObjectClass *klass = object_class_by_name(type);
> +
>      for (wl = mc->allowed_dynamic_sysbus_devices;
>           !allowed && wl;
>           wl = wl->next) {
> -        allowed |= !!object_dynamic_cast(obj, wl->value);
> +        allowed |= !!object_class_dynamic_cast(klass, wl->value);
>      }
>
>      return allowed;
> --
> 2.33.0
>
>

[PATCH v3 2/3] qdev-monitor: Check sysbus device type before creating it

[Damien Hedde]

Add an early check to test if the requested sysbus device type
is allowed by the current machine before creating the device. This
impacts both -device cli option and device_add qmp command.

Before this patch, the check was done well after the device has
been created (in a machine init done notifier). We can now report
the error right away.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---

v3: update error message
---
 softmmu/qdev-monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 4851de51a5..e49d9773d2 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -42,6 +42,7 @@
 #include "qemu/cutils.h"
 #include "hw/qdev-properties.h"
 #include "hw/clock.h"
+#include "hw/boards.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -254,6 +255,16 @@ 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 */
+        MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
+        if (!device_type_is_dynamic_sysbus(mc, *driver)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
+                       "a dynamic sysbus device type for the machine");
+            return NULL;
+        }
+    }
+
     return dc;
 }
 
-- 
2.33.0

Re: [PATCH v3 2/3] qdev-monitor: Check sysbus device type before creating it

[Philippe Mathieu-Daudé]

On 10/29/21 16:22, Damien Hedde wrote:
> Add an early check to test if the requested sysbus device type
> is allowed by the current machine before creating the device. This
> impacts both -device cli option and device_add qmp command.
> 
> Before this patch, the check was done well after the device has
> been created (in a machine init done notifier). We can now report
> the error right away.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> 
> v3: update error message
> ---
>  softmmu/qdev-monitor.c | 11 +++++++++++

/me wonders why this file is named '-monitor'.

> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 4851de51a5..e49d9773d2 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -42,6 +42,7 @@
>  #include "qemu/cutils.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/clock.h"
> +#include "hw/boards.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -254,6 +255,16 @@ 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 */
> +        MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
> +        if (!device_type_is_dynamic_sysbus(mc, *driver)) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",

Per include/qapi/qmp/qerror.h:

  /*
   * These macros will go away, please don't use in new code, and do not
   * add new ones!
   */

  #define QERR_INVALID_PARAMETER_VALUE \
      "Parameter '%s' expects %s"

> +                       "a dynamic sysbus device type for the machine");

Besides, this is easier to read:

            error_setg(errp, "Parameter 'driver' expects a dynamic"
                             " sysbus device type for the machine");

Maybe remove QERR_INVALID_PARAMETER_VALUE from qdev_get_device_class()
in a preliminary patch?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +            return NULL;
> +        }
> +    }
> +
>      return dc;
>  }
>  
> 

Re: [PATCH v3 2/3] qdev-monitor: Check sysbus device type before creating it

[Philippe Mathieu-Daudé]

On 10/29/21 19:34, Philippe Mathieu-Daudé wrote:
> On 10/29/21 16:22, Damien Hedde wrote:
>> Add an early check to test if the requested sysbus device type
>> is allowed by the current machine before creating the device. This
>> impacts both -device cli option and device_add qmp command.
>>
>> Before this patch, the check was done well after the device has
>> been created (in a machine init done notifier). 

Before / Until? Also you could mention "in a machine init_done
notifier, which we will remove in the next commit".

>> We can now report
>> the error right away.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>
>> v3: update error message
>> ---
>>  softmmu/qdev-monitor.c | 11 +++++++++++
> 
> /me wonders why this file is named '-monitor'.
> 
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 4851de51a5..e49d9773d2 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -42,6 +42,7 @@
>>  #include "qemu/cutils.h"
>>  #include "hw/qdev-properties.h"
>>  #include "hw/clock.h"
>> +#include "hw/boards.h"
>>  
>>  /*
>>   * Aliases were a bad idea from the start.  Let's keep them
>> @@ -254,6 +255,16 @@ 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 */
>> +        MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
>> +        if (!device_type_is_dynamic_sysbus(mc, *driver)) {
>> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> 
> Per include/qapi/qmp/qerror.h:
> 
>   /*
>    * These macros will go away, please don't use in new code, and do not
>    * add new ones!
>    */
> 
>   #define QERR_INVALID_PARAMETER_VALUE \
>       "Parameter '%s' expects %s"
> 
>> +                       "a dynamic sysbus device type for the machine");
> 
> Besides, this is easier to read:
> 
>             error_setg(errp, "Parameter 'driver' expects a dynamic"
>                              " sysbus device type for the machine");
> 
> Maybe remove QERR_INVALID_PARAMETER_VALUE from qdev_get_device_class()
> in a preliminary patch?
> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> +            return NULL;
>> +        }
>> +    }
>> +
>>      return dc;
>>  }
>>  
>>
> 

[PATCH v3 3/3] machine: remove the done notifier for dynamic sysbus device type check

[Damien Hedde]

Now that we check sysbus device types during device creation, we
can remove the check in the machine init done notifier.
This was the only thing done by this notifier, so we remove the
whole sysbus_notifier structure of the MachineState.

Note: This notifier was checking all /peripheral and /peripheral-anon
sysbus devices. Now we only check those added by -device cli option or
device_add qmp command when handling the command/option. So if there
are some devices added in one of these containers manually (eg in
machine C code), these will not be checked anymore.
This use case does not seem to appear apart from
hw/xen/xen-legacy-backend.c (it uses qdev_set_id() and in this case,
not for a sysbus device, so it's ok).

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---

v3: clarify the commit title
---
 include/hw/boards.h |  1 -
 hw/core/machine.c   | 27 ---------------------------
 2 files changed, 28 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 85adf660c1..680aa59d4a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -316,7 +316,6 @@ typedef struct CpuTopology {
 struct MachineState {
     /*< private >*/
     Object parent_obj;
-    Notifier sysbus_notifier;
 
     /*< public >*/
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0d20104796..2c17418628 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -572,18 +572,6 @@ bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type)
     return allowed;
 }
 
-static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
-{
-    MachineState *machine = opaque;
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
-
-    if (!device_is_dynamic_sysbus(mc, DEVICE(sbdev))) {
-        error_report("Option '-device %s' cannot be handled by this machine",
-                     object_class_get_name(object_get_class(OBJECT(sbdev))));
-        exit(1);
-    }
-}
-
 static char *machine_get_memdev(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -599,17 +587,6 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp)
     ms->ram_memdev_id = g_strdup(value);
 }
 
-static void machine_init_notify(Notifier *notifier, void *data)
-{
-    MachineState *machine = MACHINE(qdev_get_machine());
-
-    /*
-     * Loop through all dynamically created sysbus devices and check if they are
-     * all allowed.  If a device is not allowed, error out.
-     */
-    foreach_dynamic_sysbus_device(validate_sysbus_device, machine);
-}
-
 HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
 {
     int i;
@@ -1108,10 +1085,6 @@ static void machine_initfn(Object *obj)
                                         "Table (HMAT)");
     }
 
-    /* Register notifier when init is done for sysbus sanity checks */
-    ms->sysbus_notifier.notify = machine_init_notify;
-    qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
-
     /* default to mc->default_cpus */
     ms->smp.cpus = mc->default_cpus;
     ms->smp.max_cpus = mc->default_cpus;
-- 
2.33.0

Re: [PATCH v3 3/3] machine: remove the done notifier for dynamic sysbus device type check

[Philippe Mathieu-Daudé]

On 10/29/21 16:22, Damien Hedde wrote:
> Now that we check sysbus device types during device creation, we
> can remove the check in the machine init done notifier.
> This was the only thing done by this notifier, so we remove the
> whole sysbus_notifier structure of the MachineState.
> 
> Note: This notifier was checking all /peripheral and /peripheral-anon
> sysbus devices. Now we only check those added by -device cli option or
> device_add qmp command when handling the command/option. So if there
> are some devices added in one of these containers manually (eg in
> machine C code), these will not be checked anymore.
> This use case does not seem to appear apart from
> hw/xen/xen-legacy-backend.c (it uses qdev_set_id() and in this case,
> not for a sysbus device, so it's ok).
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> 
> v3: clarify the commit title
> ---
>  include/hw/boards.h |  1 -
>  hw/core/machine.c   | 27 ---------------------------
>  2 files changed, 28 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 0/3] Dynamic sysbus device check error report

[Philippe Mathieu-Daudé]

On 10/29/21 16:22, Damien Hedde wrote:
> Hi,
> 
> Dynamic sysbus devices are allowed by a per-machine basis.
> Right now, the allowance check is done during an machine_init_done
> notifier, well after such devices are created.
> 
> This series move the check at the right place (during the handling
> of a QMP device_add command or -device CLI option) so that we can
> report the error right away.
> 
> This was initially part of my RFC (hence the v3) about allowing to
> create devices during the machine initialized phase (link is below).
> But it seems to me these patches make sense already as a standalone
> cleanup.

LGTM but I'd like an Ack from Eduardo or Marcel.

Re: [PATCH v3 0/3] Dynamic sysbus device check error report

[Eduardo Habkost]

On Fri, Oct 29, 2021 at 04:22:55PM +0200, Damien Hedde wrote:
> Hi,
> 
> Dynamic sysbus devices are allowed by a per-machine basis.
> Right now, the allowance check is done during an machine_init_done
> notifier, well after such devices are created.
> 
> This series move the check at the right place (during the handling
> of a QMP device_add command or -device CLI option) so that we can
> report the error right away.
> 
> This was initially part of my RFC (hence the v3) about allowing to
> create devices during the machine initialized phase (link is below).
> But it seems to me these patches make sense already as a standalone
> cleanup.
> 
> Only patch 1 miss a review.
> 
> Thanks,
> Damien
> 
> v3:
>  + standalone series
>  + minor tweaks
> 
> v2 was part of:
> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05683.html

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH v3 0/3] Dynamic sysbus device check error report

[Philippe Mathieu-Daudé]

On 10/29/21 16:22, Damien Hedde wrote:
> Hi,
> 
> Dynamic sysbus devices are allowed by a per-machine basis.
> Right now, the allowance check is done during an machine_init_done
> notifier, well after such devices are created.
> 
> This series move the check at the right place (during the handling
> of a QMP device_add command or -device CLI option) so that we can
> report the error right away.

> Damien Hedde (3):
>   machine: add device_type_is_dynamic_sysbus function
>   qdev-monitor: Check sysbus device type before creating it
>   machine: remove the done notifier for dynamic sysbus device type check

Thanks, series queued to machine-next.