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

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

[Daniel Henrique Barboza]

Hi,

In this new version we followed Daniel's suggestion in the v1 review [1]
and moved all 'globals' related functions and definitions from qdev to
object. Functions were renamed as follows:

- qdev_prop_register_global() is now object_prop_register_global()
- qdev_find_global_prop() is now object_find_global_prop()
- qdev_prop_check_globals() is now object_prop_check_globals()
- qdev_prop_set_globals() is now object_prop_set_globals()

The API change that were done in patch 1 is now being done on
object_prop_set_globals().

It crossed my mind to rename test-qdev-global-props.c to
test-object-global-props.c, but since the file is dealing exclusively
with qdev related tests I think the rename was unneeded.

No other changes made.

Changes from v1:
- patch 1:
  - move all globals related function and declarations from qdev files
    to object
- v1 review: https://lore.kernel.org/qemu-devel/20240703094626.1704990-1-dbarboza@ventanamicro.com/

[1] https://lore.kernel.org/qemu-devel/ZoUwMr1X170aQNDS@redhat.com/ 

Daniel Henrique Barboza (2):
  qom/object, qdev: move globals functions to object.c
  qom/object, accel-system: add support to Accel globals

 accel/accel-system.c                |  2 +
 hw/core/cpu-common.c                |  2 +-
 hw/core/qdev-properties-system.c    |  2 +-
 hw/core/qdev-properties.c           | 71 -------------------------
 hw/core/qdev.c                      |  2 +-
 include/hw/qdev-core.h              | 27 ----------
 include/hw/qdev-properties.h        |  5 --
 include/qom/object.h                | 34 ++++++++++++
 qom/object.c                        | 82 +++++++++++++++++++++++++++++
 system/vl.c                         |  6 +--
 target/i386/cpu.c                   |  2 +-
 target/sparc/cpu.c                  |  2 +-
 tests/unit/test-qdev-global-props.c |  4 +-
 13 files changed, 128 insertions(+), 113 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] qom/object, qdev: move globals functions to object.c

[Daniel Henrique Barboza]

Next patch will add Accel globals support. This means that globals won't be
qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.

Move all globals related functions and declarations to object.c. Each
function is renamed from 'qdev_' to 'object_':

- qdev_prop_register_global() is now object_prop_register_global()
- qdev_find_global_prop() is now object_find_global_prop()
- qdev_prop_check_globals() is now object_prop_check_globals()
- qdev_prop_set_globals() is now object_prop_set_globals()

For object_prop_set_globals() an additional change was made: the function
was hardwired to be used with DeviceState, where dev->hotplugged is checked
to determine if object_apply_global_props() will receive a NULL or an
&error_fatal errp. The function now receives an Object and an errp, and
logic using dev->hotplugged is moved to its caller (device_post_init()).

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/core/cpu-common.c                |  2 +-
 hw/core/qdev-properties-system.c    |  2 +-
 hw/core/qdev-properties.c           | 71 -----------------------------
 hw/core/qdev.c                      |  2 +-
 include/hw/qdev-core.h              | 27 -----------
 include/hw/qdev-properties.h        |  5 --
 include/qom/object.h                | 34 ++++++++++++++
 qom/object.c                        | 70 ++++++++++++++++++++++++++++
 system/vl.c                         |  6 +--
 target/i386/cpu.c                   |  2 +-
 target/sparc/cpu.c                  |  2 +-
 tests/unit/test-qdev-global-props.c |  4 +-
 12 files changed, 114 insertions(+), 113 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index f131cde2c0..794b18f7c5 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -182,7 +182,7 @@ static void cpu_common_parse_features(const char *typename, char *features,
             prop->driver = typename;
             prop->property = g_strdup(featurestr);
             prop->value = g_strdup(val);
-            qdev_prop_register_global(prop);
+            object_prop_register_global(prop);
         } else {
             error_setg(errp, "Expected key=value format, found %s.",
                        featurestr);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index f13350b4fb..5d30ee6257 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -41,7 +41,7 @@ static bool check_prop_still_unset(Object *obj, const char *name,
                                    const void *old_val, const char *new_val,
                                    bool allow_override, Error **errp)
 {
-    const GlobalProperty *prop = qdev_find_global_prop(obj, name);
+    const GlobalProperty *prop = object_find_global_prop(obj, name);
 
     if (!old_val || (!prop && allow_override)) {
         return true;
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 86a583574d..9cba33c311 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -855,77 +855,6 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
     qobject_unref(values);
 }
 
-static GPtrArray *global_props(void)
-{
-    static GPtrArray *gp;
-
-    if (!gp) {
-        gp = g_ptr_array_new();
-    }
-
-    return gp;
-}
-
-void qdev_prop_register_global(GlobalProperty *prop)
-{
-    g_ptr_array_add(global_props(), prop);
-}
-
-const GlobalProperty *qdev_find_global_prop(Object *obj,
-                                            const char *name)
-{
-    GPtrArray *props = global_props();
-    const GlobalProperty *p;
-    int i;
-
-    for (i = 0; i < props->len; i++) {
-        p = g_ptr_array_index(props, i);
-        if (object_dynamic_cast(obj, p->driver)
-            && !strcmp(p->property, name)) {
-            return p;
-        }
-    }
-    return NULL;
-}
-
-int qdev_prop_check_globals(void)
-{
-    int i, ret = 0;
-
-    for (i = 0; i < global_props()->len; i++) {
-        GlobalProperty *prop;
-        ObjectClass *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;
-            continue;
-        }
-        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;
-        }
-    }
-    return ret;
-}
-
-void qdev_prop_set_globals(DeviceState *dev)
-{
-    object_apply_global_props(OBJECT(dev), global_props(),
-                              dev->hotplugged ? NULL : &error_fatal);
-}
-
 /* --- 64bit unsigned int 'size' type --- */
 
 static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f3a996f57d..894372b776 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));
+    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5336728a23..656eb220f2 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -397,33 +397,6 @@ struct BusState {
     ResettableState reset;
 };
 
-/**
- * typedef GlobalProperty - a global property type
- *
- * @used: Set to true if property was used when initializing a device.
- * @optional: If set to true, GlobalProperty will be skipped without errors
- *            if the property doesn't exist.
- *
- * An error is fatal for non-hotplugged devices, when the global is applied.
- */
-typedef struct GlobalProperty {
-    const char *driver;
-    const char *property;
-    const char *value;
-    bool used;
-    bool optional;
-} GlobalProperty;
-
-static inline void
-compat_props_add(GPtrArray *arr,
-                 GlobalProperty props[], size_t nelem)
-{
-    int i;
-    for (i = 0; i < nelem; i++) {
-        g_ptr_array_add(arr, (void *)&props[i]);
-    }
-}
-
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
 /**
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 09aa04ca1e..301debfe79 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -206,11 +206,6 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
 
 void *object_field_prop_ptr(Object *obj, Property *prop);
 
-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 error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
                                     const char *name, const char *value);
 
diff --git a/include/qom/object.h b/include/qom/object.h
index 13d3a655dd..72ee237776 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -695,8 +695,42 @@ Object *object_new_with_propv(const char *typename,
                               Error **errp,
                               va_list vargs);
 
+/**
+ * typedef GlobalProperty - a global property type
+ *
+ * @used: Set to true if property was used when initializing an object
+ * @optional: If set to true, GlobalProperty will be skipped without errors
+ *            if the property doesn't exist.
+ *
+ * When used with devices: an error is fatal for non-hotplugged devices when
+ * the global is applied.
+ */
+typedef struct GlobalProperty {
+    const char *driver;
+    const char *property;
+    const char *value;
+    bool used;
+    bool optional;
+} GlobalProperty;
+
+static inline void
+compat_props_add(GPtrArray *arr,
+                 GlobalProperty props[], size_t nelem)
+{
+    int i;
+    for (i = 0; i < nelem; i++) {
+        g_ptr_array_add(arr, (void *)&props[i]);
+    }
+}
+
 bool object_apply_global_props(Object *obj, const GPtrArray *props,
                                Error **errp);
+void object_prop_set_globals(Object *obj, Error **errp);
+void object_prop_register_global(GlobalProperty *prop);
+int object_prop_check_globals(void);
+const GlobalProperty *object_find_global_prop(Object *obj,
+                                              const char *name);
+
 void object_set_machine_compat_props(GPtrArray *compat_props);
 void object_set_accelerator_compat_props(GPtrArray *compat_props);
 void object_register_sugar_prop(const char *driver, const char *prop,
diff --git a/qom/object.c b/qom/object.c
index 157a45c5f8..60dbd00edd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -472,6 +472,76 @@ bool object_apply_global_props(Object *obj, const GPtrArray *props,
     return true;
 }
 
+static GPtrArray *global_props(void)
+{
+    static GPtrArray *gp;
+
+    if (!gp) {
+        gp = g_ptr_array_new();
+    }
+
+    return gp;
+}
+
+void object_prop_register_global(GlobalProperty *prop)
+{
+    g_ptr_array_add(global_props(), prop);
+}
+
+void object_prop_set_globals(Object *obj, Error **errp)
+{
+    object_apply_global_props(obj, global_props(), errp);
+}
+
+const GlobalProperty *object_find_global_prop(Object *obj,
+                                              const char *name)
+{
+    GPtrArray *props = global_props();
+    const GlobalProperty *p;
+    int i;
+
+    for (i = 0; i < props->len; i++) {
+        p = g_ptr_array_index(props, i);
+        if (object_dynamic_cast(obj, p->driver)
+            && !strcmp(p->property, name)) {
+            return p;
+        }
+    }
+    return NULL;
+}
+
+int object_prop_check_globals(void)
+{
+    int i, ret = 0;
+
+    for (i = 0; i < global_props()->len; i++) {
+        GlobalProperty *prop;
+        ObjectClass *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;
+            continue;
+        }
+        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;
+        }
+    }
+    return ret;
+}
+
 /*
  * Global property defaults
  * Slot 0: accelerator's global property defaults
diff --git a/system/vl.c b/system/vl.c
index bdd2f6ecf6..282668b817 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2153,7 +2153,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
     g->driver   = qemu_opt_get(opts, "driver");
     g->property = qemu_opt_get(opts, "property");
     g->value    = qemu_opt_get(opts, "value");
-    qdev_prop_register_global(g);
+    object_prop_register_global(g);
     return 0;
 }
 
@@ -2679,7 +2679,7 @@ static bool qemu_machine_creation_done(Error **errp)
         net_check_clients();
     }
 
-    qdev_prop_check_globals();
+    object_prop_check_globals();
 
     qdev_machine_creation_done();
 
@@ -3705,7 +3705,7 @@ void qemu_init(int argc, char **argv)
      * Beware, QOM objects created before this point miss global and
      * compat properties.
      *
-     * Global properties get set up by qdev_prop_register_global(),
+     * Global properties get set up by object_prop_register_global(),
      * called from user_register_global_props(), and certain option
      * desugaring.  Also in CPU feature desugaring (buried in
      * parse_cpu_option()), which happens below this point, but may
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4c2e6f3a71..b8838467a3 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5789,7 +5789,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
         prop->driver = typename;
         prop->property = g_strdup(name);
         prop->value = g_strdup(val);
-        qdev_prop_register_global(prop);
+        object_prop_register_global(prop);
     }
 
     if (ambiguous) {
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 9bacfb68cb..8dffa34701 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -114,7 +114,7 @@ cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
     prop->driver = typename;
     prop->property = g_strdup(name);
     prop->value = g_strdup(val);
-    qdev_prop_register_global(prop);
+    object_prop_register_global(prop);
 }
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string */
diff --git a/tests/unit/test-qdev-global-props.c b/tests/unit/test-qdev-global-props.c
index c8862cac5f..00a6b887f3 100644
--- a/tests/unit/test-qdev-global-props.c
+++ b/tests/unit/test-qdev-global-props.c
@@ -96,7 +96,7 @@ static void register_global_properties(GlobalProperty *props)
     int i;
 
     for (i = 0; props[i].driver != NULL; i++) {
-        qdev_prop_register_global(props + i);
+        object_prop_register_global(props + i);
     }
 }
 
@@ -235,7 +235,7 @@ static void test_dynamic_globalprop_subprocess(void)
 
     g_assert_cmpuint(mt->prop1, ==, 101);
     g_assert_cmpuint(mt->prop2, ==, 102);
-    global_error = qdev_prop_check_globals();
+    global_error = object_prop_check_globals();
     g_assert_cmpuint(global_error, ==, 1);
     g_assert(props[0].used);
     g_assert(props[1].used);
-- 
2.45.2

Re: [PATCH v2 1/2] qom/object, qdev: move globals functions to object.c

[Daniel P. Berrangé]

CC: Markus since he's had opinions on stuff related to -global  in
the past.

On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
> Next patch will add Accel globals support. This means that globals won't be
> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
> 
> Move all globals related functions and declarations to object.c. Each
> function is renamed from 'qdev_' to 'object_':
> 
> - qdev_prop_register_global() is now object_prop_register_global()
> - qdev_find_global_prop() is now object_find_global_prop()
> - qdev_prop_check_globals() is now object_prop_check_globals()
> - qdev_prop_set_globals() is now object_prop_set_globals()
> 
> For object_prop_set_globals() an additional change was made: the function
> was hardwired to be used with DeviceState, where dev->hotplugged is checked
> to determine if object_apply_global_props() will receive a NULL or an
> &error_fatal errp. The function now receives an Object and an errp, and
> logic using dev->hotplugged is moved to its caller (device_post_init()).
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/core/cpu-common.c                |  2 +-
>  hw/core/qdev-properties-system.c    |  2 +-
>  hw/core/qdev-properties.c           | 71 -----------------------------
>  hw/core/qdev.c                      |  2 +-
>  include/hw/qdev-core.h              | 27 -----------
>  include/hw/qdev-properties.h        |  5 --
>  include/qom/object.h                | 34 ++++++++++++++
>  qom/object.c                        | 70 ++++++++++++++++++++++++++++
>  system/vl.c                         |  6 +--
>  target/i386/cpu.c                   |  2 +-
>  target/sparc/cpu.c                  |  2 +-
>  tests/unit/test-qdev-global-props.c |  4 +-
>  12 files changed, 114 insertions(+), 113 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index f131cde2c0..794b18f7c5 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -182,7 +182,7 @@ static void cpu_common_parse_features(const char *typename, char *features,
>              prop->driver = typename;
>              prop->property = g_strdup(featurestr);
>              prop->value = g_strdup(val);
> -            qdev_prop_register_global(prop);
> +            object_prop_register_global(prop);
>          } else {
>              error_setg(errp, "Expected key=value format, found %s.",
>                         featurestr);
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index f13350b4fb..5d30ee6257 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -41,7 +41,7 @@ static bool check_prop_still_unset(Object *obj, const char *name,
>                                     const void *old_val, const char *new_val,
>                                     bool allow_override, Error **errp)
>  {
> -    const GlobalProperty *prop = qdev_find_global_prop(obj, name);
> +    const GlobalProperty *prop = object_find_global_prop(obj, name);
>  
>      if (!old_val || (!prop && allow_override)) {
>          return true;
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 86a583574d..9cba33c311 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -855,77 +855,6 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>      qobject_unref(values);
>  }
>  
> -static GPtrArray *global_props(void)
> -{
> -    static GPtrArray *gp;
> -
> -    if (!gp) {
> -        gp = g_ptr_array_new();
> -    }
> -
> -    return gp;
> -}
> -
> -void qdev_prop_register_global(GlobalProperty *prop)
> -{
> -    g_ptr_array_add(global_props(), prop);
> -}
> -
> -const GlobalProperty *qdev_find_global_prop(Object *obj,
> -                                            const char *name)
> -{
> -    GPtrArray *props = global_props();
> -    const GlobalProperty *p;
> -    int i;
> -
> -    for (i = 0; i < props->len; i++) {
> -        p = g_ptr_array_index(props, i);
> -        if (object_dynamic_cast(obj, p->driver)
> -            && !strcmp(p->property, name)) {
> -            return p;
> -        }
> -    }
> -    return NULL;
> -}
> -
> -int qdev_prop_check_globals(void)
> -{
> -    int i, ret = 0;
> -
> -    for (i = 0; i < global_props()->len; i++) {
> -        GlobalProperty *prop;
> -        ObjectClass *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;
> -            continue;
> -        }
> -        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;
> -        }
> -    }
> -    return ret;
> -}
> -
> -void qdev_prop_set_globals(DeviceState *dev)
> -{
> -    object_apply_global_props(OBJECT(dev), global_props(),
> -                              dev->hotplugged ? NULL : &error_fatal);
> -}
> -
>  /* --- 64bit unsigned int 'size' type --- */
>  
>  static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f3a996f57d..894372b776 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));
> +    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
>  }

This is pretty awkward :-(

If we're generalizing this global properties concept, then we want
object_prop_set_globals to be called from the Object base class
code. We can't do that given this need to check the 'hotplugged'
property.

That check, however, is total insanity. Pre-existing problem,
not your fault.

I imagine the rationale is that we don't want to kill QEMU
if setting a global fails, and we're in middle of device_add
on a running VM.

Throwing away errors though is unacceptable IMHO. device_add
can report errors and we should be propagating them. Likewise
for object_add, or any object HMP command creating QOM types.

The trouble is that we're about 4-5 levels deep in a call
chain that lacks "Error **errp".

The root problem is that none of object_new, object_new_with_class
and object_new_with_type have a "Error *errp" parameter.

object_new_with_props and object_new_with_propv both *do* have
a "Error *errp" parameter, but then they call into object_new_with_type
and can't get errors back from that.

IMHO we need to fix this inability to report errors from object
construction. It will certainly be a painful refactoring job,
but I think its neccessary in order to support global props
without this horrible hack checking the "hotpluggable" flag.


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

object_new() cannot fail, and that's fundamental (was: [PATCH v2 1/2] qom/object, qdev: move globals functions to object.c)

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> CC: Markus since he's had opinions on stuff related to -global  in
> the past.
>
> On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
>> Next patch will add Accel globals support. This means that globals won't be
>> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
>> 
>> Move all globals related functions and declarations to object.c. Each
>> function is renamed from 'qdev_' to 'object_':
>> 
>> - qdev_prop_register_global() is now object_prop_register_global()
>> - qdev_find_global_prop() is now object_find_global_prop()
>> - qdev_prop_check_globals() is now object_prop_check_globals()
>> - qdev_prop_set_globals() is now object_prop_set_globals()
>> 
>> For object_prop_set_globals() an additional change was made: the function
>> was hardwired to be used with DeviceState, where dev->hotplugged is checked
>> to determine if object_apply_global_props() will receive a NULL or an
>> &error_fatal errp. The function now receives an Object and an errp, and
>> logic using dev->hotplugged is moved to its caller (device_post_init()).
>> 
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

[...]

>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index f3a996f57d..894372b776 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));
>> +    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
>>  }
>
> This is pretty awkward :-(
>
> If we're generalizing this global properties concept, then we want
> object_prop_set_globals to be called from the Object base class
> code.

Yes.  This series copies the concept from devices to accelerators.  But
since it clearly makes sense for any kind of object, we better move it
to objects instead.  We may not be able to get there in one step,
though.

>       We can't do that given this need to check the 'hotplugged'
> property.
>
> That check, however, is total insanity. Pre-existing problem,
> not your fault.
>
> I imagine the rationale is that we don't want to kill QEMU
> if setting a global fails, and we're in middle of device_add
> on a running VM.

Yes.

> Throwing away errors though is unacceptable IMHO.

To be precise: we're silently ignoring any -global that fail to apply.

I agree that's wrong.

>                                                   device_add
> can report errors and we should be propagating them. Likewise
> for object_add, or any object HMP command creating QOM types.
>
> The trouble is that we're about 4-5 levels deep in a call
> chain that lacks "Error **errp".
>
> The root problem is that none of object_new, object_new_with_class
> and object_new_with_type have a "Error *errp" parameter.

This is a fundamental QOM design decision.

Not mine, mind.  Moreover, I wasn't there, so my idea on design
rationale may well be off; keep that in mind.

QOM properties are not declared statically, they are created
dynamically.  Aside: this is, in my not particularly humble opinion, a
spectacularly bad idea.

Properties are generally created in instance_init() methods.

Fine print: we later added "class properties", which are created
dynamically within the class, and cloned into the instance before 
its instance_init() method runs.

Object creation doesn't take arguments, and cannot fail.  An
instance_init() method doesn't take arguments, and cannot fail.

Objects are configured via properties.  Property setters take an
argument (the property value), and can fail.

Any part of object creation + configuration that could fail must be done
in property setters.

Common usage is create object, configure by setting properties, operate.

The state transition between "configuring" and "operating" is important.
For devices, this state transition happens when property "realized" is
set to true.  For user-creatable objects it happens when method
complete() is called.  Both can fail.  For everything else, the
transition is implicit / ad hoc / unclear.

For more on this (and other QOM design issues), see my memo "Dynamic &
heterogeneous machines, initial configuration: problems", in particular
section "Problem 5: QOM lacks a clear life cycle".
Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/

To introspect properties, you need an object.  You can always create one
for that (can't fail).  Properties created outside object initialization
cannot be introspected that way.  This is how qom-list-properties works.

> object_new_with_props and object_new_with_propv both *do* have
> a "Error *errp" parameter,

Yes, because they combine object creation, which cannot fail, with
setting properties, which can fail.

>                            but then they call into object_new_with_type
> and can't get errors back from that.
>
> IMHO we need to fix this inability to report errors from object
> construction. It will certainly be a painful refactoring job,
> but I think its neccessary in order to support global props
> without this horrible hack checking the "hotpluggable" flag.

Beyond painful.  Possibly infeasible.

Object creation cannot fail.  If we revise this fundamental QOM design
decision, we get to update all call chains leading to object creation.
That's a *massive* undertaking.

Can we solve the problems we have without revising QOM design?

We'd need to delay the actual failure to a point where the design admits
failure.

Here's an idea.  Formalize the life cycle, i.e. make it an explicit
state machine.  Add a "failed" state.  Any error during object creation
makes the object go to "failed".  Going from "failed" to "operating"
fails.  Which is fine, because the design admits failure there.

Thoughts?

Re: object_new() cannot fail, and that's fundamental (was: [PATCH v2 1/2] qom/object, qdev: move globals functions to object.c)

[Daniel P. Berrangé]

On Thu, Aug 01, 2024 at 08:58:10AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > CC: Markus since he's had opinions on stuff related to -global  in
> > the past.
> >
> > On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
> >> Next patch will add Accel globals support. This means that globals won't be
> >> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
> >> 
> >> Move all globals related functions and declarations to object.c. Each
> >> function is renamed from 'qdev_' to 'object_':
> >> 
> >> - qdev_prop_register_global() is now object_prop_register_global()
> >> - qdev_find_global_prop() is now object_find_global_prop()
> >> - qdev_prop_check_globals() is now object_prop_check_globals()
> >> - qdev_prop_set_globals() is now object_prop_set_globals()
> >> 
> >> For object_prop_set_globals() an additional change was made: the function
> >> was hardwired to be used with DeviceState, where dev->hotplugged is checked
> >> to determine if object_apply_global_props() will receive a NULL or an
> >> &error_fatal errp. The function now receives an Object and an errp, and
> >> logic using dev->hotplugged is moved to its caller (device_post_init()).
> >> 
> >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
> [...]
> 
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index f3a996f57d..894372b776 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));
> >> +    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
> >>  }
> >
> > This is pretty awkward :-(
> >
> > If we're generalizing this global properties concept, then we want
> > object_prop_set_globals to be called from the Object base class
> > code.
> 
> Yes.  This series copies the concept from devices to accelerators.  But
> since it clearly makes sense for any kind of object, we better move it
> to objects instead.  We may not be able to get there in one step,
> though.

Looking at how 'DEVICE(obj)->hotplugged' gets set, this logic does
not really need to have a dependancy on Device.

  static void device_initfn(Object *obj)
  {
      DeviceState *dev = DEVICE(obj);

      if (phase_check(PHASE_MACHINE_READY)) {
          dev->hotplugged = 1;
      ...
  }

IOW, we could at least be doing

  object_prop_set_globals(obj, phase_check(PHASE_MACHINE_READY) ? NULL : &error_fatal);

if we wanted this logic in the Object base class, however....


> >                                                   device_add
> > can report errors and we should be propagating them. Likewise
> > for object_add, or any object HMP command creating QOM types.
> >
> > The trouble is that we're about 4-5 levels deep in a call
> > chain that lacks "Error **errp".
> >
> > The root problem is that none of object_new, object_new_with_class
> > and object_new_with_type have a "Error *errp" parameter.
> 
> This is a fundamental QOM design decision.
> 
> Not mine, mind.  Moreover, I wasn't there, so my idea on design
> rationale may well be off; keep that in mind.
> 
> QOM properties are not declared statically, they are created
> dynamically.  Aside: this is, in my not particularly humble opinion, a
> spectacularly bad idea.
> 
> Properties are generally created in instance_init() methods.
> 
> Fine print: we later added "class properties", which are created
> dynamically within the class, and cloned into the instance before 
> its instance_init() method runs.
> 
> Object creation doesn't take arguments, and cannot fail.  An
> instance_init() method doesn't take arguments, and cannot fail.
> 
> Objects are configured via properties.  Property setters take an
> argument (the property value), and can fail.
> 
> Any part of object creation + configuration that could fail must be done
> in property setters.
> 
> Common usage is create object, configure by setting properties, operate.
> 
> The state transition between "configuring" and "operating" is important.
> For devices, this state transition happens when property "realized" is
> set to true.  For user-creatable objects it happens when method
> complete() is called.  Both can fail.  For everything else, the
> transition is implicit / ad hoc / unclear.




> 
> For more on this (and other QOM design issues), see my memo "Dynamic &
> heterogeneous machines, initial configuration: problems", in particular
> section "Problem 5: QOM lacks a clear life cycle".
> Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> 
> To introspect properties, you need an object.  You can always create one
> for that (can't fail).  Properties created outside object initialization
> cannot be introspected that way.  This is how qom-list-properties works.
> 
> > object_new_with_props and object_new_with_propv both *do* have
> > a "Error *errp" parameter,
> 
> Yes, because they combine object creation, which cannot fail, with
> setting properties, which can fail.
> 
> >                            but then they call into object_new_with_type
> > and can't get errors back from that.
> >
> > IMHO we need to fix this inability to report errors from object
> > construction. It will certainly be a painful refactoring job,
> > but I think its neccessary in order to support global props
> > without this horrible hack checking the "hotpluggable" flag.
> 
> Beyond painful.  Possibly infeasible.
> 
> Object creation cannot fail.  If we revise this fundamental QOM design
> decision, we get to update all call chains leading to object creation.
> That's a *massive* undertaking.
> 
> Can we solve the problems we have without revising QOM design?

Hmm, to cover  object_add QMP and -object CLI, we could just put
setting of global props into the object_new_with_prop{s,v} method,
which would deal with usage of user creatable objects, and a few
internal calls which handle errors.

device_add/-device won't hit that code path. Even if we converted
those over to object_new_with_prop{s,v}, that only handles the top
level device. Many devices will create sub-devices behind the scenes
and global properties are supposed to apply to those, and those will
all just be using plain qdev_new().

> We'd need to delay the actual failure to a point where the design admits
> failure.
> 
> Here's an idea.  Formalize the life cycle, i.e. make it an explicit
> state machine.  Add a "failed" state.  Any error during object creation
> makes the object go to "failed".  Going from "failed" to "operating"
> fails.  Which is fine, because the design admits failure there.

I guess the challenge is that we then need to be confident that the
state change is effected everywhere it needs to be, which sounds like
another bit of hairy work unless we can limit the scope.

We could do

  * Add globals support to  object_new_with_prop{s,v}
  * Convert Accel creation over to object_new_with_prop{s,v},
  * Records errors setting globals in device_post_init in a
    field in DeviceState
  * Make qdev_realize() check for & report the error from
    the step above, before doing its normal logic

IOW, going forward we have two patterns

  * Creation uses object_new_with_prop{s,v} and has globals
    set that way (preferred). Works for -object, -accel,
    object_add and some internal direct API calls

  * Creation uses a post-init hook, but the type has to have
    some state change equivalent to 'realized' to do delayed
    reporting of globals errors (discouraged, ideally only
    for Device). Works for -device, device_add

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

[PATCH v2 2/2] qom/object, 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 object_prop_set_globals() in
accel_init_machine(). As done in device_post_init(), user's global
props take precedence over compat props so object_prop_set_globals() is
called after object_set_accelerator_compat_props().

object_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 |  2 ++
 qom/object.c         | 34 +++++++++++++++++++++++-----------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/accel/accel-system.c b/accel/accel-system.c
index f6c947dd82..8cb1eaa907 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -29,6 +29,7 @@
 #include "sysemu/cpus.h"
 #include "qemu/error-report.h"
 #include "accel-system.h"
+#include "qapi/error.h"
 
 int accel_init_machine(AccelState *accel, MachineState *ms)
 {
@@ -43,6 +44,7 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
         object_unref(OBJECT(accel));
     } else {
         object_set_accelerator_compat_props(acc->compat_props);
+        object_prop_set_globals(OBJECT(accel), &error_fatal);
     }
     return ret;
 }
diff --git a/qom/object.c b/qom/object.c
index 60dbd00edd..8730e770e6 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -15,6 +15,7 @@
 #include "qapi/error.h"
 #include "qom/object.h"
 #include "qom/object_interfaces.h"
+#include "qemu/accel.h"
 #include "qemu/cutils.h"
 #include "qemu/memalign.h"
 #include "qapi/visitor.h"
@@ -516,27 +517,38 @@ int object_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 v2 2/2] qom/object, accel-system: add support to Accel globals

[Markus Armbruster]

I apologize for the delay.

Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:

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

The way you phrase this, it sounds like a bug.  But as far as I know,
-accel is meant to have fallback semantics: we use the first one that
works.

Perhaps:

  -accel has fallback semantics, i.e. we try accelerators in order until
  we find one that works.  Any remainder is ignored.

  Because of that, you can't override properties like this:

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

  When KVM is available, 'riscv-aia' will be set to 'emul', and the
  second -accel is ignored.  When KVM is not available, neither option
  works, and the command fails.

Why would you want to override accelerator properties?

Aside: not diagnosing obvious errors in fallback options we don't use is
not nice.  Not this patch's problem.

> A failed attempt to solve this issue by setting 'merge_lists' can be
> found in [1].

I guess this failed because it destroyed the fallback semantics.
Correct?

>               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 object_prop_set_globals() in
> accel_init_machine(). As done in device_post_init(), user's global
> props take precedence over compat props so object_prop_set_globals() is
> called after object_set_accelerator_compat_props().
>
> object_prop_check_globals() is then changed to recognize TYPE_ACCEL
> globals as valid.

Would it make sense to enable -global for user-creatable objects
(-object), too?

> 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

I'm confused.

-accel kvm,riscv-aia=emul creates accelerator kvm (which is an instance
of class "kvm-accel") and sets its property "riscv-aia" to "emul".

-global kvm-accel,riscv-aia=hwaccel changes the (global) default value
of class "kvm-accel" property "riscv-aia".

Are you *sure* your example sets "riscv-aia" to "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>

Re: [PATCH v2 2/2] qom/object, accel-system: add support to Accel globals

[Andrew Jones]

On Wed, Jul 31, 2024 at 08:30:46AM GMT, Markus Armbruster wrote:
> I apologize for the delay.
> 
> Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:
> 
> > 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'.
> 
> The way you phrase this, it sounds like a bug.  But as far as I know,
> -accel is meant to have fallback semantics: we use the first one that
> works.

The fact that some (most?) parameters have override semantics and some
have fallback semantics makes our complicated command line even more
complicated, especially since there's no way to know which is which.
IMHO, always having override semantics and then providing new parameters,
e.g. -accel-fallback (or a property, -accel fallback=on,...), would go a
long way to bringing some order to the universe.

> 
> Perhaps:
> 
>   -accel has fallback semantics, i.e. we try accelerators in order until
>   we find one that works.  Any remainder is ignored.
> 
>   Because of that, you can't override properties like this:
> 
>       qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \
>           -accel kvm,riscv-aia=hwaccel
> 
>   When KVM is available, 'riscv-aia' will be set to 'emul', and the
>   second -accel is ignored.  When KVM is not available, neither option
>   works, and the command fails.
> 
> Why would you want to override accelerator properties?

Testing. Many properties are only available to allow the user to force
non defaults. The example above isn't exactly what triggered this. The
real use is, '-accel kvm' is the default used by libvirt and when
riscv-aia=hwaccel is possible, it will default to hwaccel. In order to
test riscv-aia emulation support using libvirt (which doesn't yet allow
selecting anything riscv specific), I attempted to use the qemu
commandline element to override -accel with kvm,riscv-aia=emul.

Thanks,
drew

Re: [PATCH v2 2/2] qom/object, accel-system: add support to Accel globals

[Markus Armbruster]

Andrew Jones <ajones@ventanamicro.com> writes:

> On Wed, Jul 31, 2024 at 08:30:46AM GMT, Markus Armbruster wrote:
>> I apologize for the delay.
>> 
>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:
>> 
>> > 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'.
>> 
>> The way you phrase this, it sounds like a bug.  But as far as I know,
>> -accel is meant to have fallback semantics: we use the first one that
>> works.
>
> The fact that some (most?) parameters have override semantics and some
> have fallback semantics makes our complicated command line even more
> complicated, especially since there's no way to know which is which.

We traditionally tramsmit such knowledge from guru to disciple.

We certainly made an unholy mess of our command line.

> IMHO, always having override semantics and then providing new parameters,
> e.g. -accel-fallback (or a property, -accel fallback=on,...), would go a
> long way to bringing some order to the universe.
>
>> Perhaps:
>> 
>>   -accel has fallback semantics, i.e. we try accelerators in order until
>>   we find one that works.  Any remainder is ignored.
>> 
>>   Because of that, you can't override properties like this:
>> 
>>       qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \
>>           -accel kvm,riscv-aia=hwaccel
>> 
>>   When KVM is available, 'riscv-aia' will be set to 'emul', and the
>>   second -accel is ignored.  When KVM is not available, neither option
>>   works, and the command fails.
>> 
>> Why would you want to override accelerator properties?
>
> Testing. Many properties are only available to allow the user to force
> non defaults. The example above isn't exactly what triggered this. The
> real use is, '-accel kvm' is the default used by libvirt and when
> riscv-aia=hwaccel is possible, it will default to hwaccel. In order to
> test riscv-aia emulation support using libvirt (which doesn't yet allow
> selecting anything riscv specific), I attempted to use the qemu
> commandline element to override -accel with kvm,riscv-aia=emul.

Ah, that explains why -global solves your problem, too!  Thanks!

I recommend to start the commit message with the use case, then describe
the solution.  Mention other solutions last, if at all.

Re: [PATCH v2 2/2] qom/object, accel-system: add support to Accel globals

[Daniel Henrique Barboza]

On 7/31/24 3:30 AM, Markus Armbruster wrote:
> I apologize for the delay.
> 
> Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:
> 
>> 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'.
> 
> The way you phrase this, it sounds like a bug.  But as far as I know,
> -accel is meant to have fallback semantics: we use the first one that
> works.
> 
> Perhaps:
> 
>    -accel has fallback semantics, i.e. we try accelerators in order until
>    we find one that works.  Any remainder is ignored.
> 
>    Because of that, you can't override properties like this:
> 
>        qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \
>            -accel kvm,riscv-aia=hwaccel
> 
>    When KVM is available, 'riscv-aia' will be set to 'emul', and the
>    second -accel is ignored.  When KVM is not available, neither option
>    works, and the command fails.
> 
> Why would you want to override accelerator properties?
> 
> Aside: not diagnosing obvious errors in fallback options we don't use is
> not nice.  Not this patch's problem.
> 
>> A failed attempt to solve this issue by setting 'merge_lists' can be
>> found in [1].
> 
> I guess this failed because it destroyed the fallback semantics.
> Correct?

If by 'fallback' you mean the idea where we would use -accel kvm -accel tcg, where
we try to use KVM and then, if not available or failed, we use TCG, yes. That was
the reason.

To make that work I needed to homogenize all -accel options, i.e. it wouldn't be
able to have both TCG and KVM as -accel options in the command line, otherwise
something setting 'merge_lists' in a command line like this:

-accel tcg -accel kvm,propA=true -accel kvm,propB=false

would yield

-accel tcg,propA=true,propB=false



> 
>>                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 object_prop_set_globals() in
>> accel_init_machine(). As done in device_post_init(), user's global
>> props take precedence over compat props so object_prop_set_globals() is
>> called after object_set_accelerator_compat_props().
>>
>> object_prop_check_globals() is then changed to recognize TYPE_ACCEL
>> globals as valid.
> 
> Would it make sense to enable -global for user-creatable objects
> (-object), too?


To be honest, I'm not sure. I can't talk on the motivations to have -globals
for devices, but if the same reasoning applies to all other objects, I think
it's ok.

As far as -accel goes I really thing that the elephant in the room is that
-accel is an oddball. "-accel kvm -accel tcg" being a thing is weird. Why not
-accel kvm,fallback=true if we want a fallback mechanic for KVM? Do we have
a strong use case (i.e. libvirt) to keep this mixed accel options around?

Note that this patch doesn't make things particularly better: I'm using globals
to emulate what should be regular command line behavior for -accel. It's kind
of a cope out for a more complex issue.

> 
>> 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
> 
> I'm confused.
> 
> -accel kvm,riscv-aia=emul creates accelerator kvm (which is an instance
> of class "kvm-accel") and sets its property "riscv-aia" to "emul".
> 
> -global kvm-accel,riscv-aia=hwaccel changes the (global) default value
> of class "kvm-accel" property "riscv-aia".
> 
> Are you *sure* your example sets "riscv-aia" to "hwaccel"?

It does. Verified with printfs and debugs. I copied what's done for -devices and
applied to -accel, which means that the global value is applied after the command
line options, doing an overwrite on whatever was set before. You can see that
happening in accel_init_machine() in this patch.


Thanks,

Daniel

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