Re: [PATCH] tcg-plugins: add a hook for interrupts, exceptions and traps

[Alex Bennée]

Julian Ganz <neither@nut.email> writes:

> Some analysis greatly benefits, or depends on, information about
> interrupts. For example, we may need to handle the execution of a new
> translation block differently if it is not the result of normal program
> flow but of an interrupt.

I can see the benefit for plugins knowing the context - for QEMU itself
there is no real difference in how it handles blocks that are part of an
interrupt.

> Even with the existing interfaces, it is more or less possible to
> discern these situations using some heuristice. For example, the PC
> landing in a trap vector is a strong indicator that a trap, i.e. an
> interrupt or event occured. However, such heuristics require knowledge
> about the architecture and may be prone to errors.

Does this requirement go away if you can query the current execution
state via registers?

> The callback introduced by this change provides a generic and
> easy-to-use interface for plugin authors. It allows them to register a
> callback in which they may alter some plugin-internal state to convey
> the firing of an interrupt for a given CPU, or perform some stand-alone
> analysis based on the interrupt and, for example, the CPU state.
>
> Signed-off-by: Julian Ganz <neither@nut.email>
> ---
>  accel/tcg/cpu-exec.c         |  3 +++
>  include/qemu/plugin-event.h  |  1 +
>  include/qemu/plugin.h        |  4 ++++
>  include/qemu/qemu-plugin.h   | 11 +++++++++++
>  plugins/core.c               | 12 ++++++++++++
>  plugins/qemu-plugins.symbols |  1 +
>  6 files changed, 32 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 1a5bc90220..e094d9236d 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -754,6 +754,8 @@ static inline bool cpu_handle_exception(CPUState *cpu, 
> int *ret)
>              qemu_mutex_unlock_iothread();
>              cpu->exception_index = -1;
>  
> +            qemu_plugin_vcpu_interrupt_cb(cpu);
> +
>              if (unlikely(cpu->singlestep_enabled)) {
>                  /*
>                   * After processing the exception, ensure an EXCP_DEBUG is
> @@ -866,6 +868,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>                  if (need_replay_interrupt(interrupt_request)) {
>                      replay_interrupt();
>                  }
> +                qemu_plugin_vcpu_interrupt_cb(cpu);

My worry here is:

 a) we are conflating QEMU exceptions and interrupts as the same thing
 b) this is leaking internal implementation details of the translator

For example EXCP_SEMIHOST isn't actually an interrupt (we don't change
processor state or control flow). It's just the internal signalling we
use to handle our semihosting implementation. Similarly the
CPU_INTERRUPT_EXITTB interrupt isn't really changing state, just
ensuring we exit the run loop so house keeping is done.

The "correct" way for ARM for example would be to register a helper
function with arm_register_el_change_hook() and trigger the callbacks
that way. However each architecture does its own IRQ modelling so you
would need to work out where in the plumbing to do each callback.

>                  /*
>                   * After processing the interrupt, ensure an EXCP_DEBUG is
>                   * raised when single-stepping so that GDB doesn't miss the
> diff --git a/include/qemu/plugin-event.h b/include/qemu/plugin-event.h
> index 7056d8427b..d085bdda4e 100644
> --- a/include/qemu/plugin-event.h
> +++ b/include/qemu/plugin-event.h
> @@ -20,6 +20,7 @@ enum qemu_plugin_event {
>      QEMU_PLUGIN_EV_VCPU_SYSCALL_RET,
>      QEMU_PLUGIN_EV_FLUSH,
>      QEMU_PLUGIN_EV_ATEXIT,
> +    QEMU_PLUGIN_EV_VCPU_INTERRUPT,
>      QEMU_PLUGIN_EV_MAX, /* total number of plugin events we support */
>  };
>  
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index 7fdc3a4849..f942e45f41 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -190,6 +190,7 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu);
>  void qemu_plugin_tb_trans_cb(CPUState *cpu, struct qemu_plugin_tb *tb);
>  void qemu_plugin_vcpu_idle_cb(CPUState *cpu);
>  void qemu_plugin_vcpu_resume_cb(CPUState *cpu);
> +void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu);
>  void
>  qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1,
>                           uint64_t a2, uint64_t a3, uint64_t a4, uint64_t a5,
> @@ -270,6 +271,9 @@ static inline void qemu_plugin_vcpu_idle_cb(CPUState *cpu)
>  static inline void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
>  { }
>  
> +static inline void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu)
> +{ }
> +
>  static inline void
>  qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t 
> a2,
>                           uint64_t a3, uint64_t a4, uint64_t a5, uint64_t a6,
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 50a9957279..2eb4b325fe 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -206,6 +206,17 @@ void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t 
> id,
>  void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
>                                           qemu_plugin_vcpu_simple_cb_t cb);
>  
> +/**
> + * qemu_plugin_register_vcpu_interrupt_cb() - register a vCPU interrupt 
> callback
> + * @id: plugin ID
> + * @cb: callback function
> + *
> + * The @cb function is called every time a vCPU receives an interrupt, 
> exception
> + * or trap.

As discussed above you would trigger for a lot more than that. You would
also miss a lot of the other interesting transitions which don't need an
asynchronous signal. For example HELPER(exception_return) happily
changes control flow but doesn't need to use the exception mechanism to
do it. 

> + */
> +void qemu_plugin_register_vcpu_interrupt_cb(qemu_plugin_id_t id,
> +                                            qemu_plugin_vcpu_simple_cb_t cb);
> +
>  /** struct qemu_plugin_tb - Opaque handle for a translation block */
>  struct qemu_plugin_tb;
>  /** struct qemu_plugin_insn - Opaque handle for a translated instruction */
> diff --git a/plugins/core.c b/plugins/core.c
> index fcd33a2bff..3658bdef45 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -103,6 +103,7 @@ static void plugin_vcpu_cb__simple(CPUState *cpu, enum 
> qemu_plugin_event ev)
>      case QEMU_PLUGIN_EV_VCPU_EXIT:
>      case QEMU_PLUGIN_EV_VCPU_IDLE:
>      case QEMU_PLUGIN_EV_VCPU_RESUME:
> +    case QEMU_PLUGIN_EV_VCPU_INTERRUPT:
>          /* iterate safely; plugins might uninstall themselves at any time */
>          QLIST_FOREACH_SAFE_RCU(cb, &plugin.cb_lists[ev], entry, next) {
>              qemu_plugin_vcpu_simple_cb_t func = cb->f.vcpu_simple;
> @@ -400,6 +401,11 @@ void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
>      plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_RESUME);
>  }
>  
> +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)
>  {
> @@ -412,6 +418,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..1fddb4b9fd 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -35,6 +35,7 @@
>    qemu_plugin_register_vcpu_tb_exec_cb;
>    qemu_plugin_register_vcpu_tb_exec_inline;
>    qemu_plugin_register_vcpu_tb_trans_cb;
> +  qemu_plugin_register_vcpu_interrupt_cb;
>    qemu_plugin_reset;
>    qemu_plugin_start_code;
>    qemu_plugin_tb_get_insn;

I'm not opposed to adding such a API hook to plugins but I don't think
the current approach does what you want. If we do add a new API it is
customary to either expand an existing plugin or add a new one to
demonstrate the use of the API.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro