[Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Alexey Kardashevskiy]

This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.

The change to existing behaviour is that x86 only delivers NMI to
the current monitored CPU now, not to every CPU.

Please comment. Thanks.


Changes:
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 NMI callback
  target-s390x: Migrate to new nmi() CPU callback
  target-i386: Migrate to new nmi() CPU callback
  target-ppc: Add support for new nmi() 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 v3 1/4] cpus: Define NMI callback

[Alexey Kardashevskiy]

This introduces an NMI (non maskable interrupt) callback per CPU class
which QMP's "nmi" command may use to issue NMI on a CPU.

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:
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..a000bd8 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) {
+        ret = cc->nmi(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..b34f23b 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)(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 v3 2/4] target-s390x: Migrate to new nmi() CPU callback

[Alexey Kardashevskiy]

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

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

Since nmi()-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:
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 a000bd8..83223d3 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..2d50f80 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(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 = s390_cpu_nmi;
     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 v3 3/4] target-i386: Migrate to new nmi() CPU callback

[Alexey Kardashevskiy]

This defines a nmi() 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:
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 83223d3..c0c8ac9 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..af250a4 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(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 = x86_cpu_nmi;
     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 v3 4/4] target-ppc: Add support for new nmi() CPU callback

[Alexey Kardashevskiy]

This defines a nmi() 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.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
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..fd24be5 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(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 = ppc_cpu_nmi;
     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 v3 0/4] cpus: Add generic NMI support

[Paolo Bonzini]

Il 04/06/2014 10:08, Alexey Kardashevskiy ha scritto:
> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>
> The change to existing behaviour is that x86 only delivers NMI to
> the current monitored CPU now, not to every CPU.
>
> Please comment. Thanks.
>
>
> Changes:
> 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 NMI callback
>   target-s390x: Migrate to new nmi() CPU callback
>   target-i386: Migrate to new nmi() CPU callback
>   target-ppc: Add support for new nmi() 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(-)
>

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Peter Maydell]

On 4 June 2014 09:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>
> The change to existing behaviour is that x86 only delivers NMI to
> the current monitored CPU now, not to every CPU.

So this series means that the "nmi" command and handler does
 * NMI on x86
 * reset on PPC
 * restart on S390

That doesn't seem generic at all, and suggests this should
not be a common CPU method/callback.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Alexey Kardashevskiy]

On 06/04/2014 07:16 PM, Peter Maydell wrote:
> On 4 June 2014 09:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>>
>> The change to existing behaviour is that x86 only delivers NMI to
>> the current monitored CPU now, not to every CPU.
> 
> So this series means that the "nmi" command and handler does
>  * NMI on x86
>  * reset on PPC

The vector is called "reset" but it is an interrupt, and I do not see any
way to mask it.

>  * restart on S390

The vector is called "restart" but it is still an interrupt.


> That doesn't seem generic at all, and suggests this should
> not be a common CPU method/callback.

Oh. Ok. Suggestions?


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Peter Maydell]

On 4 June 2014 10:30, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 06/04/2014 07:16 PM, Peter Maydell wrote:
>> On 4 June 2014 09:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>>>
>>> The change to existing behaviour is that x86 only delivers NMI to
>>> the current monitored CPU now, not to every CPU.
>>
>> So this series means that the "nmi" command and handler does
>>  * NMI on x86
>>  * reset on PPC
>
> The vector is called "reset" but it is an interrupt, and I do not see any
> way to mask it.
>
>>  * restart on S390
>
> The vector is called "restart" but it is still an interrupt.

So? ARM has an interrupt called "NMI" but there's zero reason
you'd want to poke it from the monitor, any more than you'd
want to try to hand-send any other kind of interrupt.

>> That doesn't seem generic at all, and suggests this should
>> not be a common CPU method/callback.
>
> Oh. Ok. Suggestions?

I dunno. What are you actually trying to achieve?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Alexander Graf]

On 04.06.14 11:33, Peter Maydell wrote:
> On 4 June 2014 10:30, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> On 06/04/2014 07:16 PM, Peter Maydell wrote:
>>> On 4 June 2014 09:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>>>>
>>>> The change to existing behaviour is that x86 only delivers NMI to
>>>> the current monitored CPU now, not to every CPU.
>>> So this series means that the "nmi" command and handler does
>>>   * NMI on x86
>>>   * reset on PPC
>> The vector is called "reset" but it is an interrupt, and I do not see any
>> way to mask it.
>>
>>>   * restart on S390
>> The vector is called "restart" but it is still an interrupt.
> So? ARM has an interrupt called "NMI" but there's zero reason
> you'd want to poke it from the monitor, any more than you'd
> want to try to hand-send any other kind of interrupt.
>
>>> That doesn't seem generic at all, and suggests this should
>>> not be a common CPU method/callback.
>> Oh. Ok. Suggestions?
> I dunno. What are you actually trying to achieve?

Linux configures certain interrupts to trigger an emergency situation - 
usually to get you into a debugger or to start a crash kexec kernel.

The command is called "nmi" because it originally was used on x86 to do 
this and there the NMI interrupt is the one Linux uses for that purpose. 
In fact, on x86 bringup systems you often have an "NMI" button next to 
the "reset" and "power on" buttons.


Alex

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Alexey Kardashevskiy]

On 06/04/2014 07:33 PM, Peter Maydell wrote:
> On 4 June 2014 10:30, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> On 06/04/2014 07:16 PM, Peter Maydell wrote:
>>> On 4 June 2014 09:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>>>>
>>>> The change to existing behaviour is that x86 only delivers NMI to
>>>> the current monitored CPU now, not to every CPU.
>>>
>>> So this series means that the "nmi" command and handler does
>>>  * NMI on x86
>>>  * reset on PPC
>>
>> The vector is called "reset" but it is an interrupt, and I do not see any
>> way to mask it.
>>
>>>  * restart on S390
>>
>> The vector is called "restart" but it is still an interrupt.
> 
> So? ARM has an interrupt called "NMI" but there's zero reason
> you'd want to poke it from the monitor, any more than you'd
> want to try to hand-send any other kind of interrupt.
>
>>> That doesn't seem generic at all, and suggests this should
>>> not be a common CPU method/callback.
>>
>> Oh. Ok. Suggestions?
> 
> I dunno. What are you actually trying to achieve?


I personally want to get XMON (in-kernel debugger) on PPC.



-- 
Alexey

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Paolo Bonzini]

Il 04/06/2014 11:33, Peter Maydell ha scritto:
> On 4 June 2014 10:30, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> On 06/04/2014 07:16 PM, Peter Maydell wrote:
>>> On 4 June 2014 09:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>> This adds an NMI handler per CPUs. x86, s390 and ppc CPUS are supported.
>>>>
>>>> The change to existing behaviour is that x86 only delivers NMI to
>>>> the current monitored CPU now, not to every CPU.
>>>
>>> So this series means that the "nmi" command and handler does
>>>  * NMI on x86
>>>  * reset on PPC
>>
>> The vector is called "reset" but it is an interrupt, and I do not see any
>> way to mask it.
>>
>>>  * restart on S390
>>
>> The vector is called "restart" but it is still an interrupt.
>
> So? ARM has an interrupt called "NMI" but there's zero reason
> you'd want to poke it from the monitor, any more than you'd
> want to try to hand-send any other kind of interrupt.
>
>>> That doesn't seem generic at all, and suggests this should
>>> not be a common CPU method/callback.
>>
>> Oh. Ok. Suggestions?
>
> I dunno. What are you actually trying to achieve?

It's a kind of "emergency button" on real machines.  On PCs it sends an 
NMI and this results in some kind of crash dump if the OS is configured 
appropriately.  The command may be ill-named for historical reasons, but 
the effect is not x86-specific.

Paolo

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Peter Maydell]

On 4 June 2014 10:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> It's a kind of "emergency button" on real machines.  On PCs it sends an NMI
> and this results in some kind of crash dump if the OS is configured
> appropriately.  The command may be ill-named for historical reasons, but the
> effect is not x86-specific.

OK, so our callback function name should be sensibly named
to match what the effect is supposed to be, and we should have
a sensibly named command, and we should make "nmi" be
a historical-legacy-backwards-compatibility command alias
(possibly only exposed for x86).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Alexander Graf]

On 04.06.14 11:47, Peter Maydell wrote:
> On 4 June 2014 10:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> It's a kind of "emergency button" on real machines.  On PCs it sends an NMI
>> and this results in some kind of crash dump if the OS is configured
>> appropriately.  The command may be ill-named for historical reasons, but the
>> effect is not x86-specific.
> OK, so our callback function name should be sensibly named
> to match what the effect is supposed to be, and we should have
> a sensibly named command, and we should make "nmi" be
> a historical-legacy-backwards-compatibility command alias
> (possibly only exposed for x86).

I honestly don't have a better name :).


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 0/4] cpus: Add generic NMI support

[Greg Kurz]

On Wed, 04 Jun 2014 11:50:53 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 04.06.14 11:47, Peter Maydell wrote:
> > On 4 June 2014 10:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> It's a kind of "emergency button" on real machines.  On PCs it sends an NMI
> >> and this results in some kind of crash dump if the OS is configured
> >> appropriately.  The command may be ill-named for historical reasons, but the
> >> effect is not x86-specific.
> > OK, so our callback function name should be sensibly named
> > to match what the effect is supposed to be, and we should have
> > a sensibly named command, and we should make "nmi" be
> > a historical-legacy-backwards-compatibility command alias
> > (possibly only exposed for x86).
> 
> I honestly don't have a better name :).
> 
> 
> Alex
> 

FWIW "emergency" is used in Paolo's explanation, as well as yours.

Cheers.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 0/4] cpus: Add generic NMI support

[Cornelia Huck]

On Wed, 4 Jun 2014 12:44:28 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Wed, 04 Jun 2014 11:50:53 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
> > 
> > On 04.06.14 11:47, Peter Maydell wrote:
> > > On 4 June 2014 10:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> It's a kind of "emergency button" on real machines.  On PCs it sends an NMI
> > >> and this results in some kind of crash dump if the OS is configured
> > >> appropriately.  The command may be ill-named for historical reasons, but the
> > >> effect is not x86-specific.
> > > OK, so our callback function name should be sensibly named
> > > to match what the effect is supposed to be, and we should have
> > > a sensibly named command, and we should make "nmi" be
> > > a historical-legacy-backwards-compatibility command alias
> > > (possibly only exposed for x86).
> > 
> > I honestly don't have a better name :).
> > 
> > 
> > Alex
> > 
> 
> FWIW "emergency" is used in Paolo's explanation, as well as yours.

I wouldn't be a fan of "emergency" as it might be confused with the
emergency signal on s390.

Re: [Qemu-devel] [PATCH v3 0/4] cpus: Add generic NMI support

[Alexey Kardashevskiy]

On 06/04/2014 07:50 PM, Alexander Graf wrote:
> 
> On 04.06.14 11:47, Peter Maydell wrote:
>> On 4 June 2014 10:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> It's a kind of "emergency button" on real machines.  On PCs it sends an NMI
>>> and this results in some kind of crash dump if the OS is configured
>>> appropriately.  The command may be ill-named for historical reasons, but
>>> the
>>> effect is not x86-specific.
>> OK, so our callback function name should be sensibly named
>> to match what the effect is supposed to be, and we should have
>> a sensibly named command, and we should make "nmi" be
>> a historical-legacy-backwards-compatibility command alias
>> (possibly only exposed for x86).
> 
> I honestly don't have a better name :).

Worse name? Any name? If we come up with some name, "nmi" will still do
"restart" on s390, won't it (backward compatibility blablabla)? So there is
no point adding it for anyone but PPC64 and therefore it can be something
like "machinecheck" and we'll have yet another #ifdef in cpus.c. Which
would be very unusual and very awesome as I am normally not allowed to do
ppc-hacks in generic code :-D



-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Luiz Capitulino]

On Wed,  4 Jun 2014 18:08:47 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This introduces an NMI (non maskable interrupt) callback per CPU class
> which QMP's "nmi" command may use to issue NMI on a CPU.
> 
> 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:
> 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..a000bd8 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) {
> +        ret = cc->nmi(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..b34f23b 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)(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.

QMP doesn't have the concept of "current monitored CPU" you talk in the
intro email. In QMP you have to specify the CPU. You have to choices:

 - Add a new command that takes a CPU parameter (seems the best to me, as
   people were asking for a different command anyways)

 - Add an optional parameter to inject-nmi. When the CPU parameter is
   not specified, the command sends the NMI to all CPUs

Eric, any thoughts?

>  #
>  # 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
>  

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Cornelia Huck]

On Tue, 10 Jun 2014 09:39:51 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Wed,  4 Jun 2014 18:08:47 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > This introduces an NMI (non maskable interrupt) callback per CPU class
> > which QMP's "nmi" command may use to issue NMI on a CPU.
> > 
> > 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:
> > 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..a000bd8 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) {
> > +        ret = cc->nmi(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..b34f23b 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)(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.
> 
> QMP doesn't have the concept of "current monitored CPU" you talk in the
> intro email. In QMP you have to specify the CPU. You have to choices:
> 
>  - Add a new command that takes a CPU parameter (seems the best to me, as
>    people were asking for a different command anyways)
> 
>  - Add an optional parameter to inject-nmi. When the CPU parameter is
>    not specified, the command sends the NMI to all CPUs

The s390 restart interrupt is a per-vcpu interrupt, which we really
don't want to inject on _all_ vcpus. OTOH, we want to inject that
interrupt on any vcpu - we don't care which one it is. So I'd really
like an "inject nmi on default cpu" option.

> 
> Eric, any thoughts?
> 
> >  #
> >  # 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
> >  
> 

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Luiz Capitulino]

On Tue, 10 Jun 2014 16:41:07 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 10 Jun 2014 09:39:51 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > On Wed,  4 Jun 2014 18:08:47 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> > > This introduces an NMI (non maskable interrupt) callback per CPU class
> > > which QMP's "nmi" command may use to issue NMI on a CPU.
> > > 
> > > 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:
> > > 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..a000bd8 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) {
> > > +        ret = cc->nmi(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..b34f23b 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)(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.
> > 
> > QMP doesn't have the concept of "current monitored CPU" you talk in the
> > intro email. In QMP you have to specify the CPU. You have to choices:
> > 
> >  - Add a new command that takes a CPU parameter (seems the best to me, as
> >    people were asking for a different command anyways)
> > 
> >  - Add an optional parameter to inject-nmi. When the CPU parameter is
> >    not specified, the command sends the NMI to all CPUs
> 
> The s390 restart interrupt is a per-vcpu interrupt, which we really
> don't want to inject on _all_ vcpus. OTOH, we want to inject that
> interrupt on any vcpu - we don't care which one it is. So I'd really
> like an "inject nmi on default cpu" option.

We could define a default CPU for the command. What isn't going to work
is to use the human monitor's "current CPU" concept (set with the cpu
command).

> 
> > 
> > Eric, any thoughts?
> > 
> > >  #
> > >  # 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
> > >  
> > 
> 

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Eric Blake]

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

On 06/10/2014 07:39 AM, Luiz Capitulino wrote:
> On Wed,  4 Jun 2014 18:08:47 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> This introduces an NMI (non maskable interrupt) callback per CPU class
>> which QMP's "nmi" command may use to issue NMI on a CPU.
>>
>> 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>

>> +++ 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.
> 
> QMP doesn't have the concept of "current monitored CPU" you talk in the
> intro email. In QMP you have to specify the CPU. You have to choices:
> 
>  - Add a new command that takes a CPU parameter (seems the best to me, as
>    people were asking for a different command anyways)
> 
>  - Add an optional parameter to inject-nmi. When the CPU parameter is
>    not specified, the command sends the NMI to all CPUs
> 
> Eric, any thoughts?

How would libvirt know whether the optional parameter is supported,
short of trying it and getting a failure on older qemu that lacks it?
At this point, since we still don't have global qapi introspection, the
addition of a new command is nicer than the addition of an optional
parameter.

But I definitely agree that we have a discrepancy between HMP being able
to specify a current CPU, vs. QMP being unable to direct which CPU gets
the interrupt.

-- 
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 v3 1/4] cpus: Define NMI callback

[Alexey Kardashevskiy]

On 06/10/2014 11:39 PM, Luiz Capitulino wrote:
> On Wed,  4 Jun 2014 18:08:47 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> This introduces an NMI (non maskable interrupt) callback per CPU class
>> which QMP's "nmi" command may use to issue NMI on a CPU.
>>
>> 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:
>> 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..a000bd8 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) {
>> +        ret = cc->nmi(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..b34f23b 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)(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.
> 
> QMP doesn't have the concept of "current monitored CPU" you talk in the
> intro email. In QMP you have to specify the CPU. You have to choices:


If QMP does not have such a concept, then how does it work now?


> 
>  - Add a new command that takes a CPU parameter (seems the best to me, as
>    people were asking for a different command anyways)
> 
>  - Add an optional parameter to inject-nmi. When the CPU parameter is
>    not specified, the command sends the NMI to all CPUs
> 
> Eric, any thoughts?
> 
>>  #
>>  # 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
>>  
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Paolo Bonzini]

Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
> > The s390 restart interrupt is a per-vcpu interrupt, which we really
> > don't want to inject on _all_ vcpus. OTOH, we want to inject that
> > interrupt on any vcpu - we don't care which one it is. So I'd really
> > like an "inject nmi on default cpu" option.
>
> We could define a default CPU for the command. What isn't going to work
> is to use the human monitor's "current CPU" concept (set with the cpu
> command).

It isn't going to work, but to me it seems like a bug.  Why was the NMI 
command even converted to QAPI if it cannot work?  Let's just use 
monitor_set_cpu from qmp_cpu and call it a day.

The amount of churn that Alexey is going through for this feature is 
unreasonable.

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

On 06/10/2014 06:29 PM, Paolo Bonzini wrote:
> Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
>> > The s390 restart interrupt is a per-vcpu interrupt, which we really
>> > don't want to inject on _all_ vcpus. OTOH, we want to inject that
>> > interrupt on any vcpu - we don't care which one it is. So I'd really
>> > like an "inject nmi on default cpu" option.
>>
>> We could define a default CPU for the command. What isn't going to work
>> is to use the human monitor's "current CPU" concept (set with the cpu
>> command).
>
> It isn't going to work, but to me it seems like a bug.  Why was the 
> NMI command even converted to QAPI if it cannot work?  Let's just use 
> monitor_set_cpu from qmp_cpu and call it a day.
>
> The amount of churn that Alexey is going through for this feature is 
> unreasonable.

I agree. I see two different paths forward:

   1) Use the patches as they are - they seem pretty sound and take the 
existing x86/s390 only feature to spapr
   2) Model an "NMI" button. That button would get instantiated by the 
machine model. That would allow the wiring to be defined by the board. 
Monitor / QMP would only "press" that button (trigger an edge interrupt? 
call a function? something).


I don't mind much either way - option 2 is the architecturally correct 
way of doing this. Option 1 probably won't hurt us either.

Alex

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexey Kardashevskiy]

On 06/11/2014 04:09 AM, Alexander Graf wrote:
> On 06/10/2014 06:29 PM, Paolo Bonzini wrote:
>> Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
>>> > The s390 restart interrupt is a per-vcpu interrupt, which we really
>>> > don't want to inject on _all_ vcpus. OTOH, we want to inject that
>>> > interrupt on any vcpu - we don't care which one it is. So I'd really
>>> > like an "inject nmi on default cpu" option.
>>>
>>> We could define a default CPU for the command. What isn't going to work
>>> is to use the human monitor's "current CPU" concept (set with the cpu
>>> command).
>>
>> It isn't going to work, but to me it seems like a bug.  Why was the NMI
>> command even converted to QAPI if it cannot work?  Let's just use
>> monitor_set_cpu from qmp_cpu and call it a day.
>>
>> The amount of churn that Alexey is going through for this feature is
>> unreasonable.
> 
> I agree. I see two different paths forward:
> 
>   1) Use the patches as they are - they seem pretty sound and take the
> existing x86/s390 only feature to spapr
>   2) Model an "NMI" button. That button would get instantiated by the
> machine model. That would allow the wiring to be defined by the board.
> Monitor / QMP would only "press" that button (trigger an edge interrupt?
> call a function? something).


Ufff... A button? Any good existing example? A device like hw/input/ps2.c?
And HMP's do_mouse_button()? There are queues, I'll need to fight against
them...


> I don't mind much either way - option 2 is the architecturally correct way
> of doing this. Option 1 probably won't hurt us either.



-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

> Am 11.06.2014 um 02:12 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:
> 
>> On 06/11/2014 04:09 AM, Alexander Graf wrote:
>>> On 06/10/2014 06:29 PM, Paolo Bonzini wrote:
>>> Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
>>>>> The s390 restart interrupt is a per-vcpu interrupt, which we really
>>>>> don't want to inject on _all_ vcpus. OTOH, we want to inject that
>>>>> interrupt on any vcpu - we don't care which one it is. So I'd really
>>>>> like an "inject nmi on default cpu" option.
>>>> 
>>>> We could define a default CPU for the command. What isn't going to work
>>>> is to use the human monitor's "current CPU" concept (set with the cpu
>>>> command).
>>> 
>>> It isn't going to work, but to me it seems like a bug.  Why was the NMI
>>> command even converted to QAPI if it cannot work?  Let's just use
>>> monitor_set_cpu from qmp_cpu and call it a day.
>>> 
>>> The amount of churn that Alexey is going through for this feature is
>>> unreasonable.
>> 
>> I agree. I see two different paths forward:
>> 
>>  1) Use the patches as they are - they seem pretty sound and take the
>> existing x86/s390 only feature to spapr
>>  2) Model an "NMI" button. That button would get instantiated by the
>> machine model. That would allow the wiring to be defined by the board.
>> Monitor / QMP would only "press" that button (trigger an edge interrupt?
>> call a function? something).
> 
> 
> Ufff... A button? Any good existing example?

I think the closest thing we have today is the system_reset call ;).

> A device like hw/input/ps2.c?
> And HMP's do_mouse_button()? There are queues, I'll need to fight against
> them...

Yeah, nmi is a debug feature. As I said, I don't care all that much. IMHO we can just declare that the nmi call runs on vcpu0 always and call it a day.

Alex

> 
> 
>> I don't mind much either way - option 2 is the architecturally correct way
>> of doing this. Option 1 probably won't hurt us either.
> 
> 
> 
> -- 
> Alexey

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Peter Maydell]

On 10 June 2014 19:09, Alexander Graf <agraf@suse.de> wrote:
> I agree. I see two different paths forward:
>
>   1) Use the patches as they are - they seem pretty sound and take the
> existing x86/s390 only feature to spapr
>   2) Model an "NMI" button. That button would get instantiated by the
> machine model. That would allow the wiring to be defined by the board.
> Monitor / QMP would only "press" that button (trigger an edge interrupt?
> call a function? something).
>
>
> I don't mind much either way - option 2 is the architecturally correct way
> of doing this. Option 1 probably won't hurt us either.

In an ideal world I'd like (2), ie actually model front panel switches
per machine and with whatever the machine's behaviour actually
is. However pragmatically speaking that's an awful lot of work
(especially since it basically requires adding a lot of U/I which is
always controversial and hard to drive through). I think pragmatism
should probably win here.

-- PMM

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

> Am 11.06.2014 um 02:23 schrieb Peter Maydell <peter.maydell@linaro.org>:
> 
>> On 10 June 2014 19:09, Alexander Graf <agraf@suse.de> wrote:
>> I agree. I see two different paths forward:
>> 
>>  1) Use the patches as they are - they seem pretty sound and take the
>> existing x86/s390 only feature to spapr
>>  2) Model an "NMI" button. That button would get instantiated by the
>> machine model. That would allow the wiring to be defined by the board.
>> Monitor / QMP would only "press" that button (trigger an edge interrupt?
>> call a function? something).
>> 
>> 
>> I don't mind much either way - option 2 is the architecturally correct way
>> of doing this. Option 1 probably won't hurt us either.
> 
> In an ideal world I'd like (2), ie actually model front panel switches
> per machine and with whatever the machine's behaviour actually
> is. However pragmatically speaking that's an awful lot of work
> (especially since it basically requires adding a lot of U/I which is
> always controversial and hard to drive through). I think pragmatism
> should probably win here.

Could we just stick a new nmi function callback into the machine class with the nmi command calling it?

That gets us on the right track to the right direction without putting too much work on Alexey's shoulders. Converting from there to an actual button object should become reasonably straight forward later.


Alex

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Paolo Bonzini]

Il 11/06/2014 02:28, Alexander Graf ha scritto:
>
>
>> Am 11.06.2014 um 02:23 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>
>>> On 10 June 2014 19:09, Alexander Graf <agraf@suse.de> wrote:
>>> I agree. I see two different paths forward:
>>>
>>>  1) Use the patches as they are - they seem pretty sound and take the
>>> existing x86/s390 only feature to spapr
>>>  2) Model an "NMI" button. That button would get instantiated by the
>>> machine model. That would allow the wiring to be defined by the board.
>>> Monitor / QMP would only "press" that button (trigger an edge interrupt?
>>> call a function? something).
>>>
>>>
>>> I don't mind much either way - option 2 is the architecturally correct way
>>> of doing this. Option 1 probably won't hurt us either.
>>
>> In an ideal world I'd like (2), ie actually model front panel switches
>> per machine and with whatever the machine's behaviour actually
>> is. However pragmatically speaking that's an awful lot of work
>> (especially since it basically requires adding a lot of U/I which is
>> always controversial and hard to drive through). I think pragmatism
>> should probably win here.
>
> Could we just stick a new nmi function callback into the machine class with the nmi command calling it?
>
> That gets us on the right track to the right direction without putting too much work on Alexey's shoulders. Converting from there to an actual button object should become reasonably straight forward later.

Personally, I don't see anything wrong in these patches, apart from the 
typo that Cornelia pointed out.

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexey Kardashevskiy]

On 06/11/2014 12:41 AM, Cornelia Huck wrote:
> On Tue, 10 Jun 2014 09:39:51 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Wed,  4 Jun 2014 18:08:47 +1000
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>> This introduces an NMI (non maskable interrupt) callback per CPU class
>>> which QMP's "nmi" command may use to issue NMI on a CPU.
>>>
>>> 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:
>>> 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..a000bd8 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) {
>>> +        ret = cc->nmi(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..b34f23b 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)(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.
>>
>> QMP doesn't have the concept of "current monitored CPU" you talk in the
>> intro email. In QMP you have to specify the CPU. You have to choices:
>>
>>  - Add a new command that takes a CPU parameter (seems the best to me, as
>>    people were asking for a different command anyways)
>>
>>  - Add an optional parameter to inject-nmi. When the CPU parameter is
>>    not specified, the command sends the NMI to all CPUs
> 
> The s390 restart interrupt is a per-vcpu interrupt, which we really
> don't want to inject on _all_ vcpus. OTOH, we want to inject that
> interrupt on any vcpu - we don't care which one it is. So I'd really
> like an "inject nmi on default cpu" option.


Interesting thing. I was pushing "nmi" for ppc which did nmi only on the
current cpu but according to our architect Ben, I should inject NMI on all
CPUs because although XMON (inkernel debugger) tries to stop other CPUs, it
may fail to do that because of some interrupt deadlock. And - this is
important for us - this is what pHyp (native IBM hypervisor) does.

Now I wonder how you are getting away with injecting NMI on one CPU only
and what will happen if we inject NMI on all?

And I changed x86 to inject NMI on the default CPU only (used to be on all
CPUs), and I wonder again if it is actually ok and won't break things.




>> Eric, any thoughts?
>>
>>>  #
>>>  # 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
>>>  
>>
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Paolo Bonzini]

Il 11/06/2014 08:50, Alexey Kardashevskiy ha scritto:
> And I changed x86 to inject NMI on the default CPU only (used to be on all
> CPUs), and I wonder again if it is actually ok and won't break things.
>

For x86 it is better to send it to one CPU only.

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

On 11.06.14 06:59, Paolo Bonzini wrote:
> Il 11/06/2014 02:28, Alexander Graf ha scritto:
>>
>>
>>> Am 11.06.2014 um 02:23 schrieb Peter Maydell 
>>> <peter.maydell@linaro.org>:
>>>
>>>> On 10 June 2014 19:09, Alexander Graf <agraf@suse.de> wrote:
>>>> I agree. I see two different paths forward:
>>>>
>>>>  1) Use the patches as they are - they seem pretty sound and take the
>>>> existing x86/s390 only feature to spapr
>>>>  2) Model an "NMI" button. That button would get instantiated by the
>>>> machine model. That would allow the wiring to be defined by the board.
>>>> Monitor / QMP would only "press" that button (trigger an edge 
>>>> interrupt?
>>>> call a function? something).
>>>>
>>>>
>>>> I don't mind much either way - option 2 is the architecturally 
>>>> correct way
>>>> of doing this. Option 1 probably won't hurt us either.
>>>
>>> In an ideal world I'd like (2), ie actually model front panel switches
>>> per machine and with whatever the machine's behaviour actually
>>> is. However pragmatically speaking that's an awful lot of work
>>> (especially since it basically requires adding a lot of U/I which is
>>> always controversial and hard to drive through). I think pragmatism
>>> should probably win here.
>>
>> Could we just stick a new nmi function callback into the machine 
>> class with the nmi command calling it?
>>
>> That gets us on the right track to the right direction without 
>> putting too much work on Alexey's shoulders. Converting from there to 
>> an actual button object should become reasonably straight forward later.
>
> Personally, I don't see anything wrong in these patches, apart from 
> the typo that Cornelia pointed out.

If you wanted to inject an NMI on non-sPAPR machines, such as -M mac99 
or -M g3beige you would have to trigger an interrupt with the MPIC, not 
the CPU itself.


Alex

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Paolo Bonzini]

Il 11/06/2014 10:01, Alexander Graf ha scritto:
>> Personally, I don't see anything wrong in these patches, apart from
>> the typo that Cornelia pointed out.
>
> If you wanted to inject an NMI on non-sPAPR machines, such as -M mac99
> or -M g3beige you would have to trigger an interrupt with the MPIC, not
> the CPU itself.

But right now inject-nmi was a CPU-specific interface and whoever needs 
something different will have to find a way.

You could argue that Alexey does need something different thanks to his 
need to inject the NMI on all CPUs.

What about an NMIMonitorHandler interface that takes a Monitor*, and 
then you iterate on all of /machine looking for implementors of the 
interface?  Then -M mac99 can add it to the MPIC, -M spapr can just 
ignore the Monitor*, and i386/s390 can look at the current CPU.

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

On 11.06.14 10:27, Paolo Bonzini wrote:
> Il 11/06/2014 10:01, Alexander Graf ha scritto:
>>> Personally, I don't see anything wrong in these patches, apart from
>>> the typo that Cornelia pointed out.
>>
>> If you wanted to inject an NMI on non-sPAPR machines, such as -M mac99
>> or -M g3beige you would have to trigger an interrupt with the MPIC, not
>> the CPU itself.
>
> But right now inject-nmi was a CPU-specific interface and whoever 
> needs something different will have to find a way.
>
> You could argue that Alexey does need something different thanks to 
> his need to inject the NMI on all CPUs.
>
> What about an NMIMonitorHandler interface that takes a Monitor*, and 
> then you iterate on all of /machine looking for implementors of the 
> interface?  Then -M mac99 can add it to the MPIC, -M spapr can just 
> ignore the Monitor*, and i386/s390 can look at the current CPU.

I think modeling a button is easier and closer to what you'd actually 
get on real hardware.


Alex

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Paolo Bonzini]

Il 11/06/2014 10:29, Alexander Graf ha scritto:
>>
>> But right now inject-nmi was a CPU-specific interface and whoever
>> needs something different will have to find a way.
>>
>> You could argue that Alexey does need something different thanks to
>> his need to inject the NMI on all CPUs.
>>
>> What about an NMIMonitorHandler interface that takes a Monitor*, and
>> then you iterate on all of /machine looking for implementors of the
>> interface?  Then -M mac99 can add it to the MPIC, -M spapr can just
>> ignore the Monitor*, and i386/s390 can look at the current CPU.
>
> I think modeling a button is easier and closer to what you'd actually
> get on real hardware.

What is a button?  It certainly doesn't have any evdev support in Linux.

The NMI button is just something that closes a circuit.  Whoever is 
attached to that circuit reacts, in QEMU that's whoever implements an 
NMIMonitorHandler.  The Monitor* is just for backwards HMP compatibility.

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Cornelia Huck]

On Wed, 11 Jun 2014 16:50:35 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 06/11/2014 12:41 AM, Cornelia Huck wrote:
> > On Tue, 10 Jun 2014 09:39:51 -0400
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> >> On Wed,  4 Jun 2014 18:08:47 +1000
> >> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>
> >>> This introduces an NMI (non maskable interrupt) callback per CPU class
> >>> which QMP's "nmi" command may use to issue NMI on a CPU.
> >>>
> >>> 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:
> >>> 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/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.
> >>
> >> QMP doesn't have the concept of "current monitored CPU" you talk in the
> >> intro email. In QMP you have to specify the CPU. You have to choices:
> >>
> >>  - Add a new command that takes a CPU parameter (seems the best to me, as
> >>    people were asking for a different command anyways)
> >>
> >>  - Add an optional parameter to inject-nmi. When the CPU parameter is
> >>    not specified, the command sends the NMI to all CPUs
> > 
> > The s390 restart interrupt is a per-vcpu interrupt, which we really
> > don't want to inject on _all_ vcpus. OTOH, we want to inject that
> > interrupt on any vcpu - we don't care which one it is. So I'd really
> > like an "inject nmi on default cpu" option.
> 
> 
> Interesting thing. I was pushing "nmi" for ppc which did nmi only on the
> current cpu but according to our architect Ben, I should inject NMI on all
> CPUs because although XMON (inkernel debugger) tries to stop other CPUs, it
> may fail to do that because of some interrupt deadlock. And - this is
> important for us - this is what pHyp (native IBM hypervisor) does.
> 
> Now I wonder how you are getting away with injecting NMI on one CPU only
> and what will happen if we inject NMI on all?

The Linux restart handler does stop the other cpus before doing any
other work - see restart_int_handler in arch/s390/kernel/entry64.S and
do_restart in arch/s390/kernel/ipl.c for the gory details ;)

Other operating systems need to have something similar in place, as the
restart interrupt is always directed to a single cpu (for example, z/VM
has SYSTEM RESTART, which is always directed either to the default cpu
or to a specified cpu).

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

On 11.06.14 10:37, Paolo Bonzini wrote:
> Il 11/06/2014 10:29, Alexander Graf ha scritto:
>>>
>>> But right now inject-nmi was a CPU-specific interface and whoever
>>> needs something different will have to find a way.
>>>
>>> You could argue that Alexey does need something different thanks to
>>> his need to inject the NMI on all CPUs.
>>>
>>> What about an NMIMonitorHandler interface that takes a Monitor*, and
>>> then you iterate on all of /machine looking for implementors of the
>>> interface?  Then -M mac99 can add it to the MPIC, -M spapr can just
>>> ignore the Monitor*, and i386/s390 can look at the current CPU.
>>
>> I think modeling a button is easier and closer to what you'd actually
>> get on real hardware.
>
> What is a button?  It certainly doesn't have any evdev support in Linux.

It's a device that links a qemu_irq line and implements an interface to 
call it.

> The NMI button is just something that closes a circuit.  Whoever is 
> attached to that circuit reacts, in QEMU that's whoever implements an 
> NMIMonitorHandler.  The Monitor* is just for backwards HMP compatibility.

I think the only major difference in thinking we have here is that I 
don't think it's necessary to maintain HMP compatibility for this debug 
feature that maybe 2 people in the world ever used ;).


Alex

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexey Kardashevskiy]

On 06/11/2014 10:28 AM, Alexander Graf wrote:
> 
> 
>> Am 11.06.2014 um 02:23 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>
>>> On 10 June 2014 19:09, Alexander Graf <agraf@suse.de> wrote:
>>> I agree. I see two different paths forward:
>>>
>>>  1) Use the patches as they are - they seem pretty sound and take the
>>> existing x86/s390 only feature to spapr
>>>  2) Model an "NMI" button. That button would get instantiated by the
>>> machine model. That would allow the wiring to be defined by the board.
>>> Monitor / QMP would only "press" that button (trigger an edge interrupt?
>>> call a function? something).
>>>
>>>
>>> I don't mind much either way - option 2 is the architecturally correct way
>>> of doing this. Option 1 probably won't hurt us either.
>>
>> In an ideal world I'd like (2), ie actually model front panel switches
>> per machine and with whatever the machine's behaviour actually
>> is. However pragmatically speaking that's an awful lot of work
>> (especially since it basically requires adding a lot of U/I which is
>> always controversial and hard to drive through). I think pragmatism
>> should probably win here.
> 
> Could we just stick a new nmi function callback into the machine class with the nmi command calling it?


MachineClass::nmi_monitor_handler()? Or interface?

What about x86/s390? Leave them alone? Or implement nmi_monitor_handler()
for them too? I did "grep TYPE_MACHINE", looks like I'll have to implement
TYPE_PC_MACHINE and TYPE_S390_MACHINE (or more), is that correct?


v1 of this was implementing an interface for a SPAPR machine (like
fw_path_provider) and it was pointed out that CPUClass is the right place.
Now I am _really_ confused :)


> That gets us on the right track to the right direction without putting
> too much work on Alexey's shoulders. Converting from there to an actual
> button object should become reasonably straight forward later.
> 
> 
> Alex
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Cornelia Huck]

On Wed, 11 Jun 2014 10:38:14 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 11 Jun 2014 16:50:35 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > On 06/11/2014 12:41 AM, Cornelia Huck wrote:
> > > On Tue, 10 Jun 2014 09:39:51 -0400
> > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > 
> > >> On Wed,  4 Jun 2014 18:08:47 +1000
> > >> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >>
> > >>> This introduces an NMI (non maskable interrupt) callback per CPU class
> > >>> which QMP's "nmi" command may use to issue NMI on a CPU.
> > >>>
> > >>> 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:
> > >>> 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/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.
> > >>
> > >> QMP doesn't have the concept of "current monitored CPU" you talk in the
> > >> intro email. In QMP you have to specify the CPU. You have to choices:
> > >>
> > >>  - Add a new command that takes a CPU parameter (seems the best to me, as
> > >>    people were asking for a different command anyways)
> > >>
> > >>  - Add an optional parameter to inject-nmi. When the CPU parameter is
> > >>    not specified, the command sends the NMI to all CPUs
> > > 
> > > The s390 restart interrupt is a per-vcpu interrupt, which we really
> > > don't want to inject on _all_ vcpus. OTOH, we want to inject that
> > > interrupt on any vcpu - we don't care which one it is. So I'd really
> > > like an "inject nmi on default cpu" option.
> > 
> > 
> > Interesting thing. I was pushing "nmi" for ppc which did nmi only on the
> > current cpu but according to our architect Ben, I should inject NMI on all
> > CPUs because although XMON (inkernel debugger) tries to stop other CPUs, it
> > may fail to do that because of some interrupt deadlock. And - this is
> > important for us - this is what pHyp (native IBM hypervisor) does.
> > 
> > Now I wonder how you are getting away with injecting NMI on one CPU only
> > and what will happen if we inject NMI on all?
> 
> The Linux restart handler does stop the other cpus before doing any
> other work - see restart_int_handler in arch/s390/kernel/entry64.S and
> do_restart in arch/s390/kernel/ipl.c for the gory details ;)
> 
> Other operating systems need to have something similar in place, as the
> restart interrupt is always directed to a single cpu (for example, z/VM
> has SYSTEM RESTART, which is always directed either to the default cpu
> or to a specified cpu).

FWIW (just checked):

On the HMC (i.e., when working with LPARs), the restart is always
directed to "the first available cpu", with no way to select a specific
cpu. So I guess a machine callback that just grabs any cpu to inject
the restart interrupt on would be fine for s390.

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

On 11.06.14 11:04, Alexey Kardashevskiy wrote:
> On 06/11/2014 10:28 AM, Alexander Graf wrote:
>>
>>> Am 11.06.2014 um 02:23 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>
>>>> On 10 June 2014 19:09, Alexander Graf <agraf@suse.de> wrote:
>>>> I agree. I see two different paths forward:
>>>>
>>>>   1) Use the patches as they are - they seem pretty sound and take the
>>>> existing x86/s390 only feature to spapr
>>>>   2) Model an "NMI" button. That button would get instantiated by the
>>>> machine model. That would allow the wiring to be defined by the board.
>>>> Monitor / QMP would only "press" that button (trigger an edge interrupt?
>>>> call a function? something).
>>>>
>>>>
>>>> I don't mind much either way - option 2 is the architecturally correct way
>>>> of doing this. Option 1 probably won't hurt us either.
>>> In an ideal world I'd like (2), ie actually model front panel switches
>>> per machine and with whatever the machine's behaviour actually
>>> is. However pragmatically speaking that's an awful lot of work
>>> (especially since it basically requires adding a lot of U/I which is
>>> always controversial and hard to drive through). I think pragmatism
>>> should probably win here.
>> Could we just stick a new nmi function callback into the machine class with the nmi command calling it?
>
> MachineClass::nmi_monitor_handler()? Or interface?

That's an implementation detail to me - I don't care either way :).

> What about x86/s390? Leave them alone? Or implement nmi_monitor_handler()
> for them too? I did "grep TYPE_MACHINE", looks like I'll have to implement
> TYPE_PC_MACHINE and TYPE_S390_MACHINE (or more), is that correct?

or the button device that implements the interface ;).

Really what I'd like to see eventually is that the "nmi" monitor command 
just searches for a device labeled "NMI button". That device implements 
a way (qemu_irq or callback) to trigger the NMI.

In the future when someone who's good at UIs wants to take a stab at it, 
he could then expose that button via the UI as well.

Anything that goes into that direction is a step into the right 
direction IMHO.


Alex

>
>
> v1 of this was implementing an interface for a SPAPR machine (like
> fw_path_provider) and it was pointed out that CPUClass is the right place.
> Now I am _really_ confused :)
>
>
>> That gets us on the right track to the right direction without putting
>> too much work on Alexey's shoulders. Converting from there to an actual
>> button object should become reasonably straight forward later.
>>
>>
>> Alex
>>
>

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Paolo Bonzini]

Il 11/06/2014 10:42, Alexander Graf ha scritto:
>
> On 11.06.14 10:37, Paolo Bonzini wrote:
>> Il 11/06/2014 10:29, Alexander Graf ha scritto:
>>>>
>>>> But right now inject-nmi was a CPU-specific interface and whoever
>>>> needs something different will have to find a way.
>>>>
>>>> You could argue that Alexey does need something different thanks to
>>>> his need to inject the NMI on all CPUs.
>>>>
>>>> What about an NMIMonitorHandler interface that takes a Monitor*, and
>>>> then you iterate on all of /machine looking for implementors of the
>>>> interface?  Then -M mac99 can add it to the MPIC, -M spapr can just
>>>> ignore the Monitor*, and i386/s390 can look at the current CPU.
>>>
>>> I think modeling a button is easier and closer to what you'd actually
>>> get on real hardware.
>>
>> What is a button?  It certainly doesn't have any evdev support in Linux.
>
> It's a device that links a qemu_irq line and implements an interface to
> call it.

That seems awfully overengineered.

>> The NMI button is just something that closes a circuit.  Whoever is
>> attached to that circuit reacts, in QEMU that's whoever implements an
>> NMIMonitorHandler.  The Monitor* is just for backwards HMP compatibility.
>
> I think the only major difference in thinking we have here is that I
> don't think it's necessary to maintain HMP compatibility for this debug
> feature that maybe 2 people in the world ever used ;).

Ok, I can buy removing the support for CPU != 0.  But still, the 
overengineering remains.

If Alexey needs to trigger the NMI on all CPUs, simply moving the new 
method from CPU to Machine, and blindly using first_cpu in s390 and x86 
makes the most sense.

Paolo

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] cpus: Define NMI callback

[Benjamin Herrenschmidt]

On Wed, 2014-06-11 at 12:47 +0200, Paolo Bonzini wrote:
> Ok, I can buy removing the support for CPU != 0.  But still, the 
> overengineering remains.
> 
> If Alexey needs to trigger the NMI on all CPUs, simply moving the new 
> method from CPU to Machine, and blindly using first_cpu in s390 and x86 
> makes the most sense.

I can imagine a future where we would want to programatically trigger the
sreset (the NMI is basically a soft reset for us) on a specific target CPU
so it does make some sense to have a per CPU method and have the monitor
"nmi" command call it on all of them. But it's not a huge deal if you decide
to just make it as machine for now.

This is more than just a "debug" feature however, it is generally used to
trigger kdump on crashed partitions (this feature is called "dump" in pHyp).

Cheers,
Ben.

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Alexander Graf]

On 06/11/2014 12:47 PM, Paolo Bonzini wrote:
> Il 11/06/2014 10:42, Alexander Graf ha scritto:
>>
>> On 11.06.14 10:37, Paolo Bonzini wrote:
>>> Il 11/06/2014 10:29, Alexander Graf ha scritto:
>>>>>
>>>>> But right now inject-nmi was a CPU-specific interface and whoever
>>>>> needs something different will have to find a way.
>>>>>
>>>>> You could argue that Alexey does need something different thanks to
>>>>> his need to inject the NMI on all CPUs.
>>>>>
>>>>> What about an NMIMonitorHandler interface that takes a Monitor*, and
>>>>> then you iterate on all of /machine looking for implementors of the
>>>>> interface?  Then -M mac99 can add it to the MPIC, -M spapr can just
>>>>> ignore the Monitor*, and i386/s390 can look at the current CPU.
>>>>
>>>> I think modeling a button is easier and closer to what you'd actually
>>>> get on real hardware.
>>>
>>> What is a button?  It certainly doesn't have any evdev support in 
>>> Linux.
>>
>> It's a device that links a qemu_irq line and implements an interface to
>> call it.
>
> That seems awfully overengineered.

On PPC every input line to a CPU is a qemu_irq. I don't see how a 
linking piece between this qemu_irq object and a command line interface 
is overengineered. Whether the linking piece is "a device you look for 
and call a function in" or "a machine callback you call" is an 
implementation detail IMHO.

>
>>> The NMI button is just something that closes a circuit.  Whoever is
>>> attached to that circuit reacts, in QEMU that's whoever implements an
>>> NMIMonitorHandler.  The Monitor* is just for backwards HMP 
>>> compatibility.
>>
>> I think the only major difference in thinking we have here is that I
>> don't think it's necessary to maintain HMP compatibility for this debug
>> feature that maybe 2 people in the world ever used ;).
>
> Ok, I can buy removing the support for CPU != 0.  But still, the 
> overengineering remains.
>
> If Alexey needs to trigger the NMI on all CPUs, simply moving the new 
> method from CPU to Machine, and blindly using first_cpu in s390 and 
> x86 makes the most sense.

Yes :).


Alex

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Luiz Capitulino]

On Wed, 11 Jun 2014 01:40:14 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 06/10/2014 11:39 PM, Luiz Capitulino wrote:
> > On Wed,  4 Jun 2014 18:08:47 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> >> This introduces an NMI (non maskable interrupt) callback per CPU class
> >> which QMP's "nmi" command may use to issue NMI on a CPU.
> >>
> >> 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:
> >> 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..a000bd8 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) {
> >> +        ret = cc->nmi(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..b34f23b 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)(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.
> > 
> > QMP doesn't have the concept of "current monitored CPU" you talk in the
> > intro email. In QMP you have to specify the CPU. You have to choices:
> 
> 
> If QMP does not have such a concept, then how does it work now?

It sends the NMI to all CPUs. I hope I'm not miscommunicate my suggestion
here, which is to add a inject-nmi-cpu command that takes a CPU index as
an argument. Then you do exactly what the qmp_inject_nmi() does today, but
you use the CPU index instead of using the HMP stuff.

I don't think this should be more work than series already does, and the
end result would be nicer because it won't depend on HMP magic. If can help
you with the patches if you want.

> 
> 
> > 
> >  - Add a new command that takes a CPU parameter (seems the best to me, as
> >    people were asking for a different command anyways)
> > 
> >  - Add an optional parameter to inject-nmi. When the CPU parameter is
> >    not specified, the command sends the NMI to all CPUs
> > 
> > Eric, any thoughts?
> > 
> >>  #
> >>  # 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
> >>  
> > 
> 
> 

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Luiz Capitulino]

On Tue, 10 Jun 2014 18:29:43 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
> > > The s390 restart interrupt is a per-vcpu interrupt, which we really
> > > don't want to inject on _all_ vcpus. OTOH, we want to inject that
> > > interrupt on any vcpu - we don't care which one it is. So I'd really
> > > like an "inject nmi on default cpu" option.
> >
> > We could define a default CPU for the command. What isn't going to work
> > is to use the human monitor's "current CPU" concept (set with the cpu
> > command).
> 
> It isn't going to work, but to me it seems like a bug.  Why was the NMI 
> command even converted to QAPI if it cannot work?  

It works by sending the NMI to all CPUs. That's the solution we found to
avoid creating a dependency between a QMP command and HMP w/o breaking the
nmi command compatibility. If this is not right, I'd have expected people
to complain as I know this has been used for quite some time to generate
crash dumps.

All I'm suggesting is to add a new QMP command, say inject-nmi-cpu, which
takes a CPU index as an argument. This solves any concern on compatibility
Alex talked about, gives a chance to design the command the way we like and
doesn't create a dependency between a QMP command and HMP (which would be
a design flaw).

The QAPI had two main motivations. Automatically generate open-coded json
present in all QMP commands and decouple QMP from HMP, as the two were
tight coupled in the past. Having the two decoupled is very important so
that QMP doesn't depend on HMP's global garbage (good for concurrency and
for allowing to have a different HMP on top of QMP). Also, we were planning
having a QMP-only socket, session support and other features. Being able to
build QMP w/o building the monitor would be a good milestone.

Making a QMP command depend on HMP is step back for all this planning and
a big de-motivator for all the hard work we put on separating the two.

Again, I wonder if I'm missing something, but I see it as a very simple change
to add a new command. Note that we can do whatever we want in hmp_inject_nmi(),
we can use the new command with whatever semantics we like.

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Cornelia Huck]

On Wed, 11 Jun 2014 09:10:36 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Tue, 10 Jun 2014 18:29:43 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
> > > > The s390 restart interrupt is a per-vcpu interrupt, which we really
> > > > don't want to inject on _all_ vcpus. OTOH, we want to inject that
> > > > interrupt on any vcpu - we don't care which one it is. So I'd really
> > > > like an "inject nmi on default cpu" option.
> > >
> > > We could define a default CPU for the command. What isn't going to work
> > > is to use the human monitor's "current CPU" concept (set with the cpu
> > > command).
> > 
> > It isn't going to work, but to me it seems like a bug.  Why was the NMI 
> > command even converted to QAPI if it cannot work?  
> 
> It works by sending the NMI to all CPUs. 

Not on s390.

> That's the solution we found to
> avoid creating a dependency between a QMP command and HMP w/o breaking the
> nmi command compatibility. 

Well, for s390 the qmp command currently has a dependency on the
monitor-set cpu. But this could be replaced by "issue command on
first cpu; if it fails with anything else than 'not implemented', try
the next one", which would remove any dependencies and still be
backward-compatible. I don't think that on s390 we need to be able to
specify a cpu via a new command; the main reasoning for that would be
that we're currently able to do so under z/VM.

Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Luiz Capitulino]

On Wed, 11 Jun 2014 15:36:57 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 11 Jun 2014 09:10:36 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > On Tue, 10 Jun 2014 18:29:43 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > Il 10/06/2014 16:48, Luiz Capitulino ha scritto:
> > > > > The s390 restart interrupt is a per-vcpu interrupt, which we really
> > > > > don't want to inject on _all_ vcpus. OTOH, we want to inject that
> > > > > interrupt on any vcpu - we don't care which one it is. So I'd really
> > > > > like an "inject nmi on default cpu" option.
> > > >
> > > > We could define a default CPU for the command. What isn't going to work
> > > > is to use the human monitor's "current CPU" concept (set with the cpu
> > > > command).
> > > 
> > > It isn't going to work, but to me it seems like a bug.  Why was the NMI 
> > > command even converted to QAPI if it cannot work?  
> > 
> > It works by sending the NMI to all CPUs. 
> 
> Not on s390.

Yeah, that part of the problem with the current command I got.

> > That's the solution we found to
> > avoid creating a dependency between a QMP command and HMP w/o breaking the
> > nmi command compatibility. 
> 
> Well, for s390 the qmp command currently has a dependency on the
> monitor-set cpu. But this could be replaced by "issue command on
> first cpu; if it fails with anything else than 'not implemented', try
> the next one", which would remove any dependencies and still be
> backward-compatible. I don't think that on s390 we need to be able to
> specify a cpu via a new command; the main reasoning for that would be
> that we're currently able to do so under z/VM.

Your solution looks good to me. It's fine for different archs to have
different semantics. All we have to do is to document it (in qapi-schema.json).