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

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Apr 22, 2021 at 12:02:15PM +0300, Valeriy Vdovin wrote:
> 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.

Raw CPUID data is pretty low level x86 detail.  You are asking
for low level data to be exported, but you are asking for that
data to be exported in a nice package that doesn't require
knowledge of low level x86 details.

I understand how that could be nice and useful.  I'm not sure
this is QEMU's job, though.

If exporting the raw CPUID data in a nice self-contained format
is so important for you, you can just publish something similar
to the 5 lines of pseudo-Python code above somewhere.  Maybe it
could be shipped inside QEMU's ./scripts directory.

> 
> 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?
> 

Maybe a new range will appear during the next decade?  I don't
know.  If it happens, low level software components will have to
deal with it.

I'm sure the time I spent writing this email is probably more
than the time required to adapt your theoretical QMP callers to
support a new CPUID range.


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

I believe this boils down to either this is a job for QEMU and
for the QEMU maintainers.  Most of the feedback you got goes in
the direction of not including the QMP command at all.

I'm trying reach a compromise here: I understand `query-cpuid`
wouldn't be the interface you wish you had, but it's the one that
seems more likely to be accepted (at least without additional
refactoring of internal QEMU code or long interface design
discussions).

-- 
Eduardo