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

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote:
> On 6/17/21 5:39 PM, Valeriy Vdovin wrote:
> > 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?
> 
> query-exposed-cpuid?

Pasting the reply I sent at [1]:

  I don't really mind how the command is called, but I would prefer
  to add a more complex abstraction only if maintainers of other
  accelerators are interested and volunteer to provide similar
  functionality.  I don't want to introduce complexity for use
  cases that may not even exist.

I'm expecting this to be just a debugging mechanism, not a stable
API to be maintained and supported for decades.  (Maybe a "x-"
prefix should be added to indicate that?)

[1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net

-- 
Eduardo