[RESEND PATCH kvmtool] x86/cpuid: Stop masking the CPU vendor

[RESEND PATCH kvmtool] x86/cpuid: Stop masking the CPU vendor

[Oliver Upton]

commit bc0b99a ("kvm tools: Filter out CPU vendor string") replaced the
processor's native vendor string with a synthetic one to hack around
some interesting guest MSR accesses that were not handled in KVM. In
particular, the MC4_CTL_MASK MSR was accessed for AMD VMs, which isn't
supported by KVM. This MSR relates to masking MCEs originating from the
northbridge on real hardware, but is of zero use in virtualization.

Speaking more broadly, KVM does in fact do the right thing for such an
MSR (#GP), and it is annoying but benign that KVM does a printk for the
MSR. Masking the CPU vendor string is far from ideal, and gets in the
way of testing vendor-specific CPU features. Stop the shenanigans and
expose the vendor ID as returned by KVM_GET_SUPPORTED_CPUID.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 x86/cpuid.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/x86/cpuid.c b/x86/cpuid.c
index aa213d5..f4347a8 100644
--- a/x86/cpuid.c
+++ b/x86/cpuid.c
@@ -10,7 +10,6 @@
 
 static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, int cpu_id)
 {
-	unsigned int signature[3];
 	unsigned int i;
 
 	/*
@@ -20,13 +19,6 @@ static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, int cpu_id)
 		struct kvm_cpuid_entry2 *entry = &kvm_cpuid->entries[i];
 
 		switch (entry->function) {
-		case 0:
-			/* Vendor name */
-			memcpy(signature, "LKVMLKVMLKVM", 12);
-			entry->ebx = signature[0];
-			entry->ecx = signature[1];
-			entry->edx = signature[2];
-			break;
 		case 1:
 			entry->ebx &= ~(0xff << 24);
 			entry->ebx |= cpu_id << 24;
-- 
2.35.1.894.gb6a874cedc-goog

Re: [RESEND PATCH kvmtool] x86/cpuid: Stop masking the CPU vendor

[Andre Przywara]

On Thu, 17 Mar 2022 19:28:53 +0000
Oliver Upton <oupton@google.com> wrote:

Hi Oliver,

thanks for the patch, this overlaps with our recent discussion here:
https://lore.kernel.org/kvm/20220226060048.3-1-sidongli1997@gmail.com/

> commit bc0b99a ("kvm tools: Filter out CPU vendor string") replaced the
> processor's native vendor string with a synthetic one to hack around
> some interesting guest MSR accesses that were not handled in KVM. In
> particular, the MC4_CTL_MASK MSR was accessed for AMD VMs, which isn't
> supported by KVM. This MSR relates to masking MCEs originating from the
> northbridge on real hardware, but is of zero use in virtualization.

Yes, in general this applies to all kind of errata workarounds tied to
certain F/M/S values, something totally expected. We have the same
situation on Arm, actually, although the kernel tries to avoid IMPDEF
system register accesses.

> Speaking more broadly, KVM does in fact do the right thing for such an
> MSR (#GP), and it is annoying but benign that KVM does a printk for the
> MSR.

Yes, but the printk is the lesser of our problems, the #GP is typically
more of an issue. Fortunately other VMMs have this problem as well, so the
kernel itself learned to ignore certain MSR #GPs (rdmsrl_safe()), so we
are good now. Back then this #GP lead to a kernel crash, IIRC.

> Masking the CPU vendor string is far from ideal, and gets in the
> way of testing vendor-specific CPU features.

Not only that, it's mostly wrong and now unsustainable, see the early
kernel messages when running on an unknown vendor. Also glibc compiled for
a higher ISA level is now a showstopper.
At least the AMD CPUID spec clearly says that its CPUID register mapping
are only valid for the AMD vendor string, and I believe Intel relies on
that as well. I wouldn't know of conflicting assignments between the two,
though, but we now miss many features by exposing an unknown vendor.

> Stop the shenanigans and
> expose the vendor ID as returned by KVM_GET_SUPPORTED_CPUID.

Yes, that's the right thing to do.

So can you please:
1) make this a revert of the original kvmtool patch
2) Mention the glibc error in the commit message, so that search engines
turn this up?
3) Copy in some part of my explanation (either from this message or the
reply to the thread mentioned above).

If you don't feel like it or don't have time, let me know. I originally
wanted to send the revert myself, but got distracted.

Cheers,
Andre

> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  x86/cpuid.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/x86/cpuid.c b/x86/cpuid.c
> index aa213d5..f4347a8 100644
> --- a/x86/cpuid.c
> +++ b/x86/cpuid.c
> @@ -10,7 +10,6 @@
>  
>  static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, int cpu_id)
>  {
> -	unsigned int signature[3];
>  	unsigned int i;
>  
>  	/*
> @@ -20,13 +19,6 @@ static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, int cpu_id)
>  		struct kvm_cpuid_entry2 *entry = &kvm_cpuid->entries[i];
>  
>  		switch (entry->function) {
> -		case 0:
> -			/* Vendor name */
> -			memcpy(signature, "LKVMLKVMLKVM", 12);
> -			entry->ebx = signature[0];
> -			entry->ecx = signature[1];
> -			entry->edx = signature[2];
> -			break;
>  		case 1:
>  			entry->ebx &= ~(0xff << 24);
>  			entry->ebx |= cpu_id << 24;

Re: [RESEND PATCH kvmtool] x86/cpuid: Stop masking the CPU vendor

[Oliver Upton]

Hi Andre,

On Fri, Mar 18, 2022 at 10:54:38AM +0000, Andre Przywara wrote:
> On Thu, 17 Mar 2022 19:28:53 +0000
> Oliver Upton <oupton@google.com> wrote:
> 
> Hi Oliver,
> 
> thanks for the patch, this overlaps with our recent discussion here:
> https://lore.kernel.org/kvm/20220226060048.3-1-sidongli1997@gmail.com/

Oops! I missed this thread. Sorry about that.

> > commit bc0b99a ("kvm tools: Filter out CPU vendor string") replaced the
> > processor's native vendor string with a synthetic one to hack around
> > some interesting guest MSR accesses that were not handled in KVM. In
> > particular, the MC4_CTL_MASK MSR was accessed for AMD VMs, which isn't
> > supported by KVM. This MSR relates to masking MCEs originating from the
> > northbridge on real hardware, but is of zero use in virtualization.
> 
> Yes, in general this applies to all kind of errata workarounds tied to
> certain F/M/S values, something totally expected. We have the same
> situation on Arm, actually, although the kernel tries to avoid IMPDEF
> system register accesses.
> 
> > Speaking more broadly, KVM does in fact do the right thing for such an
> > MSR (#GP), and it is annoying but benign that KVM does a printk for the
> > MSR.
> 
> Yes, but the printk is the lesser of our problems, the #GP is typically
> more of an issue. Fortunately other VMMs have this problem as well, so the
> kernel itself learned to ignore certain MSR #GPs (rdmsrl_safe()), so we
> are good now. Back then this #GP lead to a kernel crash, IIRC.

Right, I was more alluding to the fact that the only sensible thing to
do in KVM is to #GP. Sinking reads/writes is a fast path into undefined
behavior.

Excellent detective work on the other thread, BTW. I flopped searching
around for this MSR.

> > Masking the CPU vendor string is far from ideal, and gets in the
> > way of testing vendor-specific CPU features.
> 
> Not only that, it's mostly wrong and now unsustainable, see the early
> kernel messages when running on an unknown vendor. Also glibc compiled for
> a higher ISA level is now a showstopper.
> At least the AMD CPUID spec clearly says that its CPUID register mapping
> are only valid for the AMD vendor string, and I believe Intel relies on
> that as well. I wouldn't know of conflicting assignments between the two,
> though, but we now miss many features by exposing an unknown vendor.

I did not know about the glibc dependency, that hurts!

> > Stop the shenanigans and
> > expose the vendor ID as returned by KVM_GET_SUPPORTED_CPUID.
> 
> Yes, that's the right thing to do.
> 
> So can you please:
> 1) make this a revert of the original kvmtool patch
> 2) Mention the glibc error in the commit message, so that search engines
> turn this up?
> 3) Copy in some part of my explanation (either from this message or the
> reply to the thread mentioned above).
> 
> If you don't feel like it or don't have time, let me know. I originally
> wanted to send the revert myself, but got distracted.

I'd be glad to send it out, I was actually bitten by the vendor string
issue when hacking around with [1].

[1] https://patchwork.kernel.org/project/kvm/cover/20220316005538.2282772-1-oupton@google.com/

--
Thanks,
Oliver