From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36528 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PscMq-0000AO-CX for qemu-devel@nongnu.org; Thu, 24 Feb 2011 09:40:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PscMo-000410-7M for qemu-devel@nongnu.org; Thu, 24 Feb 2011 09:40:32 -0500 Received: from e7.ny.us.ibm.com ([32.97.182.137]:52823) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PscMo-00040m-1y for qemu-devel@nongnu.org; Thu, 24 Feb 2011 09:40:30 -0500 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by e7.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p1OEKCD0005635 for ; Thu, 24 Feb 2011 09:20:12 -0500 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p1OEeQ8d2715862 for ; Thu, 24 Feb 2011 09:40:28 -0500 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p1OEePC1008350 for ; Thu, 24 Feb 2011 11:40:26 -0300 Message-ID: <4D666DE5.9090800@linux.vnet.ibm.com> Date: Thu, 24 Feb 2011 08:40:37 -0600 From: Anthony Liguori MIME-Version: 1.0 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> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Lai Jiangshan , Lai Jiangshan , kvm@vger.kernel.org, qemu-devel@nongnu.org, Luiz Capitulino , Avi Kivity On 02/24/2011 02:33 AM, Markus Armbruster wrote: > 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. > Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to. > 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. > I'm quite happy with the error design we have today. The only problem is that we don't propagate errors in a sane way but I've got that all but fixed in my qapi tree. >> 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. > That's fine, then we should do two commands. Think of it from the perspective of the client. This appears as: in C: qmp_inject_nmi(sess, false, 0, &err); in Python: sess.inject_nmi() The first example doesn't tell you at all what's happening. The second API does look really nice until you see the following later: sess.inject_nmi(0) What's the difference between these two functions? You might say this is bad form and that an explicit named argument should be given but I wouldn't count on it. Having two commands, nmi_inject and nmi_inject_on_cpu, would result in a much more readable API in both C and Python: in C: qmp_nmi_inject(sess, &err); qmp_nmi_inject_on_cpu(sess, 3, &err); in Python: sess.nmi_inject() sess.nmi_inject_on_cpu(3) Regards, Anthony Liguori