From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 1/2] KVM: Fix race between nmi injection and enabling nmi window Date: Thu, 03 Feb 2011 16:11:49 +0100 Message-ID: <4D4AC5B5.5050504@siemens.com> References: <1296745369-12066-1-git-send-email-avi@redhat.com> <1296745369-12066-2-git-send-email-avi@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" , Joerg Roedel To: Avi Kivity Return-path: Received: from david.siemens.de ([192.35.17.14]:33370 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756261Ab1BCPMF (ORCPT ); Thu, 3 Feb 2011 10:12:05 -0500 In-Reply-To: <1296745369-12066-2-git-send-email-avi@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-02-03 16:02, Avi Kivity wrote: > The interrupt injection logic looks something like > > if an nmi is pending, and nmi injection allowed > inject nmi > if an nmi is pending > request exit on nmi window > > the problem is that "nmi is pending" can be set asynchronously by > the PIT; if it happens to fire between the two if statements, we > will request an nmi window even though nmi injection is allowed. On > SVM, this has disasterous results, since it causes eflags.TF to be > set in random guest code. Good point. Fortunately never seen on production machines so far here (we have very moderate NMI rates). > > The fix is simple; make nmi_pending asynchronous using the standard You mean synchronous, no? > vcpu->requests mechanism; this ensures the code above is completely > synchronous wrt nmi_pending. > > Signed-off-by: Avi Kivity > --- > arch/x86/kvm/x86.c | 4 +++- > include/linux/kvm_host.h | 1 + > 2 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index a7f65aa..abe76c0 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -360,8 +360,8 @@ void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) > > void kvm_inject_nmi(struct kvm_vcpu *vcpu) > { > + kvm_make_request(KVM_REQ_NMI, vcpu); > kvm_make_request(KVM_REQ_EVENT, vcpu); > - vcpu->arch.nmi_pending = 1; > } > EXPORT_SYMBOL_GPL(kvm_inject_nmi); > > @@ -5182,6 +5182,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > r = 1; > goto out; > } > + if (kvm_check_request(KVM_REQ_NMI, vcpu)) > + vcpu->arch.nmi_pending = true; > } > > r = kvm_mmu_reload(vcpu); > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index c8dee22..7581090 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -43,6 +43,7 @@ > #define KVM_REQ_DEACTIVATE_FPU 10 > #define KVM_REQ_EVENT 11 > #define KVM_REQ_APF_HALT 12 > +#define KVM_REQ_NMI 13 > > #define KVM_USERSPACE_IRQ_SOURCE_ID 0 > Looks good. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux