[PATCH 0/5] Nested SVM Interrupt Fixes

[PATCH 0/5] Nested SVM Interrupt Fixes

[Alexander Graf]

Looking through the interrupt handling paths I stumbled across several
inconsitencies in the interrupt handling paths.

Using this patchset, running the most recent KVM version in nested KVM
works again.

Alexander Graf (5):
  Implement #NMI exiting for nested SVM
  Don't call svm_complete_interrupts for nested guests
  Don't #VMEXIT(INTR) if we still have event_inj waiting
  Don't bail when injecting an event in nested SVM
  Notify nested hypervisor of lost event injections

 arch/x86/kvm/svm.c |   79 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 68 insertions(+), 11 deletions(-)

[PATCH 1/5] Implement #NMI exiting for nested SVM

[Alexander Graf]

When injecting an NMI to the l1 guest while it was running the l2 guest, we
didn't #VMEXIT but just injected the NMI to the l2 guest.

Let's be closer to real hardware and #VMEXIT if we're supposed to do so.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |   38 ++++++++++++++++++++++++++++++++------
 1 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 9a4daca..f12a669 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1375,6 +1375,21 @@ static int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 	return nested_svm_exit_handled(svm);
 }
 
+static inline int nested_svm_nmi(struct vcpu_svm *svm)
+{
+	if (!is_nested(svm))
+		return 0;
+
+	svm->vmcb->control.exit_code = SVM_EXIT_NMI;
+
+	if (nested_svm_exit_handled(svm)) {
+		nsvm_printk("VMexit -> NMI\n");
+		return 1;
+	}
+
+	return 0;
+}
+
 static inline int nested_svm_intr(struct vcpu_svm *svm)
 {
 	if (!is_nested(svm))
@@ -2462,7 +2477,9 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	struct vmcb *vmcb = svm->vmcb;
 	return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
-		!(svm->vcpu.arch.hflags & HF_NMI_MASK);
+		!(svm->vcpu.arch.hflags & HF_NMI_MASK) &&
+		gif_set(svm) &&
+		!is_nested(svm);
 }
 
 static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
@@ -2488,22 +2505,31 @@ static void enable_irq_window(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	nsvm_printk("Trying to open IRQ window\n");
 
-	nested_svm_intr(svm);
+	if (nested_svm_intr(svm))
+		return;
 
 	/* In case GIF=0 we can't rely on the CPU to tell us when
 	 * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
 	 * The next time we get that intercept, this function will be
 	 * called again though and we'll get the vintr intercept. */
-	if (gif_set(svm)) {
-		svm_set_vintr(svm);
-		svm_inject_irq(svm, 0x0);
-	}
+	if (!gif_set(svm))
+		return;
+
+	svm_set_vintr(svm);
+	svm_inject_irq(svm, 0x0);
 }
 
 static void enable_nmi_window(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (nested_svm_nmi(svm))
+		return;
+
+	/* NMI is deferred until GIF == 1. Setting GIF will cause a #VMEXIT */
+	if (!gif_set(svm))
+		return;
+
 	if ((svm->vcpu.arch.hflags & (HF_NMI_MASK | HF_IRET_MASK))
 	    == HF_NMI_MASK)
 		return; /* IRET will cause a vm exit */
-- 
1.6.0.2

[PATCH 2/5] Don't call svm_complete_interrupts for nested guests

[Alexander Graf]

SVM has some cleanup code, that tries to reinject interrupts and exceptions
when the guest didn't manage to deal with them yet. It basically transfers
them to KVM internal state.

Unfortunately, the internal state is reserved for the L1 guest state, so we
shouldn't try to go through that logic when running a nested guest.

When doing something the host KVM can handle, let's just reinject the event
into the L2 guest, because we didn't touch its state anyways.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f12a669..61efd13 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2349,7 +2349,10 @@ static int handle_exit(struct kvm_vcpu *vcpu)
 	trace_kvm_exit(exit_code, svm->vmcb->save.rip);
 
 	if (is_nested(svm)) {
+		struct vmcb_control_area *control = &svm->vmcb->control;
 		int vmexit;
+		int type;
+		int vec;
 
 		nsvm_printk("nested handle_exit: 0x%x | 0x%lx | 0x%lx | 0x%lx\n",
 			    exit_code, svm->vmcb->control.exit_info_1,
@@ -2362,9 +2365,18 @@ static int handle_exit(struct kvm_vcpu *vcpu)
 
 		if (vmexit == NESTED_EXIT_DONE)
 			return 1;
-	}
 
-	svm_complete_interrupts(svm);
+		type = control->exit_int_info & SVM_EXITINTINFO_TYPE_MASK;
+		vec = control->exit_int_info & SVM_EXITINTINFO_VEC_MASK;
+		if ((type == SVM_EXITINTINFO_TYPE_INTR) ||
+		    ((type == SVM_EXITINTINFO_TYPE_EXEPT) && !kvm_exception_is_soft(vec))) {
+			control->event_inj = control->exit_int_info;
+			control->event_inj_err = control->exit_int_info_err;
+		}
+	} else {
+		/* Don't interpret exit_info for nested guests */
+		svm_complete_interrupts(svm);
+	}
 
 	if (npt_enabled) {
 		int mmu_reload = 0;
@@ -2602,8 +2614,6 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	case SVM_EXITINTINFO_TYPE_EXEPT:
 		/* In case of software exception do not reinject an exception
 		   vector, but re-execute and instruction instead */
-		if (is_nested(svm))
-			break;
 		if (kvm_exception_is_soft(vector))
 			break;
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
-- 
1.6.0.2

[PATCH 3/5] Don't #VMEXIT(INTR) if we still have event_inj waiting

[Alexander Graf]

Real hardware would first process the event_inj field and then notify the
host that an interrupt is waiting.

Let's do the same and just not EXIT_INTR if we have an event pending for the
L2 guest.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 61efd13..28fcbd0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1401,6 +1401,10 @@ static inline int nested_svm_intr(struct vcpu_svm *svm)
 	if (!(svm->vcpu.arch.hflags & HF_HIF_MASK))
 		return 0;
 
+	/* We can't EXIT_INTR when we still have an event to inject */
+	if (svm->vmcb->control.event_inj)
+		return 1;
+
 	svm->vmcb->control.exit_code = SVM_EXIT_INTR;
 
 	if (nested_svm_exit_handled(svm)) {
-- 
1.6.0.2

[PATCH 4/5] Don't bail when injecting an event in nested SVM

[Alexander Graf]

There's a warning to alarm the user when the guest has a valid exit_int_info,
but really shouldn't have.

We don't want that warning when running an L2 guest, because we're in control
of the state then anyways, so let's not warn when running nested.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 28fcbd0..12ec8ee 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2404,7 +2404,8 @@ static int handle_exit(struct kvm_vcpu *vcpu)
 		return 0;
 	}
 
-	if (is_external_interrupt(svm->vmcb->control.exit_int_info) &&
+	if (!is_nested(svm) &&
+	    is_external_interrupt(svm->vmcb->control.exit_int_info) &&
 	    exit_code != SVM_EXIT_EXCP_BASE + PF_VECTOR &&
 	    exit_code != SVM_EXIT_NPF && exit_code != SVM_EXIT_TASK_SWITCH)
 		printk(KERN_ERR "%s: unexpected exit_ini_info 0x%x "
-- 
1.6.0.2

[PATCH 5/5] Notify nested hypervisor of lost event injections

[Alexander Graf]

Normally when event_inj is valid the host CPU would write the contents to
exit_int_info, so the hypervisor knows that the event wasn't injected.

We failed to do so so far, so let's model closer to the CPU.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 12ec8ee..75e3d75 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1643,6 +1643,22 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 	nested_vmcb->control.exit_info_2       = vmcb->control.exit_info_2;
 	nested_vmcb->control.exit_int_info     = vmcb->control.exit_int_info;
 	nested_vmcb->control.exit_int_info_err = vmcb->control.exit_int_info_err;
+
+	/*
+	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
+	 * to make sure that we do not lose injected events. So check event_inj
+	 * here and copy it to exit_int_info if it is valid.
+	 * exit_int_info and event_inj can't be both valid because the below
+	 * case only happens on a VMRUN instruction intercept which has not
+	 * valid exit_int_info set.
+	 */
+	if (vmcb->control.event_inj & SVM_EVTINJ_VALID) {
+		struct vmcb_control_area *nc = &nested_vmcb->control;
+
+		nc->exit_int_info     = vmcb->control.event_inj;
+		nc->exit_int_info_err = vmcb->control.event_inj_err;
+	}
+
 	nested_vmcb->control.tlb_ctl           = 0;
 	nested_vmcb->control.event_inj         = 0;
 	nested_vmcb->control.event_inj_err     = 0;
-- 
1.6.0.2

Re: [PATCH 5/5] Notify nested hypervisor of lost event injections

[Joerg Roedel]

On Fri, Sep 18, 2009 at 03:00:32PM +0200, Alexander Graf wrote:
> Normally when event_inj is valid the host CPU would write the contents to
> exit_int_info, so the hypervisor knows that the event wasn't injected.
> 
> We failed to do so so far, so let's model closer to the CPU.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Acked-by: Joerg Roedel <joerg.roedel@amd.com>

The commit-message does not state this explicitly, but this patch fixes
a real bug with lost interrupts in nested svm.

> ---
>  arch/x86/kvm/svm.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 12ec8ee..75e3d75 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1643,6 +1643,22 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
>  	nested_vmcb->control.exit_info_2       = vmcb->control.exit_info_2;
>  	nested_vmcb->control.exit_int_info     = vmcb->control.exit_int_info;
>  	nested_vmcb->control.exit_int_info_err = vmcb->control.exit_int_info_err;
> +
> +	/*
> +	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> +	 * to make sure that we do not lose injected events. So check event_inj
> +	 * here and copy it to exit_int_info if it is valid.
> +	 * exit_int_info and event_inj can't be both valid because the below
> +	 * case only happens on a VMRUN instruction intercept which has not
> +	 * valid exit_int_info set.
> +	 */
> +	if (vmcb->control.event_inj & SVM_EVTINJ_VALID) {
> +		struct vmcb_control_area *nc = &nested_vmcb->control;
> +
> +		nc->exit_int_info     = vmcb->control.event_inj;
> +		nc->exit_int_info_err = vmcb->control.event_inj_err;
> +	}
> +
>  	nested_vmcb->control.tlb_ctl           = 0;
>  	nested_vmcb->control.event_inj         = 0;
>  	nested_vmcb->control.event_inj_err     = 0;
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] Notify nested hypervisor of lost event injections

[Joerg Roedel]

Hi Avi,

can you pleas apply this patch (only 5/5) directly before Alex does a
repost? It is pretty independet from the others and contains an
important bugfix for nested svm and should go in as soon as possible.

	Joerg

On Fri, Sep 18, 2009 at 03:00:32PM +0200, Alexander Graf wrote:
> Normally when event_inj is valid the host CPU would write the contents to
> exit_int_info, so the hypervisor knows that the event wasn't injected.
> 
> We failed to do so so far, so let's model closer to the CPU.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 12ec8ee..75e3d75 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1643,6 +1643,22 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
>  	nested_vmcb->control.exit_info_2       = vmcb->control.exit_info_2;
>  	nested_vmcb->control.exit_int_info     = vmcb->control.exit_int_info;
>  	nested_vmcb->control.exit_int_info_err = vmcb->control.exit_int_info_err;
> +
> +	/*
> +	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> +	 * to make sure that we do not lose injected events. So check event_inj
> +	 * here and copy it to exit_int_info if it is valid.
> +	 * exit_int_info and event_inj can't be both valid because the below
> +	 * case only happens on a VMRUN instruction intercept which has not
> +	 * valid exit_int_info set.
> +	 */
> +	if (vmcb->control.event_inj & SVM_EVTINJ_VALID) {
> +		struct vmcb_control_area *nc = &nested_vmcb->control;
> +
> +		nc->exit_int_info     = vmcb->control.event_inj;
> +		nc->exit_int_info_err = vmcb->control.event_inj_err;
> +	}
> +
>  	nested_vmcb->control.tlb_ctl           = 0;
>  	nested_vmcb->control.event_inj         = 0;
>  	nested_vmcb->control.event_inj_err     = 0;
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] Don't #VMEXIT(INTR) if we still have event_inj waiting

[Joerg Roedel]

On Fri, Sep 18, 2009 at 03:00:30PM +0200, Alexander Graf wrote:
> Real hardware would first process the event_inj field and then notify the
> host that an interrupt is waiting.

Does it really? I couldn't find this in the SVM spec.

> Let's do the same and just not EXIT_INTR if we have an event pending for the
> L2 guest.

Anyway. I think this case is handled good enough with patch 5/5. This
patch, to be complete must also enable single-steping to exit again
after the first instruction of the exception handler has ran to inject
the interrupt. But that would make the whole thing rather compĺicated.

> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 61efd13..28fcbd0 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1401,6 +1401,10 @@ static inline int nested_svm_intr(struct vcpu_svm *svm)
>  	if (!(svm->vcpu.arch.hflags & HF_HIF_MASK))
>  		return 0;
>  
> +	/* We can't EXIT_INTR when we still have an event to inject */
> +	if (svm->vmcb->control.event_inj)
> +		return 1;
> +
>  	svm->vmcb->control.exit_code = SVM_EXIT_INTR;
>  
>  	if (nested_svm_exit_handled(svm)) {
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] Don't #VMEXIT(INTR) if we still have event_inj waiting

[Alexander Graf]

Am 22.09.2009 um 18:39 schrieb Joerg Roedel <joro@8bytes.org>:

> On Fri, Sep 18, 2009 at 03:00:30PM +0200, Alexander Graf wrote:
>> Real hardware would first process the event_inj field and then  
>> notify the
>> host that an interrupt is waiting.
>
> Does it really? I couldn't find this in the SVM spec.

I didn't see it in the spec either, but that's what I saw real  
hardware do which is supported by the exit_info validity check in svm.c

>
>> Let's do the same and just not EXIT_INTR if we have an event  
>> pending for the
>> L2 guest.
>
> Anyway. I think this case is handled good enough with patch 5/5. This
> patch, to be complete must also enable single-steping to exit again
> after the first instruction of the exception handler has ran to inject
> the interrupt. But that would make the whole thing rather compĺicate 
> d.

The NMI injection path has logic for that, but for now it shouldn't  
hurt - we'll get an interrupt sooner or later.

>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/x86/kvm/svm.c |    4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 61efd13..28fcbd0 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1401,6 +1401,10 @@ static inline int nested_svm_intr(struct  
>> vcpu_svm *svm)
>>    if (!(svm->vcpu.arch.hflags & HF_HIF_MASK))
>>        return 0;
>>
>> +    /* We can't EXIT_INTR when we still have an event to inject */
>> +    if (svm->vmcb->control.event_inj)
>> +        return 1;
>> +
>>    svm->vmcb->control.exit_code = SVM_EXIT_INTR;
>>
>>    if (nested_svm_exit_handled(svm)) {
>> -- 
>> 1.6.0.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] Don't call svm_complete_interrupts for nested guests

[Jan Kiszka]

Alexander Graf wrote:
> SVM has some cleanup code, that tries to reinject interrupts and exceptions
> when the guest didn't manage to deal with them yet. It basically transfers
> them to KVM internal state.
> 
> Unfortunately, the internal state is reserved for the L1 guest state, so we
> shouldn't try to go through that logic when running a nested guest.
> 
> When doing something the host KVM can handle, let's just reinject the event
> into the L2 guest, because we didn't touch its state anyways.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f12a669..61efd13 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2349,7 +2349,10 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  	trace_kvm_exit(exit_code, svm->vmcb->save.rip);
>  
>  	if (is_nested(svm)) {
> +		struct vmcb_control_area *control = &svm->vmcb->control;
>  		int vmexit;
> +		int type;
> +		int vec;
>  
>  		nsvm_printk("nested handle_exit: 0x%x | 0x%lx | 0x%lx | 0x%lx\n",
>  			    exit_code, svm->vmcb->control.exit_info_1,
> @@ -2362,9 +2365,18 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  
>  		if (vmexit == NESTED_EXIT_DONE)
>  			return 1;
> -	}
>  
> -	svm_complete_interrupts(svm);
> +		type = control->exit_int_info & SVM_EXITINTINFO_TYPE_MASK;
> +		vec = control->exit_int_info & SVM_EXITINTINFO_VEC_MASK;
> +		if ((type == SVM_EXITINTINFO_TYPE_INTR) ||
> +		    ((type == SVM_EXITINTINFO_TYPE_EXEPT) && !kvm_exception_is_soft(vec))) {
> +			control->event_inj = control->exit_int_info;
> +			control->event_inj_err = control->exit_int_info_err;
> +		}
> +	} else {
> +		/* Don't interpret exit_info for nested guests */

Doesn't this comment belong to the block above?

> +		svm_complete_interrupts(svm);
> +	}
>  
>  	if (npt_enabled) {
>  		int mmu_reload = 0;
> @@ -2602,8 +2614,6 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>  	case SVM_EXITINTINFO_TYPE_EXEPT:
>  		/* In case of software exception do not reinject an exception
>  		   vector, but re-execute and instruction instead */
> -		if (is_nested(svm))
> -			break;
>  		if (kvm_exception_is_soft(vector))
>  			break;
>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 2/5] Don't call svm_complete_interrupts for nested guests

[Joerg Roedel]

On Fri, Sep 18, 2009 at 03:00:29PM +0200, Alexander Graf wrote:
> SVM has some cleanup code, that tries to reinject interrupts and exceptions
> when the guest didn't manage to deal with them yet. It basically transfers
> them to KVM internal state.
> 
> Unfortunately, the internal state is reserved for the L1 guest state, so we
> shouldn't try to go through that logic when running a nested guest.
> 
> When doing something the host KVM can handle, let's just reinject the event
> into the L2 guest, because we didn't touch its state anyways.

I don't really understandt what problem this patch addresses. There are
situations where we have events to reinject into the l2 guest directly.
But the generic reinjection code works fine for it.
The only problematic thing with it is that it implicitly relies on
exit_int_info not to be changed in the exit cycle (which would be worth
a comment).

	Joerg

> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f12a669..61efd13 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2349,7 +2349,10 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  	trace_kvm_exit(exit_code, svm->vmcb->save.rip);
>  
>  	if (is_nested(svm)) {
> +		struct vmcb_control_area *control = &svm->vmcb->control;
>  		int vmexit;
> +		int type;
> +		int vec;
>  
>  		nsvm_printk("nested handle_exit: 0x%x | 0x%lx | 0x%lx | 0x%lx\n",
>  			    exit_code, svm->vmcb->control.exit_info_1,
> @@ -2362,9 +2365,18 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  
>  		if (vmexit == NESTED_EXIT_DONE)
>  			return 1;
> -	}
>  
> -	svm_complete_interrupts(svm);
> +		type = control->exit_int_info & SVM_EXITINTINFO_TYPE_MASK;
> +		vec = control->exit_int_info & SVM_EXITINTINFO_VEC_MASK;
> +		if ((type == SVM_EXITINTINFO_TYPE_INTR) ||
> +		    ((type == SVM_EXITINTINFO_TYPE_EXEPT) && !kvm_exception_is_soft(vec))) {
> +			control->event_inj = control->exit_int_info;
> +			control->event_inj_err = control->exit_int_info_err;
> +		}
> +	} else {
> +		/* Don't interpret exit_info for nested guests */
> +		svm_complete_interrupts(svm);
> +	}
>  
>  	if (npt_enabled) {
>  		int mmu_reload = 0;
> @@ -2602,8 +2614,6 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>  	case SVM_EXITINTINFO_TYPE_EXEPT:
>  		/* In case of software exception do not reinject an exception
>  		   vector, but re-execute and instruction instead */
> -		if (is_nested(svm))
> -			break;
>  		if (kvm_exception_is_soft(vector))
>  			break;
>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] Don't call svm_complete_interrupts for nested guests

[Alexander Graf]

Am 22.09.2009 um 18:26 schrieb Joerg Roedel <joro@8bytes.org>:

> On Fri, Sep 18, 2009 at 03:00:29PM +0200, Alexander Graf wrote:
>> SVM has some cleanup code, that tries to reinject interrupts and  
>> exceptions
>> when the guest didn't manage to deal with them yet. It basically  
>> transfers
>> them to KVM internal state.
>>
>> Unfortunately, the internal state is reserved for the L1 guest  
>> state, so we
>> shouldn't try to go through that logic when running a nested guest.
>>
>> When doing something the host KVM can handle, let's just reinject  
>> the event
>> into the L2 guest, because we didn't touch its state anyways.
>
> I don't really understandt what problem this patch addresses. There  
> are
> situations where we have events to reinject into the l2 guest  
> directly.
> But the generic reinjection code works fine for it.
> The only problematic thing with it is that it implicitly relies on
> exit_int_info not to be changed in the exit cycle (which would be  
> worth
> a comment).

It si

>
>    Joerg
>
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/x86/kvm/svm.c |   18 ++++++++++++++----
>> 1 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index f12a669..61efd13 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2349,7 +2349,10 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>>    trace_kvm_exit(exit_code, svm->vmcb->save.rip);
>>
>>    if (is_nested(svm)) {
>> +        struct vmcb_control_area *control = &svm->vmcb->control;
>>        int vmexit;
>> +        int type;
>> +        int vec;
>>
>>        nsvm_printk("nested handle_exit: 0x%x | 0x%lx | 0x%lx | 0x%lx 
>> \n",
>>                exit_code, svm->vmcb->control.exit_info_1,
>> @@ -2362,9 +2365,18 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>>
>>        if (vmexit == NESTED_EXIT_DONE)
>>            return 1;
>> -    }
>>
>> -    svm_complete_interrupts(svm);
>> +        type = control->exit_int_info & SVM_EXITINTINFO_TYPE_MASK;
>> +        vec = control->exit_int_info & SVM_EXITINTINFO_VEC_MASK;
>> +        if ((type == SVM_EXITINTINFO_TYPE_INTR) ||
>> +            ((type == SVM_EXITINTINFO_TYPE_EXEPT) && ! 
>> kvm_exception_is_soft(vec))) {
>> +            control->event_inj = control->exit_int_info;
>> +            control->event_inj_err = control->exit_int_info_err;
>> +        }
>> +    } else {
>> +        /* Don't interpret exit_info for nested guests */
>> +        svm_complete_interrupts(svm);
>> +    }
>>
>>    if (npt_enabled) {
>>        int mmu_reload = 0;
>> @@ -2602,8 +2614,6 @@ static void svm_complete_interrupts(struct  
>> vcpu_svm *svm)
>>    case SVM_EXITINTINFO_TYPE_EXEPT:
>>        /* In case of software exception do not reinject an exception
>>           vector, but re-execute and instruction instead */
>> -        if (is_nested(svm))
>> -            break;
>>        if (kvm_exception_is_soft(vector))
>>            break;
>>        if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>> -- 
>> 1.6.0.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] Don't call svm_complete_interrupts for nested guests

[Alexander Graf]

Am 22.09.2009 um 18:26 schrieb Joerg Roedel <joro@8bytes.org>:

> On Fri, Sep 18, 2009 at 03:00:29PM +0200, Alexander Graf wrote:
>> SVM has some cleanup code, that tries to reinject interrupts and  
>> exceptions
>> when the guest didn't manage to deal with them yet. It basically  
>> transfers
>> them to KVM internal state.
>>
>> Unfortunately, the internal state is reserved for the L1 guest  
>> state, so we
>> shouldn't try to go through that logic when running a nested guest.
>>
>> When doing something the host KVM can handle, let's just reinject  
>> the event
>> into the L2 guest, because we didn't touch its state anyways.
>
> I don't really understandt what problem this patch addresses. There  
> are
> situations where we have events to reinject into the l2 guest  
> directly.
> But the generic reinjection code works fine for it.
> The only problematic thing with it is that it implicitly relies on
> exit_int_info not to be changed in the exit cycle (which would be  
> worth
> a comment).

It simply tries to be too clever. Reevaluating exceptions won't work  
for example.

Alex


>
>    Joerg
>
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/x86/kvm/svm.c |   18 ++++++++++++++----
>> 1 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index f12a669..61efd13 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2349,7 +2349,10 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>>    trace_kvm_exit(exit_code, svm->vmcb->save.rip);
>>
>>    if (is_nested(svm)) {
>> +        struct vmcb_control_area *control = &svm->vmcb->control;
>>        int vmexit;
>> +        int type;
>> +        int vec;
>>
>>        nsvm_printk("nested handle_exit: 0x%x | 0x%lx | 0x%lx | 0x%lx 
>> \n",
>>                exit_code, svm->vmcb->control.exit_info_1,
>> @@ -2362,9 +2365,18 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>>
>>        if (vmexit == NESTED_EXIT_DONE)
>>            return 1;
>> -    }
>>
>> -    svm_complete_interrupts(svm);
>> +        type = control->exit_int_info & SVM_EXITINTINFO_TYPE_MASK;
>> +        vec = control->exit_int_info & SVM_EXITINTINFO_VEC_MASK;
>> +        if ((type == SVM_EXITINTINFO_TYPE_INTR) ||
>> +            ((type == SVM_EXITINTINFO_TYPE_EXEPT) && ! 
>> kvm_exception_is_soft(vec))) {
>> +            control->event_inj = control->exit_int_info;
>> +            control->event_inj_err = control->exit_int_info_err;
>> +        }
>> +    } else {
>> +        /* Don't interpret exit_info for nested guests */
>> +        svm_complete_interrupts(svm);
>> +    }
>>
>>    if (npt_enabled) {
>>        int mmu_reload = 0;
>> @@ -2602,8 +2614,6 @@ static void svm_complete_interrupts(struct  
>> vcpu_svm *svm)
>>    case SVM_EXITINTINFO_TYPE_EXEPT:
>>        /* In case of software exception do not reinject an exception
>>           vector, but re-execute and instruction instead */
>> -        if (is_nested(svm))
>> -            break;
>>        if (kvm_exception_is_soft(vector))
>>            break;
>>        if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>> -- 
>> 1.6.0.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] Don't call svm_complete_interrupts for nested guests

[Gleb Natapov]

On Wed, Sep 23, 2009 at 01:05:57AM -0700, Alexander Graf wrote:
> 
> Am 22.09.2009 um 18:26 schrieb Joerg Roedel <joro@8bytes.org>:
> 
> >On Fri, Sep 18, 2009 at 03:00:29PM +0200, Alexander Graf wrote:
> >>SVM has some cleanup code, that tries to reinject interrupts and
> >>exceptions
> >>when the guest didn't manage to deal with them yet. It basically
> >>transfers
> >>them to KVM internal state.
> >>
> >>Unfortunately, the internal state is reserved for the L1 guest
> >>state, so we
> >>shouldn't try to go through that logic when running a nested guest.
> >>
> >>When doing something the host KVM can handle, let's just
> >>reinject the event
> >>into the L2 guest, because we didn't touch its state anyways.
> >
> >I don't really understandt what problem this patch addresses.
> >There are
> >situations where we have events to reinject into the l2 guest
> >directly.
> >But the generic reinjection code works fine for it.
> >The only problematic thing with it is that it implicitly relies on
> >exit_int_info not to be changed in the exit cycle (which would be
> >worth
> >a comment).
> 
> It simply tries to be too clever. Reevaluating exceptions won't work
> for example.
> 
Can you elaborate? What do you mean by "too clever" and why reevaluating
exceptions won't work?

--
			Gleb.

Re: [PATCH 1/5] Implement #NMI exiting for nested SVM

[Jan Kiszka]

Alexander Graf wrote:
> When injecting an NMI to the l1 guest while it was running the l2 guest, we
> didn't #VMEXIT but just injected the NMI to the l2 guest.
> 
> Let's be closer to real hardware and #VMEXIT if we're supposed to do so.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   38 ++++++++++++++++++++++++++++++++------
>  1 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 9a4daca..f12a669 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1375,6 +1375,21 @@ static int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
>  	return nested_svm_exit_handled(svm);
>  }
>  
> +static inline int nested_svm_nmi(struct vcpu_svm *svm)
> +{
> +	if (!is_nested(svm))
> +		return 0;
> +
> +	svm->vmcb->control.exit_code = SVM_EXIT_NMI;
> +
> +	if (nested_svm_exit_handled(svm)) {
> +		nsvm_printk("VMexit -> NMI\n");
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline int nested_svm_intr(struct vcpu_svm *svm)
>  {
>  	if (!is_nested(svm))
> @@ -2462,7 +2477,9 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct vmcb *vmcb = svm->vmcb;
>  	return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> -		!(svm->vcpu.arch.hflags & HF_NMI_MASK);
> +		!(svm->vcpu.arch.hflags & HF_NMI_MASK) &&
> +		gif_set(svm) &&
                ^^^^^^^^^^^^
I'm not claiming to be up-to-date with the SVM code around NMI
injection, but this addition irritates me. Can you explain why I don't
have to worry that a cleared IF could now defer NMI injections for L1
guests?

> +		!is_nested(svm);
>  }
>  
>  static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
> @@ -2488,22 +2505,31 @@ static void enable_irq_window(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	nsvm_printk("Trying to open IRQ window\n");
>  
> -	nested_svm_intr(svm);
> +	if (nested_svm_intr(svm))
> +		return;
>  
>  	/* In case GIF=0 we can't rely on the CPU to tell us when
>  	 * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
>  	 * The next time we get that intercept, this function will be
>  	 * called again though and we'll get the vintr intercept. */
> -	if (gif_set(svm)) {
> -		svm_set_vintr(svm);
> -		svm_inject_irq(svm, 0x0);
> -	}
> +	if (!gif_set(svm))
> +		return;
> +
> +	svm_set_vintr(svm);
> +	svm_inject_irq(svm, 0x0);

The last change is pure refactoring that should not belong into this
patch, should it?

>  }
>  
>  static void enable_nmi_window(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> +	if (nested_svm_nmi(svm))
> +		return;
> +
> +	/* NMI is deferred until GIF == 1. Setting GIF will cause a #VMEXIT */
> +	if (!gif_set(svm))
> +		return;
> +

The second half is an unrelated optimization? Then please file a
separate patch.

>  	if ((svm->vcpu.arch.hflags & (HF_NMI_MASK | HF_IRET_MASK))
>  	    == HF_NMI_MASK)
>  		return; /* IRET will cause a vm exit */

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 1/5] Implement #NMI exiting for nested SVM

[Alexander Graf]

Am 18.09.2009 um 15:33 schrieb Jan Kiszka <jan.kiszka@siemens.com>:

> Alexander Graf wrote:
>> When injecting an NMI to the l1 guest while it was running the l2  
>> guest, we
>> didn't #VMEXIT but just injected the NMI to the l2 guest.
>>
>> Let's be closer to real hardware and #VMEXIT if we're supposed to  
>> do so.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/x86/kvm/svm.c |   38 ++++++++++++++++++++++++++++++++------
>> 1 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 9a4daca..f12a669 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1375,6 +1375,21 @@ static int nested_svm_check_exception(struct  
>> vcpu_svm *svm, unsigned nr,
>>    return nested_svm_exit_handled(svm);
>> }
>>
>> +static inline int nested_svm_nmi(struct vcpu_svm *svm)
>> +{
>> +    if (!is_nested(svm))
>> +        return 0;
>> +
>> +    svm->vmcb->control.exit_code = SVM_EXIT_NMI;
>> +
>> +    if (nested_svm_exit_handled(svm)) {
>> +        nsvm_printk("VMexit -> NMI\n");
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> static inline int nested_svm_intr(struct vcpu_svm *svm)
>> {
>>    if (!is_nested(svm))
>> @@ -2462,7 +2477,9 @@ static int svm_nmi_allowed(struct kvm_vcpu  
>> *vcpu)
>>    struct vcpu_svm *svm = to_svm(vcpu);
>>    struct vmcb *vmcb = svm->vmcb;
>>    return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
>> -        !(svm->vcpu.arch.hflags & HF_NMI_MASK);
>> +        !(svm->vcpu.arch.hflags & HF_NMI_MASK) &&
>> +        gif_set(svm) &&
>                ^^^^^^^^^^^^
> I'm not claiming to be up-to-date with the SVM code around NMI
> injection, but this addition irritates me. Can you explain why I don't
> have to worry that a cleared IF could now defer NMI injections for L1
> guests?

It's not about IF, but GIF, which is a special SVM addition that also  
masks NMIs.

>
>> +        !is_nested(svm);
>> }
>>
>> static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
>> @@ -2488,22 +2505,31 @@ static void enable_irq_window(struct  
>> kvm_vcpu *vcpu)
>>    struct vcpu_svm *svm = to_svm(vcpu);
>>    nsvm_printk("Trying to open IRQ window\n");
>>
>> -    nested_svm_intr(svm);
>> +    if (nested_svm_intr(svm))
>> +        return;
>>
>>    /* In case GIF=0 we can't rely on the CPU to tell us when
>>     * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
>>     * The next time we get that intercept, this function will be
>>     * called again though and we'll get the vintr intercept. */
>> -    if (gif_set(svm)) {
>> -        svm_set_vintr(svm);
>> -        svm_inject_irq(svm, 0x0);
>> -    }
>> +    if (!gif_set(svm))
>> +        return;
>> +
>> +    svm_set_vintr(svm);
>> +    svm_inject_irq(svm, 0x0);
>
> The last change is pure refactoring that should not belong into this
> patch, should it?

It went along the same function and makes the code more alike. But if  
you feel like this really should be separate, I can split it.

>
>> }
>>
>> static void enable_nmi_window(struct kvm_vcpu *vcpu)
>> {
>>    struct vcpu_svm *svm = to_svm(vcpu);
>>
>> +    if (nested_svm_nmi(svm))
>> +        return;
>> +
>> +    /* NMI is deferred until GIF == 1. Setting GIF will cause a  
>> #VMEXIT */
>> +    if (!gif_set(svm))
>> +        return;
>> +
>
> The second half is an unrelated optimization? Then please file a
> separate patch.

Nope, it's about not injecting NMIs while GIF is not set again.

Alex

>
>>    if ((svm->vcpu.arch.hflags & (HF_NMI_MASK | HF_IRET_MASK))
>>        == HF_NMI_MASK)
>>        return; /* IRET will cause a vm exit */
>
> Jan
>
> -- 
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux

Re: [PATCH 1/5] Implement #NMI exiting for nested SVM

[Jan Kiszka]

Alexander Graf wrote:
> Am 18.09.2009 um 15:33 schrieb Jan Kiszka <jan.kiszka@siemens.com>:
> 
>> Alexander Graf wrote:
>>> When injecting an NMI to the l1 guest while it was running the l2  
>>> guest, we
>>> didn't #VMEXIT but just injected the NMI to the l2 guest.
>>>
>>> Let's be closer to real hardware and #VMEXIT if we're supposed to  
>>> do so.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> arch/x86/kvm/svm.c |   38 ++++++++++++++++++++++++++++++++------
>>> 1 files changed, 32 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index 9a4daca..f12a669 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -1375,6 +1375,21 @@ static int nested_svm_check_exception(struct  
>>> vcpu_svm *svm, unsigned nr,
>>>    return nested_svm_exit_handled(svm);
>>> }
>>>
>>> +static inline int nested_svm_nmi(struct vcpu_svm *svm)
>>> +{
>>> +    if (!is_nested(svm))
>>> +        return 0;
>>> +
>>> +    svm->vmcb->control.exit_code = SVM_EXIT_NMI;
>>> +
>>> +    if (nested_svm_exit_handled(svm)) {
>>> +        nsvm_printk("VMexit -> NMI\n");
>>> +        return 1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> static inline int nested_svm_intr(struct vcpu_svm *svm)
>>> {
>>>    if (!is_nested(svm))
>>> @@ -2462,7 +2477,9 @@ static int svm_nmi_allowed(struct kvm_vcpu  
>>> *vcpu)
>>>    struct vcpu_svm *svm = to_svm(vcpu);
>>>    struct vmcb *vmcb = svm->vmcb;
>>>    return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
>>> -        !(svm->vcpu.arch.hflags & HF_NMI_MASK);
>>> +        !(svm->vcpu.arch.hflags & HF_NMI_MASK) &&
>>> +        gif_set(svm) &&
>>                ^^^^^^^^^^^^
>> I'm not claiming to be up-to-date with the SVM code around NMI
>> injection, but this addition irritates me. Can you explain why I don't
>> have to worry that a cleared IF could now defer NMI injections for L1
>> guests?
> 
> It's not about IF, but GIF, which is a special SVM addition that also  
> masks NMIs.

Ah, now I got it: That's normally a host thing but, due to nesting, we
need to consider it for L1, too. OK, something learned today. :)

> 
>>> +        !is_nested(svm);
>>> }
>>>
>>> static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
>>> @@ -2488,22 +2505,31 @@ static void enable_irq_window(struct  
>>> kvm_vcpu *vcpu)
>>>    struct vcpu_svm *svm = to_svm(vcpu);
>>>    nsvm_printk("Trying to open IRQ window\n");
>>>
>>> -    nested_svm_intr(svm);
>>> +    if (nested_svm_intr(svm))
>>> +        return;
>>>
>>>    /* In case GIF=0 we can't rely on the CPU to tell us when
>>>     * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
>>>     * The next time we get that intercept, this function will be
>>>     * called again though and we'll get the vintr intercept. */
>>> -    if (gif_set(svm)) {
>>> -        svm_set_vintr(svm);
>>> -        svm_inject_irq(svm, 0x0);
>>> -    }
>>> +    if (!gif_set(svm))
>>> +        return;
>>> +
>>> +    svm_set_vintr(svm);
>>> +    svm_inject_irq(svm, 0x0);
>> The last change is pure refactoring that should not belong into this
>> patch, should it?
> 
> It went along the same function and makes the code more alike. But if  
> you feel like this really should be separate, I can split it.

I've been slapped a few times for mixing both, but I'm not the person
who will merge it.

> 
>>> }
>>>
>>> static void enable_nmi_window(struct kvm_vcpu *vcpu)
>>> {
>>>    struct vcpu_svm *svm = to_svm(vcpu);
>>>
>>> +    if (nested_svm_nmi(svm))
>>> +        return;
>>> +
>>> +    /* NMI is deferred until GIF == 1. Setting GIF will cause a  
>>> #VMEXIT */
>>> +    if (!gif_set(svm))
>>> +        return;
>>> +
>> The second half is an unrelated optimization? Then please file a
>> separate patch.
> 
> Nope, it's about not injecting NMIs while GIF is not set again.

Yes, got it.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 1/5] Implement #NMI exiting for nested SVM

[Joerg Roedel]

On Fri, Sep 18, 2009 at 03:00:28PM +0200, Alexander Graf wrote:
> When injecting an NMI to the l1 guest while it was running the l2 guest, we
> didn't #VMEXIT but just injected the NMI to the l2 guest.
> 
> Let's be closer to real hardware and #VMEXIT if we're supposed to do so.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   38 ++++++++++++++++++++++++++++++++------
>  1 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 9a4daca..f12a669 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1375,6 +1375,21 @@ static int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
>  	return nested_svm_exit_handled(svm);
>  }
>  
> +static inline int nested_svm_nmi(struct vcpu_svm *svm)
> +{
> +	if (!is_nested(svm))
> +		return 0;
> +
> +	svm->vmcb->control.exit_code = SVM_EXIT_NMI;
> +
> +	if (nested_svm_exit_handled(svm)) {
> +		nsvm_printk("VMexit -> NMI\n");
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline int nested_svm_intr(struct vcpu_svm *svm)
>  {
>  	if (!is_nested(svm))
> @@ -2462,7 +2477,9 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct vmcb *vmcb = svm->vmcb;
>  	return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> -		!(svm->vcpu.arch.hflags & HF_NMI_MASK);
> +		!(svm->vcpu.arch.hflags & HF_NMI_MASK) &&
> +		gif_set(svm) &&
> +		!is_nested(svm);

This check is not sufficient. It assumes that the guest intercepts NMIs
for its guests.


> -	if (gif_set(svm)) {
> -		svm_set_vintr(svm);
> -		svm_inject_irq(svm, 0x0);
> -	}
> +	if (!gif_set(svm))
> +		return;
> +
> +	svm_set_vintr(svm);
> +	svm_inject_irq(svm, 0x0);
>  }

As Jan already wrote, please put this in a seperate patch.

Joerg