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

[Anthony Liguori]

On 02/24/2011 02:33 AM, Markus Armbruster wrote:
> Anthony Liguori<address@hidden>  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<address@hidden>
> > > ---
> > > 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