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

[Valeriy Vdovin]

On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote:
> 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.
Should I also make some note about that in the commit message?
> 
> 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.
> 
I agree.
What command name do you suggest?
> >> Pick one :)
> >> 
> >> [...]
>