Re: [Qemu-devel] [PATCH v3 1/4] cpus: Define NMI callback

[Luiz Capitulino]

On Tue, 10 Jun 2014 16:41:07 +0200
Cornelia Huck <address@hidden> wrote:

> On Tue, 10 Jun 2014 09:39:51 -0400
> Luiz Capitulino <address@hidden> wrote:
> 
> > On Wed,  4 Jun 2014 18:08:47 +1000
> > Alexey Kardashevskiy <address@hidden> wrote:
> > 
> > > This introduces an NMI (non maskable interrupt) callback per CPU class
> > > which QMP's "nmi" command may use to issue NMI on a CPU.
> > > 
> > > 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>
> > > ---
> > > Changes:
> > > v3:
> > > * actual nmi() enablement moved from last patch to first patch
> > > * changed description for QMP command too
> > > ---
> > >  cpus.c            | 11 ++++++++++-
> > >  hmp-commands.hx   |  6 ++----
> > >  include/qom/cpu.h |  1 +
> > >  qapi-schema.json  |  4 +---
> > >  qmp-commands.hx   |  3 +--
> > >  5 files changed, 15 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index dd7ac13..a000bd8 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -1495,6 +1495,15 @@ void qmp_inject_nmi(Error **errp)
> > >          }
> > >      }
> > >  #else
> > > -    error_set(errp, QERR_UNSUPPORTED);
> > > +    CPUState *cs = qemu_get_cpu(monitor_get_cpu_index());
> > > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > > +    int ret = -1;
> > > +
> > > +    if (cs && cc->nmi) {
> > > +        ret = cc->nmi(cs);
> > > +    }
> > > +    if (ret) {
> > > +        error_set(errp, QERR_UNSUPPORTED);
> > > +    }
> > >  #endif
> > >  }
> > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > index 2e462c0..e97b5ec 100644
> > > --- a/hmp-commands.hx
> > > +++ b/hmp-commands.hx
> > > @@ -830,19 +830,17 @@ The values that can be specified here depend on the 
> > > machine type, but are
> > >  the same that can be specified in the @code{-boot} command line option.
> > >  ETEXI
> > >  
> > > -#if defined(TARGET_I386) || defined(TARGET_S390X)
> > >      {
> > >          .name       = "nmi",
> > >          .args_type  = "",
> > >          .params     = "",
> > > -        .help       = "inject an NMI on all guest's CPUs",
> > > +        .help       = "inject an NMI on the given guest's CPU",
> > >          .mhandler.cmd = hmp_inject_nmi,
> > >      },
> > > -#endif
> > >  STEXI
> > >  @item nmi @var{cpu}
> > >  @findex nmi
> > > -Inject an NMI (x86) or RESTART (s390x) on the given CPU.
> > > +Inject an NMI on the given CPU.
> > >  
> > >  ETEXI
> > >  
> > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > index df977c8..b34f23b 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -108,6 +108,7 @@ typedef struct CPUClass {
> > >      void (*parse_features)(CPUState *cpu, char *str, Error **errp);
> > >  
> > >      void (*reset)(CPUState *cpu);
> > > +    int (*nmi)(CPUState *cs);
> > >      int reset_dump_flags;
> > >      bool (*has_work)(CPUState *cpu);
> > >      void (*do_interrupt)(CPUState *cpu);
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 7bc33ea..dcf6642 100644
> > > --- a/qapi-schema.json
> > > +++ 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.
> > 
> > QMP doesn't have the concept of "current monitored CPU" you talk in the
> > intro email. In QMP you have to specify the CPU. You have to choices:
> > 
> >  - Add a new command that takes a CPU parameter (seems the best to me, as
> >    people were asking for a different command anyways)
> > 
> >  - Add an optional parameter to inject-nmi. When the CPU parameter is
> >    not specified, the command sends the NMI to all CPUs
> 
> The s390 restart interrupt is a per-vcpu interrupt, which we really
> don't want to inject on _all_ vcpus. OTOH, we want to inject that
> interrupt on any vcpu - we don't care which one it is. So I'd really
> like an "inject nmi on default cpu" option.

We could define a default CPU for the command. What isn't going to work
is to use the human monitor's "current CPU" concept (set with the cpu
command).

> 
> > 
> > Eric, any thoughts?
> > 
> > >  #
> > >  # Returns:  If successful, nothing
> > >  #
> > >  # Since:  0.14.0
> > > -#
> > > -# Notes: Only x86 Virtual Machines support this command.
> > >  ##
> > >  { 'command': 'inject-nmi' }
> > >  
> > > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > > index d8aa4ed..553375b 100644
> > > --- a/qmp-commands.hx
> > > +++ b/qmp-commands.hx
> > > @@ -477,7 +477,7 @@ SQMP
> > >  inject-nmi
> > >  ----------
> > >  
> > > -Inject an NMI on guest's CPUs.
> > > +Inject an NMI on the given guest's CPU.
> > >  
> > >  Arguments: None.
> > >  
> > > @@ -487,7 +487,6 @@ Example:
> > >  <- { "return": {} }
> > >  
> > >  Note: inject-nmi fails when the guest doesn't support injecting.
> > > -      Currently, only x86 (NMI) and s390x (RESTART) guests do.
> > >  
> > >  EQMP
> > >  
> > 
>