Re: [Qemu-devel] [PATCH 1/7] target/i386: handle filtered_features in a new function mark_unavailable_features

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Jul 02, 2019 at 05:01:15PM +0200, Paolo Bonzini wrote:
> The next patch will add a different reason for filtering features, unrelated
> to host feature support.  Extract a new function that takes care of disabling
> the features and reporting them.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c | 76 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index da6eb67..9149d0d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3236,17 +3236,39 @@ static char *feature_word_description(FeatureWordInfo *f, uint32_t bit)
>      return NULL;
>  }
>  
> -static void report_unavailable_features(FeatureWord w, uint32_t mask)
> +static bool x86_cpu_have_filtered_features(X86CPU *cpu)
>  {
> +    FeatureWord w;
> +
> +    for (w = 0; w < ARRAY_SIZE(feature_word_info); w++) {

I prefer to use FEATURE_WORDS instead of
ARRAY_SIZE(feature_word_info), for consistency.

I'm becoming more and more inclined to transform FeatureWordArray
into a bitmap.  We have too many "for (w; w < FEATURE_WORDS;
w++)" loops in the code that could be simplified using bitmap
operations.

But this is independent from this patch.


> +         if (cpu->filtered_features[w]) {
> +             return true;
> +         }
> +    }
> +
> +    return false;
> +}
> +
> +static void mark_unavailable_features(X86CPU *cpu, FeatureWord w, uint32_t mask,
> +                                      const char *prefix)
> +{
> +    CPUX86State *env = &cpu->env;
>      FeatureWordInfo *f = &feature_word_info[w];
>      int i;
>      char *feat_word_str;
>  
> +    env->features[w] &= ~mask;
> +    cpu->filtered_features[w] |= mask;
> +
> +    if (!cpu->check_cpuid && !cpu->enforce_cpuid) {
> +        return;
> +    }
> +
>      for (i = 0; i < 32; ++i) {
>          if ((1UL << i) & mask) {
>              feat_word_str = feature_word_description(f, i);
> -            warn_report("%s doesn't support requested feature: %s%s%s [bit %d]",
> -                        accel_uses_host_cpuid() ? "host" : "TCG",
> +            warn_report("%s: %s%s%s [bit %d]",
> +                        prefix,
>                          feat_word_str,
>                          f->feat_names[i] ? "." : "",
>                          f->feat_names[i] ? f->feat_names[i] : "", i);

This seems to undo commit 8ca30e8673af ("target-i386: Move
warning code outside x86_cpu_filter_features()").

Filtering and reporting is separate because
x86_cpu_filter_features() is also called from a QMP command
handler that is not supposed to generate any warnings on stderr
(query-cpu-model-expansion).


> @@ -3691,7 +3713,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
>  }
>  
>  static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
> -static int x86_cpu_filter_features(X86CPU *cpu);
> +static void x86_cpu_filter_features(X86CPU *cpu);
>  
>  /* Build a list with the name of all features on a feature word array */
>  static void x86_cpu_list_feature_names(FeatureWordArray features,
> @@ -3923,15 +3945,6 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>      return r;
>  }
>  
> -static void x86_cpu_report_filtered_features(X86CPU *cpu)
> -{
> -    FeatureWord w;
> -
> -    for (w = 0; w < FEATURE_WORDS; w++) {
> -        report_unavailable_features(w, cpu->filtered_features[w]);
> -    }
> -}
> -
>  static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
>  {
>      PropValue *pv;
> @@ -5170,21 +5183,20 @@ out:
>   *
>   * Returns: 0 if all flags are supported by the host, non-zero otherwise.
>   */
> -static int x86_cpu_filter_features(X86CPU *cpu)
> +static void x86_cpu_filter_features(X86CPU *cpu)
>  {
>      CPUX86State *env = &cpu->env;
>      FeatureWord w;
> -    int rv = 0;
> +    const char *prefix = accel_uses_host_cpuid()
> +                         ? "host doesn't support requested feature"
> +                         : "TCG doesn't support requested feature";
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          uint32_t host_feat =
>              x86_cpu_get_supported_feature_word(w, false);
>          uint32_t requested_features = env->features[w];
> -        env->features[w] &= host_feat;
> -        cpu->filtered_features[w] = requested_features & ~env->features[w];
> -        if (cpu->filtered_features[w]) {
> -            rv = 1;
> -        }
> +        uint32_t unavailable_features = requested_features & ~host_feat;
> +        mark_unavailable_features(cpu, w, unavailable_features, prefix);
>      }
>  
>      if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
> @@ -5210,13 +5222,9 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>               * host can't emulate the capabilities we report on
>               * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
>               */
> -            env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
> -            cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
> -            rv = 1;
> +            mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
>          }
>      }
> -
> -    return rv;
>  }
>  
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> @@ -5257,16 +5265,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> -    if (x86_cpu_filter_features(cpu) &&
> -        (cpu->check_cpuid || cpu->enforce_cpuid)) {
> -        x86_cpu_report_filtered_features(cpu);
> -        if (cpu->enforce_cpuid) {
> -            error_setg(&local_err,
> -                       accel_uses_host_cpuid() ?
> -                           "Host doesn't support requested features" :
> -                           "TCG doesn't support requested features");
> -            goto out;
> -        }
> +    x86_cpu_filter_features(cpu);
> +
> +    if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
> +        error_setg(&local_err,
> +                   accel_uses_host_cpuid() ?
> +                       "Host doesn't support requested features" :
> +                       "TCG doesn't support requested features");
> +        goto out;
>      }
>  
>      /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> -- 
> 1.8.3.1
> 
> 
> 

-- 
Eduardo