Re: [Qemu-devel] [RFC PATCH] vl: only display available accelerators

[Philippe Mathieu-Daudé]

On 11/08/2017 02:25 PM, Eduardo Habkost wrote:
> On Wed, Nov 08, 2017 at 05:06:29PM +0000, Daniel P. Berrange wrote:
>> On Wed, Nov 08, 2017 at 02:59:05PM -0200, Eduardo Habkost wrote:
>>> On Wed, Nov 08, 2017 at 01:21:33PM -0300, Philippe Mathieu-Daudé wrote:
>>>> On 11/08/2017 10:28 AM, Daniel P. Berrange wrote:
>>>>> On Mon, Oct 30, 2017 at 09:20:29AM +0100, Eduardo Habkost wrote:
>>>>>> On Mon, Oct 30, 2017 at 01:00:56AM -0300, Philippe Mathieu-Daudé wrote:
>>>>>>> examples configuring with '--enable-kvm --disable-tcg'
>>>>>>>
>>>>>>> - before
>>>>>>>
>>>>>>>   $ qemu-system-x86_64 -accel help
>>>>>>>   Possible accelerators: kvm, xen, hax, tcg
>>>>>>>
>>>>>>>   $ qemu-system-x86_64 -accel tcg
>>>>>>>   qemu-system-x86_64: -machine accel=tcg: No accelerator found
>>>>>>>
>>>>>>>   # qemu-system-x86_64 -accel hax
>>>>>>>   qemu-system-x86_64: -machine accel=hax: No accelerator found
>>>>>>>
>>>>>>>   # qemu-system-x86_64 -accel xen
>>>>>>>   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
>>>>>>>   qemu-system-x86_64: failed to initialize Xen: Operation not permitted
>>>>>>>
>>>>>>> - after
>>>>>>>
>>>>>>>   $ qemu-system-x86_64 -accel help
>>>>>>>   Possible accelerators: kvm
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>>>>>> ---
>>>>>>> RFC because:
>>>>>>>     - I don't think this is the nicest way, too much #ifdef'fery in 
>>>>>>> main()
>>>>>>
>>>>>> I suggest using object_class_get_list(TYPE_ACCEL, false).
>>>>>
>>>>> And check the result of the available() method on the returned classes
>>>>> too, to filter the results.
>>>>
>>>> Good idea! I'll use that.
>>>
>>> It looks like QTest is the only accelerator that implements
>>> ->available(), and its return value is a build-time constant that
>>> depends only on CONFIG_POSIX.
>>>
>>> I wonder why we don't simply avoid compiling the qtest class if
>>> CONFIG_POSIX is unset, making the ->available() method
>>> unnecessary.
>>
>> Yeah that does seem simpler, though I'm surprised that Xen does not
>> implement the available method. Xen is an accel I'd expect to see
>> compiled into an x86 build, but is only available if the host is
>> actually booted under a Xen hypervisor.  Likewise shouldn't kvm
>> only report itself as available if the /dev/kvm actually exists.
>> But maybe that's not the kind of semantics code using available()
>> expects ?
> 
> Currently the only caller of ->available() calls ->init_machine()
> immediately afterwards, so for the current code it doesn't matter
> if the check is inside ->available() or ->init_machine().
> 
> That said, I'm not sure we should look for /dev/kvm or check for
> the Xen hypervisor when handling "-accel help".  I expect help
> text to tell the user what the QEMU binary supports, not what the
> current host supports.

Yes I prefer that too, I'll write something up such:

static bool kvm_available(void)
{
    return access("/dev/kvm", R_OK|W_OK) == 0;
}

However I wonder, if an user is not in the kvm group but is in sudoers
and run "qemu-system-aarch64 -accel help" he won't see KVM as
available... (I tend to not use sudo when looking for -help output).

same with:

static bool hax_available(void)
{
    return access("/dev/HAX", R_OK|W_OK) == 0;
}

ok for:

static bool xen_available(void)
{
    return access("/sys/hypervisor/properties/features", F_OK) == 0;
}

On POWER7 we have:

/* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";

Isn't it cleaner to register 2 accelerators, KVM-PR and KVM-HV and have
KVM being an alias?