Re: [Qemu-devel] [PATCH v4 1/4] cpus: Define callback for QEMU "nmi" command

[Alexey Kardashevskiy]

On 06/05/2014 04:10 AM, Eric Blake wrote:
> On 06/04/2014 08:25 AM, Alexey Kardashevskiy wrote:
>> This introduces an NMI (non maskable interrupt) nmi_monitor_handler()
>> callback to the CPU class. It is called from QMP's "nmi" command and
>> performs an action required to cause debug crash dump on in-kernel
>> debugger invocation.
>>
>> This adds support for it in qmp_inject_nmi(). Since no architecture
>> supports it at the moment, there is no change in behaviour.
>>
>> This changes inject-nmi command description for HMP and QMP.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
> 
>> +    int ret = -1;
>> +
>> +    if (cs && cc->nmi_monitor_handler) {
>> +        ret = cc->nmi_monitor_handler(cs);
>> +    }
>> +    if (ret) {
>> +        error_set(errp, QERR_UNSUPPORTED);
> 
> If there is a cc->nmi_monitor_handler installed, is it allowed to return
> a value other than -1?  What's more, if the monitor handler fails,
> QERR_UNSUPPORTED no longer seems like the best error.  I think that
> means that your nmi_monitor_handler() callback needs to take an Error
> **errp parameter, and return the proper failure message, rather than
> relying on this caller botching it into an unrelated failure message.


I'd rather remove return value at all, my mistake, I cannot see how NMI
handler can possibly fail - noone can mask it after all :) If callback
exists, I just call it and that's it. If it does not exist, it is
"unsupported" and that's it. Would that be ok?


> 
>> +++ b/qapi-schema.json
>> @@ -1748,13 +1748,11 @@
>>  ##
>>  # @inject-nmi:
>>  #
>> -# Injects an Non-Maskable Interrupt into all guest's VCPUs.
>> +# Injects an Non-Maskable Interrupt into the given guest's VCPU.
> 
> Pre-existing, but as long as you are touching this line:
> 
> s/an Non/a Non/
> 
> "given guest's VCPU" is awkward - since 'inject-nmi' doesn't take any
> parameters, how do you control which guest VCPU is given the interrupt?
>  Is the interrupt delivered to all VCPUs, or just VCPU 0? 

On a CPU selected by the "cpu" monitor command - the user can choose. It
used to be on every CPU for x86 (this is in the commit log).

Ok. If it is not "given" (not the best choice, yes), then what word is it?
"current"? "currently selected"? "the current-in-monitor CPU"?


> Or does this
> mean the "VCPU of the given guest", in which case it is redundant (a
> monitor is associated with only one guest, so that guest is always the
> "given guest" of any command - a "given guest" only matters if we had an
> interface that could control multiple guests at once).




>>  #
>>  # Returns:  If successful, nothing
>>  #
>>  # Since:  0.14.0
>> -#
>> -# Notes: Only x86 Virtual Machines support this command.
> 
> Rather than completely deleting this line, it might be worth stating:
> 
> Note: prior to 2.1, this command was only supported for x86 VMs

More precisely, x86 and s390.

> 
>>  
>> -Inject an NMI on guest's CPUs.
>> +Inject an NMI on the given guest's CPU.
> 
> Why the inconsistency between "CPU" vs. "VCPU" in the different doc
> locations?  Can we pick one that works for all cases?

Ok, I'll make it CPU where I can.


-- 
Alexey