Re: [PATCH] target/i386: Always set leaf 0x1f

[Igor Mammedov <imammedo@redhat.com>]

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;
>              }