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
