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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxx > > >> >> > > >> >> 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.
