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

[Eduardo Habkost]

On Fri, May 03, 2013 at 01:34:23PM +0200, Igor Mammedov wrote:
> On Mon, 22 Apr 2013 16:00:17 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > ---
> > 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

-- 
Eduardo