[Qemu-devel] [PATCH v2 0/5] nmi: add interface

[Qemu-devel] [PATCH v2 0/5] nmi: add interface

[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:
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 (5):
  cpu: Add NMI callback
  target-i386: Implement nmi() callback
  target-s390: Implement nmi() callback
  target-ppc: Implement nmi() callback
  cpus: Enable nmi() callback use

 cpus.c                      | 33 +++++++--------------------------
 hmp-commands.hx             |  4 +---
 include/qom/cpu.h           |  1 +
 target-i386/cpu.c           | 14 ++++++++++++++
 target-ppc/cpu-qom.h        |  1 +
 target-ppc/excp_helper.c    |  2 +-
 target-ppc/translate_init.c | 18 ++++++++++++++++++
 target-s390x/cpu.c          |  6 ++++++
 8 files changed, 49 insertions(+), 30 deletions(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH v2 1/5] cpu: Add 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.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hmp-commands.hx   | 4 +---
 include/qom/cpu.h | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index f3fc514..9633260 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -827,7 +827,6 @@ 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  = "",
@@ -835,11 +834,10 @@ ETEXI
         .help       = "inject an NMI on all guest's CPUs",
         .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 f99885a..8bb7018 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -103,6 +103,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);
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v2 1/5] cpu: Add NMI callback

[Alexander Graf]

On 03/28/2014 01:51 PM, Alexey Kardashevskiy 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.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hmp-commands.hx   | 4 +---
>   include/qom/cpu.h | 1 +
>   2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f3fc514..9633260 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -827,7 +827,6 @@ 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  = "",
> @@ -835,11 +834,10 @@ ETEXI
>           .help       = "inject an NMI on all guest's CPUs",

This is not true anymore with your patch.

Also, does NMI injection only get exposed through HMP, not a specific 
QMP path?


Alex

>           .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 f99885a..8bb7018 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -103,6 +103,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);

[Qemu-devel] [PATCH v2 2/5] target-i386: Implement nmi() callback

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-i386/cpu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8fd1497..35f20e0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2489,6 +2489,19 @@ 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);
+    } else {
+        apic_deliver_nmi(cpu->apic_state);
+    }
+
+    return 0;
+}
+
 #ifndef CONFIG_USER_ONLY
 bool cpu_is_bsp(X86CPU *cpu)
 {
@@ -2797,6 +2810,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;
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v2 2/5] target-i386: Implement nmi() callback

[Alexey Kardashevskiy]

On 03/28/2014 11:51 PM, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  target-i386/cpu.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8fd1497..35f20e0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2489,6 +2489,19 @@ 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);
> +    } else {
> +        apic_deliver_nmi(cpu->apic_state);

This symbols is undefined for (i386|x86_64)-linux-user.

What would be the right fix here? #ifndef CONFIG_USER_ONLY here or empty
stub for apic_deliver_nmi() in include/hw/i386/apic.h? Thanks.



> +    }
> +
> +    return 0;
> +}
> +
>  #ifndef CONFIG_USER_ONLY
>  bool cpu_is_bsp(X86CPU *cpu)
>  {
> @@ -2797,6 +2810,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;
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 2/5] target-i386: Implement nmi() callback

[Richard Henderson]

On 03/30/2014 07:55 PM, Alexey Kardashevskiy wrote:
> On 03/28/2014 11:51 PM, Alexey Kardashevskiy wrote:
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  target-i386/cpu.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 8fd1497..35f20e0 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2489,6 +2489,19 @@ 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);
>> +    } else {
>> +        apic_deliver_nmi(cpu->apic_state);
> 
> This symbols is undefined for (i386|x86_64)-linux-user.
> 
> What would be the right fix here? #ifndef CONFIG_USER_ONLY here or empty
> stub for apic_deliver_nmi() in include/hw/i386/apic.h? Thanks.

The ifndef is the normal solution.


r~

Re: [Qemu-devel] [PATCH v2 2/5] target-i386: Implement nmi() callback

[Alexander Graf]

On 03/28/2014 01:51 PM, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   target-i386/cpu.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8fd1497..35f20e0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2489,6 +2489,19 @@ 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);
> +    } else {
> +        apic_deliver_nmi(cpu->apic_state);
> +    }

Where does this hunk come from? Shouldn't this be moved from somewhere 
else to here?


Alex

> +
> +    return 0;
> +}
> +
>   #ifndef CONFIG_USER_ONLY
>   bool cpu_is_bsp(X86CPU *cpu)
>   {
> @@ -2797,6 +2810,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;

[Qemu-devel] [PATCH v2 3/5] target-s390: Implement nmi() callback

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-s390x/cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index dfd83e8..89470a2 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -156,6 +156,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)
 {
@@ -241,6 +246,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;
-- 
1.8.4.rc4

[Qemu-devel] [PATCH v2 4/5] target-ppc: Implement nmi() callback

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-ppc/cpu-qom.h        |  1 +
 target-ppc/excp_helper.c    |  2 +-
 target-ppc/translate_init.c | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 47dc8e6..b522664 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -106,6 +106,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
 PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr);
 PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr);
 
+void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp);
 void ppc_cpu_do_interrupt(CPUState *cpu);
 void ppc_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
                         int flags);
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 19bc6b6..e19a5f5 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -68,7 +68,7 @@ static inline void dump_syscall(CPUPPCState *env)
 /* Note that this function should be greatly optimized
  * when called with a constant excp, from ppc_hw_interrupt
  */
-static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
+void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index d07e186..faa9f21 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8455,6 +8455,23 @@ static void ppc_cpu_reset(CPUState *s)
     tlb_flush(s, 1);
 }
 
+static void ppc_cpu_do_nmi(void *arg)
+{
+    CPUState *cs = arg;
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    cpu_synchronize_state(cs);
+    powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
+}
+
+static int ppc_cpu_nmi(CPUState *cs)
+{
+    async_run_on_cpu(cs, ppc_cpu_do_nmi, cs);
+
+    return 0;
+}
+
 static void ppc_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -8516,6 +8533,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 
     pcc->parent_reset = cc->reset;
     cc->reset = ppc_cpu_reset;
+    cc->nmi = ppc_cpu_nmi;
 
     cc->class_by_name = ppc_cpu_class_by_name;
     cc->has_work = ppc_cpu_has_work;
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v2 4/5] target-ppc: Implement nmi() callback

[Alexander Graf]

On 03/28/2014 01:51 PM, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   target-ppc/cpu-qom.h        |  1 +
>   target-ppc/excp_helper.c    |  2 +-
>   target-ppc/translate_init.c | 18 ++++++++++++++++++
>   3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 47dc8e6..b522664 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -106,6 +106,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>   PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr);
>   PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr);
>   
> +void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp);
>   void ppc_cpu_do_interrupt(CPUState *cpu);
>   void ppc_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
>                           int flags);
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 19bc6b6..e19a5f5 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -68,7 +68,7 @@ static inline void dump_syscall(CPUPPCState *env)
>   /* Note that this function should be greatly optimized
>    * when called with a constant excp, from ppc_hw_interrupt
>    */
> -static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> +void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>   {
>       CPUState *cs = CPU(cpu);
>       CPUPPCState *env = &cpu->env;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index d07e186..faa9f21 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8455,6 +8455,23 @@ static void ppc_cpu_reset(CPUState *s)
>       tlb_flush(s, 1);
>   }
>   
> +static void ppc_cpu_do_nmi(void *arg)
> +{
> +    CPUState *cs = arg;
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_synchronize_state(cs);
> +    powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);

I'd prefer to keep powerpc_excp internal to excp_helper.c. Can't you 
just export a do_nmi function from excp_helper.c instead?


Alex

> +}
> +
> +static int ppc_cpu_nmi(CPUState *cs)
> +{
> +    async_run_on_cpu(cs, ppc_cpu_do_nmi, cs);
> +
> +    return 0;
> +}
> +
>   static void ppc_cpu_initfn(Object *obj)
>   {
>       CPUState *cs = CPU(obj);
> @@ -8516,6 +8533,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>   
>       pcc->parent_reset = cc->reset;
>       cc->reset = ppc_cpu_reset;
> +    cc->nmi = ppc_cpu_nmi;
>   
>       cc->class_by_name = ppc_cpu_class_by_name;
>       cc->has_work = ppc_cpu_has_work;

[Qemu-devel] [PATCH v2 5/5] cpus: Enable nmi() callback use

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 cpus.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/cpus.c b/cpus.c
index 1104d61..2c8d620 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1469,33 +1469,14 @@ exit:
 
 void qmp_inject_nmi(Error **errp)
 {
-#if defined(TARGET_I386)
-    CPUState *cs;
+    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int ret = -1;
 
-    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);
-        }
+    if (cs && cc->nmi) {
+        ret = cc->nmi(cs);
     }
-#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;
-        }
+    if (ret) {
+        error_set(errp, QERR_UNSUPPORTED);
     }
-#else
-    error_set(errp, QERR_UNSUPPORTED);
-#endif
 }
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v2 5/5] cpus: Enable nmi() callback use

[Alexander Graf]

On 03/28/2014 01:51 PM, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Missing patch description.

Also, this patch should be among the first in your patch set. It makes 
review a lot easier on code movements like this when you always remove 
and add back the same code inside the same patch. So you'd keep the 
#ifdef's here, then slowly move x86 and s390 over to QMP functions.

Apart from the missing QMP description patch I think this patch set 
makes sense. But please verify that x86 and s390x NMIs still work.


Alex

> ---
>   cpus.c | 33 +++++++--------------------------
>   1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 1104d61..2c8d620 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1469,33 +1469,14 @@ exit:
>   
>   void qmp_inject_nmi(Error **errp)
>   {
> -#if defined(TARGET_I386)
> -    CPUState *cs;
> +    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int ret = -1;
>   
> -    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);
> -        }
> +    if (cs && cc->nmi) {
> +        ret = cc->nmi(cs);
>       }
> -#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;
> -        }
> +    if (ret) {
> +        error_set(errp, QERR_UNSUPPORTED);
>       }
> -#else
> -    error_set(errp, QERR_UNSUPPORTED);
> -#endif
>   }