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

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
> On Thu, 26 May 2011 22:23:10 +0300
> Blue Swirl<blauwirbel@gmail.com>  wrote:
>
>> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>
>>>> On Fri, 6 May 2011 18:36:31 +0300
>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>>>
>>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>>>
>>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz Capitulino<lcapitulino@redhat.com>  wrote:
>>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300
>>>>>>>> Blue Swirl<blauwirbel@gmail.com>  wrote:
>>>>>>>>
>>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz Capitulino<lcapitulino@redhat.com>  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.
>