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

[Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>]

On Mon, Jun 21, 2021 at 07:09:51PM +0300, Valeriy Vdovin wrote:
> 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] 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.
Hello.
I'm waiting some kind of response/confirmation on this message, so I could continue.