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

[Luiz Capitulino]

On Fri, 27 May 2011 09:55:05 -0500
Anthony Liguori <address@hidden> wrote:

> On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
> > On Thu, 26 May 2011 22:23:10 +0300
> > Blue Swirl<address@hidden>  wrote:
> >
> >> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster<address@hidden>  wrote:
> >>> Luiz Capitulino<address@hidden>  writes:
> >>>
> >>>> On Fri, 6 May 2011 18:36:31 +0300
> >>>> Blue Swirl<address@hidden>  wrote:
> >>>>
> >>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster<address@hidden>  
> >>>>> wrote:
> >>>>>> Blue Swirl<address@hidden>  writes:
> >>>>>>
> >>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino<address@hidden>  
> >>>>>>> wrote:
> >>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300
> >>>>>>>> Blue Swirl<address@hidden>  wrote:
> >>>>>>>>
> >>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino<address@hidden>  
> >>>>>>>>> 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 /address@hidden:l1int
> >>>>>>> which would be a shorthand for
> >>>>>>> raise /address@hidden:l1int
> >>>>>>> lower /address@hidden: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": "/address@hidden: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.
> >
>