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

[Qemu-devel] [PATCH v4 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:
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           | 16 ++++++++++++++++
 target-ppc/cpu-qom.h        |  1 +
 target-ppc/excp_helper.c    |  8 ++++++++
 target-ppc/translate_init.c | 17 +++++++++++++++++
 target-s390x/cpu.c          |  6 ++++++
 10 files changed, 60 insertions(+), 35 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v4 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.

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:
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            | 11 ++++++++++-
 hmp-commands.hx   |  6 ++----
 include/qom/cpu.h |  1 +
 qapi-schema.json  |  4 +---
 qmp-commands.hx   |  3 +--
 5 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/cpus.c b/cpus.c
index dd7ac13..c0b8918 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1495,6 +1495,15 @@ 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);
+    int ret = -1;
+
+    if (cs && cc->nmi_monitor_handler) {
+        ret = cc->nmi_monitor_handler(cs);
+    }
+    if (ret) {
+        error_set(errp, QERR_UNSUPPORTED);
+    }
 #endif
 }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2e462c0..e97b5ec 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 given guest's 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 given CPU.
 
 ETEXI
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index df977c8..d2547bc 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -108,6 +108,7 @@ typedef struct CPUClass {
     void (*parse_features)(CPUState *cpu, char *str, Error **errp);
 
     void (*reset)(CPUState *cpu);
+    int (*nmi_monitor_handler)(CPUState *cs);
     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 7bc33ea..dcf6642 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1748,13 +1748,11 @@
 ##
 # @inject-nmi:
 #
-# Injects an Non-Maskable Interrupt into all guest's VCPUs.
+# Injects an Non-Maskable Interrupt into the given guest's VCPU.
 #
 # Returns:  If successful, nothing
 #
 # Since:  0.14.0
-#
-# Notes: Only x86 Virtual Machines support this command.
 ##
 { 'command': 'inject-nmi' }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d8aa4ed..553375b 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 given guest's 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 v4 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.

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

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

diff --git a/cpus.c b/cpus.c
index c0b8918..53ae516 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..267cfa4 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -160,6 +160,11 @@ static void s390_cpu_full_reset(CPUState *s)
     tlb_flush(s, 1);
 }
 
+static int s390_cpu_nmi_monitor_handler(CPUState *cs)
+{
+    return s390_cpu_restart(S390_CPU(cs));
+}
+
 #if !defined(CONFIG_USER_ONLY)
 static void s390_cpu_machine_reset_cb(void *opaque)
 {
@@ -245,6 +250,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 v4 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 | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/cpus.c b/cpus.c
index 53ae516..5be64a1 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);
     int ret = -1;
@@ -1491,5 +1478,4 @@ void qmp_inject_nmi(Error **errp)
     if (ret) {
         error_set(errp, QERR_UNSUPPORTED);
     }
-#endif
 }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 042a48d..3d6ec8d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2500,6 +2500,21 @@ static void x86_cpu_reset(CPUState *s)
 #endif
 }
 
+static int x86_cpu_nmi_monitor_handler(CPUState *cs)
+{
+    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
+    }
+
+    return 0;
+}
+
 #ifndef CONFIG_USER_ONLY
 bool cpu_is_bsp(X86CPU *cpu)
 {
@@ -2808,6 +2823,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 v4 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 | 17 +++++++++++++++++
 3 files changed, 26 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 a0c9fdc..11c23e7 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -801,6 +801,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..72bdce3 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8510,6 +8510,22 @@ 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 int ppc_cpu_nmi_monitor_handler(CPUState *cs)
+{
+    async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, cs);
+    return 0;
+}
+#endif
+
 static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
@@ -8536,6 +8552,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 v4 2/4] target-s390x: Migrate to new nmi_monitor_handler() CPU callback

[Cornelia Huck]

On Thu,  5 Jun 2014 00:25:59 +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.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

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

> ---
> Changes:
> 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 :)

Seems to be historical afaics. The not-merged cpu hotplug code mucks
around with it a bit, but it would probably be safe to use cpu_index
instead.

> ---
>  cpus.c             | 14 --------------
>  target-s390x/cpu.c |  6 ++++++
>  2 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index c0b8918..53ae516 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..267cfa4 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -160,6 +160,11 @@ static void s390_cpu_full_reset(CPUState *s)
>      tlb_flush(s, 1);
>  }
> 
> +static int s390_cpu_nmi_monitor_handler(CPUState *cs)
> +{
> +    return s390_cpu_restart(S390_CPU(cs));
> +}
> +
>  #if !defined(CONFIG_USER_ONLY)
>  static void s390_cpu_machine_reset_cb(void *opaque)
>  {
> @@ -245,6 +250,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;

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

[Eric Blake]

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

On 06/04/2014 08:25 AM, Alexey Kardashevskiy 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.
> 
> 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>
> ---

> +    int ret = -1;
> +
> +    if (cs && cc->nmi_monitor_handler) {
> +        ret = cc->nmi_monitor_handler(cs);
> +    }
> +    if (ret) {
> +        error_set(errp, QERR_UNSUPPORTED);

If there is a cc->nmi_monitor_handler installed, is it allowed to return
a value other than -1?  What's more, if the monitor handler fails,
QERR_UNSUPPORTED no longer seems like the best error.  I think that
means that your nmi_monitor_handler() callback needs to take an Error
**errp parameter, and return the proper failure message, rather than
relying on this caller botching it into an unrelated failure message.


> +++ b/qapi-schema.json
> @@ -1748,13 +1748,11 @@
>  ##
>  # @inject-nmi:
>  #
> -# Injects an Non-Maskable Interrupt into all guest's VCPUs.
> +# Injects an Non-Maskable Interrupt into the given guest's VCPU.

Pre-existing, but as long as you are touching this line:

s/an Non/a Non/

"given guest's VCPU" is awkward - since 'inject-nmi' doesn't take any
parameters, how do you control which guest VCPU is given the interrupt?
 Is the interrupt delivered to all VCPUs, or just VCPU 0?  Or does this
mean the "VCPU of the given guest", in which case it is redundant (a
monitor is associated with only one guest, so that guest is always the
"given guest" of any command - a "given guest" only matters if we had an
interface that could control multiple guests at once).

>  #
>  # Returns:  If successful, nothing
>  #
>  # Since:  0.14.0
> -#
> -# Notes: Only x86 Virtual Machines support this command.

Rather than completely deleting this line, it might be worth stating:

Note: prior to 2.1, this command was only supported for x86 VMs

>  
> -Inject an NMI on guest's CPUs.
> +Inject an NMI on the given guest's CPU.

Why the inconsistency between "CPU" vs. "VCPU" in the different doc
locations?  Can we pick one that works for all cases?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Alexey Kardashevskiy]

On 06/05/2014 04:10 AM, Eric Blake wrote:
> On 06/04/2014 08:25 AM, Alexey Kardashevskiy 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.
>>
>> 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>
>> ---
> 
>> +    int ret = -1;
>> +
>> +    if (cs && cc->nmi_monitor_handler) {
>> +        ret = cc->nmi_monitor_handler(cs);
>> +    }
>> +    if (ret) {
>> +        error_set(errp, QERR_UNSUPPORTED);
> 
> If there is a cc->nmi_monitor_handler installed, is it allowed to return
> a value other than -1?  What's more, if the monitor handler fails,
> QERR_UNSUPPORTED no longer seems like the best error.  I think that
> means that your nmi_monitor_handler() callback needs to take an Error
> **errp parameter, and return the proper failure message, rather than
> relying on this caller botching it into an unrelated failure message.


I'd rather remove return value at all, my mistake, I cannot see how NMI
handler can possibly fail - noone can mask it after all :) If callback
exists, I just call it and that's it. If it does not exist, it is
"unsupported" and that's it. Would that be ok?


> 
>> +++ b/qapi-schema.json
>> @@ -1748,13 +1748,11 @@
>>  ##
>>  # @inject-nmi:
>>  #
>> -# Injects an Non-Maskable Interrupt into all guest's VCPUs.
>> +# Injects an Non-Maskable Interrupt into the given guest's VCPU.
> 
> Pre-existing, but as long as you are touching this line:
> 
> s/an Non/a Non/
> 
> "given guest's VCPU" is awkward - since 'inject-nmi' doesn't take any
> parameters, how do you control which guest VCPU is given the interrupt?
>  Is the interrupt delivered to all VCPUs, or just VCPU 0? 

On a CPU selected by the "cpu" monitor command - the user can choose. It
used to be on every CPU for x86 (this is in the commit log).

Ok. If it is not "given" (not the best choice, yes), then what word is it?
"current"? "currently selected"? "the current-in-monitor CPU"?


> Or does this
> mean the "VCPU of the given guest", in which case it is redundant (a
> monitor is associated with only one guest, so that guest is always the
> "given guest" of any command - a "given guest" only matters if we had an
> interface that could control multiple guests at once).




>>  #
>>  # Returns:  If successful, nothing
>>  #
>>  # Since:  0.14.0
>> -#
>> -# Notes: Only x86 Virtual Machines support this command.
> 
> Rather than completely deleting this line, it might be worth stating:
> 
> Note: prior to 2.1, this command was only supported for x86 VMs

More precisely, x86 and s390.

> 
>>  
>> -Inject an NMI on guest's CPUs.
>> +Inject an NMI on the given guest's CPU.
> 
> Why the inconsistency between "CPU" vs. "VCPU" in the different doc
> locations?  Can we pick one that works for all cases?

Ok, I'll make it CPU where I can.


-- 
Alexey

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

[Peter Maydell]

On 5 June 2014 00:36, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> I'd rather remove return value at all, my mistake, I cannot see how NMI
> handler can possibly fail - noone can mask it after all :) If callback
> exists, I just call it and that's it. If it does not exist, it is
> "unsupported" and that's it. Would that be ok?

You're assuming the 'emergency prod the guest' mechanism
in all CPUs will always be a "can't possibly fail' one. You
need to either provide a failure code or have the documentation
specifically say this is a "best-effort" kind of command (the
latter would be fine I expect).

thanks
-- PMM