From mboxrd@z Thu Jan 1 00:00:00 1970 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject Date: Thu, 24 Feb 2011 09:33:21 +0100 Message-ID: References: <4D412AD2.1090509@cn.fujitsu.com> <4D655F32.30307@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Lai Jiangshan , aliguori@us.ibm.com, Lai Jiangshan , kvm@vger.kernel.org, qemu-devel@nongnu.org, Avi Kivity , Luiz Capitulino To: Anthony Liguori Return-path: Received: from mx1.redhat.com ([209.132.183.28]:59139 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751275Ab1BXId7 (ORCPT ); Thu, 24 Feb 2011 03:33:59 -0500 In-Reply-To: <4D655F32.30307@codemonkey.ws> (Anthony Liguori's message of "Wed, 23 Feb 2011 13:25:38 -0600") Sender: kvm-owner@vger.kernel.org List-ID: Anthony Liguori writes: > On 01/27/2011 02:20 AM, Lai Jiangshan wrote: >> Make we can inject NMI via qemu-monitor-protocol. >> We use "inject-nmi" for the qmp command name, the meaning is clearer. >> >> Signed-off-by: Lai Jiangshan >> --- >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index ec1a4db..e763bf9 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -725,7 +725,8 @@ ETEXI >> .params = "[cpu]", >> .help = "Inject an NMI on all CPUs if no argument is given, " >> "otherwise inject it on the specified 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 387b020..1b1c0ba 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -2542,7 +2542,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; >> @@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict) >> if (!qdict_haskey(qdict, "cpu-index")) { >> for (env = first_cpu; env != NULL; env = env->next_cpu) >> cpu_interrupt(env, CPU_INTERRUPT_NMI); >> - return; >> + return 0; >> } >> >> cpu_index = qdict_get_int(qdict, "cpu-index"); >> @@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict) >> kvm_inject_interrupt(env, CPU_INTERRUPT_NMI); >> else >> cpu_interrupt(env, CPU_INTERRUPT_NMI); >> - break; >> + return 0; >> } >> + >> + return -1; >> } >> #endif >> >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 56c4d8b..a887dd5 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -429,6 +429,34 @@ Example: >> >> EQMP >> >> +#if defined(TARGET_I386) >> + { >> + .name = "inject-nmi", >> + .args_type = "cpu-index:i?", >> + .params = "[cpu]", >> + .help = "Inject an NMI on all CPUs if no argument is given, " >> + "otherwise inject it on the specified CPU", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_inject_nmi, >> + }, >> +#endif >> +SQMP >> +inject-nmi >> +---------- >> + >> +Inject an NMI on all CPUs or the given CPU (x86 only). >> + >> +Arguments: >> + >> +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional) >> + >> +Example: >> + >> +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } >> +<- { "return": {} } >> > > Please describe all expected errors. Quoting qmp-commands.hx: 3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key) Indeed, not a single error is documented there. This is intentional. Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now. > Don't hide this command for > !defined(TARGET_I386), instead have it throw an error in the > implementation. Works for me. > Don't have commands that multiple behavior based on the presence or > absence of arguments. Make it take a list of cpus if you want the > ability to inject the NMI to more than one CPU. Having optional arguments is fine. It's good taste to give them "default semantics", i.e. "no argument" is shorthand for one specific argument value. Luiz already pointed to the thread where we discussed this command before. Executive summary: * Real hardware's NMI button injects all CPUs. This is the primary use case. * Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case. Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP. * No other use cases have been presented. Therefore, the "list of CPUs" idea was shot down as overly general. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46847 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PsWdv-0000rO-RS for qemu-devel@nongnu.org; Thu, 24 Feb 2011 03:33:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PsWdq-0004Kt-T3 for qemu-devel@nongnu.org; Thu, 24 Feb 2011 03:33:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35239) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PsWdq-0004Ka-FP for qemu-devel@nongnu.org; Thu, 24 Feb 2011 03:33:42 -0500 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject References: <4D412AD2.1090509@cn.fujitsu.com> <4D655F32.30307@codemonkey.ws> Date: Thu, 24 Feb 2011 09:33:21 +0100 In-Reply-To: <4D655F32.30307@codemonkey.ws> (Anthony Liguori's message of "Wed, 23 Feb 2011 13:25:38 -0600") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: aliguori@us.ibm.com, Lai Jiangshan , Lai Jiangshan , kvm@vger.kernel.org, qemu-devel@nongnu.org, Luiz Capitulino , Avi Kivity Anthony Liguori writes: > On 01/27/2011 02:20 AM, Lai Jiangshan wrote: >> Make we can inject NMI via qemu-monitor-protocol. >> We use "inject-nmi" for the qmp command name, the meaning is clearer. >> >> Signed-off-by: Lai Jiangshan >> --- >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index ec1a4db..e763bf9 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -725,7 +725,8 @@ ETEXI >> .params = "[cpu]", >> .help = "Inject an NMI on all CPUs if no argument is given, " >> "otherwise inject it on the specified 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 387b020..1b1c0ba 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -2542,7 +2542,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; >> @@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict) >> if (!qdict_haskey(qdict, "cpu-index")) { >> for (env = first_cpu; env != NULL; env = env->next_cpu) >> cpu_interrupt(env, CPU_INTERRUPT_NMI); >> - return; >> + return 0; >> } >> >> cpu_index = qdict_get_int(qdict, "cpu-index"); >> @@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict) >> kvm_inject_interrupt(env, CPU_INTERRUPT_NMI); >> else >> cpu_interrupt(env, CPU_INTERRUPT_NMI); >> - break; >> + return 0; >> } >> + >> + return -1; >> } >> #endif >> >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 56c4d8b..a887dd5 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -429,6 +429,34 @@ Example: >> >> EQMP >> >> +#if defined(TARGET_I386) >> + { >> + .name = "inject-nmi", >> + .args_type = "cpu-index:i?", >> + .params = "[cpu]", >> + .help = "Inject an NMI on all CPUs if no argument is given, " >> + "otherwise inject it on the specified CPU", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_inject_nmi, >> + }, >> +#endif >> +SQMP >> +inject-nmi >> +---------- >> + >> +Inject an NMI on all CPUs or the given CPU (x86 only). >> + >> +Arguments: >> + >> +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional) >> + >> +Example: >> + >> +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } >> +<- { "return": {} } >> > > Please describe all expected errors. Quoting qmp-commands.hx: 3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key) Indeed, not a single error is documented there. This is intentional. Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now. > Don't hide this command for > !defined(TARGET_I386), instead have it throw an error in the > implementation. Works for me. > Don't have commands that multiple behavior based on the presence or > absence of arguments. Make it take a list of cpus if you want the > ability to inject the NMI to more than one CPU. Having optional arguments is fine. It's good taste to give them "default semantics", i.e. "no argument" is shorthand for one specific argument value. Luiz already pointed to the thread where we discussed this command before. Executive summary: * Real hardware's NMI button injects all CPUs. This is the primary use case. * Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case. Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP. * No other use cases have been presented. Therefore, the "list of CPUs" idea was shot down as overly general.