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@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/ 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@xxxxxxxxxx/ -- Thanks, Oliver
