Re: [PATCH] i386/cpu: Introduce enable_cpuid_0x1f to force exposing CPUID 0x1f

[Igor Mammedov <imammedo@redhat.com>]

On Fri,  2 Aug 2024 03:24:26 -0400
Xiaoyao Li <xiaoyao.li@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.

> 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?
 
> 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 

 
> [1] https://lore.kernel.org/qemu-devel/21ca5c19-677b-4fac-84d4-72413577f260@nutanix.com/
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@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

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