| Message ID | 20181209193749.12277-6-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Plugin support | expand |
Emilio, First, thanks for putting this together - I think everyone doing this sort of thing will benefit if we're able to agree on one upstream plugin interface. One thing I'd like to see is support for unregistering callbacks once they are registered. For instance, you can imagine that a plugin may have 'modality', where it may care about tracing very detailed information in one mode, but trace only limited information otherwise - perhaps only enough to figure out when it needs to switch back to the other mode. I added a function to the user-facing plugin API in my own version of Pavel's plugin patchset to clear all existing plugin instrumentation, allowing the plugin to drop instrumentation if it was no longer interested. Of course, you could always add logic to a plugin to ignore unwanted callbacks, but it could be significantly more efficient to remove them entirely. In Pavel's patchset, this was essentially just a call to tb_flush(), though I think it would be a little more involved for yours. -Aaron
On Dec 14 10:57, Aaron Lindsay wrote: > One thing I'd like to see is support for unregistering callbacks once > they are registered. By the way, I'm willing to work on this if we agree it sounds reasonable and fits in with the rest of your implementation. -Aaron
On Fri, Dec 14, 2018 at 15:57:32 +0000, Aaron Lindsay wrote: > Emilio, > > First, thanks for putting this together - I think everyone doing this > sort of thing will benefit if we're able to agree on one upstream plugin > interface. > > One thing I'd like to see is support for unregistering callbacks once > they are registered. For instance, you can imagine that a plugin may > have 'modality', where it may care about tracing very detailed > information in one mode, but trace only limited information otherwise - > perhaps only enough to figure out when it needs to switch back to the > other mode. > > I added a function to the user-facing plugin API in my own version of > Pavel's plugin patchset to clear all existing plugin instrumentation, > allowing the plugin to drop instrumentation if it was no longer > interested. Of course, you could always add logic to a plugin to ignore > unwanted callbacks, but it could be significantly more efficient to > remove them entirely. In Pavel's patchset, this was essentially just a > call to tb_flush(), though I think it would be a little more involved > for yours. This is a good point. "Regular" callbacks can be unregistered by just passing NULL as the callback (see plugin_unregister_cb__locked, which is called from do_plugin_register_cb). "Direct" callbacks, however (i.e. the ones that embed a callback directly in the translated code), would have to be unregistered by flushing the particular TB (or all TBs, which is probably better from an API viewpoint). I think the following API call would do what you need: typedef int (*qemu_plugin_reset_cb_t)(qemu_plugin_id_t id); void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_reset_cb_t cb); (or maybe qemu_plugin_reinstall?) The idea is that a plugin can "reset" itself, so that (1) all its CBs are cleared and (2) the plugin can register new callbacks. This would all happen in an atomic context (no vCPU running), so that the plugin would miss no CPU events. How does this sound? Thanks, Emilio
On Fri, Dec 14, 2018 at 12:08:22 -0500, Emilio G. Cota wrote: > On Fri, Dec 14, 2018 at 15:57:32 +0000, Aaron Lindsay wrote: (snip) > > I added a function to the user-facing plugin API in my own version of > > Pavel's plugin patchset to clear all existing plugin instrumentation, (snip) > I think the following API call would do what you need: > > typedef int (*qemu_plugin_reset_cb_t)(qemu_plugin_id_t id); > void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_reset_cb_t cb); > > (or maybe qemu_plugin_reinstall?) An alternative is to track the TBs that a plugin has inserted instrumentation into, and only flush those. This would require us to do an additional hash table insert when adding a direct callback, but it allow us to avoid exporting tb_flush indirectly to plugins, which could be abused by malicious plugins to perform a DoS attack. I'll look into this. E.
On Dec 14 12:08, Emilio G. Cota wrote: > On Fri, Dec 14, 2018 at 15:57:32 +0000, Aaron Lindsay wrote: > > Emilio, > > > > First, thanks for putting this together - I think everyone doing this > > sort of thing will benefit if we're able to agree on one upstream plugin > > interface. > > > > One thing I'd like to see is support for unregistering callbacks once > > they are registered. For instance, you can imagine that a plugin may > > have 'modality', where it may care about tracing very detailed > > information in one mode, but trace only limited information otherwise - > > perhaps only enough to figure out when it needs to switch back to the > > other mode. > > > > I added a function to the user-facing plugin API in my own version of > > Pavel's plugin patchset to clear all existing plugin instrumentation, > > allowing the plugin to drop instrumentation if it was no longer > > interested. Of course, you could always add logic to a plugin to ignore > > unwanted callbacks, but it could be significantly more efficient to > > remove them entirely. In Pavel's patchset, this was essentially just a > > call to tb_flush(), though I think it would be a little more involved > > for yours. > > This is a good point. > > "Regular" callbacks can be unregistered by just passing NULL as the > callback (see plugin_unregister_cb__locked, which is called from > do_plugin_register_cb). Ah, okay, thanks - I missed this detail when looking through your patches earlier. > "Direct" callbacks, however (i.e. the ones > that embed a callback directly in the translated code), > would have to be unregistered by flushing the particular TB > (or all TBs, which is probably better from an API viewpoint). > > I think the following API call would do what you need: > > typedef int (*qemu_plugin_reset_cb_t)(qemu_plugin_id_t id); > void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_reset_cb_t cb); > > (or maybe qemu_plugin_reinstall?) I think I prefer your initial suggestion of qemu_plugin_reset - to me it does a better job communicating that existing callbacks will be removed. But, then again, _reinstall makes it more clear that you are responsible for re-adding any callbacks you still want... > The idea is that a plugin can "reset" itself, so that (1) all > its CBs are cleared and (2) the plugin can register new callbacks. > This would all happen in an atomic context (no vCPU running), so > that the plugin would miss no CPU events. The implication being that there would not be the same possibility of other callbacks being called between when qemu_plugin_reset and the qemu_plugin_reset_cb_t callback are called as there is at plugin un-installation time? > How does this sound? I think what you describe is exactly what I'm interested in. -Aaron
On Fri, Dec 14, 2018 at 17:59:20 +0000, Aaron Lindsay wrote: > On Dec 14 12:08, Emilio G. Cota wrote: (snip) > > The idea is that a plugin can "reset" itself, so that (1) all > > its CBs are cleared and (2) the plugin can register new callbacks. > > This would all happen in an atomic context (no vCPU running), so > > that the plugin would miss no CPU events. > > The implication being that there would not be the same possibility of > other callbacks being called between when qemu_plugin_reset and the > qemu_plugin_reset_cb_t callback are called as there is at plugin > un-installation time? The callback is needed for the same reason -- we can only guarantee that there will be no callbacks once the current RCU read critical section expires. > > How does this sound? > > I think what you describe is exactly what I'm interested in. Nice. I'll work on this for v3. Thanks, Emilio
On Dec 14 12:50, Emilio G. Cota wrote: > On Fri, Dec 14, 2018 at 12:08:22 -0500, Emilio G. Cota wrote: > > On Fri, Dec 14, 2018 at 15:57:32 +0000, Aaron Lindsay wrote: > (snip) > > > I added a function to the user-facing plugin API in my own version of > > > Pavel's plugin patchset to clear all existing plugin instrumentation, > (snip) > > I think the following API call would do what you need: > > > > typedef int (*qemu_plugin_reset_cb_t)(qemu_plugin_id_t id); > > void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_reset_cb_t cb); > > > > (or maybe qemu_plugin_reinstall?) > > An alternative is to track the TBs that a plugin has inserted > instrumentation into, and only flush those. This would require > us to do an additional hash table insert when adding a > direct callback, but it allow us to avoid exporting tb_flush indirectly > to plugins, which could be abused by malicious plugins to perform > a DoS attack. I don't think I have a preference. Though now I'm curious - when/why do you expect users might run plugins that could be malicious in this way? -Aaron
On Fri, Dec 14, 2018 at 18:47:42 +0000, Aaron Lindsay wrote: > On Dec 14 12:50, Emilio G. Cota wrote: > > On Fri, Dec 14, 2018 at 12:08:22 -0500, Emilio G. Cota wrote: > > > On Fri, Dec 14, 2018 at 15:57:32 +0000, Aaron Lindsay wrote: > > (snip) > > > > I added a function to the user-facing plugin API in my own version of > > > > Pavel's plugin patchset to clear all existing plugin instrumentation, > > (snip) > > > I think the following API call would do what you need: > > > > > > typedef int (*qemu_plugin_reset_cb_t)(qemu_plugin_id_t id); > > > void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_reset_cb_t cb); > > > > > > (or maybe qemu_plugin_reinstall?) > > > > An alternative is to track the TBs that a plugin has inserted > > instrumentation into, and only flush those. This would require > > us to do an additional hash table insert when adding a > > direct callback, but it allow us to avoid exporting tb_flush indirectly > > to plugins, which could be abused by malicious plugins to perform > > a DoS attack. > > I don't think I have a preference. Though now I'm curious - when/why do > you expect users might run plugins that could be malicious in this way? When this work started, others expressed concern about potentially malicious plugins, although I think they were thinking of preventing plugins from gaining access to data structures that could affect the guest, e.g. CPUState. That's why I'm using a hash table for the plugin context id, although that's probably overkill (do we care about malicious plugins messing with the subscriptions of other plugins? Not sure we should.) Here I'm thinking more about giving plugin developers too much rope to hang themselves with, e.g. calling repeatedly plugin_reset() out of convenience, without realising the performance impact that it entails. That's why I'd like to explore the alternative. Emilio
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h new file mode 100644 index 0000000000..6c67211900 --- /dev/null +++ b/include/qemu/qemu-plugin.h @@ -0,0 +1,241 @@ +/* + * Copyright (C) 2017, Emilio G. Cota <cota@braap.org> + * + * License: GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ +#ifndef QEMU_PLUGIN_API_H +#define QEMU_PLUGIN_API_H + +#include <inttypes.h> +#include <stdbool.h> + +/* + * For best performance, build the plugin with -fvisibility=hidden so that + * QEMU_PLUGIN_LOCAL is implicit. Then, just mark qemu_plugin_install with + * QEMU_PLUGIN_EXPORT. For more info, see + * https://gcc.gnu.org/wiki/Visibility + */ +#if defined _WIN32 || defined __CYGWIN__ + #ifdef BUILDING_DLL + #define QEMU_PLUGIN_EXPORT __declspec(dllexport) + #else + #define QEMU_PLUGIN_EXPORT __declspec(dllimport) + #endif + #define QEMU_PLUGIN_LOCAL +#else + #if __GNUC__ >= 4 + #define QEMU_PLUGIN_EXPORT __attribute__((visibility("default"))) + #define QEMU_PLUGIN_LOCAL __attribute__((visibility("hidden"))) + #else + #define QEMU_PLUGIN_EXPORT + #define QEMU_PLUGIN_LOCAL + #endif +#endif + +typedef uint64_t qemu_plugin_id_t; + +/** + * qemu_plugin_install - Install a plugin + * @id: this plugin's opaque ID + * @argc: number of arguments + * @argv: array of arguments (@argc elements) + * + * All plugins must export this symbol. + * + * Note: @argv is freed after this function returns. + */ +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, int argc, + char **argv); + +typedef void (*qemu_plugin_uninstall_cb_t)(qemu_plugin_id_t id); + +/** + * qemu_plugin_uninstall - Uninstall a plugin + * @id: this plugin's opaque ID + * @cb: callback to be called once the plugin has been removed + * + * Do NOT assume that the plugin has been uninstalled once this + * function returns. Plugins are uninstalled asynchronously, + * and therefore the given plugin might still receive callbacks + * from prior subscriptions _until_ @cb is called. + */ +void qemu_plugin_uninstall(qemu_plugin_id_t id, qemu_plugin_uninstall_cb_t cb); + +typedef void (*qemu_plugin_simple_cb_t)(qemu_plugin_id_t id); + +typedef void (*qemu_plugin_udata_cb_t)(qemu_plugin_id_t id, void *userdata); + +typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id, + unsigned int vcpu_index); + +typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index, + void *userdata); + +/** + * qemu_plugin_register_vcpu_init_cb - register a vCPU initialization callback + * @id: plugin ID + * @cb: callback function + * + * The @cb function is called every time a vCPU is initialized. + * + * See also: qemu_plugin_register_vcpu_exit_cb() + */ +void qemu_plugin_register_vcpu_init_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_simple_cb_t cb); + +/** + * qemu_plugin_register_vcpu_exit_cb - register a vCPU exit callback + * @id: plugin ID + * @cb: callback function + * + * The @cb function is called every time a vCPU exits. + * + * See also: qemu_plugin_register_vcpu_init_cb() + */ +void qemu_plugin_register_vcpu_exit_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_simple_cb_t cb); + +void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_simple_cb_t cb); + +void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_simple_cb_t cb); + +struct qemu_plugin_tb; +struct qemu_plugin_insn; + +enum qemu_plugin_cb_flags { + QEMU_PLUGIN_CB_NO_REGS, /* callback does not access the CPU's regs */ + QEMU_PLUGIN_CB_R_REGS, /* callback reads the CPU's regs */ + QEMU_PLUGIN_CB_RW_REGS, /* callback reads and writes the CPU's regs */ +}; + +enum qemu_plugin_mem_rw { + QEMU_PLUGIN_MEM_R = 1, + QEMU_PLUGIN_MEM_W, + QEMU_PLUGIN_MEM_RW, +}; + +typedef void (*qemu_plugin_vcpu_tb_trans_cb_t)(qemu_plugin_id_t id, + unsigned int vcpu_index, + struct qemu_plugin_tb *tb); + +void qemu_plugin_register_vcpu_tb_trans_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_tb_trans_cb_t cb); + +/* can only call from tb_trans_cb callback */ +void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb, + qemu_plugin_vcpu_udata_cb_t cb, + enum qemu_plugin_cb_flags flags, + void *userdata); + +enum qemu_plugin_op { + QEMU_PLUGIN_INLINE_ADD_U64, +}; + +void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb, + enum qemu_plugin_op op, + void *ptr, uint64_t imm); + +void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn, + qemu_plugin_vcpu_udata_cb_t cb, + enum qemu_plugin_cb_flags flags, + void *userdata); + +void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn, + enum qemu_plugin_op op, + void *ptr, uint64_t imm); + +typedef uint32_t qemu_plugin_meminfo_t; + +unsigned qemu_plugin_mem_size_shift(qemu_plugin_meminfo_t info); +bool qemu_plugin_mem_is_sign_extended(qemu_plugin_meminfo_t info); +bool qemu_plugin_mem_is_big_endian(qemu_plugin_meminfo_t info); +bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info); + +typedef void +(*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index, + qemu_plugin_meminfo_t info, uint64_t vaddr, + void *userdata); + +typedef void +(*qemu_plugin_vcpu_mem_haddr_cb_t)(unsigned int vcpu_index, + qemu_plugin_meminfo_t info, uint64_t vaddr, + void *haddr, void *userdata); + +void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn, + qemu_plugin_vcpu_mem_cb_t cb, + enum qemu_plugin_cb_flags flags, + enum qemu_plugin_mem_rw rw, + void *userdata); + +void qemu_plugin_register_vcpu_mem_haddr_cb(struct qemu_plugin_insn *insn, + qemu_plugin_vcpu_mem_haddr_cb_t cb, + enum qemu_plugin_cb_flags flags, + enum qemu_plugin_mem_rw rw, + void *userdata); + +void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn, + enum qemu_plugin_mem_rw rw, + enum qemu_plugin_op op, void *ptr, + uint64_t imm); + +uint64_t qemu_plugin_ram_addr_from_host(void *haddr); + +typedef void +(*qemu_plugin_vcpu_syscall_cb_t)(qemu_plugin_id_t id, unsigned int vcpu_index, + int64_t num, uint64_t a1, uint64_t a2, + uint64_t a3, uint64_t a4, uint64_t a5, + uint64_t a6, uint64_t a7, uint64_t a8); + +void qemu_plugin_register_vcpu_syscall_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_syscall_cb_t cb); + +typedef void +(*qemu_plugin_vcpu_syscall_ret_cb_t)(qemu_plugin_id_t id, unsigned int vcpu_idx, + int64_t num, int64_t ret); + +void +qemu_plugin_register_vcpu_syscall_ret_cb(qemu_plugin_id_t id, + qemu_plugin_vcpu_syscall_ret_cb_t cb); + +size_t qemu_plugin_tb_n_insns(const struct qemu_plugin_tb *tb); + +uint64_t qemu_plugin_tb_vaddr(const struct qemu_plugin_tb *tb); + +struct qemu_plugin_insn * +qemu_plugin_tb_get_insn(const struct qemu_plugin_tb *tb, size_t idx); + +const void *qemu_plugin_insn_data(const struct qemu_plugin_insn *insn); + +size_t qemu_plugin_insn_size(const struct qemu_plugin_insn *insn); + +uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn); +void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn); + +/** + * qemu_plugin_vcpu_for_each - iterate over the existing vCPU + * @id: plugin ID + * @cb: callback function + * + * The @cb function is called once for each existing vCPU. + * + * See also: qemu_plugin_register_vcpu_init_cb() + */ +void qemu_plugin_vcpu_for_each(qemu_plugin_id_t id, + qemu_plugin_vcpu_simple_cb_t cb); + +void qemu_plugin_register_flush_cb(qemu_plugin_id_t id, + qemu_plugin_simple_cb_t cb); + +void qemu_plugin_register_atexit_cb(qemu_plugin_id_t id, + qemu_plugin_udata_cb_t cb, void *userdata); + +/* returns -1 in user-mode */ +int qemu_plugin_n_vcpus(void); + +/* returns -1 in user-mode */ +int qemu_plugin_n_max_vcpus(void); + +#endif /* QEMU_PLUGIN_API_H */
Add the API first to ease review. Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/qemu/qemu-plugin.h | 241 +++++++++++++++++++++++++++++++++++++ 1 file changed, 241 insertions(+) create mode 100644 include/qemu/qemu-plugin.h