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

[Valeriy Vdovin]

On Mon, Jun 21, 2021 at 05:50:55PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Jun 21, 2021 at 10:07:44AM +0200, Claudio Fontana wrote:
> >> On 6/18/21 10:40 PM, Eduardo Habkost wrote:
> >> > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote:
> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >>
> >> >>> 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:
> >> >>
> >> >> [...]
> >> >>
> >> >>>>>>>> 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] 
> >> >>> 20210602204604.crsxvqixkkll4ef4@habkost.net">https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net
> >> >>
> >> >> x-query-x86_64-cpuid?
> >> >>
> >> > 
> >> > Unless somebody wants to spend time designing a generic
> >> > abstraction around this (and justify the extra complexity), this
> >> > is a KVM-specific command.  Is there a reason to avoid "kvm" in
> >> > the command name?
> >> > 
> >> 
> >> If the point of all of this is "please get me the cpuid, as seen by the 
> >> guest", then I fail to see how this should be kvm-only.
> >> We can still return "not implemented" of some kind for HVF, TCG etc.
> >> 
> >> But maybe I misread the use case?
> >
> > A generic interface would require additional glue to connect the
> > generic code to the accel-specific implementation.  I'm trying to
> > avoid wasting everybody's time with the extra complexity unless
> > necessary.
> 
> If I read the patch correctly, the *interface* is specific to x86_64,
> but not to any accelerator.  It's *implemented* only for KVM, though.
> Is that correct?
> 
Yes, it's a x86 specific instruction, and KVM is a bit of implementation
detail right now. It could actually have stubs in other accels instead
of CONFIG_KVM.

> > But if you all believe the extra complexity is worth it, I won't
> > object.
> 
> I'm not arguing for a complete implementation now.
> 
> I think the command name is a matter of taste.
> 
> The command exists only if defined(TARGET_I386).  Putting -x86_64- or
> similar in the name isn't strictly required, but why not.  Maybe just
> -x86-.
> 
> Putting -kvm- in the name signals (1) the command works only with KVM,
> and (2) we don't intend to make it work with other accelerators.  If we
> later decide to make it work with other accelerators, we get to rename
> the command.
> 
> Not putting -kvm- in the name doesn't commit us to anything.
> 
> Either way, the command exists and fails when defined(CONFIG_KVM) and
> accel!=kvm.
> 
> Aside: I think having the command depend on defined(CONFIG_KVM)
> accomplishes nothing but enlarging the test matrix:
> 
>     defined(CONFIG_KVM) and accel=kvm   command exists and works
>     defined(CONFIG_KVM) and accel!=kvm  command exists and fails
>     !defined(CONFIG_KVM)                command does not exist
> 
> Simpler:
> 
>     accel=kvm                           command exists and works
>     accel!=kvm                          command exists and fails
> 
Well, it accomplishes not having stub implementations all over the place.
But looks like having the right error message in stubs really seems more
appropriate.

Your reasoning is pretty clear and in the light of it I now fill that
platform in the name is better that one of the possible accel implementations
in the name.

So should the command name be renamed from 'query-kvm-cpuid' to
'query-x86-cpuid'?

And considering CONFIG_KVM, I guess it would be better to drop this ifdef and 
instead put stub functions in several places? If yes, please let me know
the exact list of places that should have that stub, as well as the right way
to state the "unimplemented" error for these stubs, (sorry, this last
one is just to shorten some of the iterations)

Thanks.