Julian Ganz <[email protected]> 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 <[email protected]> > --- > 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
