| Message ID | 20240627140628.1025317-3-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: drop AMD machine check bits from Intel CPUID | expand |
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); > }
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); > > } > >
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); }
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(-)