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

From: Markus Armbruster <armbru@redhat.com>
To: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
Cc: qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	Eric Blake <eblake@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Thomas Huth <thuth@redhat.com>,
	Laurent Vivier <lvivier@redhat.com>,
	kvm@vger.kernel.org, Denis Lunev <den@openvz.org>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Subject: Re: [PATCH v9] qapi: introduce 'query-kvm-cpuid' action
Date: Thu, 17 Jun 2021 13:09:08 +0200	[thread overview]
Message-ID: <87a6no3fzf.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210617074919.GA998232@dhcp-172-16-24-191.sw.ru> (Valeriy Vdovin's message of "Thu, 17 Jun 2021 10:49:19 +0300")

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 :)

[...]