Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Paolo Bonzini]

On 18/11/20 09:36, Markus Armbruster wrote:
> The part that counts is do_configure_accelerator().  I works as follows:
>
> 1. Look up the chosen accelerator's QOM type (can fail)
> 2. Instantiate it (can't fail)
> 3. Set properties (can fail)
> 4. Connect the accelerator to the current machine (can fail)
>
> Aside: step 3 uses &error_fatal, unlike the other failures.  Fishy.

That's because step 3 is a user error, unlike (in the usual case) the 
others:

1. You get it from "-accel whpx" if whpx is not available; this is not a 
user error if there is a fallback.  You also get it from misspellings 
such as "-accel kvmm", which is presumably a user error, but it's hard 
to distinguish the two especially if a future version of QEMU ends up 
adding a "kvmm" accelerator

3. You get it from "-accel tcg,misspeled-property=off".  This is a user 
error.  You also get it from "-accel tcg,absent-property=on" and "-accel 
tcg,invalid-value=42".  This may be a property that the user wants to 
set but was only added in a future version of QEMU.  Either way, it's 
quite likely that the user does _not_ want a fallback here.

4. Here the accelerator exists but the machine does not support it.  The 
choice is to fallback to the next accelerato.

This means there is no way for the user to distinguish "the accelerator 
doesn't exist" from "the accelerator exists but is not supported".  This 
was done for no particular reason other than to keep the code simple.

> Failure in step 1 is "accelerator not compiled in".  Predictable with
> qom-list-types.
>
> Failure in step 3 doesn't look predictable.

It's more or less predictable with qom-list-properties, though of course 
not the "invalid value for the property" case.

> Failure in step 4 can be due to kernel lacking (a workable version of)
> KVM, but the current machine gets a say, too.  Also doesn't look
> predictable.
>
> Aside: kvm_init() reports errors with fprintf(), tsk, tsk, tsk.
>
> Existing query-kvm doesn't really predict failure, either.  'present' is
> true if CONFIG_KVM.  Should be equivalent to "QOM type exists",
> i.e. step 1.  I guess 'enabled' is true when the KVM accelerator is in
> use.
>
> While figuring this out, I noticed that the TYPE_ACCEL instance we
> create doesn't get its parent set.  It's therefore not in the QOM
> composition tree, and inaccessible with qom-get.  Paolo, is this as it
> should be?

It should be added, I agree, perhaps as /machine/accel.

Paolo