object,accel-system: allow Accel type globals

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

Posted by Daniel Henrique Barboza 4 months, 3 weeks ago

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

Posted by Markus Armbruster 3 months, 3 weeks ago

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

Posted by Daniel Henrique Barboza 3 months, 2 weeks ago


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

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

Posted by Andrew Jones 3 months, 3 weeks ago

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

Posted by Markus Armbruster 3 months, 3 weeks ago

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.

[PATCH v2 1/2] qom/object, qdev: move globals functions to object.c
[PATCH v2 2/2] qom/object, accel-system: add support to Accel globals