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<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.

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.