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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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
>