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

[Philippe Mathieu-Daudé]

On 11/08/2017 03:35 PM, Eduardo Habkost wrote:
> On Wed, Nov 08, 2017 at 03:03:34PM -0300, Philippe Mathieu-Daudé wrote:
>> 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).
> 
> If we do that, I think we should include kvm in the list anyway,
> but just add a column indicating that it doesn't seem to be
> available on the host.

So with CONFIG_KVM we have 'KVM enabled' and we should list it, OK.

kvm_available() could be:

     return access("/dev/kvm", F_OK) == 0;

(missing kmod).

Checking for R_OK|W_OK would give kvm_usable() but if the user forgot to
use sudo kvm_arch_init() will fail anyway, so no need to worry about it.

> 
> In either case, I suggest implementing it as a separate patch,
> and just ignore ->available() in the first version.

Sure, availability check are for 2.12.

This -accel help message was added before 2.10, so this is not a bug fix
(nobody complained about it since 2.10). I'll postpone this series for 2.12.

>> 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?
> 
> I believe "-accel kvm,kvm-type=HV" and "-accel kvm,kvm-type=PR"
> would be cleaner ways to represent the two modes, instead of
> having two different accelerator classes.  This is not supported
> by the current "<accel1>:<accel2>" syntax, but I think we should
> replace that syntax with something better anyway.

Ok.

Thanks,

Phil.