[Qemu-devel] [PATCH v5 0/4] cpus: Add generic "nmi" monitor command support

[Qemu-devel] [PATCH v5 0/4] cpus: Add generic "nmi" monitor command support

[Alexey Kardashevskiy]

This adds an "nmi" monitor command handler per CPUs.
x86, s390 and ppc CPUS are supported.

Please comment. Thanks.

Changes:
v5:
* added Error** to the callback
* fixed some comments

v4:
* now it is not nmi() but nmi_monitor_handler() to avoid confusion

v3:
* patches reorganized
* comments from v2 addressed, more details are in individual commit logs

v2:
* moved from machine interface to CPUClass callback
* s390 and x86 moved to target-s390/target-i386
* x86 handler delivers to the current CPU only now


Alexey Kardashevskiy (4):
  cpus: Define callback for QEMU "nmi" command
  target-s390x: Migrate to new nmi_monitor_handler() CPU callback
  target-i386: Migrate to new nmi_monitor_handler() CPU callback
  target-ppc: Add support for new nmi_monitor_handler() CPU callback

 cpus.c                      | 33 ++++++---------------------------
 hmp-commands.hx             |  6 ++----
 include/qom/cpu.h           |  1 +
 qapi-schema.json            |  4 ++--
 qmp-commands.hx             |  3 +--
 target-i386/cpu.c           | 14 ++++++++++++++
 target-ppc/cpu-qom.h        |  1 +
 target-ppc/excp_helper.c    |  8 ++++++++
 target-ppc/translate_init.c | 16 ++++++++++++++++
 target-s390x/cpu.c          |  9 +++++++++
 10 files changed, 60 insertions(+), 35 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v5 1/4] cpus: Define callback for QEMU "nmi" command

[Alexey Kardashevskiy]

This introduces an NMI (Non Maskable Interrupt) nmi_monitor_handler()
callback to the CPU class. It is called from QMP's "nmi" command and
performs an action required to cause debug crash dump on in-kernel
debugger invocation. The callback returns Error**.

This adds support for it in qmp_inject_nmi(). Since no architecture
supports it at the moment, there is no change in behaviour.

This changes inject-nmi command description for HMP and QMP.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v5:
* s/given guest's (CPU|VCPU)/default CPU/
* nmi_monitor_handler() now returns Error**


v4:
* s/\<nmi\>/nmi_monitor_handler/

v3:
* actual nmi() enablement moved from last patch to first patch
* changed description for QMP command too
---
 cpus.c            | 9 ++++++++-
 hmp-commands.hx   | 6 ++----
 include/qom/cpu.h | 1 +
 qapi-schema.json  | 4 ++--
 qmp-commands.hx   | 3 +--
 5 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/cpus.c b/cpus.c
index dd7ac13..b9d6602 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1495,6 +1495,13 @@ void qmp_inject_nmi(Error **errp)
         }
     }
 #else
-    error_set(errp, QERR_UNSUPPORTED);
+    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
+    CPUClass *cc = CPU_GET_CLASS(cs);
+
+    if (cs && cc->nmi_monitor_handler) {
+        cc->nmi_monitor_handler(cs, errp);
+    } else {
+        error_set(errp, QERR_UNSUPPORTED);
+    }
 #endif
 }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2e462c0..0f26c20 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -830,19 +830,17 @@ The values that can be specified here depend on the machine type, but are
 the same that can be specified in the @code{-boot} command line option.
 ETEXI
 
-#if defined(TARGET_I386) || defined(TARGET_S390X)
     {
         .name       = "nmi",
         .args_type  = "",
         .params     = "",
-        .help       = "inject an NMI on all guest's CPUs",
+        .help       = "inject an NMI on the default CPU",
         .mhandler.cmd = hmp_inject_nmi,
     },
-#endif
 STEXI
 @item nmi @var{cpu}
 @findex nmi
-Inject an NMI (x86) or RESTART (s390x) on the given CPU.
+Inject an NMI on the default CPU.
 
 ETEXI
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 4b352a2..f8bd1d5 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -110,6 +110,7 @@ typedef struct CPUClass {
     void (*parse_features)(CPUState *cpu, char *str, Error **errp);
 
     void (*reset)(CPUState *cpu);
+    void (*nmi_monitor_handler)(CPUState *cs, Error **errp);
     int reset_dump_flags;
     bool (*has_work)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
diff --git a/qapi-schema.json b/qapi-schema.json
index 14b498b..5c03d87 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1116,13 +1116,13 @@
 ##
 # @inject-nmi:
 #
-# Injects an Non-Maskable Interrupt into all guest's VCPUs.
+# Injects an Non-Maskable Interrupt into the default CPU.
 #
 # Returns:  If successful, nothing
 #
 # Since:  0.14.0
 #
-# Notes: Only x86 Virtual Machines support this command.
+# Note: prior to 2.1, this command was only supported for x86 and s390 VMs
 ##
 { 'command': 'inject-nmi' }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d8aa4ed..b16a0c9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -477,7 +477,7 @@ SQMP
 inject-nmi
 ----------
 
-Inject an NMI on guest's CPUs.
+Inject an NMI on the default CPU.
 
 Arguments: None.
 
@@ -487,7 +487,6 @@ Example:
 <- { "return": {} }
 
 Note: inject-nmi fails when the guest doesn't support injecting.
-      Currently, only x86 (NMI) and s390x (RESTART) guests do.
 
 EQMP
 
-- 
2.0.0

[Qemu-devel] [PATCH v5 2/4] target-s390x: Migrate to new nmi_monitor_handler() CPU callback

[Alexey Kardashevskiy]

This defines a nmi_monitor_handler() callback for s390 CPU class.

This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
nmi_monitor_handler() callback is going to be used for NMI.

Since nmi_monitor_handler()-calling code is platform independent,
CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
There should not be any change in behaviour as both @cpu_index and
@cpu_num are global CPU numbers.

Also, s390_cpu_restart() takes care of preforming operations in
the specific CPU thread so no extra measure is required here either.

Since the only error s390_cpu_restart() can return is ENOSYS, convert
it to QERR_UNSUPPORTED.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v5:
* added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this

v4:
* s/\<nmi\>/nmi_monitor_handler/

v3:
* now contains both old code removal and new code insertion, easier to
track changes

---
Is there any good reason to have @cpu_num in addition to @cpu_index?
Just asking :)
---
 cpus.c             | 14 --------------
 target-s390x/cpu.c |  9 +++++++++
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/cpus.c b/cpus.c
index b9d6602..87188ce 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1480,20 +1480,6 @@ void qmp_inject_nmi(Error **errp)
             apic_deliver_nmi(cpu->apic_state);
         }
     }
-#elif defined(TARGET_S390X)
-    CPUState *cs;
-    S390CPU *cpu;
-
-    CPU_FOREACH(cs) {
-        cpu = S390_CPU(cs);
-        if (cpu->env.cpu_num == monitor_get_cpu_index()) {
-            if (s390_cpu_restart(S390_CPU(cs)) == -1) {
-                error_set(errp, QERR_UNSUPPORTED);
-                return;
-            }
-            break;
-        }
-    }
 #else
     CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
     CPUClass *cc = CPU_GET_CLASS(cs);
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c3082b7..8f40bcc 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -27,6 +27,7 @@
 #include "qemu-common.h"
 #include "qemu/timer.h"
 #include "hw/hw.h"
+#include "qapi/qmp/qerror.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -160,6 +161,13 @@ static void s390_cpu_full_reset(CPUState *s)
     tlb_flush(s, 1);
 }
 
+static void s390_cpu_nmi_monitor_handler(CPUState *cs, Error **errp)
+{
+    if (s390_cpu_restart(S390_CPU(cs))) {
+        error_set(errp, QERR_UNSUPPORTED);
+    }
+}
+
 #if !defined(CONFIG_USER_ONLY)
 static void s390_cpu_machine_reset_cb(void *opaque)
 {
@@ -245,6 +253,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->cpu_reset = s390_cpu_reset;
     scc->initial_cpu_reset = s390_cpu_initial_reset;
     cc->reset = s390_cpu_full_reset;
+    cc->nmi_monitor_handler = s390_cpu_nmi_monitor_handler;
     cc->has_work = s390_cpu_has_work;
     cc->do_interrupt = s390_cpu_do_interrupt;
     cc->dump_state = s390_cpu_dump_state;
-- 
2.0.0

[Qemu-devel] [PATCH v5 3/4] target-i386: Migrate to new nmi_monitor_handler() CPU callback

[Alexey Kardashevskiy]

This defines a nmi_monitor_handler() callback for i386 CPU class.

This removes #ifdef I386 branch in qmp_inject_nmi so new i386's nmi()
callback is going to be used for NMI.

This changes code to inject NMI on the current CPU instead of injecting
it on every CPU. However that does not seem to be an issue.

Since kvm_apic_external_nmi() takes care of preforming operations in
the specific CPU thread so no extra measure is required here.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* s/\<nmi\>/nmi_monitor_handler/

v3:
* now contains both old code removal and new code insertion, easier to
track changes
* fixed compile for linux-user
---
 cpus.c            | 14 --------------
 target-i386/cpu.c | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/cpus.c b/cpus.c
index 87188ce..463f8d0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1468,19 +1468,6 @@ exit:
 
 void qmp_inject_nmi(Error **errp)
 {
-#if defined(TARGET_I386)
-    CPUState *cs;
-
-    CPU_FOREACH(cs) {
-        X86CPU *cpu = X86_CPU(cs);
-
-        if (!cpu->apic_state) {
-            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
-        } else {
-            apic_deliver_nmi(cpu->apic_state);
-        }
-    }
-#else
     CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
     CPUClass *cc = CPU_GET_CLASS(cs);
 
@@ -1489,5 +1476,4 @@ void qmp_inject_nmi(Error **errp)
     } else {
         error_set(errp, QERR_UNSUPPORTED);
     }
-#endif
 }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index dde052c..96e268e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2497,6 +2497,19 @@ static void x86_cpu_reset(CPUState *s)
 #endif
 }
 
+static void x86_cpu_nmi_monitor_handler(CPUState *cs, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    if (!cpu->apic_state) {
+        cpu_interrupt(cs, CPU_INTERRUPT_NMI);
+#ifndef CONFIG_USER_ONLY
+    } else {
+        apic_deliver_nmi(cpu->apic_state);
+#endif
+    }
+}
+
 #ifndef CONFIG_USER_ONLY
 bool cpu_is_bsp(X86CPU *cpu)
 {
@@ -2806,6 +2819,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
+    cc->nmi_monitor_handler = x86_cpu_nmi_monitor_handler;
     cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
 
     cc->class_by_name = x86_cpu_class_by_name;
-- 
2.0.0

[Qemu-devel] [PATCH v5 4/4] target-ppc: Add support for new nmi_monitor_handler() CPU callback

[Alexey Kardashevskiy]

This defines a nmi_monitor_handler() callback for POWERPC CPU class so
the "nmi" HMP/QMP command gets supported for POWERPC machines.

This calls POWERPC_EXCP_RESET (vector 0x100) in the guest to deliver NMI.
The expected result is XMON (in-kernel debugger) invocation.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* s/\<nmi\>/nmi_monitor_handler/
* added note about XMON into commit log

v3:
* ppc_cpu_do_nmi() is exported from excp_helper.c instead of powerpc_excp()
---
 target-ppc/cpu-qom.h        |  1 +
 target-ppc/excp_helper.c    |  8 ++++++++
 target-ppc/translate_init.c | 16 ++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 47dc8e6..fe7d602 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -119,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
 int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                                int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
+void ppc_cpu_do_nmi(CPUState *cs);
 extern const struct VMStateDescription vmstate_ppc_cpu;
 #endif
 
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 7dfc52d..a574ba3 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -802,6 +802,14 @@ void ppc_hw_interrupt(CPUPPCState *env)
         }
     }
 }
+
+void ppc_cpu_do_nmi(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
+}
 #endif /* !CONFIG_USER_ONLY */
 
 #if defined(DEBUG_OP)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 4d94015..ee28c27 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8510,6 +8510,21 @@ static void ppc_cpu_initfn(Object *obj)
     }
 }
 
+#ifndef CONFIG_USER_ONLY
+static void ppc_cpu_do_nmi_on_cpu(void *arg)
+{
+    CPUState *cs = arg;
+
+    cpu_synchronize_state(cs);
+    ppc_cpu_do_nmi(cs);
+}
+
+static void ppc_cpu_nmi_monitor_handler(CPUState *cs, Error **errp)
+{
+    async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, cs);
+}
+#endif
+
 static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
@@ -8536,6 +8551,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #ifdef CONFIG_USER_ONLY
     cc->handle_mmu_fault = ppc_cpu_handle_mmu_fault;
 #else
+    cc->nmi_monitor_handler = ppc_cpu_nmi_monitor_handler;
     cc->get_phys_page_debug = ppc_cpu_get_phys_page_debug;
     cc->vmsd = &vmstate_ppc_cpu;
 #if defined(TARGET_PPC64)
-- 
2.0.0

Re: [Qemu-devel] [PATCH v5 1/4] cpus: Define callback for QEMU "nmi" command

[Cornelia Huck]

On Tue, 10 Jun 2014 16:18:00 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This introduces an NMI (Non Maskable Interrupt) nmi_monitor_handler()
> callback to the CPU class. It is called from QMP's "nmi" command and
> performs an action required to cause debug crash dump on in-kernel
> debugger invocation. The callback returns Error**.
> 
> This adds support for it in qmp_inject_nmi(). Since no architecture
> supports it at the moment, there is no change in behaviour.
> 
> This changes inject-nmi command description for HMP and QMP.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v5:
> * s/given guest's (CPU|VCPU)/default CPU/
> * nmi_monitor_handler() now returns Error**
> 
> 
> v4:
> * s/\<nmi\>/nmi_monitor_handler/
> 
> v3:
> * actual nmi() enablement moved from last patch to first patch
> * changed description for QMP command too
> ---
>  cpus.c            | 9 ++++++++-
>  hmp-commands.hx   | 6 ++----
>  include/qom/cpu.h | 1 +
>  qapi-schema.json  | 4 ++--
>  qmp-commands.hx   | 3 +--
>  5 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index dd7ac13..b9d6602 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1495,6 +1495,13 @@ void qmp_inject_nmi(Error **errp)
>          }
>      }
>  #else
> -    error_set(errp, QERR_UNSUPPORTED);
> +    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
> +    CPUClass *cc = CPU_GET_CLASS(cs);

Just wondering: Is CPU_GET_CLASS(NULL) really safe?

> +
> +    if (cs && cc->nmi_monitor_handler) {

Or is cs == NULL simply not possible, and the code should check for
cc != NULL here instead?

> +        cc->nmi_monitor_handler(cs, errp);
> +    } else {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
>  #endif
>  }

Re: [Qemu-devel] [PATCH v5 1/4] cpus: Define callback for QEMU "nmi" command

[Paolo Bonzini]

Il 10/06/2014 13:43, Cornelia Huck ha scritto:
>> > +    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
>> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> Just wondering: Is CPU_GET_CLASS(NULL) really safe?

No, it's not...

>> > +
>> > +    if (cs && cc->nmi_monitor_handler) {
> Or is cs == NULL simply not possible, and the code should check for
> cc != NULL here instead?

... and cc cannot be NULL either here.

Paolo

>> > +        cc->nmi_monitor_handler(cs, errp);

Re: [Qemu-devel] [PATCH v5 2/4] target-s390x: Migrate to new nmi_monitor_handler() CPU callback

[Cornelia Huck]

On Tue, 10 Jun 2014 16:18:01 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This defines a nmi_monitor_handler() callback for s390 CPU class.
> 
> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
> nmi_monitor_handler() callback is going to be used for NMI.
> 
> Since nmi_monitor_handler()-calling code is platform independent,
> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
> There should not be any change in behaviour as both @cpu_index and
> @cpu_num are global CPU numbers.
> 
> Also, s390_cpu_restart() takes care of preforming operations in
> the specific CPU thread so no extra measure is required here either.
> 
> Since the only error s390_cpu_restart() can return is ENOSYS, convert
> it to QERR_UNSUPPORTED.

That's an improvement over the current check for == -1.

> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v5:
> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
> 
> v4:
> * s/\<nmi\>/nmi_monitor_handler/
> 
> v3:
> * now contains both old code removal and new code insertion, easier to
> track changes
> 
> ---
> Is there any good reason to have @cpu_num in addition to @cpu_index?
> Just asking :)
> ---
>  cpus.c             | 14 --------------
>  target-s390x/cpu.c |  9 +++++++++
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v5 1/4] cpus: Define callback for QEMU "nmi" command

[Alexey Kardashevskiy]

On 06/10/2014 09:43 PM, Cornelia Huck wrote:
> On Tue, 10 Jun 2014 16:18:00 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> This introduces an NMI (Non Maskable Interrupt) nmi_monitor_handler()
>> callback to the CPU class. It is called from QMP's "nmi" command and
>> performs an action required to cause debug crash dump on in-kernel
>> debugger invocation. The callback returns Error**.
>>
>> This adds support for it in qmp_inject_nmi(). Since no architecture
>> supports it at the moment, there is no change in behaviour.
>>
>> This changes inject-nmi command description for HMP and QMP.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v5:
>> * s/given guest's (CPU|VCPU)/default CPU/
>> * nmi_monitor_handler() now returns Error**
>>
>>
>> v4:
>> * s/\<nmi\>/nmi_monitor_handler/
>>
>> v3:
>> * actual nmi() enablement moved from last patch to first patch
>> * changed description for QMP command too
>> ---
>>  cpus.c            | 9 ++++++++-
>>  hmp-commands.hx   | 6 ++----
>>  include/qom/cpu.h | 1 +
>>  qapi-schema.json  | 4 ++--
>>  qmp-commands.hx   | 3 +--
>>  5 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index dd7ac13..b9d6602 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1495,6 +1495,13 @@ void qmp_inject_nmi(Error **errp)
>>          }
>>      }
>>  #else
>> -    error_set(errp, QERR_UNSUPPORTED);
>> +    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
>> +    CPUClass *cc = CPU_GET_CLASS(cs);
> 
> Just wondering: Is CPU_GET_CLASS(NULL) really safe?


I expect it to assert so it is kind of safe.


> 
>> +
>> +    if (cs && cc->nmi_monitor_handler) {
> 
> Or is cs == NULL simply not possible, and the code should check for
> cc != NULL here instead?

This is definitely a mistype, I meant "if (cc &&...)"



>> +        cc->nmi_monitor_handler(cs, errp);
>> +    } else {
>> +        error_set(errp, QERR_UNSUPPORTED);
>> +    }
>>  #endif
>>  }
> 


-- 
Alexey