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

[Blue Swirl]

On Fri, May 27, 2011 at 5:55 PM, 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

But we don't have change_floppy, change_ide1-cd0 etc. Why should there
be a special command in this case?

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

OK.

pulse
----------

Pulse a signal on guest machine.

Arguments: signal name.

Example:

-> { "execute": "pulse",
"arguments": { "signal": "nmi" } }
<- { "return": {} }

-> { "execute": "inject",
"arguments": { "signal": "/address@hidden:l1int" } }
<- { "return": {} }

Note: the set of signals supported depends on the CPU architecture and
board type, unknown or unsupported names will
    return "Unsupported" error.

The second example could be implemented when the pin registration is
in place. For NMI, some specific hack could be introduced at this
point which could be replaced later by adding convenience names for
common signals. That way the interface would remain stable.