On Wed, Jun 01, 2016 at 06:37:23PM +0200, Igor Mammedov wrote: > it will allow to make x86_cpu_parse_featurestr() a pure convertor > of legacy features string into global properties. > That way -cpu FOOCPU,feat1=x,feat2=y,... is parsed only once
I would write it as "will be parsed only once (this will be implemented by the next patches)". > and as result of x86_cpu_parse_featurestr() a corresponding set > of global properties for specified CPU type are registered. Global properties are not registered yet (after applying this patch), so please explain that the static variables are a temporary hack that will be replaced by global properties in following patches. > So whenever a CPU device is created, generic device machinery > will apply that global properties for us. I would write "apply those global properties" instead. > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > --- > target-i386/cpu.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > 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) > /* 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 > -- Eduardo