Re: [Qemu-devel] [PATCH v4 12/18] target-i386: Support check/enforce flags in TCG mode, too

[Eduardo Habkost]

On Thu, May 15, 2014 at 08:54:15PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > If enforce/check is specified in TCG mode, QEMU will ensure all CPU
> > features are supported by TCG, so no CPU feature is silently disabled.
> > 
> > Reviewed-by: Richard Henderson <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> >  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
> >    (Reviewed-by line kept)
> > Changes v2 -> v3:
> >  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
> >    (Reviewed-by line kept)
> > ---
> >  target-i386/cpu.c | 34 ++++++++++++++++------------------
> >  1 file changed, 16 insertions(+), 18 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b2e30ca..53b5038 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, 
> > uint32_t mask)
> >          if (1 << i & mask) {
> >              const char *reg = get_register_name_32(f->cpuid_reg);
> >              assert(reg);
> > -            fprintf(stderr, "warning: host doesn't support requested 
> > feature: "
> > +            fprintf(stderr, "warning: %s doesn't support requested 
> > feature: "
> >                  "CPUID.%02XH:%s%s%s [bit %d]\n",
> > +                kvm_enabled() ? "host" : "TCG",
> >                  f->cpuid_eax, reg,
> >                  f->feat_names[i] ? "." : "",
> >                  f->feat_names[i] ? f->feat_names[i] : "", i);
> > @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList 
> > *arch_query_cpu_definitions(Error **errp)
> >  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
> >  {
> >      FeatureWordInfo *wi = &feature_word_info[w];
> > -    assert(kvm_enabled());
> > -    return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> > -                                                   wi->cpuid_ecx,
> > -                                                   wi->cpuid_reg);
> > +    if (kvm_enabled()) {
> > +        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> > +                                                       wi->cpuid_ecx,
> > +                                                       wi->cpuid_reg);
> > +    } else {
> > +        return wi->tcg_features;
> > +    }
> >  }
> 
> This function is called unconditionally now, so apply the following?
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 48ba1d8..112b437 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1839,8 +1839,10 @@ static uint32_t
> x86_cpu_get_supported_feature_word(FeatureWord w)
>          return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                         wi->cpuid_ecx,
>                                                         wi->cpuid_reg);
> -    } else {
> +    } else if (tcg_enabled()) {
>          return wi->tcg_features;
> +    } else {
> +        return UINT32_MAX;

Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX.

>      }
>  }
> 
> 
> Not sure what to do about the warning message. It wouldn't occur though
> due to the suggested mask, so we could just ignore it for now.

One day we may be able to simply ask the machine object for the current
accelerator name. In the meantime, we could use:

  "warning: host (%s) doesn't support requested feature [...]",
  kvm_enabled() ? "KVM" : (tcg_enabled() ? "TCG" : "QEMU")

(But I won't object if you prefer to keep the warning message I
originally sent.)

-- 
Eduardo