Patchwork [32/37] target-i386: use static properties for setting cpuid features

login
register
mail settings
Submitter Igor Mammedov
Date Oct. 22, 2012, 3:03 p.m.
Message ID <1350918203-25198-33-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/193211/
State New
Headers show

Comments

Igor Mammedov - Oct. 22, 2012, 3:03 p.m.
- def->kvm_features was used only for setting hadcoded defaults,
   with defaults set by static props, we do not need it anymore,
   ignore it in cpudef_2_x86_cpu().
 - take in account CPUID_EXT_HYPERVISOR feature set by static
   property default value;

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  * fix double free of features in cpu_x86_find_by_name()
---
 target-i386/cpu.c | 271 +++++++-----------------------------------------------
 1 file changed, 31 insertions(+), 240 deletions(-)
Don Slutz - Oct. 23, 2012, 3:29 p.m.
On 10/22/12 11:03, Igor Mammedov wrote:
>   - def->kvm_features was used only for setting hadcoded defaults,
>     with defaults set by static props, we do not need it anymore,
>     ignore it in cpudef_2_x86_cpu().
>   - take in account CPUID_EXT_HYPERVISOR feature set by static
>     property default value;
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>    * fix double free of features in cpu_x86_find_by_name()
> ---
>   target-i386/cpu.c | 271 +++++++-----------------------------------------------
>   1 file changed, 31 insertions(+), 240 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c804965..59809f8 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -87,31 +87,6 @@ static const char *ext3_feature_name[] = {
>       NULL, NULL, NULL, NULL,
>   };
>   
> -static const char *kvm_feature_name[] = {
> -    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -};
> -
> -static const char *svm_feature_name[] = {
> -    "npt", "lbrv", "svm_lock", "nrip_save",
> -    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> -    NULL, NULL, "pause_filter", NULL,
> -    "pfthreshold", NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL,
> -};
> -
> -static const char *cpuid_7_0_ebx_feature_name[] = {
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, "smep",
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL, "smap", NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -};
> -
>   #if defined(CONFIG_KVM)
>   static void x86_cpu_get_kvmclock(Object *obj, Visitor *v, void *opaque,
>                                    const char *name, Error **errp)
> @@ -752,88 +727,6 @@ void host_cpuid(uint32_t function, uint32_t count,
>   #endif
>   }
>   
> -#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
> -
> -/* general substring compare of *[s1..e1) and *[s2..e2).  sx is start of
> - * a substring.  ex if !NULL points to the first char after a substring,
> - * otherwise the string is assumed to sized by a terminating nul.
> - * Return lexical ordering of *s1:*s2.
> - */
> -static int sstrcmp(const char *s1, const char *e1, const char *s2,
> -    const char *e2)
> -{
> -    for (;;) {
> -        if (!*s1 || !*s2 || *s1 != *s2)
> -            return (*s1 - *s2);
> -        ++s1, ++s2;
> -        if (s1 == e1 && s2 == e2)
> -            return (0);
> -        else if (s1 == e1)
> -            return (*s2);
> -        else if (s2 == e2)
> -            return (*s1);
> -    }
> -}
> -
> -/* compare *[s..e) to *altstr.  *altstr may be a simple string or multiple
> - * '|' delimited (possibly empty) strings in which case search for a match
> - * within the alternatives proceeds left to right.  Return 0 for success,
> - * non-zero otherwise.
> - */
> -static int altcmp(const char *s, const char *e, const char *altstr)
> -{
> -    const char *p, *q;
> -
> -    for (q = p = altstr; ; ) {
> -        while (*p && *p != '|')
> -            ++p;
> -        if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
> -            return (0);
> -        if (!*p)
> -            return (1);
> -        else
> -            q = ++p;
> -    }
> -}
> -
> -/* search featureset for flag *[s..e), if found set corresponding bit in
> - * *pval and return true, otherwise return false
> - */
> -static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
> -                           const char **featureset)
> -{
> -    uint32_t mask;
> -    const char **ppc;
> -    bool found = false;
> -
> -    for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
> -        if (*ppc && !altcmp(s, e, *ppc)) {
> -            *pval |= mask;
> -            found = true;
> -        }
> -    }
> -    return found;
> -}
> -
> -static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
> -                                    uint32_t *ext_features,
> -                                    uint32_t *ext2_features,
> -                                    uint32_t *ext3_features,
> -                                    uint32_t *kvm_features,
> -                                    uint32_t *svm_features,
> -                                    uint32_t *cpuid_7_0_ebx_features)
> -{
> -    if (!lookup_feature(features, flagname, NULL, feature_name) &&
> -        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
> -        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
> -        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
> -        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
> -        !lookup_feature(svm_features, flagname, NULL, svm_feature_name) &&
> -        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
> -                        cpuid_7_0_ebx_feature_name))
> -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> -}
> -
>   typedef struct x86_def_t {
>       struct x86_def_t *next;
>       const char *name;
> @@ -1432,10 +1325,10 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>       object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
>                               "tsc-frequency", errp);
>       env->cpuid_features = def->features;
> -    env->cpuid_ext_features = def->ext_features;
> +    env->cpuid_ext_features = (CPUID_EXT_HYPERVISOR & env->cpuid_ext_features) |
> +                              def->ext_features;
>       env->cpuid_ext2_features = def->ext2_features;
>       env->cpuid_ext3_features = def->ext3_features;
> -    env->cpuid_kvm_features = def->kvm_features;
>       env->cpuid_svm_features = def->svm_features;
>       env->cpuid_ext4_features = def->ext4_features;
>       env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> @@ -1532,25 +1425,32 @@ out:
>       return;
>   }
>   
> +/* Set features on X86CPU object based on a QDict */
> +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
> +{
> +    const QDictEntry *ent;
> +
> +    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
> +        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
> +        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
> +                              qdict_entry_key(ent), errp);
> +        if (error_is_set(errp)) {
> +            return;
> +        }
> +    }
> +}
> +
>   static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>                                   const char *cpu_model, Error **errp)
>   {
>       x86_def_t *def;
> -    CPUX86State *env = &cpu->env;
> +    QDict *features = NULL;
> +    char *name = NULL;
>   
> -    char *s = g_strdup(cpu_model);
> -    char *featurestr, *name = strtok(s, ",");
> -    /* Features to be added*/
> -    uint32_t plus_features = 0, plus_ext_features = env->cpuid_ext_features;
> -    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> -    uint32_t plus_kvm_features = env->cpuid_kvm_features, plus_svm_features = 0;
> -    uint32_t plus_7_0_ebx_features = 0;
> -    /* Features to be removed */
> -    uint32_t minus_features = 0, minus_ext_features = 0;
> -    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
> -    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> -    uint32_t minus_7_0_ebx_features = 0;
> -    uint32_t numvalue;
> +    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
> +    if (error_is_set(errp)) {
> +        goto error;
> +    }
>   
>       for (def = x86_defs; def; def = def->next)
>           if (name && !strcmp(name, def->name))
> @@ -1563,127 +1463,18 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>           memcpy(x86_cpu_def, def, sizeof(*def));
>       }
>   
> -    featurestr = strtok(NULL, ",");
> -
> -    while (featurestr) {
> -        char *val;
> -        if (featurestr[0] == '+') {
> -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> -                            &plus_ext_features, &plus_ext2_features,
> -                            &plus_ext3_features, &plus_kvm_features,
> -                            &plus_svm_features, &plus_7_0_ebx_features);
> -        } else if (featurestr[0] == '-') {
> -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> -                            &minus_ext_features, &minus_ext2_features,
> -                            &minus_ext3_features, &minus_kvm_features,
> -                            &minus_svm_features, &minus_7_0_ebx_features);
> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> -            if (!strcmp(featurestr, "family")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff + 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->family = numvalue;
> -            } else if (!strcmp(featurestr, "model")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xff) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->model = numvalue;
> -            } else if (!strcmp(featurestr, "stepping")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err || numvalue > 0xf) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->stepping = numvalue ;
> -            } else if (!strcmp(featurestr, "level")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->level = numvalue;
> -            } else if (!strcmp(featurestr, "xlevel")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                if (numvalue < 0x80000000) {
> -                    numvalue += 0x80000000;
> -                }
> -                x86_cpu_def->xlevel = numvalue;
> -            } else if (!strcmp(featurestr, "vendor")) {
> -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> -                x86_cpu_def->vendor_override = 1;
> -            } else if (!strcmp(featurestr, "model_id")) {
This makes model_id= as a feature stop working.  It needs to be added to 
this patch, or maybe #10 where "tsc_freq" was handled.
> -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> -                        val);
> -            } else if (!strcmp(featurestr, "tsc_freq")) {
> -                int64_t tsc_freq;
> -                char *err;
> -
> -                tsc_freq = strtosz_suffix_unit(val, &err,
> -                                               STRTOSZ_DEFSUFFIX_B, 1000);
> -                if (tsc_freq < 0 || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> -            } else if (!strcmp(featurestr, "hv_spinlocks")) {
> -                char *err;
> -                numvalue = strtoul(val, &err, 0);
> -                if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> -                }
> -                hyperv_set_spinlock_retries(numvalue);
> -            } else {
> -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> -                goto error;
> -            }
> -        } else if (!strcmp(featurestr, "check")) {
> -            check_cpuid = 1;
> -        } else if (!strcmp(featurestr, "enforce")) {
> -            check_cpuid = enforce_cpuid = 1;
> -        } else if (!strcmp(featurestr, "hv_relaxed")) {
> -            hyperv_enable_relaxed_timing(true);
> -        } else if (!strcmp(featurestr, "hv_vapic")) {
> -            hyperv_enable_vapic_recommended(true);
> -        } else {
> -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> -            goto error;
> -        }
> -        featurestr = strtok(NULL, ",");
> +    cpu_x86_set_props(cpu, features, errp);
> +    if (error_is_set(errp)) {
> +        goto error;
>       }
> -    x86_cpu_def->features |= plus_features;
> -    x86_cpu_def->ext_features |= plus_ext_features;
> -    x86_cpu_def->ext2_features |= plus_ext2_features;
> -    x86_cpu_def->ext3_features |= plus_ext3_features;
> -    x86_cpu_def->kvm_features |= plus_kvm_features;
> -    x86_cpu_def->svm_features |= plus_svm_features;
> -    x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_features;
> -    x86_cpu_def->features &= ~minus_features;
> -    x86_cpu_def->ext_features &= ~minus_ext_features;
> -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> -    x86_cpu_def->svm_features &= ~minus_svm_features;
> -    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> -    g_free(s);
> +
> +    g_free(name);
> +    QDECREF(features);
>       return 0;
>   
>   error:
> -    g_free(s);
> +    g_free(name);
> +    QDECREF(features);
>       if (!error_is_set(errp)) {
>           error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
>       }
    -Don Slutz
Igor Mammedov - Oct. 23, 2012, 3:50 p.m.
On Tue, 23 Oct 2012 11:29:46 -0400
Don Slutz <Don@cloudswitch.com> wrote:

> On 10/22/12 11:03, Igor Mammedov wrote:
> >   - def->kvm_features was used only for setting hadcoded defaults,
> >     with defaults set by static props, we do not need it anymore,
> >     ignore it in cpudef_2_x86_cpu().
> >   - take in account CPUID_EXT_HYPERVISOR feature set by static
> >     property default value;
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >    * fix double free of features in cpu_x86_find_by_name()
> > ---
> >   target-i386/cpu.c | 271 +++++++-----------------------------------------------
> >   1 file changed, 31 insertions(+), 240 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c804965..59809f8 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -87,31 +87,6 @@ static const char *ext3_feature_name[] = {
> >       NULL, NULL, NULL, NULL,
> >   };
> >   
> > -static const char *kvm_feature_name[] = {
> > -    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -};
> > -
> > -static const char *svm_feature_name[] = {
> > -    "npt", "lbrv", "svm_lock", "nrip_save",
> > -    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> > -    NULL, NULL, "pause_filter", NULL,
> > -    "pfthreshold", NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL,
> > -};
> > -
> > -static const char *cpuid_7_0_ebx_feature_name[] = {
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, "smep",
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL, "smap", NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -};
> > -
> >   #if defined(CONFIG_KVM)
> >   static void x86_cpu_get_kvmclock(Object *obj, Visitor *v, void *opaque,
> >                                    const char *name, Error **errp)
> > @@ -752,88 +727,6 @@ void host_cpuid(uint32_t function, uint32_t count,
> >   #endif
> >   }
> >   
> > -#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
> > -
> > -/* general substring compare of *[s1..e1) and *[s2..e2).  sx is start of
> > - * a substring.  ex if !NULL points to the first char after a substring,
> > - * otherwise the string is assumed to sized by a terminating nul.
> > - * Return lexical ordering of *s1:*s2.
> > - */
> > -static int sstrcmp(const char *s1, const char *e1, const char *s2,
> > -    const char *e2)
> > -{
> > -    for (;;) {
> > -        if (!*s1 || !*s2 || *s1 != *s2)
> > -            return (*s1 - *s2);
> > -        ++s1, ++s2;
> > -        if (s1 == e1 && s2 == e2)
> > -            return (0);
> > -        else if (s1 == e1)
> > -            return (*s2);
> > -        else if (s2 == e2)
> > -            return (*s1);
> > -    }
> > -}
> > -
> > -/* compare *[s..e) to *altstr.  *altstr may be a simple string or multiple
> > - * '|' delimited (possibly empty) strings in which case search for a match
> > - * within the alternatives proceeds left to right.  Return 0 for success,
> > - * non-zero otherwise.
> > - */
> > -static int altcmp(const char *s, const char *e, const char *altstr)
> > -{
> > -    const char *p, *q;
> > -
> > -    for (q = p = altstr; ; ) {
> > -        while (*p && *p != '|')
> > -            ++p;
> > -        if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
> > -            return (0);
> > -        if (!*p)
> > -            return (1);
> > -        else
> > -            q = ++p;
> > -    }
> > -}
> > -
> > -/* search featureset for flag *[s..e), if found set corresponding bit in
> > - * *pval and return true, otherwise return false
> > - */
> > -static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
> > -                           const char **featureset)
> > -{
> > -    uint32_t mask;
> > -    const char **ppc;
> > -    bool found = false;
> > -
> > -    for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
> > -        if (*ppc && !altcmp(s, e, *ppc)) {
> > -            *pval |= mask;
> > -            found = true;
> > -        }
> > -    }
> > -    return found;
> > -}
> > -
> > -static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
> > -                                    uint32_t *ext_features,
> > -                                    uint32_t *ext2_features,
> > -                                    uint32_t *ext3_features,
> > -                                    uint32_t *kvm_features,
> > -                                    uint32_t *svm_features,
> > -                                    uint32_t *cpuid_7_0_ebx_features)
> > -{
> > -    if (!lookup_feature(features, flagname, NULL, feature_name) &&
> > -        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
> > -        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
> > -        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
> > -        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
> > -        !lookup_feature(svm_features, flagname, NULL, svm_feature_name) &&
> > -        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
> > -                        cpuid_7_0_ebx_feature_name))
> > -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> > -}
> > -
> >   typedef struct x86_def_t {
> >       struct x86_def_t *next;
> >       const char *name;
> > @@ -1432,10 +1325,10 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> >       object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> >                               "tsc-frequency", errp);
> >       env->cpuid_features = def->features;
> > -    env->cpuid_ext_features = def->ext_features;
> > +    env->cpuid_ext_features = (CPUID_EXT_HYPERVISOR & env->cpuid_ext_features) |
> > +                              def->ext_features;
> >       env->cpuid_ext2_features = def->ext2_features;
> >       env->cpuid_ext3_features = def->ext3_features;
> > -    env->cpuid_kvm_features = def->kvm_features;
> >       env->cpuid_svm_features = def->svm_features;
> >       env->cpuid_ext4_features = def->ext4_features;
> >       env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> > @@ -1532,25 +1425,32 @@ out:
> >       return;
> >   }
> >   
> > +/* Set features on X86CPU object based on a QDict */
> > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
> > +{
> > +    const QDictEntry *ent;
> > +
> > +    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
> > +        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
> > +        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
> > +                              qdict_entry_key(ent), errp);
> > +        if (error_is_set(errp)) {
> > +            return;
> > +        }
> > +    }
> > +}
> > +
> >   static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >                                   const char *cpu_model, Error **errp)
> >   {
> >       x86_def_t *def;
> > -    CPUX86State *env = &cpu->env;
> > +    QDict *features = NULL;
> > +    char *name = NULL;
> >   
> > -    char *s = g_strdup(cpu_model);
> > -    char *featurestr, *name = strtok(s, ",");
> > -    /* Features to be added*/
> > -    uint32_t plus_features = 0, plus_ext_features = env->cpuid_ext_features;
> > -    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> > -    uint32_t plus_kvm_features = env->cpuid_kvm_features, plus_svm_features = 0;
> > -    uint32_t plus_7_0_ebx_features = 0;
> > -    /* Features to be removed */
> > -    uint32_t minus_features = 0, minus_ext_features = 0;
> > -    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
> > -    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> > -    uint32_t minus_7_0_ebx_features = 0;
> > -    uint32_t numvalue;
> > +    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
> > +    if (error_is_set(errp)) {
> > +        goto error;
> > +    }
> >   
> >       for (def = x86_defs; def; def = def->next)
> >           if (name && !strcmp(name, def->name))
> > @@ -1563,127 +1463,18 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >           memcpy(x86_cpu_def, def, sizeof(*def));
> >       }
> >   
> > -    featurestr = strtok(NULL, ",");
> > -
> > -    while (featurestr) {
> > -        char *val;
> > -        if (featurestr[0] == '+') {
> > -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > -                            &plus_ext_features, &plus_ext2_features,
> > -                            &plus_ext3_features, &plus_kvm_features,
> > -                            &plus_svm_features, &plus_7_0_ebx_features);
> > -        } else if (featurestr[0] == '-') {
> > -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> > -                            &minus_ext_features, &minus_ext2_features,
> > -                            &minus_ext3_features, &minus_kvm_features,
> > -                            &minus_svm_features, &minus_7_0_ebx_features);
> > -        } else if ((val = strchr(featurestr, '='))) {
> > -            *val = 0; val++;
> > -            if (!strcmp(featurestr, "family")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xff + 0xf) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->family = numvalue;
> > -            } else if (!strcmp(featurestr, "model")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xff) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->model = numvalue;
> > -            } else if (!strcmp(featurestr, "stepping")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err || numvalue > 0xf) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->stepping = numvalue ;
> > -            } else if (!strcmp(featurestr, "level")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->level = numvalue;
> > -            } else if (!strcmp(featurestr, "xlevel")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                if (numvalue < 0x80000000) {
> > -                    numvalue += 0x80000000;
> > -                }
> > -                x86_cpu_def->xlevel = numvalue;
> > -            } else if (!strcmp(featurestr, "vendor")) {
> > -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> > -                x86_cpu_def->vendor_override = 1;
> > -            } else if (!strcmp(featurestr, "model_id")) {
> This makes model_id= as a feature stop working.  It needs to be added to 
> this patch, or maybe #10 where "tsc_freq" was handled.
Thanks for finding this one, I'll put it in compat_normalize_cpu_model()
alongside with "tsc_freq" legacy name handling.

> > -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> > -                        val);
> > -            } else if (!strcmp(featurestr, "tsc_freq")) {
> > -                int64_t tsc_freq;
> > -                char *err;
> > -
> > -                tsc_freq = strtosz_suffix_unit(val, &err,
> > -                                               STRTOSZ_DEFSUFFIX_B, 1000);
> > -                if (tsc_freq < 0 || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > -            } else if (!strcmp(featurestr, "hv_spinlocks")) {
> > -                char *err;
> > -                numvalue = strtoul(val, &err, 0);
> > -                if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > -                }
> > -                hyperv_set_spinlock_retries(numvalue);
> > -            } else {
> > -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> > -                goto error;
> > -            }
> > -        } else if (!strcmp(featurestr, "check")) {
> > -            check_cpuid = 1;
> > -        } else if (!strcmp(featurestr, "enforce")) {
> > -            check_cpuid = enforce_cpuid = 1;
> > -        } else if (!strcmp(featurestr, "hv_relaxed")) {
> > -            hyperv_enable_relaxed_timing(true);
> > -        } else if (!strcmp(featurestr, "hv_vapic")) {
> > -            hyperv_enable_vapic_recommended(true);
> > -        } else {
> > -            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> > -            goto error;
> > -        }
> > -        featurestr = strtok(NULL, ",");
> > +    cpu_x86_set_props(cpu, features, errp);
> > +    if (error_is_set(errp)) {
> > +        goto error;
> >       }
> > -    x86_cpu_def->features |= plus_features;
> > -    x86_cpu_def->ext_features |= plus_ext_features;
> > -    x86_cpu_def->ext2_features |= plus_ext2_features;
> > -    x86_cpu_def->ext3_features |= plus_ext3_features;
> > -    x86_cpu_def->kvm_features |= plus_kvm_features;
> > -    x86_cpu_def->svm_features |= plus_svm_features;
> > -    x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_features;
> > -    x86_cpu_def->features &= ~minus_features;
> > -    x86_cpu_def->ext_features &= ~minus_ext_features;
> > -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> > -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> > -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> > -    x86_cpu_def->svm_features &= ~minus_svm_features;
> > -    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> > -    g_free(s);
> > +
> > +    g_free(name);
> > +    QDECREF(features);
> >       return 0;
> >   
> >   error:
> > -    g_free(s);
> > +    g_free(name);
> > +    QDECREF(features);
> >       if (!error_is_set(errp)) {
> >           error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> >       }
>     -Don Slutz

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c804965..59809f8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -87,31 +87,6 @@  static const char *ext3_feature_name[] = {
     NULL, NULL, NULL, NULL,
 };
 
-static const char *kvm_feature_name[] = {
-    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-};
-
-static const char *svm_feature_name[] = {
-    "npt", "lbrv", "svm_lock", "nrip_save",
-    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-    NULL, NULL, "pause_filter", NULL,
-    "pfthreshold", NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL,
-};
-
-static const char *cpuid_7_0_ebx_feature_name[] = {
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, "smep",
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL, "smap", NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-};
-
 #if defined(CONFIG_KVM)
 static void x86_cpu_get_kvmclock(Object *obj, Visitor *v, void *opaque,
                                  const char *name, Error **errp)
@@ -752,88 +727,6 @@  void host_cpuid(uint32_t function, uint32_t count,
 #endif
 }
 
-#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
-
-/* general substring compare of *[s1..e1) and *[s2..e2).  sx is start of
- * a substring.  ex if !NULL points to the first char after a substring,
- * otherwise the string is assumed to sized by a terminating nul.
- * Return lexical ordering of *s1:*s2.
- */
-static int sstrcmp(const char *s1, const char *e1, const char *s2,
-    const char *e2)
-{
-    for (;;) {
-        if (!*s1 || !*s2 || *s1 != *s2)
-            return (*s1 - *s2);
-        ++s1, ++s2;
-        if (s1 == e1 && s2 == e2)
-            return (0);
-        else if (s1 == e1)
-            return (*s2);
-        else if (s2 == e2)
-            return (*s1);
-    }
-}
-
-/* compare *[s..e) to *altstr.  *altstr may be a simple string or multiple
- * '|' delimited (possibly empty) strings in which case search for a match
- * within the alternatives proceeds left to right.  Return 0 for success,
- * non-zero otherwise.
- */
-static int altcmp(const char *s, const char *e, const char *altstr)
-{
-    const char *p, *q;
-
-    for (q = p = altstr; ; ) {
-        while (*p && *p != '|')
-            ++p;
-        if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
-            return (0);
-        if (!*p)
-            return (1);
-        else
-            q = ++p;
-    }
-}
-
-/* search featureset for flag *[s..e), if found set corresponding bit in
- * *pval and return true, otherwise return false
- */
-static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
-                           const char **featureset)
-{
-    uint32_t mask;
-    const char **ppc;
-    bool found = false;
-
-    for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
-        if (*ppc && !altcmp(s, e, *ppc)) {
-            *pval |= mask;
-            found = true;
-        }
-    }
-    return found;
-}
-
-static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
-                                    uint32_t *ext_features,
-                                    uint32_t *ext2_features,
-                                    uint32_t *ext3_features,
-                                    uint32_t *kvm_features,
-                                    uint32_t *svm_features,
-                                    uint32_t *cpuid_7_0_ebx_features)
-{
-    if (!lookup_feature(features, flagname, NULL, feature_name) &&
-        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
-        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
-        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
-        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
-        !lookup_feature(svm_features, flagname, NULL, svm_feature_name) &&
-        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
-                        cpuid_7_0_ebx_feature_name))
-            fprintf(stderr, "CPU feature %s not found\n", flagname);
-}
-
 typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
@@ -1432,10 +1325,10 @@  static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
                             "tsc-frequency", errp);
     env->cpuid_features = def->features;
-    env->cpuid_ext_features = def->ext_features;
+    env->cpuid_ext_features = (CPUID_EXT_HYPERVISOR & env->cpuid_ext_features) |
+                              def->ext_features;
     env->cpuid_ext2_features = def->ext2_features;
     env->cpuid_ext3_features = def->ext3_features;
-    env->cpuid_kvm_features = def->kvm_features;
     env->cpuid_svm_features = def->svm_features;
     env->cpuid_ext4_features = def->ext4_features;
     env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
@@ -1532,25 +1425,32 @@  out:
     return;
 }
 
+/* Set features on X86CPU object based on a QDict */
+static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
+        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
+        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
+                              qdict_entry_key(ent), errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
+}
+
 static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *cpu_model, Error **errp)
 {
     x86_def_t *def;
-    CPUX86State *env = &cpu->env;
+    QDict *features = NULL;
+    char *name = NULL;
 
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
-    /* Features to be added*/
-    uint32_t plus_features = 0, plus_ext_features = env->cpuid_ext_features;
-    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = env->cpuid_kvm_features, plus_svm_features = 0;
-    uint32_t plus_7_0_ebx_features = 0;
-    /* Features to be removed */
-    uint32_t minus_features = 0, minus_ext_features = 0;
-    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
-    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
-    uint32_t minus_7_0_ebx_features = 0;
-    uint32_t numvalue;
+    compat_normalize_cpu_model(cpu_model, &name, &features, errp);
+    if (error_is_set(errp)) {
+        goto error;
+    }
 
     for (def = x86_defs; def; def = def->next)
         if (name && !strcmp(name, def->name))
@@ -1563,127 +1463,18 @@  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
-    featurestr = strtok(NULL, ",");
-
-    while (featurestr) {
-        char *val;
-        if (featurestr[0] == '+') {
-            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
-                            &plus_ext_features, &plus_ext2_features,
-                            &plus_ext3_features, &plus_kvm_features,
-                            &plus_svm_features, &plus_7_0_ebx_features);
-        } else if (featurestr[0] == '-') {
-            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
-                            &minus_ext_features, &minus_ext2_features,
-                            &minus_ext3_features, &minus_kvm_features,
-                            &minus_svm_features, &minus_7_0_ebx_features);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
-            if (!strcmp(featurestr, "family")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff + 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->family = numvalue;
-            } else if (!strcmp(featurestr, "model")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->model = numvalue;
-            } else if (!strcmp(featurestr, "stepping")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->stepping = numvalue ;
-            } else if (!strcmp(featurestr, "level")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->level = numvalue;
-            } else if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                if (numvalue < 0x80000000) {
-                    numvalue += 0x80000000;
-                }
-                x86_cpu_def->xlevel = numvalue;
-            } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
-            } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
-            } else if (!strcmp(featurestr, "tsc_freq")) {
-                int64_t tsc_freq;
-                char *err;
-
-                tsc_freq = strtosz_suffix_unit(val, &err,
-                                               STRTOSZ_DEFSUFFIX_B, 1000);
-                if (tsc_freq < 0 || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
-            } else if (!strcmp(featurestr, "hv_spinlocks")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                hyperv_set_spinlock_retries(numvalue);
-            } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
-                goto error;
-            }
-        } else if (!strcmp(featurestr, "check")) {
-            check_cpuid = 1;
-        } else if (!strcmp(featurestr, "enforce")) {
-            check_cpuid = enforce_cpuid = 1;
-        } else if (!strcmp(featurestr, "hv_relaxed")) {
-            hyperv_enable_relaxed_timing(true);
-        } else if (!strcmp(featurestr, "hv_vapic")) {
-            hyperv_enable_vapic_recommended(true);
-        } else {
-            fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
-            goto error;
-        }
-        featurestr = strtok(NULL, ",");
+    cpu_x86_set_props(cpu, features, errp);
+    if (error_is_set(errp)) {
+        goto error;
     }
-    x86_cpu_def->features |= plus_features;
-    x86_cpu_def->ext_features |= plus_ext_features;
-    x86_cpu_def->ext2_features |= plus_ext2_features;
-    x86_cpu_def->ext3_features |= plus_ext3_features;
-    x86_cpu_def->kvm_features |= plus_kvm_features;
-    x86_cpu_def->svm_features |= plus_svm_features;
-    x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_features;
-    x86_cpu_def->features &= ~minus_features;
-    x86_cpu_def->ext_features &= ~minus_ext_features;
-    x86_cpu_def->ext2_features &= ~minus_ext2_features;
-    x86_cpu_def->ext3_features &= ~minus_ext3_features;
-    x86_cpu_def->kvm_features &= ~minus_kvm_features;
-    x86_cpu_def->svm_features &= ~minus_svm_features;
-    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
-    g_free(s);
+
+    g_free(name);
+    QDECREF(features);
     return 0;
 
 error:
-    g_free(s);
+    g_free(name);
+    QDECREF(features);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }