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,

Thanks for your reply!

On Thu, Dec 05, 2024 at 03:54:36PM +0800, Xiaoyao Li wrote:
> Date: Thu, 5 Dec 2024 15:54:36 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v1 2/4] i386/cpu: Set up CPUID_HT in
>  x86_cpu_expand_features() instead of cpu_x86_cpuid()
> 
> On 12/5/2024 3:19 PM, Zhao Liu wrote:
> > 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.
> 
> yah, it needs to be placed before manipulation of plus_features and
> minus_features.

Please refer my comment at cover letter, you should do such thing in TDX
context.

> > 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.
> 
> No, commit 83629b1 doesn't make HT configurable but fix the warning of
> 
>    warning: host doesn't support requested feature: CPUID.01H:EDX.ht [bit
> 28]
> 
> when "-cpu *,+ht"
>
> > Regarding commit 83629b1, in what cases do we need to actively set HT?
> 
> when users want to do so. QEMU allows users to so do.

You haven't told the exact case you are fixing with HT.

HT is inherently tied to the topology, and custom modifications to HT
has already broken this. And I think we shouldn't go any further.

> > 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). :(
> 
> For the case "-cpu *,-ht -smp 2" we can add some warn like what for AMD:
> 
>     if (IS_AMD_CPU(env) &&
>         !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
>         cs->nr_threads > 1) {
>             warn_report_once("This family of AMD CPU doesn't support "
>                              "hyperthreading(%d). Please configure -smp "
>                              "options properly or try enabling topoext "
>                              "feature.", cs->nr_threads);
>     }
> 
> for the case of "-cpu *,+ht, -smp 1", we can add a dependency between "HT"
> and "smp > 1", similar as feature_dependencies[]

So I think the 83629b1 just masked the issue, a thorough fix should be
to avoid CLI configuration.

> > In fact, HT should not be freely configurable in hardware emulation;
> > users should configure it in the BIOS.
> 
> How users configure it in the BIOS? Or do you mean the BIOS will set/clear
> it based on the actual (v)cpus get activated? Any reference to teh BIOS
> spec?

Sorry, I think we should focus more on this issue. Such rhetorical
questions are not very helpful...

Thanks,
Zhao