| Message ID | 20240722101859.47408-1-manish.mishra@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: Always set leaf 0x1f | expand |
(+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.mishra@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. > 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: - 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. > 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/ > + */ > + 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); } Thanks, Zhao
On Mon, 22 Jul 2024 10:18:59 +0000 "manish.mishra" <manish.mishra@nutanix.com> wrote: > 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 ^^^^^^^^^^^^^ be more clear about > populated. This is observed with windows VMs with secure boot, uefi > and HyperV role enabled. add here exact QME CLI and necessary guest OS details to reproduce the issue. > 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. > --- maybe instead of adding workaround it would be better to enable 1F leaf on CPU models that supposed to have it? > 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); > > 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); > + } > 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. > + */ > + 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; > }
On 7/23/2024 10:26 PM, Zhao Liu wrote: > (+Xiaoyao, whose TDX work may also be related with this.) I have a similar patch for TDX because TDX requires CPUID leaf 0x1f to configure topology as a must. (I haven't post to QEMU community yet. I'm not sure how people want to proceed, refine this patch or I can post my version?) https://github.com/intel-staging/qemu-tdx/commit/de08fd30926bc9d7997af6bd12cfff1b998da8b7 https://github.com/intel-staging/qemu-tdx/commit/f81d2bcb67e4b01577723cc621099b0c6d558334 > 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.mishra@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. > >> 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: > > - 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. > >> 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/ > >> + */ >> + 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); > } > > Thanks, > Zhao >
On Wed, Jul 24, 2024 at 08:25:12AM +0800, Xiaoyao Li wrote: > Date: Wed, 24 Jul 2024 08:25:12 +0800 > From: Xiaoyao Li <xiaoyao.li@intel.com> > Subject: Re: [PATCH] target/i386: Always set leaf 0x1f > > On 7/23/2024 10:26 PM, Zhao Liu wrote: > > (+Xiaoyao, whose TDX work may also be related with this.) > > I have a similar patch for TDX because TDX requires CPUID leaf 0x1f to > configure topology as a must. > > (I haven't post to QEMU community yet. I'm not sure how people want to > proceed, refine this patch or I can post my version?) > > https://github.com/intel-staging/qemu-tdx/commit/de08fd30926bc9d7997af6bd12cfff1b998da8b7 Hi Xiaoyao, the logic is similar, if Manish is willing to keep iterating, we can help him improve to cover all the cases we need, then TDX and his case could both benefit. > https://github.com/intel-staging/qemu-tdx/commit/f81d2bcb67e4b01577723cc621099b0c6d558334 > > > > > 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.mishra@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. > > > > > 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: > > > > - 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. > > > > > 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/ > > > > > + */ > > > + 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); > > } > > > > Thanks, > > Zhao > > >
Thanks Zhao, Igor and Xiaoyao, I have sent v1 for this https://patchwork.ozlabs.org/project/qemu-devel/patch/20240724075226.212882-1-manish.mishra@nutanix.com/.
On 23/07/24 8:33 pm, Igor Mammedov wrote:
!-------------------------------------------------------------------|
CAUTION: External Email
|-------------------------------------------------------------------!
On Mon, 22 Jul 2024 10:18:59 +0000
"manish.mishra" <manish.mishra@nutanix.com><mailto:manish.mishra@nutanix.com> wrote:
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
^^^^^^^^^^^^^
be more clear about
Done now, please let me know if i need to add more details.
populated. This is observed with windows VMs with secure boot, uefi
and HyperV role enabled.
add here exact QME CLI and necessary guest OS details to reproduce the issue.
Done
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.
---
maybe instead of adding workaround it would be better to enable
1F leaf on CPU models that supposed to have it?
Windows expects it only when we have set max cpuid level greater than or equal to 0x1f. I mean if it is exposed it should not be all zeros. SapphireRapids CPU definition raised cpuid level to 0x20, so we starting seeing it with SapphireRapids.
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);
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);
+ }
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.
+ */
+ 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;
}
Thanks Zhao for quick review. I have sent v1 for this. https://patchwork.ozlabs.org/project/qemu-devel/patch/20240724075226.212882-1-manish.mishra@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.mishra@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
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); 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); + } 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. + */ + 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; }