On 8/8/2024 5:29 PM, Igor Mammedov wrote:
On Fri, 2 Aug 2024 03:24:26 -0400 Xiaoyao Li <xiaoyao...@intel.com> wrote:Currently, QEMU exposes CPUID 0x1f to guest only when necessary, i.e., when topology level that cannot be enumerated by leaf 0xB, e.g., die or module level, are configured for the guest.above implies that there could be a workaround to enable this leaf by tweaking -smp CLI, so I'd suggest to put it here. So users would be able to boot Windows without requiring to set this property.
sure. will add such statement.
However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU topology. and 2) There is a bug in Windows that Windows expects valid 0x1f leafs when the maximum basic leaf > 0x1f[1].Which Windows versions are affected by this?
@Manish and @John, can you answer it?
Introduce a bool flag enable_cpuid_0x1f in CPU for the cases that requires CPUID leaf 0x1f to be exposed to guest. For case 2), introduce a user settable property as well, which provides an opt-in interface for people to run the buggy Windows as a workaround. The default value of the property is set to false, thus making no effect on existing setup.Opportunistically rename x86_has_extended_topo() to x86_has_v2_extended_topo() because per Intel SDM, leaf 0xb is for extended topology enumeration leaf and leaf 0x1f is v2 extended topology enumration leaf.I don't see any point in renaming, just drop it
ok
[1] https://lore.kernel.org/qemu-devel/21ca5c19-677b-4fac-84d4-72413577f...@nutanix.com/ Signed-off-by: Xiaoyao Li <xiaoyao...@intel.com> --- include/hw/i386/topology.h | 9 --------- target/i386/cpu.c | 18 ++++++++++++++++-- target/i386/cpu.h | 4 ++++ target/i386/kvm/kvm.c | 2 +- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h index dff49fce1154..b63bce2f4c82 100644 --- a/include/hw/i386/topology.h +++ b/include/hw/i386/topology.h @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, return x86_apicid_from_topo_ids(topo_info, &topo_ids); }-/*- * Check whether there's extended topology level (module or die)? - */ -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) -{ - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); -} - #endif /* HW_I386_TOPOLOGY_H */ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 4688d140c2d9..b5b7ac96c272 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -300,6 +300,19 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache, (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0); }+/*+ * Check whether there's v2 extended topology level (module or die)? + */ +bool x86_has_v2_extended_topo(X86CPU *cpu) +{ + if (cpu->enable_cpuid_0x1f) { + return true; + } + + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); +} + static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info, enum CPUTopoLevel topo_level) { @@ -6637,7 +6650,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break; case 0x1F: /* V2 Extended Topology Enumeration Leaf */ - if (!x86_has_extended_topo(env->avail_cpu_topo)) { + if (!x86_has_v2_extended_topo(cpu)) { *eax = *ebx = *ecx = *edx = 0; break; } @@ -7450,7 +7463,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp) * cpu->vendor_cpuid_only has been unset for compatibility with older * machine types. */ - if (x86_has_extended_topo(env->avail_cpu_topo) && + if (x86_has_v2_extended_topo(cpu) && (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) { x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F); } @@ -8316,6 +8329,7 @@ static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true), DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor), DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true), + DEFINE_PROP_BOOL("cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false),if we ever add this knob, rename it to 'x-cpuid-0x1f' meaning: internal/unstable: use it on your own risk subject to removal without notice
I agree it now. Because exposing it as a user-settable interface is only for the case of Windows bug. For TDX case, QEMU can set it internally.
DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true), DEFINE_PROP_BOOL("x-amd-topoext-features-only", X86CPU, amd_topoext_features_only, true), DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false), diff --git a/target/i386/cpu.h b/target/i386/cpu.h index c6cc035df3d8..211a42ffbfa6 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -2110,6 +2110,9 @@ struct ArchCPU { /* Compatibility bits for old machine types: */ bool enable_cpuid_0xb;+ /* Force to expose cpuid 0x1f */+ bool enable_cpuid_0x1f; + /* Enable auto level-increase for all CPUID leaves */ bool full_cpuid_auto_level;@@ -2368,6 +2371,7 @@ void cpu_set_apic_feature(CPUX86State *env);void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); bool cpu_has_x2apic_feature(CPUX86State *env); +bool x86_has_v2_extended_topo(X86CPU *cpu);/* helper.c */void x86_cpu_set_a20(X86CPU *cpu, int a20_state); diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index b4aab9a410b5..d43c1fa26746 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1835,7 +1835,7 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env, break; } case 0x1f: - if (!x86_has_extended_topo(env->avail_cpu_topo)) { + if (!x86_has_v2_extended_topo(env_archcpu(env))) { cpuid_i--; break; }
