On Thu, 17 Mar 2022 19:28:53 +0000 Oliver Upton <oupton@xxxxxxxxxx> wrote: Hi Oliver, thanks for the patch, this overlaps with our recent discussion here: https://lore.kernel.org/kvm/20220226060048.3-1-sidongli1997@xxxxxxxxx/ > 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@xxxxxxxxxx> > --- > 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;
