Re: [PATCH v9] qapi: introduce 'query-kvm-cpuid' action

[Markus Armbruster <armbru@redhat.com>]

Claudio Fontana <cfontana@suse.de> writes:

> On 6/17/21 1:09 PM, Markus Armbruster wrote:
>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>> 
>>> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
>>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>>>>
>>>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
>>>>
>>>> It's actually a QMP command.  There are no "qapi methods".
>>>>
>>>>> get virtualized cpu model info generated by QEMU during VM initialization in
>>>>> the form of cpuid representation.
>>>>>
>>>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
>>>>
>>>> virtual CPU
>>>>
>>>>> command line option. From there it takes the name of the model as the basis for
>>>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
>>>>> that state if additional cpu features should be present on the virtual cpu or
>>>>> excluded from it (tokens '+'/'-' or '=on'/'=off').
>>>>> After that QEMU checks if the host's cpu can actually support the derived
>>>>> feature set and applies host limitations to it.
>>>>> After this initialization procedure, virtual cpu has it's model and
>>>>> vendor names, and a working feature set and is ready for identification
>>>>> instructions such as CPUID.
>>>>>
>>>>> Currently full output for this method is only supported for x86 cpus.
>>>>
>>>> Not sure about "currently": the interface looks quite x86-specific to me.
>>>>
>>> Yes, at some point I was thinking this interface could become generic,
>>> but does not seem possible, so I'll remove this note.
>>>
>>>> The commit message doesn't mention KVM except in the command name.  The
>>>> schema provides the command only if defined(CONFIG_KVM).
>>>>
>>>> Can you explain why you need the restriction to CONFIG_KVM?
>>>>
>>> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
>>> flag is set. I was choosing between this and writing empty implementation into
>>> kvm-stub.c
>> 
>> If the command only makes sense for KVM, then it's named correctly, but
>> the commit message lacks a (brief!) explanation why it only makes for
>> KVM.
>
>
> Is it meaningful for HVF?

I can't see why it couldn't be.

Different tack: if KVM is compiled out entirely, the command isn't
there, and trying to use it fails like

    {"error": {"class": "CommandNotFound", "desc": "The command query-kvm-cpuid has not been found"}}

If KVM is compiled in, but disabled, e.g. with -machine accel=tcg, then
the command fails like

    {"error": {"class": "GenericError", "desc": "VCPU was not initialized yet"}}

This is misleading.  The VCPU is actually running, it's just the wrong
kind of VCPU.

>> If it just isn't implemented for anything but KVM, then putting "kvm"
>> into the command name is a bad idea.  Also, the commit message should
>> briefly note the restriction to KVM.

Perhaps this one is closer to reality.

>> Pick one :)
>> 
>> [...]