From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35270) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QPyRU-0001g0-FW for qemu-devel@nongnu.org; Fri, 27 May 2011 10:55:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QPyRT-0001pJ-CG for qemu-devel@nongnu.org; Fri, 27 May 2011 10:55:12 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:57516) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QPyRT-0001p6-8r for qemu-devel@nongnu.org; Fri, 27 May 2011 10:55:11 -0400 Received: by ywl41 with SMTP id 41so929485ywl.4 for ; Fri, 27 May 2011 07:55:09 -0700 (PDT) Message-ID: <4DDFBB49.5050505@codemonkey.ws> Date: Fri, 27 May 2011 09:55:05 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1304116821-18201-1-git-send-email-lcapitulino@redhat.com> <20110502125746.5ef8bc78@doriath> <20110509103210.337ea7af@doriath> <20110527110455.1b42df32@doriath> In-Reply-To: <20110527110455.1b42df32@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/3]: QMP: Introduce inject-nmi command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Blue Swirl , qemu-devel@nongnu.org, laijs@cn.fujitsu.com, Markus Armbruster On 05/27/2011 09:04 AM, Luiz Capitulino wrote: > On Thu, 26 May 2011 22:23:10 +0300 > Blue Swirl wrote: > >> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster wrote: >>> Luiz Capitulino writes: >>> >>>> On Fri, 6 May 2011 18:36:31 +0300 >>>> Blue Swirl wrote: >>>> >>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster wrote: >>>>>> Blue Swirl writes: >>>>>> >>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino wrote: >>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300 >>>>>>>> Blue Swirl wrote: >>>>>>>> >>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino 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. >