Patchwork [RFC,18/20] target-i386: use properties to set/unset user specified features on CPU

login
register
mail settings
Submitter Igor Mammedov
Date Aug. 10, 2012, 11:22 a.m.
Message ID <1344597756-2890-19-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/176476/
State New
Headers show

Comments

Igor Mammedov - Aug. 10, 2012, 11:22 a.m.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 167 +++++++-----------------------------------------------
 1 file changed, 20 insertions(+), 147 deletions(-)
Eduardo Habkost - Aug. 13, 2012, 8:48 p.m.
On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 167 +++++++-----------------------------------------------
>  1 file changed, 20 insertions(+), 147 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7555b08..0174c4d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -206,22 +206,6 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
>      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)
> -{
> -    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))
> -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> -}
> -
>  typedef struct x86_def_t {
>      struct x86_def_t *next;
>      const char *name;
> @@ -1104,7 +1088,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>      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 = def->cpuid_7_0_ebx_features;
> @@ -1168,17 +1151,14 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>  {
>      x86_def_t *def;
>  
> -    char *s = g_strdup(cpu_model);
> -    char *featurestr, *name = strtok(s, ",");
> -    /* Features to be added*/
> -    uint32_t plus_features = 0, plus_ext_features = 0;
> -    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> -    uint32_t plus_kvm_features = 0, plus_svm_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 numvalue;
> +    QDict *features;
> +    const QDictEntry *ent;
> +    char *name;
> +
> +    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))
> @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>          memcpy(x86_cpu_def, def, sizeof(*def));
>      }
>  
> -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> -
> -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -        &plus_kvm_features, &plus_svm_features);
> +    cpudef_2_x86_cpu(cpu, def, errp);

It looks like you have to use 'x86_cpu_def' instead of 'def', here, as
'def' is the variable for the x86_defs name-lookup loop, only,
'x86_cpu_def' is the actual CPU definition.


>  
> -    featurestr = strtok(NULL, ",");
> +    /* not supported bits will be filtered out later */
> +    env->cpuid_kvm_features = ~0;
>  
> -    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);
> -        } 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);
> -        } 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;
> -                }
> -                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;
> -        }
> +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
>  
> +    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)) {
>              goto error;
>          }
> -
> -        featurestr = strtok(NULL, ",");
>      }
> -    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->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;
> -    g_free(s);
> +    QDECREF(features);
> +
> +    g_free(name);
>      return 0;
>  
>  error:
> -    g_free(s);
> +    g_free(name);
>      if (!error_is_set(errp)) {
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
>      }
> @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
>          goto out;
>  
> -    cpudef_2_x86_cpu(cpu, def, &error);
> -

You could move the 'def' and 'def1' variables inside
cpu_x86_find_by_name(), now, as they are now only used inside that
function.

>  out:
>      if (error_is_set(&error)) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> -- 
> 1.7.11.2
> 
>
Igor Mammedov - Aug. 14, 2012, 10:55 p.m.
On Mon, 13 Aug 2012 17:48:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 167 +++++++-----------------------------------------------
> >  1 file changed, 20 insertions(+), 147 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7555b08..0174c4d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -206,22 +206,6 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
> >      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)
> > -{
> > -    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))
> > -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> > -}
> > -
> >  typedef struct x86_def_t {
> >      struct x86_def_t *next;
> >      const char *name;
> > @@ -1104,7 +1088,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> >      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 = def->cpuid_7_0_ebx_features;
> > @@ -1168,17 +1151,14 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >  {
> >      x86_def_t *def;
> >  
> > -    char *s = g_strdup(cpu_model);
> > -    char *featurestr, *name = strtok(s, ",");
> > -    /* Features to be added*/
> > -    uint32_t plus_features = 0, plus_ext_features = 0;
> > -    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> > -    uint32_t plus_kvm_features = 0, plus_svm_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 numvalue;
> > +    QDict *features;
> > +    const QDictEntry *ent;
> > +    char *name;
> > +
> > +    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))
> > @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >          memcpy(x86_cpu_def, def, sizeof(*def));
> >      }
> >  
> > -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> > -
> > -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> > -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> > -        &plus_kvm_features, &plus_svm_features);
> > +    cpudef_2_x86_cpu(cpu, def, errp);
> 
> It looks like you have to use 'x86_cpu_def' instead of 'def', here, as
> 'def' is the variable for the x86_defs name-lookup loop, only,
> 'x86_cpu_def' is the actual CPU definition.
yep, I'll fix it.

> 
> 
> >  
> > -    featurestr = strtok(NULL, ",");
> > +    /* not supported bits will be filtered out later */
> > +    env->cpuid_kvm_features = ~0;
> >  
> > -    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);
> > -        } 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);
> > -        } 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;
> > -                }
> > -                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;
> > -        }
> > +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> >  
> > +    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)) {
> >              goto error;
> >          }
> > -
> > -        featurestr = strtok(NULL, ",");
> >      }
> > -    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->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;
> > -    g_free(s);
> > +    QDECREF(features);
> > +
> > +    g_free(name);
> >      return 0;
> >  
> >  error:
> > -    g_free(s);
> > +    g_free(name);
> >      if (!error_is_set(errp)) {
> >          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> >      }
> > @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
> >          goto out;
> >  
> > -    cpudef_2_x86_cpu(cpu, def, &error);
> > -

> 
> You could move the 'def' and 'def1' variables inside
> cpu_x86_find_by_name(), now, as they are now only used inside that
> function.

it's better stay here, I'll make another patch that will leave only def lookup
in cpu_x86_find_by_name() and move the rest in cpu_x86_register().
with subclasses cpu_x86_find_by_name() should go away and cpu_x86_register()
could be disbanded into pieces as it was shown in [NOT RFC] patch I've posted
recently.

and if you do not object I'd take your "create cpu_x86_set_props() function"
patch from
https://github.com/ehabkost/qemu-hacks/commit/4d734b316625d0b689031460328ce6e531be5b90

> 
> >  out:
> >      if (error_is_set(&error)) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > -- 
> > 1.7.11.2
> > 
> > 
> 
> -- 
> Eduardo
Eduardo Habkost - Aug. 15, 2012, 12:37 p.m.
Another issue:

On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 167 +++++++-----------------------------------------------
>  1 file changed, 20 insertions(+), 147 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7555b08..0174c4d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
[...]
> @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>          memcpy(x86_cpu_def, def, sizeof(*def));
>      }
>  
> -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> -
> -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -        &plus_kvm_features, &plus_svm_features);
> +    cpudef_2_x86_cpu(cpu, def, errp);
>  
> -    featurestr = strtok(NULL, ",");
> +    /* not supported bits will be filtered out later */
> +    env->cpuid_kvm_features = ~0;

This doesn't compile. The 'env' variable doesn't exist inside
cpu_x86_find_by_name().

>  
> -    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);
> -        } 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);
> -        } 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;
> -                }
> -                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;
> -        }
> +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
>  
> +    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)) {
>              goto error;
>          }
> -
> -        featurestr = strtok(NULL, ",");
>      }
> -    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->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;
> -    g_free(s);
> +    QDECREF(features);
> +
> +    g_free(name);
>      return 0;
>  
>  error:
> -    g_free(s);
> +    g_free(name);
>      if (!error_is_set(errp)) {
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
>      }
> @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
>          goto out;
>  
> -    cpudef_2_x86_cpu(cpu, def, &error);
> -
>  out:
>      if (error_is_set(&error)) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> -- 
> 1.7.11.2
> 
>
Igor Mammedov - Aug. 15, 2012, 12:41 p.m.
On Wed, 15 Aug 2012 09:37:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Another issue:
> 
> On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 167 +++++++-----------------------------------------------
> >  1 file changed, 20 insertions(+), 147 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7555b08..0174c4d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> [...]
> > @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >          memcpy(x86_cpu_def, def, sizeof(*def));
> >      }
> >  
> > -    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> > -
> > -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> > -        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> > -        &plus_kvm_features, &plus_svm_features);
> > +    cpudef_2_x86_cpu(cpu, def, errp);
> >  
> > -    featurestr = strtok(NULL, ",");
> > +    /* not supported bits will be filtered out later */
> > +    env->cpuid_kvm_features = ~0;
> 
> This doesn't compile. The 'env' variable doesn't exist inside
> cpu_x86_find_by_name().
Thanks, it's fixed now.
> 
> >  
> > -    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);
> > -        } 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);
> > -        } 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;
> > -                }
> > -                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;
> > -        }
> > +    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> >  
> > +    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)) {
> >              goto error;
> >          }
> > -
> > -        featurestr = strtok(NULL, ",");
> >      }
> > -    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->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;
> > -    g_free(s);
> > +    QDECREF(features);
> > +
> > +    g_free(name);
> >      return 0;
> >  
> >  error:
> > -    g_free(s);
> > +    g_free(name);
> >      if (!error_is_set(errp)) {
> >          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> >      }
> > @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
> >          goto out;
> >  
> > -    cpudef_2_x86_cpu(cpu, def, &error);
> > -
> >  out:
> >      if (error_is_set(&error)) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > -- 
> > 1.7.11.2
> > 
> > 
> 
> -- 
> Eduardo

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7555b08..0174c4d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -206,22 +206,6 @@  static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
     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)
-{
-    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))
-            fprintf(stderr, "CPU feature %s not found\n", flagname);
-}
-
 typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
@@ -1104,7 +1088,6 @@  static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
     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 = def->cpuid_7_0_ebx_features;
@@ -1168,17 +1151,14 @@  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
 {
     x86_def_t *def;
 
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
-    /* Features to be added*/
-    uint32_t plus_features = 0, plus_ext_features = 0;
-    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = 0, plus_svm_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 numvalue;
+    QDict *features;
+    const QDictEntry *ent;
+    char *name;
+
+    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))
@@ -1191,133 +1171,28 @@  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         memcpy(x86_cpu_def, def, sizeof(*def));
     }
 
-    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
-
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-        &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features, &plus_svm_features);
+    cpudef_2_x86_cpu(cpu, def, errp);
 
-    featurestr = strtok(NULL, ",");
+    /* not supported bits will be filtered out later */
+    env->cpuid_kvm_features = ~0;
 
-    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);
-        } 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);
-        } 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;
-                }
-                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;
-        }
+    object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
 
+    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)) {
             goto error;
         }
-
-        featurestr = strtok(NULL, ",");
     }
-    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->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;
-    g_free(s);
+    QDECREF(features);
+
+    g_free(name);
     return 0;
 
 error:
-    g_free(s);
+    g_free(name);
     if (!error_is_set(errp)) {
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
     }
@@ -1427,8 +1302,6 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
         goto out;
 
-    cpudef_2_x86_cpu(cpu, def, &error);
-
 out:
     if (error_is_set(&error)) {
         fprintf(stderr, "%s\n", error_get_pretty(error));