Re: [Qemu-devel] [PATCH 1/5] target-i386: cpu: separate feature string parsing from CPU model lookup

[Igor Mammedov]

On Mon,  3 Dec 2012 15:27:57 -0200
Eduardo Habkost <address@hidden> wrote:

> Instead of using parsing the whole cpu_model string inside
> cpu_x86_find_by_name(), first split it into the CPU model name and the
> full feature string, then parse the feature string into pieces.
> 
> When using CPU model classes, those two pieces of information will be
> used at different moments (CPU model name will be used to find CPU
> class, feature string will be used after CPU object was created), so
> making the split in two steps will make it easier to refactor the code
> later.
> 
> This should also help on the CPU properties work, that will just need to
> replace the cpu_x86_parse_featurestr() logic (and can keep the CPU model
> lookup code as-is).
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  target-i386/cpu.c | 64 
> ++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..89fd700 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1208,13 +1208,31 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
> Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char 
> *cpu_model)
> +static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
> -    unsigned int i;
>      x86_def_t *def;
>  
> -    char *s = g_strdup(cpu_model);
> -    char *featurestr, *name = strtok(s, ",");
> +    for (def = x86_defs; def; def = def->next)
> +        if (name && !strcmp(name, def->name))
> +            break;
> +    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> +        kvm_cpu_fill_host(x86_cpu_def);
> +    } else if (!def) {
> +        goto error;
Could you use "return -1;" here, there is no point to use goto in case of
single error exit point.

> +    } else {
> +        memcpy(x86_cpu_def, def, sizeof(*def));
> +    }
> +    return 0;
> +error:
> +    return -1;
and would be nice to remove above 2 lines, it will make patch shorter.

> +}
> +
> +/* Parse "+feature,-feature,feature=foo" CPU feature string
> + */
> +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> +{
> +    unsigned int i;
> +    char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added*/
>      uint32_t plus_features = 0, plus_ext_features = 0;
>      uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> @@ -1227,22 +1245,11 @@ static int cpu_x86_find_by_name(x86_def_t 
> *x86_cpu_def, const char *cpu_model)
>      uint32_t minus_7_0_ebx_features = 0;
>      uint32_t numvalue;
>  
> -    for (def = x86_defs; def; def = def->next)
> -        if (name && !strcmp(name, def->name))
> -            break;
> -    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> -        kvm_cpu_fill_host(x86_cpu_def);
> -    } else if (!def) {
> -        goto error;
> -    } else {
> -        memcpy(x86_cpu_def, def, sizeof(*def));
> -    }
> -
>      add_flagname_to_bitmaps("hypervisor", &plus_features,
>              &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
>              &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
>  
> -    featurestr = strtok(NULL, ",");
> +    featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
>          char *val;
> @@ -1376,11 +1383,9 @@ static int cpu_x86_find_by_name(x86_def_t 
> *x86_cpu_def, const char *cpu_model)
>      if (x86_cpu_def->cpuid_7_0_ebx_features && x86_cpu_def->level < 7) {
>          x86_cpu_def->level = 7;
>      }
> -    g_free(s);
>      return 0;
>  
>  error:
> -    g_free(s);
>      return -1;
>  }
>  
> @@ -1490,11 +1495,25 @@ int cpu_x86_register(X86CPU *cpu, const char 
> *cpu_model)
>      CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
> +    char *name, *features;
> +    gchar **model_pieces;
>  
>      memset(def, 0, sizeof(*def));
>  
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    model_pieces = g_strsplit(cpu_model, ",", 2);
> +    if (!model_pieces[0]) {
> +        goto error;
> +    }
> +    name = model_pieces[0];
> +    features = model_pieces[1];
> +
> +    if (cpu_x86_find_by_name(def, name) < 0) {
> +        goto error;
> +    }
> +
> +    if (cpu_x86_parse_featurestr(def, features) < 0) {
> +        goto error;
> +    }
>      if (def->vendor1) {
>          env->cpuid_vendor1 = def->vendor1;
>          env->cpuid_vendor2 = def->vendor2;
> @@ -1553,7 +1572,12 @@ int cpu_x86_register(X86CPU *cpu, const char 
> *cpu_model)
>          error_free(error);
>          return -1;
>      }
> +
> +    g_strfreev(model_pieces);
>      return 0;
> +error:
> +    g_strfreev(model_pieces);
> +    return -1;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.11.7
> 
> 

Provided requested changes are applied,
 Reviewed-By:  Igor Mammedov <address@hidden>

-- 
Regards,
  Igor