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

[Valeriy Vdovin]

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.

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.

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.

Thanks.
> 
> -- 
> Eduardo
>