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

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

[Xiaoyao Li]

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, e.g., -smp xx,dies=2.

However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU
topology. and 2) There is a bug in Windows that Windows 10/11 expects valid
0x1f leafs when the maximum basic leaf > 0x1f[1].

Introduce a bool flag, enable_cpuid_0x1f, in CPU for the cases that
require CPUID leaf 0x1f to be exposed to guest. For case 2), introduce
a user settable property, "x-cpuid-0x1f" ,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.

Introduce a new function x86_has_cpuid_0x1f(), which is the warpper of
cpu->enable_cpuid_0x1f and x86_has_extended_topo() to check if it needs
to enable cpuid leaf 0x1f for the guest.

[1] https://lore.kernel.org/qemu-devel/20240724075226.212882-1-manish.mishra@nutanix.com/

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
changes in v2:
 - Add more details in commit message;
 - introduce a separate function x86_has_cpuid_0x1f() instead of
   modifying x86_has_extended_topo();
---
 target/i386/cpu.c     | 5 +++--
 target/i386/cpu.h     | 9 +++++++++
 target/i386/kvm/kvm.c | 2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 85ef7452c04e..1e8653c6b051 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6649,7 +6649,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_cpuid_0x1f(cpu)) {
             *eax = *ebx = *ecx = *edx = 0;
             break;
         }
@@ -7462,7 +7462,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_cpuid_0x1f(cpu) &&
             (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) {
             x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
         }
@@ -8328,6 +8328,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("x-cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false),
     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..085272d4c74a 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 enable cpuid 0x1f */
+    bool enable_cpuid_0x1f;
+
     /* Enable auto level-increase for all CPUID leaves */
     bool full_cpuid_auto_level;
 
@@ -2369,6 +2372,12 @@ 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);
 
+static inline bool x86_has_cpuid_0x1f(X86CPU *cpu)
+{
+    return cpu->enable_cpuid_0x1f ||
+           x86_has_extended_topo(cpu->env.avail_cpu_topo);
+}
+
 /* helper.c */
 void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
 void cpu_sync_avx_hflag(CPUX86State *env);
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 31f149c9902c..8af43cb48101 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_cpuid_0x1f(env_archcpu(env))) {
                 cpuid_i--;
                 break;
             }
-- 
2.34.1

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

[Zhao Liu]

On Mon, Aug 12, 2024 at 11:31:45PM -0400, Xiaoyao Li wrote:
> Date: Mon, 12 Aug 2024 23:31:45 -0400
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v2] i386/cpu: Introduce enable_cpuid_0x1f to force exposing
>  CPUID 0x1f
> X-Mailer: git-send-email 2.34.1
> 
> 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, e.g., -smp xx,dies=2.
> 
> However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU
> topology. and 2) There is a bug in Windows that Windows 10/11 expects valid
> 0x1f leafs when the maximum basic leaf > 0x1f[1].
> 
> Introduce a bool flag, enable_cpuid_0x1f, in CPU for the cases that
> require CPUID leaf 0x1f to be exposed to guest. For case 2), introduce
> a user settable property, "x-cpuid-0x1f" ,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.
> 
> Introduce a new function x86_has_cpuid_0x1f(), which is the warpper of
> cpu->enable_cpuid_0x1f and x86_has_extended_topo() to check if it needs
> to enable cpuid leaf 0x1f for the guest.
> 
> [1] https://lore.kernel.org/qemu-devel/20240724075226.212882-1-manish.mishra@nutanix.com/
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> changes in v2:
>  - Add more details in commit message;
>  - introduce a separate function x86_has_cpuid_0x1f() instead of
>    modifying x86_has_extended_topo();
> ---
>  target/i386/cpu.c     | 5 +++--
>  target/i386/cpu.h     | 9 +++++++++
>  target/i386/kvm/kvm.c | 2 +-
>  3 files changed, 13 insertions(+), 3 deletions(-)

This wrapper x86_has_cpuid_0x1f() looks good for me.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

[Igor Mammedov]

On Mon, 12 Aug 2024 23:31:45 -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, e.g., -smp xx,dies=2.
> 
> However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU
> topology. and 2) There is a bug in Windows that Windows 10/11 expects valid
> 0x1f leafs when the maximum basic leaf > 0x1f[1].
 1. will it boot if you use older cpu model?
 2. how user would know that this option would be needed?

> 
> Introduce a bool flag, enable_cpuid_0x1f, in CPU for the cases that
> require CPUID leaf 0x1f to be exposed to guest. For case 2), introduce
> a user settable property, "x-cpuid-0x1f" ,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.
> 
> Introduce a new function x86_has_cpuid_0x1f(), which is the warpper of
> cpu->enable_cpuid_0x1f and x86_has_extended_topo() to check if it needs
> to enable cpuid leaf 0x1f for the guest.
> 
> [1] https://lore.kernel.org/qemu-devel/20240724075226.212882-1-manish.mishra@nutanix.com/
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> changes in v2:
>  - Add more details in commit message;
>  - introduce a separate function x86_has_cpuid_0x1f() instead of
>    modifying x86_has_extended_topo();
> ---
>  target/i386/cpu.c     | 5 +++--
>  target/i386/cpu.h     | 9 +++++++++
>  target/i386/kvm/kvm.c | 2 +-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 85ef7452c04e..1e8653c6b051 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6649,7 +6649,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_cpuid_0x1f(cpu)) {
>              *eax = *ebx = *ecx = *edx = 0;
>              break;
>          }
> @@ -7462,7 +7462,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_cpuid_0x1f(cpu) &&
>              (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) {
>              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>          }
> @@ -8328,6 +8328,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("x-cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false),
>      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..085272d4c74a 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 enable cpuid 0x1f */
> +    bool enable_cpuid_0x1f;
> +
>      /* Enable auto level-increase for all CPUID leaves */
>      bool full_cpuid_auto_level;
>  
> @@ -2369,6 +2372,12 @@ 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);
>  
> +static inline bool x86_has_cpuid_0x1f(X86CPU *cpu)
> +{
> +    return cpu->enable_cpuid_0x1f ||
> +           x86_has_extended_topo(cpu->env.avail_cpu_topo);
> +}
> +
>  /* helper.c */
>  void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
>  void cpu_sync_avx_hflag(CPUX86State *env);
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 31f149c9902c..8af43cb48101 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_cpuid_0x1f(env_archcpu(env))) {
>                  cpuid_i--;
>                  break;
>              }

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

[Xiaoyao Li]

On 8/13/2024 5:27 PM, Igor Mammedov wrote:
> On Mon, 12 Aug 2024 23:31:45 -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, e.g., -smp xx,dies=2.
>>
>> However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU
>> topology. and 2) There is a bug in Windows that Windows 10/11 expects valid
>> 0x1f leafs when the maximum basic leaf > 0x1f[1].
>   1. will it boot if you use older cpu model?

It can boot with any cpu model that has .level < 0x1f.

>   2. how user would know that this option would be needed?

Honestly, I don't have an answer for it.

I'm not sure if it is the duty of QEMU to identify this case and print 
some hint to user. It's the bug of Windows, maybe Mircosoft should put 
something in their known bugs list for users?

> 
>>
>> Introduce a bool flag, enable_cpuid_0x1f, in CPU for the cases that
>> require CPUID leaf 0x1f to be exposed to guest. For case 2), introduce
>> a user settable property, "x-cpuid-0x1f" ,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.
>>
>> Introduce a new function x86_has_cpuid_0x1f(), which is the warpper of
>> cpu->enable_cpuid_0x1f and x86_has_extended_topo() to check if it needs
>> to enable cpuid leaf 0x1f for the guest.
>>
>> [1] https://lore.kernel.org/qemu-devel/20240724075226.212882-1-manish.mishra@nutanix.com/
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> changes in v2:
>>   - Add more details in commit message;
>>   - introduce a separate function x86_has_cpuid_0x1f() instead of
>>     modifying x86_has_extended_topo();
>> ---
>>   target/i386/cpu.c     | 5 +++--
>>   target/i386/cpu.h     | 9 +++++++++
>>   target/i386/kvm/kvm.c | 2 +-
>>   3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 85ef7452c04e..1e8653c6b051 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6649,7 +6649,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_cpuid_0x1f(cpu)) {
>>               *eax = *ebx = *ecx = *edx = 0;
>>               break;
>>           }
>> @@ -7462,7 +7462,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_cpuid_0x1f(cpu) &&
>>               (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) {
>>               x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>>           }
>> @@ -8328,6 +8328,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("x-cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false),
>>       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..085272d4c74a 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 enable cpuid 0x1f */
>> +    bool enable_cpuid_0x1f;
>> +
>>       /* Enable auto level-increase for all CPUID leaves */
>>       bool full_cpuid_auto_level;
>>   
>> @@ -2369,6 +2372,12 @@ 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);
>>   
>> +static inline bool x86_has_cpuid_0x1f(X86CPU *cpu)
>> +{
>> +    return cpu->enable_cpuid_0x1f ||
>> +           x86_has_extended_topo(cpu->env.avail_cpu_topo);
>> +}
>> +
>>   /* helper.c */
>>   void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
>>   void cpu_sync_avx_hflag(CPUX86State *env);
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 31f149c9902c..8af43cb48101 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_cpuid_0x1f(env_archcpu(env))) {
>>                   cpuid_i--;
>>                   break;
>>               }
> 

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

[Xiaoyao Li]

On 8/13/2024 10:51 PM, Xiaoyao Li wrote:
> On 8/13/2024 5:27 PM, Igor Mammedov wrote:
>> On Mon, 12 Aug 2024 23:31:45 -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, e.g., -smp xx,dies=2.
>>>
>>> However, 1) TDX architecture forces to require CPUID 0x1f to 
>>> configure CPU
>>> topology. and 2) There is a bug in Windows that Windows 10/11 expects 
>>> valid
>>> 0x1f leafs when the maximum basic leaf > 0x1f[1].
>>   1. will it boot if you use older cpu model?
> 
> It can boot with any cpu model that has .level < 0x1f.

I realize just now that we don't need to introduce "x-cpuid-1f" as the 
workaround for buggy windows. We can always workaround it by limiting 
the maximum basic CPUID leaf to less than 0x1f, i.e., -cpu xxx,level=0x1e

I think we can ignore this patch for now. I will re-submit it with TDX 
enabling series, and with "x-cpuid-1f" interface removed.

>>   2. how user would know that this option would be needed?
> 
> Honestly, I don't have an answer for it.
> 
> I'm not sure if it is the duty of QEMU to identify this case and print 
> some hint to user. It's the bug of Windows, maybe Mircosoft should put 
> something in their known bugs list for users?
> 

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

[Igor Mammedov]

On Wed, 14 Aug 2024 00:39:57 +0800
Xiaoyao Li <xiaoyao.li@intel.com> wrote:

> On 8/13/2024 10:51 PM, Xiaoyao Li wrote:
> > On 8/13/2024 5:27 PM, Igor Mammedov wrote:  
> >> On Mon, 12 Aug 2024 23:31:45 -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, e.g., -smp xx,dies=2.
> >>>
> >>> However, 1) TDX architecture forces to require CPUID 0x1f to 
> >>> configure CPU
> >>> topology. and 2) There is a bug in Windows that Windows 10/11 expects 
> >>> valid
> >>> 0x1f leafs when the maximum basic leaf > 0x1f[1].  
> >>   1. will it boot if you use older cpu model?  
> > 
> > It can boot with any cpu model that has .level < 0x1f.  
> 
> I realize just now that we don't need to introduce "x-cpuid-1f" as the 
> workaround for buggy windows. We can always workaround it by limiting 
> the maximum basic CPUID leaf to less than 0x1f, i.e., -cpu xxx,level=0x1e

I'd suggest to add this to change log into 'known issues' section.
(I mean Windows bug symptoms and suggested workaround)  

> 
> I think we can ignore this patch for now. I will re-submit it with TDX 
> enabling series, and with "x-cpuid-1f" interface removed.
> 
> >>   2. how user would know that this option would be needed?  
> > 
> > Honestly, I don't have an answer for it.
> > 
> > I'm not sure if it is the duty of QEMU to identify this case and print 
> > some hint to user. It's the bug of Windows, maybe Mircosoft should put 
> > something in their known bugs list for users?
I guess you've answered your own question alredy we have a workaround
and there is not need for yet another option that user won't know how to use.

As for configuring workaround, it's upto upper layers which know what OS
would be running in VM.