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

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

On Wed, Apr 21, 2021 at 04:17:59PM -0400, Eduardo Habkost wrote:
> 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))
> 
This is a question of architecture and design. Number of lines is
secondary (up to some reasonable point of course).

I want to decouple components as much as possible. It's not a burden to pass
4 digits once you know them, but how exactly should a caller come to these 4 
digits? It's like a password. It's easy once you know it. Check out Intel's
Instruction Set Manual on CPUID - obvious place to learn about ranges for the
caller, yet you wont see exactly these numbers in plain text. And where is 
0x40000000 in this manual exactly? One should read QEMU git history to know 
what it is. Correct me here if I'm wrong.

The work of figuring out the required ranges should not be duplicated without
need. QEMU does that already, there is a nice way of passing them to the caller,
and it makes component interaction more generic (no private knowledge pased),
so why not do that.

Please take into account the design of applications that will use this
method. With less restrictive API, components could be more isolated, for
example one component could only know how to call qmp methods, the other would
have to khow how to process CPUID data, resulting in a clean layered architecture.

Also I'm not sure that these ranges would never change. Are you sure that some
other range won't appear soon? If so, shouldn't we try to make less locations,
where this would have to be supported?

So my pros are: loose coupling / generic interface, less places to
maintain in case of chages. These are pretty basic.
Cons: more lines of code as you've said, but otherwize more code will be
in the callers, and more callers == more duplicated code.

Thanks.
> > 
> > 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
>