[Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Luiz Capitulino]

This series introduces the inject-nmi command for QMP, which sends an
NMI to _all_ guest's CPUs.

Also note that this series changes the human monitor nmi command to use
the QMP implementation, which means that it now has a DIFFERENT behavior.
Please, check patch 3/3 for details.

[Qemu-devel] [PATCH 1/3] QMP: QError: New QERR_UNSUPPORTED

[Luiz Capitulino]

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

New QERR_UNSUPPORTED for unsupported commands or requests.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 4855604..4f3b7ca 100644
--- a/qerror.c
+++ b/qerror.c
@@ -201,6 +201,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "An undefined error has ocurred",
     },
     {
+        .error_fmt = QERR_UNSUPPORTED,
+        .desc      = "this feature or command is not currently supported",
+    },
+    {
         .error_fmt = QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
         .desc      = "'%(device)' uses a %(format) feature which is not "
                      "supported by this qemu version: %(feature)",
diff --git a/qerror.h b/qerror.h
index df61d2c..582b5ef 100644
--- a/qerror.h
+++ b/qerror.h
@@ -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 } }"
 
-- 
1.7.5.GIT

[Qemu-devel] [PATCH 2/3] QMP: add inject-nmi qmp command

[Luiz Capitulino]

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

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.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   17 +++++++++++++++++
 qmp-commands.hx |   27 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5f3bc72..455a528 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2555,6 +2555,23 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
             break;
         }
 }
+
+static int do_inject_nmi_all(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_all(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)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fbd98ee..1058c38 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -430,6 +430,33 @@ Example:
 EQMP
 
     {
+        .name       = "inject-nmi",
+        .args_type  = "",
+        .params     = "",
+        .help       = "",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi_all,
+    },
+
+SQMP
+inject-nmi
+----------
+
+Inject an NMI on guest's CPUs.
+
+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",
-- 
1.7.5.GIT

[Qemu-devel] [PATCH 3/3] HMP: Use QMP inject nmi implementation

[Luiz Capitulino]

This **CHANGES** the human monitor "nmi" command behavior.

Currently it accepts an CPU argument which, when provided, will send
the NMI to the specified CPU. This feature is of discussable value
though and HMP shouldn't have more features than QMP, so let's use
QMP's instead (it's also simpler).

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx |    9 +++++----
 monitor.c       |   16 ++--------------
 qmp-commands.hx |    2 +-
 3 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 834e6a8..6ad8806 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -740,10 +740,11 @@ 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,
+        .args_type  = "",
+        .params     = "",
+        .help       = "inject an NMI on all guest's CPUs",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_inject_nmi,
     },
 #endif
 STEXI
diff --git a/monitor.c b/monitor.c
index 455a528..1ba98b0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2544,19 +2544,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)
-{
-    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;
-        }
-}
-
-static int do_inject_nmi_all(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     CPUState *env;
 
@@ -2567,7 +2555,7 @@ static int do_inject_nmi_all(Monitor *mon, const QDict *qdict, QObject **ret_dat
     return 0;
 }
 #else
-static int do_inject_nmi_all(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     qerror_report(QERR_UNSUPPORTED);
     return -1;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1058c38..1957730 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -435,7 +435,7 @@ EQMP
         .params     = "",
         .help       = "",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_inject_nmi_all,
+        .mhandler.cmd_new = do_inject_nmi,
     },
 
 SQMP
-- 
1.7.5.GIT

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Blue Swirl]

On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> This series introduces the inject-nmi command for QMP, which sends an
> NMI to _all_ guest's CPUs.
>
> Also note that this series changes the human monitor nmi command to use
> the QMP implementation, which means that it now has a DIFFERENT behavior.
> Please, check patch 3/3 for details.

As discussed earlier, please change the QMP version for future
expandability so that instead of single command 'inject-nmi', 'inject'
takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
'inject' should be added.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Luiz Capitulino]

On Sat, 30 Apr 2011 09:33:15 +0300
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > This series introduces the inject-nmi command for QMP, which sends an
> > NMI to _all_ guest's CPUs.
> >
> > Also note that this series changes the human monitor nmi command to use
> > the QMP implementation, which means that it now has a DIFFERENT behavior.
> > Please, check patch 3/3 for details.
> 
> As discussed earlier, please change the QMP version for future
> expandability so that instead of single command 'inject-nmi', 'inject'
> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
> 'inject' should be added.

I'm not sure I agree with this, because we risky overloading 'inject' the
same way we did with the 'change' command.

What's 'inject' supposed to do in the future?

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Blue Swirl]

On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Sat, 30 Apr 2011 09:33:15 +0300
> Blue Swirl <blauwirbel@gmail.com> wrote:
>
>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> > This series introduces the inject-nmi command for QMP, which sends an
>> > NMI to _all_ guest's CPUs.
>> >
>> > Also note that this series changes the human monitor nmi command to use
>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
>> > Please, check patch 3/3 for details.
>>
>> As discussed earlier, please change the QMP version for future
>> expandability so that instead of single command 'inject-nmi', 'inject'
>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>> 'inject' should be added.
>
> I'm not sure I agree with this, because we risky overloading 'inject' the
> same way we did with the 'change' command.
>
> What's 'inject' supposed to do in the future?

Inject other IRQs, for example inject nmi could become an alias to
something like
inject /apic@fee00000:l1int
which would be a shorthand for
raise /apic@fee00000:l1int
lower /apic@fee00000:l1int

I think we only need a registration framework for IRQs and other signals.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> On Sat, 30 Apr 2011 09:33:15 +0300
>> Blue Swirl <blauwirbel@gmail.com> wrote:
>>
>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>> > This series introduces the inject-nmi command for QMP, which sends an
>>> > NMI to _all_ guest's CPUs.
>>> >
>>> > Also note that this series changes the human monitor nmi command to use
>>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
>>> > Please, check patch 3/3 for details.
>>>
>>> As discussed earlier, please change the QMP version for future
>>> expandability so that instead of single command 'inject-nmi', 'inject'
>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>>> 'inject' should be added.
>>
>> I'm not sure I agree with this, because we risky overloading 'inject' the
>> same way we did with the 'change' command.
>>
>> What's 'inject' supposed to do in the future?
>
> Inject other IRQs, for example inject nmi could become an alias to
> something like
> inject /apic@fee00000:l1int
> which would be a shorthand for
> raise /apic@fee00000:l1int
> lower /apic@fee00000:l1int
>
> I think we only need a registration framework for IRQs and other signals.

Yes, we could use nicer infrastructure for modeling IRQs.  No, we
shouldn't reject Lai's work because it doesn't get us there.  Perfect is
the enemy of good.

Pick one:

1. We take inject-nmi now.  Should we get a more general inject command
like the one you envisage later, we can deprecate inject-nmi, and remove
it after a suitable grace time.  Big deal.  We get the special problem
solved now, without really compromising future solutions for the general
problem.

2. We reject inject-nmi now.  The itch Lai tries to scratch remains
unscratched until we get a more general inject command.

2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
it motivates somebody else.  We get the general problem solved sooner.
And maybe I get a pony for my birthday, too.

2b. The general problem remains unsolved along with the special problem.
We get nothing.


[*] He's been trying to give us NMI injection via QMP for five months,
so what's a few months more to him.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Luiz Capitulino]

On Fri, 06 May 2011 11:08:08 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Blue Swirl <blauwirbel@gmail.com> writes:
> 
> > On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >> On Sat, 30 Apr 2011 09:33:15 +0300
> >> Blue Swirl <blauwirbel@gmail.com> wrote:
> >>
> >>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >>> > This series introduces the inject-nmi command for QMP, which sends an
> >>> > NMI to _all_ guest's CPUs.
> >>> >
> >>> > Also note that this series changes the human monitor nmi command to use
> >>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
> >>> > Please, check patch 3/3 for details.
> >>>
> >>> As discussed earlier, please change the QMP version for future
> >>> expandability so that instead of single command 'inject-nmi', 'inject'
> >>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
> >>> 'inject' should be added.
> >>
> >> I'm not sure I agree with this, because we risky overloading 'inject' the
> >> same way we did with the 'change' command.
> >>
> >> What's 'inject' supposed to do in the future?
> >
> > Inject other IRQs, for example inject nmi could become an alias to
> > something like
> > inject /apic@fee00000:l1int
> > which would be a shorthand for
> > raise /apic@fee00000:l1int
> > lower /apic@fee00000:l1int
> >
> > I think we only need a registration framework for IRQs and other signals.
> 
> Yes, we could use nicer infrastructure for modeling IRQs.  No, we
> shouldn't reject Lai's work because it doesn't get us there.  Perfect is
> the enemy of good.
> 
> Pick one:
> 
> 1. We take inject-nmi now.  Should we get a more general inject command
> like the one you envisage later, we can deprecate inject-nmi, and remove
> it after a suitable grace time.  Big deal.  We get the special problem
> solved now, without really compromising future solutions for the general
> problem.

We don't even need to drop it, we just call the new one 'inject' or something
like it and we're set (internally we could re-write 'inject-nmi' to use
'inject' in the future).

> 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
> unscratched until we get a more general inject command.
> 
> 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
> it motivates somebody else.  We get the general problem solved sooner.
> And maybe I get a pony for my birthday, too.
> 
> 2b. The general problem remains unsolved along with the special problem.
> We get nothing.
> 
> 
> [*] He's been trying to give us NMI injection via QMP for five months,
> so what's a few months more to him.
> 

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Blue Swirl]

On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>> On Sat, 30 Apr 2011 09:33:15 +0300
>>> Blue Swirl <blauwirbel@gmail.com> wrote:
>>>
>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>> > This series introduces the inject-nmi command for QMP, which sends an
>>>> > NMI to _all_ guest's CPUs.
>>>> >
>>>> > Also note that this series changes the human monitor nmi command to use
>>>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
>>>> > Please, check patch 3/3 for details.
>>>>
>>>> As discussed earlier, please change the QMP version for future
>>>> expandability so that instead of single command 'inject-nmi', 'inject'
>>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>>>> 'inject' should be added.
>>>
>>> I'm not sure I agree with this, because we risky overloading 'inject' the
>>> same way we did with the 'change' command.
>>>
>>> What's 'inject' supposed to do in the future?
>>
>> Inject other IRQs, for example inject nmi could become an alias to
>> something like
>> inject /apic@fee00000:l1int
>> which would be a shorthand for
>> raise /apic@fee00000:l1int
>> lower /apic@fee00000:l1int
>>
>> I think we only need a registration framework for IRQs and other signals.
>
> Yes, we could use nicer infrastructure for modeling IRQs.  No, we
> shouldn't reject Lai's work because it doesn't get us there.  Perfect is
> the enemy of good.
>
> Pick one:
>
> 1. We take inject-nmi now.  Should we get a more general inject command
> like the one you envisage later, we can deprecate inject-nmi, and remove
> it after a suitable grace time.  Big deal.  We get the special problem
> solved now, without really compromising future solutions for the general
> problem.
>
> 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
> unscratched until we get a more general inject command.
>
> 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
> it motivates somebody else.  We get the general problem solved sooner.
> And maybe I get a pony for my birthday, too.
>
> 2b. The general problem remains unsolved along with the special problem.
> We get nothing.

2c. Don't add full generic IRQ registration and aliases just now but
handle 'inject' with only 'nmi'. That way we introduce no legacy
baggage to the syntax.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Luiz Capitulino]

On Fri, 6 May 2011 18:36:31 +0300
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > Blue Swirl <blauwirbel@gmail.com> writes:
> >
> >> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >>> On Sat, 30 Apr 2011 09:33:15 +0300
> >>> Blue Swirl <blauwirbel@gmail.com> wrote:
> >>>
> >>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >>>> > This series introduces the inject-nmi command for QMP, which sends an
> >>>> > NMI to _all_ guest's CPUs.
> >>>> >
> >>>> > Also note that this series changes the human monitor nmi command to use
> >>>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
> >>>> > Please, check patch 3/3 for details.
> >>>>
> >>>> As discussed earlier, please change the QMP version for future
> >>>> expandability so that instead of single command 'inject-nmi', 'inject'
> >>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
> >>>> 'inject' should be added.
> >>>
> >>> I'm not sure I agree with this, because we risky overloading 'inject' the
> >>> same way we did with the 'change' command.
> >>>
> >>> What's 'inject' supposed to do in the future?
> >>
> >> Inject other IRQs, for example inject nmi could become an alias to
> >> something like
> >> inject /apic@fee00000:l1int
> >> which would be a shorthand for
> >> raise /apic@fee00000:l1int
> >> lower /apic@fee00000:l1int
> >>
> >> I think we only need a registration framework for IRQs and other signals.
> >
> > Yes, we could use nicer infrastructure for modeling IRQs.  No, we
> > shouldn't reject Lai's work because it doesn't get us there.  Perfect is
> > the enemy of good.
> >
> > Pick one:
> >
> > 1. We take inject-nmi now.  Should we get a more general inject command
> > like the one you envisage later, we can deprecate inject-nmi, and remove
> > it after a suitable grace time.  Big deal.  We get the special problem
> > solved now, without really compromising future solutions for the general
> > problem.
> >
> > 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
> > unscratched until we get a more general inject command.
> >
> > 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
> > it motivates somebody else.  We get the general problem solved sooner.
> > And maybe I get a pony for my birthday, too.
> >
> > 2b. The general problem remains unsolved along with the special problem.
> > We get nothing.
> 
> 2c. Don't add full generic IRQ registration and aliases just now but
> handle 'inject' with only 'nmi'. That way we introduce no legacy
> baggage to the syntax.

Can you give an example on how this is supposed to look like?

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 6 May 2011 18:36:31 +0300
> Blue Swirl <blauwirbel@gmail.com> wrote:
>
>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> > Blue Swirl <blauwirbel@gmail.com> writes:
>> >
>> >> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> >>> On Sat, 30 Apr 2011 09:33:15 +0300
>> >>> Blue Swirl <blauwirbel@gmail.com> wrote:
>> >>>
>> >>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> >>>> > This series introduces the inject-nmi command for QMP, which sends an
>> >>>> > NMI to _all_ guest's CPUs.
>> >>>> >
>> >>>> > Also note that this series changes the human monitor nmi command to use
>> >>>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
>> >>>> > Please, check patch 3/3 for details.
>> >>>>
>> >>>> As discussed earlier, please change the QMP version for future
>> >>>> expandability so that instead of single command 'inject-nmi', 'inject'
>> >>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>> >>>> 'inject' should be added.
>> >>>
>> >>> I'm not sure I agree with this, because we risky overloading 'inject' the
>> >>> same way we did with the 'change' command.
>> >>>
>> >>> What's 'inject' supposed to do in the future?
>> >>
>> >> Inject other IRQs, for example inject nmi could become an alias to
>> >> something like
>> >> inject /apic@fee00000:l1int
>> >> which would be a shorthand for
>> >> raise /apic@fee00000:l1int
>> >> lower /apic@fee00000:l1int
>> >>
>> >> I think we only need a registration framework for IRQs and other signals.
>> >
>> > Yes, we could use nicer infrastructure for modeling IRQs.  No, we
>> > shouldn't reject Lai's work because it doesn't get us there.  Perfect is
>> > the enemy of good.
>> >
>> > Pick one:
>> >
>> > 1. We take inject-nmi now.  Should we get a more general inject command
>> > like the one you envisage later, we can deprecate inject-nmi, and remove
>> > it after a suitable grace time.  Big deal.  We get the special problem
>> > solved now, without really compromising future solutions for the general
>> > problem.
>> >
>> > 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
>> > unscratched until we get a more general inject command.
>> >
>> > 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
>> > it motivates somebody else.  We get the general problem solved sooner.
>> > And maybe I get a pony for my birthday, too.
>> >
>> > 2b. The general problem remains unsolved along with the special problem.
>> > We get nothing.
>> 
>> 2c. Don't add full generic IRQ registration and aliases just now but
>> handle 'inject' with only 'nmi'. That way we introduce no legacy
>> baggage to the syntax.
>
> Can you give an example on how this is supposed to look like?

No reply.  When you demand a redesign to generalize a simple feature to
something only you envisage, please explain what exactly you want.
Documentation to stick into qmp-commands.hx would be a start.  Here's
the baseline from Luiz, for your editing convenience.


inject-nmi
----------

Inject an NMI on guest's CPUs.

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.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Blue Swirl]

On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> On Fri, 6 May 2011 18:36:31 +0300
>> Blue Swirl <blauwirbel@gmail.com> wrote:
>>
>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> > Blue Swirl <blauwirbel@gmail.com> writes:
>>> >
>>> >> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>> >>> On Sat, 30 Apr 2011 09:33:15 +0300
>>> >>> Blue Swirl <blauwirbel@gmail.com> wrote:
>>> >>>
>>> >>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>> >>>> > This series introduces the inject-nmi command for QMP, which sends an
>>> >>>> > NMI to _all_ guest's CPUs.
>>> >>>> >
>>> >>>> > Also note that this series changes the human monitor nmi command to use
>>> >>>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
>>> >>>> > Please, check patch 3/3 for details.
>>> >>>>
>>> >>>> As discussed earlier, please change the QMP version for future
>>> >>>> expandability so that instead of single command 'inject-nmi', 'inject'
>>> >>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>>> >>>> 'inject' should be added.
>>> >>>
>>> >>> I'm not sure I agree with this, because we risky overloading 'inject' the
>>> >>> same way we did with the 'change' command.
>>> >>>
>>> >>> What's 'inject' supposed to do in the future?
>>> >>
>>> >> Inject other IRQs, for example inject nmi could become an alias to
>>> >> something like
>>> >> inject /apic@fee00000:l1int
>>> >> which would be a shorthand for
>>> >> raise /apic@fee00000:l1int
>>> >> lower /apic@fee00000:l1int
>>> >>
>>> >> I think we only need a registration framework for IRQs and other signals.
>>> >
>>> > Yes, we could use nicer infrastructure for modeling IRQs.  No, we
>>> > shouldn't reject Lai's work because it doesn't get us there.  Perfect is
>>> > the enemy of good.
>>> >
>>> > Pick one:
>>> >
>>> > 1. We take inject-nmi now.  Should we get a more general inject command
>>> > like the one you envisage later, we can deprecate inject-nmi, and remove
>>> > it after a suitable grace time.  Big deal.  We get the special problem
>>> > solved now, without really compromising future solutions for the general
>>> > problem.
>>> >
>>> > 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
>>> > unscratched until we get a more general inject command.
>>> >
>>> > 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
>>> > it motivates somebody else.  We get the general problem solved sooner.
>>> > And maybe I get a pony for my birthday, too.
>>> >
>>> > 2b. The general problem remains unsolved along with the special problem.
>>> > We get nothing.
>>>
>>> 2c. Don't add full generic IRQ registration and aliases just now but
>>> handle 'inject' with only 'nmi'. That way we introduce no legacy
>>> baggage to the syntax.
>>
>> Can you give an example on how this is supposed to look like?
>
> No reply.  When you demand a redesign to generalize a simple feature to
> something only you envisage, please explain what exactly you want.
> Documentation to stick into qmp-commands.hx would be a start.  Here's
> the baseline from Luiz, for your editing convenience.
>
>
> inject-nmi
> ----------
>
> Inject an NMI on guest's CPUs.
>
> 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.

I think I explained it many times, but let's try again.

inject
----------

Inject a signal on guest machine.

Arguments: signal name.

Example:

-> { "execute": "inject",
"arguments": { "signal": "nmi" } }
<- { "return": {} }

-> { "execute": "inject",
"arguments": { "signal": "/apic@fee00000:l1int" } }
<- { "return": {} }

Note: the set of signals supported depends on the CPU architecture and
board type, unknown or unsupported names will
     return "Unsupported" error.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Luiz Capitulino]

On Thu, 26 May 2011 22:23:10 +0300
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >> On Fri, 6 May 2011 18:36:31 +0300
> >> Blue Swirl <blauwirbel@gmail.com> wrote:
> >>
> >>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> >>> > Blue Swirl <blauwirbel@gmail.com> writes:
> >>> >
> >>> >> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >>> >>> On Sat, 30 Apr 2011 09:33:15 +0300
> >>> >>> Blue Swirl <blauwirbel@gmail.com> wrote:
> >>> >>>
> >>> >>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >>> >>>> > This series introduces the inject-nmi command for QMP, which sends an
> >>> >>>> > NMI to _all_ guest's CPUs.
> >>> >>>> >
> >>> >>>> > Also note that this series changes the human monitor nmi command to use
> >>> >>>> > the QMP implementation, which means that it now has a DIFFERENT behavior.
> >>> >>>> > Please, check patch 3/3 for details.
> >>> >>>>
> >>> >>>> As discussed earlier, please change the QMP version for future
> >>> >>>> expandability so that instead of single command 'inject-nmi', 'inject'
> >>> >>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
> >>> >>>> 'inject' should be added.
> >>> >>>
> >>> >>> I'm not sure I agree with this, because we risky overloading 'inject' the
> >>> >>> same way we did with the 'change' command.
> >>> >>>
> >>> >>> What's 'inject' supposed to do in the future?
> >>> >>
> >>> >> Inject other IRQs, for example inject nmi could become an alias to
> >>> >> something like
> >>> >> inject /apic@fee00000:l1int
> >>> >> which would be a shorthand for
> >>> >> raise /apic@fee00000:l1int
> >>> >> lower /apic@fee00000:l1int
> >>> >>
> >>> >> I think we only need a registration framework for IRQs and other signals.
> >>> >
> >>> > Yes, we could use nicer infrastructure for modeling IRQs.  No, we
> >>> > shouldn't reject Lai's work because it doesn't get us there.  Perfect is
> >>> > the enemy of good.
> >>> >
> >>> > Pick one:
> >>> >
> >>> > 1. We take inject-nmi now.  Should we get a more general inject command
> >>> > like the one you envisage later, we can deprecate inject-nmi, and remove
> >>> > it after a suitable grace time.  Big deal.  We get the special problem
> >>> > solved now, without really compromising future solutions for the general
> >>> > problem.
> >>> >
> >>> > 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
> >>> > unscratched until we get a more general inject command.
> >>> >
> >>> > 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
> >>> > it motivates somebody else.  We get the general problem solved sooner.
> >>> > And maybe I get a pony for my birthday, too.
> >>> >
> >>> > 2b. The general problem remains unsolved along with the special problem.
> >>> > We get nothing.
> >>>
> >>> 2c. Don't add full generic IRQ registration and aliases just now but
> >>> handle 'inject' with only 'nmi'. That way we introduce no legacy
> >>> baggage to the syntax.
> >>
> >> Can you give an example on how this is supposed to look like?
> >
> > No reply.  When you demand a redesign to generalize a simple feature to
> > something only you envisage, please explain what exactly you want.
> > Documentation to stick into qmp-commands.hx would be a start.  Here's
> > the baseline from Luiz, for your editing convenience.
> >
> >
> > inject-nmi
> > ----------
> >
> > Inject an NMI on guest's CPUs.
> >
> > 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.
> 
> I think I explained it many times, but let's try again.
> 
> inject
> ----------
> 
> Inject a signal on guest machine.
> 
> Arguments: signal name.
> 
> Example:
> 
> -> { "execute": "inject",
> "arguments": { "signal": "nmi" } }
> <- { "return": {} }
> 
> -> { "execute": "inject",
> "arguments": { "signal": "/apic@fee00000:l1int" } }
> <- { "return": {} }

Shouldn't this be broken into device and signal (or pin) arguments?

> Note: the set of signals supported depends on the CPU architecture and
> board type, unknown or unsupported names will
>      return "Unsupported" error.

Unsuported error != bad usage error.

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Anthony Liguori]

On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
> On Thu, 26 May 2011 22:23:10 +0300
> Blue Swirl<blauwirbel@gmail.com>  wrote:
>
>> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>
>>>> On Fri, 6 May 2011 18:36:31 +0300
>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>>>
>>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>>>
>>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino<lcapitulino@redhat.com>  wrote:
>>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300
>>>>>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>>>>>>>
>>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino<lcapitulino@redhat.com>  wrote:
>>>>>>>>>> This series introduces the inject-nmi command for QMP, which sends an
>>>>>>>>>> NMI to _all_ guest's CPUs.
>>>>>>>>>>
>>>>>>>>>> Also note that this series changes the human monitor nmi command to use
>>>>>>>>>> the QMP implementation, which means that it now has a DIFFERENT behavior.
>>>>>>>>>> Please, check patch 3/3 for details.
>>>>>>>>>
>>>>>>>>> As discussed earlier, please change the QMP version for future
>>>>>>>>> expandability so that instead of single command 'inject-nmi', 'inject'
>>>>>>>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>>>>>>>>> 'inject' should be added.
>>>>>>>>
>>>>>>>> I'm not sure I agree with this, because we risky overloading 'inject' the
>>>>>>>> same way we did with the 'change' command.
>>>>>>>>
>>>>>>>> What's 'inject' supposed to do in the future?
>>>>>>>
>>>>>>> Inject other IRQs, for example inject nmi could become an alias to
>>>>>>> something like
>>>>>>> inject /apic@fee00000:l1int
>>>>>>> which would be a shorthand for
>>>>>>> raise /apic@fee00000:l1int
>>>>>>> lower /apic@fee00000:l1int
>>>>>>>
>>>>>>> I think we only need a registration framework for IRQs and other signals.
>>>>>>
>>>>>> Yes, we could use nicer infrastructure for modeling IRQs.  No, we
>>>>>> shouldn't reject Lai's work because it doesn't get us there.  Perfect is
>>>>>> the enemy of good.
>>>>>>
>>>>>> Pick one:
>>>>>>
>>>>>> 1. We take inject-nmi now.  Should we get a more general inject command
>>>>>> like the one you envisage later, we can deprecate inject-nmi, and remove
>>>>>> it after a suitable grace time.  Big deal.  We get the special problem
>>>>>> solved now, without really compromising future solutions for the general
>>>>>> problem.
>>>>>>
>>>>>> 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
>>>>>> unscratched until we get a more general inject command.
>>>>>>
>>>>>> 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
>>>>>> it motivates somebody else.  We get the general problem solved sooner.
>>>>>> And maybe I get a pony for my birthday, too.
>>>>>>
>>>>>> 2b. The general problem remains unsolved along with the special problem.
>>>>>> We get nothing.
>>>>>
>>>>> 2c. Don't add full generic IRQ registration and aliases just now but
>>>>> handle 'inject' with only 'nmi'. That way we introduce no legacy
>>>>> baggage to the syntax.
>>>>
>>>> Can you give an example on how this is supposed to look like?
>>>
>>> No reply.  When you demand a redesign to generalize a simple feature to
>>> something only you envisage, please explain what exactly you want.
>>> Documentation to stick into qmp-commands.hx would be a start.  Here's
>>> the baseline from Luiz, for your editing convenience.
>>>
>>>
>>> inject-nmi
>>> ----------
>>>
>>> Inject an NMI on guest's CPUs.
>>>
>>> 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.
>>
>> I think I explained it many times, but let's try again.
>>
>> inject
>> ----------
>>
>> Inject a signal on guest machine.
>>
>> Arguments: signal name.
>>
>> Example:
>>
>> ->  { "execute": "inject",
>> "arguments": { "signal": "nmi" } }
>> <- { "return": {} }
>>
>> ->  { "execute": "inject",
>> "arguments": { "signal": "/apic@fee00000:l1int" } }
>> <- { "return": {} }
>
> Shouldn't this be broken into device and signal (or pin) arguments?


I dislike this approach strongly.

Overloading verbs to have multiple meanings is a bad thing for QMP.  It 
means less type safety.  Think of a C interface:

inject_nmi() <- good
inject_nim() <- compile error

inject("nmi") <- good
inject("nim") <- runtime error

Not to mention that "inject" doesn't mean "raise and then lower a pin". 
  Inject means insert or put in.

I'm not opposed to being able to have a way to raise/lower a qemu_irq, 
but (a) that's orthogonal to this operation (b) we should design that 
interface properly.  b means that we should be able to enumerate pins, 
raise and lower pins, and pulse pins.

Regards,

Anthony Liguori

>> Note: the set of signals supported depends on the CPU architecture and
>> board type, unknown or unsupported names will
>>       return "Unsupported" error.
>
> Unsuported error != bad usage error.
>

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Luiz Capitulino]

On Fri, 27 May 2011 09:55:05 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
> > On Thu, 26 May 2011 22:23:10 +0300
> > Blue Swirl<blauwirbel@gmail.com>  wrote:
> >
> >> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster<armbru@redhat.com>  wrote:
> >>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
> >>>
> >>>> On Fri, 6 May 2011 18:36:31 +0300
> >>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
> >>>>
> >>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster<armbru@redhat.com>  wrote:
> >>>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
> >>>>>>
> >>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino<lcapitulino@redhat.com>  wrote:
> >>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300
> >>>>>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
> >>>>>>>>
> >>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino<lcapitulino@redhat.com>  wrote:
> >>>>>>>>>> This series introduces the inject-nmi command for QMP, which sends an
> >>>>>>>>>> NMI to _all_ guest's CPUs.
> >>>>>>>>>>
> >>>>>>>>>> Also note that this series changes the human monitor nmi command to use
> >>>>>>>>>> the QMP implementation, which means that it now has a DIFFERENT behavior.
> >>>>>>>>>> Please, check patch 3/3 for details.
> >>>>>>>>>
> >>>>>>>>> As discussed earlier, please change the QMP version for future
> >>>>>>>>> expandability so that instead of single command 'inject-nmi', 'inject'
> >>>>>>>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
> >>>>>>>>> 'inject' should be added.
> >>>>>>>>
> >>>>>>>> I'm not sure I agree with this, because we risky overloading 'inject' the
> >>>>>>>> same way we did with the 'change' command.
> >>>>>>>>
> >>>>>>>> What's 'inject' supposed to do in the future?
> >>>>>>>
> >>>>>>> Inject other IRQs, for example inject nmi could become an alias to
> >>>>>>> something like
> >>>>>>> inject /apic@fee00000:l1int
> >>>>>>> which would be a shorthand for
> >>>>>>> raise /apic@fee00000:l1int
> >>>>>>> lower /apic@fee00000:l1int
> >>>>>>>
> >>>>>>> I think we only need a registration framework for IRQs and other signals.
> >>>>>>
> >>>>>> Yes, we could use nicer infrastructure for modeling IRQs.  No, we
> >>>>>> shouldn't reject Lai's work because it doesn't get us there.  Perfect is
> >>>>>> the enemy of good.
> >>>>>>
> >>>>>> Pick one:
> >>>>>>
> >>>>>> 1. We take inject-nmi now.  Should we get a more general inject command
> >>>>>> like the one you envisage later, we can deprecate inject-nmi, and remove
> >>>>>> it after a suitable grace time.  Big deal.  We get the special problem
> >>>>>> solved now, without really compromising future solutions for the general
> >>>>>> problem.
> >>>>>>
> >>>>>> 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
> >>>>>> unscratched until we get a more general inject command.
> >>>>>>
> >>>>>> 2a. Rejection "motivates" Lai to solve the general problem[*].  Or maybe
> >>>>>> it motivates somebody else.  We get the general problem solved sooner.
> >>>>>> And maybe I get a pony for my birthday, too.
> >>>>>>
> >>>>>> 2b. The general problem remains unsolved along with the special problem.
> >>>>>> We get nothing.
> >>>>>
> >>>>> 2c. Don't add full generic IRQ registration and aliases just now but
> >>>>> handle 'inject' with only 'nmi'. That way we introduce no legacy
> >>>>> baggage to the syntax.
> >>>>
> >>>> Can you give an example on how this is supposed to look like?
> >>>
> >>> No reply.  When you demand a redesign to generalize a simple feature to
> >>> something only you envisage, please explain what exactly you want.
> >>> Documentation to stick into qmp-commands.hx would be a start.  Here's
> >>> the baseline from Luiz, for your editing convenience.
> >>>
> >>>
> >>> inject-nmi
> >>> ----------
> >>>
> >>> Inject an NMI on guest's CPUs.
> >>>
> >>> 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.
> >>
> >> I think I explained it many times, but let's try again.
> >>
> >> inject
> >> ----------
> >>
> >> Inject a signal on guest machine.
> >>
> >> Arguments: signal name.
> >>
> >> Example:
> >>
> >> ->  { "execute": "inject",
> >> "arguments": { "signal": "nmi" } }
> >> <- { "return": {} }
> >>
> >> ->  { "execute": "inject",
> >> "arguments": { "signal": "/apic@fee00000:l1int" } }
> >> <- { "return": {} }
> >
> > Shouldn't this be broken into device and signal (or pin) arguments?
> 
> 
> I dislike this approach strongly.
> 
> Overloading verbs to have multiple meanings is a bad thing for QMP.  It 
> means less type safety.  Think of a C interface:
> 
> inject_nmi() <- good
> inject_nim() <- compile error
> 
> inject("nmi") <- good
> inject("nim") <- runtime error
> 
> Not to mention that "inject" doesn't mean "raise and then lower a pin". 
>   Inject means insert or put in.
> 
> I'm not opposed to being able to have a way to raise/lower a qemu_irq, 
> but (a) that's orthogonal to this operation (b) we should design that 
> interface properly.  b means that we should be able to enumerate pins, 
> raise and lower pins, and pulse pins.

So, would you be in favor of merging the current series as it stands
currently and design this new interface as a new command?

> 
> Regards,
> 
> Anthony Liguori
> 
> >> Note: the set of signals supported depends on the CPU architecture and
> >> board type, unknown or unsupported names will
> >>       return "Unsupported" error.
> >
> > Unsuported error != bad usage error.
> >
> 

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 27 May 2011 09:55:05 -0500
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>> On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
>> > On Thu, 26 May 2011 22:23:10 +0300
>> > Blue Swirl<blauwirbel@gmail.com>  wrote:
>> >
>> >> I think I explained it many times, but let's try again.

Thanks!

>> >>
>> >> inject
>> >> ----------
>> >>
>> >> Inject a signal on guest machine.
>> >>
>> >> Arguments: signal name.
>> >>
>> >> Example:
>> >>
>> >> ->  { "execute": "inject",
>> >> "arguments": { "signal": "nmi" } }
>> >> <- { "return": {} }
>> >>
>> >> ->  { "execute": "inject",
>> >> "arguments": { "signal": "/apic@fee00000:l1int" } }
>> >> <- { "return": {} }
>> >
>> > Shouldn't this be broken into device and signal (or pin) arguments?
>> 
>> 
>> I dislike this approach strongly.
>> 
>> Overloading verbs to have multiple meanings is a bad thing for QMP.  It 
>> means less type safety.  Think of a C interface:
>> 
>> inject_nmi() <- good
>> inject_nim() <- compile error
>> 
>> inject("nmi") <- good
>> inject("nim") <- runtime error

Not sure how much mileage we'll get out of static typing in QMP, but
overloading commands for dissimilar tasks is bad taste.  One ugly
bastard like "change" is enough.  Thus, this "inject" command should
always be limited to irq-like operands.

>> Not to mention that "inject" doesn't mean "raise and then lower a pin". 
>>   Inject means insert or put in.

Yes.  Like "inject a fault".

>> I'm not opposed to being able to have a way to raise/lower a qemu_irq, 
>> but (a) that's orthogonal to this operation (b) we should design that 
>> interface properly.  b means that we should be able to enumerate pins, 
>> raise and lower pins, and pulse pins.

Once qdev models pins nicely, a command to manipulate them shouldn't be
hard.

> So, would you be in favor of merging the current series as it stands
> currently and design this new interface as a new command?

I recommend to commit the sucker already.  It's simple, and it solves a
problem that's relevant enough for Lai to pursue it for *months*, and
starting over now is just not worth it.

[...]

Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command

[Blue Swirl]

On Fri, May 27, 2011 at 5:55 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
>>
>> On Thu, 26 May 2011 22:23:10 +0300
>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>
>>> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster<armbru@redhat.com>
>>>  wrote:
>>>>
>>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>>
>>>>> On Fri, 6 May 2011 18:36:31 +0300
>>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>>>>
>>>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster<armbru@redhat.com>
>>>>>>  wrote:
>>>>>>>
>>>>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>>>>
>>>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz
>>>>>>>> Capitulino<lcapitulino@redhat.com>  wrote:
>>>>>>>>>
>>>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300
>>>>>>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>>>>>>>>
>>>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz
>>>>>>>>>> Capitulino<lcapitulino@redhat.com>  wrote:
>>>>>>>>>>>
>>>>>>>>>>> This series introduces the inject-nmi command for QMP, which
>>>>>>>>>>> sends an
>>>>>>>>>>> NMI to _all_ guest's CPUs.
>>>>>>>>>>>
>>>>>>>>>>> Also note that this series changes the human monitor nmi command
>>>>>>>>>>> to use
>>>>>>>>>>> the QMP implementation, which means that it now has a DIFFERENT
>>>>>>>>>>> behavior.
>>>>>>>>>>> Please, check patch 3/3 for details.
>>>>>>>>>>
>>>>>>>>>> As discussed earlier, please change the QMP version for future
>>>>>>>>>> expandability so that instead of single command 'inject-nmi',
>>>>>>>>>> 'inject'
>>>>>>>>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
>>>>>>>>>> 'inject' should be added.
>>>>>>>>>
>>>>>>>>> I'm not sure I agree with this, because we risky overloading
>>>>>>>>> 'inject' the
>>>>>>>>> same way we did with the 'change' command.
>>>>>>>>>
>>>>>>>>> What's 'inject' supposed to do in the future?
>>>>>>>>
>>>>>>>> Inject other IRQs, for example inject nmi could become an alias to
>>>>>>>> something like
>>>>>>>> inject /apic@fee00000:l1int
>>>>>>>> which would be a shorthand for
>>>>>>>> raise /apic@fee00000:l1int
>>>>>>>> lower /apic@fee00000:l1int
>>>>>>>>
>>>>>>>> I think we only need a registration framework for IRQs and other
>>>>>>>> signals.
>>>>>>>
>>>>>>> Yes, we could use nicer infrastructure for modeling IRQs.  No, we
>>>>>>> shouldn't reject Lai's work because it doesn't get us there.  Perfect
>>>>>>> is
>>>>>>> the enemy of good.
>>>>>>>
>>>>>>> Pick one:
>>>>>>>
>>>>>>> 1. We take inject-nmi now.  Should we get a more general inject
>>>>>>> command
>>>>>>> like the one you envisage later, we can deprecate inject-nmi, and
>>>>>>> remove
>>>>>>> it after a suitable grace time.  Big deal.  We get the special
>>>>>>> problem
>>>>>>> solved now, without really compromising future solutions for the
>>>>>>> general
>>>>>>> problem.
>>>>>>>
>>>>>>> 2. We reject inject-nmi now.  The itch Lai tries to scratch remains
>>>>>>> unscratched until we get a more general inject command.
>>>>>>>
>>>>>>> 2a. Rejection "motivates" Lai to solve the general problem[*].  Or
>>>>>>> maybe
>>>>>>> it motivates somebody else.  We get the general problem solved
>>>>>>> sooner.
>>>>>>> And maybe I get a pony for my birthday, too.
>>>>>>>
>>>>>>> 2b. The general problem remains unsolved along with the special
>>>>>>> problem.
>>>>>>> We get nothing.
>>>>>>
>>>>>> 2c. Don't add full generic IRQ registration and aliases just now but
>>>>>> handle 'inject' with only 'nmi'. That way we introduce no legacy
>>>>>> baggage to the syntax.
>>>>>
>>>>> Can you give an example on how this is supposed to look like?
>>>>
>>>> No reply.  When you demand a redesign to generalize a simple feature to
>>>> something only you envisage, please explain what exactly you want.
>>>> Documentation to stick into qmp-commands.hx would be a start.  Here's
>>>> the baseline from Luiz, for your editing convenience.
>>>>
>>>>
>>>> inject-nmi
>>>> ----------
>>>>
>>>> Inject an NMI on guest's CPUs.
>>>>
>>>> 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.
>>>
>>> I think I explained it many times, but let's try again.
>>>
>>> inject
>>> ----------
>>>
>>> Inject a signal on guest machine.
>>>
>>> Arguments: signal name.
>>>
>>> Example:
>>>
>>> ->  { "execute": "inject",
>>> "arguments": { "signal": "nmi" } }
>>> <- { "return": {} }
>>>
>>> ->  { "execute": "inject",
>>> "arguments": { "signal": "/apic@fee00000:l1int" } }
>>> <- { "return": {} }
>>
>> Shouldn't this be broken into device and signal (or pin) arguments?
>
>
> I dislike this approach strongly.
>
> Overloading verbs to have multiple meanings is a bad thing for QMP.  It
> means less type safety.  Think of a C interface:
>
> inject_nmi() <- good
> inject_nim() <- compile error
>
> inject("nmi") <- good
> inject("nim") <- runtime error

But we don't have change_floppy, change_ide1-cd0 etc. Why should there
be a special command in this case?

> Not to mention that "inject" doesn't mean "raise and then lower a pin".
>  Inject means insert or put in.
>
> I'm not opposed to being able to have a way to raise/lower a qemu_irq, but
> (a) that's orthogonal to this operation (b) we should design that interface
> properly.  b means that we should be able to enumerate pins, raise and lower
> pins, and pulse pins.

OK.

pulse
----------

Pulse a signal on guest machine.

Arguments: signal name.

Example:

-> { "execute": "pulse",
"arguments": { "signal": "nmi" } }
<- { "return": {} }

-> { "execute": "inject",
"arguments": { "signal": "/apic@fee00000:l1int" } }
<- { "return": {} }

Note: the set of signals supported depends on the CPU architecture and
board type, unknown or unsupported names will
    return "Unsupported" error.

The second example could be implemented when the pin registration is
in place. For NMI, some specific hack could be introduced at this
point which could be replaced later by adding convenience names for
common signals. That way the interface would remain stable.