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

[Eric Blake]

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.


> +++ 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?  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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature