[PATCH] kvm-userspace: Cleanup user space NMI injection

[PATCH] kvm-userspace: Cleanup user space NMI injection

[Jan Kiszka]

Cleanup redundant check for an open NMI window before injecting. This
will no longer be supported by the kernel, and it was broken by design
anyway.

This change still allows to run the user space against older kernel
modules.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 libkvm/libkvm.c     |   20 +++-----------------
 libkvm/libkvm.h     |   13 +------------
 qemu/qemu-kvm-x86.c |   16 ++++++----------
 qemu/qemu-kvm.c     |    6 +++---
 qemu/qemu-kvm.h     |    2 +-
 user/main.c         |    5 ++---
 6 files changed, 16 insertions(+), 46 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index f6948f5..40c95ce 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -832,9 +832,9 @@ int try_push_interrupts(kvm_context_t kvm)
 	return kvm->callbacks->try_push_interrupts(kvm->opaque);
 }
 
-int try_push_nmi(kvm_context_t kvm)
+void push_nmi(kvm_context_t kvm)
 {
-	return kvm->callbacks->try_push_nmi(kvm->opaque);
+	kvm->callbacks->push_nmi(kvm->opaque);
 }
 
 void post_kvm_run(kvm_context_t kvm, void *env)
@@ -861,17 +861,6 @@ int kvm_is_ready_for_interrupt_injection(kvm_context_t kvm, int vcpu)
 	return run->ready_for_interrupt_injection;
 }
 
-int kvm_is_ready_for_nmi_injection(kvm_context_t kvm, int vcpu)
-{
-#ifdef KVM_CAP_NMI
-	struct kvm_run *run = kvm->run[vcpu];
-
-	return run->ready_for_nmi_injection;
-#else
-	return 0;
-#endif
-}
-
 int kvm_run(kvm_context_t kvm, int vcpu, void *env)
 {
 	int r;
@@ -880,7 +869,7 @@ int kvm_run(kvm_context_t kvm, int vcpu, void *env)
 
 again:
 #ifdef KVM_CAP_NMI
-	run->request_nmi_window = try_push_nmi(kvm);
+	push_nmi(kvm);
 #endif
 #if !defined(__s390__)
 	if (!kvm->irqchip_in_kernel)
@@ -957,9 +946,6 @@ again:
 			r = handle_halt(kvm, vcpu);
 			break;
 		case KVM_EXIT_IRQ_WINDOW_OPEN:
-#ifdef KVM_CAP_NMI
-		case KVM_EXIT_NMI_WINDOW_OPEN:
-#endif
 			break;
 		case KVM_EXIT_SHUTDOWN:
 			r = handle_shutdown(kvm, env);
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index aae9f03..aaad4fb 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -66,7 +66,7 @@ struct kvm_callbacks {
     int (*shutdown)(void *opaque, void *env);
     int (*io_window)(void *opaque);
     int (*try_push_interrupts)(void *opaque);
-    int (*try_push_nmi)(void *opaque);
+    void (*push_nmi)(void *opaque);
     void (*post_kvm_run)(void *opaque, void *env);
     int (*pre_kvm_run)(void *opaque, void *env);
     int (*tpr_access)(void *opaque, int vcpu, uint64_t rip, int is_write);
@@ -217,17 +217,6 @@ uint64_t kvm_get_apic_base(kvm_context_t kvm, int vcpu);
 int kvm_is_ready_for_interrupt_injection(kvm_context_t kvm, int vcpu);
 
 /*!
- * \brief Check if a vcpu is ready for NMI injection
- *
- * This checks if vcpu is not already running in NMI context.
- *
- * \param kvm Pointer to the current kvm_context
- * \param vcpu Which virtual CPU should get dumped
- * \return boolean indicating NMI injection readiness
- */
-int kvm_is_ready_for_nmi_injection(kvm_context_t kvm, int vcpu);
-
-/*!
  * \brief Read VCPU registers
  *
  * This gets the GP registers from the VCPU and outputs them
diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index a4ae7ed..671b5b3 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -667,22 +667,18 @@ int kvm_arch_try_push_interrupts(void *opaque)
     return (env->interrupt_request & CPU_INTERRUPT_HARD) != 0;
 }
 
-int kvm_arch_try_push_nmi(void *opaque)
+void kvm_arch_push_nmi(void *opaque)
 {
     CPUState *env = cpu_single_env;
     int r;
 
     if (likely(!(env->interrupt_request & CPU_INTERRUPT_NMI)))
-        return 0;
-
-    if (kvm_is_ready_for_nmi_injection(kvm_context, env->cpu_index)) {
-        env->interrupt_request &= ~CPU_INTERRUPT_NMI;
-        r = kvm_inject_nmi(kvm_context, env->cpu_index);
-        if (r < 0)
-            printf("cpu %d fail inject NMI\n", env->cpu_index);
-    }
+        return;
 
-    return (env->interrupt_request & CPU_INTERRUPT_NMI) != 0;
+    env->interrupt_request &= ~CPU_INTERRUPT_NMI;
+    r = kvm_inject_nmi(kvm_context, env->cpu_index);
+    if (r < 0)
+        printf("cpu %d fail inject NMI\n", env->cpu_index);
 }
 
 void kvm_arch_update_regs_for_sipi(CPUState *env)
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 8b4cdd6..cf0e85d 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -154,9 +154,9 @@ static int try_push_interrupts(void *opaque)
     return kvm_arch_try_push_interrupts(opaque);
 }
 
-static int try_push_nmi(void *opaque)
+static void push_nmi(void *opaque)
 {
-    return kvm_arch_try_push_nmi(opaque);
+    kvm_arch_push_nmi(opaque);
 }
 
 static void post_kvm_run(void *opaque, void *data)
@@ -742,7 +742,7 @@ static struct kvm_callbacks qemu_kvm_ops = {
     .shutdown = kvm_shutdown,
     .io_window = kvm_io_window,
     .try_push_interrupts = try_push_interrupts,
-    .try_push_nmi = try_push_nmi,
+    .push_nmi = push_nmi,
     .post_kvm_run = post_kvm_run,
     .pre_kvm_run = pre_kvm_run,
 #ifdef TARGET_I386
diff --git a/qemu/qemu-kvm.h b/qemu/qemu-kvm.h
index 6da518a..d05d969 100644
--- a/qemu/qemu-kvm.h
+++ b/qemu/qemu-kvm.h
@@ -66,7 +66,7 @@ void kvm_arch_pre_kvm_run(void *opaque, CPUState *env);
 void kvm_arch_post_kvm_run(void *opaque, CPUState *env);
 int kvm_arch_has_work(CPUState *env);
 int kvm_arch_try_push_interrupts(void *opaque);
-int kvm_arch_try_push_nmi(void *opaque);
+void kvm_arch_push_nmi(void *opaque);
 void kvm_arch_update_regs_for_sipi(CPUState *env);
 void kvm_arch_cpu_reset(CPUState *env);
 
diff --git a/user/main.c b/user/main.c
index dceec99..55639b5 100644
--- a/user/main.c
+++ b/user/main.c
@@ -323,9 +323,8 @@ static int test_try_push_interrupts(void *opaque)
 	return 0;
 }
 
-static int test_try_push_nmi(void *opaque)
+static void test_push_nmi(void *opaque)
 {
-	return 0;
 }
 
 static void test_post_kvm_run(void *opaque, void *vcpu)
@@ -374,7 +373,7 @@ static struct kvm_callbacks test_callbacks = {
 	.halt        = test_halt,
 	.io_window = test_io_window,
 	.try_push_interrupts = test_try_push_interrupts,
-	.try_push_nmi = test_try_push_nmi,
+	.push_nmi = test_push_nmi,
 	.post_kvm_run = test_post_kvm_run,
 	.pre_kvm_run = test_pre_kvm_run,
 	.shutdown = test_shutdown,

Re: [PATCH] kvm-userspace: Cleanup user space NMI injection

[Avi Kivity]

Jan Kiszka wrote:
> Cleanup redundant check for an open NMI window before injecting. This
> will no longer be supported by the kernel, and it was broken by design
> anyway.
>
> This change still allows to run the user space against older kernel
> modules.
>
>   

Hm.  For interrupts we need to check, since the interrupt might be 
deasserted or masked while the window is closed.

Is there no way that this can happen for NMIs?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: Cleanup user space NMI injection

[Jan Kiszka]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Cleanup redundant check for an open NMI window before injecting. This
>> will no longer be supported by the kernel, and it was broken by design
>> anyway.
>>
>> This change still allows to run the user space against older kernel
>> modules.
>>
>>   
> 
> Hm.  For interrupts we need to check, since the interrupt might be
> deasserted or masked while the window is closed.
> 
> Is there no way that this can happen for NMIs?

In our emulation, but I also think in real life, there is no way to
deassert an NMI. Therefore, our user space API just sets the nmi_pending
flag. Masking of NMIs (due to mov ss etc.) is handled by the NMI
injection code, and this has to happen in kernel anyway as the other NMI
sources (APIC and IOAPIC) do not bother about the NMI windows as well.

Jan

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

Re: [PATCH] kvm-userspace: Cleanup user space NMI injection

[Avi Kivity]

Jan Kiszka wrote:
>> Hm.  For interrupts we need to check, since the interrupt might be
>> deasserted or masked while the window is closed.
>>
>> Is there no way that this can happen for NMIs?
>>     
>
> In our emulation, but I also think in real life, there is no way to
> deassert an NMI. Therefore, our user space API just sets the nmi_pending
> flag. Masking of NMIs (due to mov ss etc.) is handled by the NMI
> injection code, and this has to happen in kernel anyway as the other NMI
> sources (APIC and IOAPIC) do not bother about the NMI windows as well.
>   

I think you're right, and applied the patch.  Thanks.

-- 
error compiling committee.c: too many arguments to function