(+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.
> 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
