Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action

[Eduardo Habkost]

On Wed, Apr 21, 2021 at 08:39:42PM +0300, Valeriy Vdovin wrote:
> On Tue, Apr 20, 2021 at 01:09:00PM -0400, Eduardo Habkost wrote:
> > On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
> > [...]
> > > +##
> > > +# @query-cpu-model-cpuid:
> > > +#
> > > +# Returns description of a virtual CPU model, created by QEMU after cpu
> > > +# initialization routines. The resulting information is a reflection of 
> > > a parsed
> > > +# '-cpu' command line option, filtered by available host cpu features.
> > > +#
> > > +# Returns:  @CpuModelCpuidDescription
> > > +#
> > > +# Example:
> > > +#
> > > +# -> { "execute": "query-cpu-model-cpuid" }
> > > +# <- { "return": 'CpuModelCpuidDescription' }
> > > +#
> > > +# Since: 6.1
> > > +##
> > > +{ 'command': 'query-cpu-model-cpuid',
> > > +  'returns': 'CpuModelCpuidDescription',
> > > +  'if': 'defined(TARGET_I386)' }
> > 
> > I was assuming the command was going to get a CPU model name as
> > argument.
> > 
> > If you are only going to return info on the current CPUs, the
> > interface could be simplified a lot.
> > 
> > What about a simple `query-cpuid` command that only takes:
> > 
> >  { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
> >    'eax': 'uint32',
> >    '*ecx': 'uint32' }
> > 
> > as argument, and returns
> > 
> >  { 'present': 'bool',
> >    'max_eax': 'uint32',    # max value of EAX for this range
> >    '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
> >    'eax': 'uint32',
> >    'ebx': 'uint32',
> >    'ecx': 'uint32',
> >    'edx': 'uint32' }
> > 
> > ?
> Hi. The interface that you suggest looks good. But it has one critical
> point that deems it unusable for our initial needs. The point of this
> whole new API is to take away the strain of knowing about leaf ranges
> from the caller of this API. In my current patch this goal works. So
> the caller does not need to know in advance what ranges there are in
> original CPUID as well as in it's tweaked QEMU's version.
>

Raw CPUID data is a pretty low level interface, already.  Is it
really too much of a burden for callers to know that CPUID ranges
start at 0, 0x40000000, 0x80000000, and 0xC0000000?

(Especially considering that it would save us ~100 extra lines of
C code and maybe 50-100 extra lines of QAPI schema code.)


> But you suggested API is not so kind to the caller, so he would need
> to add some logic around the call that knows about exact leaf ranges.
> If you have a solution to that also, I'll be happy to discuss it.

Would be following (Python-like pseudocode) be too much of a
burden for consumers of the command?

    for start in (0, 0x40000000, 0x80000000, 0xC0000000):
        leaf = query_cpuid(qom_path, start)
        for eax in range(start, leaf.max_eax + 1):
            for ecx in range(0, leaf.get('max_ecx', 0) + 1):
                all_leaves.append(query_cpuid(qom_path, eax, ecx))

> 
> The obvious thing that comes to mind is changing the exists/max_ecx pair
> to something like next_eax, next_ecx. But this idea will probably require
> the same amount of complexity that I currently have in this patch.

I agree.  I'm trying to reduce the complexity of the interface
and of the command implementation.

-- 
Eduardo