Re: [PATCH v1 2/4] i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of cpu_x86_cpuid()

[Zhao Liu <zhao1.liu@intel.com>]

Hi Xiaoyao,

Sorry for late reply.

> @@ -7490,6 +7489,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>  void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>  {
>      CPUX86State *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
>      FeatureWord w;
>      int i;
>      GList *l;
> @@ -7531,6 +7531,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>          }
>      }
>  
> +    if (cs->nr_cores * cs->nr_threads > 1) {
> +        env->features[FEAT_1_EDX] |= CPUID_HT;
> +    }
> +

We shouldn't place any CLI-configurable features here,
especially after expanding plus_features and minus_features.

HT has been made configurable since the commit 83629b1 ("target/i386/
cpu: Fix CPUID_HT exposure"), so if you want palce HT here, you
should make it un-configurable first.

Regarding commit 83629b1, in what cases do we need to actively set HT?

That commit even introduces more issues. Ideally, the hardware being
emulated by setting or masking feature bits should be feature-consistent.

However, "-cpu *,-ht -smp 2" does not remove the HT flag (which is
unexpected), and "-cpu *,+ht -smp 1" forcibly sets HT (which results in
buggy emulation). :(

In fact, HT should not be freely configurable in hardware emulation;
users should configure it in the BIOS.


>      for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
>          FeatureDep *d = &feature_dependencies[i];
>          if (!(env->features[d->from.index] & d->from.mask)) {
> -- 
> 2.34.1
> 
>