Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

On 02/24/2011 02:33 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>    
>> On 01/27/2011 02:20 AM, Lai Jiangshan wrote:
>>      
>>> Make we can inject NMI via qemu-monitor-protocol.
>>> We use "inject-nmi" for the qmp command name, the meaning is clearer.
>>>
>>> Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>
>>> ---
>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>> index ec1a4db..e763bf9 100644
>>> --- a/hmp-commands.hx
>>> +++ b/hmp-commands.hx
>>> @@ -725,7 +725,8 @@ ETEXI
>>>            .params     = "[cpu]",
>>>            .help       = "Inject an NMI on all CPUs if no argument is given, "
>>>                          "otherwise inject it on the specified CPU",
>>> -        .mhandler.cmd = do_inject_nmi,
>>> +        .user_print = monitor_user_noop,
>>> +        .mhandler.cmd_new = do_inject_nmi,
>>>        },
>>>    #endif
>>>    STEXI
>>> diff --git a/monitor.c b/monitor.c
>>> index 387b020..1b1c0ba 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)
>>>    #endif
>>>
>>>    #if defined(TARGET_I386)
>>> -static void do_inject_nmi(Monitor *mon, const QDict *qdict)
>>> +static int do_inject_nmi(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>    {
>>>        CPUState *env;
>>>        int cpu_index;
>>> @@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
>>>        if (!qdict_haskey(qdict, "cpu-index")) {
>>>            for (env = first_cpu; env != NULL; env = env->next_cpu)
>>>                cpu_interrupt(env, CPU_INTERRUPT_NMI);
>>> -        return;
>>> +        return 0;
>>>        }
>>>
>>>        cpu_index = qdict_get_int(qdict, "cpu-index");
>>> @@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)
>>>                    kvm_inject_interrupt(env, CPU_INTERRUPT_NMI);
>>>                else
>>>                    cpu_interrupt(env, CPU_INTERRUPT_NMI);
>>> -            break;
>>> +            return 0;
>>>            }
>>> +
>>> +    return -1;
>>>    }
>>>    #endif
>>>
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 56c4d8b..a887dd5 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -429,6 +429,34 @@ Example:
>>>
>>>    EQMP
>>>
>>> +#if defined(TARGET_I386)
>>> +    {
>>> +        .name       = "inject-nmi",
>>> +        .args_type  = "cpu-index:i?",
>>> +        .params     = "[cpu]",
>>> +        .help       = "Inject an NMI on all CPUs if no argument is given, "
>>> +                      "otherwise inject it on the specified CPU",
>>> +        .user_print = monitor_user_noop,
>>> +        .mhandler.cmd_new = do_inject_nmi,
>>> +    },
>>> +#endif
>>> +SQMP
>>> +inject-nmi
>>> +----------
>>> +
>>> +Inject an NMI on all CPUs or the given CPU (x86 only).
>>> +
>>> +Arguments:
>>> +
>>> +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional)
>>> +
>>> +Example:
>>> +
>>> +->   { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } }
>>> +<- { "return": {} }
>>>
>>>        
>> Please describe all expected errors.
>>      
> Quoting qmp-commands.hx:
>
>      3. Errors, in special, are not documented. Applications should NOT check
>         for specific errors classes or data (it's strongly recommended to only
>         check for the "error" key)
>
> Indeed, not a single error is documented there.  This is intentional.
>    

Yeah, but we're not 0.14 anymore and for 0.15, we need to document 
errors.  If you are suggesting I send a patch to remove that section, 
I'm more than happy to.

> Once we have an error design in place that has a reasonable hope to
> stand the test of time, and have errors documented for at least some of
> the commands here, we can start to require proper error documentation
> for new commands.  But not now.
>    

I'm quite happy with the error design we have today.  The only problem 
is that we don't propagate errors in a sane way but I've got that all 
but fixed in my qapi tree.

>>                                        Don't hide this command for
>> !defined(TARGET_I386), instead have it throw an error in the
>> implementation.
>>      
> Works for me.
>
>    
>> Don't have commands that multiple behavior based on the presence or
>> absence of arguments.  Make it take a list of cpus if you want the
>> ability to inject the NMI to more than one CPU.
>>      
> Having optional arguments is fine.  It's good taste to give them
> "default semantics", i.e. "no argument" is shorthand for one specific
> argument value.
>
> Luiz already pointed to the thread where we discussed this command
> before.  Executive summary:
>
> * Real hardware's NMI button injects all CPUs.  This is the primary use
>    case.
>
> * Lai said injecting a single CPU can be useful for debugging.  Was
>    deemed acceptable as secondary use case.
>
>    Lai also pointed out that the human monitor's nmi command injects a
>    single CPU.  That was dismissed as irrelevant for QMP.
>
> * No other use cases have been presented.
>
> Therefore, the "list of CPUs" idea was shot down as overly general.
>    

That's fine, then we should do two commands.  Think of it from the 
perspective of the client.  This appears as:

in C:

qmp_inject_nmi(sess, false, 0, &err);

in Python:

sess.inject_nmi()

The first example doesn't tell you at all what's happening.  The second 
API does look really nice until you see the following later:

sess.inject_nmi(0)

What's the difference between these two functions?  You might say this 
is bad form and that an explicit named argument should be given but I 
wouldn't count on it.

Having two commands, nmi_inject and nmi_inject_on_cpu, would result in a 
much more readable API in both C and Python:

in C:

qmp_nmi_inject(sess, &err);
qmp_nmi_inject_on_cpu(sess, 3, &err);

in Python:

sess.nmi_inject()
sess.nmi_inject_on_cpu(3)

Regards,

Anthony Liguori