KVM: nVMX: Make direct IRQ/NMI injection work

PATCH In review Jan Kiszka 2 participants 16 messages Apr 14, 2013 – Apr 22, 2013

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2013-04-14 10:13 UTC #0 lore raw

v4 cover KVM: nVMX: Make direct IRQ/NMI injection work

Changes in this version:
- reworked patch 2, 4 and 5 according to review
- restored clearing of vm_entry_intr_info_field
- no longer read idt_vectoring_info_field from VMCS
- removed check of idt_vectoring_info_field on vmx_nmi_allowed/ vmx_interrupt_allowed
- added optimization patch 6

Jan Kiszka (6):
KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to

L1

KVM: nVMX: Rework event injection and recovery
KVM: VMX: Move vmx_nmi_allowed after vmx_set_nmi_mask
KVM: nVMX: Fix conditions for interrupt injection
KVM: nVMX: Fix conditions for NMI injection
KVM: nVMX: Avoid reading VM_EXIT_INTR_ERROR_CODE needlessly on nested
exits

 arch/x86/kvm/vmx.c |  182 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 121 insertions(+), 61 deletions(-)

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2013-04-14 10:12 UTC #1 lore raw

PATCH v4 1/6 KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to L1

From: Jan Kiszka <[email protected]>

Check if the interrupt or NMI window exit is for L1 by testing if it has the corresponding controls enabled. This is required when we allow direct injection from L0 to L2

Signed-off-by Jan Kiszka <[email protected]>

Reviewed-by Gleb Natapov <[email protected]>

---
 arch/x86/kvm/vmx.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

arch/x86/kvm/vmx.c +2 -7

index 669b803..752dbb5 100644
@@ -6112,14 +6112,9 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_TRIPLE_FAULT:
 		return 1;
 	case EXIT_REASON_PENDING_INTERRUPT:
+		return nested_cpu_has(vmcs12, CPU_BASED_VIRTUAL_INTR_PENDING);
 	case EXIT_REASON_NMI_WINDOW:
-		/*
-		 * prepare_vmcs02() set the CPU_BASED_VIRTUAL_INTR_PENDING bit
-		 * (aka Interrupt Window Exiting) only when L1 turned it on,
-		 * so if we got a PENDING_INTERRUPT exit, this must be for L1.
-		 * Same for NMI Window Exiting.
-		 */
-		return 1;
+		return nested_cpu_has(vmcs12, CPU_BASED_VIRTUAL_NMI_PENDING);
 	case EXIT_REASON_TASK_SWITCH:
 		return 1;
 	case EXIT_REASON_CPUID:

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2013-04-14 10:12 UTC #2 lore raw

PATCH v4 2/6 KVM: nVMX: Rework event injection and recovery

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

The basic idea is to always transfer the pending event injection on vmexit into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1, i.e. if we enter prepare_vmcs12.

vmcs12_save_pending_events takes care to transfer pending L0 events into the queue of L1. That is mandatory as L1 may decide to switch the guest state completely, invalidating or preserving the pending events for later injection (including on a different node, once we support migration).

This concept is based on the rule that a pending vmlaunch/vmresume is not canceled. Otherwise, we would risk to lose injected events or leak them into the wrong queues. Encode this rule via a WARN_ON_ONCE at the entry of nested_vmx_vmexit.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kvm/vmx.c |   99 +++++++++++++++++++++++++++++++++------------------
 1 files changed, 64 insertions(+), 35 deletions(-)

arch/x86/kvm/vmx.c +64 ~~-35~~

index 752dbb5..ca321e1 100644
@@ -6493,8 +6493,6 @@ static void __vmx_complete_interrupts(struct kvm_vcpu *vcpu,
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	if (is_guest_mode(&vmx->vcpu))
-		return;
 	__vmx_complete_interrupts(&vmx->vcpu, vmx->idt_vectoring_info,
 				  VM_EXIT_INSTRUCTION_LEN,
 				  IDT_VECTORING_ERROR_CODE);

@@ -6502,8 +6500,6 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 
 static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
 {
-	if (is_guest_mode(vcpu))
-		return;
 	__vmx_complete_interrupts(vcpu,
 				  vmcs_read32(VM_ENTRY_INTR_INFO_FIELD),
 				  VM_ENTRY_INSTRUCTION_LEN,

@@ -6535,21 +6531,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	unsigned long debugctlmsr;
 
-	if (is_guest_mode(vcpu) && !vmx->nested.nested_run_pending) {
-		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
-		if (vmcs12->idt_vectoring_info_field &
-				VECTORING_INFO_VALID_MASK) {
-			vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
-				vmcs12->idt_vectoring_info_field);
-			vmcs_write32(VM_ENTRY_INSTRUCTION_LEN,
-				vmcs12->vm_exit_instruction_len);
-			if (vmcs12->idt_vectoring_info_field &
-					VECTORING_INFO_DELIVER_CODE_MASK)
-				vmcs_write32(VM_ENTRY_EXCEPTION_ERROR_CODE,
-					vmcs12->idt_vectoring_error_code);
-		}
-	}
-
 	/* Record the guest's net vcpu time for enforced NMI injections. */
 	if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked))
 		vmx->entry_time = ktime_get();

@@ -6708,17 +6689,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
 	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
 
-	if (is_guest_mode(vcpu)) {
-		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
-		vmcs12->idt_vectoring_info_field = vmx->idt_vectoring_info;
-		if (vmx->idt_vectoring_info & VECTORING_INFO_VALID_MASK) {
-			vmcs12->idt_vectoring_error_code =
-				vmcs_read32(IDT_VECTORING_ERROR_CODE);
-			vmcs12->vm_exit_instruction_len =
-				vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
-		}
-	}
-
 	vmx->loaded_vmcs->launched = 1;
 
 	vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);

@@ -7326,6 +7296,48 @@ vmcs12_guest_cr4(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 			vcpu->arch.cr4_guest_owned_bits));
 }
 
+static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
+{
+	u32 idt_vectoring;
+	unsigned int nr;
+
+	if (vcpu->arch.exception.pending) {
+		nr = vcpu->arch.exception.nr;
+		idt_vectoring = nr | VECTORING_INFO_VALID_MASK;
+
+		if (kvm_exception_is_soft(nr)) {
+			vmcs12->vm_exit_instruction_len =
+				vcpu->arch.event_exit_inst_len;
+			idt_vectoring |= INTR_TYPE_SOFT_EXCEPTION;
+		} else
+			idt_vectoring |= INTR_TYPE_HARD_EXCEPTION;
+
+		if (vcpu->arch.exception.has_error_code) {
+			idt_vectoring |= VECTORING_INFO_DELIVER_CODE_MASK;
+			vmcs12->idt_vectoring_error_code =
+				vcpu->arch.exception.error_code;
+		}
+
+		vmcs12->idt_vectoring_info_field = idt_vectoring;
+	} else if (vcpu->arch.nmi_pending) {
+		vmcs12->idt_vectoring_info_field =
+			INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK | NMI_VECTOR;
+	} else if (vcpu->arch.interrupt.pending) {
+		nr = vcpu->arch.interrupt.nr;
+		idt_vectoring = nr | VECTORING_INFO_VALID_MASK;
+
+		if (vcpu->arch.interrupt.soft) {
+			idt_vectoring |= INTR_TYPE_SOFT_INTR;
+			vmcs12->vm_entry_instruction_len =
+				vcpu->arch.event_exit_inst_len;
+		} else
+			idt_vectoring |= INTR_TYPE_EXT_INTR;
+
+		vmcs12->idt_vectoring_info_field = idt_vectoring;
+	}
+}
+
 /*
  * prepare_vmcs12 is part of what we need to do when the nested L2 guest exits
  * and we want to prepare to run its L1 parent. L1 keeps a vmcs for L2 (vmcs12),

@@ -7411,15 +7423,29 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 
 	vmcs12->vm_exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
 	vmcs12->vm_exit_intr_error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
-	vmcs12->idt_vectoring_info_field = to_vmx(vcpu)->idt_vectoring_info;
-	vmcs12->idt_vectoring_error_code =
-		vmcs_read32(IDT_VECTORING_ERROR_CODE);
+	vmcs12->idt_vectoring_info_field = 0;
 	vmcs12->vm_exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
 
-	/* clear vm-entry fields which are to be cleared on exit */
-	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
+		/* vm_entry_intr_info_field is cleared on exit. Emulate this
+		 * instead of reading the real value. */
 		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
+
+		/*
+		 * Transfer the event that L0 or L1 may wanted to inject into
+		 * L2 to IDT_VECTORING_INFO_FIELD.
+		 */
+		vmcs12_save_pending_event(vcpu, vmcs12);
+	}
+
+	/*
+	 * Drop what we picked up for L2 via vmx_complete_interrupts. It is
+	 * preserved above and would only end up incorrectly in L1.
+	 */
+	vcpu->arch.nmi_injected = false;
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
 }
 
 /*

@@ -7519,6 +7545,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
 	int cpu;
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 
+	/* trying to cancel vmlaunch/vmresume is a bug */
+	WARN_ON_ONCE(vmx->nested.nested_run_pending);
+
 	leave_guest_mode(vcpu);
 	prepare_vmcs12(vcpu, vmcs12);

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2013-04-14 10:12 UTC #3 lore raw

PATCH v4 3/6 KVM: VMX: Move vmx_nmi_allowed after vmx_set_nmi_mask

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

vmx_set_nmi_mask will soon be used by vmx_nmi_allowed. No functional changes.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kvm/vmx.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

arch/x86/kvm/vmx.c +10 ~~-10~~

index ca321e1..24680a9 100644
@@ -4284,16 +4284,6 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
 			INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK | NMI_VECTOR);
 }
 
-static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
-{
-	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
-		return 0;
-
-	return	!(vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) &
-		  (GUEST_INTR_STATE_MOV_SS | GUEST_INTR_STATE_STI
-		   | GUEST_INTR_STATE_NMI));
-}
-
 static bool vmx_get_nmi_mask(struct kvm_vcpu *vcpu)
 {
 	if (!cpu_has_virtual_nmis())

@@ -4323,6 +4313,16 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 	}
 }
 
+static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
+{
+	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
+		return 0;
+
+	return	!(vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) &
+		  (GUEST_INTR_STATE_MOV_SS | GUEST_INTR_STATE_STI
+		   | GUEST_INTR_STATE_NMI));
+}
+
 static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)
 {
 	if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) {

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2013-04-14 10:13 UTC #4 lore raw

PATCH v4 4/6 KVM: nVMX: Fix conditions for interrupt injection

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

If we are entering guest mode, we do not want L0 to interrupt this vmentry with all its side effects on the vmcs. Therefore, injection shall be disallowed during L1->L2 transitions, as in the previous version. However, this check is conceptually independent of nested_exit_on_intr, so decouple it.

If L1 traps external interrupts, we can kick the guest from L2 to L1, also just like the previous code worked. But we no longer need to consider L1's idt_vectoring_info_field. It will always be empty at this point. Instead, if L2 has pending events, those are now found in the architectural queues and will, thus, prevent vmx_interrupt_allowed from being called at all.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kvm/vmx.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +12 -8

index 24680a9..56e7519 100644
@@ -4325,16 +4325,20 @@ static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 
 static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)
 {
-	if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) {
+	if (is_guest_mode(vcpu)) {
 		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
-		if (to_vmx(vcpu)->nested.nested_run_pending ||
-		    (vmcs12->idt_vectoring_info_field &
-		     VECTORING_INFO_VALID_MASK))
+
+		if (to_vmx(vcpu)->nested.nested_run_pending)
 			return 0;
-		nested_vmx_vmexit(vcpu);
-		vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT;
-		vmcs12->vm_exit_intr_info = 0;
-		/* fall through to normal code, but now in L1, not L2 */
+		if (nested_exit_on_intr(vcpu)) {
+			nested_vmx_vmexit(vcpu);
+			vmcs12->vm_exit_reason =
+				EXIT_REASON_EXTERNAL_INTERRUPT;
+			vmcs12->vm_exit_intr_info = 0;
+			/*
+			 * fall through to normal code, but now in L1, not L2
+			 */
+		}
 	}
 
 	return (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2013-04-14 10:13 UTC #5 lore raw

PATCH v4 5/6 KVM: nVMX: Fix conditions for NMI injection

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +28 -0

index 56e7519..ad9b4bc 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;

@@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending ||
+		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
+			   GUEST_ACTIVITY_WAIT_SIPI)
+			return 0;
+		if (nested_exit_on_nmi(vcpu)) {
+			nested_vmx_vmexit(vcpu);
+			vmcs12->vm_exit_reason = EXIT_REASON_EXCEPTION_NMI;
+			vmcs12->vm_exit_intr_info = NMI_VECTOR |
+				INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK;
+			/*
+			 * The NMI-triggered VM exit counts as injection:
+			 * clear this one and block further NMIs.
+			 */
+			vcpu->arch.nmi_pending = 0;
+			vmx_set_nmi_mask(vcpu, true);
+			return 0;
+		}
+	}
+
 	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
 		return 0;

Signature · 1 line

1.7.3.4

Gleb Natapov

2013-04-14 15:24 UTC #6 lore raw

Re: [PATCH v4 5/6] KVM: nVMX: Fix conditions for NMI injection

Quoted 29 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +6 -0
index 56e7519..ad9b4bc 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;

arch/x86/kvm/vmx.c static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) · line 4329

Quoted 9 lines

arch/x86/kvm/vmx.c +6 -0

@@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending ||
+		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
+			   GUEST_ACTIVITY_WAIT_SIPI)

The same is true for interrupt too, but I do not think that we should allow nested guest directly enter GUEST_ACTIVITY_WAIT_SIPI state or any other state except ACTIVE. They should be emulated instead. From quick look at the spec it looks like external interrupts do not cause VMEXIT while vcpu is in GUEST_ACTIVITY_WAIT_SIPI.

Quoted 21 lines

arch/x86/kvm/vmx.c +16 -0

+			return 0;
+		if (nested_exit_on_nmi(vcpu)) {
+			nested_vmx_vmexit(vcpu);
+			vmcs12->vm_exit_reason = EXIT_REASON_EXCEPTION_NMI;
+			vmcs12->vm_exit_intr_info = NMI_VECTOR |
+				INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK;
+			/*
+			 * The NMI-triggered VM exit counts as injection:
+			 * clear this one and block further NMIs.
+			 */
+			vcpu->arch.nmi_pending = 0;
+			vmx_set_nmi_mask(vcpu, true);
+			return 0;
+		}
+	}
+
 	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
 		return 0;

Signature · 1 line

1.7.3.4

--
Gleb.

Jan Kiszka

author

2013-04-14 15:53 UTC #7 lore raw

Re: [PATCH v4 5/6] KVM: nVMX: Fix conditions for NMI injection

Quoted 31 lines · Gleb Natapov

Quoted 30 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +6 -0
index 56e7519..ad9b4bc 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;
@@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 

Quoted 9 lines

arch/x86/kvm/vmx.c static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) · line 4329
Quoted 8 lines
arch/x86/kvm/vmx.c +6 -0
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending ||
+		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
+			   GUEST_ACTIVITY_WAIT_SIPI)
The same is true for interrupt too,

Yes, but aren't we already waiting with interrupts disabled in that state?

Quoted 3 lines

but I do not think that we should allow
nested guest directly enter GUEST_ACTIVITY_WAIT_SIPI state or any other state except ACTIVE. They should be emulated instead.

Well, they aren't emulated yet but directly applied. So I think the patch is correct in the current context at least.

What negative effects do you expect from entering those states with L2?

Quoted 3 lines

From quick look at
the spec it looks like external interrupts do not cause VMEXIT while vcpu is in GUEST_ACTIVITY_WAIT_SIPI.

Yes, see above.

Jan

Gleb Natapov

2013-04-14 16:18 UTC #8 lore raw

Re: [PATCH v4 5/6] KVM: nVMX: Fix conditions for NMI injection

Quoted 42 lines · Jan Kiszka

Quoted 31 lines · Gleb Natapov
Quoted 30 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +6 -0
index 56e7519..ad9b4bc 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;
@@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
Quoted 9 lines
arch/x86/kvm/vmx.c static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) · line 4329
Quoted 8 lines
arch/x86/kvm/vmx.c +6 -0
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending ||
+		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
+			   GUEST_ACTIVITY_WAIT_SIPI)
The same is true for interrupt too,

Quoted 2 lines

Yes, but aren't we already waiting with interrupts disabled in that state?

Why? L1 can do vmcs_write(GUEST_ACTIVITY_STATE,
GUEST_ACTIVITY_WAIT_SIPI) at any point.

Quoted 7 lines

Quoted 3 lines
but I do not think that we should allow
nested guest directly enter GUEST_ACTIVITY_WAIT_SIPI state or any other state except ACTIVE. They should be emulated instead.
Well, they aren't emulated yet but directly applied. So I think the patch is correct in the current context at least.

If my understanding is correct the facts that it is directly applied is a serious bug and should be fixed.

Quoted 2 lines

What negative effects do you expect from entering those states with L2?

As I wrote below (and you misunderstood me :)) it looks like L0 external interrupts do not generate vmexit while active VMCS is in Wait-For-SIPI state. In any case we should carefully examine what are the implications of this state since KVM never uses it and does not know how to handle it.

Quoted 9 lines

Quoted 3 lines
From quick look at
the spec it looks like external interrupts do not cause VMEXIT while vcpu is in GUEST_ACTIVITY_WAIT_SIPI.
Yes, see above.
Jan

--
Gleb.

Jan Kiszka

author

2013-04-14 16:35 UTC #9 lore raw

Re: [PATCH v4 5/6] KVM: nVMX: Fix conditions for NMI injection

Quoted 43 lines · Gleb Natapov

Quoted 42 lines · Jan Kiszka
Quoted 40 lines · Gleb Natapov
Quoted 29 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +6 -0
index 56e7519..ad9b4bc 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;
arch/x86/kvm/vmx.c static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) · line 4329
Quoted 9 lines
arch/x86/kvm/vmx.c +6 -0
@@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending ||
+		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
+			   GUEST_ACTIVITY_WAIT_SIPI)
The same is true for interrupt too,

Quoted 4 lines

Quoted 2 lines
Yes, but aren't we already waiting with interrupts disabled in that state?
Why? L1 can do vmcs_write(GUEST_ACTIVITY_STATE,
GUEST_ACTIVITY_WAIT_SIPI) at any point.

Hmm, ok.

Quoted 10 lines

Quoted 8 lines
Quoted 4 lines
but I do not think that we should allow
nested guest directly enter GUEST_ACTIVITY_WAIT_SIPI state or any other state except ACTIVE. They should be emulated instead.
Well, they aren't emulated yet but directly applied. So I think the patch is correct in the current context at least.
If my understanding is correct the facts that it is directly applied is a serious bug and should be fixed.

Yeah, L1 could put the vCPU in wait-for-SIPI, and only that physical signal will wake it up again. But L0 will not send it...

Quoted 7 lines

Quoted 3 lines
What negative effects do you expect from entering those states with L2?
As I wrote below (and you misunderstood me :)) it looks like L0 external interrupts do not generate vmexit while active VMCS is in Wait-For-SIPI state. In any case we should carefully examine what are the implications of this state since KVM never uses it and does not know how to handle it.

OK, but this should be conceptually unrelated to this patch. So, given that you applied patch 4, should I simply remove the activity state check from this one in order to proceed with it?

Jan

Gleb Natapov

2013-04-14 16:43 UTC #10 lore raw

Re: [PATCH v4 5/6] KVM: nVMX: Fix conditions for NMI injection

Quoted 68 lines · Jan Kiszka

Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Jan Kiszka
Quoted 40 lines · Gleb Natapov
Quoted 29 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +6 -0
index 56e7519..ad9b4bc 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;
arch/x86/kvm/vmx.c static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) · line 4329
Quoted 9 lines
arch/x86/kvm/vmx.c +6 -0
@@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending ||
+		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
+			   GUEST_ACTIVITY_WAIT_SIPI)
The same is true for interrupt too,
Quoted 4 lines
Quoted 2 lines
Yes, but aren't we already waiting with interrupts disabled in that state?
Why? L1 can do vmcs_write(GUEST_ACTIVITY_STATE,
GUEST_ACTIVITY_WAIT_SIPI) at any point.
Hmm, ok.
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
but I do not think that we should allow
nested guest directly enter GUEST_ACTIVITY_WAIT_SIPI state or any other state except ACTIVE. They should be emulated instead.
Well, they aren't emulated yet but directly applied. So I think the patch is correct in the current context at least.
If my understanding is correct the facts that it is directly applied is a serious bug and should be fixed.
Yeah, L1 could put the vCPU in wait-for-SIPI, and only that physical signal will wake it up again. But L0 will not send it...
Quoted 3 lines
Quoted 3 lines
What negative effects do you expect from entering those states with L2?

Quoted 9 lines

Quoted 4 lines
As I wrote below (and you misunderstood me :)) it looks like L0 external interrupts do not generate vmexit while active VMCS is in Wait-For-SIPI state. In any case we should carefully examine what are the implications of this state since KVM never uses it and does not know how to handle it.
OK, but this should be conceptually unrelated to this patch. So, given that you applied patch 4, should I simply remove the activity state check from this one in order to proceed with it?

Yes. Non ACTIVE vmcs state issue should be handled separately. But until it is fixed we should remember to not enable vmx nesting by default :)

--
Gleb.

Jan Kiszka

author

2013-04-14 19:04 UTC #11 lore raw

PATCH v5 5/6 KVM: nVMX: Fix conditions for NMI injection

From: Jan Kiszka <[email protected]>

The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kvm/vmx.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +26 -0

index 74dea94..9ad30d8 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;

@@ -4315,6 +4321,26 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending)
+			return 0;
+		if (nested_exit_on_nmi(vcpu)) {
+			nested_vmx_vmexit(vcpu);
+			vmcs12->vm_exit_reason = EXIT_REASON_EXCEPTION_NMI;
+			vmcs12->vm_exit_intr_info = NMI_VECTOR |
+				INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK;
+			/*
+			 * The NMI-triggered VM exit counts as injection:
+			 * clear this one and block further NMIs.
+			 */
+			vcpu->arch.nmi_pending = 0;
+			vmx_set_nmi_mask(vcpu, true);
+			return 0;
+		}
+	}
+
 	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
 		return 0;

Signature · 1 line

1.7.3.4

Gleb Natapov

Reviewed-by on v5 5/6

2013-04-15 15:42 UTC #12 lore raw

Re: [PATCH v5 5/6] KVM: nVMX: Fix conditions for NMI injection

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>

Reviewed-by Gleb Natapov <[email protected]>

This should be applied after not yet applied 3/6.

Quoted 50 lines

---
 arch/x86/kvm/vmx.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +26 -0

index 74dea94..9ad30d8 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;

@@ -4315,6 +4321,26 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending)
+			return 0;
+		if (nested_exit_on_nmi(vcpu)) {
+			nested_vmx_vmexit(vcpu);
+			vmcs12->vm_exit_reason = EXIT_REASON_EXCEPTION_NMI;
+			vmcs12->vm_exit_intr_info = NMI_VECTOR |
+				INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK;
+			/*
+			 * The NMI-triggered VM exit counts as injection:
+			 * clear this one and block further NMIs.
+			 */
+			vcpu->arch.nmi_pending = 0;
+			vmx_set_nmi_mask(vcpu, true);
+			return 0;
+		}
+	}
+
 	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
 		return 0;

Signature · 1 line

1.7.3.4

--
Gleb.

Gleb Natapov

Applied

2013-04-22 08:11 UTC #13 lore raw

Re: [PATCH v5 5/6] KVM: nVMX: Fix conditions for NMI injection

In reply to Jan Kiszka

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
The logic for checking if interrupts can be injected has to be applied also on NMIs. The difference is that if NMI interception is on these events are consumed and blocked by the VM exit.
Signed-off-by Jan Kiszka <[email protected]>

Applied, thanks.

Quoted 50 lines

---
 arch/x86/kvm/vmx.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +26 -0

index 74dea94..9ad30d8 100644
@@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
+		PIN_BASED_NMI_EXITING;
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	u32 cpu_based_vm_exec_control;

@@ -4315,6 +4321,26 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
 {
+	if (is_guest_mode(vcpu)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+
+		if (to_vmx(vcpu)->nested.nested_run_pending)
+			return 0;
+		if (nested_exit_on_nmi(vcpu)) {
+			nested_vmx_vmexit(vcpu);
+			vmcs12->vm_exit_reason = EXIT_REASON_EXCEPTION_NMI;
+			vmcs12->vm_exit_intr_info = NMI_VECTOR |
+				INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK;
+			/*
+			 * The NMI-triggered VM exit counts as injection:
+			 * clear this one and block further NMIs.
+			 */
+			vcpu->arch.nmi_pending = 0;
+			vmx_set_nmi_mask(vcpu, true);
+			return 0;
+		}
+	}
+
 	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
 		return 0;

Signature · 1 line

1.7.3.4

--
Gleb.

Jan Kiszka

author

2013-04-14 10:13 UTC #14 lore raw

PATCH v4 6/6 KVM: nVMX: Avoid reading VM_EXIT_INTR_ERROR_CODE needlessly on nested exits

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

We only need to update vm_exit_intr_error_code if there is a valid exit interruption information and it comes with a valid error code.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kvm/vmx.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

arch/x86/kvm/vmx.c +5 -1

index ad9b4bc..805deff 100644
@@ -7454,7 +7454,11 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	vmcs12->exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
 
 	vmcs12->vm_exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
-	vmcs12->vm_exit_intr_error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
+	if ((vmcs12->vm_exit_intr_info &
+	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
+	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK))
+		vmcs12->vm_exit_intr_error_code =
+			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
 	vmcs12->idt_vectoring_info_field = 0;
 	vmcs12->vm_exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);

Signature · 1 line

1.7.3.4

Gleb Natapov

2013-04-14 15:34 UTC #15 lore raw

In reply to Jan Kiszka

Quoted 8 lines · Jan Kiszka

Changes in this version:
- reworked patch 2, 4 and 5 according to review
- restored clearing of vm_entry_intr_info_field
- no longer read idt_vectoring_info_field from VMCS
- removed check of idt_vectoring_info_field on vmx_nmi_allowed/ vmx_interrupt_allowed
- added optimization patch 6

Applied everything except 3 and 5. Thanks.

Quoted 10 lines

Jan Kiszka (6):
KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to
    L1
KVM: nVMX: Rework event injection and recovery
KVM: VMX: Move vmx_nmi_allowed after vmx_set_nmi_mask
KVM: nVMX: Fix conditions for interrupt injection
KVM: nVMX: Fix conditions for NMI injection
KVM: nVMX: Avoid reading VM_EXIT_INTR_ERROR_CODE needlessly on nested
exits

Quoted 5 lines

 arch/x86/kvm/vmx.c |  182 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 121 insertions(+), 61 deletions(-)

Signature · 1 line

1.7.3.4

--
Gleb.