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

[Alexey Kardashevskiy]

On 06/11/2014 12:41 AM, Cornelia Huck 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.


Interesting thing. I was pushing "nmi" for ppc which did nmi only on the
current cpu but according to our architect Ben, I should inject NMI on all
CPUs because although XMON (inkernel debugger) tries to stop other CPUs, it
may fail to do that because of some interrupt deadlock. And - this is
important for us - this is what pHyp (native IBM hypervisor) does.

Now I wonder how you are getting away with injecting NMI on one CPU only
and what will happen if we inject NMI on all?

And I changed x86 to inject NMI on the default CPU only (used to be on all
CPUs), and I wonder again if it is actually ok and won't break things.




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


-- 
Alexey