[PATCH 0/2] qdev,accel-system: allow Accel type globals

[PATCH 0/2] qdev,accel-system: allow Accel type globals

[Daniel Henrique Barboza]

Hi,

This is another approach of the problem we tried to fix with [1]. It was
suggested by Paolo during the review.

In the current handling of '-accel' only the first instance is parsed.
All other instances (aside from a 'helper' command that triggers the
help text and exits) is ignored. So this command line:

qemu-system-riscv64 -accel kvm -accel kvm,riscv-aia=hwaccel

Won't change 'riscv-aia' to 'hwaccel'. 

This is affecting at least one use case we have with libvirt and RISC-V:
we can't set 'riscv-aia' by appending '-accel kvm,riscv-aia=val'
via <qemu:cmdline> in the domain XML. libvirt will add a leading
'-accel kvm' in the regular command line and ignore the second. We'll
add official libvirt support for this KVM property in the near future
(we're still discussing if this prop should be a bool instead), but
for now a QEMU side change would unlock this RISC-V KVM use case in
libvirt.

With this series we'll be able to set 'riscv-aia' using globals as
follows:

qemu-system-riscv64 -accel kvm -global kvm-accel.riscv-aia=hwaccel

This change benefit all archs (not just RISC-V) and will allow globals
to be used with all accelerators, not just KVM.

[1] https://lore.kernel.org/qemu-devel/20240701133038.1489043-1-dbarboza@ventanamicro.com/

Daniel Henrique Barboza (2):
  qdev: change qdev_prop_set_globals() to use Object*
  qdev, accel-system: add support to Accel globals

 accel/accel-system.c         |  3 +++
 hw/core/qdev-properties.c    | 39 +++++++++++++++++++++++-------------
 hw/core/qdev.c               |  2 +-
 include/hw/qdev-properties.h |  2 +-
 4 files changed, 30 insertions(+), 16 deletions(-)

-- 
2.45.2

[PATCH 1/2] qdev: change qdev_prop_set_globals() to use Object*

[Daniel Henrique Barboza]

Next patch will add Accel globals support and will use
qdev_prop_set_globals().

At this moment this function is hardwired to be used with DeviceState.
dev->hotplugged is checked to determine if object_apply_global_props()
will receive a NULL or an &error_fatal errp.

Change qdev_prop_set_globals() to receive an Object and an errp. The
logic using dev->hotplugged is moved to the caller, device_post_init().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/core/qdev-properties.c    | 5 ++---
 hw/core/qdev.c               | 2 +-
 include/hw/qdev-properties.h | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 86a583574d..4867d7dd9e 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -920,10 +920,9 @@ int qdev_prop_check_globals(void)
     return ret;
 }
 
-void qdev_prop_set_globals(DeviceState *dev)
+void qdev_prop_set_globals(Object *obj, Error **errp)
 {
-    object_apply_global_props(OBJECT(dev), global_props(),
-                              dev->hotplugged ? NULL : &error_fatal);
+    object_apply_global_props(obj, global_props(), errp);
 }
 
 /* --- 64bit unsigned int 'size' type --- */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f3a996f57d..923f9ca74c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
      * precedence.
      */
     object_apply_compat_props(obj);
-    qdev_prop_set_globals(DEVICE(obj));
+    qdev_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 09aa04ca1e..a1737bf4d8 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -210,7 +210,7 @@ void qdev_prop_register_global(GlobalProperty *prop);
 const GlobalProperty *qdev_find_global_prop(Object *obj,
                                             const char *name);
 int qdev_prop_check_globals(void);
-void qdev_prop_set_globals(DeviceState *dev);
+void qdev_prop_set_globals(Object *obj, Error **errp);
 void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
                                     const char *name, const char *value);
 
-- 
2.45.2

[PATCH 2/2] qdev, accel-system: add support to Accel globals

[Daniel Henrique Barboza]

We're not honouring KVM options that are provided by any -accel option
aside from the first. In this example:

qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \
    -accel kvm,riscv-aia=hwaccel

'riscv-aia' will be set to 'emul', ignoring the last occurrence of the
option that set 'riscv-aia' to 'hwaccel'.

A failed attempt to solve this issue by setting 'merge_lists' can be
found in [1]. A different approach was suggested in [2]: enable global
properties for accelerators. This allows an user to override any accel
setting by using '-global' and we won't need to change how '-accel' opts
are handled.

This is done by calling qdev_prop_set_globals() in accel_init_machine().
As done in device_post_init(), user's global props take precedence over
compat props so qdev_prop_set_globals() is called after
object_set_accelerator_compat_props().

qdev_prop_check_globals() is then changed to recognize TYPE_ACCEL
globals as valid.

Re-using the fore-mentioned example we'll be able to set
riscv-aia=hwaccel by doing the following:

qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \
    -global kvm-accel.riscv-aia=hwaccel

[1] https://lore.kernel.org/qemu-devel/20240701133038.1489043-1-dbarboza@ventanamicro.com/
[2] https://lore.kernel.org/qemu-devel/CABgObfbVJkCdpHV2uA7LGTbYEaoaizrbeG0vNo_T1ua5b=jq7Q@mail.gmail.com/

Reported-by: Andrew Jones <ajones@ventanamicro.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 accel/accel-system.c      |  3 +++
 hw/core/qdev-properties.c | 34 +++++++++++++++++++++++-----------
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/accel/accel-system.c b/accel/accel-system.c
index f6c947dd82..40c73ec62f 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -29,6 +29,8 @@
 #include "sysemu/cpus.h"
 #include "qemu/error-report.h"
 #include "accel-system.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
 
 int accel_init_machine(AccelState *accel, MachineState *ms)
 {
@@ -43,6 +45,7 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
         object_unref(OBJECT(accel));
     } else {
         object_set_accelerator_compat_props(acc->compat_props);
+        qdev_prop_set_globals(OBJECT(accel), &error_fatal);
     }
     return ret;
 }
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 4867d7dd9e..462a8f96e0 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -10,6 +10,7 @@
 #include "qemu/cutils.h"
 #include "qdev-prop-internal.h"
 #include "qom/qom-qobject.h"
+#include "qemu/accel.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -894,27 +895,38 @@ int qdev_prop_check_globals(void)
 
     for (i = 0; i < global_props()->len; i++) {
         GlobalProperty *prop;
-        ObjectClass *oc;
+        ObjectClass *globalc, *oc;
         DeviceClass *dc;
 
         prop = g_ptr_array_index(global_props(), i);
         if (prop->used) {
             continue;
         }
-        oc = object_class_by_name(prop->driver);
-        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
-        if (!oc) {
-            warn_report("global %s.%s has invalid class name",
-                        prop->driver, prop->property);
-            ret = 1;
+        globalc = object_class_by_name(prop->driver);
+        oc = object_class_dynamic_cast(globalc, TYPE_DEVICE);
+        if (oc) {
+            dc = DEVICE_CLASS(oc);
+            if (!dc->hotpluggable && !prop->used) {
+                warn_report("global %s.%s=%s not used",
+                            prop->driver, prop->property, prop->value);
+                ret = 1;
+            }
             continue;
         }
-        dc = DEVICE_CLASS(oc);
-        if (!dc->hotpluggable && !prop->used) {
+
+        /*
+         * At this point is either an accelerator global that is
+         * unused or an invalid global. Both set ret = 1.
+         */
+        ret = 1;
+
+        oc = object_class_dynamic_cast(globalc, TYPE_ACCEL);
+        if (oc) {
             warn_report("global %s.%s=%s not used",
                         prop->driver, prop->property, prop->value);
-            ret = 1;
-            continue;
+        } else {
+            warn_report("global %s.%s has invalid class name",
+                        prop->driver, prop->property);
         }
     }
     return ret;
-- 
2.45.2

Re: [PATCH 1/2] qdev: change qdev_prop_set_globals() to use Object*

[Daniel P. Berrangé]

On Wed, Jul 03, 2024 at 06:46:25AM -0300, Daniel Henrique Barboza wrote:
> Next patch will add Accel globals support and will use
> qdev_prop_set_globals().
> 
> At this moment this function is hardwired to be used with DeviceState.
> dev->hotplugged is checked to determine if object_apply_global_props()
> will receive a NULL or an &error_fatal errp.
> 
> Change qdev_prop_set_globals() to receive an Object and an errp. The
> logic using dev->hotplugged is moved to the caller, device_post_init().
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/core/qdev-properties.c    | 5 ++---
>  hw/core/qdev.c               | 2 +-
>  include/hw/qdev-properties.h | 2 +-
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 86a583574d..4867d7dd9e 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -920,10 +920,9 @@ int qdev_prop_check_globals(void)
>      return ret;
>  }
>  
> -void qdev_prop_set_globals(DeviceState *dev)
> +void qdev_prop_set_globals(Object *obj, Error **errp)
>  {
> -    object_apply_global_props(OBJECT(dev), global_props(),
> -                              dev->hotplugged ? NULL : &error_fatal);
> +    object_apply_global_props(obj, global_props(), errp);
>  }

If you're going to make this work on Object, instead fo merely
DeviceState, then all this globals functionality should be
moved into qom/object.c, and the methods renamed, as it is no
longer qdev logic.

>  
>  /* --- 64bit unsigned int 'size' type --- */
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f3a996f57d..923f9ca74c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
>       * precedence.
>       */
>      object_apply_compat_props(obj);
> -    qdev_prop_set_globals(DEVICE(obj));
> +    qdev_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
>  }
>  
>  /* Unlink device from bus and free the structure.  */
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 09aa04ca1e..a1737bf4d8 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -210,7 +210,7 @@ void qdev_prop_register_global(GlobalProperty *prop);
>  const GlobalProperty *qdev_find_global_prop(Object *obj,
>                                              const char *name);
>  int qdev_prop_check_globals(void);
> -void qdev_prop_set_globals(DeviceState *dev);
> +void qdev_prop_set_globals(Object *obj, Error **errp);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
>                                      const char *name, const char *value);
>  
> -- 
> 2.45.2
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|