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

[Markus Armbruster <armbru@redhat.com>]

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>