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

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 14:54:30 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > 
> > 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.

Is it really worth it to use global properties when you can simply set
the properties at the code that handles the "-cpu" string (and already
has to create the CPU object)?

(more comments about globals are below)


> 
> 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)

I'm expecting it to look different: instead of a target-specific parsing
function, you just need target-specific legacy properties on the CPU
object, that would be set by the (generic) featurestr parsing code if
present.

(e.g. "legacy-xlevel" could be used for the legacy "1 becomes
0x80000001" behavior, and "xlevel" would be the more sane xlevel
property without any magic).


> 
>         // 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

The cpu_model string will be immediately used to create the CPU object
just after this code runs. So do we really need to do the parsing before
creating the CPU object and use global properties for that? It looks
like unnecessary added complexity.

> }
> 
> > 
> > (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.

Well, you can make feature parsing code could simply save the
+feat/-feat results internally, and set/unset the properties directly at
the CPU in the end. You can even use a dictionary internally, the point
is that you don't need to expose the dictionary-based interface to the
outside if not necessary (making the API much simpler, IMO).

> 
> 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.

If we really are going to use global properties for the featurestring
result, we will need to parse the featurestring before creating the CPU
object, then you are correct.

I'm questioning the need to use global properties for the
featurestring parsing, if we can simply set the properties after
creating the CPU object. I expect "-cpu" to be easily translatable to
"-device" options, not to "-global" options.

In other words, I expect this:

  -cpu foo,+xxx,-yyy

to be translated in the future into something like:

  -device foo-x86-cpu,xxx=on,yyy=off

and not to:

  -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu

In other words, I disagree about "-cpu ..." looking 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

-- 
Eduardo