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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3347 bytes --]

On 06/11/2014 11:03 AM, Alexey Kardashevskiy wrote:
> This introduces an NMI (Non Maskable Interrupt) interface with
> a single nmi_monitor_handler() method. A machine or a device can
> implement it. This searches for an QOM object which supports the interface
> and if it is implemented , calls it. The callback implements an action
> required to cause debug crash dump on in-kernel debugger invocation.
> The callback returns Error**.
> 
> 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 <aik@ozlabs.ru>
> ---

> +++ b/hw/core/nmi.c
> @@ -0,0 +1,81 @@
> +/*
> + *  NMI monitor handler class and helpers.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License,
> + *  or (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */

For the GPL license to work, there has to be a Copyright line.

> +static int do_nmi(Object *o, void *opaque)
> +{
> +    struct do_nmi_s *ns = opaque;
> +    NMI *n = (NMI *) object_dynamic_cast(o, TYPE_NMI);

Is the cast to (NMI *) necessary, or does object_dynamic_cast() already
return something that can be assigned to an arbitrary pointer type?


> +++ b/include/hw/nmi.h
> @@ -0,0 +1,46 @@
> +/*
> + *  NMI monitor handler class and helpers definitions.

Same comment about Copyright.


> +#define TYPE_NMI "nmi"
> +
> +#define NMI_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(NMIClass, (klass), TYPE_NMI)
> +#define NMI_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(NMIClass, (obj), TYPE_NMI)
> +#define NMI(obj) \
> +     INTERFACE_CHECK(NMI, (obj), TYPE_NMI)
> +
> +typedef struct NMI {
> +    Object parent_obj;
> +} NMI;

Eww.  This is declaring both 'NMI' as a type, and 'NMI()' as a macro.
Is that going to trip people up?  Is 'NMIState' a better name?


> +++ b/qapi-schema.json
> @@ -1116,13 +1116,13 @@
>  ##
>  # @inject-nmi:
>  #
> -# Injects an Non-Maskable Interrupt into all guest's VCPUs.
> +# Injects an Non-Maskable Interrupt into the default CPU (x86/s390) or all CPUs (ppc64).

s/an Non/a Non/

> +++ b/qmp-commands.hx
> @@ -477,7 +477,7 @@ SQMP
>  inject-nmi
>  ----------
>  
> -Inject an NMI on guest's CPUs.
> +Inject an NMI on the default CPU (x86/s390) or all CPUs (ppc64).

Oh, I see the problem.  The acronym 'NMI' is pronounced with a leading
vowel (an "en-em-eye"), but the expansion is pronounced with a consonant
(a non-maskable interrupt), so the correct article depends on whether
you are spelling things out.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]