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

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

[Alexey Kardashevskiy]

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

Please comment. Thanks.

Changes:
v6:
* back 5 steps and make it an interface again :)

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

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

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

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



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

 cpus.c                   | 31 ++----------------
 hmp-commands.hx          |  6 ++--
 hw/core/Makefile.objs    |  1 +
 hw/core/nmi.c            | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc_piix.c        | 42 +++++++++++++++++++++++++
 hw/ppc/spapr.c           | 27 ++++++++++++++++
 hw/s390x/s390-virtio.c   | 31 ++++++++++++++++++
 include/hw/nmi.h         | 46 +++++++++++++++++++++++++++
 qapi-schema.json         |  4 +--
 qmp-commands.hx          |  3 +-
 target-ppc/cpu-qom.h     |  1 +
 target-ppc/excp_helper.c |  8 +++++
 target-s390x/cpu.c       |  1 +
 13 files changed, 245 insertions(+), 37 deletions(-)
 create mode 100644 hw/core/nmi.c
 create mode 100644 include/hw/nmi.h

-- 
2.0.0

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

[Alexey Kardashevskiy]

This introduces an NMI (Non Maskable Interrupt) interface with
a single nmi_monitor_handler() method. A machine or a device can
implement it. This searches for an QOM object which supports the interface
and if it is implemented , calls it. The callback implements an action
required to cause debug crash dump on in-kernel debugger invocation.
The callback returns Error**.

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

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

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v6:
* NMI is an interface again

v5:
* s/given guest's (CPU|VCPU)/default CPU/
* nmi_monitor_handler() now returns Error**

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

v3:
* actual nmi() enablement moved from last patch to first patch
* changed description for QMP command too
---
 cpus.c                |  3 +-
 hmp-commands.hx       |  6 ++--
 hw/core/Makefile.objs |  1 +
 hw/core/nmi.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/nmi.h      | 46 +++++++++++++++++++++++++++++
 qapi-schema.json      |  4 +--
 qmp-commands.hx       |  3 +-
 7 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 hw/core/nmi.c
 create mode 100644 include/hw/nmi.h

diff --git a/cpus.c b/cpus.c
index dd7ac13..da1f6a9 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"
@@ -1495,6 +1496,6 @@ void qmp_inject_nmi(Error **errp)
         }
     }
 #else
-    error_set(errp, QERR_UNSUPPORTED);
+    nmi(monitor_get_cpu_index(), errp);
 #endif
 }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2e462c0..c25f88f 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",
         .mhandler.cmd = hmp_inject_nmi,
     },
-#endif
 STEXI
 @item nmi @var{cpu}
 @findex nmi
-Inject an NMI (x86) or RESTART (s390x) on the given CPU.
+Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).
 
 ETEXI
 
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 5377d05..17845df 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -4,6 +4,7 @@ common-obj-y += fw-path-provider.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
 common-obj-y += hotplug.o
+common-obj-y += nmi.o
 
 common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
 common-obj-$(CONFIG_XILINX_AXI) += stream.o
diff --git a/hw/core/nmi.c b/hw/core/nmi.c
new file mode 100644
index 0000000..e81ebab
--- /dev/null
+++ b/hw/core/nmi.c
@@ -0,0 +1,81 @@
+/*
+ *  NMI monitor handler class and helpers.
+ *
+ *  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; either 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"
+#include "qapi/qmp/qerror.h"
+
+struct do_nmi_s {
+    int cpu_index;
+    Error *errp;
+    bool handled;
+};
+
+static void nmi_children(Object *o, struct do_nmi_s *ns);
+
+static int do_nmi(Object *o, void *opaque)
+{
+    struct do_nmi_s *ns = opaque;
+    NMI *n = (NMI *) object_dynamic_cast(o, TYPE_NMI);
+
+    if (n) {
+        NMIClass *nc = NMI_GET_CLASS(n);
+
+        ns->handled = true;
+        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->errp);
+        if (ns->errp) {
+            return -1;
+        }
+    }
+    nmi_children(o, ns);
+
+    return 0;
+}
+
+void nmi_children(Object *o, struct do_nmi_s *ns)
+{
+    object_child_foreach(o, do_nmi, ns);
+}
+
+void nmi(int cpu_index, Error **errp)
+{
+    Object *root = object_get_root();
+    struct do_nmi_s ns = {
+        .cpu_index = cpu_index,
+        .errp = NULL,
+        .handled = false
+    };
+
+    nmi_children(root, &ns);
+    if (ns.handled) {
+        error_propagate(errp, ns.errp);
+    } else {
+        error_set(errp, QERR_UNSUPPORTED);
+    }
+}
+
+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..c2d8206
--- /dev/null
+++ b/include/hw/nmi.h
@@ -0,0 +1,46 @@
+/*
+ *  NMI monitor handler class and helpers 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; either 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 "qemu-common.h"
+#include "qom/object.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(NMI, (obj), TYPE_NMI)
+
+typedef struct NMI {
+    Object parent_obj;
+} NMI;
+
+typedef struct NMIClass {
+    InterfaceClass parent_class;
+
+    void (*nmi_monitor_handler)(NMI *n, int cpu_index, Error **errp);
+} NMIClass;
+
+void nmi(int cpu_index, Error **errp);
+
+#endif /* NMI_H */
+
diff --git a/qapi-schema.json b/qapi-schema.json
index 14b498b..8bf9849 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1116,13 +1116,13 @@
 ##
 # @inject-nmi:
 #
-# Injects an Non-Maskable Interrupt into all guest's VCPUs.
+# Injects an Non-Maskable Interrupt into the default CPU (x86/s390) or all CPUs (ppc64).
 #
 # Returns:  If successful, nothing
 #
 # Since:  0.14.0
 #
-# Notes: Only x86 Virtual Machines support this command.
+# Note: prior to 2.1, this command was only supported for x86 and s390 VMs
 ##
 { 'command': 'inject-nmi' }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d8aa4ed..0028d96 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -477,7 +477,7 @@ SQMP
 inject-nmi
 ----------
 
-Inject an NMI on guest's CPUs.
+Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).
 
 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

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

[Eric Blake]

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

On 06/11/2014 11:03 AM, Alexey Kardashevskiy wrote:
> This introduces an NMI (Non Maskable Interrupt) interface with
> a single nmi_monitor_handler() method. A machine or a device can
> implement it. This searches for an QOM object which supports the interface
> and if it is implemented , calls it. The callback implements an action
> required to cause debug crash dump on in-kernel debugger invocation.
> The callback returns Error**.
> 
> This adds support for it in qmp_inject_nmi(). Since no architecture
> supports it at the moment, there is no change in behaviour.
> 
> This changes inject-nmi command description for HMP and QMP.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---

> +++ b/hw/core/nmi.c
> @@ -0,0 +1,81 @@
> +/*
> + *  NMI monitor handler class and helpers.
> + *
> + *  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; either 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/>.
> + */

For the GPL license to work, there has to be a Copyright line.

> +static int do_nmi(Object *o, void *opaque)
> +{
> +    struct do_nmi_s *ns = opaque;
> +    NMI *n = (NMI *) object_dynamic_cast(o, TYPE_NMI);

Is the cast to (NMI *) necessary, or does object_dynamic_cast() already
return something that can be assigned to an arbitrary pointer type?


> +++ b/include/hw/nmi.h
> @@ -0,0 +1,46 @@
> +/*
> + *  NMI monitor handler class and helpers definitions.

Same comment about Copyright.


> +#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(NMI, (obj), TYPE_NMI)
> +
> +typedef struct NMI {
> +    Object parent_obj;
> +} NMI;

Eww.  This is declaring both 'NMI' as a type, and 'NMI()' as a macro.
Is that going to trip people up?  Is 'NMIState' a better name?


> +++ b/qapi-schema.json
> @@ -1116,13 +1116,13 @@
>  ##
>  # @inject-nmi:
>  #
> -# Injects an Non-Maskable Interrupt into all guest's VCPUs.
> +# Injects an Non-Maskable Interrupt into the default CPU (x86/s390) or all CPUs (ppc64).

s/an Non/a Non/

> +++ b/qmp-commands.hx
> @@ -477,7 +477,7 @@ SQMP
>  inject-nmi
>  ----------
>  
> -Inject an NMI on guest's CPUs.
> +Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).

Oh, I see the problem.  The acronym 'NMI' is pronounced with a leading
vowel (an "en-em-eye"), but the expansion is pronounced with a consonant
(a non-maskable interrupt), so the correct article depends on whether
you are spelling things out.

-- 
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 v6 1/4] cpus: Define callback for QEMU "nmi" command

[Alexey Kardashevskiy]

On 06/12/2014 03:21 AM, Eric Blake wrote:
> On 06/11/2014 11:03 AM, Alexey Kardashevskiy wrote:
>> This introduces an NMI (Non Maskable Interrupt) interface with
>> a single nmi_monitor_handler() method. A machine or a device can
>> implement it. This searches for an QOM object which supports the interface
>> and if it is implemented , calls it. The callback implements an action
>> required to cause debug crash dump on in-kernel debugger invocation.
>> The callback returns Error**.
>>
>> This adds support for it in qmp_inject_nmi(). Since no architecture
>> supports it at the moment, there is no change in behaviour.
>>
>> This changes inject-nmi command description for HMP and QMP.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
> 
>> +++ b/hw/core/nmi.c
>> @@ -0,0 +1,81 @@
>> +/*
>> + *  NMI monitor handler class and helpers.
>> + *
>> + *  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; either 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/>.
>> + */
> 
> For the GPL license to work, there has to be a Copyright line.

Hm. Ok. include/hw/fw-path-provider.h (where I copied this from) does not
have one and it was ok.

> 
>> +static int do_nmi(Object *o, void *opaque)
>> +{
>> +    struct do_nmi_s *ns = opaque;
>> +    NMI *n = (NMI *) object_dynamic_cast(o, TYPE_NMI);
> 
> Is the cast to (NMI *) necessary, or does object_dynamic_cast() already
> return something that can be assigned to an arbitrary pointer type?


All the macros from include/qom/object.h do this cast and so do I.

object_dynamic_cast returns Object*.


>> +++ b/include/hw/nmi.h
>> @@ -0,0 +1,46 @@
>> +/*
>> + *  NMI monitor handler class and helpers definitions.
> 
> Same comment about Copyright.
> 
> 
>> +#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(NMI, (obj), TYPE_NMI)
>> +
>> +typedef struct NMI {
>> +    Object parent_obj;
>> +} NMI;
> 
> Eww.  This is declaring both 'NMI' as a type, and 'NMI()' as a macro.
> Is that going to trip people up?  Is 'NMIState' a better name?

My bad, I'll fix. I'll probably get rid of the struct.

> 
> 
>> +++ b/qapi-schema.json
>> @@ -1116,13 +1116,13 @@
>>  ##
>>  # @inject-nmi:
>>  #
>> -# Injects an Non-Maskable Interrupt into all guest's VCPUs.
>> +# Injects an Non-Maskable Interrupt into the default CPU (x86/s390) or all CPUs (ppc64).
> 
> s/an Non/a Non/

Yep, here and in other places I read it as "en-em-eye" but it is not
correct right here :) I'll fix, thanks.


>> +++ b/qmp-commands.hx
>> @@ -477,7 +477,7 @@ SQMP
>>  inject-nmi
>>  ----------
>>  
>> -Inject an NMI on guest's CPUs.
>> +Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).
> 
> Oh, I see the problem.  The acronym 'NMI' is pronounced with a leading
> vowel (an "en-em-eye"), but the expansion is pronounced with a consonant
> (a non-maskable interrupt), so the correct article depends on whether
> you are spelling things out.



-- 
Alexey

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

[Eric Blake]

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

On 06/11/2014 06:10 PM, Alexey Kardashevskiy wrote:

>>> + *  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; either version 2 of the License,
>>> + *  or (at your option) any later version.

>>
>> For the GPL license to work, there has to be a Copyright line.
> 
> Hm. Ok. include/hw/fw-path-provider.h (where I copied this from) does not
> have one and it was ok.

Just because there's existing bad practice in the code base doesn't mean
new files should perpetuate it :)

> 
>>
>>> +static int do_nmi(Object *o, void *opaque)
>>> +{
>>> +    struct do_nmi_s *ns = opaque;
>>> +    NMI *n = (NMI *) object_dynamic_cast(o, TYPE_NMI);
>>
>> Is the cast to (NMI *) necessary, or does object_dynamic_cast() already
>> return something that can be assigned to an arbitrary pointer type?
> 
> 
> All the macros from include/qom/object.h do this cast and so do I.
> 
> object_dynamic_cast returns Object*.

Okay, then the cast is needed.  (I wonder if object_dynamic_cast()
should return void* to avoid the need for casting, but that probably
invites more errors than what it saves from the eliminated casts).

-- 
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 v6 1/4] cpus: Define callback for QEMU "nmi" command

[Alexey Kardashevskiy]

On 06/12/2014 11:46 AM, Eric Blake wrote:
> On 06/11/2014 06:10 PM, Alexey Kardashevskiy wrote:
> 
>>>> + *  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; either version 2 of the License,
>>>> + *  or (at your option) any later version.
> 
>>>
>>> For the GPL license to work, there has to be a Copyright line.
>>
>> Hm. Ok. include/hw/fw-path-provider.h (where I copied this from) does not
>> have one and it was ok.
> 
> Just because there's existing bad practice in the code base doesn't mean
> new files should perpetuate it :)

Paolo did that in the first place and if he does it, I assume this is at
least not "bad" ;)

Like this? I do not know what should be there, corporation or something or
just me?

--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -1,6 +1,8 @@
 /*
  *  NMI monitor handler class and helpers.
  *
+ *  Copyright (c) 2014 Alexey Kardashevskiy <aik@ozlabs.ru>
+ *
  *  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; either version 2 of the License,




-- 
Alexey

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

[Eric Blake]

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

On 06/11/2014 09:29 PM, Alexey Kardashevskiy wrote:
> On 06/12/2014 11:46 AM, Eric Blake wrote:
>> On 06/11/2014 06:10 PM, Alexey Kardashevskiy wrote:
>>
>>>>> + *  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; either version 2 of the License,
>>>>> + *  or (at your option) any later version.
>>
>>>>
>>>> For the GPL license to work, there has to be a Copyright line.
>>>
>>> Hm. Ok. include/hw/fw-path-provider.h (where I copied this from) does not
>>> have one and it was ok.
>>
>> Just because there's existing bad practice in the code base doesn't mean
>> new files should perpetuate it :)
> 
> Paolo did that in the first place and if he does it, I assume this is at
> least not "bad" ;)
> 
> Like this? I do not know what should be there, corporation or something or
> just me?

If you are submitting on behalf of an employer, then list your employer;
otherwise listing yourself is just fine.


> 
> --- a/hw/core/nmi.c
> +++ b/hw/core/nmi.c
> @@ -1,6 +1,8 @@
>  /*
>   *  NMI monitor handler class and helpers.
>   *
> + *  Copyright (c) 2014 Alexey Kardashevskiy <aik@ozlabs.ru>
> + *
>   *  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; either version 2 of the License,
> 

Yes, that is probably just fine since you are the initial contributor of
the file.  Another option is if your new file is significantly
copy-and-pasted from some other file, then you would preserve whatever
copyright was present in that earlier file.

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

[Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexey Kardashevskiy]

This implements an NMI interface for s390 machine.

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

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

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

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

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v6:
* supported NMI interface

v5:
* added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this

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

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

---
Is there any good reason to have @cpu_num in addition to @cpu_index?
Just asking :)
---
 cpus.c                 | 14 --------------
 hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
 target-s390x/cpu.c     |  1 +
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/cpus.c b/cpus.c
index da1f6a9..f3756b4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1481,20 +1481,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
     nmi(monitor_get_cpu_index(), errp);
 #endif
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 93c7ace..9c5b7b2 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -38,6 +38,7 @@
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/s390-virtio.h"
+#include "hw/nmi.h"
 
 //#define DEBUG_S390
 
@@ -51,6 +52,7 @@
 
 #define MAX_BLK_DEVS                    10
 #define ZIPL_FILENAME                   "s390-zipl.rom"
+#define TYPE_NMI_S390                   "s390_nmi"
 
 static VirtIOS390Bus *s390_bus;
 static S390CPU **ipi_states;
@@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
 
     /* Create VirtIO network adapters */
     s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
+
+    object_property_add_child(OBJECT(machine), "nmi",
+                              object_new(TYPE_NMI_S390), NULL);
 }
 
 static QEMUMachine s390_machine = {
@@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
     .is_default = 1,
 };
 
+static void s390_nmi(NMI *n, int cpu_index, Error **errp)
+{
+    CPUState *cs = qemu_get_cpu(cpu_index);
+
+    if (s390_cpu_restart(S390_CPU(cs))) {
+        error_set(errp, QERR_UNSUPPORTED);
+    }
+}
+
+static void s390_nmi_class_init(ObjectClass *oc, void *data)
+{
+    NMIClass *nc = NMI_CLASS(oc);
+    nc->nmi_monitor_handler = s390_nmi;
+}
+
+static const TypeInfo s390_nmi_info = {
+    .name          = TYPE_NMI_S390,
+    .parent        = TYPE_OBJECT,
+    .class_init    = s390_nmi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_NMI },
+        { }
+    },
+};
+
 static void s390_machine_init(void)
 {
+    type_register_static(&s390_nmi_info);
     qemu_register_machine(&s390_machine);
 }
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c3082b7..15142ff 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -27,6 +27,7 @@
 #include "qemu-common.h"
 #include "qemu/timer.h"
 #include "hw/hw.h"
+#include "qapi/qmp/qerror.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
-- 
2.0.0

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Paolo Bonzini]

Il 11/06/2014 19:03, Alexey Kardashevskiy ha scritto:
> This implements an NMI interface for s390 machine.
>
> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
> nmi_monitor_handler() callback is going to be used for NMI.
>
> Since nmi_monitor_handler()-calling code is platform independent,
> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
> There should not be any change in behaviour as both @cpu_index and
> @cpu_num are global CPU numbers.
>
> Also, s390_cpu_restart() takes care of preforming operations in
> the specific CPU thread so no extra measure is required here either.
>
> Since the only error s390_cpu_restart() can return is ENOSYS, convert
> it to QERR_UNSUPPORTED.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v6:
> * supported NMI interface
>
> v5:
> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>
> v4:
> * s/\<nmi\>/nmi_monitor_handler/
>
> v3:
> * now contains both old code removal and new code insertion, easier to
> track changes
>
> ---
> Is there any good reason to have @cpu_num in addition to @cpu_index?
> Just asking :)
> ---
>  cpus.c                 | 14 --------------
>  hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>  target-s390x/cpu.c     |  1 +
>  3 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index da1f6a9..f3756b4 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1481,20 +1481,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
>      nmi(monitor_get_cpu_index(), errp);
>  #endif
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 93c7ace..9c5b7b2 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -38,6 +38,7 @@
>  #include "hw/s390x/sclp.h"
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/s390-virtio.h"
> +#include "hw/nmi.h"
>
>  //#define DEBUG_S390
>
> @@ -51,6 +52,7 @@
>
>  #define MAX_BLK_DEVS                    10
>  #define ZIPL_FILENAME                   "s390-zipl.rom"
> +#define TYPE_NMI_S390                   "s390_nmi"
>
>  static VirtIOS390Bus *s390_bus;
>  static S390CPU **ipi_states;
> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
>
>      /* Create VirtIO network adapters */
>      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
> +
> +    object_property_add_child(OBJECT(machine), "nmi",
> +                              object_new(TYPE_NMI_S390), NULL);
>  }
>
>  static QEMUMachine s390_machine = {
> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>      .is_default = 1,
>  };
>
> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
> +{
> +    CPUState *cs = qemu_get_cpu(cpu_index);
> +
> +    if (s390_cpu_restart(S390_CPU(cs))) {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
> +{
> +    NMIClass *nc = NMI_CLASS(oc);
> +    nc->nmi_monitor_handler = s390_nmi;
> +}
> +
> +static const TypeInfo s390_nmi_info = {
> +    .name          = TYPE_NMI_S390,
> +    .parent        = TYPE_OBJECT,
> +    .class_init    = s390_nmi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_NMI },
> +        { }
> +    },
> +};
> +
>  static void s390_machine_init(void)
>  {
> +    type_register_static(&s390_nmi_info);
>      qemu_register_machine(&s390_machine);

I would use the /machine object itself, not an extra object.  Otherwise 
this looks good.

Paolo

>  }
>
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index c3082b7..15142ff 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -27,6 +27,7 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/hw.h"
> +#include "qapi/qmp/qerror.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
>  #endif
>

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexey Kardashevskiy]

On 06/12/2014 03:29 AM, Paolo Bonzini wrote:
> Il 11/06/2014 19:03, Alexey Kardashevskiy ha scritto:
>> This implements an NMI interface for s390 machine.
>>
>> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
>> nmi_monitor_handler() callback is going to be used for NMI.
>>
>> Since nmi_monitor_handler()-calling code is platform independent,
>> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
>> There should not be any change in behaviour as both @cpu_index and
>> @cpu_num are global CPU numbers.
>>
>> Also, s390_cpu_restart() takes care of preforming operations in
>> the specific CPU thread so no extra measure is required here either.
>>
>> Since the only error s390_cpu_restart() can return is ENOSYS, convert
>> it to QERR_UNSUPPORTED.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v6:
>> * supported NMI interface
>>
>> v5:
>> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>>
>> v4:
>> * s/\<nmi\>/nmi_monitor_handler/
>>
>> v3:
>> * now contains both old code removal and new code insertion, easier to
>> track changes
>>
>> ---
>> Is there any good reason to have @cpu_num in addition to @cpu_index?
>> Just asking :)
>> ---
>>  cpus.c                 | 14 --------------
>>  hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>>  target-s390x/cpu.c     |  1 +
>>  3 files changed, 32 insertions(+), 14 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index da1f6a9..f3756b4 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1481,20 +1481,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
>>      nmi(monitor_get_cpu_index(), errp);
>>  #endif
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 93c7ace..9c5b7b2 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -38,6 +38,7 @@
>>  #include "hw/s390x/sclp.h"
>>  #include "hw/s390x/s390_flic.h"
>>  #include "hw/s390x/s390-virtio.h"
>> +#include "hw/nmi.h"
>>
>>  //#define DEBUG_S390
>>
>> @@ -51,6 +52,7 @@
>>
>>  #define MAX_BLK_DEVS                    10
>>  #define ZIPL_FILENAME                   "s390-zipl.rom"
>> +#define TYPE_NMI_S390                   "s390_nmi"
>>
>>  static VirtIOS390Bus *s390_bus;
>>  static S390CPU **ipi_states;
>> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
>>
>>      /* Create VirtIO network adapters */
>>      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
>> +
>> +    object_property_add_child(OBJECT(machine), "nmi",
>> +                              object_new(TYPE_NMI_S390), NULL);
>>  }
>>
>>  static QEMUMachine s390_machine = {
>> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>>      .is_default = 1,
>>  };
>>
>> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
>> +{
>> +    CPUState *cs = qemu_get_cpu(cpu_index);
>> +
>> +    if (s390_cpu_restart(S390_CPU(cs))) {
>> +        error_set(errp, QERR_UNSUPPORTED);
>> +    }
>> +}
>> +
>> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
>> +{
>> +    NMIClass *nc = NMI_CLASS(oc);
>> +    nc->nmi_monitor_handler = s390_nmi;
>> +}
>> +
>> +static const TypeInfo s390_nmi_info = {
>> +    .name          = TYPE_NMI_S390,
>> +    .parent        = TYPE_OBJECT,
>> +    .class_init    = s390_nmi_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_NMI },
>> +        { }
>> +    },
>> +};
>> +
>>  static void s390_machine_init(void)
>>  {
>> +    type_register_static(&s390_nmi_info);
>>      qemu_register_machine(&s390_machine);
> 
> I would use the /machine object itself, not an extra object.  Otherwise
> this looks good.


"/machine" object is "machine" parameter from s390_init()? How do I add an
interface to it? Create a machine class as it is done by
spapr_machine_info? I am not that familiar with QOM and definitely miss
something.



> 
> Paolo
> 
>>  }
>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index c3082b7..15142ff 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -27,6 +27,7 @@
>>  #include "qemu-common.h"
>>  #include "qemu/timer.h"
>>  #include "hw/hw.h"
>> +#include "qapi/qmp/qerror.h"
>>  #ifndef CONFIG_USER_ONLY
>>  #include "sysemu/arch_init.h"
>>  #endif
>>
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Paolo Bonzini]

Il 12/06/2014 02:08, Alexey Kardashevskiy ha scritto:
>> > I would use the /machine object itself, not an extra object.  Otherwise
>> > this looks good.
>
> "/machine" object is "machine" parameter from s390_init()? How do I add an
> interface to it? Create a machine class as it is done by
> spapr_machine_info? I am not that familiar with QOM and definitely miss
> something.

No, you are not. :)  You need exactly to do what was done for sPAPR. 
Note that s390 has two machine classes.

Paolo

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexey Kardashevskiy]

On 06/12/2014 05:42 PM, Paolo Bonzini wrote:
> Il 12/06/2014 02:08, Alexey Kardashevskiy ha scritto:
>>> > I would use the /machine object itself, not an extra object.  Otherwise
>>> > this looks good.
>>
>> "/machine" object is "machine" parameter from s390_init()? How do I add an
>> interface to it? Create a machine class as it is done by
>> spapr_machine_info? I am not that familiar with QOM and definitely miss
>> something.
> 
> No, you are not. :)  You need exactly to do what was done for sPAPR. Note
> that s390 has two machine classes.

2 machines... Heh. If only. Here is x86:

qemu_register_machine(&pc_i440fx_machine_v2_1);
qemu_register_machine(&pc_i440fx_machine_v2_0);
qemu_register_machine(&pc_i440fx_machine_v1_7);
qemu_register_machine(&pc_i440fx_machine_v1_6);
qemu_register_machine(&pc_i440fx_machine_v1_5);
qemu_register_machine(&pc_i440fx_machine_v1_4);
qemu_register_machine(&pc_machine_v1_3);
qemu_register_machine(&pc_machine_v1_2);
qemu_register_machine(&pc_machine_v1_1);
qemu_register_machine(&pc_machine_v1_0);
qemu_register_machine(&pc_machine_v0_15);
qemu_register_machine(&pc_machine_v0_14);
qemu_register_machine(&pc_machine_v0_13);
qemu_register_machine(&pc_machine_v0_12);
qemu_register_machine(&pc_machine_v0_11);
qemu_register_machine(&pc_machine_v0_10);
qemu_register_machine(&isapc_machine);
qemu_register_machine(&xenfv_machine);


qemu_register_machine(&pc_q35_machine_v2_1);
qemu_register_machine(&pc_q35_machine_v2_0);
qemu_register_machine(&pc_q35_machine_v1_7);
qemu_register_machine(&pc_q35_machine_v1_6);
qemu_register_machine(&pc_q35_machine_v1_5);
qemu_register_machine(&pc_q35_machine_v1_4);


These all are QEMUMachine's, not TYPE_MACHINE so I'll need to convert them too.

Converting SPAPR's QEMUMachine took several patches,
958db90cd54823c33345000c995453a8c9b7a005 "machine: Remove QEMUMachine
indirection from MachineClass" was quite big.

If you tell me to convert them all, ok, I'll do it :)


-- 
Alexey

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Paolo Bonzini]

Il 12/06/2014 10:35, Alexey Kardashevskiy ha scritto:
> On 06/12/2014 05:42 PM, Paolo Bonzini wrote:
>> Il 12/06/2014 02:08, Alexey Kardashevskiy ha scritto:
>>>>> I would use the /machine object itself, not an extra object.  Otherwise
>>>>> this looks good.
>>>
>>> "/machine" object is "machine" parameter from s390_init()? How do I add an
>>> interface to it? Create a machine class as it is done by
>>> spapr_machine_info? I am not that familiar with QOM and definitely miss
>>> something.
>>
>> No, you are not. :)  You need exactly to do what was done for sPAPR. Note
>> that s390 has two machine classes.
>
> 2 machines... Heh. If only. Here is x86:
>
> qemu_register_machine(&pc_i440fx_machine_v2_1);
> qemu_register_machine(&pc_i440fx_machine_v2_0);
> qemu_register_machine(&pc_i440fx_machine_v1_7);
> qemu_register_machine(&pc_i440fx_machine_v1_6);
> qemu_register_machine(&pc_i440fx_machine_v1_5);
> qemu_register_machine(&pc_i440fx_machine_v1_4);
> qemu_register_machine(&pc_machine_v1_3);
> qemu_register_machine(&pc_machine_v1_2);
> qemu_register_machine(&pc_machine_v1_1);
> qemu_register_machine(&pc_machine_v1_0);
> qemu_register_machine(&pc_machine_v0_15);
> qemu_register_machine(&pc_machine_v0_14);
> qemu_register_machine(&pc_machine_v0_13);
> qemu_register_machine(&pc_machine_v0_12);
> qemu_register_machine(&pc_machine_v0_11);
> qemu_register_machine(&pc_machine_v0_10);
> qemu_register_machine(&isapc_machine);
> qemu_register_machine(&xenfv_machine);
>
>
> qemu_register_machine(&pc_q35_machine_v2_1);
> qemu_register_machine(&pc_q35_machine_v2_0);
> qemu_register_machine(&pc_q35_machine_v1_7);
> qemu_register_machine(&pc_q35_machine_v1_6);
> qemu_register_machine(&pc_q35_machine_v1_5);
> qemu_register_machine(&pc_q35_machine_v1_4);
>
>
> These all are QEMUMachine's, not TYPE_MACHINE so I'll need to convert them too.

There is a patch on the list from Eduardo Habkost to convert them and 
especially to add a common TYPE_PC_MACHINE superclass.

Just do s390 and PPC for now.  We'll take care later of x86.

Paolo

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Cornelia Huck]

On Thu, 12 Jun 2014 03:03:01 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This implements an NMI interface for s390 machine.
> 
> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
> nmi_monitor_handler() callback is going to be used for NMI.
> 
> Since nmi_monitor_handler()-calling code is platform independent,
> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
> There should not be any change in behaviour as both @cpu_index and
> @cpu_num are global CPU numbers.
> 
> Also, s390_cpu_restart() takes care of preforming operations in
> the specific CPU thread so no extra measure is required here either.
> 
> Since the only error s390_cpu_restart() can return is ENOSYS, convert
> it to QERR_UNSUPPORTED.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v6:
> * supported NMI interface
> 
> v5:
> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
> 
> v4:
> * s/\<nmi\>/nmi_monitor_handler/
> 
> v3:
> * now contains both old code removal and new code insertion, easier to
> track changes
> 
> ---
> Is there any good reason to have @cpu_num in addition to @cpu_index?
> Just asking :)
> ---
>  cpus.c                 | 14 --------------
>  hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>  target-s390x/cpu.c     |  1 +
>  3 files changed, 32 insertions(+), 14 deletions(-)
> 

> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 93c7ace..9c5b7b2 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -38,6 +38,7 @@
>  #include "hw/s390x/sclp.h"
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/s390-virtio.h"
> +#include "hw/nmi.h"
> 
>  //#define DEBUG_S390
> 
> @@ -51,6 +52,7 @@
> 
>  #define MAX_BLK_DEVS                    10
>  #define ZIPL_FILENAME                   "s390-zipl.rom"
> +#define TYPE_NMI_S390                   "s390_nmi"

I'd prefer "s390-nmi" instead.

> 
>  static VirtIOS390Bus *s390_bus;
>  static S390CPU **ipi_states;
> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
> 
>      /* Create VirtIO network adapters */
>      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
> +
> +    object_property_add_child(OBJECT(machine), "nmi",
> +                              object_new(TYPE_NMI_S390), NULL);

This only adds the nmi interface to the old s390-virtio machine; we
want this for the s390-virtio-ccw machine as well.

>  }
> 
>  static QEMUMachine s390_machine = {
> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>      .is_default = 1,
>  };
> 
> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
> +{
> +    CPUState *cs = qemu_get_cpu(cpu_index);
> +
> +    if (s390_cpu_restart(S390_CPU(cs))) {
> +        error_set(errp, QERR_UNSUPPORTED);
> +    }
> +}
> +
> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
> +{
> +    NMIClass *nc = NMI_CLASS(oc);
> +    nc->nmi_monitor_handler = s390_nmi;
> +}
> +
> +static const TypeInfo s390_nmi_info = {
> +    .name          = TYPE_NMI_S390,
> +    .parent        = TYPE_OBJECT,
> +    .class_init    = s390_nmi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_NMI },
> +        { }
> +    },
> +};
> +
>  static void s390_machine_init(void)
>  {
> +    type_register_static(&s390_nmi_info);

s390-virtio-ccw needs this as well.

>      qemu_register_machine(&s390_machine);
>  }

The best way would probably be to put all nmi-related things into a new
file that registers the nmi type and provides an s390_register_nmi()
helper.

> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index c3082b7..15142ff 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -27,6 +27,7 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/hw.h"
> +#include "qapi/qmp/qerror.h"

Leftover?

>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
>  #endif

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexey Kardashevskiy]

On 06/12/2014 04:31 PM, Cornelia Huck wrote:
> On Thu, 12 Jun 2014 03:03:01 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> This implements an NMI interface for s390 machine.
>>
>> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
>> nmi_monitor_handler() callback is going to be used for NMI.
>>
>> Since nmi_monitor_handler()-calling code is platform independent,
>> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
>> There should not be any change in behaviour as both @cpu_index and
>> @cpu_num are global CPU numbers.
>>
>> Also, s390_cpu_restart() takes care of preforming operations in
>> the specific CPU thread so no extra measure is required here either.
>>
>> Since the only error s390_cpu_restart() can return is ENOSYS, convert
>> it to QERR_UNSUPPORTED.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v6:
>> * supported NMI interface
>>
>> v5:
>> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>>
>> v4:
>> * s/\<nmi\>/nmi_monitor_handler/
>>
>> v3:
>> * now contains both old code removal and new code insertion, easier to
>> track changes
>>
>> ---
>> Is there any good reason to have @cpu_num in addition to @cpu_index?
>> Just asking :)
>> ---
>>  cpus.c                 | 14 --------------
>>  hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>>  target-s390x/cpu.c     |  1 +
>>  3 files changed, 32 insertions(+), 14 deletions(-)
>>
> 
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 93c7ace..9c5b7b2 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -38,6 +38,7 @@
>>  #include "hw/s390x/sclp.h"
>>  #include "hw/s390x/s390_flic.h"
>>  #include "hw/s390x/s390-virtio.h"
>> +#include "hw/nmi.h"
>>
>>  //#define DEBUG_S390
>>
>> @@ -51,6 +52,7 @@
>>
>>  #define MAX_BLK_DEVS                    10
>>  #define ZIPL_FILENAME                   "s390-zipl.rom"
>> +#define TYPE_NMI_S390                   "s390_nmi"
> 
> I'd prefer "s390-nmi" instead.
>
>>
>>  static VirtIOS390Bus *s390_bus;
>>  static S390CPU **ipi_states;
>> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
>>
>>      /* Create VirtIO network adapters */
>>      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
>> +
>> +    object_property_add_child(OBJECT(machine), "nmi",
>> +                              object_new(TYPE_NMI_S390), NULL);
> 
> This only adds the nmi interface to the old s390-virtio machine; we
> want this for the s390-virtio-ccw machine as well.
> 
>>  }
>>
>>  static QEMUMachine s390_machine = {
>> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>>      .is_default = 1,
>>  };
>>
>> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
>> +{
>> +    CPUState *cs = qemu_get_cpu(cpu_index);
>> +
>> +    if (s390_cpu_restart(S390_CPU(cs))) {
>> +        error_set(errp, QERR_UNSUPPORTED);
>> +    }
>> +}
>> +
>> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
>> +{
>> +    NMIClass *nc = NMI_CLASS(oc);
>> +    nc->nmi_monitor_handler = s390_nmi;
>> +}
>> +
>> +static const TypeInfo s390_nmi_info = {
>> +    .name          = TYPE_NMI_S390,
>> +    .parent        = TYPE_OBJECT,
>> +    .class_init    = s390_nmi_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_NMI },
>> +        { }
>> +    },
>> +};
>> +
>>  static void s390_machine_init(void)
>>  {
>> +    type_register_static(&s390_nmi_info);
> 
> s390-virtio-ccw needs this as well.
> 
>>      qemu_register_machine(&s390_machine);
>>  }
> 
> The best way would probably be to put all nmi-related things into a new
> file that registers the nmi type and provides an s390_register_nmi()
> helper.


I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7
Please have a look and give it a go - I do not have s390 kernel/images
handy. Thanks!

It does not look like we really need a new file for NMI now.
Also, should I put it under #ifdef CONFIG_USER_ONLY on s390?


>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index c3082b7..15142ff 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -27,6 +27,7 @@
>>  #include "qemu-common.h"
>>  #include "qemu/timer.h"
>>  #include "hw/hw.h"
>> +#include "qapi/qmp/qerror.h"
> 
> Leftover?

yes :(


>>  #ifndef CONFIG_USER_ONLY
>>  #include "sysemu/arch_init.h"
>>  #endif
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexander Graf]

On 12.06.14 11:38, Alexey Kardashevskiy wrote:
> On 06/12/2014 04:31 PM, Cornelia Huck wrote:
>> On Thu, 12 Jun 2014 03:03:01 +1000
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>> This implements an NMI interface for s390 machine.
>>>
>>> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
>>> nmi_monitor_handler() callback is going to be used for NMI.
>>>
>>> Since nmi_monitor_handler()-calling code is platform independent,
>>> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
>>> There should not be any change in behaviour as both @cpu_index and
>>> @cpu_num are global CPU numbers.
>>>
>>> Also, s390_cpu_restart() takes care of preforming operations in
>>> the specific CPU thread so no extra measure is required here either.
>>>
>>> Since the only error s390_cpu_restart() can return is ENOSYS, convert
>>> it to QERR_UNSUPPORTED.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v6:
>>> * supported NMI interface
>>>
>>> v5:
>>> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>>>
>>> v4:
>>> * s/\<nmi\>/nmi_monitor_handler/
>>>
>>> v3:
>>> * now contains both old code removal and new code insertion, easier to
>>> track changes
>>>
>>> ---
>>> Is there any good reason to have @cpu_num in addition to @cpu_index?
>>> Just asking :)
>>> ---
>>>   cpus.c                 | 14 --------------
>>>   hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>>>   target-s390x/cpu.c     |  1 +
>>>   3 files changed, 32 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>>> index 93c7ace..9c5b7b2 100644
>>> --- a/hw/s390x/s390-virtio.c
>>> +++ b/hw/s390x/s390-virtio.c
>>> @@ -38,6 +38,7 @@
>>>   #include "hw/s390x/sclp.h"
>>>   #include "hw/s390x/s390_flic.h"
>>>   #include "hw/s390x/s390-virtio.h"
>>> +#include "hw/nmi.h"
>>>
>>>   //#define DEBUG_S390
>>>
>>> @@ -51,6 +52,7 @@
>>>
>>>   #define MAX_BLK_DEVS                    10
>>>   #define ZIPL_FILENAME                   "s390-zipl.rom"
>>> +#define TYPE_NMI_S390                   "s390_nmi"
>> I'd prefer "s390-nmi" instead.
>>
>>>   static VirtIOS390Bus *s390_bus;
>>>   static S390CPU **ipi_states;
>>> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
>>>
>>>       /* Create VirtIO network adapters */
>>>       s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
>>> +
>>> +    object_property_add_child(OBJECT(machine), "nmi",
>>> +                              object_new(TYPE_NMI_S390), NULL);
>> This only adds the nmi interface to the old s390-virtio machine; we
>> want this for the s390-virtio-ccw machine as well.
>>
>>>   }
>>>
>>>   static QEMUMachine s390_machine = {
>>> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>>>       .is_default = 1,
>>>   };
>>>
>>> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
>>> +{
>>> +    CPUState *cs = qemu_get_cpu(cpu_index);
>>> +
>>> +    if (s390_cpu_restart(S390_CPU(cs))) {
>>> +        error_set(errp, QERR_UNSUPPORTED);
>>> +    }
>>> +}
>>> +
>>> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    NMIClass *nc = NMI_CLASS(oc);
>>> +    nc->nmi_monitor_handler = s390_nmi;
>>> +}
>>> +
>>> +static const TypeInfo s390_nmi_info = {
>>> +    .name          = TYPE_NMI_S390,
>>> +    .parent        = TYPE_OBJECT,
>>> +    .class_init    = s390_nmi_class_init,
>>> +    .interfaces = (InterfaceInfo[]) {
>>> +        { TYPE_NMI },
>>> +        { }
>>> +    },
>>> +};
>>> +
>>>   static void s390_machine_init(void)
>>>   {
>>> +    type_register_static(&s390_nmi_info);
>> s390-virtio-ccw needs this as well.
>>
>>>       qemu_register_machine(&s390_machine);
>>>   }
>> The best way would probably be to put all nmi-related things into a new
>> file that registers the nmi type and provides an s390_register_nmi()
>> helper.
>
> I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7
> Please have a look and give it a go - I do not have s390 kernel/images
> handy. Thanks!
>
> It does not look like we really need a new file for NMI now.
> Also, should I put it under #ifdef CONFIG_USER_ONLY on s390?

The machine only ever gets compiled on for non-user, so why would you 
need any #ifdefs?


Alex

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexey Kardashevskiy]

On 06/12/2014 07:39 PM, Alexander Graf wrote:
> 
> On 12.06.14 11:38, Alexey Kardashevskiy wrote:
>> On 06/12/2014 04:31 PM, Cornelia Huck wrote:
>>> On Thu, 12 Jun 2014 03:03:01 +1000
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> This implements an NMI interface for s390 machine.
>>>>
>>>> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
>>>> nmi_monitor_handler() callback is going to be used for NMI.
>>>>
>>>> Since nmi_monitor_handler()-calling code is platform independent,
>>>> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
>>>> There should not be any change in behaviour as both @cpu_index and
>>>> @cpu_num are global CPU numbers.
>>>>
>>>> Also, s390_cpu_restart() takes care of preforming operations in
>>>> the specific CPU thread so no extra measure is required here either.
>>>>
>>>> Since the only error s390_cpu_restart() can return is ENOSYS, convert
>>>> it to QERR_UNSUPPORTED.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v6:
>>>> * supported NMI interface
>>>>
>>>> v5:
>>>> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>>>>
>>>> v4:
>>>> * s/\<nmi\>/nmi_monitor_handler/
>>>>
>>>> v3:
>>>> * now contains both old code removal and new code insertion, easier to
>>>> track changes
>>>>
>>>> ---
>>>> Is there any good reason to have @cpu_num in addition to @cpu_index?
>>>> Just asking :)
>>>> ---
>>>>   cpus.c                 | 14 --------------
>>>>   hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>>>>   target-s390x/cpu.c     |  1 +
>>>>   3 files changed, 32 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>>>> index 93c7ace..9c5b7b2 100644
>>>> --- a/hw/s390x/s390-virtio.c
>>>> +++ b/hw/s390x/s390-virtio.c
>>>> @@ -38,6 +38,7 @@
>>>>   #include "hw/s390x/sclp.h"
>>>>   #include "hw/s390x/s390_flic.h"
>>>>   #include "hw/s390x/s390-virtio.h"
>>>> +#include "hw/nmi.h"
>>>>
>>>>   //#define DEBUG_S390
>>>>
>>>> @@ -51,6 +52,7 @@
>>>>
>>>>   #define MAX_BLK_DEVS                    10
>>>>   #define ZIPL_FILENAME                   "s390-zipl.rom"
>>>> +#define TYPE_NMI_S390                   "s390_nmi"
>>> I'd prefer "s390-nmi" instead.
>>>
>>>>   static VirtIOS390Bus *s390_bus;
>>>>   static S390CPU **ipi_states;
>>>> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
>>>>
>>>>       /* Create VirtIO network adapters */
>>>>       s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
>>>> +
>>>> +    object_property_add_child(OBJECT(machine), "nmi",
>>>> +                              object_new(TYPE_NMI_S390), NULL);
>>> This only adds the nmi interface to the old s390-virtio machine; we
>>> want this for the s390-virtio-ccw machine as well.
>>>
>>>>   }
>>>>
>>>>   static QEMUMachine s390_machine = {
>>>> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>>>>       .is_default = 1,
>>>>   };
>>>>
>>>> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
>>>> +{
>>>> +    CPUState *cs = qemu_get_cpu(cpu_index);
>>>> +
>>>> +    if (s390_cpu_restart(S390_CPU(cs))) {
>>>> +        error_set(errp, QERR_UNSUPPORTED);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
>>>> +{
>>>> +    NMIClass *nc = NMI_CLASS(oc);
>>>> +    nc->nmi_monitor_handler = s390_nmi;
>>>> +}
>>>> +
>>>> +static const TypeInfo s390_nmi_info = {
>>>> +    .name          = TYPE_NMI_S390,
>>>> +    .parent        = TYPE_OBJECT,
>>>> +    .class_init    = s390_nmi_class_init,
>>>> +    .interfaces = (InterfaceInfo[]) {
>>>> +        { TYPE_NMI },
>>>> +        { }
>>>> +    },
>>>> +};
>>>> +
>>>>   static void s390_machine_init(void)
>>>>   {
>>>> +    type_register_static(&s390_nmi_info);
>>> s390-virtio-ccw needs this as well.
>>>
>>>>       qemu_register_machine(&s390_machine);
>>>>   }
>>> The best way would probably be to put all nmi-related things into a new
>>> file that registers the nmi type and provides an s390_register_nmi()
>>> helper.
>>
>> I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7
>> Please have a look and give it a go - I do not have s390 kernel/images
>> handy. Thanks!
>>
>> It does not look like we really need a new file for NMI now.
>> Also, should I put it under #ifdef CONFIG_USER_ONLY on s390?
> 
> The machine only ever gets compiled on for non-user, so why would you need
> any #ifdefs?

Oh, right, this is a leftover in my head from when it was a CPU callback :)



-- 
Alexey

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Cornelia Huck]

On Thu, 12 Jun 2014 19:38:40 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 06/12/2014 04:31 PM, Cornelia Huck wrote:
> > On Thu, 12 Jun 2014 03:03:01 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> >> This implements an NMI interface for s390 machine.
> >>
> >> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
> >> nmi_monitor_handler() callback is going to be used for NMI.
> >>
> >> Since nmi_monitor_handler()-calling code is platform independent,
> >> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
> >> There should not be any change in behaviour as both @cpu_index and
> >> @cpu_num are global CPU numbers.
> >>
> >> Also, s390_cpu_restart() takes care of preforming operations in
> >> the specific CPU thread so no extra measure is required here either.
> >>
> >> Since the only error s390_cpu_restart() can return is ENOSYS, convert
> >> it to QERR_UNSUPPORTED.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> Changes:
> >> v6:
> >> * supported NMI interface
> >>
> >> v5:
> >> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
> >>
> >> v4:
> >> * s/\<nmi\>/nmi_monitor_handler/
> >>
> >> v3:
> >> * now contains both old code removal and new code insertion, easier to
> >> track changes
> >>
> >> ---
> >> Is there any good reason to have @cpu_num in addition to @cpu_index?
> >> Just asking :)
> >> ---
> >>  cpus.c                 | 14 --------------
> >>  hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
> >>  target-s390x/cpu.c     |  1 +
> >>  3 files changed, 32 insertions(+), 14 deletions(-)
> >>

> I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7
> Please have a look and give it a go - I do not have s390 kernel/images
> handy. Thanks!

Gave it a try with both of the machines: Triggering 'nmi' from the
monitor still triggers the configured on_restart action, so this seems
to work as well as before.

> 
> It does not look like we really need a new file for NMI now.

Yes, I think the v7 code looks fine.

Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexey Kardashevskiy]

On 06/12/2014 09:02 PM, Cornelia Huck wrote:
> On Thu, 12 Jun 2014 19:38:40 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 06/12/2014 04:31 PM, Cornelia Huck wrote:
>>> On Thu, 12 Jun 2014 03:03:01 +1000
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> This implements an NMI interface for s390 machine.
>>>>
>>>> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
>>>> nmi_monitor_handler() callback is going to be used for NMI.
>>>>
>>>> Since nmi_monitor_handler()-calling code is platform independent,
>>>> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
>>>> There should not be any change in behaviour as both @cpu_index and
>>>> @cpu_num are global CPU numbers.
>>>>
>>>> Also, s390_cpu_restart() takes care of preforming operations in
>>>> the specific CPU thread so no extra measure is required here either.
>>>>
>>>> Since the only error s390_cpu_restart() can return is ENOSYS, convert
>>>> it to QERR_UNSUPPORTED.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v6:
>>>> * supported NMI interface
>>>>
>>>> v5:
>>>> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>>>>
>>>> v4:
>>>> * s/\<nmi\>/nmi_monitor_handler/
>>>>
>>>> v3:
>>>> * now contains both old code removal and new code insertion, easier to
>>>> track changes
>>>>
>>>> ---
>>>> Is there any good reason to have @cpu_num in addition to @cpu_index?
>>>> Just asking :)
>>>> ---
>>>>  cpus.c                 | 14 --------------
>>>>  hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>>>>  target-s390x/cpu.c     |  1 +
>>>>  3 files changed, 32 insertions(+), 14 deletions(-)
>>>>
> 
>> I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7
>> Please have a look and give it a go - I do not have s390 kernel/images
>> handy. Thanks!
> 
> Gave it a try with both of the machines: Triggering 'nmi' from the
> monitor still triggers the configured on_restart action, so this seems
> to work as well as before.
> 
>>
>> It does not look like we really need a new file for NMI now.
> 
> Yes, I think the v7 code looks fine.

Cool, thanks! I'll wait for a bit till everybody speaks and repost :-)



-- 
Alexey

[Qemu-devel] [PATCH v6 3/4] target-i386: Migrate to new NMI interface

[Alexey Kardashevskiy]

This implements an NMI interface for i386 PC machines.

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:
v6:
* make use of NMI interface

v5:
* make use of NMI interface

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

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

diff --git a/cpus.c b/cpus.c
index f3756b4..f7b25f5 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1469,19 +1469,5 @@ 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
     nmi(monitor_get_cpu_index(), errp);
-#endif
 }
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a48e263..9604831 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -48,11 +48,13 @@
 #include "exec/address-spaces.h"
 #include "hw/acpi/acpi.h"
 #include "cpu.h"
+#include "hw/nmi.h"
 #ifdef CONFIG_XEN
 #  include <xen/hvm/hvm_info_table.h>
 #endif
 
 #define MAX_IDE_BUS 2
+#define TYPE_NMI_X86        "x86-nmi"
 
 static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
@@ -257,6 +259,9 @@ static void pc_init1(MachineState *machine,
     if (pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
+
+    object_property_add_child(OBJECT(machine), "nmi",
+                              object_new(TYPE_NMI_X86), NULL);
 }
 
 static void pc_init_pci(MachineState *machine)
@@ -866,3 +871,40 @@ static void pc_machine_init(void)
 }
 
 machine_init(pc_machine_init);
+
+static void x86_nmi(NMI *n, int cpu_index, Error **errp)
+{
+    CPUState *cs = qemu_get_cpu(cpu_index);
+    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
+    }
+}
+
+static void x86_nmi_class_init(ObjectClass *oc, void *data)
+{
+    NMIClass *nc = NMI_CLASS(oc);
+    nc->nmi_monitor_handler = x86_nmi;
+}
+
+static const TypeInfo x86_nmi_info = {
+    .name          = TYPE_NMI_X86,
+    .parent        = TYPE_OBJECT,
+    .class_init    = x86_nmi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_NMI },
+        { }
+    },
+};
+
+static void x86_nmi_register_types(void)
+{
+    type_register_static(&x86_nmi_info);
+}
+
+type_init(x86_nmi_register_types)
-- 
2.0.0

[Qemu-devel] [PATCH v6 4/4] target-ppc: Add support for new NMI interface

[Alexey Kardashevskiy]

This implements an NMI interface POWERPC SPAPR machine.
This enables an "nmi" HMP/QMP command supported on SPAPR.

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

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v6:
* support NMI interface

v4:
* s/\<nmi\>/nmi_monitor_handler/
* added note about XMON into commit log

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 57e9578..006c37f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -53,6 +53,7 @@
 #include "hw/usb.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
+#include "hw/nmi.h"
 
 #include <libfdt.h>
 
@@ -1476,10 +1477,34 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
     return NULL;
 }
 
+#ifndef CONFIG_USER_ONLY
+static void ppc_cpu_do_nmi_on_cpu(void *arg)
+{
+    CPUState *cs = arg;
+
+    cpu_synchronize_state(cs);
+    ppc_cpu_do_nmi(cs);
+}
+
+static void spapr_nmi(NMI *n, int cpu_index, Error **errp)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, cs);
+    }
+}
+#else
+static void spapr_nmi(NMI *n, int cpu_index, Error **errp)
+{
+}
+#endif
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
+    NMIClass *nc = NMI_CLASS(oc);
 
     mc->name = "pseries";
     mc->desc = "pSeries Logical Partition (PAPR compliant)";
@@ -1493,6 +1518,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->kvm_type = spapr_kvm_type;
 
     fwc->get_dev_path = spapr_get_fw_dev_path;
+    nc->nmi_monitor_handler = spapr_nmi;
 }
 
 static const TypeInfo spapr_machine_info = {
@@ -1501,6 +1527,7 @@ static const TypeInfo spapr_machine_info = {
     .class_init    = spapr_machine_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_FW_PATH_PROVIDER },
+        { TYPE_NMI },
         { }
     },
 };
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 47dc8e6..fe7d602 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -119,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction f,
 int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                                int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
+void ppc_cpu_do_nmi(CPUState *cs);
 extern const struct VMStateDescription vmstate_ppc_cpu;
 #endif
 
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 7dfc52d..a574ba3 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -802,6 +802,14 @@ void ppc_hw_interrupt(CPUPPCState *env)
         }
     }
 }
+
+void ppc_cpu_do_nmi(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
+}
 #endif /* !CONFIG_USER_ONLY */
 
 #if defined(DEBUG_OP)
-- 
2.0.0