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

[Alexey Kardashevskiy]

On 06/12/2014 03:21 AM, Eric Blake wrote:
> 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 <address@hidden>
>> ---
> 
>> +++ 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.

Hm. Ok. include/hw/fw-path-provider.h (where I copied this from) does not
have one and it was ok.

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


All the macros from include/qom/object.h do this cast and so do I.

object_dynamic_cast returns Object*.


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

My bad, I'll fix. I'll probably get rid of the struct.

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

Yep, here and in other places I read it as "en-em-eye" but it is not
correct right here :) I'll fix, thanks.


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



-- 
Alexey