KVM: nVMX: Rework event injection and recovery

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) &&
+	    vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK) {
+		/*
+		 * Preserve the event that was supposed to be injected
+		 * by emulating it would have been returned in
+		 * IDT_VECTORING_INFO_FIELD.

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

Quoted 5 lines

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

@@ -7403,9 +7375,32 @@ void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	vmcs12->vm_exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
 

arch/x86/kvm/vmx.c void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) · line 7381

Quoted 6 lines

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

-	/* clear vm-entry fields which are to be cleared on exit */
-	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
+	/* drop what we picked up for L0 via vmx_complete_interrupts */
+	vcpu->arch.nmi_injected = false;
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);

It would be nice to move these lines out of prepare_vmcs12(), since they don't really do anything with vmcs12, and move it into nested_vmx_vmexit() (which is the one which called prepare_vmcs12()).

Did you test this both with PIN_BASED_EXT_INTR_MASK (the usual case) and !PIN_BASED_EXT_INTR_MASK (the case which interests you)? We need to make sure that in the former case, this doesn't clear the interrupt queue after we put an interrupt to be injected in it (at first glance it seems fine, but these code paths are so convoluted, it's hard to be sure).

Quoted 16 lines

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

+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) &&
+	    vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK) {
+		/*
+		 * Preserve the event that was supposed to be injected
+		 * by emulating it would have been returned in
+		 * IDT_VECTORING_INFO_FIELD.
+		 */
+		if (vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) &
+		    INTR_INFO_VALID_MASK) {
+			vmcs12->idt_vectoring_info_field =
+				vmcs12->vm_entry_intr_info_field;
+			vmcs12->idt_vectoring_error_code =
+				vmcs12->vm_entry_exception_error_code;
+			vmcs12->vm_exit_instruction_len =
+				vmcs12->vm_entry_instruction_len;

arch/x86/kvm/vmx.c void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) · line 7398

Quoted 1 line

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

+			vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);

I'm afraid I'm missing what you are trying to do here. Why would vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) & INTR_INFO_VALID_MASK ever be true? After all, the processor clears it after each sucessful exit so last if() will only succeed on failed entries - but this is NOT the case if we're in the enclosing if (note that vmcs12->vm_exit_reason = vmcs_read32(VM_EXIT_REASON)). Maybe I'm missing something?

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

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

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

index dd3a8a0..7d2fbd2 100644
@@ -6489,8 +6489,6 @@ static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	if (is_guest_mode(&vmx->vcpu))
-		return;
 	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
 				  VM_EXIT_INSTRUCTION_LEN,
 				  IDT_VECTORING_ERROR_CODE);

@@ -6498,7 +6496,7 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 
 static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
 {
-	if (is_guest_mode(vcpu))
+	if (to_vmx(vcpu)->nested.nested_run_pending)
 		return;
 	__vmx_complete_interrupts(to_vmx(vcpu),
 				  vmcs_read32(VM_ENTRY_INTR_INFO_FIELD),

@@ -6531,21 +6529,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();

@@ -6704,17 +6687,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);

@@ -7403,9 +7375,32 @@ void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	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))
+	/* drop what we picked up for L0 via vmx_complete_interrupts */
+	vcpu->arch.nmi_injected = false;
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
+
+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) &&
+	    vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK) {
+		/*
+		 * Preserve the event that was supposed to be injected
+		 * by emulating it would have been returned in
+		 * IDT_VECTORING_INFO_FIELD.
+		 */
+		if (vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) &
+		    INTR_INFO_VALID_MASK) {
+			vmcs12->idt_vectoring_info_field =
+				vmcs12->vm_entry_intr_info_field;
+			vmcs12->idt_vectoring_error_code =
+				vmcs12->vm_entry_exception_error_code;
+			vmcs12->vm_exit_instruction_len =
+				vmcs12->vm_entry_instruction_len;
+			vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);
+		}
+
+		/* clear vm-entry fields which are to be cleared on exit */
 		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
+	}
 }
 
 /*

Quoted 19 lines · Jan Kiszka

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Quoted 3 lines

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

Forgot to point this out again: It still takes "KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to L1" to make L0->L2 injection work. So this patch logically depends on it.

Jan

Quoted 100 lines

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

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

index dd3a8a0..7d2fbd2 100644
@@ -6489,8 +6489,6 @@ static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	if (is_guest_mode(&vmx->vcpu))
-		return;
 	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
 				  VM_EXIT_INSTRUCTION_LEN,
 				  IDT_VECTORING_ERROR_CODE);

@@ -6498,7 +6496,7 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 
 static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
 {
-	if (is_guest_mode(vcpu))
+	if (to_vmx(vcpu)->nested.nested_run_pending)
 		return;
 	__vmx_complete_interrupts(to_vmx(vcpu),
 				  vmcs_read32(VM_ENTRY_INTR_INFO_FIELD),

@@ -6531,21 +6529,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();

@@ -6704,17 +6687,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);

@@ -7403,9 +7375,32 @@ void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	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))
+	/* drop what we picked up for L0 via vmx_complete_interrupts */
+	vcpu->arch.nmi_injected = false;
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
+
+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) &&
+	    vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK) {
+		/*
+		 * Preserve the event that was supposed to be injected
+		 * by emulating it would have been returned in
+		 * IDT_VECTORING_INFO_FIELD.
+		 */
+		if (vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) &
+		    INTR_INFO_VALID_MASK) {
+			vmcs12->idt_vectoring_info_field =
+				vmcs12->vm_entry_intr_info_field;
+			vmcs12->idt_vectoring_error_code =
+				vmcs12->vm_entry_exception_error_code;
+			vmcs12->vm_exit_instruction_len =
+				vmcs12->vm_entry_instruction_len;
+			vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);
+		}
+
+		/* clear vm-entry fields which are to be cleared on exit */
 		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
+	}
 }
 

Quoted 2 lines

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

 /*

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Quoted 24 lines · Jan Kiszka

Quoted 22 lines · Jan Kiszka

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

Quoted 4 lines

Forgot to point this out again: It still takes "KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to L1" to make L0->L2 injection work. So this patch logically depends on it.

But this patch has hunks from that patch.

Quoted 25 lines · Gleb Natapov

Quoted 24 lines · Jan Kiszka

Quoted 22 lines · Jan Kiszka

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

Quoted 5 lines

Quoted 4 lines

Forgot to point this out again: It still takes "KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to L1" to make L0->L2 injection work. So this patch logically depends on it.

But this patch has hunks from that patch.

Not mechanically.

If you prefer me merging them together, let me know.

Quoted 30 lines · Jan Kiszka

Quoted 29 lines · Gleb Natapov

Quoted 28 lines · Jan Kiszka

Quoted 16 lines · Jan Kiszka

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

Quoted 6 lines

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

Forgot to point this out again: It still takes "KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to L1" to make L0->L2 injection work. So this patch logically depends on it.

Quoted 4 lines

Quoted 1 line

But this patch has hunks from that patch.

Not mechanically.

What do you mean?

Quoted 3 lines

If you prefer me merging them together, let me know.

For review not necessary, for applying preferably.

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

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

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

index dd3a8a0..7d2fbd2 100644
@@ -6489,8 +6489,6 @@ static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	if (is_guest_mode(&vmx->vcpu))
-		return;
 	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
 				  VM_EXIT_INSTRUCTION_LEN,
 				  IDT_VECTORING_ERROR_CODE);

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

 	__vmx_complete_interrupts(to_vmx(vcpu),
 				  vmcs_read32(VM_ENTRY_INTR_INFO_FIELD),

@@ -6531,21 +6529,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();

@@ -6704,17 +6687,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);

@@ -7403,9 +7375,32 @@ void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	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))
+	/* drop what we picked up for L0 via vmx_complete_interrupts */
+	vcpu->arch.nmi_injected = false;
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
+
+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) &&
+	    vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK) {
+		/*
+		 * Preserve the event that was supposed to be injected
+		 * by emulating it would have been returned in
+		 * IDT_VECTORING_INFO_FIELD.
+		 */
+		if (vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) &
+		    INTR_INFO_VALID_MASK) {
+			vmcs12->idt_vectoring_info_field =
+				vmcs12->vm_entry_intr_info_field;
+			vmcs12->idt_vectoring_error_code =
+				vmcs12->vm_entry_exception_error_code;
+			vmcs12->vm_exit_instruction_len =
+				vmcs12->vm_entry_instruction_len;
+			vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);
+		}
+
+		/* clear vm-entry fields which are to be cleared on exit */
 		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
+	}
 }
 
 /*

Signature · 1 line

1.7.3.4

Quoted 41 lines · Jan Kiszka

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

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

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

index dd3a8a0..7d2fbd2 100644
@@ -6489,8 +6489,6 @@ static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	if (is_guest_mode(&vmx->vcpu))
-		return;
 	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
 				  VM_EXIT_INSTRUCTION_LEN,
 				  IDT_VECTORING_ERROR_CODE);

@@ -6498,7 +6496,7 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 

arch/x86/kvm/vmx.c static void vmx_complete_interrupts(struct vcpu_vmx *vmx) · line 6500

Quoted 5 lines

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

 static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
 {
-	if (is_guest_mode(vcpu))
+	if (to_vmx(vcpu)->nested.nested_run_pending)
 		return;

Why is this needed here?

Quoted 42 lines · Gleb Natapov

Quoted 41 lines · Jan Kiszka

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

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

---

Survived moderate testing here and (currently) makes sense to me, but please review very carefully. I wouldn't be surprised if I'm still missing some subtle corner case.

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

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

index dd3a8a0..7d2fbd2 100644
@@ -6489,8 +6489,6 @@ static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	if (is_guest_mode(&vmx->vcpu))
-		return;
 	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
 				  VM_EXIT_INSTRUCTION_LEN,
 				  IDT_VECTORING_ERROR_CODE);

@@ -6498,7 +6496,7 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 

Quoted 6 lines

arch/x86/kvm/vmx.c static void vmx_complete_interrupts(struct vcpu_vmx *vmx) · line 6500

Quoted 5 lines

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

 static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
 {
-	if (is_guest_mode(vcpu))
+	if (to_vmx(vcpu)->nested.nested_run_pending)
 		return;

Why is this needed here?

Please check if my reply to Nadav explains this sufficiently.

Quoted 31 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 7 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

Quoted 1 line

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 34 lines · Jan Kiszka

Quoted 32 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 7 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

Quoted 5 lines

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

Quoted 45 lines · Jan Kiszka

Quoted 43 lines · Gleb Natapov

Quoted 32 lines · Jan Kiszka

Quoted 31 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 7 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

Quoted 7 lines

Quoted 1 line

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

Quoted 3 lines

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Not sure if we can and should handle this scenario in enable_irq_window in a nicer way. Open for suggestions.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Quoted 50 lines · Jan Kiszka

Quoted 48 lines · Jan Kiszka

Quoted 35 lines · Gleb Natapov

Quoted 34 lines · Jan Kiszka

Quoted 32 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 7 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

Quoted 8 lines

Quoted 5 lines

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

Quoted 5 lines

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Quoted 56 lines · Gleb Natapov

Quoted 55 lines · Jan Kiszka

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 7 lines

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Quoted 3 lines

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Precisely this is the case if an IRQ for L1 arrived while we tried to enter L2 and caused the cancellation above.

Quoted 57 lines · Jan Kiszka

Quoted 56 lines · Gleb Natapov

Quoted 55 lines · Jan Kiszka

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 7 lines

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Quoted 7 lines

Quoted 3 lines

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Precisely this is the case if an IRQ for L1 arrived while we tried to enter L2 and caused the cancellation above.

But during next entry the cancelled interrupt is transfered from architectural VCPU state to VM_ENTRY_INTR_INFO_FIELD by inject_pending_event()->vmx_inject_irq(), so at the point where enable_irq_window() is called the state is exactly the same no matter whether we canceled interrupt or not during previous entry attempt. What am I missing?

Oh may be I am missing that if we do not cancel interrupt then inject_pending_event() will skip
if (vcpu->arch.interrupt.pending)

    ....

and will inject interrupt from APIC that caused cancellation of previous entry, but then this is a bug since this new interrupt will overwrite the one that is still in VM_ENTRY_INTR_INFO_FIELD from previous entry attempt and there may be another pending interrupt in APIC anyway that will cause enable_irq_window() too.

Quoted 65 lines · Gleb Natapov

Quoted 57 lines · Jan Kiszka

Quoted 56 lines · Gleb Natapov

Quoted 55 lines · Jan Kiszka

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 7 lines

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Quoted 7 lines

Quoted 3 lines

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Precisely this is the case if an IRQ for L1 arrived while we tried to enter L2 and caused the cancellation above.

Quoted 6 lines

But during next entry the cancelled interrupt is transfered from architectural VCPU state to VM_ENTRY_INTR_INFO_FIELD by inject_pending_event()->vmx_inject_irq(), so at the point where enable_irq_window() is called the state is exactly the same no matter whether we canceled interrupt or not during previous entry attempt. What am I missing?

Maybe that we normally either have an external IRQ pending in some IRQ chip or in the VCPU architectural state, not both at the same time? By transferring something that doesn't come from a virtual IRQ chip of L0 (but from the one in L1) into the architectural state, we break this assumption.

Quoted 4 lines

Oh may be I am missing that if we do not cancel interrupt then inject_pending_event() will skip
if (vcpu->arch.interrupt.pending)

    ....

If we do not cancel, we will not inject at all (due to missing KVM_REQ_EVENT).

Quoted 5 lines

and will inject interrupt from APIC that caused cancellation of previous entry, but then this is a bug since this new interrupt will overwrite the one that is still in VM_ENTRY_INTR_INFO_FIELD from previous entry attempt and there may be another pending interrupt in APIC anyway that will cause enable_irq_window() too.

Maybe the issue is that we do not properly simulate a VMEXIT on an external interrupt during vmrun (like SVM does). Need to check for this case again...

Quoted 93 lines · Jan Kiszka

Quoted 65 lines · Gleb Natapov

Quoted 57 lines · Jan Kiszka

Quoted 56 lines · Gleb Natapov

Quoted 55 lines · Jan Kiszka

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 7 lines

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Quoted 7 lines

Quoted 3 lines

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Precisely this is the case if an IRQ for L1 arrived while we tried to enter L2 and caused the cancellation above.

Quoted 6 lines

But during next entry the cancelled interrupt is transfered from architectural VCPU state to VM_ENTRY_INTR_INFO_FIELD by inject_pending_event()->vmx_inject_irq(), so at the point where enable_irq_window() is called the state is exactly the same no matter whether we canceled interrupt or not during previous entry attempt. What am I missing?

Maybe that we normally either have an external IRQ pending in some IRQ chip or in the VCPU architectural state, not both at the same time? By transferring something that doesn't come from a virtual IRQ chip of L0 (but from the one in L1) into the architectural state, we break this assumption.

Quoted 4 lines

Oh may be I am missing that if we do not cancel interrupt then inject_pending_event() will skip
if (vcpu->arch.interrupt.pending)

    ....

If we do not cancel, we will not inject at all (due to missing KVM_REQ_EVENT).

Quoted 5 lines

and will inject interrupt from APIC that caused cancellation of previous entry, but then this is a bug since this new interrupt will overwrite the one that is still in VM_ENTRY_INTR_INFO_FIELD from previous entry attempt and there may be another pending interrupt in APIC anyway that will cause enable_irq_window() too.

Quoted 3 lines

Maybe the issue is that we do not properly simulate a VMEXIT on an external interrupt during vmrun (like SVM does). Need to check for this case again...

static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)

{
	if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) {
		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
		if (to_vmx(vcpu)->nested.nested_run_pending ||
		    (vmcs12->idt_vectoring_info_field &
		     VECTORING_INFO_VALID_MASK))
			return 0;
		nested_vmx_vmexit(vcpu);
		vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT;
		vmcs12->vm_exit_intr_info = 0;
		...

I do not understand ATM why we refuse to simulate a vmexit due to an external interrupt when we are about to run L2 or have something in idt_vectoring_info_field. The external interrupt would not overwrite idt_vectoring_info_field but should end up in vm_exit_intr_info.

Quoted 111 lines · Jan Kiszka

Quoted 93 lines · Jan Kiszka

Quoted 65 lines · Gleb Natapov

Quoted 57 lines · Jan Kiszka

Quoted 56 lines · Gleb Natapov

Quoted 55 lines · Jan Kiszka

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for nested guests. The changes allow to inject interrupts directly from L0 to L2.

One difference to SVM is that we always transfer the pending event injection into the architectural state of the VCPU and then drop it from there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD still contains a valid event and, if yes, transfer the content into L1's idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 7 lines

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Quoted 7 lines

Quoted 3 lines

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Precisely this is the case if an IRQ for L1 arrived while we tried to enter L2 and caused the cancellation above.

Quoted 6 lines

But during next entry the cancelled interrupt is transfered from architectural VCPU state to VM_ENTRY_INTR_INFO_FIELD by inject_pending_event()->vmx_inject_irq(), so at the point where enable_irq_window() is called the state is exactly the same no matter whether we canceled interrupt or not during previous entry attempt. What am I missing?

Maybe that we normally either have an external IRQ pending in some IRQ chip or in the VCPU architectural state, not both at the same time? By transferring something that doesn't come from a virtual IRQ chip of L0 (but from the one in L1) into the architectural state, we break this assumption.

Quoted 4 lines

Oh may be I am missing that if we do not cancel interrupt then inject_pending_event() will skip
if (vcpu->arch.interrupt.pending)

    ....

If we do not cancel, we will not inject at all (due to missing KVM_REQ_EVENT).

Quoted 5 lines

and will inject interrupt from APIC that caused cancellation of previous entry, but then this is a bug since this new interrupt will overwrite the one that is still in VM_ENTRY_INTR_INFO_FIELD from previous entry attempt and there may be another pending interrupt in APIC anyway that will cause enable_irq_window() too.

Quoted 3 lines

Maybe the issue is that we do not properly simulate a VMEXIT on an external interrupt during vmrun (like SVM does). Need to check for this case again...

static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)

{
	if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) {
		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
		if (to_vmx(vcpu)->nested.nested_run_pending ||
		    (vmcs12->idt_vectoring_info_field &
		     VECTORING_INFO_VALID_MASK))
			return 0;
		nested_vmx_vmexit(vcpu);
		vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT;
		vmcs12->vm_exit_intr_info = 0;
		...

Quoted 4 lines

I do not understand ATM why we refuse to simulate a vmexit due to an external interrupt when we are about to run L2 or have something in idt_vectoring_info_field. The external interrupt would not overwrite idt_vectoring_info_field but should end up in vm_exit_intr_info.

Explained in 51cfe38ea5: idt_vectoring_info_field and vm_exit_intr_info must not be valid at the same time.

Quoted 112 lines · Jan Kiszka

Quoted 111 lines · Jan Kiszka

Quoted 93 lines · Jan Kiszka

Quoted 65 lines · Gleb Natapov

Quoted 57 lines · Jan Kiszka

Quoted 56 lines · Gleb Natapov

Quoted 55 lines · Jan Kiszka

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Jan Kiszka

Quoted 28 lines · Nadav Har'El

Hi,

By the way, if you haven't seen my description of why the current code did what it did, take a look at
http://www.mail-archive.com/[email protected]/msg54478.html
Another description might also come in handy:
http://www.mail-archive.com/[email protected]/msg54476.html

On Wed, Feb 20, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 7 lines

This aligns VMX more with SVM regarding event injection and recovery for
nested guests. The changes allow to inject interrupts directly from L0
to L2.

One difference to SVM is that we always transfer the pending event
injection into the architectural state of the VCPU and then drop it from
there if it turns out that we left L2 to enter L1.

Last time I checked, if I'm remembering correctly, the nested SVM code did something a bit different: After the exit from L2 to L1 and unnecessarily queuing the pending interrupt for injection, it skipped one entry into L1, and as usual after the entry the interrupt queue is cleared so next time around, when L1 one is really entered, the wrong injection is not attempted.

Quoted 4 lines

VMX and SVM are now identical in how they recover event injections from
unperformed vmlaunch/vmresume: We detect that VM_ENTRY_INTR_INFO_FIELD
still contains a valid event and, if yes, transfer the content into L1's
idt_vectoring_info_field.

Quoted 4 lines

Quoted 2 lines

To avoid that we incorrectly leak an event into the architectural VCPU
state that L1 wants to inject, we skip cancellation on nested run.

I didn't understand this last point.

- prepare_vmcs02 sets event to be injected into L2
- while trying to enter L2, a cancel condition is met
- we call vmx_cancel_interrupts but should now avoid filling L1's event into the arch event queues - it's kept in vmcs12

Quoted 7 lines

Quoted 3 lines

But what if we put it in arch event queue? It will be reinjected during next entry attempt, so nothing bad happens and we have one less if() to explain, or do I miss something terrible that will happen?

I started without that if but ran into troubles with KVM-on-KVM (L1 locks up). Let me dig out the instrumentation and check the event flow again.

OK, got it again: If we transfer an IRQ that L1 wants to send to L2 into the architectural VCPU state, we will also trigger enable_irq_window. And that raises KVM_REQ_IMMEDIATE_EXIT again as it thinks L0 wants inject. That will send us into an endless loop.

Quoted 7 lines

Quoted 3 lines

Why would we trigger enable_irq_window()? enable_irq_window() triggers only if interrupt is pending in one of irq chips, not in architectural VCPU state.

Precisely this is the case if an IRQ for L1 arrived while we tried to enter L2 and caused the cancellation above.

Quoted 6 lines

But during next entry the cancelled interrupt is transfered from architectural VCPU state to VM_ENTRY_INTR_INFO_FIELD by inject_pending_event()->vmx_inject_irq(), so at the point where enable_irq_window() is called the state is exactly the same no matter whether we canceled interrupt or not during previous entry attempt. What am I missing?

Maybe that we normally either have an external IRQ pending in some IRQ chip or in the VCPU architectural state, not both at the same time? By transferring something that doesn't come from a virtual IRQ chip of L0 (but from the one in L1) into the architectural state, we break this assumption.

Quoted 4 lines

Oh may be I am missing that if we do not cancel interrupt then inject_pending_event() will skip
if (vcpu->arch.interrupt.pending)

    ....

If we do not cancel, we will not inject at all (due to missing KVM_REQ_EVENT).

Quoted 5 lines

and will inject interrupt from APIC that caused cancellation of previous entry, but then this is a bug since this new interrupt will overwrite the one that is still in VM_ENTRY_INTR_INFO_FIELD from previous entry attempt and there may be another pending interrupt in APIC anyway that will cause enable_irq_window() too.

Quoted 3 lines

Maybe the issue is that we do not properly simulate a VMEXIT on an external interrupt during vmrun (like SVM does). Need to check for this case again...

static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)

{
	if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) {
		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
		if (to_vmx(vcpu)->nested.nested_run_pending ||
		    (vmcs12->idt_vectoring_info_field &
		     VECTORING_INFO_VALID_MASK))
			return 0;
		nested_vmx_vmexit(vcpu);
		vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT;
		vmcs12->vm_exit_intr_info = 0;
		...

Quoted 8 lines

Quoted 4 lines

I do not understand ATM why we refuse to simulate a vmexit due to an external interrupt when we are about to run L2 or have something in idt_vectoring_info_field. The external interrupt would not overwrite idt_vectoring_info_field but should end up in vm_exit_intr_info.

Explained in 51cfe38ea5: idt_vectoring_info_field and vm_exit_intr_info must not be valid at the same time.

Interestingly, if we transfer interrupt from idt_vectoring_info into arch VCPU state we can drop this check because vmx_interrupt_allowed() will not be called while there is an event to reinject. 51cfe38ea5 still does not explain why nested_run_pending is needed. We cannot #vmexit without entering L2, but we can undo VMLAUNCH/VMRESUME emulation leaving rip pointing to the instruction. We can start by moving skip_emulated_instruction() from nested_vmx_run() to nested_vmx_vmexit().

will not be called while there is an event to reinject. 51cfe38ea5 still does not explain why nested_run_pending is needed. We cannot #vmexit without entering L2, but we can undo VMLAUNCH/VMRESUME emulation leaving rip pointing to the instruction. We can start by moving skip_emulated_instruction() from nested_vmx_run() to nested_vmx_vmexit().

That generally does not help to inject/report an external IRQ to L1 as L1 runs with IRQs disabled around VMLAUNCH/RESUME.

On Thu, Feb 21, 2013, Jan Kiszka wrote about "Re: [PATCH] KVM: nVMX: Rework event injection and recovery":

Quoted 2 lines

That generally does not help to inject/report an external IRQ to L1 as L1 runs with IRQs disabled around VMLAUNCH/RESUME.

Good point, I forgot that :(

So it looks like nested_run_pending was necessary, after all.