Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Igor Mammedov]

On Tue,  4 Mar 2014 03:55:44 +0100
Andreas Färber <address@hidden> wrote:

> Adapt the X86CPU implementation to suit the generic hook.
> This involves a cleanup of error handling to cope with NULL errp.
> 
> Signed-off-by: Andreas Färber <address@hidden>
> ---
>  include/qom/cpu.h |  3 +++
>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 64ebfa5..43d253a 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -67,6 +67,8 @@ struct TranslationBlock;
>   * CPUClass:
>   * @class_by_name: Callback to map -cpu command line model name to an
>   * instantiatable CPU type.
> + * @parse_features: Callback to parse command line arguments.
> + * The argument may be modified by the callback.
Could you specify which argument is expected to be modified?

>   * @reset: Callback to reset the #CPUState to its initial state.
>   * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
>   * @has_work: Callback for checking if there is work to do.
> @@ -96,6 +98,7 @@ typedef struct CPUClass {
>      /*< public >*/
>  
>      ObjectClass *(*class_by_name)(const char *cpu_model);
> +    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
>  
>      void (*reset)(CPUState *cpu);
>      int reset_dump_flags;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cbc8cd6..653840a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1626,8 +1626,10 @@ static inline void feat2prop(char *s)
>  
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error 
> **errp)
> +static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> +                                     Error **errp)
>  {
> +    X86CPU *cpu = X86_CPU(cs);
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
>      FeatureWordArray plus_features = { 0 };
> @@ -1635,6 +1637,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char 
> *features, Error **errp)
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
>      CPUX86State *env = &cpu->env;
> +    Error *local_err = NULL;
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
> @@ -1653,16 +1656,16 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, 
> char *features, Error **errp)
>  
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> +                    error_setg(&local_err, "bad numerical value %s", val);
>                      goto out;
>                  }
>                  if (numvalue < 0x80000000) {
> -                    fprintf(stderr, "xlevel value shall always be >= 
> 0x80000000"
> -                            ", fixup will be removed in future versions\n");
> +                    error_report("xlevel value shall always be >= 0x80000000"
> +                                 ", fixup will be removed in future 
> versions");
>                      numvalue += 0x80000000;
>                  }
>                  snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> -                object_property_parse(OBJECT(cpu), num, featurestr, errp);
> +                object_property_parse(OBJECT(cpu), num, featurestr, 
> &local_err);
>              } else if (!strcmp(featurestr, "tsc-freq")) {
>                  int64_t tsc_freq;
>                  char *err;
> @@ -1671,36 +1674,38 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, 
> char *features, Error **errp)
>                  tsc_freq = strtosz_suffix_unit(val, &err,
>                                                 STRTOSZ_DEFSUFFIX_B, 1000);
>                  if (tsc_freq < 0 || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> +                    error_setg(&local_err, "bad numerical value %s", val);
>                      goto out;
>                  }
>                  snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> -                object_property_parse(OBJECT(cpu), num, "tsc-frequency", 
> errp);
> +                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> +                                      &local_err);
>              } else if (!strcmp(featurestr, "hv-spinlocks")) {
>                  char *err;
>                  const int min = 0xFFF;
>                  char num[32];
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> +                    error_setg(&local_err, "bad numerical value %s", val);
>                      goto out;
>                  }
>                  if (numvalue < min) {
> -                    fprintf(stderr, "hv-spinlocks value shall always be >= 
> 0x%x"
> -                            ", fixup will be removed in future versions\n",
> +                    error_report("hv-spinlocks value shall always be >= 0x%x"
> +                            ", fixup will be removed in future versions",
>                              min);
>                      numvalue = min;
>                  }
>                  snprintf(num, sizeof(num), "%" PRId32, numvalue);
> -                object_property_parse(OBJECT(cpu), num, featurestr, errp);
> +                object_property_parse(OBJECT(cpu), num, featurestr, 
> &local_err);
>              } else {
> -                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> +                object_property_parse(OBJECT(cpu), val, featurestr, 
> &local_err);
>              }
>          } else {
>              feat2prop(featurestr);
> -            object_property_parse(OBJECT(cpu), "on", featurestr, errp);
> +            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
>          }
> -        if (error_is_set(errp)) {
> +        if (local_err) {
> +            error_propagate(errp, local_err);
>              goto out;
>          }
>          featurestr = strtok(NULL, ",");
> @@ -1926,7 +1931,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, 
> DeviceState *icc_bridge,
>          goto out;
>      }
>  
> -    cpu_x86_parse_featurestr(cpu, features, &error);
> +    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
>      if (error) {
>          goto out;
>      }
> @@ -2748,6 +2753,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, 
> void *data)
>      cc->reset = x86_cpu_reset;
>      cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
>  
> +    cc->parse_features = x86_cpu_parse_featurestr;
>      cc->has_work = x86_cpu_has_work;
>      cc->do_interrupt = x86_cpu_do_interrupt;
>      cc->dump_state = x86_cpu_dump_state;