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

[Eduardo Habkost]

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 <address@hidden>
> ---
>  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