Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 19 Dec 2012 14:54:30 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > It prepares for converting "+feature,-feature,feature=foo,feature" into
> > a set of key,value property pairs that will be applied to CPU by
> > cpu_x86_set_props().
> > 
> > Each feature handled by cpu_x86_parse_featurestr() will be converted into
> > foo,val pair and a corresponding property setter by following patches.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> object as parameter and set the properties directly? Or you can see some
> use case where saving the property-setting dictionary for later use will
> be useful?

I plan to use cpu_x86_parse_featurestr() + list of properties later when we
have properties and subclasses in place. Then it would be possible to
transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of global
properties.

In the end the option handler for -cpu XXX,... could be changed into:

cpu_opt_parser() {
    // hides legacy target specific ugliness 
    target_XXX_cpu_compat_parser_callback() {
        cpu_class_name = get_class_name(optval)

        // dumb compatibility parser, which converts old format into
        // canonical form feature,val property list
        prop_list = parse_featurestr(optval)

        // with classes and global properties we could get rid of the field
        // cpu_model_str in CPUxxxState
        return prop_list, cpu_class_name        
    }

    foreach (prop_list)
        add_global_prop(cpu_class_name,prop,val)

    // could be later transformed to property of board object and
    // we could get rid of one more global var
    cpu_model = cpu_class_name
}

> 
> (This will probably require calling cpudef_2_x86_cpu() before
> cpu_x86_parse_featurestr(), and changing the existing
> cpu_x86_parse_featurestr() code to change the cpu object, not a
> x86_def_t struct, but I think this will simplify the logic a lot)
You cannot set/uset +-feat directly on CPU without breaking current
behavior where -feat overrides all +feat no matter in what order they are
specified. That's why dictionary is used to easily maintain "if(not feat)
ignore" logic and avoid duplication. Pls, look at commit
https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
that converts +-feat into canonical feat=on/off form.

And if features are applied directly to CPU, it would require to another
rewrite if global properties are to be used. Which IMHO should be eventually
done since -cpu ... looks like global parameters for a specific cpu type.

> 
> > ---
> >  target-i386/cpu.c |   33 +++++++++++++++++++++++++++------
> >  1 files changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e075b59..a74d74b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> >      return 0;
> >  }
> >  
> > +/* Set features on X86CPU object based on a provide key,value list */
> > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
> > +{
> > +    const QDictEntry *ent;
> > +
> > +    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)) {
> > +            return;
> > +        }
> > +    }
> > +}
> > +
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
> > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> > +                                    QDict **props)
> >  {
> >      unsigned int i;
> >      char *featurestr; /* Single 'key=value" string being parsed */
> > @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >      uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> >      uint32_t minus_7_0_ebx_features = 0;
> >      uint32_t numvalue;
> > +    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
> > +    *props = qdict_new();
> > +    int j = 0;
> >  
> > -    featurestr = features ? strtok(features, ",") : NULL;
> > -
> > -    while (featurestr) {
> > +    while ((featurestr = feat_array[j++])) {
> >          char *val;
> >          if (featurestr[0] == '+') {
> >              add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> >              goto error;
> >          }
> > -        featurestr = strtok(NULL, ",");
> >      }
> >      x86_cpu_def->features |= plus_features;
> >      x86_cpu_def->ext_features |= plus_ext_features;
> > @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >      x86_cpu_def->kvm_features &= ~minus_kvm_features;
> >      x86_cpu_def->svm_features &= ~minus_svm_features;
> >      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> > +    g_strfreev(feat_array);
> >      return 0;
> >  
> >  error:
> > +    g_strfreev(feat_array);
> >      return -1;
> >  }
> >  
> > @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
> >  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >  {
> >      x86_def_t def1, *def = &def1;
> > +    QDict *props = NULL;
> >      Error *error = NULL;
> >      char *name, *features;
> >      gchar **model_pieces;
> > @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >                              &def->ext3_features, &def->kvm_features,
> >                              &def->svm_features, &def->cpuid_7_0_ebx_features);
> >  
> > -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> > +    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
> >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> >          goto out;
> >      }
> >  
> >      cpudef_2_x86_cpu(cpu, def, &error);
> > +    cpu_x86_set_props(cpu, props, &error);
> >  
> >  out:
> > +    QDECREF(props);
> >      g_strfreev(model_pieces);
> >      if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > -- 
> > 1.7.1
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor