Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 1 Jun 2016 14:43:09 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]

> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3fbc6f3..6159a7f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> >      }
> >  }
> >  
> > +/* Features to be added */
> 
> Please add something like "Features to be added. Will be replaced
> by global variables in the future".
> 
> > +static FeatureWordArray plus_features = { 0 };
> > +/* Features to be removed */
> > +static FeatureWordArray minus_features = { 0 };
> > +
> 
> I see that this hack is replaced by the following patches, but is
> there an easy way to remove the CPUState argument from
> x86_cpu_parse_featurestr() before we introduce these static
> variables? (No problem if there's no way to do that, as long as
> the static variables are explicitly documented as a temporary
> hack)
It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
local to x86 that probably would stay here forever.
I should add comment that explains why +- can't be replaced
with normal properties.

I don't plan to replace plus/minus_features with anything nor to
make this variables a global ones to spread +- x86/sparc legacy
format everywhere.
 
What I would do though before enabling -device/device_add for X86CPU is
to disable +- handling for new machine types so that CPUs would
follow generic property semantic of device used everywhere else.

> 
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
> >  static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > @@ -1939,13 +1944,7 @@ static void
> > x86_cpu_parse_featurestr(CPUState *cs, char *features, {
> >      X86CPU *cpu = X86_CPU(cs);
> >      char *featurestr; /* Single 'key=value" string being parsed */
> > -    FeatureWord w;
> > -    /* Features to be added */
> > -    FeatureWordArray plus_features = { 0 };
> > -    /* Features to be removed */
> > -    FeatureWordArray minus_features = { 0 };
> >      uint32_t numvalue;
> > -    CPUX86State *env = &cpu->env;
> >      Error *local_err = NULL;
> >  
> >      featurestr = features ? strtok(features, ",") : NULL;
> > @@ -2019,18 +2018,6 @@ static void
> > x86_cpu_parse_featurestr(CPUState *cs, char *features, }
> >          featurestr = strtok(NULL, ",");
> >      }
> > -
> > -    if (cpu->host_features) {
> > -        for (w = 0; w < FEATURE_WORDS; w++) {
> > -            env->features[w] =
> > -                x86_cpu_get_supported_feature_word(w,
> > cpu->migratable);
> > -        }
> > -    }
> > -
> > -    for (w = 0; w < FEATURE_WORDS; w++) {
> > -        env->features[w] |= plus_features[w];
> > -        env->features[w] &= ~minus_features[w];
> > -    }
> >  }
> >  
> >  /* Print all cpuid feature names in featureset
> > @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState
> > *dev, Error **errp) CPUX86State *env = &cpu->env;
> >      Error *local_err = NULL;
> >      static bool ht_warned;
> > +    FeatureWord w;
> >  
> >      if (cpu->apic_id < 0) {
> >          error_setg(errp, "apic-id property was not initialized
> > properly"); return;
> >      }
> >  
> > +    if (cpu->host_features) {
> > +        for (w = 0; w < FEATURE_WORDS; w++) {
> > +            env->features[w] =
> > +                x86_cpu_get_supported_feature_word(w,
> > cpu->migratable);
> > +        }
> > +    }
> > +
> > +    for (w = 0; w < FEATURE_WORDS; w++) {
> > +        cpu->env.features[w] |= plus_features[w];
> > +        cpu->env.features[w] &= ~minus_features[w];
> > +    }
> > +
> >      if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
> >          env->cpuid_level = 7;
> >      }
> > -- 
> > 1.8.3.1
> > 
>