[PATCH 0/3] KVM: nVMX: Make direct IRQ/NMI injection work

[PATCH 0/3] KVM: nVMX: Make direct IRQ/NMI injection work

[Jan Kiszka]

As everyone is so busy taking nVMX patches today, I'm pushing some more:

Patch 1 I already posted earlier, it is still valid. Patch 2 & 3 were
developed out of a previous version which turned out to be incomplete.
Things became more complex since then, but they seem to work correctly
now.

These patches apply on top of what I posted so far, but should not
directly depend on the other fixes.

Thanks in advance for wrapping your mind around this topic.

Jan Kiszka (3):
  KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to
    L1
  KVM: nVMX: Fix conditions for NMI and interrupt injection
  KVM: nVMX: Rework event injection and recovery

 arch/x86/kvm/vmx.c |  187 +++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 141 insertions(+), 46 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] KVM: nVMX: Fix injection of PENDING_INTERRUPT and NMI_WINDOW exits to L1

[Jan Kiszka]

The comment was wrong: enable_irq_window might be called after
prepare_vmcs02 when we left L2 to prepare IRQ injecting for L1. Same for
NMIs.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 313f8b3..b50174d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6134,14 +6134,10 @@ 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 vmcs12->pin_based_vm_exec_control &
+			PIN_BASED_NMI_EXITING;
 	case EXIT_REASON_TASK_SWITCH:
 		return 1;
 	case EXIT_REASON_CPUID:
-- 
1.7.3.4

[PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Jan Kiszka]

If we are in guest mode, L0 can only inject events into L2 if L1 has
nothing pending. Otherwise, L0 would overwrite L1's events and they
would get lost. This check is conceptually independent of
nested_exit_on_intr.

If L1 traps external interrupts, then we also need to look at L1's
idt_vectoring_info_field. If it is empty, we can kick the guest from L2
to L1, just like the previous code worked.

Finally, the logic for checking interrupt has to be applied also on NMIs
in an analogous way. This enables NMI interception for nested guests.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b50174d..10de336 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4211,6 +4211,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;
@@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
 
 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 &&
+		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
+			return 0;
+		if (nested_exit_on_nmi(vcpu)) {
+			/*
+			 * Check if the idt_vectoring_info_field is free. We
+			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
+			 */
+			if (vmcs12->idt_vectoring_info_field &
+			    VECTORING_INFO_VALID_MASK)
+				return 0;
+			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;
+			/*
+			 * fall through to normal code, but now in L1, not L2
+			 */
+		}
+	}
+
 	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
 		return 0;
 
@@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 
 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 &&
+		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
 			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)) {
+			/*
+			 * Check if the idt_vectoring_info_field is free. We
+			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
+			 * isn't.
+			 */
+			if (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;
+			/*
+			 * fall through to normal code, but now in L1, not L2
+			 */
+		}
 	}
 
 	return (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&
-- 
1.7.3.4

[PATCH 3/3] KVM: nVMX: Rework event injection and recovery

[Jan Kiszka]

The changes finally allow to inject interrupts directly from L0 to L2.

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.

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.

However, we differ on how to deal with events that L0 wanted to inject
into L2. Likely, this case is still broken in SVM. For VMX, the function
vmcs12_save_pending_events deals with transferring 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).

Note that we treat directly injected NMIs differently as they can hit
both L1 and L2. In this case, we let L0 try to injection again also over
L1 after leaving L2.

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 <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |  118 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 87 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 10de336..3ca5461 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6559,8 +6559,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);
@@ -6568,7 +6566,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(vcpu,
 				  vmcs_read32(VM_ENTRY_INTR_INFO_FIELD),
@@ -6601,21 +6599,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();
@@ -6774,17 +6757,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);
@@ -7391,6 +7363,52 @@ vmcs12_guest_cr4(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 			vcpu->arch.cr4_guest_owned_bits));
 }
 
+static void vmcs12_save_pending_events(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
+{
+	u32 idt_vectoring;
+	unsigned int nr;
+
+	/*
+	 * We only transfer exceptions and maskable interrupts. It is fine if
+	 * L0 retries to inject a pending NMI over L1.
+	 */
+	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.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;
+	}
+
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
+}
+
 /*
  * 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),
@@ -7482,9 +7500,47 @@ static 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))
+	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
+		if ((vmcs12->vm_entry_intr_info_field &
+		     INTR_INFO_VALID_MASK) &&
+		    (vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) &
+		     INTR_INFO_VALID_MASK)) {
+			/*
+			 * Preserve the event that was supposed to be injected
+			 * by L1 via emulating it would have been returned in
+			 * IDT_VECTORING_INFO_FIELD.
+			 */
+			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);
+
+			/*
+			 * We do not drop NMIs that targeted L2 below as they
+			 * can also be reinjected over L1. But if this event
+			 * was an NMI, it was synthetic and came from L1.
+			 */
+			vcpu->arch.nmi_injected = false;
+		} else
+			/*
+			 * Transfer the event L0 may wanted to inject into L2
+			 * to IDT_VECTORING_INFO_FIELD.
+			 */
+			vmcs12_save_pending_events(vcpu, vmcs12);
+
+		/* clear vm-entry fields which are to be cleared on exit */
 		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
+	}
+
+	/*
+	 * Drop what we picked up for L2 via vmx_complete_interrupts. It is
+	 * preserved above and would only end up incorrectly in L1.
+	 */
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
 }
 
 /*
-- 
1.7.3.4

Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Gleb Natapov]

On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
> If we are in guest mode, L0 can only inject events into L2 if L1 has
> nothing pending. Otherwise, L0 would overwrite L1's events and they
> would get lost. This check is conceptually independent of
> nested_exit_on_intr.
> 
> If L1 traps external interrupts, then we also need to look at L1's
> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
> to L1, just like the previous code worked.
> 
> Finally, the logic for checking interrupt has to be applied also on NMIs
> in an analogous way. This enables NMI interception for nested guests.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b50174d..10de336 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4211,6 +4211,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;
> +}
> +
It will take me some time to review this, but I have a small nit now.
You open code checking of this bit in your previous patch, why not move
this hunk there?

>  static void enable_irq_window(struct kvm_vcpu *vcpu)
>  {
>  	u32 cpu_based_vm_exec_control;
> @@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>  
>  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 &&
> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
> +			return 0;
> +		if (nested_exit_on_nmi(vcpu)) {
> +			/*
> +			 * Check if the idt_vectoring_info_field is free. We
> +			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
> +			 */
> +			if (vmcs12->idt_vectoring_info_field &
> +			    VECTORING_INFO_VALID_MASK)
> +				return 0;
> +			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;
> +			/*
> +			 * fall through to normal code, but now in L1, not L2
> +			 */
> +		}
> +	}
> +
>  	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
>  		return 0;
>  
> @@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>  
>  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 &&
> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>  			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)) {
> +			/*
> +			 * Check if the idt_vectoring_info_field is free. We
> +			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
> +			 * isn't.
> +			 */
> +			if (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;
> +			/*
> +			 * fall through to normal code, but now in L1, not L2
> +			 */
> +		}
>  	}
>  
>  	return (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&
> -- 
> 1.7.3.4

--
			Gleb.

Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Gleb Natapov]

On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
> If we are in guest mode, L0 can only inject events into L2 if L1 has
> nothing pending. Otherwise, L0 would overwrite L1's events and they
> would get lost. This check is conceptually independent of
> nested_exit_on_intr.
> 
> If L1 traps external interrupts, then we also need to look at L1's
> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
> to L1, just like the previous code worked.
> 
> Finally, the logic for checking interrupt has to be applied also on NMIs
> in an analogous way. This enables NMI interception for nested guests.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b50174d..10de336 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4211,6 +4211,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;
> @@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>  
>  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 &&
> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
> +			return 0;
> +		if (nested_exit_on_nmi(vcpu)) {
> +			/*
> +			 * Check if the idt_vectoring_info_field is free. We
> +			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
> +			 */
> +			if (vmcs12->idt_vectoring_info_field &
> +			    VECTORING_INFO_VALID_MASK)
> +				return 0;
> +			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;
> +			/*
> +			 * fall through to normal code, but now in L1, not L2
> +			 */
> +		}
> +	}
> +
>  	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
>  		return 0;
>  
> @@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>  
>  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 &&
> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>  			return 0;
I do not understand this. As far as I remember Nadav's explanation we
have to enter guest if nested_run_pending is set because VMX does not
expect vmexit to happen without running guest at all. May be
idt_vectoring_info_field processing is the only reason, may be not. I
wouldn't gamble on it.

> -		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)) {
> +			/*
> +			 * Check if the idt_vectoring_info_field is free. We
> +			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
> +			 * isn't.
> +			 */
> +			if (vmcs12->idt_vectoring_info_field &
> +			    VECTORING_INFO_VALID_MASK)
> +				return 0;
I think we actually need to return 0 if idt_vectoring_info_field is
valid even if !nested_exit_on_intr(). If we do not we let L0 inject
interrupt into L2 and then overwrite it on entry from
vmcs12->idt_vectoring_info_field.

> +			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) &&
> -- 
> 1.7.3.4

--
			Gleb.

Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Jan Kiszka]

On 2013-03-14 16:12, Gleb Natapov wrote:
> On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
>> If we are in guest mode, L0 can only inject events into L2 if L1 has
>> nothing pending. Otherwise, L0 would overwrite L1's events and they
>> would get lost. This check is conceptually independent of
>> nested_exit_on_intr.
>>
>> If L1 traps external interrupts, then we also need to look at L1's
>> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
>> to L1, just like the previous code worked.
>>
>> Finally, the logic for checking interrupt has to be applied also on NMIs
>> in an analogous way. This enables NMI interception for nested guests.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 files changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b50174d..10de336 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4211,6 +4211,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;
>> @@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>>  
>>  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 &&
>> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>> +			return 0;
>> +		if (nested_exit_on_nmi(vcpu)) {
>> +			/*
>> +			 * Check if the idt_vectoring_info_field is free. We
>> +			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
>> +			 */
>> +			if (vmcs12->idt_vectoring_info_field &
>> +			    VECTORING_INFO_VALID_MASK)
>> +				return 0;
>> +			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;
>> +			/*
>> +			 * fall through to normal code, but now in L1, not L2
>> +			 */
>> +		}
>> +	}
>> +
>>  	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
>>  		return 0;
>>  
>> @@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>>  
>>  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 &&
>> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>>  			return 0;
> I do not understand this. As far as I remember Nadav's explanation we
> have to enter guest if nested_run_pending is set because VMX does not
> expect vmexit to happen without running guest at all. May be
> idt_vectoring_info_field processing is the only reason, may be not. I
> wouldn't gamble on it.

What are the problems? Specifically if we emulate the effects of an
immediate vmexit properly. I'm not categorically excluding that some
case is missing. If we do not allow soft-vmexit here, we will set the
interrupt window later and will get such an immediate exit as well
(provided the L2 was interruptible).

> 
>> -		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)) {
>> +			/*
>> +			 * Check if the idt_vectoring_info_field is free. We
>> +			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
>> +			 * isn't.
>> +			 */
>> +			if (vmcs12->idt_vectoring_info_field &
>> +			    VECTORING_INFO_VALID_MASK)
>> +				return 0;
> I think we actually need to return 0 if idt_vectoring_info_field is
> valid even if !nested_exit_on_intr(). If we do not we let L0 inject
> interrupt into L2 and then overwrite it on entry from
> vmcs12->idt_vectoring_info_field.

Sorry, don't understand the last sentence. This check is about the
software idt_vectoring_info_field, the one we keep for L1, not the real
field.

Jan

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

Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Jan Kiszka]

On 2013-03-14 14:59, Gleb Natapov wrote:
> On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
>> If we are in guest mode, L0 can only inject events into L2 if L1 has
>> nothing pending. Otherwise, L0 would overwrite L1's events and they
>> would get lost. This check is conceptually independent of
>> nested_exit_on_intr.
>>
>> If L1 traps external interrupts, then we also need to look at L1's
>> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
>> to L1, just like the previous code worked.
>>
>> Finally, the logic for checking interrupt has to be applied also on NMIs
>> in an analogous way. This enables NMI interception for nested guests.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 files changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b50174d..10de336 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4211,6 +4211,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;
>> +}
>> +
> It will take me some time to review this, but I have a small nit now.
> You open code checking of this bit in your previous patch, why not move
> this hunk there?

True. Patch 1 is several weeks older, and I didn't recheck this. Will
clean it up.

Jan

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

Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Gleb Natapov]

On Thu, Mar 14, 2013 at 04:24:18PM +0100, Jan Kiszka wrote:
> On 2013-03-14 16:12, Gleb Natapov wrote:
> > On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
> >> If we are in guest mode, L0 can only inject events into L2 if L1 has
> >> nothing pending. Otherwise, L0 would overwrite L1's events and they
> >> would get lost. This check is conceptually independent of
> >> nested_exit_on_intr.
> >>
> >> If L1 traps external interrupts, then we also need to look at L1's
> >> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
> >> to L1, just like the previous code worked.
> >>
> >> Finally, the logic for checking interrupt has to be applied also on NMIs
> >> in an analogous way. This enables NMI interception for nested guests.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
> >>  1 files changed, 51 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index b50174d..10de336 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -4211,6 +4211,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;
> >> @@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
> >>  
> >>  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 &&
> >> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
> >> +			return 0;
> >> +		if (nested_exit_on_nmi(vcpu)) {
> >> +			/*
> >> +			 * Check if the idt_vectoring_info_field is free. We
> >> +			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
> >> +			 */
> >> +			if (vmcs12->idt_vectoring_info_field &
> >> +			    VECTORING_INFO_VALID_MASK)
> >> +				return 0;
> >> +			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;
> >> +			/*
> >> +			 * fall through to normal code, but now in L1, not L2
> >> +			 */
> >> +		}
> >> +	}
> >> +
> >>  	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
> >>  		return 0;
> >>  
> >> @@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
> >>  
> >>  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 &&
> >> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
> >>  			return 0;
> > I do not understand this. As far as I remember Nadav's explanation we
> > have to enter guest if nested_run_pending is set because VMX does not
> > expect vmexit to happen without running guest at all. May be
> > idt_vectoring_info_field processing is the only reason, may be not. I
> > wouldn't gamble on it.
> 
> What are the problems? Specifically if we emulate the effects of an
> immediate vmexit properly. I'm not categorically excluding that some
> case is missing. If we do not allow soft-vmexit here, we will set the
> interrupt window later and will get such an immediate exit as well
> (provided the L2 was interruptible).
> 
Don't know. Some field that VMX change on vmresume and since from L2
point of view vmresume was executed, but in reality it was not the
result that L2 will see will be incorrect. vm_entry_intr_info_field is
on of such fields, may be there are or will be more (vAPIC+posted
interrupt)?

> > 
> >> -		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)) {
> >> +			/*
> >> +			 * Check if the idt_vectoring_info_field is free. We
> >> +			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
> >> +			 * isn't.
> >> +			 */
> >> +			if (vmcs12->idt_vectoring_info_field &
> >> +			    VECTORING_INFO_VALID_MASK)
> >> +				return 0;
> > I think we actually need to return 0 if idt_vectoring_info_field is
> > valid even if !nested_exit_on_intr(). If we do not we let L0 inject
> > interrupt into L2 and then overwrite it on entry from
> > vmcs12->idt_vectoring_info_field.
> 
> Sorry, don't understand the last sentence. This check is about the
> software idt_vectoring_info_field, the one we keep for L1, not the real
> field.
> 

Suppose the vmcs12->idt_vectoring_info_field is valid and L0 want to
inject an interrupt directly into L2 and L2 does not block interrupts.
vmx_interrupt_allowed() will return true, so vmx_inject_irq()
will be called and L0->L2 interrupt information will be written
to VM_ENTRY_INTR_INFO_FIELD.  Now in vmx_vcpu_run() there is again
check for valid vmcs12->idt_vectoring_info_field and if it is valid
VM_ENTRY_INTR_INFO_FIELD is overwritten with it. Interrupt that L0 just
injected into L2 was lost forever.

--
			Gleb.

Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Jan Kiszka]

On 2013-03-14 16:37, Gleb Natapov wrote:
> On Thu, Mar 14, 2013 at 04:24:18PM +0100, Jan Kiszka wrote:
>> On 2013-03-14 16:12, Gleb Natapov wrote:
>>> On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
>>>> If we are in guest mode, L0 can only inject events into L2 if L1 has
>>>> nothing pending. Otherwise, L0 would overwrite L1's events and they
>>>> would get lost. This check is conceptually independent of
>>>> nested_exit_on_intr.
>>>>
>>>> If L1 traps external interrupts, then we also need to look at L1's
>>>> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
>>>> to L1, just like the previous code worked.
>>>>
>>>> Finally, the logic for checking interrupt has to be applied also on NMIs
>>>> in an analogous way. This enables NMI interception for nested guests.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  1 files changed, 51 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index b50174d..10de336 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -4211,6 +4211,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;
>>>> @@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>>>>  
>>>>  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 &&
>>>> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>>>> +			return 0;
>>>> +		if (nested_exit_on_nmi(vcpu)) {
>>>> +			/*
>>>> +			 * Check if the idt_vectoring_info_field is free. We
>>>> +			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
>>>> +			 */
>>>> +			if (vmcs12->idt_vectoring_info_field &
>>>> +			    VECTORING_INFO_VALID_MASK)
>>>> +				return 0;
>>>> +			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;
>>>> +			/*
>>>> +			 * fall through to normal code, but now in L1, not L2
>>>> +			 */
>>>> +		}
>>>> +	}
>>>> +
>>>>  	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
>>>>  		return 0;
>>>>  
>>>> @@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>>>>  
>>>>  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 &&
>>>> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>>>>  			return 0;
>>> I do not understand this. As far as I remember Nadav's explanation we
>>> have to enter guest if nested_run_pending is set because VMX does not
>>> expect vmexit to happen without running guest at all. May be
>>> idt_vectoring_info_field processing is the only reason, may be not. I
>>> wouldn't gamble on it.
>>
>> What are the problems? Specifically if we emulate the effects of an
>> immediate vmexit properly. I'm not categorically excluding that some
>> case is missing. If we do not allow soft-vmexit here, we will set the
>> interrupt window later and will get such an immediate exit as well
>> (provided the L2 was interruptible).
>>
> Don't know. Some field that VMX change on vmresume and since from L2
> point of view vmresume was executed, but in reality it was not the
> result that L2 will see will be incorrect. vm_entry_intr_info_field is
> on of such fields, may be there are or will be more (vAPIC+posted
> interrupt)?

OK, better safe than sorry. Easy to change.

> 
>>>
>>>> -		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)) {
>>>> +			/*
>>>> +			 * Check if the idt_vectoring_info_field is free. We
>>>> +			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
>>>> +			 * isn't.
>>>> +			 */
>>>> +			if (vmcs12->idt_vectoring_info_field &
>>>> +			    VECTORING_INFO_VALID_MASK)
>>>> +				return 0;
>>> I think we actually need to return 0 if idt_vectoring_info_field is
>>> valid even if !nested_exit_on_intr(). If we do not we let L0 inject
>>> interrupt into L2 and then overwrite it on entry from
>>> vmcs12->idt_vectoring_info_field.
>>
>> Sorry, don't understand the last sentence. This check is about the
>> software idt_vectoring_info_field, the one we keep for L1, not the real
>> field.
>>
> 
> Suppose the vmcs12->idt_vectoring_info_field is valid and L0 want to
> inject an interrupt directly into L2 and L2 does not block interrupts.
> vmx_interrupt_allowed() will return true, so vmx_inject_irq()
> will be called and L0->L2 interrupt information will be written
> to VM_ENTRY_INTR_INFO_FIELD.  Now in vmx_vcpu_run() there is again
> check for valid vmcs12->idt_vectoring_info_field and if it is valid
> VM_ENTRY_INTR_INFO_FIELD is overwritten with it. Interrupt that L0 just
> injected into L2 was lost forever.

OK, that's my fault in trying to split patch 2 and 3. Patch 3 will
remove that bogus overwriting from vmx_vcpu_run. I'll merge both patches
in the next round.

Jan

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