Re: [PATCH] accel/tcg: TCG Plugin callback on a vCpu interrupt

[Alex Bennée]

Mikhail Tyutin <m.tyutin@yadro.com> writes:

> TCG Plugin callback to notify plugins when interrupt is triggered for
> a vCpu. The plugin can optionally use this notification to see reason
> of aborted instruction execution.
>
> Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
> ---
>  accel/tcg/cpu-exec.c         |  5 +++++
>  include/qemu/plugin-event.h  |  1 +
>  include/qemu/plugin.h        |  4 ++++
>  include/qemu/qemu-plugin.h   | 12 +++++++++++-
>  plugins/core.c               | 12 ++++++++++++
>  plugins/qemu-plugins.symbols |  1 +
>  6 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index c938eb96f8..9110f7e290 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c

The biggest problem I have with this approach is we are adding to an
already overly complex exception/interrupt code in the main loop.

> @@ -750,6 +750,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, 
> int *ret)
>          if (replay_exception()) {
>              CPUClass *cc = CPU_GET_CLASS(cpu);
>              qemu_mutex_lock_iothread();
> +            qemu_plugin_vcpu_interrupt_cb(cpu);
>              cc->tcg_ops->do_interrupt(cpu);

This is at least what we think of as a traditional IRQ. Something that
interrupts the flow of the processor and after some target specific
processing goes somewhere else.

>              qemu_mutex_unlock_iothread();
>              cpu->exception_index = -1;
> @@ -829,6 +830,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>              /* Do nothing */
>          } else if (interrupt_request & CPU_INTERRUPT_HALT) {
>              replay_interrupt();
> +            qemu_plugin_vcpu_interrupt_cb(cpu);
>              cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
>              cpu->halted = 1;

This isn't really an interrupt - its used by a selection of the various
architectures to bring the vCPU to a halted state. However it's not the
only way to halt the CPU as grepping for "->halted = 1;" will show you.
If we successfully halt we'll eventually end up calling:

  qemu_plugin_vcpu_idle_cb() 

>              cpu->exception_index = EXCP_HLT;
> @@ -840,6 +842,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>              X86CPU *x86_cpu = X86_CPU(cpu);
>              CPUArchState *env = &x86_cpu->env;
>              replay_interrupt();
> +            qemu_plugin_vcpu_interrupt_cb(cpu);
>              cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
>              do_cpu_init(x86_cpu);
>              cpu->exception_index = EXCP_HALTED;

This is some x86 specific hack that we've not managed to find a sensible
way to excise out of the common code. 

> @@ -849,6 +852,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>  #else
>          else if (interrupt_request & CPU_INTERRUPT_RESET) {
>              replay_interrupt();
> +            qemu_plugin_vcpu_interrupt_cb(cpu);
>              cpu_reset(cpu);

This again is not an IRQ really but another (minority) method for
generating a cold reset in the vCPU. There may be an argument to add a
plugin hook in cpu_reset() but I suspect most of the calls to it would
be artificial artefacts of the way to start and configure vCPUS as we
bring objects up.

>              qemu_mutex_unlock_iothread();
>              return true;
> @@ -866,6 +870,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>                  if (need_replay_interrupt(interrupt_request)) {
>                      replay_interrupt();
>                  }
> +                qemu_plugin_vcpu_interrupt_cb(cpu);

This may be an interrupt but we also may have restarted the loop via
cpu_loop_exit() so missing the rest of the logic. Some of these handlers
will also call cc->tcg_ops->do_interrupt(cpu) like above.

<snip>
>  
> +void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu)
> +{
> +    plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INTERRUPT);
> +}
> +
>  void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id,
>                                         qemu_plugin_vcpu_simple_cb_t cb)
>  {
> @@ -411,6 +417,12 @@ void 
> qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
>      plugin_register_cb(id, QEMU_PLUGIN_EV_VCPU_RESUME, cb);
>  }
>  
> +void qemu_plugin_register_vcpu_interrupt_cb(qemu_plugin_id_t id,
> +                                            qemu_plugin_vcpu_simple_cb_t cb)
> +{
> +    plugin_register_cb(id, QEMU_PLUGIN_EV_VCPU_INTERRUPT, cb);
> +}
> +
>  void qemu_plugin_register_flush_cb(qemu_plugin_id_t id,
>                                     qemu_plugin_simple_cb_t cb)
>  {
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index 71f6c90549..c8621f9950 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -30,6 +30,7 @@
>    qemu_plugin_register_vcpu_mem_cb;
>    qemu_plugin_register_vcpu_mem_inline;
>    qemu_plugin_register_vcpu_resume_cb;
> +  qemu_plugin_register_vcpu_interrupt_cb;
>    qemu_plugin_register_vcpu_syscall_cb;
>    qemu_plugin_register_vcpu_syscall_ret_cb;
>    qemu_plugin_register_vcpu_tb_exec_cb;

When we add new callbacks we should at least use them in one of the
plugins to check they work. 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro