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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 11 Jun 2014 01:40:14 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 06/10/2014 11:39 PM, Luiz Capitulino wrote:
> > On Wed,  4 Jun 2014 18:08:47 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> 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 <aik@ozlabs.ru>
> >> ---
> >> 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:
> 
> 
> If QMP does not have such a concept, then how does it work now?

It sends the NMI to all CPUs. I hope I'm not miscommunicate my suggestion
here, which is to add a inject-nmi-cpu command that takes a CPU index as
an argument. Then you do exactly what the qmp_inject_nmi() does today, but
you use the CPU index instead of using the HMP stuff.

I don't think this should be more work than series already does, and the
end result would be nicer because it won't depend on HMP magic. If can help
you with the patches if you want.

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