Skip to contentSkip to conversation

add inject-nmi qmp command

PATCH Superseded Lai Jiangshan 12 participants 60 messages Mar 7, 2011 – May 3, 2011 KVM

lore
Share & tools
lore
#0 lore raw

v7 cover add inject-nmi qmp command

From: Lai Jiangshan <[email protected]>
Date: Mon, 7 Mar 2011 17:08:46 +0800
Subject: [PATCH 0/2 V7] qemu,qmp: add inject-nmi qmp command

The new qmp command "inject-nmi" is different from the hmp monitor command "nmi". The first one injects an NMI on all CPUs, and the second one injects an NMI to the specified CPU.

The first one(qmp command) simulates the Real hardware NMI button, but I don't want to change the existed hmp monitor command "nmi", so I didn't change it except the handler name. Maybe we will add a qmp command "inject-nmi-cpu" in future, but not now. (it seems it is only requested by me currently)

The qmp command "inject-nmi" is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest. This error and this behavior are described in the comments.

Lai Jiangshan (2):
qemu,qmp: QError: New QERR_UNSUPPORTED
qemu,qmp: add inject-nmi qmp command

 hmp-commands.hx |    2 +-
 monitor.c       |   18 +++++++++++++++++-
 qerror.h        |    3 +++
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.4
#1 lore raw

PATCH v7 1/2 qemu,qmp: QError: New QERR_UNSUPPORTED

From: Lai Jiangshan <[email protected]>
Date: Mon, 7 Mar 2011 17:05:04 +0800
Subject: [PATCH 1/2] qemu,qmp: QError: New QERR_UNSUPPORTED

New QERR_UNSUPPORTED for unsupported commands or requests.

---
 qerror.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
qerror.h +3 -0
index f732d45..43cc87b 100644
@@ -165,6 +165,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
 
+#define QERR_UNSUPPORTED \
+    "{ 'class': 'Unsupported', 'data': {} }"
+
 #define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
     "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'format': %s, 'feature': %s } }"
 
Signature · 1 line
1.7.4
#2 lore raw

PATCH v7 2/2 qemu,qmp: add inject-nmi qmp command

In reply to Lai Jiangshan

From: Lai Jiangshan <[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

---
 hmp-commands.hx |    2 +-
 monitor.c       |   18 +++++++++++++++++-
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)
hmp-commands.hx +1 -1
index 372bef4..8aea56c 100644
@@ -743,7 +743,7 @@ ETEXI
         .args_type  = "cpu_index:i",
         .params     = "cpu",
         .help       = "inject an NMI on the given CPU",
-        .mhandler.cmd = do_inject_nmi,
+        .mhandler.cmd = do_inject_nmi_cpu,
     },
 #endif
 STEXI
monitor.c +17 -1
index 22ae3bb..aebcc0c 100644
@@ -2555,7 +2555,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)
 #endif
 
 #if defined(TARGET_I386)
-static void do_inject_nmi(Monitor *mon, const QDict *qdict)
+static void do_inject_nmi_cpu(Monitor *mon, const QDict *qdict)
 {
     CPUState *env;
     int cpu_index = qdict_get_int(qdict, "cpu_index");
@@ -2566,6 +2566,22 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
             break;
         }
 }
+
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu)
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+
+    return 0;
+}
+#else
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    qerror_report(QERR_UNSUPPORTED);
+    return -1;
+}
 #endif
 
 static void do_info_status_print(Monitor *mon, const QObject *data)
qmp-commands.hx +29 -0
index df40a3d..51f479e 100644
@@ -430,6 +430,35 @@ Example:
 EQMP
 
     {
+        .name       = "inject-nmi",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Inject an NMI on guest.\n"
+                      "Returns \"Unsupported\" error when the guest does"
+                      "not support NMI injection",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi,
+    },
+
+SQMP
+inject-nmi
+----------
+
+Inject an NMI on guest.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "inject-nmi" }
+<- { "return": {} }
+
+Note: inject-nmi is only supported for x86 guest currently, it will
+      returns "Unsupported" error for non-x86 guest.
+
+EQMP
+
+    {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
         .params     = "[-d] [-b] [-i] uri",
Signature · 1 line
1.7.4
#3 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan <[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 5 lines
---
 hmp-commands.hx |    2 +-
 monitor.c       |   18 +++++++++++++++++-
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

We'd like to support it in libvirt and thus want it to be available in QMP, as well as HMP.

Quoted 12 lines
monitor.c +8 -0
@@ -2566,6 +2566,22 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
             break;
         }
 }
+
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu)
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+

monitor.c static void do_inject_nmi(Monitor *mon, const QDict *qdict) · line 2586

Quoted 10 lines
monitor.c +8 -0
+    return 0;
+}
+#else
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    qerror_report(QERR_UNSUPPORTED);
+    return -1;
+}
 #endif
 

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#4 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

[Note cc: Anthony]

Quoted 9 lines · Daniel P. Berrange
Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan <[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 8 lines
Quoted 5 lines
---
 hmp-commands.hx |    2 +-
 monitor.c       |   18 +++++++++++++++++-
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

That would be bad. Anthony, what's holding this back?

Quoted 22 lines

We'd like to support it in libvirt and thus want it to be available in QMP, as well as HMP.

Quoted 19 lines
monitor.c +15 -0
@@ -2566,6 +2566,22 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
             break;
         }
 }
+
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu)
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+
+    return 0;
+}
+#else
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    qerror_report(QERR_UNSUPPORTED);
+    return -1;
Quoted 8 lines

monitor.c static void do_inject_nmi(Monitor *mon, const QDict *qdict) · line 2586

Quoted 3 lines
monitor.c +1 -0
+}
 #endif
 

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

Quoting my own executive summary on this issue:

* Real hardware's NMI button injects all CPUs. This is the primary use case.

* Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case.

Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP.

* No other use cases have been presented.

#5 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 21 lines · On Mon, 04 Apr 2011 14:19:58 +0200 Markus Armbruster <[email protected]> wrote:

[Note cc: Anthony]

Quoted 17 lines · Daniel P. Berrange
Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan <[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 5 lines
---
 hmp-commands.hx |    2 +-
 monitor.c       |   18 +++++++++++++++++-
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

No, we agreed on merging stuff against current QMP.

Quoted 1 line

That would be bad. Anthony, what's holding this back?

I remember Anthony asked for errors descriptions.

Quoted 45 lines
Quoted 23 lines

We'd like to support it in libvirt and thus want it to be available in QMP, as well as HMP.

Quoted 19 lines
monitor.c +15 -0
@@ -2566,6 +2566,22 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
             break;
         }
 }
+
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu)
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+
+    return 0;
+}
+#else
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    qerror_report(QERR_UNSUPPORTED);
+    return -1;
Quoted 8 lines

monitor.c static void do_inject_nmi(Monitor *mon, const QDict *qdict) · line 2586

Quoted 3 lines
monitor.c +1 -0
+}
 #endif
 

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

Quoting my own executive summary on this issue:

* Real hardware's NMI button injects all CPUs. This is the primary use case.

* Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case.

Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP.

* No other use cases have been presented.

#6 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Markus Armbruster
Quoted 13 lines · Markus Armbruster

[Note cc: Anthony]

Quoted 9 lines · Daniel P. Berrange
Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan<[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
---
  hmp-commands.hx |    2 +-
  monitor.c       |   18 +++++++++++++++++-
  qmp-commands.hx |   29 +++++++++++++++++++++++++++++
  3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

That would be bad. Anthony, what's holding this back?

It doesn't pass checkpath.pl.

But I'd also expect this to come through Luiz's QMP tree.

Regards,

Anthony Liguori

#7 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 19 lines · On Mon, 04 Apr 2011 08:09:29 -0500 Anthony Liguori <[email protected]>…
Quoted 18 lines · Markus Armbruster

[Note cc: Anthony]

Quoted 14 lines · Daniel P. Berrange
Quoted 13 lines · Lai Jiangshan

From: Lai Jiangshan<[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

---
  hmp-commands.hx |    2 +-
  monitor.c       |   18 +++++++++++++++++-
  qmp-commands.hx |   29 +++++++++++++++++++++++++++++
  3 files changed, 47 insertions(+), 2 deletions(-)
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

That would be bad. Anthony, what's holding this back?

It doesn't pass checkpath.pl.

But I'd also expect this to come through Luiz's QMP tree.

I had this ready in my tree some time ago, but you commented on that version asking for errors descriptions and other things, so I didn't push it.

But we have to set expectations here. My tree will eventually die, specially wrt new interfaces where I expect you to jump in ASAP. First because you have to be sure a new interface "conforms" to QAPI, and second (and more importantly) because it's time to pass this on to someone else (preferably you).

#8 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Anthony Liguori
Quoted 23 lines · Anthony Liguori
Quoted 13 lines · Markus Armbruster

[Note cc: Anthony]

Quoted 9 lines · Daniel P. Berrange
Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan<[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
---
  hmp-commands.hx |    2 +-
  monitor.c       |   18 +++++++++++++++++-
  qmp-commands.hx |   29 +++++++++++++++++++++++++++++
  3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

That would be bad. Anthony, what's holding this back?

Quoted 8 lines

It doesn't pass checkpath.pl.

But I'd also expect this to come through Luiz's QMP tree.

Regards,

Anthony Liguori

Hi, Anthony,

I cannot find checkpath.pl in the source tree.
And how/where to write errors descriptions? Is the following description suitable?

##
# @inject-nmi:
#
# Inject an NMI on the guest.
#
# Returns: Nothing on success.
# If the guest(non-x86) does not support NMI injection, Unsupported
#
# Since: 0.15.0
##
{ 'command': 'inject-nmi' }

Thanks,
Lai

#9 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 28 lines · Lai Jiangshan
Quoted 27 lines · Anthony Liguori
Quoted 13 lines · Markus Armbruster

[Note cc: Anthony]

Quoted 9 lines · Daniel P. Berrange
Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan<[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
---
  hmp-commands.hx |    2 +-
  monitor.c       |   18 +++++++++++++++++-
  qmp-commands.hx |   29 +++++++++++++++++++++++++++++
  3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

That would be bad. Anthony, what's holding this back?

It doesn't pass checkpath.pl.

But I'd also expect this to come through Luiz's QMP tree.

Quoted 8 lines
Quoted 4 lines

Regards,

Anthony Liguori

Hi, Anthony,

I cannot find checkpath.pl in the source tree.

Sorry, I found it in the mainline tree.

Quoted 23 lines

And how/where to write errors descriptions? Is the following description suitable?

##
# @inject-nmi:
#
# Inject an NMI on the guest.
#
# Returns: Nothing on success.
# If the guest(non-x86) does not support NMI injection, Unsupported
#
# Since: 0.15.0
##
{ 'command': 'inject-nmi' }

Thanks,
Lai
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw

Re: [PATCH 2/2 V7] qemu, qmp: add inject-nmi qmp command

In reply to Lai Jiangshan
Quoted 28 lines · On Wed, 20 Apr 2011 09:53:56 +0800 Lai Jiangshan <[email protected]> wrote:
Quoted 27 lines · Anthony Liguori
Quoted 13 lines · Markus Armbruster

[Note cc: Anthony]

Quoted 9 lines · Daniel P. Berrange
Quoted 8 lines · Lai Jiangshan

From: Lai Jiangshan<[email protected]>
Date: Mon, 7 Mar 2011 17:05:15 +0800
Subject: [PATCH 2/2] qemu,qmp: add inject-nmi qmp command

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
---
  hmp-commands.hx |    2 +-
  monitor.c       |   18 +++++++++++++++++-
  qmp-commands.hx |   29 +++++++++++++++++++++++++++++
  3 files changed, 47 insertions(+), 2 deletions(-)

Does anyone have any feedback on this addition, or are all new QMP patch proposals blocked pending Anthony's QAPI work ?

That would be bad. Anthony, what's holding this back?

It doesn't pass checkpath.pl.

But I'd also expect this to come through Luiz's QMP tree.

Quoted 8 lines
Quoted 4 lines

Regards,

Anthony Liguori

Hi, Anthony,

I cannot find checkpath.pl in the source tree.

It's ./scripts/checkpatch.pl

Quoted 19 lines

And how/where to write errors descriptions? Is the following description suitable?

##
# @inject-nmi:
#
# Inject an NMI on the guest.
#
# Returns: Nothing on success.
# If the guest(non-x86) does not support NMI injection, Unsupported
#
# Since: 0.15.0
##
{ 'command': 'inject-nmi' }

Thanks,
Lai

#11 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Daniel P. Berrange
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

Signature · 1 line
error compiling committee.c: too many arguments to function
#12 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 9 lines · Avi Kivity
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

It either needs to be removed from HMP or added to QMP. HMP shouldn't have more features than QMP (even if those features are non-sensible).

Regards,

Anthony Liguori

#13 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 12 lines · On Mon, 04 Apr 2011 08:05:48 -0500 Anthony Liguori <[email protected]>…
Quoted 11 lines · Avi Kivity
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

Quoted 6 lines
Quoted 3 lines

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

It either needs to be removed from HMP or added to QMP. HMP shouldn't have more features than QMP (even if those features are non-sensible).

Is anyone against changing HMP behavior to send it to all CPUs?

#14 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 14 lines · Luiz Capitulino
Quoted 11 lines · On Mon, 04 Apr 2011 08:05:48 -0500 Anthony Liguori<[email protected]>…
Quoted 10 lines · Avi Kivity
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

It either needs to be removed from HMP or added to QMP. HMP shouldn't have more features than QMP (even if those features are non-sensible).

Is anyone against changing HMP behavior to send it to all CPUs?

Makes sense to me.

Regards,

Anthony Liguori

#15 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 15 lines · On Wed, 06 Apr 2011 13:03:37 -0500 Anthony Liguori <[email protected]>…
Quoted 14 lines · Luiz Capitulino
Quoted 11 lines · On Mon, 04 Apr 2011 08:05:48 -0500 Anthony Liguori<[email protected]>…
Quoted 10 lines · Avi Kivity
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

It either needs to be removed from HMP or added to QMP. HMP shouldn't have more features than QMP (even if those features are non-sensible).

Is anyone against changing HMP behavior to send it to all CPUs?

Makes sense to me.

So, Lai, in order to get this merged could you please do the following:

1. Address checkpath.pl errors

2. Change the HMP to use this implementation, which send the NMI to all CPUs

3. Any other review comments I might be missing :)

Quoted 5 lines

Regards,

Anthony Liguori

#16 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 25 lines · Luiz Capitulino
Quoted 22 lines · On Wed, 06 Apr 2011 13:03:37 -0500 Anthony Liguori <[email protected]>…
Quoted 20 lines · Luiz Capitulino
Quoted 11 lines · On Mon, 04 Apr 2011 08:05:48 -0500 Anthony Liguori<[email protected]>…
Quoted 10 lines · Avi Kivity
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

Quoted 5 lines
Quoted 3 lines

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

It either needs to be removed from HMP or added to QMP. HMP shouldn't have more features than QMP (even if those features are non-sensible).

Is anyone against changing HMP behavior to send it to all CPUs?

Quoted 8 lines
Quoted 1 line

Makes sense to me.

So, Lai, in order to get this merged could you please do the following:

1. Address checkpath.pl errors

2. Change the HMP to use this implementation, which send the NMI to all CPUs

HMP is currently x86-only, thus it's probably OK to model it after some PC feature (though I don't know if there aren't NMI buttons with BP-only wirings). But will the consolidate version be defined for all architectures? We should avoid "exporting" x86-specific assumptions.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#17 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 32 lines · On Wed, 06 Apr 2011 20:17:47 +0200 Jan Kiszka <[email protected]> wrote:
Quoted 31 lines · Luiz Capitulino
Quoted 15 lines · On Wed, 06 Apr 2011 13:03:37 -0500 Anthony Liguori <[email protected]>…
Quoted 14 lines · Luiz Capitulino
Quoted 11 lines · On Mon, 04 Apr 2011 08:05:48 -0500 Anthony Liguori<[email protected]>…
Quoted 10 lines · Avi Kivity
Quoted 4 lines · Daniel P. Berrange

Interesting that with HMP you need to specify a single CPU index, but with QMP it is injecting to all CPUs at once. Is there any compelling reason why we'd ever need the ability to only inject to a single CPU from an app developer POV ?

When a PC has an NMI button, it is (I presume) connected to all CPUs' LINT1 pin, which is often configured as an NMI input. So the all-cpu variant corresponds to real hardware, while the single-cpu variant doesn't.

Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

wrt the app developer POV, the only use I'm aware of is that you can configure Windows to dump core when the NMI button is pressed and thus debug driver problems. It's likely more reliable when sent to all cpus.

It either needs to be removed from HMP or added to QMP. HMP shouldn't have more features than QMP (even if those features are non-sensible).

Is anyone against changing HMP behavior to send it to all CPUs?

Makes sense to me.

So, Lai, in order to get this merged could you please do the following:

1. Address checkpath.pl errors

Quoted 7 lines
Quoted 2 lines

2. Change the HMP to use this implementation, which send the NMI to all CPUs

HMP is currently x86-only, thus it's probably OK to model it after some PC feature (though I don't know if there aren't NMI buttons with BP-only wirings). But will the consolidate version be defined for all architectures? We should avoid "exporting" x86-specific assumptions.

Right, but honestly speaking, I don't know how this works for other arches.

So, the best thing to do is to have a general design that can be used by any architecture. Of course that we can also add a new command later if needed.

#18 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 7 lines · Luiz Capitulino
Quoted 4 lines · On Wed, 06 Apr 2011 20:17:47 +0200 Jan Kiszka <[email protected]> wrote:

HMP is currently x86-only, thus it's probably OK to model it after some PC feature (though I don't know if there aren't NMI buttons with BP-only wirings). But will the consolidate version be defined for all architectures? We should avoid "exporting" x86-specific assumptions.

Quoted 5 lines

Right, but honestly speaking, I don't know how this works for other arches.

So, the best thing to do is to have a general design that can be used by any architecture. Of course that we can also add a new command later if needed.

Well, I'm not sure "send a random interrupt to the core" makes much sense for ARM... So what are we actually trying to model here? Some sort of way to do "press a front panel switch" via remote monitor protocol? I guess you could have an API for boards to register any switches they had...

-- PMM

#19 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 10 lines · Peter Maydell
Quoted 5 lines

Right, but honestly speaking, I don't know how this works for other arches.

So, the best thing to do is to have a general design that can be used by any architecture. Of course that we can also add a new command later if needed.

Well, I'm not sure "send a random interrupt to the core" makes much sense for ARM... So what are we actually trying to model here? Some sort of way to do "press a front panel switch" via remote monitor protocol? I guess you could have an API for boards to register any switches they had...

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too. The real semantics are basically force guest crash dump.

Regards,

Anthony Liguori

Quoted 2 lines

-- PMM

#20 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 12 lines · Anthony Liguori
Quoted 10 lines · Peter Maydell
Quoted 5 lines

Right, but honestly speaking, I don't know how this works for other arches.

So, the best thing to do is to have a general design that can be used by any architecture. Of course that we can also add a new command later if needed.

Well, I'm not sure "send a random interrupt to the core" makes much sense for ARM... So what are we actually trying to model here? Some sort of way to do "press a front panel switch" via remote monitor protocol? I guess you could have an API for boards to register any switches they had...

Quoted 8 lines

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too. The real semantics are basically force guest crash dump.

Right, it's a debugging tool. And that does not necessarily means it has to match real hardware. I could imagine scenarios where it could be helpful to direct the NMI to a specific core, e.g. in AMP setups if only one OS ran wild.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#21 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Anthony Liguori
Quoted 8 lines · Anthony Liguori

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too.  The real semantics are basically force guest crash dump.

Ah, right. (There isn't really an equivalent to this on ARM since we don't have a real NMI equivalent. So any implementation for ARM qemu would be board dependent since you could wire a watchdog up to any interrupt.)

Should we try to pick a command name that says what it's supposed to do rather than how it happens to be implemented on x86 ?

-- PMM

#22 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 9 lines · Peter Maydell
Quoted 8 lines · Anthony Liguori

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too. The real semantics are basically force guest crash dump.

Quoted 7 lines

Ah, right. (There isn't really an equivalent to this on ARM since we don't have a real NMI equivalent. So any implementation for ARM qemu would be board dependent since you could wire a watchdog up to any interrupt.)

Should we try to pick a command name that says what it's supposed to do rather than how it happens to be implemented on x86 ?

Yup, I was thinking the same thing after I sent the note above. If we call it 'force-crash-dump', we can implement it as an NMI on target-i386 and potentially as something else on a different target.

Regards,

Anthony Liguori

Quoted 2 lines

-- PMM

#23 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 15 lines · Anthony Liguori
Quoted 14 lines · Peter Maydell
Quoted 8 lines · Anthony Liguori

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too. The real semantics are basically force guest crash dump.

Ah, right. (There isn't really an equivalent to this on ARM since we don't have a real NMI equivalent. So any implementation for ARM qemu would be board dependent since you could wire a watchdog up to any interrupt.)

Quoted 7 lines
Quoted 2 lines

Should we try to pick a command name that says what it's supposed to do rather than how it happens to be implemented on x86 ?

Yup, I was thinking the same thing after I sent the note above. If we call it 'force-crash-dump', we can implement it as an NMI on target-i386 and potentially as something else on a different target.

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives. Lets give meaningful name, like inject-nmi, for nmi injection command.

--
Gleb.

#24 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 23 lines · Gleb Natapov
Quoted 15 lines · Anthony Liguori
Quoted 14 lines · Peter Maydell
Quoted 8 lines · Anthony Liguori

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too.  The real semantics are basically force guest crash dump.

Ah, right. (There isn't really an equivalent to this on ARM since we don't have a real NMI equivalent. So any implementation for ARM qemu would be board dependent since you could wire a watchdog up to any interrupt.)

Quoted 7 lines
Quoted 2 lines

Should we try to pick a command name that says what it's supposed to do rather than how it happens to be implemented on x86 ?

Yup, I was thinking the same thing after I sent the note above.  If we call it 'force-crash-dump', we can implement it as an NMI on target-i386 and potentially as something else on a different target.

Quoted 5 lines

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives. Lets give meaningful name, like inject-nmi, for nmi injection command.

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Some kind of IRQ registration would be needed for this to work without lots of changes.

#25 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 34 lines · Blue Swirl
Quoted 23 lines · Gleb Natapov
Quoted 15 lines · Anthony Liguori
Quoted 14 lines · Peter Maydell
Quoted 8 lines · Anthony Liguori

http://publib.boulder.ibm.com/infocenter/lnxinfo/v3r0m0/index.jsp?topic=/liaai/crashdump/liaaicrashdumpnmiipmi.htm

If an OS is totally hosed (spinning with interrupts disabled), and NMI can be used to generate a crash dump.

It's a debug feature and modelling it exactly the way we are probably makes sense for other architectures too.  The real semantics are basically force guest crash dump.

Ah, right. (There isn't really an equivalent to this on ARM since we don't have a real NMI equivalent. So any implementation for ARM qemu would be board dependent since you could wire a watchdog up to any interrupt.)

Quoted 7 lines
Quoted 2 lines

Should we try to pick a command name that says what it's supposed to do rather than how it happens to be implemented on x86 ?

Yup, I was thinking the same thing after I sent the note above.  If we call it 'force-crash-dump', we can implement it as an NMI on target-i386 and potentially as something else on a different target.

Quoted 5 lines

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives. Lets give meaningful name, like inject-nmi, for nmi injection command.

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

Quoted 5 lines

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Some kind of IRQ registration would be needed for this to work without lots of changes.

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

--
Gleb.

#26 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 7 lines · Gleb Natapov
Quoted 6 lines · Blue Swirl

On Thu, Apr 7, 2011 at 9:51 PM, Gleb Natapov<[email protected]> wrote:

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

Quoted 7 lines
Quoted 5 lines

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Some kind of IRQ registration would be needed for this to work without lots of changes.

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Regards,

Anthony Liguori

Quoted 3 lines

--
Gleb.

#27 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 11 lines · Anthony Liguori
Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Blue Swirl

On Thu, Apr 7, 2011 at 9:51 PM, Gleb Natapov<[email protected]> wrote:

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

Some kind of IRQ registration would be needed for this to work without lots of changes.

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Well, this is not intended to be used by regular users directly and management can provide nicer interface for issuing NMI. But really, my point is that NMI actually generates guest core dump in such rare cases (only preconfigured Windows guests) that it doesn't warrant to name command as such. Management is in much better position to implement functionality with such name since it knows what type of guest it runs and can tell agent to configure guest accordingly.

--
Gleb.

#28 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 21 lines · Gleb Natapov
Quoted 11 lines · Anthony Liguori
Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Blue Swirl

On Thu, Apr 7, 2011 at 9:51 PM, Gleb Natapov<[email protected]>  wrote:

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

Some kind of IRQ registration would be needed for this to work without lots of changes.

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 7 lines

Well, this is not intended to be used by regular users directly and management can provide nicer interface for issuing NMI. But really, my point is that NMI actually generates guest core dump in such rare cases (only preconfigured Windows guests) that it doesn't warrant to name command as such. Management is in much better position to implement functionality with such name since it knows what type of guest it runs and can tell agent to configure guest accordingly.

Does the management need to know about each and every debugging oriented interface? For example, "info regs", "info mem", "info irq" and tracepoints?

I think giving IRQs symbolic names could solve some other problems as well. Maybe it should be possible to connect IRQs in a configuration file and even with command line:
-device port90,irqid=p90out -device pckbd,irqid=kbdout -device and,in=p90out,in=kbdout,out=sreset device system_reset,in=sreset

or

-device and,in=/i44FX-pcihost/PIIX3/i8042/out1,in=/i44FX-pcihost/PIIX3/p90/out1,out=/QEMU/system_reset

#29 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 32 lines · Blue Swirl
Quoted 20 lines · Gleb Natapov
Quoted 11 lines · Anthony Liguori
Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Blue Swirl

On Thu, Apr 7, 2011 at 9:51 PM, Gleb Natapov<[email protected]> wrote:

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

Some kind of IRQ registration would be needed for this to work without lots of changes.

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 7 lines

Well, this is not intended to be used by regular users directly and management can provide nicer interface for issuing NMI. But really, my point is that NMI actually generates guest core dump in such rare cases (only preconfigured Windows guests) that it doesn't warrant to name command as such. Management is in much better position to implement functionality with such name since it knows what type of guest it runs and can tell agent to configure guest accordingly.

Does the management need to know about each and every debugging oriented interface? For example, "info regs", "info mem", "info irq" and tracepoints?

Quoted 5 lines

I think giving IRQs symbolic names could solve some other problems as well. Maybe it should be possible to connect IRQs in a configuration file and even with command line:
-device port90,irqid=p90out -device pckbd,irqid=kbdout -device and,in=p90out,in=kbdout,out=sreset device system_reset,in=sreset

You really want devices to have properties and for the device properties to be discoverable. For instance:

struct DeviceInfo

{
     .name = "and",
     .properties = {
          DEFINE_IRQ_IN(AndDevice, in[0]),
          DEFINE_IRQ_IN(AndDevice, in[1]),
          DEFINE_IRQ_OUT(AndDevice, out),
     },
};

And then you can do:

-device port90,id=port90 -device pckbd,id=pckbd \
-device and,in[0]=port90.out,in[1]=pckbd.out,id=reset_and \
-device system_reset.in=reset_and

Regards,

Anthony Liguori

Quoted 4 lines

or

-device and,in=/i44FX-pcihost/PIIX3/i8042/out1,in=/i44FX-pcihost/PIIX3/p90/out1,out=/QEMU/system_reset

#30 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 63 lines · Anthony Liguori
Quoted 41 lines · Blue Swirl
Quoted 27 lines · Gleb Natapov
Quoted 18 lines · Anthony Liguori
Quoted 16 lines · Gleb Natapov
Quoted 13 lines · Blue Swirl

On Thu, Apr 7, 2011 at 9:51 PM, Gleb Natapov<[email protected]> wrote:

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Some kind of IRQ registration would be needed for this to work without lots of changes.

Quoted 7 lines
Quoted 2 lines

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 7 lines

Well, this is not intended to be used by regular users directly and management can provide nicer interface for issuing NMI. But really, my point is that NMI actually generates guest core dump in such rare cases (only preconfigured Windows guests) that it doesn't warrant to name command as such. Management is in much better position to implement functionality with such name since it knows what type of guest it runs and can tell agent to configure guest accordingly.

Does the management need to know about each and every debugging oriented interface? For example, "info regs",  "info mem", "info irq" and tracepoints?

Quoted 5 lines

I think giving IRQs symbolic names could solve some other problems as well. Maybe it should be possible to connect IRQs in a configuration file and even with command line:
-device port90,irqid=p90out -device pckbd,irqid=kbdout -device and,in=p90out,in=kbdout,out=sreset device system_reset,in=sreset

You really want devices to have properties and for the device properties to be discoverable.  For instance:

struct DeviceInfo
{
    .name = "and",
    .properties = {
         DEFINE_IRQ_IN(AndDevice, in[0]),
         DEFINE_IRQ_IN(AndDevice, in[1]),
         DEFINE_IRQ_OUT(AndDevice, out),
    },
};

And then you can do:

Review

Quoted 3 lines
(quoted diff) +0 -3
-device port90,id=port90 -device pckbd,id=pckbd \
-device and,in[0]=port90.out,in[1]=pckbd.out,id=reset_and \
-device system_reset.in=reset_and

Exactly. Given a NAND device, we could construct entire machines from CLI or for example co-simulate SoCs with FPGAs using cells based on the net lists.

#31 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Blue Swirl
Quoted 30 lines · Blue Swirl
Quoted 21 lines · Gleb Natapov
Quoted 11 lines · Anthony Liguori
Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Blue Swirl

On Thu, Apr 7, 2011 at 9:51 PM, Gleb Natapov<[email protected]>  wrote:

I'd prefer something more generic like these:
raise /apic@fee00000:l1int
lower /i44FX-pcihost/[email protected]/pinD

The clumsier syntax shouldn't be a problem, since this would be a system developer tool.

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

Some kind of IRQ registration would be needed for this to work without lots of changes.

True. The ability to trigger any interrupt line is very useful for debugging. I often re-implement it during debug.

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 7 lines

Well, this is not intended to be used by regular users directly and management can provide nicer interface for issuing NMI. But really, my point is that NMI actually generates guest core dump in such rare cases (only preconfigured Windows guests) that it doesn't warrant to name command as such. Management is in much better position to implement functionality with such name since it knows what type of guest it runs and can tell agent to configure guest accordingly.

Quoted 3 lines

Does the management need to know about each and every debugging oriented interface? For example, "info regs", "info mem", "info irq" and tracepoints?

Linux uses NMI for performance tracing, profiling, watchdog etc. so in practice, NMI is very similar to the other IRQs. I.e. highly guest specific and depending on what's wired up to it. Injecting NMI to all CPUs at once does not make any sense for those Linux guests.

For Windows crash dumps, I think it makes sense to have a "button wired to NMI" device, rather than inject-nmi directly, but I can see that inject-nmi solves the intended problem quite neatly.

For Linux crash dumps, for example, there are other key combinations, as well as watchdog devices, that can be used to trigger them. A virtual "button wired to GPIO/PCI-IRQ/etc." device might be quite handy for debugging Linux guests, and would fit comfortably in a management interface.

-- Jamie

#32 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Anthony Liguori
Quoted 2 lines · Anthony Liguori

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

generate-crash-dump is a wrong name for something that doesn't generate a crash dump (the guest may not be configured for it, or it may fail to work). I'd expect that to be host-side functionality.

Signature · 1 line
error compiling committee.c: too many arguments to function
#33 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 8 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

Agree.

Quoted 3 lines

generate-crash-dump is a wrong name for something that doesn't generate a crash dump (the guest may not be configured for it, or it may fail to work).

Or the OS uses the NMI button for something else.

Quoted 1 line

I'd expect that to be host-side functionality.

#34 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 4 lines · Markus Armbruster
Quoted 2 lines · Avi Kivity
Quoted 1 line · Anthony Liguori
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

#35 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 9 lines · Blue Swirl
Quoted 8 lines · Markus Armbruster
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Yes. Perhaps limited to the human monitor.

Signature · 1 line
error compiling committee.c: too many arguments to function
#36 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 15 lines · Avi Kivity
Quoted 14 lines · Blue Swirl
Quoted 11 lines · Markus Armbruster
Quoted 8 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

 And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

 nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

 Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Yes.  Perhaps limited to the human monitor.

I'd limit all debugging commands (including NMI) to the human monitor.

#37 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 18 lines · On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl <[email protected]> wrote:
Quoted 17 lines · Avi Kivity
Quoted 16 lines · Blue Swirl
Quoted 12 lines · Markus Armbruster
Quoted 8 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

 And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

 nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

 Agree.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Yes.  Perhaps limited to the human monitor.

I'd limit all debugging commands (including NMI) to the human monitor.

Why?

#38 lore raw

Re: [PATCH 2/2 V7] qemu, qmp: add inject-nmi qmp command

Quoted 25 lines · Luiz Capitulino
Quoted 22 lines · On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl <[email protected]> wrote:
Quoted 21 lines · Avi Kivity
Quoted 19 lines · Blue Swirl
Quoted 4 lines · Markus Armbruster
Quoted 2 lines · Avi Kivity
Quoted 1 line · Anthony Liguori
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

 And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

 nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

 Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Yes.  Perhaps limited to the human monitor.

I'd limit all debugging commands (including NMI) to the human monitor.

Why?

Do they have any real use in production environment? Also, we should have the freedom to change the debugging facilities (for example, to improve some internal implementation) as we want without regard to compatibility to previous versions.

#39 lore raw

Re: [PATCH 2/2 V7] qemu, qmp: add inject-nmi qmp command

Quoted 28 lines · Blue Swirl
Quoted 27 lines · Luiz Capitulino
Quoted 24 lines · On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl <[email protected]> wrote:
Quoted 22 lines · Avi Kivity
Quoted 11 lines · Blue Swirl
Quoted 8 lines · Markus Armbruster
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

 And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

 nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

 Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Yes.  Perhaps limited to the human monitor.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I'd limit all debugging commands (including NMI) to the human monitor.

Why?

Do they have any real use in production environment? Also, we should have the freedom to change the debugging facilities (for example, to improve some internal implementation) as we want without regard to compatibility to previous versions.

For what it's worth, Lai (original poster) has been trying for many months to get inject-nmi into QMP, and I suspect he has a really good reason for his super-human persistence.

#40 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Blue Swirl
Quoted 28 lines · Blue Swirl
Quoted 27 lines · Luiz Capitulino
Quoted 24 lines · On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl <[email protected]> wrote:
Quoted 22 lines · Avi Kivity
Quoted 11 lines · Blue Swirl
Quoted 8 lines · Markus Armbruster
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

 And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

 nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

 Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Yes.  Perhaps limited to the human monitor.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I'd limit all debugging commands (including NMI) to the human monitor.

Why?

Do they have any real use in production environment? Also, we should have the freedom to change the debugging facilities (for example, to improve some internal implementation) as we want without regard to compatibility to previous versions.

We have users of libvirt requesting that we add an API for triggering a NMI. They want this for support in a production environment, to be able to initiate Windows crash dumps. We really don't want to have to use HMP passthrough for this, instead of a proper QMP command.

More generally I don't want to see stuff in HMP, that isn't in the QMP. We already have far too much that we have to do via HMP passthrough in libvirt due to lack of QMP commands, to the extent that we might as well have just ignored QMP and continued with HMP for everything.

If we want the flexibility to change the debugging commands between releases then we should come up with a plan to do this within the scope of QMP, not restrict them to HMP only.

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#41 lore raw

Re: [PATCH 2/2 V7] qemu, qmp: add inject-nmi qmp command

Quoted 38 lines · Daniel P. Berrange
Quoted 28 lines · Blue Swirl
Quoted 27 lines · Luiz Capitulino
Quoted 24 lines · On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl <[email protected]> wrote:
Quoted 22 lines · Avi Kivity
Quoted 11 lines · Blue Swirl
Quoted 8 lines · Markus Armbruster
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Anthony Liguori

 And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

 nmi is a fine name for something that corresponds to a real-life nmi button (often labeled "NMI").

 Agree.

We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well.

Yes.  Perhaps limited to the human monitor.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I'd limit all debugging commands (including NMI) to the human monitor.

Why?

Do they have any real use in production environment? Also, we should have the freedom to change the debugging facilities (for example, to improve some internal implementation) as we want without regard to compatibility to previous versions.

Quoted 4 lines

We have users of libvirt requesting that we add an API for triggering a NMI. They want this for support in a production environment, to be able to initiate Windows crash dumps.  We really don't want to have to use HMP passthrough for this, instead of a proper QMP command.

OK, maybe I shouldn't be very proud of my imagination.

Quoted 8 lines

More generally I don't want to see stuff in HMP, that isn't in the QMP. We already have far too much that we have to do via HMP passthrough in libvirt due to lack of QMP commands, to the extent that we might as well have just ignored QMP and continued with HMP for everything.

If we want the flexibility to change the debugging commands between releases then we should come up with a plan to do this within the scope of QMP, not restrict them to HMP only.

If there is a need for the debugging facilities, full QMP support and some level of compatibility is fine.

#42 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Gleb Natapov
Quoted 4 lines · Gleb Natapov

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives.

And for all we know, a guest can respond to an ACPI poweroff event by tweeting the star spangled banner but we still call the corresponding QMP command system_poweroff.

Regards,

Anthony Liguori

#43 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

Quoted 10 lines · Anthony Liguori
Quoted 4 lines · Gleb Natapov

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives.

And for all we know, a guest can respond to an ACPI poweroff event by tweeting the star spangled banner but we still call the corresponding QMP command system_poweroff.

Correct :) But at least system_poweroff implements ACPI poweroff as defined by ACPI spec. NMI is not designed as core dump event and is not used as such by majority of the guests.

--
Gleb.

#44 lore raw

Re: [PATCH 2/2 V7] qemu, qmp: add inject-nmi qmp command

Quoted 7 lines · Gleb Natapov
Quoted 6 lines · Anthony Liguori
Quoted 4 lines · Gleb Natapov

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives.

Quoted 7 lines
Quoted 4 lines

And for all we know, a guest can respond to an ACPI poweroff event by tweeting the star spangled banner but we still call the corresponding QMP command system_poweroff.

Correct :) But at least system_poweroff implements ACPI poweroff as defined by ACPI spec. NMI is not designed as core dump event and is not used as such by majority of the guests.

Imho acpi_poweroff or poweroff_button would have been a clearer name. Or even 'sendkey poweroff' - it's just a button someone on the keyboard on a lot of systems anyway. Next to the email button and what looks, on my laptop, like the play-a-tune button :-)

I put system_poweroff into some QEMU-controlling scripts once, and was disappointed when several guests ignored it.

But it's done now.

-- Jamie

#45 lore raw

Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command

In reply to Anthony Liguori
Quoted 8 lines · Anthony Liguori
Quoted 4 lines · Gleb Natapov

NMI does not have to generate crash dump on every guest we support. Actually even for windows guest it does not generate one without tweaking registry. For all I know there is a guest that checks mail when NMI arrives.

And for all we know, a guest can respond to an ACPI poweroff event by tweeting the star spangled banner but we still call the corresponding QMP command system_poweroff.

A better name for it would be system_power_button. While guests that blast away national anthems when the button is pressed are rare (no doubt to the high localization costs), it's possible to configure an OS to go into S3 sleep when the button is pressed (I used to do it when I had a desktop at home).

Signature · 1 line
error compiling committee.c: too many arguments to function
#46 lore raw

RFC PATCH v8 1/3 QError: Introduce QERR_UNSUPPORTED

In reply to Luiz Capitulino

New QERR_UNSUPPORTED for unsupported commands or requests.

---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)
qerror.c +4 -0
index c76257f..bafe520 100644
@@ -213,6 +213,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Cannot set union '%(name)' of type '%(type)' to value '%(new-value)' because it already is set to value '%(value)'"
     },
     {
+        .error_fmt = QERR_UNSUPPORTED,
+        .desc      = "Unsupported: %(detail)",
+    },
+    {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },
qerror.h +3 -0
index 1f98be1..01ec87d 100644
@@ -193,6 +193,9 @@ void qerror_set_desc(QError *qerr, const char *fmt);
 #define QERR_UNION_MULTIPLE_ENTRIES \
     "{ 'class': 'UnionMultipleEntries', 'data': { 'name': %s, 'type': %s, 'value': %s, 'new-value': %s } }"
 
+#define QERR_UNSUPPORTED \
+    "{ 'class': 'Unsupported', 'data': { 'detail': %s } }"
+
 #define QERR_VNC_SERVER_FAILED \
     "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
 
Signature · 1 line
1.7.4
#47 lore raw

v8 cover QAPI: add inject-nmi qmp command

In reply to Luiz Capitulino

These patches are applied for "http://repo.or.cz/r/qemu/aliguori.git glib".

These patches add QAPI inject-nmi. They are passed checkpatch.pl and the build.

But the result qemu executable file is not tested, because the result qemu of "http://repo.or.cz/r/qemu/aliguori.git glib" can't work in my box.

Lai Jiangshan (3):
QError: Introduce QERR_UNSUPPORTED
qapi,nmi: add inject-nmi qmp command
qapi-hmp: Convert HMP nmi to use QMP

 hmp-commands.hx  |   18 ++++++++----------
 hmp.c            |   12 ++++++++++++
 hmp.h            |    1 +
 monitor.c        |   14 --------------
 qapi-schema.json |   12 ++++++++++++
 qerror.c         |    4 ++++
 qerror.h         |    3 +++
 qmp.c            |   17 +++++++++++++++++
 8 files changed, 57 insertions(+), 24 deletions(-)
Signature · 1 line
1.7.4
#48 lore raw

Re: [RFC PATCH 0/3 V8] QAPI: add inject-nmi qmp command

Hi, Anthony Liguori

Any suggestion?

Although all command line interfaces will be converted to to use QMP interfaces in 0.16, I hope inject-nmi come into QAPI earlier, 0.15.

Thanks,
Lai

#49 lore raw

Re: [RFC PATCH 0/3 V8] QAPI: add inject-nmi qmp command

Quoted 7 lines · On Thu, 21 Apr 2011 11:23:54 +0800 Lai Jiangshan <[email protected]> wrote:

Hi, Anthony Liguori

Any suggestion?

Although all command line interfaces will be converted to to use QMP interfaces in 0.16, I hope inject-nmi come into QAPI earlier, 0.15.

I don't know what Anthony thinks about adding new commands like this one that early to the new QMP interface, but adding them to current QMP will certainly cause less code churn on your side. That's what I'd recommend for now.

#50 lore raw

Re: [Qemu-devel] [RFC PATCH 0/3 V8] QAPI: add inject-nmi qmp command

Quoted 6 lines · Luiz Capitulino
Quoted 3 lines · On Thu, 21 Apr 2011 11:23:54 +0800 Lai Jiangshan<[email protected]> wrote:

Hi, Anthony Liguori

Quoted 8 lines
Quoted 4 lines

Any suggestion?

Although all command line interfaces will be converted to to use QMP interfaces in 0.16, I hope inject-nmi come into QAPI earlier, 0.15.

I don't know what Anthony thinks about adding new commands like this one that early to the new QMP interface, but adding them to current QMP will certainly cause less code churn on your side. That's what I'd recommend for now.

Yeah, sorry, this whole series has been confused in the QAPI discussion.

I did not intend for QAPI to be disruptive to current development.

As far as I can tell, the last series that was posted (before the QAPI post) still had checkpatch.pl issues (scripts/checkpatch.pl btw) and we had agreed that once that was resolved, it would come in through Luiz's tree.

Regards,

Anthony Liguori

#51 lore raw

Re: [Qemu-devel] [RFC PATCH 0/3 V8] QAPI: add inject-nmi qmp command

Quoted 16 lines · Anthony Liguori
Quoted 6 lines · Luiz Capitulino
Quoted 3 lines · On Thu, 21 Apr 2011 11:23:54 +0800 Lai Jiangshan<[email protected]> wrote:

Hi, Anthony Liguori

Quoted 8 lines
Quoted 4 lines

Any suggestion?

Although all command line interfaces will be converted to to use QMP interfaces in 0.16, I hope inject-nmi come into QAPI earlier, 0.15.

I don't know what Anthony thinks about adding new commands like this one that early to the new QMP interface, but adding them to current QMP will certainly cause less code churn on your side. That's what I'd recommend for now.

Quoted 6 lines

Yeah, sorry, this whole series has been confused in the QAPI discussion.

I did not intend for QAPI to be disruptive to current development.

As far as I can tell, the last series that was posted (before the QAPI post) still had checkpatch.pl issues (scripts/checkpatch.pl btw) and we had agreed that once that was resolved, it would come in through Luiz's tree.

Sorry, I didn't caught the meaning.
Fix checkpatch.pl issues of V7 Patch, and sent it again?

Thanks,
Lai

#52 lore raw

Re: [Qemu-devel] [RFC PATCH 0/3 V8] QAPI: add inject-nmi qmp command

Quoted 19 lines · On Wed, 27 Apr 2011 09:54:34 +0800 Lai Jiangshan <[email protected]> wrote:
Quoted 18 lines · Anthony Liguori
Quoted 6 lines · Luiz Capitulino
Quoted 3 lines · On Thu, 21 Apr 2011 11:23:54 +0800 Lai Jiangshan<[email protected]> wrote:

Hi, Anthony Liguori

Quoted 8 lines
Quoted 4 lines

Any suggestion?

Although all command line interfaces will be converted to to use QMP interfaces in 0.16, I hope inject-nmi come into QAPI earlier, 0.15.

I don't know what Anthony thinks about adding new commands like this one that early to the new QMP interface, but adding them to current QMP will certainly cause less code churn on your side. That's what I'd recommend for now.

Yeah, sorry, this whole series has been confused in the QAPI discussion.

Quoted 7 lines
Quoted 4 lines

I did not intend for QAPI to be disruptive to current development.

As far as I can tell, the last series that was posted (before the QAPI post) still had checkpatch.pl issues (scripts/checkpatch.pl btw) and we had agreed that once that was resolved, it would come in through Luiz's tree.

Sorry, I didn't caught the meaning.
Fix checkpatch.pl issues of V7 Patch, and sent it again?

Yes, my recommendation for your series is:

1. Address checkpatch.pl errors

2. Change the HMP to use your implementation, which send the NMI to all CPUs

3. Any other _code_ review comments I might be missing

#53 lore raw

v9 cover hmp,qmp: add inject-nmi

Adds new QERR_UNSUPPORTED, converts "nmi" to "inject-nmi" and make it supports qmp.

Lai Jiangshan (2):
qemu,qmp: QError: New QERR_UNSUPPORTED qmp,inject-nmi: convert do_inject_nmi() to QObject

 hmp-commands.hx |   21 +++++++++++----------
 monitor.c       |   20 +++++++++++++-------
 qerror.c        |    4 ++++
 qerror.h        |    3 +++
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 17 deletions(-)
Signature · 1 line
1.7.4
#54 lore raw

Re: [PATCH 0/2 V9] hmp,qmp: add inject-nmi

Quoted 4 lines · On Thu, 28 Apr 2011 11:35:20 +0800 Lai Jiangshan <[email protected]> wrote:

Adds new QERR_UNSUPPORTED, converts "nmi" to "inject-nmi" and make it supports qmp.

Lai, unfortunately this series still have some issues (like changing the HMP command name). I think V7 was the best submission so far, so I decided to do this: I've incorporated your v7 patches in a new series and fixed a few issues. I'll submit it for review.

#55 lore raw

PATCH v9 1/2 qemu,qmp: QError: New QERR_UNSUPPORTED

In reply to Luiz Capitulino

New QERR_UNSUPPORTED for unsupported commands or requests.

---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)
qerror.c +4 -0
index 4855604..f905887 100644
@@ -206,6 +206,10 @@ static const QErrorStringTable qerror_table[] = {
                      "supported by this qemu version: %(feature)",
     },
     {
+        .error_fmt = QERR_UNSUPPORTED,
+        .desc      = "Unsupported: %(detail)",
+    },
+    {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },
qerror.h +3 -0
index df61d2c..b3455ce 100644
@@ -168,6 +168,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
     "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'format': %s, 'feature': %s } }"
 
+#define QERR_UNSUPPORTED \
+    "{ 'class': 'Unsupported', 'data': { 'detail': %s } }"
+
 #define QERR_VNC_SERVER_FAILED \
     "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
 
Signature · 1 line
1.7.4
#56 lore raw

PATCH v9 2/2 qmp,inject-nmi: convert do_inject_nmi() to QObject

In reply to Luiz Capitulino

Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the command name, the meaning is clearer. The behavior is cheanged to "injecting NMI to all CPU" which simulates the Real hardware NMI button.

The command "inject-nmi" is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest. This error and this behavior are described in the comments.

---
 hmp-commands.hx |   21 +++++++++++----------
 monitor.c       |   20 +++++++++++++-------
 qmp-commands.hx |   29 +++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 17 deletions(-)
hmp-commands.hx +11 -10
index 834e6a8..b511850 100644
@@ -737,19 +737,20 @@ 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)
     {
-        .name       = "nmi",
-        .args_type  = "cpu_index:i",
-        .params     = "cpu",
-        .help       = "inject an NMI on the given CPU",
-        .mhandler.cmd = do_inject_nmi,
+        .name       = "inject-nmi",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Inject an NMI on guest.\n"
+                      "Returns \"Unsupported\" error when the guest does"
+                      "not support NMI injection",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi,
     },
-#endif
 STEXI
-@item nmi @var{cpu}
-@findex nmi
-Inject an NMI on the given CPU (x86 only).
+@item inject-nmi
+@findex inject-nmi
+Inject an NMI on the guest (x86 only).
 ETEXI
 
     {
monitor.c +13 -7
index 5f3bc72..129eed1 100644
@@ -2544,16 +2544,22 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)
 #endif
 
 #if defined(TARGET_I386)
-static void do_inject_nmi(Monitor *mon, const QDict *qdict)
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     CPUState *env;
-    int cpu_index = qdict_get_int(qdict, "cpu_index");
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
-        if (env->cpu_index == cpu_index) {
-            cpu_interrupt(env, CPU_INTERRUPT_NMI);
-            break;
-        }
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+    }
+
+    return 0;
+}
+#else
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    qerror_report(QERR_UNSUPPORTED,
+                  "Injecting NMI is unsupported for the non-x86 guest");
+    return -1;
 }
 #endif
 
qmp-commands.hx +29 -0
index fbd98ee..e1b9b40 100644
@@ -430,6 +430,35 @@ Example:
 EQMP
 
     {
+        .name       = "inject-nmi",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Inject an NMI on guest.\n"
+                      "Returns \"Unsupported\" error when the guest does"
+                      "not support NMI injection",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi,
+    },
+
+SQMP
+inject-nmi
+----------
+
+Inject an NMI on guest.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "inject-nmi" }
+<- { "return": {} }
+
+Note: inject-nmi is only supported for x86 guest currently, it will
+      returns "Unsupported" error for non-x86 guest.
+
+EQMP
+
+    {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
         .params     = "[-d] [-b] [-i] uri",
Signature · 1 line
1.7.4
#57 lore raw

Re: [Qemu-devel] [RFC PATCH 0/3 V8] QAPI: add inject-nmi qmp command

In reply to Lai Jiangshan
Quoted 8 lines · Lai Jiangshan

These patches are applied for "http://repo.or.cz/r/qemu/aliguori.git glib".

These patches add QAPI inject-nmi. They are passed checkpatch.pl and the build.

But the result qemu executable file is not tested, because the result qemu of "http://repo.or.cz/r/qemu/aliguori.git glib" can't work in my box.

What issues are you seeing using the binary from the glib tree? AFAIK that tree should be functional, except potentially with TCG. I've only been using it with KVM and --enable-io-thread though so don't know for sure.

Quoted 16 lines

Lai Jiangshan (3):
QError: Introduce QERR_UNSUPPORTED
qapi,nmi: add inject-nmi qmp command
qapi-hmp: Convert HMP nmi to use QMP

  hmp-commands.hx  |   18 ++++++++----------
  hmp.c            |   12 ++++++++++++
  hmp.h            |    1 +
  monitor.c        |   14 --------------
  qapi-schema.json |   12 ++++++++++++
  qerror.c         |    4 ++++
  qerror.h         |    3 +++
  qmp.c            |   17 +++++++++++++++++
  8 files changed, 57 insertions(+), 24 deletions(-)
#58 lore raw

RFC PATCH v8 2/3 qapi,nmi: add inject-nmi qmp command

In reply to Luiz Capitulino

inject-nmi command injects an NMI on all CPUs of guest. It is only supported for x86 guest currently, it will returns "Unsupported" error for non-x86 guest.

---
 qapi-schema.json |   12 ++++++++++++
 qmp.c            |   17 +++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
qapi-schema.json +12 -0
index de6c9a3..6a94d78 100644
@@ -1498,3 +1498,15 @@
 # Since: 0.14.0
 ##
 { 'option': 'vnc', 'data': 'VncConfig', 'implicit': 'address' }
+
+##
+# @inject-nmi:
+#
+# Inject an NMI on the guest.
+#
+# Returns: Nothing on success.
+#          If the guest(non-x86) does not support NMI injection, Unsupported
+#
+# Since: 0.15.0
+##
+{ 'command': 'inject-nmi' }
qmp.c +17 -0
index 9fdde8f..dac411a 100644
@@ -379,3 +379,20 @@ StatusInfo *qmp_query_status(Error **errp)
 
     return info;
 }
+
+#if defined(TARGET_I386)
+void qmp_inject_nmi(Error **errp)
+{
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cpu_interrupt(env, CPU_INTERRUPT_NMI);
+    }
+}
+#else
+void qmp_inject_nmi(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED,
+              "Injecting NMI is unsupported for the non-x86 guest");
+}
+#endif
Signature · 1 line
1.7.4
#59 lore raw

RFC PATCH v8 3/3 qapi-hmp: Convert HMP nmi to use QMP

In reply to Luiz Capitulino

Convert the name of HMP nmi to inject-nmi, and use QMP inject-nmi. The behavier is also changed, it injects NMI to all CPUs of the guest. When the guest is non-x86, it reports "Unsupported" error.

---
 hmp-commands.hx |   18 ++++++++----------
 hmp.c           |   12 ++++++++++++
 hmp.h           |    1 +
 monitor.c       |   14 --------------
 4 files changed, 21 insertions(+), 24 deletions(-)
hmp-commands.hx +8 -10
index c0719eb..70a9dd9 100644
@@ -705,19 +705,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)
     {
-        .name       = "nmi",
-        .args_type  = "cpu_index:i",
-        .params     = "cpu",
-        .help       = "inject an NMI on the given CPU",
-        .mhandler.cmd = do_inject_nmi,
+        .name       = "inject-nmi",
+        .args_type  = "",
+        .params     = "",
+        .help       = "inject an NMI on the guest",
+        .mhandler.cmd = hmp_inject_nmi,
     },
-#endif
 STEXI
-@item nmi @var{cpu}
-@findex nmi
-Inject an NMI on the given CPU (x86 only).
+@item inject-nmi
+@findex inject-nmi
+Inject an NMI on the guest (x86 only).
 ETEXI
 
     {
hmp.c +12 -0
index 758b085..07170fd 100644
@@ -369,6 +369,18 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "closefd: Command Not Supported, use QMP\n");
 }
 
+void hmp_inject_nmi(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+
+    qmp_inject_nmi(&err);
+
+    if (err) {
+        monitor_printf(mon, "inject-nmi: %s\n", error_get_pretty(err));
+        error_free(err);
+    }
+}
+
 void hmp_info_version(Monitor *mon)
 {
     VersionInfo *info;
hmp.h +1 -0
index d205b3e..3bc1f54 100644
@@ -31,6 +31,7 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
+void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
 
 void hmp_info_version(Monitor *mon);
 void hmp_info_status(Monitor *mon);
monitor.c +0 -14
index 25a4ab5..925c143 100644
@@ -1768,20 +1768,6 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)
 }
 #endif
 
-#if defined(TARGET_I386)
-static void do_inject_nmi(Monitor *mon, const QDict *qdict)
-{
-    CPUState *env;
-    int cpu_index = qdict_get_int(qdict, "cpu_index");
-
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
-        if (env->cpu_index == cpu_index) {
-            cpu_interrupt(env, CPU_INTERRUPT_NMI);
-            break;
-        }
-}
-#endif
-
 static qemu_acl *find_acl(Monitor *mon, const char *name)
 {
     qemu_acl *acl = qemu_acl_find(name);
Signature · 1 line
1.7.4