| Message ID | 1465226212-254093-3-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 06, 2016 at 05:16:44PM +0200, Igor Mammedov wrote: > Making x86_cpu_parse_featurestr() a pure convertor > of legacy feature string into global properties, needs > it to be called before a CPU instance is created so > parser shouldn't modify CPUState directly or access > it at all. Hence move current hack that directly pokes > into CPUState, to set/unset +-feats, from parser to > CPU's realize method. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> I will just edit a comment below, when applying, for clarity: [...] > + /* TODO: convert plus_features & minus_features static vars > + * to global properties, once broken host_features is fixed > + */ I will rewrite this to: /*TODO: cpu->host_features inclurrectly overwrites features * set using "feat=on|off". Once we fix this, we can convert * plus_features & minus_features to global properties * inside x86_cpu_parse_featurestr() too. */ > + 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 >
On 06/07/2016 02:25 PM, Eduardo Habkost wrote: > [...] >> + /* TODO: convert plus_features & minus_features static vars >> + * to global properties, once broken host_features is fixed >> + */ > > I will rewrite this to: > > /*TODO: cpu->host_features inclurrectly overwrites features Was that supposed to be "incorrectly" or "currently"? > * set using "feat=on|off". Once we fix this, we can convert > * plus_features & minus_features to global properties > * inside x86_cpu_parse_featurestr() too. > */
On Tue, Jun 07, 2016 at 03:07:33PM -0600, Eric Blake wrote: > On 06/07/2016 02:25 PM, Eduardo Habkost wrote: > > > [...] > >> + /* TODO: convert plus_features & minus_features static vars > >> + * to global properties, once broken host_features is fixed > >> + */ > > > > I will rewrite this to: > > > > /*TODO: cpu->host_features inclurrectly overwrites features > > Was that supposed to be "incorrectly" or "currently"? Wow, that's an weird typo. I wrote "incorrectly" inclurrectly.
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 07bbab7..79a9b61 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1932,6 +1932,14 @@ static inline void feat2prop(char *s) } } +/* Compatibily hack to maintain legacy +-feat semantic, + * where +-feat overwrites any feature set by + * feat=on|feat even if the later is parsed after +-feat + * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled) + */ +static FeatureWordArray plus_features = { 0 }; +static FeatureWordArray minus_features = { 0 }; + /* Parse "+feature,-feature,feature=foo" CPU feature string */ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, @@ -1939,12 +1947,6 @@ 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 }; - CPUX86State *env = &cpu->env; Error *local_err = NULL; featurestr = features ? strtok(features, ",") : NULL; @@ -1985,18 +1987,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 @@ -2878,12 +2868,28 @@ 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; } + /* TODO: convert plus_features & minus_features static vars + * to global properties, once broken host_features is fixed + */ + 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; }
Making x86_cpu_parse_featurestr() a pure convertor of legacy feature string into global properties, needs it to be called before a CPU instance is created so parser shouldn't modify CPUState directly or access it at all. Hence move current hack that directly pokes into CPUState, to set/unset +-feats, from parser to CPU's realize method. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v1: - rewrite commit message - add comment why +-feat is special - add TODO comment to remove plus_features & minus_features static vars --- target-i386/cpu.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-)