Track CPUID_HT in env->features[FEAT_1_EDX] instead of evaluating it
each time in cpu_x86_cpuid(). env->features[] should be set up in cpu's
realizefn() and cpu_x86_cpuid() should be the consumer of it.

Beside, TDX support also depends on it because TDX is going to validate
the feature configuration by comparing what TDX module reports with
env->features[]. If not tracking CPUID_HT in env->features[], it gets
warnings like below when number of vcpus > 1:

  warning: TDX enforces set the feature: CPUID.01H:EDX.ht [bit 28]

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1179b7a3ce62..e0c5a61ff615 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6544,7 +6544,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = env->features[FEAT_1_EDX];
         if (threads_per_pkg > 1) {
             *ebx |= threads_per_pkg << 16;
-            *edx |= CPUID_HT;
         }
         if (!cpu->enable_pmu) {
             *ecx &= ~CPUID_EXT_PDCM;
@@ -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;
+    }
+
     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

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

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

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

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
>