On Wed, 1 Jun 2016 14:43:09 -0300
Eduardo Habkost <ehabk...@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
> >
>
