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

[Igor Mammedov <imammedo@redhat.com>]

On Tue,  4 Dec 2012 16:58:19 -0200
Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  - Coding style changes
>  - Replace "goto error" with "return -1"
> ---
>  target-i386/cpu.c | 67 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 22 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7afe839..4090152 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1208,25 +1208,10 @@ 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, ",");
> -    /* 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 = kvm_default_features, plus_svm_features = 0;
> -    uint32_t plus_7_0_ebx_features = 0;
> -    /* Features to be removed */
> -    uint32_t minus_features = 0, minus_ext_features = 0;
> -    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
> -    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> -    uint32_t minus_7_0_ebx_features = 0;
> -    uint32_t numvalue;
> -
>      for (def = x86_defs; def; def = def->next) {
>          if (name && !strcmp(name, def->name)) {
>              break;
> @@ -1235,16 +1220,37 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
>          kvm_cpu_fill_host(x86_cpu_def);
>      } else if (!def) {
> -        goto error;
> +        return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));
>      }
>  
> +    return 0;
> +}
> +
> +/* 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;
> +    uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0;
> +    uint32_t plus_7_0_ebx_features = 0;
> +    /* Features to be removed */
> +    uint32_t minus_features = 0, minus_ext_features = 0;
> +    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
> +    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> +    uint32_t minus_7_0_ebx_features = 0;
> +    uint32_t numvalue;
> +
>      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;
> @@ -1378,11 +1384,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;
>  }
>  
> @@ -1492,11 +1496,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;
> @@ -1555,7 +1573,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          error_free(error);
model_pieces is leaked,
s/return -1/goto 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
> 
> 


-- 
Regards,
  Igor