| Message ID | 20231021122502.26746-1-neither@nut.email |
|---|---|
| State | New |
| Headers | show |
| Series | tcg-plugins: add a hook for interrupts, exceptions and traps | expand |
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.
Hi all, thank's for the feedback! Maybe for broader context: at work a colleague and I are developing a plugin that implements RISC-V specific tracing [1]. It writes a trace to a file in the format you'd expect from actual hardware implementing the relevant interface. Currently, it's developed on top of Xilinx' qemu-fork with yet another bunch of un-upstreamable patches on top, some of which introduce some API we use in the plugin. Meaning that if I open-source the plugin one day, I'll probably need to rewrite parts of it anyway. On Mon, 23 Oct 2023 14:08:51 +0100 Alex Bennée <alex.bennee@linaro.org> wrote: > > 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. Yes, that I'm aware of. > > 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? So my colleague an I were discussing this again today: knowing the PC, EPC, ecause (or their equivalent) and the priviledge/mode are not sufficient since there are too many cases where detecting an exception (or an interrupt) is non-trivial or impossible. For example, there's no gurantee that ecause will ever be reset between interrupts and an exception may occur without a PC discontinuity. Probably the worst case would be an illegal instruction at the trap handler. But we also found a few other cases where even if we know excactly which instructions belonged to an ISR, we could still not tell how we ended up there with sufficient certainty. But I'm aware of [2] if that's what you were after, and we do need the ability to query some registers for our plugin in addition to an interrupt callback or similar API. > > --- 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. Yes. Funnily enough, that's more or less what we want anyway for our plugin (because we need to handle `ebreak`s for example). However, in this specific use-case we need to half-disassemble some instructions anyway so we would at least have no problem finding and special-casing `ebreaks`. But this is indeed likely not what you want for the general case. In some cases you may still be able to detect the absence of a PC discontinuity and then just assume that you can ignore an interrupt, but that would involve logic you normally wouldn't have (e.g. a callback on individual instructions) and require some knowledge about the architecture. Which is exctly not what you want for the TCG plugin API. > Similarly the CPU_INTERRUPT_EXITTB interrupt isn't really changing > state, just ensuring we exit the run loop so house keeping is done. Mh... that one we've not observed yet. Likely because our test-cases (running OpenSBI with very simple payloads) were too simple. > 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. I tried to avoid that. But it looks like I don't have a choice :/ > > --- 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. Huh. I wasn't aware of that. > 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. After receiving your feedback I agree that I was maybe a bit too eager. I'll return to the drawing board and prepare a new series with a simple demonstrator (maybe a simple interrupt-counter and/or plugin meassuring the "time" spent in an inteerupt). But since I'm not too intimate with qemu internals, this will likely take some time. Kind regards, Julian [1] https://github.com/riscv-non-isa/riscv-trace-spec [2] https://gitlab.com/qemu-project/qemu/-/issues/1706
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); /* * 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. + */ +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;
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. 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. 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(+)