* [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. @ 2008-11-27 9:36 Zhang, Xiantao 2008-11-27 11:39 ` Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Zhang, Xiantao @ 2008-11-27 9:36 UTC (permalink / raw) To: Avi Kivity, kvm@vger.kernel.org; +Cc: kvm-ia64@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 1180 bytes --] >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 -- 1.6.0 [-- Attachment #2: 0002-KVM-Qemu-push_nmi-should-be-only-used-by-I386-Arch.patch --] [-- Type: application/octet-stream, Size: 1140 bytes --] 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 -- 1.6.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-11-27 9:36 [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch Zhang, Xiantao @ 2008-11-27 11:39 ` Jan Kiszka 2008-11-28 1:47 ` Zhang, Xiantao 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2008-11-27 11:39 UTC (permalink / raw) To: Zhang, Xiantao; +Cc: Avi Kivity, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org 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. 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... Jan -- Siemens AG, Corporate Technology, CT SE 2 ES-OS Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-11-27 11:39 ` Jan Kiszka @ 2008-11-28 1:47 ` Zhang, Xiantao 2008-11-28 9:26 ` Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Zhang, Xiantao @ 2008-11-28 1:47 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-11-28 1:47 ` Zhang, Xiantao @ 2008-11-28 9:26 ` Jan Kiszka 2008-12-01 16:38 ` Hollis Blanchard 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2008-11-28 9:26 UTC (permalink / raw) To: Zhang, Xiantao; +Cc: Avi Kivity, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org Zhang, Xiantao wrote: > 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 This will now break when KVM_CAP_NMI is undefined, ie. when there is no KVM_NMI IOCTL (=> older kvm module sets). Jan -- Siemens AG, Corporate Technology, CT SE 2 ES-OS Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-11-28 9:26 ` Jan Kiszka @ 2008-12-01 16:38 ` Hollis Blanchard 2008-12-01 23:02 ` Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Hollis Blanchard @ 2008-12-01 16:38 UTC (permalink / raw) To: Avi Kivity Cc: Zhang, Xiantao, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org, Jan Kiszka 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 <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 > > 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. Avi, could you revert a8d12f98755be9330fcde055134511f76ecaa538 please? -- Hollis Blanchard IBM Linux Technology Center ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-12-01 16:38 ` Hollis Blanchard @ 2008-12-01 23:02 ` Jan Kiszka 2008-12-01 23:18 ` Hollis Blanchard 2008-12-02 2:01 ` Zhang, Xiantao 0 siblings, 2 replies; 9+ messages in thread From: Jan Kiszka @ 2008-12-01 23:02 UTC (permalink / raw) To: Hollis Blanchard Cc: Avi Kivity, Zhang, Xiantao, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 4917 bytes --] 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 <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 >> 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 <jan.kiszka@siemens.com> --- 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-12-01 23:02 ` Jan Kiszka @ 2008-12-01 23:18 ` Hollis Blanchard 2008-12-02 9:26 ` Avi Kivity 2008-12-02 2:01 ` Zhang, Xiantao 1 sibling, 1 reply; 9+ messages in thread From: Hollis Blanchard @ 2008-12-01 23:18 UTC (permalink / raw) To: Jan Kiszka Cc: Avi Kivity, Zhang, Xiantao, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org On Tue, 2008-12-02 at 00:02 +0100, Jan Kiszka wrote: > > > > 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. Well, it happens, but I do wish that more people would use cscope or even grep to find all users of a symbol. I also wish that Avi would get his PPC box working so he could catch build breaks like these. Cross-compilers would do as well. I would also like a pony. > > Avi, could you revert a8d12f98755be9330fcde055134511f76ecaa538 please? > > > > Here is a patch that reverts change and fixes the root of the issue. Acked-by: Hollis Blanchard <hollisb@us.ibm.com> -- Hollis Blanchard IBM Linux Technology Center ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-12-01 23:18 ` Hollis Blanchard @ 2008-12-02 9:26 ` Avi Kivity 0 siblings, 0 replies; 9+ messages in thread From: Avi Kivity @ 2008-12-02 9:26 UTC (permalink / raw) To: Hollis Blanchard Cc: Jan Kiszka, Zhang, Xiantao, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org Hollis Blanchard wrote: > Well, it happens, but I do wish that more people would use cscope or > even grep to find all users of a symbol. > > That's reasonable. > I also wish that Avi would get his PPC box working so he could catch > build breaks like these. Cross-compilers would do as well. > > I now have a build box somewhere. It's now cloning the source repositories. Once I start rejecting patches as "won't build", I hope people will be more careful. > Acked-by: Hollis Blanchard <hollisb@us.ibm.com> > Applied, thanks Jan. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch. 2008-12-01 23:02 ` Jan Kiszka 2008-12-01 23:18 ` Hollis Blanchard @ 2008-12-02 2:01 ` Zhang, Xiantao 1 sibling, 0 replies; 9+ messages in thread From: Zhang, Xiantao @ 2008-12-02 2:01 UTC (permalink / raw) To: jan.kiszka@web.de, Hollis Blanchard Cc: Avi Kivity, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org Oops, seems we introduced the issue together. Acked-by Xiantao Zhang <xiantao.zhang@intel.com> -----Original Message----- From: jan.kiszka@web.de [mailto:jan.kiszka@web.de] Sent: Tuesday, December 02, 2008 7:03 AM To: Hollis Blanchard Cc: Avi Kivity; Zhang, Xiantao; kvm@vger.kernel.org; kvm-ia64@vger.kernel.org Subject: 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 <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 >> 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 <jan.kiszka@siemens.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-12-02 9:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-11-27 9:36 [PATCH] KVM: Qemu: push_nmi should be only used by I386 Arch Zhang, Xiantao 2008-11-27 11:39 ` Jan Kiszka 2008-11-28 1:47 ` Zhang, Xiantao 2008-11-28 9:26 ` Jan Kiszka 2008-12-01 16:38 ` Hollis Blanchard 2008-12-01 23:02 ` Jan Kiszka 2008-12-01 23:18 ` Hollis Blanchard 2008-12-02 9:26 ` Avi Kivity 2008-12-02 2:01 ` Zhang, Xiantao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox