[PATCH 0/2] target/i386: drop AMD machine check bits from Intel CPUID

[PATCH 0/2] target/i386: drop AMD machine check bits from Intel CPUID

[Paolo Bonzini]

The recent addition of the SUCCOR bit to kvm_arch_get_supported_cpuid()
causes the bit to be visible when "-cpu host" VMs are started on Intel
processors.

While this should in principle be harmless, it's not tidy and we don't
even know for sure that it doesn't cause any guest OS to take unexpected
paths.  So plumb in a mechanism for x86_cpu_get_supported_feature_word()
to return different values depending on the *guest* CPU vendor (which,
for KVM, is by default the same as the host vendor); and then use it
to hide the SUCCOR bit if the guest has non-AMD vendor.

Paolo Bonzini (2):
  target/i386: pass X86CPU to x86_cpu_get_supported_feature_word
  target/i386: drop AMD machine check bits from Intel CPUID

 target/i386/cpu.h         |  3 +--
 target/i386/cpu.c         | 29 +++++++++++++++++++++--------
 target/i386/kvm/kvm-cpu.c |  2 +-
 3 files changed, 23 insertions(+), 11 deletions(-)

-- 
2.45.2

[PATCH 1/2] target/i386: pass X86CPU to x86_cpu_get_supported_feature_word

[Paolo Bonzini]

This allows modifying the bits in "-cpu max"/"-cpu host" depending on
the guest CPU vendor (which, at least by default, is the host vendor in
the case of KVM).

For example, machine check architecture differs between Intel and AMD,
and bits from AMD should be dropped when configuring the guest for
an Intel model.

Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: John Allen <john.allen@amd.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h         |  3 +--
 target/i386/cpu.c         | 13 ++++++-------
 target/i386/kvm/kvm-cpu.c |  2 +-
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 29daf370485..9bea7142bf4 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -666,8 +666,7 @@ typedef enum FeatureWord {
 } FeatureWord;
 
 typedef uint64_t FeatureWordArray[FEATURE_WORDS];
-uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
-                                            bool migratable_only);
+uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
 
 /* cpuid_features bits */
 #define CPUID_FP87 (1U << 0)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4c2e6f3a71e..deb58670651 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6035,8 +6035,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
 
 #endif /* !CONFIG_USER_ONLY */
 
-uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
-                                            bool migratable_only)
+uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
 {
     FeatureWordInfo *wi = &feature_word_info[w];
     uint64_t r = 0;
@@ -6076,9 +6075,9 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
             ? CPUID_EXT2_LM & ~CPUID_EXT2_KERNEL_FEATURES
             : CPUID_EXT2_LM;
         r &= ~unavail;
-    }
+        break;
 #endif
-    if (migratable_only) {
+    if (cpu && cpu->migratable) {
         r &= x86_cpu_get_migratable_flags(w);
     }
     return r;
@@ -7371,7 +7370,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
              * by the user.
              */
             env->features[w] |=
-                x86_cpu_get_supported_feature_word(w, cpu->migratable) &
+                x86_cpu_get_supported_feature_word(cpu, w) &
                 ~env->user_features[w] &
                 ~feature_word_info[w].no_autoenable_flags;
         }
@@ -7497,7 +7496,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
 
     for (w = 0; w < FEATURE_WORDS; w++) {
         uint64_t host_feat =
-            x86_cpu_get_supported_feature_word(w, false);
+            x86_cpu_get_supported_feature_word(NULL, w);
         uint64_t requested_features = env->features[w];
         uint64_t unavailable_features = requested_features & ~host_feat;
         mark_unavailable_features(cpu, w, unavailable_features, prefix);
@@ -7617,7 +7616,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         env->features[FEAT_PERF_CAPABILITIES] & PERF_CAP_LBR_FMT;
     if (requested_lbr_fmt && kvm_enabled()) {
         uint64_t host_perf_cap =
-            x86_cpu_get_supported_feature_word(FEAT_PERF_CAPABILITIES, false);
+            x86_cpu_get_supported_feature_word(NULL, FEAT_PERF_CAPABILITIES);
         unsigned host_lbr_fmt = host_perf_cap & PERF_CAP_LBR_FMT;
 
         if (!cpu->enable_pmu) {
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index f9b99b5f50d..b2735d6efee 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -136,7 +136,7 @@ static void kvm_cpu_xsave_init(void)
         if (!esa->size) {
             continue;
         }
-        if ((x86_cpu_get_supported_feature_word(esa->feature, false) & esa->bits)
+        if ((x86_cpu_get_supported_feature_word(NULL, esa->feature) & esa->bits)
             != esa->bits) {
             continue;
         }
-- 
2.45.2

Re: [PATCH 1/2] target/i386: pass X86CPU to x86_cpu_get_supported_feature_word

[Xiaoyao Li]

On 6/27/2024 10:06 PM, Paolo Bonzini wrote:
> This allows modifying the bits in "-cpu max"/"-cpu host" depending on
> the guest CPU vendor (which, at least by default, is the host vendor in
> the case of KVM).
> 
> For example, machine check architecture differs between Intel and AMD,
> and bits from AMD should be dropped when configuring the guest for
> an Intel model.
> 
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: John Allen <john.allen@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/cpu.h         |  3 +--
>   target/i386/cpu.c         | 13 ++++++-------
>   target/i386/kvm/kvm-cpu.c |  2 +-
>   3 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 29daf370485..9bea7142bf4 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -666,8 +666,7 @@ typedef enum FeatureWord {
>   } FeatureWord;
>   
>   typedef uint64_t FeatureWordArray[FEATURE_WORDS];
> -uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
> -                                            bool migratable_only);
> +uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
>   
>   /* cpuid_features bits */
>   #define CPUID_FP87 (1U << 0)
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 4c2e6f3a71e..deb58670651 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6035,8 +6035,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
>   
>   #endif /* !CONFIG_USER_ONLY */
>   
> -uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
> -                                            bool migratable_only)
> +uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
>   {
>       FeatureWordInfo *wi = &feature_word_info[w];
>       uint64_t r = 0;
> @@ -6076,9 +6075,9 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
>               ? CPUID_EXT2_LM & ~CPUID_EXT2_KERNEL_FEATURES
>               : CPUID_EXT2_LM;
>           r &= ~unavail;
> -    }
> +        break;

It seems some leftover when spliting from next patch. Need to remove it.

Other than it,

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

>   #endif
> -    if (migratable_only) {
> +    if (cpu && cpu->migratable) {
>           r &= x86_cpu_get_migratable_flags(w);
>       }
>       return r;
> @@ -7371,7 +7370,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>                * by the user.
>                */
>               env->features[w] |=
> -                x86_cpu_get_supported_feature_word(w, cpu->migratable) &
> +                x86_cpu_get_supported_feature_word(cpu, w) &
>                   ~env->user_features[w] &
>                   ~feature_word_info[w].no_autoenable_flags;
>           }
> @@ -7497,7 +7496,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>   
>       for (w = 0; w < FEATURE_WORDS; w++) {
>           uint64_t host_feat =
> -            x86_cpu_get_supported_feature_word(w, false);
> +            x86_cpu_get_supported_feature_word(NULL, w);
>           uint64_t requested_features = env->features[w];
>           uint64_t unavailable_features = requested_features & ~host_feat;
>           mark_unavailable_features(cpu, w, unavailable_features, prefix);
> @@ -7617,7 +7616,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>           env->features[FEAT_PERF_CAPABILITIES] & PERF_CAP_LBR_FMT;
>       if (requested_lbr_fmt && kvm_enabled()) {
>           uint64_t host_perf_cap =
> -            x86_cpu_get_supported_feature_word(FEAT_PERF_CAPABILITIES, false);
> +            x86_cpu_get_supported_feature_word(NULL, FEAT_PERF_CAPABILITIES);
>           unsigned host_lbr_fmt = host_perf_cap & PERF_CAP_LBR_FMT;
>   
>           if (!cpu->enable_pmu) {
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index f9b99b5f50d..b2735d6efee 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -136,7 +136,7 @@ static void kvm_cpu_xsave_init(void)
>           if (!esa->size) {
>               continue;
>           }
> -        if ((x86_cpu_get_supported_feature_word(esa->feature, false) & esa->bits)
> +        if ((x86_cpu_get_supported_feature_word(NULL, esa->feature) & esa->bits)
>               != esa->bits) {
>               continue;
>           }

[PATCH 2/2] target/i386: drop AMD machine check bits from Intel CPUID

[Paolo Bonzini]

The recent addition of the SUCCOR bit to kvm_arch_get_supported_cpuid()
causes the bit to be visible when "-cpu host" VMs are started on Intel
processors.

While this should in principle be harmless, it's not tidy and we don't
even know for sure that it doesn't cause any guest OS to take unexpected
paths.  Since x86_cpu_get_supported_feature_word() can return different
different values depending on the guest, adjust it to hide the SUCCOR
bit if the guest has non-AMD vendor.

Suggested-by: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: John Allen <john.allen@amd.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index deb58670651..f3e9b543682 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6064,8 +6064,10 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
     } else {
         return ~0;
     }
+
+    switch (w) {
 #ifndef TARGET_X86_64
-    if (w == FEAT_8000_0001_EDX) {
+    case FEAT_8000_0001_EDX:
         /*
          * 32-bit TCG can emulate 64-bit compatibility mode.  If there is no
          * way for userspace to get out of its 32-bit jail, we can leave
@@ -6077,6 +6079,18 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
         r &= ~unavail;
         break;
 #endif
+
+    case FEAT_8000_0007_EBX:
+        if (cpu && !IS_AMD_CPU(&cpu->env)) {
+            /* Disable AMD machine check architecture for Intel CPU.  */
+            r = 0;
+        }
+        break;
+
+    default:
+        break;
+    }
+
     if (cpu && cpu->migratable) {
         r &= x86_cpu_get_migratable_flags(w);
     }
-- 
2.45.2

Re: [PATCH 2/2] target/i386: drop AMD machine check bits from Intel CPUID

[Xiaoyao Li]

On 6/27/2024 10:06 PM, Paolo Bonzini wrote:
> The recent addition of the SUCCOR bit to kvm_arch_get_supported_cpuid()
> causes the bit to be visible when "-cpu host" VMs are started on Intel
> processors.
> 
> While this should in principle be harmless, it's not tidy and we don't
> even know for sure that it doesn't cause any guest OS to take unexpected
> paths.  Since x86_cpu_get_supported_feature_word() can return different
> different values depending on the guest, adjust it to hide the SUCCOR

superfluous different

> bit if the guest has non-AMD vendor.

It seems to adjust it based on vendor in kvm_arch_get_supported_cpuid() 
is better than in x86_cpu_get_supported_feature_word(). Otherwise 
kvm_arch_get_supported_cpuid() still returns "risky" value for Intel VMs.

> 
> Suggested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: John Allen <john.allen@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/cpu.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index deb58670651..f3e9b543682 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6064,8 +6064,10 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
>       } else {
>           return ~0;
>       }
> +
> +    switch (w) {
>   #ifndef TARGET_X86_64
> -    if (w == FEAT_8000_0001_EDX) {
> +    case FEAT_8000_0001_EDX:
>           /*
>            * 32-bit TCG can emulate 64-bit compatibility mode.  If there is no
>            * way for userspace to get out of its 32-bit jail, we can leave
> @@ -6077,6 +6079,18 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
>           r &= ~unavail;
>           break;
>   #endif
> +
> +    case FEAT_8000_0007_EBX:
> +        if (cpu && !IS_AMD_CPU(&cpu->env)) {
> +            /* Disable AMD machine check architecture for Intel CPU.  */
> +            r = 0;
> +        }
> +        break;
> +
> +    default:
> +        break;
> +    }
> +
>       if (cpu && cpu->migratable) {
>           r &= x86_cpu_get_migratable_flags(w);
>       }

Re: [PATCH 2/2] target/i386: drop AMD machine check bits from Intel CPUID

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Il ven 28 giu 2024, 10:32 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> On 6/27/2024 10:06 PM, Paolo Bonzini wrote:
> > The recent addition of the SUCCOR bit to kvm_arch_get_supported_cpuid()
> > causes the bit to be visible when "-cpu host" VMs are started on Intel
> > processors.
> >
> > While this should in principle be harmless, it's not tidy and we don't
> > even know for sure that it doesn't cause any guest OS to take unexpected
> > paths.  Since x86_cpu_get_supported_feature_word() can return different
> > different values depending on the guest, adjust it to hide the SUCCOR
>
> superfluous different
>
> > bit if the guest has non-AMD vendor.
>
> It seems to adjust it based on vendor in kvm_arch_get_supported_cpuid()
> is better than in x86_cpu_get_supported_feature_word(). Otherwise
> kvm_arch_get_supported_cpuid() still returns "risky" value for Intel VMs.
>

But the cpuid bit is only invalid for Intel *guest* vendor, not host. It is
not a problem to have it if you run on Intel host but have a guest model
with AMD vendor.

I will check if there are other callers of kvm_arch_get_supported_cpuid(),
or callers of x86_cpu_get_supported_feature_word() with NULL cpu, that
might care about the difference.

Paolo

>
> > Suggested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Cc: John Allen <john.allen@amd.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   target/i386/cpu.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index deb58670651..f3e9b543682 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6064,8 +6064,10 @@ uint64_t
> x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
> >       } else {
> >           return ~0;
> >       }
> > +
> > +    switch (w) {
> >   #ifndef TARGET_X86_64
> > -    if (w == FEAT_8000_0001_EDX) {
> > +    case FEAT_8000_0001_EDX:
> >           /*
> >            * 32-bit TCG can emulate 64-bit compatibility mode.  If there
> is no
> >            * way for userspace to get out of its 32-bit jail, we can
> leave
> > @@ -6077,6 +6079,18 @@ uint64_t
> x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
> >           r &= ~unavail;
> >           break;
> >   #endif
> > +
> > +    case FEAT_8000_0007_EBX:
> > +        if (cpu && !IS_AMD_CPU(&cpu->env)) {
> > +            /* Disable AMD machine check architecture for Intel CPU.  */
> > +            r = 0;
> > +        }
> > +        break;
> > +
> > +    default:
> > +        break;
> > +    }
> > +
> >       if (cpu && cpu->migratable) {
> >           r &= x86_cpu_get_migratable_flags(w);
> >       }
>
>

[-- Attachment #2: Type: text/html, Size: 4083 bytes --]

Re: [PATCH 2/2] target/i386: drop AMD machine check bits from Intel CPUID

[Zhao Liu]

On Fri, Jun 28, 2024 at 03:23:11PM +0200, Paolo Bonzini wrote:
> Date: Fri, 28 Jun 2024 15:23:11 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 2/2] target/i386: drop AMD machine check bits from
>  Intel CPUID
> 
> Il ven 28 giu 2024, 10:32 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:
> 
> > On 6/27/2024 10:06 PM, Paolo Bonzini wrote:
> > > The recent addition of the SUCCOR bit to kvm_arch_get_supported_cpuid()
> > > causes the bit to be visible when "-cpu host" VMs are started on Intel
> > > processors.
> > >
> > > While this should in principle be harmless, it's not tidy and we don't
> > > even know for sure that it doesn't cause any guest OS to take unexpected
> > > paths.  Since x86_cpu_get_supported_feature_word() can return different
> > > different values depending on the guest, adjust it to hide the SUCCOR
> >
> > superfluous different
> >
> > > bit if the guest has non-AMD vendor.
> >
> > It seems to adjust it based on vendor in kvm_arch_get_supported_cpuid()
> > is better than in x86_cpu_get_supported_feature_word(). Otherwise
> > kvm_arch_get_supported_cpuid() still returns "risky" value for Intel VMs.
> >
> 
> But the cpuid bit is only invalid for Intel *guest* vendor, not host. It is
> not a problem to have it if you run on Intel host but have a guest model
> with AMD vendor.
> 
> I will check if there are other callers of kvm_arch_get_supported_cpuid(),
> or callers of x86_cpu_get_supported_feature_word() with NULL cpu, that
> might care about the difference.

Another example is CPUID_EXT3_TOPOEXT, though it's a no_autoenable_flags,
it can be set by "-cpu host,+topoext" on Intel platforms.

For this case, we have recognized that that the host/max CPU should only
contain vender specific features, and I think it would be hard to expand
such a rule afterwards, especially since there's other x86 vender like
zhaoxin who implement a subset of Intel/AMD:

https://lore.kernel.org/qemu-devel/d4c0dae5-b9d5-4deb-b300-78492ab11ed8@zhaoxin.com/#t

What about a new flag "host_bare_metal_check" in FeatureWordInfo? Then
if a feature is marked as "host_bare_metal_check", in addition to the
current checks in x86_cpu_get_supported_feature_word(), bare-metal CPUID
check is also needed (by host_cpuid()) for "host" CPU.

-Zhao

Re: [PATCH 2/2] target/i386: drop AMD machine check bits from Intel CPUID

[Paolo Bonzini]

On Mon, Jul 1, 2024 at 6:08 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > > It seems to adjust it based on vendor in kvm_arch_get_supported_cpuid()
> > > is better than in x86_cpu_get_supported_feature_word(). Otherwise
> > > kvm_arch_get_supported_cpuid() still returns "risky" value for Intel VMs.
> >
> > But the cpuid bit is only invalid for Intel *guest* vendor, not host. It is
> > not a problem to have it if you run on Intel host but have a guest model
> > with AMD vendor.
> >
> > I will check if there are other callers of kvm_arch_get_supported_cpuid(),
> > or callers of x86_cpu_get_supported_feature_word() with NULL cpu, that
> > might care about the difference.
>
> Another example is CPUID_EXT3_TOPOEXT, though it's a no_autoenable_flags,
> it can be set by "-cpu host,+topoext" on Intel platforms.

That was done by commit 7210a02c585 ("i386: Disable TOPOEXT by default
on "-cpu host"", 2018-08-16) which however does not explain what the
bug was. It talks about missing or inconsistent cache topology
information, but that's not precise enough to decide what the problem
was.

> For this case, we have recognized that that the host/max CPU should only
> contain vender specific features, and I think it would be hard to expand
> such a rule afterwards, especially since there's other x86 vender like
> zhaoxin who implement a subset of Intel/AMD:
>
> What about a new flag "host_bare_metal_check" in FeatureWordInfo? Then
> if a feature is marked as "host_bare_metal_check", in addition to the
> current checks in x86_cpu_get_supported_feature_word(), bare-metal CPUID
> check is also needed (by host_cpuid()) for "host" CPU.

I don't see why it's needed. The bare metal vendor is not visible to
the guest, therefore it should have no bearing on whether a bit is
included in CPUID.

Paolo