Thanks Zhao for quick review. I have sent v1 for this.
https://patchwork.ozlabs.org/project/qemu-devel/patch/20240724075226.212882-1-manish.mis...@nutanix.com/.
On 23/07/24 7:56 pm, Zhao Liu wrote:
!-------------------------------------------------------------------|
CAUTION: External Email
|-------------------------------------------------------------------!
(+Xiaoyao, whose TDX work may also be related with this.)
Hi Manish,
Thanks for your patch! Some comments below.
On Mon, Jul 22, 2024 at 10:18:59AM +0000, manish.mishra wrote:
Date: Mon, 22 Jul 2024 10:18:59 +0000
From: "manish.mishra" <manish.mis...@nutanix.com>
Subject: [PATCH] target/i386: Always set leaf 0x1f
X-Mailer: git-send-email 2.22.3
QEMU does not set 0x1f in case VM does not have extended CPU topology
and expects guests to fallback to 0xb. Some versions of windows i.e.
windows 10, 11 does not like this behavior and expects this leaf to be
populated. This is observed with windows VMs with secure boot, uefi
and HyperV role enabled.
Leaf 0x1f is superset of 0xb, so it makes sense to set 0x1f equivalent
to 0xb by default and workaround windows issue. This change adds a
new property 'cpuid-0x1f-enforce' to set leaf 0x1f equivalent to 0xb in
case extended CPU topology is not present and behave as before otherwise.
---
hw/i386/pc.c | 1 +
target/i386/cpu.c | 71 +++++++++++++++++++++++++++----------------
target/i386/cpu.h | 5 +++
target/i386/kvm/kvm.c | 4 ++-
4 files changed, 53 insertions(+), 28 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c74931d577..4cab04e443 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -85,6 +85,7 @@ GlobalProperty pc_compat_9_0[] = {
{ TYPE_X86_CPU, "guest-phys-bits", "0" },
{ "sev-guest", "legacy-vm-type", "on" },
{ TYPE_X86_CPU, "legacy-multi-node", "on" },
+ { TYPE_X86_CPU, "cpuid-0x1f-enforce", "false" },
};
const size_t pc_compat_9_0_len = G_N_ELEMENTS(pc_compat_9_0);
Yes, this is needed, but the 9.1 soft freeze is coming close soon, so
you may have to add pc_compat_9_1[] if it doesn't get accepted before
the soft freeze.
Sure, i will update it if required.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4688d140c2..f89b2ef335 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -416,6 +416,43 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t
count,
assert(!(*eax & ~0x1f));
}
+static void encode_topo_cpuid_b(CPUX86State *env, uint32_t count,
+ X86CPUTopoInfo *topo_info,
+ uint32_t threads_per_pkg,
+ uint32_t *eax, uint32_t *ebx,
+ uint32_t *ecx, uint32_t *edx)
+{
+ X86CPU *cpu = env_archcpu(env);
+
+ if (!cpu->enable_cpuid_0xb) {
+ *eax = *ebx = *ecx = *edx = 0;
+ return;
+ }
+
+ *ecx = count & 0xff;
+ *edx = cpu->apic_id;
+
+ switch (count) {
+ case 0:
+ *eax = apicid_core_offset(topo_info);
+ *ebx = topo_info->threads_per_core;
+ *ecx |= CPUID_B_ECX_TOPO_LEVEL_SMT << 8;
+ break;
+ case 1:
+ *eax = apicid_pkg_offset(topo_info);
+ *ebx = threads_per_pkg;
+ *ecx |= CPUID_B_ECX_TOPO_LEVEL_CORE << 8;
+ break;
+ default:
+ *eax = 0;
+ *ebx = 0;
+ *ecx |= CPUID_B_ECX_TOPO_LEVEL_INVALID << 8;
+ }
+
+ assert(!(*eax & ~0x1f));
+ *ebx &= 0xffff; /* The count doesn't need to be reliable. */
+}
+
/* Encode cache info for CPUID[0x80000005].ECX or CPUID[0x80000005].EDX */
static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache)
{
@@ -6601,33 +6638,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
break;
case 0xB:
/* Extended Topology Enumeration Leaf */
- if (!cpu->enable_cpuid_0xb) {
- *eax = *ebx = *ecx = *edx = 0;
- break;
- }
-
- *ecx = count & 0xff;
- *edx = cpu->apic_id;
-
- switch (count) {
- case 0:
- *eax = apicid_core_offset(&topo_info);
- *ebx = topo_info.threads_per_core;
- *ecx |= CPUID_B_ECX_TOPO_LEVEL_SMT << 8;
- break;
- case 1:
- *eax = apicid_pkg_offset(&topo_info);
- *ebx = threads_per_pkg;
- *ecx |= CPUID_B_ECX_TOPO_LEVEL_CORE << 8;
- break;
- default:
- *eax = 0;
- *ebx = 0;
- *ecx |= CPUID_B_ECX_TOPO_LEVEL_INVALID << 8;
- }
-
- assert(!(*eax & ~0x1f));
- *ebx &= 0xffff; /* The count doesn't need to be reliable. */
+ encode_topo_cpuid_b(env, count, &topo_info, threads_per_pkg,
+ eax, ebx, ecx, edx);
break;
case 0x1C:
if (cpu->enable_pmu && (env->features[FEAT_7_0_EDX] &
CPUID_7_0_EDX_ARCH_LBR)) {
@@ -6639,6 +6651,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
/* V2 Extended Topology Enumeration Leaf */
if (!x86_has_extended_topo(env->avail_cpu_topo)) {
*eax = *ebx = *ecx = *edx = 0;
+ if (cpu->enable_cpuid_0x1f_enforce) {
+ encode_topo_cpuid_b(env, count, &topo_info, threads_per_pkg,
+ eax, ebx, ecx, edx);
+ }
encode_topo_cpuid_b() is not necessary since encode_topo_cpuid1f() could
encode SMT/core levels with the same type code as 0x0b.
So here we just need tweak the encoding condition:
Done
- if (!x86_has_extended_topo(env->avail_cpu_topo)) {
+ if (!x86_has_extended_topo(env->avail_cpu_topo) &&
+ !cpu->enable_cpuid_0x1f_enforce) {
*eax = *ebx = *ecx = *edx = 0;
break;
}
encode_topo_cpuid1f(env, count, &topo_info, eax, ebx, ecx, edx);
break;
Then wrapping encode_topo_cpuid_b() could also be omitted, which keeps the
code changes as minor as possible.
Done
break;
}
@@ -8316,6 +8332,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-enforce", X86CPU, enable_cpuid_0x1f_enforce,
true),
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 1e121acef5..718b9f2b0b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2102,6 +2102,11 @@ struct ArchCPU {
/* Compatibility bits for old machine types: */
bool enable_cpuid_0xb;
+ /* Always return values for 0x1f leaf. In cases where extended CPU topology
+ * is not supported, return values equivalent of leaf 0xb.
s/is not supported/is not configured/
Done
+ */
+ bool enable_cpuid_0x1f_enforce;
+
/* Enable auto level-increase for all CPUID leaves */
bool full_cpuid_auto_level;
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index becca2efa5..a9c6f02900 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1799,6 +1799,7 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
uint32_t limit, i, j;
uint32_t unused;
struct kvm_cpuid_entry2 *c;
+ X86CPU *cpu = env_archcpu(env);
cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
@@ -1831,7 +1832,8 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
break;
}
case 0x1f:
- if (!x86_has_extended_topo(env->avail_cpu_topo)) {
+ if (!x86_has_extended_topo(env->avail_cpu_topo) &&
+ !cpu->enable_cpuid_0x1f_enforce) {
cpuid_i--;
break;
}
In addition to the above changes, we need to adjust the min_level of the
CPUID to ensure that 0x1f can be encoded:
@@ -7466,7 +7466,8 @@ 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_extended_topo(env->avail_cpu_topo) ||
+ cpu->enable_cpuid_0x1f_enforce) &&
(IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) {
x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
}
Done
Thanks,
Zhao
Thanks
Manish Mishra
