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 <ehabk...@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 <ehabk...@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 <cfont...@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.crsxvqixkkll4...@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.
