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

Zhang, Xiantao
2008-11-28T01:47:12Z

[-- Attachment #1: Type: text/plain, Size: 3818 bytes --]

Jan Kiszka wrote:
> Zhang, Xiantao wrote:
>> From 1b89616f99abc8e0983ef58a1f984f31a52fe828 Mon Sep 17 00:00:00
>> 2001 From: Xiantao Zhang <xiantao.zhang@intel.com>
>> Date: Thu, 27 Nov 2008 17:24:51 +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 <xiantao.zhang@intel.com> ---
>> qemu/qemu-kvm.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> 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
>
> Well, doesn't push_nmi() from libkvm.c call into this hook
> unconditionally if KVM_CAP_NMI is set (which is the case for all
> recent kernel headers)? That should cause SEGVs, so you need to patch
> kvm_run() as well.

Since it doesn't generate compiler error, I didn't notice this issue, Thanks!

> Makes me wonder if we shouldn't have better defined KVM_CAP_NMI
> conditionally, only for arch that actually have NMIs (/wrt KVM: only
> x86 ATM). But now it's too late...

But a funny thing is that KVM_CAP_NMI is defined in kernel, but is not used in any code except userspace. We had better use
TARGET_I386 to constrain it for x86 in userspace. Okay ? Attached the patch.

>From c25fa2e4de40e500bd364c3267d5be89a9cfbb4d Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
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 <xiantao.zhang@intel.com>
---
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
--
1.6.0

Xiantao

[-- Attachment #2: 0002-KVM-Qemu-push_nmi-should-be-only-used-by-I386-Arch.patch --]
[-- Type: application/octet-stream, Size: 1727 bytes --]

From c25fa2e4de40e500bd364c3267d5be89a9cfbb4d Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
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 <xiantao.zhang@intel.com>
---
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
--
1.6.0