Patchwork [qom-cpu,6/9] target-i386: Add "feature-words" property

login
register
mail settings
Submitter Eduardo Habkost
Date April 22, 2013, 7 p.m.
Message ID <1366657220-776-7-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/238637/
State New
Headers show

Comments

Eduardo Habkost - April 22, 2013, 7 p.m.
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

Example output:

  $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words
  item[0].cpuid-register: EDX
  item[0].cpuid-input-eax: 2147483658
  item[0].features: 0
  item[1].cpuid-register: EAX
  item[1].cpuid-input-eax: 1073741825
  item[1].features: 0
  item[2].cpuid-register: EDX
  item[2].cpuid-input-eax: 3221225473
  item[2].features: 0
  item[3].cpuid-register: ECX
  item[3].cpuid-input-eax: 2147483649
  item[3].features: 101
  item[4].cpuid-register: EDX
  item[4].cpuid-input-eax: 2147483649
  item[4].features: 563346425
  item[5].cpuid-register: EBX
  item[5].cpuid-input-eax: 7
  item[5].features: 0
  item[5].cpuid-input-ecx: 0
  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

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(-)
Eric Blake - April 22, 2013, 8:37 p.m.
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
> 
> Example output:
> 
>   $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words

If I'm not mistaken, the QMP counterpart that libvirt will use is:

{ "execute":"qom-get",
  "arguments": { "path":"/machine/unattached/device[1]",
                 "property":"feature-words" } }

>   item[0].cpuid-register: EDX
>   item[0].cpuid-input-eax: 2147483658
>   item[0].features: 0
>   item[1].cpuid-register: EAX
>   item[1].cpuid-input-eax: 1073741825
>   item[1].features: 0
>   item[2].cpuid-register: EDX
>   item[2].cpuid-input-eax: 3221225473
>   item[2].features: 0
>   item[3].cpuid-register: ECX
>   item[3].cpuid-input-eax: 2147483649
>   item[3].features: 101
>   item[4].cpuid-register: EDX
>   item[4].cpuid-input-eax: 2147483649
>   item[4].features: 563346425
>   item[5].cpuid-register: EBX
>   item[5].cpuid-input-eax: 7
>   item[5].features: 0
>   item[5].cpuid-input-ecx: 0
>   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

And this would then be returned as a JSON array containing struct
members looking like this:

> +{ 'type': 'X86CPUFeatureWordInfo',
> +  'data': { 'cpuid-input-eax': 'int',
> +            '*cpuid-input-ecx': 'int',
> +            'cpuid-register': 'X86CPURegister32',
> +            'features': 'int' } }

Looks reasonable (and better than what we've had in the past!), although
I'll let Jiri Denemark give final say on whether it meets libvirt's needs.
Eduardo Habkost - April 23, 2013, 7:25 p.m.
On Mon, Apr 22, 2013 at 02:37:06PM -0600, Eric Blake wrote:
> 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
> > 
> > Example output:
> > 
> >   $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words
> 
> If I'm not mistaken, the QMP counterpart that libvirt will use is:
> 
> { "execute":"qom-get",
>   "arguments": { "path":"/machine/unattached/device[1]",
>                  "property":"feature-words" } }

Yes, but note that libvirt needs to look for the X86CPU objects, to find
the actual path as there's no guarantee that it will always be
/machine/unattached/devices[1]. Maybe the CPU hotplug work will end up
offering a predictable path for the X86CPU objects, but this is not
available yet.

> 
> >   item[0].cpuid-register: EDX
> >   item[0].cpuid-input-eax: 2147483658
> >   item[0].features: 0
> >   item[1].cpuid-register: EAX
> >   item[1].cpuid-input-eax: 1073741825
> >   item[1].features: 0
> >   item[2].cpuid-register: EDX
> >   item[2].cpuid-input-eax: 3221225473
> >   item[2].features: 0
> >   item[3].cpuid-register: ECX
> >   item[3].cpuid-input-eax: 2147483649
> >   item[3].features: 101
> >   item[4].cpuid-register: EDX
> >   item[4].cpuid-input-eax: 2147483649
> >   item[4].features: 563346425
> >   item[5].cpuid-register: EBX
> >   item[5].cpuid-input-eax: 7
> >   item[5].features: 0
> >   item[5].cpuid-input-ecx: 0
> >   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
> 
> And this would then be returned as a JSON array containing struct
> members looking like this:
> 
> > +{ 'type': 'X86CPUFeatureWordInfo',
> > +  'data': { 'cpuid-input-eax': 'int',
> > +            '*cpuid-input-ecx': 'int',
> > +            'cpuid-register': 'X86CPURegister32',
> > +            'features': 'int' } }

Yes.

> 
> Looks reasonable (and better than what we've had in the past!), although
> I'll let Jiri Denemark give final say on whether it meets libvirt's needs.

Thanks for the feedback!
Igor Mammedov - May 3, 2013, 11:34 a.m.
On Mon, 22 Apr 2013 16:00:17 -0300
Eduardo Habkost <ehabkost@redhat.com> 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

patch doesn't apply to current qom-cpu, more comments below.

> 
> Example output:
> 
>   $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words
>   item[0].cpuid-register: EDX
>   item[0].cpuid-input-eax: 2147483658
>   item[0].features: 0
>   item[1].cpuid-register: EAX
>   item[1].cpuid-input-eax: 1073741825
>   item[1].features: 0
>   item[2].cpuid-register: EDX
>   item[2].cpuid-input-eax: 3221225473
>   item[2].features: 0
>   item[3].cpuid-register: ECX
>   item[3].cpuid-input-eax: 2147483649
>   item[3].features: 101
>   item[4].cpuid-register: EDX
>   item[4].cpuid-input-eax: 2147483649
>   item[4].features: 563346425
>   item[5].cpuid-register: EBX
>   item[5].cpuid-input-eax: 7
>   item[5].features: 0
>   item[5].cpuid-input-ecx: 0
could item be represented as CPUID function in format used in Intel's SDM?

item[5].CPUID: EAX=7h,ECX=0h
item[5].EBX: 0
item[5].EAX: 0

for simplicity/uniformity ECX could be not optional, always present, and
ignored when not needed.
 
>   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
> 
> 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(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 487813a..71408e3 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -21,6 +21,8 @@ linux-headers/asm
>  qapi-generated
>  qapi-types.[ch]
>  qapi-visit.[ch]
> +cpu-qapi-types.[ch]
> +cpu-qapi-visit.[ch]
still needed?

>  qmp-commands.h
>  qmp-marshal.c
>  qemu-doc.html
> diff --git a/Makefile.objs b/Makefile.objs
> index a473348..1835d37 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -78,10 +78,15 @@ common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y)
>  ######################################################################
>  # qapi
>  
> -common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o
> +common-obj-y += qmp-marshal.o
>  common-obj-y += qmp.o hmp.o
>  endif
>  
> +######################################################################
> +# some qapi visitors are used by both system and user emulation:
> +
> +common-obj-y += qapi-visit.o qapi-types.o
> +
>  #######################################################################
>  # Target-independent parts used in system and user emulation
>  common-obj-y += qemu-log.o
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 751d3c2..424434f 100644
> --- a/qapi-schema.json
> +++ 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
> +##
> +{ 'enum': 'X86CPURegister32',
> +  'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] }
> +
> +##
> +# @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' } }
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 314931e..757749c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -30,6 +30,8 @@
>  #include "qemu/config-file.h"
>  #include "qapi/qmp/qerror.h"
>  
> +#include "qapi-types.h"
> +#include "qapi-visit.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/arch_init.h"
>  
> @@ -194,23 +196,34 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>      },
>  };
>  
> +typedef struct X86RegisterInfo32 {
> +    /* Name of register */
> +    const char *name;
> +    /* QAPI enum value register */
> +    X86CPURegister32 qapi_enum;
> +} X86RegisterInfo32;
> +
> +#define REGISTER(reg) \
> +    [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg }
                                                ^^^^
Using auto-generated names like this probably is very fragile.

> +X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
> +    REGISTER(EAX),
> +    REGISTER(ECX),
> +    REGISTER(EDX),
> +    REGISTER(EBX),
> +    REGISTER(ESP),
> +    REGISTER(EBP),
> +    REGISTER(ESI),
> +    REGISTER(EDI),
> +};
> +#undef REGISTER
> +
> +
>  const char *get_register_name_32(unsigned int reg)
>  {
> -    static const char *reg_names[CPU_NB_REGS32] = {
> -        [R_EAX] = "EAX",
> -        [R_ECX] = "ECX",
> -        [R_EDX] = "EDX",
> -        [R_EBX] = "EBX",
> -        [R_ESP] = "ESP",
> -        [R_EBP] = "EBP",
> -        [R_ESI] = "ESI",
> -        [R_EDI] = "EDI",
> -    };
> -
>      if (reg > CPU_NB_REGS32) {
>          return NULL;
>      }
> -    return reg_names[reg];
> +    return x86_reg_info_32[reg].name;
>  }
>  
>  /* collects per-function cpuid data
> @@ -1360,6 +1373,36 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque,
> +                                   const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    FeatureWord w;
> +    Error *err = NULL;
> +    X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { };
> +    X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { };
> +    X86CPUFeatureWordInfoList *list = NULL;
> +
> +    for (w = 0; w < FEATURE_WORDS; w++) {
> +        FeatureWordInfo *wi = &feature_word_info[w];
> +        X86CPUFeatureWordInfo *qwi = &word_infos[w];
> +        qwi->cpuid_input_eax = wi->cpuid_eax;
> +        qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx;
> +        qwi->cpuid_input_ecx = wi->cpuid_ecx;
> +        qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum;
Is there way not to use qapi_enum at all and use name instead?

> +        qwi->features = env->features[w];
> +
> +        /* List will be in reverse order, but order shouldn't matter */
> +        list_entries[w].next = list;
> +        list_entries[w].value = &word_infos[w];
list_entries[w].value = qwi;

> +        list = &list_entries[w];
> +    }
> +
> +    visit_type_X86CPUFeatureWordInfoList(v, &list, "feature-words", &err);
> +    error_propagate(errp, err);
> +}
> +
>  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
>      x86_def_t *def;
> @@ -2348,6 +2391,9 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
> +                        x86_cpu_get_feature_words,
> +                        NULL, NULL, NULL, NULL);
>  
>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>  
> -- 
> 1.8.1.4
> 
>
Eduardo Habkost - May 3, 2013, 1:17 p.m.
On Fri, May 03, 2013 at 01:34:23PM +0200, Igor Mammedov wrote:
> On Mon, 22 Apr 2013 16:00:17 -0300
> Eduardo Habkost <ehabkost@redhat.com> 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
> 
> patch doesn't apply to current qom-cpu, more comments below.
> 
> > 
> > Example output:
> > 
> >   $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words
> >   item[0].cpuid-register: EDX
> >   item[0].cpuid-input-eax: 2147483658
> >   item[0].features: 0
> >   item[1].cpuid-register: EAX
> >   item[1].cpuid-input-eax: 1073741825
> >   item[1].features: 0
> >   item[2].cpuid-register: EDX
> >   item[2].cpuid-input-eax: 3221225473
> >   item[2].features: 0
> >   item[3].cpuid-register: ECX
> >   item[3].cpuid-input-eax: 2147483649
> >   item[3].features: 101
> >   item[4].cpuid-register: EDX
> >   item[4].cpuid-input-eax: 2147483649
> >   item[4].features: 563346425
> >   item[5].cpuid-register: EBX
> >   item[5].cpuid-input-eax: 7
> >   item[5].features: 0
> >   item[5].cpuid-input-ecx: 0
> could item be represented as CPUID function in format used in Intel's SDM?
> 

We could, but maybe it would make the interface harder to use and not
easier?

Even when two feature words are returned in the same CPUID leaf, they
are independent and separate feature-words that must be checked
individually by libvirt, so I believe returning one feature-word per
array-item makes more sense. Having an extra item in the array would
make it clear for libvirt that QEMU has a new feature-word that libvirt
doesn't know about, and easier to spot than an extra field in an
existing array item.


> item[5].CPUID: EAX=7h,ECX=0h

What would be the data type of this "CPUID" field? Are you suggesting
returning a string to be parsed manually?


> item[5].EBX: 0
> item[5].EAX: 0
> 
> for simplicity/uniformity ECX could be not optional, always present, and
> ignored when not needed.

Then how would we represent the fact that ECX is optional? If ECX is not
important for a given CPUID leaf, I don't see what's the point of
including it with a fake value.

Note that this interface is not supposed to be a comprehensive "CPUID
dump" with all CPUID bits returned by the CPU, but just a list of "CPU
capability words" that happen to be returned inside CPUID leaves.

>  
> >   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
> > 
> > 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(-)
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 487813a..71408e3 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -21,6 +21,8 @@ linux-headers/asm
> >  qapi-generated
> >  qapi-types.[ch]
> >  qapi-visit.[ch]
> > +cpu-qapi-types.[ch]
> > +cpu-qapi-visit.[ch]
> still needed?

Not needed anymore. Thanks for spotting it.

> 
> >  qmp-commands.h
> >  qmp-marshal.c
> >  qemu-doc.html
> > diff --git a/Makefile.objs b/Makefile.objs
> > index a473348..1835d37 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -78,10 +78,15 @@ common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y)
> >  ######################################################################
> >  # qapi
> >  
> > -common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o
> > +common-obj-y += qmp-marshal.o
> >  common-obj-y += qmp.o hmp.o
> >  endif
> >  
> > +######################################################################
> > +# some qapi visitors are used by both system and user emulation:
> > +
> > +common-obj-y += qapi-visit.o qapi-types.o
> > +
> >  #######################################################################
> >  # Target-independent parts used in system and user emulation
> >  common-obj-y += qemu-log.o
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 751d3c2..424434f 100644
> > --- a/qapi-schema.json
> > +++ 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
> > +##
> > +{ 'enum': 'X86CPURegister32',
> > +  'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] }
> > +
> > +##
> > +# @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' } }
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 314931e..757749c 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -30,6 +30,8 @@
> >  #include "qemu/config-file.h"
> >  #include "qapi/qmp/qerror.h"
> >  
> > +#include "qapi-types.h"
> > +#include "qapi-visit.h"
> >  #include "qapi/visitor.h"
> >  #include "sysemu/arch_init.h"
> >  
> > @@ -194,23 +196,34 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> >      },
> >  };
> >  
> > +typedef struct X86RegisterInfo32 {
> > +    /* Name of register */
> > +    const char *name;
> > +    /* QAPI enum value register */
> > +    X86CPURegister32 qapi_enum;
> > +} X86RegisterInfo32;
> > +
> > +#define REGISTER(reg) \
> > +    [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg }
>                                                 ^^^^
> Using auto-generated names like this probably is very fragile.

AFAIK, using the auto-generated name is the only way of using qapi enums
and the qapi code generator is supposed to never break this.


[...]
> > +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque,
> > +                                   const char *name, Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    CPUX86State *env = &cpu->env;
> > +    FeatureWord w;
> > +    Error *err = NULL;
> > +    X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { };
> > +    X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { };
> > +    X86CPUFeatureWordInfoList *list = NULL;
> > +
> > +    for (w = 0; w < FEATURE_WORDS; w++) {
> > +        FeatureWordInfo *wi = &feature_word_info[w];
> > +        X86CPUFeatureWordInfo *qwi = &word_infos[w];
> > +        qwi->cpuid_input_eax = wi->cpuid_eax;
> > +        qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx;
> > +        qwi->cpuid_input_ecx = wi->cpuid_ecx;
> > +        qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum;
> Is there way not to use qapi_enum at all and use name instead?

Are you suggesting making the qapi interface be string-based instead of
using an enum? Why?


> 
> > +        qwi->features = env->features[w];
> > +
> > +        /* List will be in reverse order, but order shouldn't matter */
> > +        list_entries[w].next = list;
> > +        list_entries[w].value = &word_infos[w];
> list_entries[w].value = qwi;

Thanks, I will fix this.

> 
> > +        list = &list_entries[w];
> > +    }
> > +
> > +    visit_type_X86CPUFeatureWordInfoList(v, &list, "feature-words", &err);
> > +    error_propagate(errp, err);
> > +}
> > +
> >  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> >  {
> >      x86_def_t *def;
> > @@ -2348,6 +2391,9 @@ static void x86_cpu_initfn(Object *obj)
> >      object_property_add(obj, "tsc-frequency", "int",
> >                          x86_cpuid_get_tsc_freq,
> >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > +    object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
> > +                        x86_cpu_get_feature_words,
> > +                        NULL, NULL, NULL, NULL);
> >  
> >      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> >  
> > -- 
> > 1.8.1.4
> > 
> > 
> 
> 
> -- 
> Regards,
>   Igor
Eric Blake - May 3, 2013, 2:25 p.m.
On 05/03/2013 07:17 AM, Eduardo Habkost wrote:
> 
> We could, but maybe it would make the interface harder to use and not
> easier?
> 
> Even when two feature words are returned in the same CPUID leaf, they
> are independent and separate feature-words that must be checked
> individually by libvirt, so I believe returning one feature-word per
> array-item makes more sense. Having an extra item in the array would
> make it clear for libvirt that QEMU has a new feature-word that libvirt
> doesn't know about, and easier to spot than an extra field in an
> existing array item.

Firmly agree - bundling multiple features into one array item is not nice.

> 
> 
>> item[5].CPUID: EAX=7h,ECX=0h
> 
> What would be the data type of this "CPUID" field? Are you suggesting
> returning a string to be parsed manually?

Anything that requires parsing to break into pieces on the receiving end
implies that it was not correctly represented in JSON in the first
place.  I'd much rather see it kept as multiple fields.

>>> +    for (w = 0; w < FEATURE_WORDS; w++) {
>>> +        FeatureWordInfo *wi = &feature_word_info[w];
>>> +        X86CPUFeatureWordInfo *qwi = &word_infos[w];
>>> +        qwi->cpuid_input_eax = wi->cpuid_eax;
>>> +        qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx;
>>> +        qwi->cpuid_input_ecx = wi->cpuid_ecx;
>>> +        qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum;
>> Is there way not to use qapi_enum at all and use name instead?
> 
> Are you suggesting making the qapi interface be string-based instead of
> using an enum? Why?

enum-based is better than string based.  That way, when we add
introspection in qemu 1.6, libvirt can see what enum values to expect,
instead of having an open-ended set of strings with no idea what strings
will be present.
Eric Blake - May 3, 2013, 2:57 p.m.
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>

Patch

diff --git a/.gitignore b/.gitignore
index 487813a..71408e3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -21,6 +21,8 @@  linux-headers/asm
 qapi-generated
 qapi-types.[ch]
 qapi-visit.[ch]
+cpu-qapi-types.[ch]
+cpu-qapi-visit.[ch]
 qmp-commands.h
 qmp-marshal.c
 qemu-doc.html
diff --git a/Makefile.objs b/Makefile.objs
index a473348..1835d37 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -78,10 +78,15 @@  common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y)
 ######################################################################
 # qapi
 
-common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o
+common-obj-y += qmp-marshal.o
 common-obj-y += qmp.o hmp.o
 endif
 
+######################################################################
+# some qapi visitors are used by both system and user emulation:
+
+common-obj-y += qapi-visit.o qapi-types.o
+
 #######################################################################
 # Target-independent parts used in system and user emulation
 common-obj-y += qemu-log.o
diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..424434f 100644
--- a/qapi-schema.json
+++ 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
+##
+{ 'enum': 'X86CPURegister32',
+  'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] }
+
+##
+# @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' } }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 314931e..757749c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -30,6 +30,8 @@ 
 #include "qemu/config-file.h"
 #include "qapi/qmp/qerror.h"
 
+#include "qapi-types.h"
+#include "qapi-visit.h"
 #include "qapi/visitor.h"
 #include "sysemu/arch_init.h"
 
@@ -194,23 +196,34 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
     },
 };
 
+typedef struct X86RegisterInfo32 {
+    /* Name of register */
+    const char *name;
+    /* QAPI enum value register */
+    X86CPURegister32 qapi_enum;
+} X86RegisterInfo32;
+
+#define REGISTER(reg) \
+    [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg }
+X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
+    REGISTER(EAX),
+    REGISTER(ECX),
+    REGISTER(EDX),
+    REGISTER(EBX),
+    REGISTER(ESP),
+    REGISTER(EBP),
+    REGISTER(ESI),
+    REGISTER(EDI),
+};
+#undef REGISTER
+
+
 const char *get_register_name_32(unsigned int reg)
 {
-    static const char *reg_names[CPU_NB_REGS32] = {
-        [R_EAX] = "EAX",
-        [R_ECX] = "ECX",
-        [R_EDX] = "EDX",
-        [R_EBX] = "EBX",
-        [R_ESP] = "ESP",
-        [R_EBP] = "EBP",
-        [R_ESI] = "ESI",
-        [R_EDI] = "EDI",
-    };
-
     if (reg > CPU_NB_REGS32) {
         return NULL;
     }
-    return reg_names[reg];
+    return x86_reg_info_32[reg].name;
 }
 
 /* collects per-function cpuid data
@@ -1360,6 +1373,36 @@  static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    FeatureWord w;
+    Error *err = NULL;
+    X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { };
+    X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { };
+    X86CPUFeatureWordInfoList *list = NULL;
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        FeatureWordInfo *wi = &feature_word_info[w];
+        X86CPUFeatureWordInfo *qwi = &word_infos[w];
+        qwi->cpuid_input_eax = wi->cpuid_eax;
+        qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx;
+        qwi->cpuid_input_ecx = wi->cpuid_ecx;
+        qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum;
+        qwi->features = env->features[w];
+
+        /* List will be in reverse order, but order shouldn't matter */
+        list_entries[w].next = list;
+        list_entries[w].value = &word_infos[w];
+        list = &list_entries[w];
+    }
+
+    visit_type_X86CPUFeatureWordInfoList(v, &list, "feature-words", &err);
+    error_propagate(errp, err);
+}
+
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
     x86_def_t *def;
@@ -2348,6 +2391,9 @@  static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
+                        x86_cpu_get_feature_words,
+                        NULL, NULL, NULL, NULL);
 
     env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);