On 7/31/24 3:30 AM, Markus Armbruster wrote:
I apologize for the delay.
Daniel Henrique Barboza <dbarb...@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-dbarb...@ventanamicro.com/
[2]
https://lore.kernel.org/qemu-devel/CABgObfbVJkCdpHV2uA7LGTbYEaoaizrbeG0vNo_T1ua5b=j...@mail.gmail.com/
Reported-by: Andrew Jones <ajo...@ventanamicro.com>
Suggested-by: Paolo Bonzini <pbonz...@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
