Re: [PATCH 2/2] kvm/x86: don't expose MSR_IA32_UMWAIT_CONTROL unconditionally

From: Maxim Levitsky <mlevitsk@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>, Tao Xu <tao3.xu@intel.com>,
	Xiaoyao Li <xiaoyao.li@intel.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] kvm/x86: don't expose MSR_IA32_UMWAIT_CONTROL unconditionally
Date: Tue, 30 Jun 2020 16:41:49 +0300	[thread overview]
Message-ID: <08986e161635cc83d2d96b9729257dec91fc4562.camel@redhat.com> (raw)
In-Reply-To: <3642373d-8d1d-de80-d3db-e835a8f29449@redhat.com>

On Thu, 2020-05-21 at 10:40 +0200, Paolo Bonzini wrote:
> On 21/05/20 08:44, Tao Xu wrote:
> > I am sorry, I mean:
> > By default, we don't expose WAITPKG to guest. For QEMU, we can use
> > "-overcommit cpu-pm=on" to use WAITPKG.
> 
> But UMONITOR, UMWAIT and TPAUSE are not NOPs on older processors (which
> I should have checked before committing your patch, I admit).  So you
> have broken "-cpu host -overcommit cpu-pm=on" on any processor that
> doesn't have WAITPKG.  I'll send a patch.
> 
> Paolo
> 
Any update on that?

I accidently hit this today while updating my guest's kernel.
Turns out I had '-overcommit cpu-pm=on' enabled and -cpu host,
and waitpkg (which my AMD cpu doesn't have by any means) was silently
exposed to the guest but it didn't use it, but the mainline kernel started
using it and so it crashes.
Took me some time to realize that I am hitting this issue.


The CPUID_7_0_ECX_WAITPKG is indeed cleared in KVM_GET_SUPPORTED_CPUID,
and code in qemu sets/clears it depending on 'cpu-pm' value.

This patch (copy-pasted so probably not to apply) works for me regardless if we want to fix the KVM_GET_SUPPORTED_CPUID
returned value (which I think we should).
It basically detects the presence of the UMWAIT by presense of MSR_IA32_UMWAIT_CONTROL
in the global KVM_GET_MSR_INDEX_LIST, which I recently fixed too, to not return this
msr if the host CPUID doesn't support it.
So this works but is a bit ugly IMHO.

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 6adbff3d74..e9933d2e68 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -412,7 +412,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
             ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
         }
     } else if (function == 7 && index == 0 && reg == R_ECX) {
-        if (enable_cpu_pm) {
+        if (enable_cpu_pm && has_msr_umwait) {
             ret |= CPUID_7_0_ECX_WAITPKG;
         } else {
             ret &= ~CPUID_7_0_ECX_WAITPKG;
-- 

Should I send this patch officially?

Best regards,
	Maxim Levitsky


