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

[Oliver Upton <oupton@google.com>]

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