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 4:31 PM, Zhao Liu wrote:
> 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.

TDX is one of the reasons for this patch, but not the whole reason.

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

just when users try to boot a VM with "-cpu *,+ht -smp n" where n > 1.

This is a valid user configuration. Before commit 83629b1, it got 
warning, and commit 83629b1 tried to fix the warning.

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

I don't object on this direction.

But it has nothing to do with this patch. This patch is trying to track 
HT in env->features[].

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

Again, I don't object on this direction. But it's another thing against 
this patch.

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

Sorry? it is you said "users should configure it in the BIOS". I was 
curious on it

> Thanks,
> Zhao