Re: [Qemu-devel] [PATCH qom-cpu 6/9] target-i386: Add "feature-words" property

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3758 bytes --]

On 04/22/2013 01:00 PM, Eduardo Habkost wrote:
> This property will be useful for libvirt, as libvirt already has logic
> based on low-level feature bits (not feature names), so it will be
> really easy to convert the current libvirt logic to something using the
> "feature-words" property.
> 
> The property will have two main use cases:
>  - Checking host capabilities, by checking the features of the "host"
>    CPU model
>  - Checking which features are enabled on each CPU model
> 

>   item[6].cpuid-register: ECX
>   item[6].cpuid-input-eax: 1
>   item[6].features: 2155880449
>   item[7].cpuid-register: EDX
>   item[7].cpuid-input-eax: 1
>   item[7].features: 126614521

I'm guessing the corresponding JSON passed over QMP would look something
like:

[ ...
  { "cpuid-register": "ECX",
    "cpuid-input-eax": 1,
    "features": 2155880449 },
  { "cpuid-register": "EDX",
    "cpuid-input-eax": 1,
    "features": 126614521 } ]

which libvirt can reasonably parse.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  * Merge the non-qapi and qapi patches, to keep series simpler
>  * Use the feature word array series as base, so we don't have
>    to set the feature word values one-by-one in the code
>  * Change type name of property from "x86-cpu-feature-words" to
>    "X86CPUFeatureWordInfo"
>  * Remove cpu-qapi-schema.json and simply add the type definitions
>    to qapi-schema.json, to keep the changes simpler
>    * This required compiling qapi-types.o and qapi-visit.o into
>      *-user as well
> ---
>  .gitignore        |  2 ++
>  Makefile.objs     |  7 +++++-
>  qapi-schema.json  | 31 ++++++++++++++++++++++++
>  target-i386/cpu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++----------
>  4 files changed, 97 insertions(+), 13 deletions(-)

I'm not sure I'm the best person to review cpu.c, but I can at least
review the interface from what libvirt plans on using:

> +++ b/qapi-schema.json
> @@ -3505,3 +3505,34 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +# @X86CPURegister32
> +#
> +# A X86 32-bit register
> +#
> +# Since: 1.5

Yes, I'd still like to get this into 1.5.  On some enums, we have called
out doc-text for each enum value; but I'm fine with your choice here to
omit that.

> +##
> +{ 'enum': 'X86CPURegister32',
> +  'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] }

Any reason you favored ALL-CAPS names?  But it doesn't matter to me, as
long as we stick to the name once baked into a release.

> +
> +##
> +# @X86CPUFeatureWordInfo
> +#
> +# Information about a X86 CPU feature word
> +#
> +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word
> +#
> +# @cpuid-input-ecx: #optional Input ECX value for CPUID instruction for that
> +#                   feature word
> +#
> +# @cpuid-register: Output register containing the feature bits
> +#
> +# @features: value of output register, containing the feature bits
> +#
> +# Since: 1.5
> +##
> +{ 'type': 'X86CPUFeatureWordInfo',
> +  'data': { 'cpuid-input-eax': 'int',
> +            '*cpuid-input-ecx': 'int',
> +            'cpuid-register': 'X86CPURegister32',
> +            'features': 'int' } }

Looks reasonable for the interface side of things.

>  
> +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque,
> +                                   const char *name, Error **errp)

Indentation looks off.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]