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

[Xiaoyao Li]

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.

> 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.

> 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[]

> 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?

> >       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