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

[Markus Armbruster]

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.

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.

Pick one :)

[...]