Patchwork [4/5] target-i386: set custom features/properties without intermediate x86_def_t

login
register
mail settings
Submitter Igor Mammedov
Date Jan. 21, 2013, 2:06 p.m.
Message ID <1358777199-5735-5-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/214183/
State New
Headers show

Comments

Igor Mammedov - Jan. 21, 2013, 2:06 p.m.
Move custom features parsing after built-in cpu_model defaults are set
and set custom features directly on CPU instance. That allows to make
clear distinction between built-in cpu model defaults that eventually
should go into clas_init() and extra property setting which is done
after defaults are set on CPU instance.

Impl. details:
   * use object_property_parse() property setter so it would be mechanical
     change to switch to global properties later.
   * And after all current features/properties are converted into static
     properties, it will take a trivial patch to switch to global properties.
     Which will allow to:
     * get CPU instance initialized with all parameters passed on -cpu ...
       cmd. line from object_new() call.
     * call cpu_model/featurestr parsing only once before CPUs are created
     * open a road for removing CPUxxxState.cpu_model_str field, when other
       CPUs are similarly converted to subclasses and static properties.
 - re-factor error handling, to use Error instead of fprintf()s, since
   it is anyway passed in for property setter.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
    - removed temporary list of properties.
        Suggested-By: Eduardo Habkost <ehabkost@redhat.com>
    - check if error is set before cpu_x86_parse_featurestr() is called
      to avoid assert in error_setg().
---
 target-i386/cpu.c |  118 +++++++++++++++++++++++------------------------------
 1 files changed, 51 insertions(+), 67 deletions(-)
Andreas Färber - Jan. 26, 2013, 3:26 p.m.
Am 21.01.2013 15:06, schrieb Igor Mammedov:
> Move custom features parsing after built-in cpu_model defaults are set
> and set custom features directly on CPU instance. That allows to make
> clear distinction between built-in cpu model defaults that eventually
> should go into clas_init() and extra property setting which is done
> after defaults are set on CPU instance.
> 
> Impl. details:
>    * use object_property_parse() property setter so it would be mechanical
>      change to switch to global properties later.
>    * And after all current features/properties are converted into static
>      properties, it will take a trivial patch to switch to global properties.
>      Which will allow to:
>      * get CPU instance initialized with all parameters passed on -cpu ...
>        cmd. line from object_new() call.
>      * call cpu_model/featurestr parsing only once before CPUs are created
>      * open a road for removing CPUxxxState.cpu_model_str field, when other
>        CPUs are similarly converted to subclasses and static properties.
>  - re-factor error handling, to use Error instead of fprintf()s, since
>    it is anyway passed in for property setter.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v2:
>     - removed temporary list of properties.
>         Suggested-By: Eduardo Habkost <ehabkost@redhat.com>
>     - check if error is set before cpu_x86_parse_featurestr() is called
>       to avoid assert in error_setg().
> ---
>  target-i386/cpu.c |  118 +++++++++++++++++++++++------------------------------
>  1 files changed, 51 insertions(+), 67 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 768ca69..640cc8f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1299,7 +1299,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> +static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
>  {
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
> @@ -1307,6 +1307,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      /* Features to be removed */
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
> +    CPUX86State *env = &cpu->env;
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
> @@ -1319,77 +1320,57 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
> +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
>              } 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;
> +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
>              } 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 ;
> +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
>              } 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;
> +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
>              } else if (!strcmp(featurestr, "xlevel")) {
>                  char *err;
> +                char num[32];
> +
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
> -                    fprintf(stderr, "bad numerical value %s\n", val);
> -                    goto error;
> +                    error_setg(errp, "bad numerical value %s\n", val);
> +                    goto out;
>                  }
>                  if (numvalue < 0x80000000) {
>                      fprintf(stderr, "xlevel value shall always be >= 0x80000000"
>                              ", fixup will be removed in future versions\n");
>                      numvalue += 0x80000000;
>                  }
> -                x86_cpu_def->xlevel = numvalue;
> +                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> +                object_property_parse(OBJECT(cpu), num, featurestr, errp);
>              } else if (!strcmp(featurestr, "vendor")) {
> -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
>              } else if (!strcmp(featurestr, "model_id")) {
> -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> -                        val);
> +                object_property_parse(OBJECT(cpu), val, "model-id", errp);
>              } else if (!strcmp(featurestr, "tsc_freq")) {
>                  int64_t tsc_freq;
>                  char *err;
> +                char num[32];
>  
>                  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;
> +                    error_setg(errp, "bad numerical value %s\n", val);
> +                    goto out;
>                  }
> -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> +                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> +                object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp);
>              } 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;
> +                    error_setg(errp, "bad numerical value %s\n", val);
> +                    goto out;
>                  }
>                  hyperv_set_spinlock_retries(numvalue);
>              } else {
> -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> -                goto error;
> +                error_setg(errp, "unrecognized feature %s\n", featurestr);
> +                goto out;
>              }
>          } else if (!strcmp(featurestr, "check")) {
>              check_cpuid = 1;
> @@ -1400,31 +1381,34 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>          } 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;
> +            error_setg(errp, "feature string `%s' not in format (+feature|"
> +                       "-feature|feature=xyz)\n", featurestr);
> +            goto out;
> +        }
> +        if (error_is_set(errp)) {

Note that such error_is_set() is dangerous in that it relies on errp
being non-NULL for expected behavior. If we want to call this outside of
cpu_x86_register(), please follow-up with an Error *local_err and
error_propagate().

> +            goto out;

Any reason not to just do "return;" here and above?

Andreas

>          }
>          featurestr = strtok(NULL, ",");
>      }
> -    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
> -    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
> -    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
> -    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
> -    x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX];
> -    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
> -    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
> -    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> -    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
> -    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
> -    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> -    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> -    x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
> -    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
> -    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
> -    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> -    return 0;
> +    env->cpuid_features |= plus_features[FEAT_1_EDX];
> +    env->cpuid_ext_features |= plus_features[FEAT_1_ECX];
> +    env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX];
> +    env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX];
> +    env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX];
> +    env->cpuid_kvm_features |= plus_features[FEAT_KVM];
> +    env->cpuid_svm_features |= plus_features[FEAT_SVM];
> +    env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> +    env->cpuid_features &= ~minus_features[FEAT_1_EDX];
> +    env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX];
> +    env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> +    env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> +    env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
> +    env->cpuid_kvm_features &= ~minus_features[FEAT_KVM];
> +    env->cpuid_svm_features &= ~minus_features[FEAT_SVM];
> +    env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
>  
> -error:
> -    return -1;
> +out:
> +    return;
>  }
>  
>  /* generate a composite string into buf of all cpuid names in featureset
> @@ -1556,10 +1540,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      }
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
> -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> -        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> -        goto out;
> -    }
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
>      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
>      object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> @@ -1579,7 +1559,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              "tsc-frequency", &error);
>  
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +    if (error) {
> +        goto out;
> +    }
>  
> +    cpu_x86_parse_featurestr(cpu, features, &error);
>  out:
>      g_strfreev(model_pieces);
>      if (error) {
>
Igor Mammedov - Jan. 28, 2013, 10:31 a.m.
On Sat, 26 Jan 2013 16:26:07 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 21.01.2013 15:06, schrieb Igor Mammedov:
> > Move custom features parsing after built-in cpu_model defaults are set
> > and set custom features directly on CPU instance. That allows to make
> > clear distinction between built-in cpu model defaults that eventually
> > should go into clas_init() and extra property setting which is done
> > after defaults are set on CPU instance.
> > 
> > Impl. details:
> >    * use object_property_parse() property setter so it would be mechanical
> >      change to switch to global properties later.
> >    * And after all current features/properties are converted into static
> >      properties, it will take a trivial patch to switch to global properties.
> >      Which will allow to:
> >      * get CPU instance initialized with all parameters passed on -cpu ...
> >        cmd. line from object_new() call.
> >      * call cpu_model/featurestr parsing only once before CPUs are created
> >      * open a road for removing CPUxxxState.cpu_model_str field, when other
> >        CPUs are similarly converted to subclasses and static properties.
> >  - re-factor error handling, to use Error instead of fprintf()s, since
> >    it is anyway passed in for property setter.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v2:
> >     - removed temporary list of properties.
> >         Suggested-By: Eduardo Habkost <ehabkost@redhat.com>
> >     - check if error is set before cpu_x86_parse_featurestr() is called
> >       to avoid assert in error_setg().
> > ---
> >  target-i386/cpu.c |  118 +++++++++++++++++++++++------------------------------
> >  1 files changed, 51 insertions(+), 67 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 768ca69..640cc8f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1299,7 +1299,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> >  
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
> > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > +static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
> >  {
> >      char *featurestr; /* Single 'key=value" string being parsed */
> >      /* Features to be added */
> > @@ -1307,6 +1307,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >      /* Features to be removed */
> >      FeatureWordArray minus_features = { 0 };
> >      uint32_t numvalue;
> > +    CPUX86State *env = &cpu->env;
> >  
> >      featurestr = features ? strtok(features, ",") : NULL;
> >  
> > @@ -1319,77 +1320,57 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
> > +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> >              } 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;
> > +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> >              } 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 ;
> > +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> >              } 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;
> > +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> >              } else if (!strcmp(featurestr, "xlevel")) {
> >                  char *err;
> > +                char num[32];
> > +
> >                  numvalue = strtoul(val, &err, 0);
> >                  if (!*val || *err) {
> > -                    fprintf(stderr, "bad numerical value %s\n", val);
> > -                    goto error;
> > +                    error_setg(errp, "bad numerical value %s\n", val);
> > +                    goto out;
> >                  }
> >                  if (numvalue < 0x80000000) {
> >                      fprintf(stderr, "xlevel value shall always be >= 0x80000000"
> >                              ", fixup will be removed in future versions\n");
> >                      numvalue += 0x80000000;
> >                  }
> > -                x86_cpu_def->xlevel = numvalue;
> > +                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > +                object_property_parse(OBJECT(cpu), num, featurestr, errp);
> >              } else if (!strcmp(featurestr, "vendor")) {
> > -                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> > +                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> >              } else if (!strcmp(featurestr, "model_id")) {
> > -                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> > -                        val);
> > +                object_property_parse(OBJECT(cpu), val, "model-id", errp);
> >              } else if (!strcmp(featurestr, "tsc_freq")) {
> >                  int64_t tsc_freq;
> >                  char *err;
> > +                char num[32];
> >  
> >                  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;
> > +                    error_setg(errp, "bad numerical value %s\n", val);
> > +                    goto out;
> >                  }
> > -                x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > +                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > +                object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp);
> >              } 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;
> > +                    error_setg(errp, "bad numerical value %s\n", val);
> > +                    goto out;
> >                  }
> >                  hyperv_set_spinlock_retries(numvalue);
> >              } else {
> > -                fprintf(stderr, "unrecognized feature %s\n", featurestr);
> > -                goto error;
> > +                error_setg(errp, "unrecognized feature %s\n", featurestr);
> > +                goto out;
> >              }
> >          } else if (!strcmp(featurestr, "check")) {
> >              check_cpuid = 1;
> > @@ -1400,31 +1381,34 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >          } 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;
> > +            error_setg(errp, "feature string `%s' not in format (+feature|"
> > +                       "-feature|feature=xyz)\n", featurestr);
> > +            goto out;
> > +        }
> > +        if (error_is_set(errp)) {
> 
> Note that such error_is_set() is dangerous in that it relies on errp
> being non-NULL for expected behavior. If we want to call this outside of
> cpu_x86_register(), please follow-up with an Error *local_err and
> error_propagate().
Sure, I'll follow up with patch.

> 
> > +            goto out;
> 
> Any reason not to just do "return;" here and above?
we might need cleanup out path, when converting flags features to properties
in case qdict is used as a temporary var to maintain '-foo' behavior.   

> Andreas
> 
> >          }
> >          featurestr = strtok(NULL, ",");
> >      }
> > -    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
> > -    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
> > -    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
> > -    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
> > -    x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX];
> > -    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
> > -    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
> > -    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> > -    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
> > -    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
> > -    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> > -    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> > -    x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
> > -    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
> > -    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
> > -    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> > -    return 0;
> > +    env->cpuid_features |= plus_features[FEAT_1_EDX];
> > +    env->cpuid_ext_features |= plus_features[FEAT_1_ECX];
> > +    env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX];
> > +    env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX];
> > +    env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX];
> > +    env->cpuid_kvm_features |= plus_features[FEAT_KVM];
> > +    env->cpuid_svm_features |= plus_features[FEAT_SVM];
> > +    env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> > +    env->cpuid_features &= ~minus_features[FEAT_1_EDX];
> > +    env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX];
> > +    env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> > +    env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> > +    env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
> > +    env->cpuid_kvm_features &= ~minus_features[FEAT_KVM];
> > +    env->cpuid_svm_features &= ~minus_features[FEAT_SVM];
> > +    env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
> >  
> > -error:
> > -    return -1;
> > +out:
> > +    return;
> >  }
> >  
> >  /* generate a composite string into buf of all cpuid names in featureset
> > @@ -1556,10 +1540,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      }
> >      def->ext_features |= CPUID_EXT_HYPERVISOR;
> >  
> > -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> > -        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> > -        goto out;
> > -    }
> >      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> >      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> >      object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> > @@ -1579,7 +1559,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >                              "tsc-frequency", &error);
> >  
> >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> > +    if (error) {
> > +        goto out;
> > +    }
> >  
> > +    cpu_x86_parse_featurestr(cpu, features, &error);
> >  out:
> >      g_strfreev(model_pieces);
> >      if (error) {
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 768ca69..640cc8f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1299,7 +1299,7 @@  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
+static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
 {
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
@@ -1307,6 +1307,7 @@  static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     /* Features to be removed */
     FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
+    CPUX86State *env = &cpu->env;
 
     featurestr = features ? strtok(features, ",") : NULL;
 
@@ -1319,77 +1320,57 @@  static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
+                object_property_parse(OBJECT(cpu), val, featurestr, errp);
             } 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;
+                object_property_parse(OBJECT(cpu), val, featurestr, errp);
             } 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 ;
+                object_property_parse(OBJECT(cpu), val, featurestr, errp);
             } 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;
+                object_property_parse(OBJECT(cpu), val, featurestr, errp);
             } else if (!strcmp(featurestr, "xlevel")) {
                 char *err;
+                char num[32];
+
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s\n", val);
+                    goto out;
                 }
                 if (numvalue < 0x80000000) {
                     fprintf(stderr, "xlevel value shall always be >= 0x80000000"
                             ", fixup will be removed in future versions\n");
                     numvalue += 0x80000000;
                 }
-                x86_cpu_def->xlevel = numvalue;
+                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
+                object_property_parse(OBJECT(cpu), num, featurestr, errp);
             } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
+                object_property_parse(OBJECT(cpu), val, featurestr, errp);
             } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
+                object_property_parse(OBJECT(cpu), val, "model-id", errp);
             } else if (!strcmp(featurestr, "tsc_freq")) {
                 int64_t tsc_freq;
                 char *err;
+                char num[32];
 
                 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;
+                    error_setg(errp, "bad numerical value %s\n", val);
+                    goto out;
                 }
-                x86_cpu_def->tsc_khz = tsc_freq / 1000;
+                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
+                object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp);
             } 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;
+                    error_setg(errp, "bad numerical value %s\n", val);
+                    goto out;
                 }
                 hyperv_set_spinlock_retries(numvalue);
             } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
-                goto error;
+                error_setg(errp, "unrecognized feature %s\n", featurestr);
+                goto out;
             }
         } else if (!strcmp(featurestr, "check")) {
             check_cpuid = 1;
@@ -1400,31 +1381,34 @@  static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
         } 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;
+            error_setg(errp, "feature string `%s' not in format (+feature|"
+                       "-feature|feature=xyz)\n", featurestr);
+            goto out;
+        }
+        if (error_is_set(errp)) {
+            goto out;
         }
         featurestr = strtok(NULL, ",");
     }
-    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
-    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
-    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
-    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
-    x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX];
-    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
-    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
-    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
-    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
-    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
-    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
-    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
-    x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
-    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
-    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
-    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
-    return 0;
+    env->cpuid_features |= plus_features[FEAT_1_EDX];
+    env->cpuid_ext_features |= plus_features[FEAT_1_ECX];
+    env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX];
+    env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX];
+    env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX];
+    env->cpuid_kvm_features |= plus_features[FEAT_KVM];
+    env->cpuid_svm_features |= plus_features[FEAT_SVM];
+    env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
+    env->cpuid_features &= ~minus_features[FEAT_1_EDX];
+    env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX];
+    env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
+    env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
+    env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
+    env->cpuid_kvm_features &= ~minus_features[FEAT_KVM];
+    env->cpuid_svm_features &= ~minus_features[FEAT_SVM];
+    env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
 
-error:
-    return -1;
+out:
+    return;
 }
 
 /* generate a composite string into buf of all cpuid names in featureset
@@ -1556,10 +1540,6 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     }
     def->ext_features |= CPUID_EXT_HYPERVISOR;
 
-    if (cpu_x86_parse_featurestr(def, features) < 0) {
-        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
-        goto out;
-    }
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
     object_property_set_int(OBJECT(cpu), def->level, "level", &error);
     object_property_set_int(OBJECT(cpu), def->family, "family", &error);
@@ -1579,7 +1559,11 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             "tsc-frequency", &error);
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    if (error) {
+        goto out;
+    }
 
+    cpu_x86_parse_featurestr(cpu, features, &error);
 out:
     g_strfreev(model_pieces);
     if (error) {