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

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

[Alexey Kardashevskiy]

This adds machine-specific NMI handlers support. This QOM approach
was copied from FWPathProvider.

Few questions so far.

Should deliver_nmi() accept a CPU? A comment in hmp-commands.hx says
"Inject an NMI (x86), RESTART (s390x) on the given CPU" but in fact
qmp_inject_nmi() delivers NMI to every x86 CPU while it delivers only
to the current s390 CPU.

Please comment. Thanks!


Alexey Kardashevskiy (2):
  spapr: Add NMI interface
  spapr: Define NMI interface

 cpus.c                |  7 ++++++-
 hmp-commands.hx       |  4 +---
 hw/core/Makefile.objs |  1 +
 hw/core/nmi.c         | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c        | 26 ++++++++++++++++++++++++++
 include/hw/nmi.h      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 132 insertions(+), 4 deletions(-)
 create mode 100644 hw/core/nmi.c
 create mode 100644 include/hw/nmi.h

-- 
1.8.4.rc4

[Qemu-devel] [PATCH 1/2] spapr: Add NMI interface

[Alexey Kardashevskiy]

This introduces an NMI (non maskable interrupt) interface which
QMP's "nmi" command may use to issue NMI on a CPU. A machine class
is expected to implement it.

This adds a helper to obtain the interface pointer and call
the deliver_nmi handler.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 cpus.c                |  7 ++++++-
 hmp-commands.hx       |  4 +---
 hw/core/Makefile.objs |  1 +
 hw/core/nmi.c         | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/nmi.h      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 106 insertions(+), 4 deletions(-)
 create mode 100644 hw/core/nmi.c
 create mode 100644 include/hw/nmi.h

diff --git a/cpus.c b/cpus.c
index 1104d61..c5cab31 100644
--- a/cpus.c
+++ b/cpus.c
@@ -38,6 +38,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/bitmap.h"
 #include "qemu/seqlock.h"
+#include "hw/nmi.h"
 
 #ifndef _WIN32
 #include "qemu/compatfd.h"
@@ -1496,6 +1497,10 @@ void qmp_inject_nmi(Error **errp)
         }
     }
 #else
-    error_set(errp, QERR_UNSUPPORTED);
+    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
+
+    if (cs && nmi_try_deliver(cs)) {
+        error_set(errp, QERR_UNSUPPORTED);
+    }
 #endif
 }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index f3fc514..c25a0f4 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 (x86), RESTART (s390x) or platform-defined NMI on the given CPU.
 
 ETEXI
 
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 5377d05..d2a7be1 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,6 +1,7 @@
 # core qdev-related obj files, also used by *-user:
 common-obj-y += qdev.o qdev-properties.o
 common-obj-y += fw-path-provider.o
+common-obj-y += nmi.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
 common-obj-y += hotplug.o
diff --git a/hw/core/nmi.c b/hw/core/nmi.c
new file mode 100644
index 0000000..01009d3
--- /dev/null
+++ b/hw/core/nmi.c
@@ -0,0 +1,50 @@
+/*
+ *  NMI (non-maskable interrupt) interface and helper.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/nmi.h"
+
+int nmi_try_deliver(CPUState *cs)
+{
+    Object *obj = OBJECT(first_cpu);
+    NMIObj *nmi;
+    NMIClass *nmic;
+
+    while (obj->parent) {
+        nmi = (NMIObj *) object_dynamic_cast(obj, TYPE_NMI);
+        if (nmi) {
+            nmic = NMI_GET_CLASS(nmi);
+            nmic->deliver_nmi(nmi, cs);
+            return 0;
+        }
+        obj = obj->parent;
+    }
+
+    return -1;
+}
+
+static const TypeInfo nmi_info = {
+    .name           = TYPE_NMI,
+    .parent         = TYPE_INTERFACE,
+    .class_size     = sizeof(NMIClass),
+};
+
+static void nmi_register_types(void)
+{
+    type_register_static(&nmi_info);
+}
+
+type_init(nmi_register_types)
diff --git a/include/hw/nmi.h b/include/hw/nmi.h
new file mode 100644
index 0000000..53075e3
--- /dev/null
+++ b/include/hw/nmi.h
@@ -0,0 +1,48 @@
+/*
+ *  NMI (non-maskable interrupt) interface and helper definitions.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef NMI_H
+#define NMI_H 1
+
+#include "qom/object.h"
+#include "qom/cpu.h"
+
+#define TYPE_NMI "nmi"
+
+#define NMI_CLASS(klass) \
+     OBJECT_CLASS_CHECK(NMIClass, (klass), TYPE_NMI)
+#define NMI_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(NMIClass, (obj), TYPE_NMI)
+#define NMI(obj) \
+     INTERFACE_CHECK(NMIObj, (obj), TYPE_NMI)
+
+typedef struct NMIiObj {
+    /* private */
+    Object parent_obj;
+} NMIObj;
+
+typedef struct NMIClass {
+    /* private */
+    InterfaceClass parent_class;
+
+    /* public */
+    void (*deliver_nmi)(NMIObj *p, CPUState *cs);
+} NMIClass;
+
+extern int nmi_try_deliver(CPUState *cs);
+
+#endif /* NMI_H */
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 2/2] spapr: Define NMI interface

[Alexey Kardashevskiy]

This defines and makes use of an NMI interface in order to support
the "nmi" command.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 62ddb4d..495fa88 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -48,6 +48,7 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "hw/virtio/virtio-scsi.h"
+#include "hw/nmi.h"
 
 #include "exec/address-spaces.h"
 #include "hw/usb.h"
@@ -1539,13 +1540,36 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
     return NULL;
 }
 
+static void spapr_do_nmi(void *arg)
+{
+    CPUState *cs = arg;
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    cpu_synchronize_state(cs);
+    env->spr[SPR_SRR0] = env->nip;
+    env->spr[SPR_SRR1] = env->msr;
+    env->nip = 0x100;
+    env->msr = (1ULL << MSR_SF) | (1 << MSR_ME);
+    if (env->spr[SPR_LPCR] & LPCR_ILE) {
+        env->msr |= 1 << MSR_LE;
+    }
+}
+
+void spapr_deliver_nmi(NMIObj *p, CPUState *cs)
+{
+    async_run_on_cpu(cs, spapr_do_nmi, cs);
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
+    NMIClass *nmic = NMI_CLASS(oc);
 
     mc->qemu_machine = data;
     fwc->get_dev_path = spapr_get_fw_dev_path;
+    nmic->deliver_nmi = spapr_deliver_nmi;
 }
 
 static const TypeInfo spapr_machine_info = {
@@ -1555,10 +1579,12 @@ static const TypeInfo spapr_machine_info = {
     .class_data    = &spapr_machine,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_FW_PATH_PROVIDER },
+        { TYPE_NMI },
         { }
     },
 };
 
+
 static void spapr_machine_register_types(void)
 {
     type_register_static(&spapr_machine_info);
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH 2/2] spapr: Define NMI interface

[Paolo Bonzini]

Il 27/03/2014 03:21, Alexey Kardashevskiy ha scritto:
> This defines and makes use of an NMI interface in order to support
> the "nmi" command.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/ppc/spapr.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 62ddb4d..495fa88 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -48,6 +48,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
>  #include "hw/virtio/virtio-scsi.h"
> +#include "hw/nmi.h"
>
>  #include "exec/address-spaces.h"
>  #include "hw/usb.h"
> @@ -1539,13 +1540,36 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>      return NULL;
>  }
>
> +static void spapr_do_nmi(void *arg)
> +{
> +    CPUState *cs = arg;
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_synchronize_state(cs);
> +    env->spr[SPR_SRR0] = env->nip;
> +    env->spr[SPR_SRR1] = env->msr;
> +    env->nip = 0x100;
> +    env->msr = (1ULL << MSR_SF) | (1 << MSR_ME);
> +    if (env->spr[SPR_LPCR] & LPCR_ILE) {
> +        env->msr |= 1 << MSR_LE;
> +    }
> +}


I think an interface isn't the right tool here.  You want a method in 
CPUClass, and you also should move the existing code for x86 and s390 to 
target-i386 and target-s390.

Paolo

Re: [Qemu-devel] [PATCH 2/2] spapr: Define NMI interface

[Alexey Kardashevskiy]

On 03/27/2014 10:58 PM, Paolo Bonzini wrote:
> Il 27/03/2014 03:21, Alexey Kardashevskiy ha scritto:
>> This defines and makes use of an NMI interface in order to support
>> the "nmi" command.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  hw/ppc/spapr.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 62ddb4d..495fa88 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -48,6 +48,7 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/scsi/scsi.h"
>>  #include "hw/virtio/virtio-scsi.h"
>> +#include "hw/nmi.h"
>>
>>  #include "exec/address-spaces.h"
>>  #include "hw/usb.h"
>> @@ -1539,13 +1540,36 @@ static char *spapr_get_fw_dev_path(FWPathProvider
>> *p, BusState *bus,
>>      return NULL;
>>  }
>>
>> +static void spapr_do_nmi(void *arg)
>> +{
>> +    CPUState *cs = arg;
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    cpu_synchronize_state(cs);
>> +    env->spr[SPR_SRR0] = env->nip;
>> +    env->spr[SPR_SRR1] = env->msr;
>> +    env->nip = 0x100;
>> +    env->msr = (1ULL << MSR_SF) | (1 << MSR_ME);
>> +    if (env->spr[SPR_LPCR] & LPCR_ILE) {
>> +        env->msr |= 1 << MSR_LE;
>> +    }
>> +}
> 
> 
> I think an interface isn't the right tool here.  You want a method in
> CPUClass, and you also should move the existing code for x86 and s390 to
> target-i386 and target-s390.


I can do that, no big deal (bit afraid there will be some third approach
then :) ), but

1) how many x86 CPUs/families are there to support? On ppc I'll add it for
POWER7/7+/8 families and I am ok.

2) what should "nmi" really do - deliver NMI to the current CPU (as s390
does) or on all CPUs (as x86 does)?



-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] spapr: Define NMI interface

[Alexander Graf]

> Am 27.03.2014 um 19:58 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 27/03/2014 03:21, Alexey Kardashevskiy ha scritto:
>> This defines and makes use of an NMI interface in order to support
>> the "nmi" command.
>> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> hw/ppc/spapr.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 62ddb4d..495fa88 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -48,6 +48,7 @@
>> #include "hw/pci/pci.h"
>> #include "hw/scsi/scsi.h"
>> #include "hw/virtio/virtio-scsi.h"
>> +#include "hw/nmi.h"
>> 
>> #include "exec/address-spaces.h"
>> #include "hw/usb.h"
>> @@ -1539,13 +1540,36 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>>     return NULL;
>> }
>> 
>> +static void spapr_do_nmi(void *arg)
>> +{
>> +    CPUState *cs = arg;
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    cpu_synchronize_state(cs);
>> +    env->spr[SPR_SRR0] = env->nip;
>> +    env->spr[SPR_SRR1] = env->msr;
>> +    env->nip = 0x100;
>> +    env->msr = (1ULL << MSR_SF) | (1 << MSR_ME);
>> +    if (env->spr[SPR_LPCR] & LPCR_ILE) {
>> +        env->msr |= 1 << MSR_LE;
>> +    }
>> +}
> 
> 
> I think an interface isn't the right tool here.  You want a method in CPUClass, and you also should move the existing code for x86 and s390 to target-i386 and target-s390.

Also the code above is duplicating the existing sreset pin logic. We should either just trigger the sreset itq line or - if that's too hard to get working with kvm - at least share the code.

Alex

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 2/2] spapr: Define NMI interface

[Paolo Bonzini]

Il 27/03/2014 14:14, Alexey Kardashevskiy ha scritto:
> 1) how many x86 CPUs/families are there to support? On ppc I'll add it for
> POWER7/7+/8 families and I am ok.

Just move the existing x86 code.  It works on all CPUs from 486 on.

> 2) what should "nmi" really do - deliver NMI to the current CPU (as s390
> does) or on all CPUs (as x86 does)?

As you prefer.  Current CPU, as s390 does, was in your patch here and 
it's fine.  I don't know why x86 delivers it to all CPUs.

Paolo