Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch.

Hollis Blanchard wrote:
> On Fri, 2008-11-28 at 10:26 +0100, Jan Kiszka wrote:
>> Zhang, Xiantao wrote:
>>> >From c25fa2e4de40e500bd364c3267d5be89a9cfbb4d Mon Sep 17 00:00:00 2001
>>> From: Xiantao Zhang <[email protected]>
>>> Date: Fri, 28 Nov 2008 09:38:46 +0800
>>> Subject: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch.
>>>
>>> Use TARGET_I386 to exclude other archs.
>>> Signed-off-by: Xiantao Zhang <[email protected]>
>>> ---
>>>  libkvm/libkvm.c |    4 ++--
>>>  qemu/qemu-kvm.c |    4 ++++
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
>>> index 40c95ce..851a93a 100644
>>> --- a/libkvm/libkvm.c
>>> +++ b/libkvm/libkvm.c
>>> @@ -868,7 +868,7 @@ int kvm_run(kvm_context_t kvm, int vcpu, void *env)
>>>  	struct kvm_run *run = kvm->run[vcpu];
>>>  
>>>  again:
>>> -#ifdef KVM_CAP_NMI
>>> +#ifdef TARGET_I386
>>>  	push_nmi(kvm);
>>>  #endif
>>>  #if !defined(__s390__)
>>> @@ -1032,7 +1032,7 @@ int kvm_has_sync_mmu(kvm_context_t kvm)
>>>  
>>>  int kvm_inject_nmi(kvm_context_t kvm, int vcpu)
>>>  {
>>> -#ifdef KVM_CAP_NMI
>>> +#ifdef TARGET_I386
>>>  	return ioctl(kvm->vcpu_fd[vcpu], KVM_NMI);
>>>  #else
>>>  	return -ENOSYS;
>>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
>>> index cf0e85d..b6c8288 100644
>>> --- a/qemu/qemu-kvm.c
>>> +++ b/qemu/qemu-kvm.c
>>> @@ -154,10 +154,12 @@ static int try_push_interrupts(void *opaque)
>>>      return kvm_arch_try_push_interrupts(opaque);
>>>  }
>>>  
>>> +#ifdef TARGET_I386
>>>  static void push_nmi(void *opaque)
>>>  {
>>>      kvm_arch_push_nmi(opaque);
>>>  }
>>> +#endif
>>>  
>>>  static void post_kvm_run(void *opaque, void *data)
>>>  {
>>> @@ -742,7 +744,9 @@ static struct kvm_callbacks qemu_kvm_ops = {
>>>      .shutdown = kvm_shutdown,
>>>      .io_window = kvm_io_window,
>>>      .try_push_interrupts = try_push_interrupts,
>>> +#ifdef TARGET_I386
>>>      .push_nmi = push_nmi,
>>> +#endif
>>>      .post_kvm_run = post_kvm_run,
>>>      .pre_kvm_run = pre_kvm_run,
>>>  #ifdef TARGET_I386
>> This will now break when KVM_CAP_NMI is undefined, ie. when there is no
>> KVM_NMI IOCTL (=> older kvm module sets).
> 
> Guys, we already have stubs for this (although they've been turned into
> dead code). Jan broke IA64 and PowerPC builds when he renamed
> "kvm_arch_try_push_nmi" to "kvm_arch_push_nmi", and the obvious fix is
> to update the stubs to match. That avoids all these ifdefs and
> associated problems.

Ouch - I'm sorry.

> 
> Avi, could you revert a8d12f98755be9330fcde055134511f76ecaa538 please?
> 

Here is a patch that reverts change and fixes the root of the issue.

-----------

Subject: Fix non-x86 NMI hooks

My previous x86-only change to the NMI push hook broke PPC and IA64.
This is a proper fix plus a cleanup of the #ifdef-based approach to
solve the breakage.

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

 qemu/qemu-kvm-ia64.c    |    3 +--
 qemu/qemu-kvm-powerpc.c |    3 +--
 qemu/qemu-kvm.c         |    4 ----
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/qemu/qemu-kvm-ia64.c b/qemu/qemu-kvm-ia64.c
index 8380f39..a6b17af 100644
--- a/qemu/qemu-kvm-ia64.c
+++ b/qemu/qemu-kvm-ia64.c
@@ -57,9 +57,8 @@ int kvm_arch_try_push_interrupts(void *opaque)
     return 1;
 }
 
-int kvm_arch_try_push_nmi(void *opaque)
+void kvm_arch_push_nmi(void *opaque)
 {
-    return 1;
 }
 
 void kvm_arch_update_regs_for_sipi(CPUState *env)
diff --git a/qemu/qemu-kvm-powerpc.c b/qemu/qemu-kvm-powerpc.c
index 19fde40..fa534ed 100644
--- a/qemu/qemu-kvm-powerpc.c
+++ b/qemu/qemu-kvm-powerpc.c
@@ -188,12 +188,11 @@ int kvm_arch_try_push_interrupts(void *opaque)
     return 0;
 }
 
-int kvm_arch_try_push_nmi(void *opaque)
+void kvm_arch_push_nmi(void *opaque)
 {
 	/* no nmi irq, so discard that call for now and return success.
 	 * This might later get mapped to something on powerpc too if we want
 	 *  to support the nmi monitor command somwhow */
-	return 0;
 }
 
 void kvm_arch_update_regs_for_sipi(CPUState *env)
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index b6c8288..cf0e85d 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -154,12 +154,10 @@ static int try_push_interrupts(void *opaque)
     return kvm_arch_try_push_interrupts(opaque);
 }
 
-#ifdef TARGET_I386
 static void push_nmi(void *opaque)
 {
     kvm_arch_push_nmi(opaque);
 }
-#endif
 
 static void post_kvm_run(void *opaque, void *data)
 {
@@ -744,9 +742,7 @@ static struct kvm_callbacks qemu_kvm_ops = {
     .shutdown = kvm_shutdown,
     .io_window = kvm_io_window,
     .try_push_interrupts = try_push_interrupts,
-#ifdef TARGET_I386
     .push_nmi = push_nmi,
-#endif
     .post_kvm_run = post_kvm_run,
     .pre_kvm_run = pre_kvm_run,
 #ifdef TARGET_I386

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkk0bRMACgkQniDOoMHTA+nfXACeJsRSkiPOqOAnojkyUKLPcNNS
zHkAnAlEsygL240DqhleHGaEpkALBj3+
=fpwk
-----END PGP SIGNATURE-----