| Message ID | 20240703204149.1957136-3-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | object,accel-system: allow Accel type globals | expand |
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>
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
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.
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> >
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;
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(-)