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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

The resulting error reporting is perhaps not as clear as it could be.

We report several kinds of errors:

(a) Accelerator misconfiguration, immediately fatal

(b) Accelerator doesn't work, not fatal (we fall back to the next one)

(c) "no accelerator found", fatal

(d) "falling back to", not fatal (we carry on)

Reporting (b) as error is questionable, because it need not be an actual
error.

We report (d) when an accelerator works after other(s) didn't.  This is
not actually an error.

Example:

    $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm
    xencall: error: Could not obtain handle on privileged command interface: No 
such file or directory
    xen be core: xen be core: can't open xen interface
    can't open xen interface

So far, this is just libxen* and accel/xen/xen-all.c being loquacious.

    qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not 
permitted
    qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent
    qemu-system-x86_64: falling back to KVM

These look like errors, but aren't; things are working exactly as
intended, and QEMU runs.  If we want to be chatty about it, we should
make them info, not error.

Note that a nonsensical accelerator is treated just like an accelerator
that exists, but happens to be unavailable.  Trap for less than perfect
typists.

Same with /dev/kvm made inaccessible:

    $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm
    [Xen chatter...]
    qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not 
permitted
    qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent
    Could not access KVM kernel module: Permission denied
    qemu-system-x86_64: -accel kvm: failed to initialize kvm: Permission denied

Here, we do have a fatal error.  We report it as four errors.
Tolerable.

If we turn the maybe-not-really-errors into info to make the first
example less confusing, we need to report a "no accelerator works" error
at the end.

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

Makes sense.

Thanks!