[Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Lai Jiangshan]

Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).

changed from v1
Add document.
Add error handling when the cpu index is invalid.

changed from v2
use QERR_INVALID_PARAMETER_VALUE as Markus suggest.

Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 23024ba..f86d9fe 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -724,7 +724,8 @@ ETEXI
         .args_type  = "cpu_index:i",
         .params     = "cpu",
         .help       = "inject an NMI on the given CPU",
-        .mhandler.cmd = do_inject_nmi,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi,
     },
 #endif
 STEXI
diff --git a/monitor.c b/monitor.c
index ec31eac..3e33a96 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2119,7 +2119,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 int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     CPUState *env;
     int cpu_index = qdict_get_int(qdict, "cpu_index");
@@ -2127,8 +2127,12 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
     for (env = first_cpu; env != NULL; env = env->next_cpu)
         if (env->cpu_index == cpu_index) {
             cpu_interrupt(env, CPU_INTERRUPT_NMI);
-            break;
+            return 0;
         }
+
+    qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu_index",
+                  "a CPU number");
+    return -1;
 }
 #endif
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e5f157f..fcb6bf2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -429,6 +429,33 @@ Example:
 
 EQMP
 
+#if defined(TARGET_I386)
+    {
+        .name       = "inject_nmi",
+        .args_type  = "cpu_index:i",
+        .params     = "cpu",
+        .help       = "inject an NMI on the given CPU",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi,
+    },
+#endif
+SQMP
+inject_nmi
+----------
+
+Inject an NMI on the given CPU (x86 only).
+
+Arguments:
+
+- "cpu_index": the index of the CPU to be injected NMI (json-int)
+
+Example:
+
+-> { "execute": "inject_nmi", "arguments": { "cpu_index": 0 } }
+<- { "return": {} }
+
+EQMP
+
     {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",

Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Markus Armbruster]

Lai Jiangshan <laijs@cn.fujitsu.com> writes:

> Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
>
> changed from v1
> Add document.
> Add error handling when the cpu index is invalid.
>
> changed from v2
> use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
>
> Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>

A note on commit messages:

The commit message should describe the current version of the patch.
Don't repeat the subject line in the body.

Patch history is very useful for review, but usually uninteresting once
the patch is committed.  Thus, it's best to put it after the "---"
separator.

Subsystem tags in the subject line are helpful.  But "qemu" doesn't
provide any information there :)


Regarding the patch:

The conversion looks good.

The new QMP command is called "inject_nmi", while the existing human
monitor command is called "nmi".  Luiz asked for this name change.  I
don't mind.  But should we rename the human monitor command for
consistency?

Regardless, the differing command name is worth mentioning in the commit
message.

Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Wed, 15 Dec 2010 11:49:23 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Lai Jiangshan <laijs@cn.fujitsu.com> writes:
> 
> > Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
> >
> > changed from v1
> > Add document.
> > Add error handling when the cpu index is invalid.
> >
> > changed from v2
> > use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
> >
> > Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
> 
> A note on commit messages:
> 
> The commit message should describe the current version of the patch.
> Don't repeat the subject line in the body.
> 
> Patch history is very useful for review, but usually uninteresting once
> the patch is committed.  Thus, it's best to put it after the "---"
> separator.
> 
> Subsystem tags in the subject line are helpful.  But "qemu" doesn't
> provide any information there :)
> 
> 
> Regarding the patch:
> 
> The conversion looks good.
> 
> The new QMP command is called "inject_nmi", while the existing human
> monitor command is called "nmi".  Luiz asked for this name change.  I
> don't mind.  But should we rename the human monitor command for
> consistency?

I don't think so, we don't need (and maybe don't even want) naming parity
between QMP and HMP. Remember that one of our mistakes was to couple the two.

Also, Avi asked for more descriptive names in QMP and I agree with him, I
would even be in favor of calling it inject-non-maskable-interrupt.

> 
> Regardless, the differing command name is worth mentioning in the commit
> message.

Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 15 Dec 2010 11:49:23 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Lai Jiangshan <laijs@cn.fujitsu.com> writes:
>> 
>> > Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
>> >
>> > changed from v1
>> > Add document.
>> > Add error handling when the cpu index is invalid.
>> >
>> > changed from v2
>> > use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
>> >
>> > Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
>> 
>> A note on commit messages:
>> 
>> The commit message should describe the current version of the patch.
>> Don't repeat the subject line in the body.
>> 
>> Patch history is very useful for review, but usually uninteresting once
>> the patch is committed.  Thus, it's best to put it after the "---"
>> separator.
>> 
>> Subsystem tags in the subject line are helpful.  But "qemu" doesn't
>> provide any information there :)
>> 
>> 
>> Regarding the patch:
>> 
>> The conversion looks good.
>> 
>> The new QMP command is called "inject_nmi", while the existing human
>> monitor command is called "nmi".  Luiz asked for this name change.  I
>> don't mind.  But should we rename the human monitor command for
>> consistency?
>
> I don't think so, we don't need (and maybe don't even want) naming parity
> between QMP and HMP. Remember that one of our mistakes was to couple the two.

Human "nmi" and QMP "inject_nmi" are identical commands, aren't they?
Giving the same things the same name isn't coupling :)

The mistake that matters here was adopting existing human commands for
QMP uncritically.

> Also, Avi asked for more descriptive names in QMP and I agree with him, I
> would even be in favor of calling it inject-non-maskable-interrupt.

I like inject_nmi better than nmi.  inject-non-maskable-interrupt is too
long even for QMP.

>> Regardless, the differing command name is worth mentioning in the commit
>> message.

Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Wed, 15 Dec 2010 18:39:07 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 15 Dec 2010 11:49:23 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Lai Jiangshan <laijs@cn.fujitsu.com> writes:
> >> 
> >> > Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
> >> >
> >> > changed from v1
> >> > Add document.
> >> > Add error handling when the cpu index is invalid.
> >> >
> >> > changed from v2
> >> > use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
> >> >
> >> > Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
> >> 
> >> A note on commit messages:
> >> 
> >> The commit message should describe the current version of the patch.
> >> Don't repeat the subject line in the body.
> >> 
> >> Patch history is very useful for review, but usually uninteresting once
> >> the patch is committed.  Thus, it's best to put it after the "---"
> >> separator.
> >> 
> >> Subsystem tags in the subject line are helpful.  But "qemu" doesn't
> >> provide any information there :)
> >> 
> >> 
> >> Regarding the patch:
> >> 
> >> The conversion looks good.
> >> 
> >> The new QMP command is called "inject_nmi", while the existing human
> >> monitor command is called "nmi".  Luiz asked for this name change.  I
> >> don't mind.  But should we rename the human monitor command for
> >> consistency?
> >
> > I don't think so, we don't need (and maybe don't even want) naming parity
> > between QMP and HMP. Remember that one of our mistakes was to couple the two.
> 
> Human "nmi" and QMP "inject_nmi" are identical commands, aren't they?

At this point in time yes, but they might not be in the near future. Assuming
they might be different is the safest thing to do.

That's true for all existing commands.

> Giving the same things the same name isn't coupling :)

Expecting them to be the same in the future is.

> The mistake that matters here was adopting existing human commands for
> QMP uncritically.

That's the protocol visible mistake, yes.

> > Also, Avi asked for more descriptive names in QMP and I agree with him, I
> > would even be in favor of calling it inject-non-maskable-interrupt.
> 
> I like inject_nmi better than nmi.  inject-non-maskable-interrupt is too
> long even for QMP.

It's not supposed to be typed that much, but I'm not that strong about that.

nitpick: I think we should be consistent in the use of "_" or "-", eg. we
         should pick inject-nmi or inject_nmi?

> 
> >> Regardless, the differing command name is worth mentioning in the commit
> >> message.
> 

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Wed, 15 Dec 2010 17:49:27 +0800
Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> 
> Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
> 
> changed from v1
> Add document.
> Add error handling when the cpu index is invalid.
> 
> changed from v2
> use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
> 
> Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 23024ba..f86d9fe 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -724,7 +724,8 @@ ETEXI
>          .args_type  = "cpu_index:i",
>          .params     = "cpu",
>          .help       = "inject an NMI on the given CPU",
> -        .mhandler.cmd = do_inject_nmi,
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_inject_nmi,
>      },
>  #endif
>  STEXI
> diff --git a/monitor.c b/monitor.c
> index ec31eac..3e33a96 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2119,7 +2119,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 int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      CPUState *env;
>      int cpu_index = qdict_get_int(qdict, "cpu_index");
> @@ -2127,8 +2127,12 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
>      for (env = first_cpu; env != NULL; env = env->next_cpu)
>          if (env->cpu_index == cpu_index) {
>              cpu_interrupt(env, CPU_INTERRUPT_NMI);
> -            break;
> +            return 0;
>          }
> +
> +    qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu_index",
> +                  "a CPU number");
> +    return -1;
>  }
>  #endif
>  
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e5f157f..fcb6bf2 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -429,6 +429,33 @@ Example:
>  
>  EQMP
>  
> +#if defined(TARGET_I386)
> +    {
> +        .name       = "inject_nmi",
> +        .args_type  = "cpu_index:i",
> +        .params     = "cpu",
> +        .help       = "inject an NMI on the given CPU",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_inject_nmi,
> +    },
> +#endif
> +SQMP
> +inject_nmi
> +----------
> +
> +Inject an NMI on the given CPU (x86 only).
> +
> +Arguments:
> +
> +- "cpu_index": the index of the CPU to be injected NMI (json-int)

Please, use cpu-index, that's what we're using for the human-monitor-command.

Avi, Anthony, can you please ACK this new command?

> +
> +Example:
> +
> +-> { "execute": "inject_nmi", "arguments": { "cpu_index": 0 } }
> +<- { "return": {} }
> +
> +EQMP
> +
>      {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> 

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Avi Kivity]

On 12/15/2010 07:09 PM, Luiz Capitulino wrote:
> On Wed, 15 Dec 2010 17:49:27 +0800
> Lai Jiangshan<laijs@cn.fujitsu.com>  wrote:
>
> >
> >  Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
> >
> >  changed from v1
> >  Add document.
> >  Add error handling when the cpu index is invalid.
> >
> >  changed from v2
> >  use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
> >
> >  Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>
> >  ---
> >  diff --git a/hmp-commands.hx b/hmp-commands.hx
> >  index 23024ba..f86d9fe 100644
> >  --- a/hmp-commands.hx
> >  +++ b/hmp-commands.hx
> >  @@ -724,7 +724,8 @@ ETEXI
> >           .args_type  = "cpu_index:i",
> >           .params     = "cpu",
> >           .help       = "inject an NMI on the given CPU",
> >  -        .mhandler.cmd = do_inject_nmi,
> >  +        .user_print = monitor_user_noop,
> >  +        .mhandler.cmd_new = do_inject_nmi,
> >       },
> >   #endif
> >   STEXI
> >  diff --git a/monitor.c b/monitor.c
> >  index ec31eac..3e33a96 100644
> >  --- a/monitor.c
> >  +++ b/monitor.c
> >  @@ -2119,7 +2119,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 int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >   {
> >       CPUState *env;
> >       int cpu_index = qdict_get_int(qdict, "cpu_index");
> >  @@ -2127,8 +2127,12 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
> >       for (env = first_cpu; env != NULL; env = env->next_cpu)
> >           if (env->cpu_index == cpu_index) {
> >               cpu_interrupt(env, CPU_INTERRUPT_NMI);
> >  -            break;
> >  +            return 0;
> >           }
> >  +
> >  +    qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu_index",
> >  +                  "a CPU number");
> >  +    return -1;
> >   }
> >   #endif
> >
> >  diff --git a/qmp-commands.hx b/qmp-commands.hx
> >  index e5f157f..fcb6bf2 100644
> >  --- a/qmp-commands.hx
> >  +++ b/qmp-commands.hx
> >  @@ -429,6 +429,33 @@ Example:
> >
> >   EQMP
> >
> >  +#if defined(TARGET_I386)
> >  +    {
> >  +        .name       = "inject_nmi",
> >  +        .args_type  = "cpu_index:i",
> >  +        .params     = "cpu",
> >  +        .help       = "inject an NMI on the given CPU",
> >  +        .user_print = monitor_user_noop,
> >  +        .mhandler.cmd_new = do_inject_nmi,
> >  +    },
> >  +#endif
> >  +SQMP
> >  +inject_nmi
> >  +----------
> >  +
> >  +Inject an NMI on the given CPU (x86 only).
> >  +
> >  +Arguments:
> >  +
> >  +- "cpu_index": the index of the CPU to be injected NMI (json-int)
>
> Please, use cpu-index, that's what we're using for the human-monitor-command.
>
> Avi, Anthony, can you please ACK this new command?
>

I'd like to see cpu-index made optional; if not present, nmi all cpus 
(that's what the nmi button on many machines does, or at least I think 
that's what it does).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Wed, 15 Dec 2010 19:18:32 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 12/15/2010 07:09 PM, Luiz Capitulino wrote:
> > On Wed, 15 Dec 2010 17:49:27 +0800
> > Lai Jiangshan<laijs@cn.fujitsu.com>  wrote:
> >
> > >
> > >  Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
> > >
> > >  changed from v1
> > >  Add document.
> > >  Add error handling when the cpu index is invalid.
> > >
> > >  changed from v2
> > >  use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
> > >
> > >  Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>
> > >  ---
> > >  diff --git a/hmp-commands.hx b/hmp-commands.hx
> > >  index 23024ba..f86d9fe 100644
> > >  --- a/hmp-commands.hx
> > >  +++ b/hmp-commands.hx
> > >  @@ -724,7 +724,8 @@ ETEXI
> > >           .args_type  = "cpu_index:i",
> > >           .params     = "cpu",
> > >           .help       = "inject an NMI on the given CPU",
> > >  -        .mhandler.cmd = do_inject_nmi,
> > >  +        .user_print = monitor_user_noop,
> > >  +        .mhandler.cmd_new = do_inject_nmi,
> > >       },
> > >   #endif
> > >   STEXI
> > >  diff --git a/monitor.c b/monitor.c
> > >  index ec31eac..3e33a96 100644
> > >  --- a/monitor.c
> > >  +++ b/monitor.c
> > >  @@ -2119,7 +2119,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 int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > >   {
> > >       CPUState *env;
> > >       int cpu_index = qdict_get_int(qdict, "cpu_index");
> > >  @@ -2127,8 +2127,12 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
> > >       for (env = first_cpu; env != NULL; env = env->next_cpu)
> > >           if (env->cpu_index == cpu_index) {
> > >               cpu_interrupt(env, CPU_INTERRUPT_NMI);
> > >  -            break;
> > >  +            return 0;
> > >           }
> > >  +
> > >  +    qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu_index",
> > >  +                  "a CPU number");
> > >  +    return -1;
> > >   }
> > >   #endif
> > >
> > >  diff --git a/qmp-commands.hx b/qmp-commands.hx
> > >  index e5f157f..fcb6bf2 100644
> > >  --- a/qmp-commands.hx
> > >  +++ b/qmp-commands.hx
> > >  @@ -429,6 +429,33 @@ Example:
> > >
> > >   EQMP
> > >
> > >  +#if defined(TARGET_I386)
> > >  +    {
> > >  +        .name       = "inject_nmi",
> > >  +        .args_type  = "cpu_index:i",
> > >  +        .params     = "cpu",
> > >  +        .help       = "inject an NMI on the given CPU",
> > >  +        .user_print = monitor_user_noop,
> > >  +        .mhandler.cmd_new = do_inject_nmi,
> > >  +    },
> > >  +#endif
> > >  +SQMP
> > >  +inject_nmi
> > >  +----------
> > >  +
> > >  +Inject an NMI on the given CPU (x86 only).
> > >  +
> > >  +Arguments:
> > >  +
> > >  +- "cpu_index": the index of the CPU to be injected NMI (json-int)
> >
> > Please, use cpu-index, that's what we're using for the human-monitor-command.
> >
> > Avi, Anthony, can you please ACK this new command?
> >
> 
> I'd like to see cpu-index made optional; if not present, nmi all cpus 
> (that's what the nmi button on many machines does, or at least I think 
> that's what it does).

Looks like a GUI feature to me, _might_ turn out to be an undesirable
side effect to client writers. I guess I prefer a to-all-cpus argument.

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 15 Dec 2010 19:18:32 +0200
> Avi Kivity <avi@redhat.com> wrote:
>
>> On 12/15/2010 07:09 PM, Luiz Capitulino wrote:
>> > On Wed, 15 Dec 2010 17:49:27 +0800
>> > Lai Jiangshan<laijs@cn.fujitsu.com>  wrote:
>> >
>> > >
>> > >  Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
>> > >
>> > >  changed from v1
>> > >  Add document.
>> > >  Add error handling when the cpu index is invalid.
>> > >
>> > >  changed from v2
>> > >  use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
>> > >
>> > >  Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>
>> > >  ---
>> > >  diff --git a/hmp-commands.hx b/hmp-commands.hx
>> > >  index 23024ba..f86d9fe 100644
>> > >  --- a/hmp-commands.hx
>> > >  +++ b/hmp-commands.hx
>> > >  @@ -724,7 +724,8 @@ ETEXI
>> > >           .args_type  = "cpu_index:i",
>> > >           .params     = "cpu",
>> > >           .help       = "inject an NMI on the given CPU",
>> > >  -        .mhandler.cmd = do_inject_nmi,
>> > >  +        .user_print = monitor_user_noop,
>> > >  +        .mhandler.cmd_new = do_inject_nmi,
>> > >       },
>> > >   #endif
>> > >   STEXI
>> > >  diff --git a/monitor.c b/monitor.c
>> > >  index ec31eac..3e33a96 100644
>> > >  --- a/monitor.c
>> > >  +++ b/monitor.c
>> > >  @@ -2119,7 +2119,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 int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> > >   {
>> > >       CPUState *env;
>> > >       int cpu_index = qdict_get_int(qdict, "cpu_index");
>> > >  @@ -2127,8 +2127,12 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
>> > >       for (env = first_cpu; env != NULL; env = env->next_cpu)
>> > >           if (env->cpu_index == cpu_index) {
>> > >               cpu_interrupt(env, CPU_INTERRUPT_NMI);
>> > >  -            break;
>> > >  +            return 0;
>> > >           }
>> > >  +
>> > >  +    qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu_index",
>> > >  +                  "a CPU number");
>> > >  +    return -1;
>> > >   }
>> > >   #endif
>> > >
>> > >  diff --git a/qmp-commands.hx b/qmp-commands.hx
>> > >  index e5f157f..fcb6bf2 100644
>> > >  --- a/qmp-commands.hx
>> > >  +++ b/qmp-commands.hx
>> > >  @@ -429,6 +429,33 @@ Example:
>> > >
>> > >   EQMP
>> > >
>> > >  +#if defined(TARGET_I386)
>> > >  +    {
>> > >  +        .name       = "inject_nmi",
>> > >  +        .args_type  = "cpu_index:i",
>> > >  +        .params     = "cpu",
>> > >  +        .help       = "inject an NMI on the given CPU",
>> > >  +        .user_print = monitor_user_noop,
>> > >  +        .mhandler.cmd_new = do_inject_nmi,
>> > >  +    },
>> > >  +#endif
>> > >  +SQMP
>> > >  +inject_nmi
>> > >  +----------
>> > >  +
>> > >  +Inject an NMI on the given CPU (x86 only).
>> > >  +
>> > >  +Arguments:
>> > >  +
>> > >  +- "cpu_index": the index of the CPU to be injected NMI (json-int)
>> >
>> > Please, use cpu-index, that's what we're using for the human-monitor-command.
>> >
>> > Avi, Anthony, can you please ACK this new command?
>> >
>> 
>> I'd like to see cpu-index made optional; if not present, nmi all cpus 
>> (that's what the nmi button on many machines does, or at least I think 
>> that's what it does).
>
> Looks like a GUI feature to me,

Really?  Can't see how you can build "NMI to all CPUs" from "NMI this
CPU".  Or am I misunderstanding you?

>                                 _might_ turn out to be an undesirable
> side effect to client writers.

They seem to be coping fine with optional arguments elsewhere.

>                                I guess I prefer a to-all-cpus argument.

How would that look like?  "cpu-index": "all"?

I find "optional json-int" a simpler schema than "either a json-int or
the json-string "all".

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Wed, 15 Dec 2010 18:45:09 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 15 Dec 2010 19:18:32 +0200
> > Avi Kivity <avi@redhat.com> wrote:
> >
> >> On 12/15/2010 07:09 PM, Luiz Capitulino wrote:
> >> > On Wed, 15 Dec 2010 17:49:27 +0800
> >> > Lai Jiangshan<laijs@cn.fujitsu.com>  wrote:
> >> >
> >> > >
> >> > >  Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
> >> > >
> >> > >  changed from v1
> >> > >  Add document.
> >> > >  Add error handling when the cpu index is invalid.
> >> > >
> >> > >  changed from v2
> >> > >  use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
> >> > >
> >> > >  Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>
> >> > >  ---
> >> > >  diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> > >  index 23024ba..f86d9fe 100644
> >> > >  --- a/hmp-commands.hx
> >> > >  +++ b/hmp-commands.hx
> >> > >  @@ -724,7 +724,8 @@ ETEXI
> >> > >           .args_type  = "cpu_index:i",
> >> > >           .params     = "cpu",
> >> > >           .help       = "inject an NMI on the given CPU",
> >> > >  -        .mhandler.cmd = do_inject_nmi,
> >> > >  +        .user_print = monitor_user_noop,
> >> > >  +        .mhandler.cmd_new = do_inject_nmi,
> >> > >       },
> >> > >   #endif
> >> > >   STEXI
> >> > >  diff --git a/monitor.c b/monitor.c
> >> > >  index ec31eac..3e33a96 100644
> >> > >  --- a/monitor.c
> >> > >  +++ b/monitor.c
> >> > >  @@ -2119,7 +2119,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 int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >> > >   {
> >> > >       CPUState *env;
> >> > >       int cpu_index = qdict_get_int(qdict, "cpu_index");
> >> > >  @@ -2127,8 +2127,12 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
> >> > >       for (env = first_cpu; env != NULL; env = env->next_cpu)
> >> > >           if (env->cpu_index == cpu_index) {
> >> > >               cpu_interrupt(env, CPU_INTERRUPT_NMI);
> >> > >  -            break;
> >> > >  +            return 0;
> >> > >           }
> >> > >  +
> >> > >  +    qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu_index",
> >> > >  +                  "a CPU number");
> >> > >  +    return -1;
> >> > >   }
> >> > >   #endif
> >> > >
> >> > >  diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> > >  index e5f157f..fcb6bf2 100644
> >> > >  --- a/qmp-commands.hx
> >> > >  +++ b/qmp-commands.hx
> >> > >  @@ -429,6 +429,33 @@ Example:
> >> > >
> >> > >   EQMP
> >> > >
> >> > >  +#if defined(TARGET_I386)
> >> > >  +    {
> >> > >  +        .name       = "inject_nmi",
> >> > >  +        .args_type  = "cpu_index:i",
> >> > >  +        .params     = "cpu",
> >> > >  +        .help       = "inject an NMI on the given CPU",
> >> > >  +        .user_print = monitor_user_noop,
> >> > >  +        .mhandler.cmd_new = do_inject_nmi,
> >> > >  +    },
> >> > >  +#endif
> >> > >  +SQMP
> >> > >  +inject_nmi
> >> > >  +----------
> >> > >  +
> >> > >  +Inject an NMI on the given CPU (x86 only).
> >> > >  +
> >> > >  +Arguments:
> >> > >  +
> >> > >  +- "cpu_index": the index of the CPU to be injected NMI (json-int)
> >> >
> >> > Please, use cpu-index, that's what we're using for the human-monitor-command.
> >> >
> >> > Avi, Anthony, can you please ACK this new command?
> >> >
> >> 
> >> I'd like to see cpu-index made optional; if not present, nmi all cpus 
> >> (that's what the nmi button on many machines does, or at least I think 
> >> that's what it does).
> >
> > Looks like a GUI feature to me,
> 
> Really?  Can't see how you can build "NMI to all CPUs" from "NMI this
> CPU".  Or am I misunderstanding you?

I guess so. Avi referred to 'nmi button on many machines', I assumed he
meant a virtual machine GUI, am I wrong?

> >                                 _might_ turn out to be an undesirable
> > side effect to client writers.
> 
> They seem to be coping fine with optional arguments elsewhere.

Which we might want to review.

> >                                I guess I prefer a to-all-cpus argument.
> 
> How would that look like?  "cpu-index": "all"?

Like this:

{ "execute": "inject-nmi", "arguments": { "to-all-cpus": true } }

But this looks like an optimization to me, because it's also easy to do:

for cpu in query-cpus; do
  inject-nmi cpu

Unless we want to do this in an "atomic" way, due to side effects I'm
not aware about.

> I find "optional json-int" a simpler schema than "either a json-int or
> the json-string "all".
> 

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Avi Kivity]

On 12/15/2010 08:00 PM, Luiz Capitulino wrote:
> >  >
> >  >  Looks like a GUI feature to me,
> >
> >  Really?  Can't see how you can build "NMI to all CPUs" from "NMI this
> >  CPU".  Or am I misunderstanding you?
>
> I guess so. Avi referred to 'nmi button on many machines', I assumed he
> meant a virtual machine GUI, am I wrong?

I meant a real machine's GUI (it's a physical button you can press with 
your finger, if you have thin fingers).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Thu, 16 Dec 2010 11:03:38 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 12/15/2010 08:00 PM, Luiz Capitulino wrote:
> > >  >
> > >  >  Looks like a GUI feature to me,
> > >
> > >  Really?  Can't see how you can build "NMI to all CPUs" from "NMI this
> > >  CPU".  Or am I misunderstanding you?
> >
> > I guess so. Avi referred to 'nmi button on many machines', I assumed he
> > meant a virtual machine GUI, am I wrong?
> 
> I meant a real machine's GUI (it's a physical button you can press with 
> your finger, if you have thin fingers).

Ok, I didn't know that, but I had another idea: the command could accept
either a single cpu index or a list:


  { "execute": "inject-nmi", "arguments": { "cpus": 2 } }

  { "execute": "inject-nmi", "arguments": { "cpus": [1, 2, 3, 4] } }

This has the feature of injecting the nmi in just some cpus, although I'm
not sure this is going to be desired/useful.

If we agree on this we'll have to wait because the monitor doesn't currently
support "hybrid" arguments.

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Avi Kivity]

On 12/16/2010 12:48 PM, Luiz Capitulino wrote:
> Ok, I didn't know that, but I had another idea: the command could accept
> either a single cpu index or a list:
>
>
>    { "execute": "inject-nmi", "arguments": { "cpus": 2 } }
>
>    { "execute": "inject-nmi", "arguments": { "cpus": [1, 2, 3, 4] } }
>
> This has the feature of injecting the nmi in just some cpus, although I'm
> not sure this is going to be desired/useful.
>
> If we agree on this we'll have to wait because the monitor doesn't currently
> support "hybrid" arguments.

I hope it never does.  They're hard to support in old-school statically 
typed languages.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Thu, 16 Dec 2010 12:51:14 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 12/16/2010 12:48 PM, Luiz Capitulino wrote:
> > Ok, I didn't know that, but I had another idea: the command could accept
> > either a single cpu index or a list:
> >
> >
> >    { "execute": "inject-nmi", "arguments": { "cpus": 2 } }
> >
> >    { "execute": "inject-nmi", "arguments": { "cpus": [1, 2, 3, 4] } }
> >
> > This has the feature of injecting the nmi in just some cpus, although I'm
> > not sure this is going to be desired/useful.
> >
> > If we agree on this we'll have to wait because the monitor doesn't currently
> > support "hybrid" arguments.
> 
> I hope it never does.  They're hard to support in old-school statically 
> typed languages.

We could accept only a list then.

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 16 Dec 2010 11:03:38 +0200
> Avi Kivity <avi@redhat.com> wrote:
>
>> On 12/15/2010 08:00 PM, Luiz Capitulino wrote:
>> > >  >
>> > >  >  Looks like a GUI feature to me,
>> > >
>> > >  Really?  Can't see how you can build "NMI to all CPUs" from "NMI this
>> > >  CPU".  Or am I misunderstanding you?
>> >
>> > I guess so. Avi referred to 'nmi button on many machines', I assumed he
>> > meant a virtual machine GUI, am I wrong?
>> 
>> I meant a real machine's GUI (it's a physical button you can press with 
>> your finger, if you have thin fingers).
>
> Ok, I didn't know that, but I had another idea: the command could accept
> either a single cpu index or a list:
>
>
>   { "execute": "inject-nmi", "arguments": { "cpus": 2 } }
>
>   { "execute": "inject-nmi", "arguments": { "cpus": [1, 2, 3, 4] } }
>
> This has the feature of injecting the nmi in just some cpus, although I'm
> not sure this is going to be desired/useful.

Use case for NMI-ing a subset of the CPUs?

Heck, use case for anything else but "NMI all"?

> If we agree on this we'll have to wait because the monitor doesn't currently
> support "hybrid" arguments.

Let's keep the schema simple.

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Avi Kivity]

On 12/16/2010 01:47 PM, Markus Armbruster wrote:
> >
> >  This has the feature of injecting the nmi in just some cpus, although I'm
> >  not sure this is going to be desired/useful.
>
> Use case for NMI-ing a subset of the CPUs?
>
> Heck, use case for anything else but "NMI all"?

Exactly.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Thu, 16 Dec 2010 14:50:08 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 12/16/2010 01:47 PM, Markus Armbruster wrote:
> > >
> > >  This has the feature of injecting the nmi in just some cpus, although I'm
> > >  not sure this is going to be desired/useful.
> >
> > Use case for NMI-ing a subset of the CPUs?
> >
> > Heck, use case for anything else but "NMI all"?
> 
> Exactly.

Then I think the to-all-cpus argument is better.

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Avi Kivity]

On 12/16/2010 03:09 PM, Luiz Capitulino wrote:
> On Thu, 16 Dec 2010 14:50:08 +0200
> Avi Kivity<avi@redhat.com>  wrote:
>
> >  On 12/16/2010 01:47 PM, Markus Armbruster wrote:
> >  >  >
> >  >  >   This has the feature of injecting the nmi in just some cpus, although I'm
> >  >  >   not sure this is going to be desired/useful.
> >  >
> >  >  Use case for NMI-ing a subset of the CPUs?
> >  >
> >  >  Heck, use case for anything else but "NMI all"?
> >
> >  Exactly.
>
> Then I think the to-all-cpus argument is better.

Why have an argument at all?  Always nmi to all cpus.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Thu, 16 Dec 2010 15:11:50 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 12/16/2010 03:09 PM, Luiz Capitulino wrote:
> > On Thu, 16 Dec 2010 14:50:08 +0200
> > Avi Kivity<avi@redhat.com>  wrote:
> >
> > >  On 12/16/2010 01:47 PM, Markus Armbruster wrote:
> > >  >  >
> > >  >  >   This has the feature of injecting the nmi in just some cpus, although I'm
> > >  >  >   not sure this is going to be desired/useful.
> > >  >
> > >  >  Use case for NMI-ing a subset of the CPUs?
> > >  >
> > >  >  Heck, use case for anything else but "NMI all"?
> > >
> > >  Exactly.
> >
> > Then I think the to-all-cpus argument is better.
> 
> Why have an argument at all?  Always nmi to all cpus.

That's possible too.

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Lai Jiangshan]

On 12/16/2010 09:17 PM, Luiz Capitulino wrote:
> On Thu, 16 Dec 2010 15:11:50 +0200
> Avi Kivity <avi@redhat.com> wrote:
>>
>> Why have an argument at all?  Always nmi to all cpus.
> 

I think Avi's suggest is better, and I will use
"inject-nmi" (without cpu-index argument) to send NMI to all cpus,
like physical GUI. If some one want to send NMI to a set of cpus, 
he can use "inject-nmi" multiple times.

I also like "cpu-index", so I have to add another patch for
coverting current "cpu_index" to "cpu-index".

Thanks,
Lai

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Fri, 17 Dec 2010 14:20:15 +0800
Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> On 12/16/2010 09:17 PM, Luiz Capitulino wrote:
> > On Thu, 16 Dec 2010 15:11:50 +0200
> > Avi Kivity <avi@redhat.com> wrote:
> >>
> >> Why have an argument at all?  Always nmi to all cpus.
> > 
> 
> I think Avi's suggest is better, and I will use
> "inject-nmi" (without cpu-index argument) to send NMI to all cpus,
> like physical GUI. If some one want to send NMI to a set of cpus, 
> he can use "inject-nmi" multiple times.

His suggestion is to drop _all_ arguments, right Avi?

This will simplify things, but you'll need a small refactoring to keep
the human monitor behavior (which accepts a cpu index).

> I also like "cpu-index", so I have to add another patch for
> coverting current "cpu_index" to "cpu-index".
> 
> Thanks,
> Lai
> 

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Avi Kivity]

On 12/17/2010 01:22 PM, Luiz Capitulino wrote:
> >
> >  I think Avi's suggest is better, and I will use
> >  "inject-nmi" (without cpu-index argument) to send NMI to all cpus,
> >  like physical GUI. If some one want to send NMI to a set of cpus,
> >  he can use "inject-nmi" multiple times.
>
> His suggestion is to drop _all_ arguments, right Avi?

Yes.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Lai Jiangshan]

On 12/17/2010 11:25 PM, Avi Kivity wrote:
> On 12/17/2010 01:22 PM, Luiz Capitulino wrote:
>> >
>> >  I think Avi's suggest is better, and I will use
>> >  "inject-nmi" (without cpu-index argument) to send NMI to all cpus,
>> >  like physical GUI. If some one want to send NMI to a set of cpus,
>> >  he can use "inject-nmi" multiple times.
>>
>> His suggestion is to drop _all_ arguments, right Avi?
> 
> Yes.
> 

We don't need to drop the cpu-index argument,
the upstream tools(libvirt etc.) can just issue "inject-nmi"
command without any argument when need.

Reasons to keep this argument
1) Useful for kernel developer or debuger sending NMI to a special CPU.
2) Share the code with nmi of hmp version. Share the way how to
   use these two commands.(hmp version and qmp version)

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Luiz Capitulino]

On Mon, 20 Dec 2010 14:09:05 +0800
Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> On 12/17/2010 11:25 PM, Avi Kivity wrote:
> > On 12/17/2010 01:22 PM, Luiz Capitulino wrote:
> >> >
> >> >  I think Avi's suggest is better, and I will use
> >> >  "inject-nmi" (without cpu-index argument) to send NMI to all cpus,
> >> >  like physical GUI. If some one want to send NMI to a set of cpus,
> >> >  he can use "inject-nmi" multiple times.
> >>
> >> His suggestion is to drop _all_ arguments, right Avi?
> > 
> > Yes.
> > 
> 
> We don't need to drop the cpu-index argument,
> the upstream tools(libvirt etc.) can just issue "inject-nmi"
> command without any argument when need.
> 
> Reasons to keep this argument
> 1) Useful for kernel developer or debuger sending NMI to a special CPU.

Ok.

> 2) Share the code with nmi of hmp version. Share the way how to
>    use these two commands.(hmp version and qmp version)

This is bad. As a general rule, we shouldn't tweak QMP interfaces with
the intention of sharing code with HMP or anything like that.

Anyway, I buy your first argument, although I'm not a kernel developer
so I'm just trusting your use case.

Re: [Qemu-devel] Re: [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 15 Dec 2010 18:45:09 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Wed, 15 Dec 2010 19:18:32 +0200
>> > Avi Kivity <avi@redhat.com> wrote:
[...]
>> >> I'd like to see cpu-index made optional; if not present, nmi all cpus 
>> >> (that's what the nmi button on many machines does, or at least I think 
>> >> that's what it does).
>> >
>> > Looks like a GUI feature to me,
>> 
>> Really?  Can't see how you can build "NMI to all CPUs" from "NMI this
>> CPU".  Or am I misunderstanding you?
>
> I guess so. Avi referred to 'nmi button on many machines', I assumed he
> meant a virtual machine GUI, am I wrong?
>
>> >                                 _might_ turn out to be an undesirable
>> > side effect to client writers.
>> 
>> They seem to be coping fine with optional arguments elsewhere.
>
> Which we might want to review.
>
>> >                                I guess I prefer a to-all-cpus argument.
>> 
>> How would that look like?  "cpu-index": "all"?
>
> Like this:
>
> { "execute": "inject-nmi", "arguments": { "to-all-cpus": true } }
>
> But this looks like an optimization to me, because it's also easy to do:
>
> for cpu in query-cpus; do
>   inject-nmi cpu
>
> Unless we want to do this in an "atomic" way, due to side effects I'm
> not aware about.

I'd expect a physical NMI button to interrupt all CPUs simultaneously
(modulo wire length).

[...]