KVM: SVM: Implement check_nested_events for NMI

With NMI intercept moved to check_nested_events there is a race condition where vcpu->arch.nmi_pending is set late causing

the execution of check_nested_events to not setup correctly for nested.exit_required. A second call to check_nested_events allows the injectable nmi to be detected in time in order to require immediate exit from L2 to L1.

Signed-off-by Cathy Avery <[email protected]>

---
 arch/x86/kvm/x86.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

arch/x86/kvm/x86.c +11 -4

index 027dfd278a97..ecfafcd93536 100644
@@ -7734,10 +7734,17 @@ static int inject_pending_event(struct kvm_vcpu *vcpu)
 		vcpu->arch.smi_pending = false;
 		++vcpu->arch.smi_count;
 		enter_smm(vcpu);
-	} else if (vcpu->arch.nmi_pending && kvm_x86_ops.nmi_allowed(vcpu)) {
-		--vcpu->arch.nmi_pending;
-		vcpu->arch.nmi_injected = true;
-		kvm_x86_ops.set_nmi(vcpu);
+	} else if (vcpu->arch.nmi_pending) {
+		if (is_guest_mode(vcpu) && kvm_x86_ops.check_nested_events) {
+			r = kvm_x86_ops.check_nested_events(vcpu);
+			if (r != 0)
+				return r;
+		}
+		if (kvm_x86_ops.nmi_allowed(vcpu)) {
+			--vcpu->arch.nmi_pending;
+			vcpu->arch.nmi_injected = true;
+			kvm_x86_ops.set_nmi(vcpu);
+		}
 	} else if (kvm_cpu_has_injectable_intr(vcpu)) {
 		/*
 		 * Because interrupts can be injected asynchronously, we are

Signature · 1 line

2.20.1

Quoted 2 lines · Cathy Avery

With NMI intercept moved to check_nested_events there is a race condition where vcpu->arch.nmi_pending is set late causing

How is nmi_pending set late? The KVM_{G,S}ET_VCPU_EVENTS paths can't set it because the current KVM_RUN thread holds the mutex, and the only other call to process_nmi() is in the request path of vcpu_enter_guest, which has already executed.

Quoted 3 lines · Sean Christopherson

Quoted 2 lines · Cathy Avery

With NMI intercept moved to check_nested_events there is a race condition where vcpu->arch.nmi_pending is set late causing

Quoted 5 lines

How is nmi_pending set late? The KVM_{G,S}ET_VCPU_EVENTS paths can't set it because the current KVM_RUN thread holds the mutex, and the only other call to process_nmi() is in the request path of vcpu_enter_guest, which has already executed.

I think the actual cause is priority inversion between NMI and interrupts, because NMI is added last in patch 1.

Quoted 9 lines · Paolo Bonzini

Quoted 8 lines · Sean Christopherson

Quoted 2 lines · Cathy Avery

With NMI intercept moved to check_nested_events there is a race condition where vcpu->arch.nmi_pending is set late causing

How is nmi_pending set late? The KVM_{G,S}ET_VCPU_EVENTS paths can't set it because the current KVM_RUN thread holds the mutex, and the only other call to process_nmi() is in the request path of vcpu_enter_guest, which has already executed.

Quoted 2 lines

I think the actual cause is priority inversion between NMI and interrupts, because NMI is added last in patch 1.

Ah, that makes more sense. I stared/glared at this exact code for a long while and came to the conclusion that the "late" behavior was exclusive to interrupts, would have been a shame if all that glaring was for naught.

Quoted 1 line

Ah no, it's a bug in Cathy's patch and it's a weird one.

The problem is that on AMD you exit guest mode with the NMI latched and GIF=0. So check_nested_events should enable the NMI window in addition to causing a vmexit.

So why does it work? Because on AMD we don't have (yet) nested_run_pending, so we just check if we already have a vmexit scheduled and if so return -EBUSY. The second call causes inject_pending_event to return -EBUSY and thus go through KVM_REQ_EVENT again, which enables the NMI window.

Quoted 2 lines · Cathy Avery

With NMI intercept moved to check_nested_events there is a race condition where vcpu->arch.nmi_pending is set late causing

How is nmi_pending set late? The KVM_{G,S}ET_VCPU_EVENTS paths can't set it because the current KVM_RUN thread holds the mutex, and the only other call to process_nmi() is in the request path of vcpu_enter_guest, which has already executed.

Quoted 39 lines

the execution of check_nested_events to not setup correctly for nested.exit_required. A second call to check_nested_events allows the injectable nmi to be detected in time in order to require immediate exit from L2 to L1.

Signed-off-by Cathy Avery <[email protected]>

---
  arch/x86/kvm/x86.c | 15 +++++++++++----
  1 file changed, 11 insertions(+), 4 deletions(-)

arch/x86/kvm/x86.c +11 -4

index 027dfd278a97..ecfafcd93536 100644
@@ -7734,10 +7734,17 @@ static int inject_pending_event(struct kvm_vcpu *vcpu)
  		vcpu->arch.smi_pending = false;
  		++vcpu->arch.smi_count;
  		enter_smm(vcpu);
-	} else if (vcpu->arch.nmi_pending && kvm_x86_ops.nmi_allowed(vcpu)) {
-		--vcpu->arch.nmi_pending;
-		vcpu->arch.nmi_injected = true;
-		kvm_x86_ops.set_nmi(vcpu);
+	} else if (vcpu->arch.nmi_pending) {
+		if (is_guest_mode(vcpu) && kvm_x86_ops.check_nested_events) {
+			r = kvm_x86_ops.check_nested_events(vcpu);
+			if (r != 0)
+				return r;
+		}
+		if (kvm_x86_ops.nmi_allowed(vcpu)) {
+			--vcpu->arch.nmi_pending;
+			vcpu->arch.nmi_injected = true;
+			kvm_x86_ops.set_nmi(vcpu);
+		}
  	} else if (kvm_cpu_has_injectable_intr(vcpu)) {
  		/*
  		 * Because interrupts can be injected asynchronously, we are

Signature · 1 line

2.20.1

You will have to forgive me as I am new to KVM and any help would be most appreciated.

Quoted 3 lines · Cathy Avery

You will have to forgive me as I am new to KVM and any help would be most appreciated.

No problem---this is a _really_ hairy part. At least every time we make some changes it suddenly starts making more sense (both hacks and bugs decrease over time).

Quoted 8 lines · Paolo Bonzini

Quoted 3 lines · Cathy Avery

You will have to forgive me as I am new to KVM and any help would be most appreciated.

No problem---this is a _really_ hairy part. At least every time we make some changes it suddenly starts making more sense (both hacks and bugs decrease over time).

LOL, no kidding, what sadist gave Cathy nested NMI as a ramp task? :-D

Moved nested NMI exit to new check_nested_events.
The second patch fixes the NMI pending race condition that now occurs.

Cathy Avery (2):
KVM: SVM: Implement check_nested_events for NMI
KVM: x86: check_nested_events if there is an injectable NMI

 arch/x86/kvm/svm/nested.c | 21 +++++++++++++++++++++
 arch/x86/kvm/svm/svm.c    |  2 +-
 arch/x86/kvm/svm/svm.h    | 15 ---------------
 arch/x86/kvm/x86.c        | 15 +++++++++++----
 4 files changed, 33 insertions(+), 20 deletions(-)

Not directly related to this series but I just noticed that we have the following comment in inject_pending_event():

	/* try to inject new event if pending */
	if (vcpu->arch.exception.pending) {
                ...
		if (vcpu->arch.exception.nr == DB_VECTOR) {
			/*
			 * This code assumes that nSVM doesn't use
			 * check_nested_events(). If it does, the
			 * DR6/DR7 changes should happen before L1
			 * gets a #VMEXIT for an intercepted #DB in
			 * L2.  (Under VMX, on the other hand, the
			 * DR6/DR7 changes should not happen in the
			 * event of a VM-exit to L1 for an intercepted
			 * #DB in L2.)
			 */
			kvm_deliver_exception_payload(vcpu);
			if (vcpu->arch.dr7 & DR7_GD) {
				vcpu->arch.dr7 &= ~DR7_GD;
				kvm_update_dr7(vcpu);
			}
		}

		kvm_x86_ops.queue_exception(vcpu);
	}

As we already implement check_nested_events() on SVM, do we need to do anything here? CC: Jim who added the guardian (f10c729ff9652).

Moved nested NMI exit to new check_nested_events.
The second patch fixes the NMI pending race condition that now occurs.

Cathy Avery (2):
KVM: SVM: Implement check_nested_events for NMI
KVM: x86: check_nested_events if there is an injectable NMI

 arch/x86/kvm/svm/nested.c | 21 +++++++++++++++++++++
 arch/x86/kvm/svm/svm.c    |  2 +-
 arch/x86/kvm/svm/svm.h    | 15 ---------------
 arch/x86/kvm/x86.c        | 15 +++++++++++----
 4 files changed, 33 insertions(+), 20 deletions(-)