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

[Markus Armbruster]

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?

> 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