| Message ID | 152819517756.30857.1862569750260837574.stgit@pasha-ThinkPad-T60 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> writes: > From: Pavel Dovgalyuk <pavel.dovgaluk@gmail.com> > > This is a samples of the instrumenting interface and implementation > of some instruction tracing tasks. > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > --- > accel/tcg/translator.c | 5 +++++ > include/qemu/instrument.h | 7 +++++++ > plugins/helper.h | 1 + > plugins/plugins.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 54 insertions(+) > create mode 100644 include/qemu/instrument.h > create mode 100644 plugins/helper.h > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index 0f9dca9..48773ac 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -17,6 +17,7 @@ > #include "exec/gen-icount.h" > #include "exec/log.h" > #include "exec/translator.h" > +#include "qemu/instrument.h" > > /* Pairs with tcg_clear_temp_count. > To be called by #TranslatorOps.{translate_insn,tb_stop} if > @@ -89,6 +90,10 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > } > } > > + if (plugins_need_before_insn(db->pc_next, cpu)) { > + plugins_instrument_before_insn(db->pc_next, cpu); > + } > + I don't really see the need for a plugin_need_foo call here. Can't we just iterate over all plugins that provide a before_insn binding and leave it at that? If the plugin decides it doesn't want to do anything with this particular instruction it can always bail early. > /* Disassemble one instruction. The translate_insn hook should > update db->pc_next and db->is_jmp to indicate what should be > done next -- either exiting this loop or locate the start of > diff --git a/include/qemu/instrument.h b/include/qemu/instrument.h > new file mode 100644 > index 0000000..e8f279f > --- /dev/null > +++ b/include/qemu/instrument.h > @@ -0,0 +1,7 @@ > +#ifndef INSTRUMENT_H > +#define INSTRUMENT_H > + > +bool plugins_need_before_insn(target_ulong pc, CPUState *cpu); > +void plugins_instrument_before_insn(target_ulong pc, CPUState *cpu); Need empty inline cases for #ifndef CONFIG_PLUGINS > + > +#endif /* INSTRUMENT_H */ > diff --git a/plugins/helper.h b/plugins/helper.h > new file mode 100644 > index 0000000..007b395 > --- /dev/null > +++ b/plugins/helper.h > @@ -0,0 +1 @@ > +DEF_HELPER_2(before_insn, void, tl, ptr) I wonder if we should have an explicit DEF_HELPER_FLAGS_2. Looking at the flags though: /* call flags */ /* Helper does not read globals (either directly or through an exception). It implies TCG_CALL_NO_WRITE_GLOBALS. */ #define TCG_CALL_NO_READ_GLOBALS 0x0010 /* Helper does not write globals */ #define TCG_CALL_NO_WRITE_GLOBALS 0x0020 /* Helper can be safely suppressed if the return value is not used. */ #define TCG_CALL_NO_SIDE_EFFECTS 0x0040 /* convenience version of most used call flags */ #define TCG_CALL_NO_RWG TCG_CALL_NO_READ_GLOBALS #define TCG_CALL_NO_WG TCG_CALL_NO_WRITE_GLOBALS #define TCG_CALL_NO_SE TCG_CALL_NO_SIDE_EFFECTS #define TCG_CALL_NO_RWG_SE (TCG_CALL_NO_RWG | TCG_CALL_NO_SE) #define TCG_CALL_NO_WG_SE (TCG_CALL_NO_WG | TCG_CALL_NO_SE) I guess no flags means machine state is fully consistent before we make the helper call? > diff --git a/plugins/plugins.c b/plugins/plugins.c > index eabc931..5a08e71 100644 > --- a/plugins/plugins.c > +++ b/plugins/plugins.c > @@ -1,8 +1,13 @@ > #include "qemu/osdep.h" > #include "qemu-common.h" > +#include "cpu.h" > #include "qemu/error-report.h" > #include "qemu/plugins.h" > +#include "qemu/instrument.h" > +#include "tcg/tcg.h" > +#include "tcg/tcg-op.h" > #include "qemu/queue.h" > +#include "qemu/option.h" > #include <gmodule.h> > > typedef bool (*PluginInitFunc)(const char *); > @@ -80,6 +85,40 @@ void qemu_plugin_load(const char *filename, const char *args) > return; > } > > +bool plugins_need_before_insn(target_ulong pc, CPUState *cpu) > +{ > + QemuPluginInfo *info; > + QLIST_FOREACH(info, &qemu_plugins, next) { > + if (info->needs_before_insn && info->needs_before_insn(pc, cpu)) { > + return true; > + } > + } > + > + return false; > +} > + > +void plugins_instrument_before_insn(target_ulong pc, CPUState *cpu) > +{ > + TCGv t_pc = tcg_const_tl(pc); > + TCGv_ptr t_cpu = tcg_const_ptr(cpu); > + /* We will dispatch plugins' callbacks in our own helper below */ > + gen_helper_before_insn(t_pc, t_cpu); > + tcg_temp_free(t_pc); > + tcg_temp_free_ptr(t_cpu); > +} OK I see what is happening here - but I worry about pushing off all plugins into a single helper call with a fairly fixed amount of information. Would it be better to expose the generate helper API and maybe a few TCG constants to the plugin function itself. It could then either emit additional TCG operations or call to the helper - and deal with the logic about if it should or shouldn't in a single call rather than doing the need_foo call first. Richard, Will the TCG be smart enough to drop the pc/t_cpu variables if they are ultimately not used by the instrument in this particular iteration? > + > +void helper_before_insn(target_ulong pc, void *cpu) > +{ > + QemuPluginInfo *info; > + QLIST_FOREACH(info, &qemu_plugins, next) { > + if (info->needs_before_insn && info->needs_before_insn(pc, cpu)) { > + if (info->before_insn) { > + info->before_insn(pc, cpu); > + } > + } > + } > +} > + > void qemu_plugins_init(void) > { > QemuPluginInfo *info; > @@ -88,4 +127,6 @@ void qemu_plugins_init(void) > info->init(info->args); > } > } > + > +#include "exec/helper-register.h" > } -- Alex Bennée
> From: Alex Bennée [mailto:alex.bennee@linaro.org] > Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> writes: > > > From: Pavel Dovgalyuk <pavel.dovgaluk@gmail.com> > > > > This is a samples of the instrumenting interface and implementation > > of some instruction tracing tasks. > > > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > > --- > > accel/tcg/translator.c | 5 +++++ > > include/qemu/instrument.h | 7 +++++++ > > plugins/helper.h | 1 + > > plugins/plugins.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 54 insertions(+) > > create mode 100644 include/qemu/instrument.h > > create mode 100644 plugins/helper.h > > > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > > index 0f9dca9..48773ac 100644 > > --- a/accel/tcg/translator.c > > +++ b/accel/tcg/translator.c > > @@ -17,6 +17,7 @@ > > #include "exec/gen-icount.h" > > #include "exec/log.h" > > #include "exec/translator.h" > > +#include "qemu/instrument.h" > > > > /* Pairs with tcg_clear_temp_count. > > To be called by #TranslatorOps.{translate_insn,tb_stop} if > > @@ -89,6 +90,10 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > > } > > } > > > > + if (plugins_need_before_insn(db->pc_next, cpu)) { > > + plugins_instrument_before_insn(db->pc_next, cpu); > > + } > > + > > I don't really see the need for a plugin_need_foo call here. Can't we > just iterate over all plugins that provide a before_insn binding and > leave it at that? > > If the plugin decides it doesn't want to do anything with this > particular instruction it can always bail early. > > > /* Disassemble one instruction. The translate_insn hook should > > update db->pc_next and db->is_jmp to indicate what should be > > done next -- either exiting this loop or locate the start of > > diff --git a/include/qemu/instrument.h b/include/qemu/instrument.h > > new file mode 100644 > > index 0000000..e8f279f > > --- /dev/null > > +++ b/include/qemu/instrument.h > > @@ -0,0 +1,7 @@ > > +#ifndef INSTRUMENT_H > > +#define INSTRUMENT_H > > + > > +bool plugins_need_before_insn(target_ulong pc, CPUState *cpu); > > +void plugins_instrument_before_insn(target_ulong pc, CPUState *cpu); > > Need empty inline cases for #ifndef CONFIG_PLUGINS > > > + > > +#endif /* INSTRUMENT_H */ > > diff --git a/plugins/helper.h b/plugins/helper.h > > new file mode 100644 > > index 0000000..007b395 > > --- /dev/null > > +++ b/plugins/helper.h > > @@ -0,0 +1 @@ > > +DEF_HELPER_2(before_insn, void, tl, ptr) > > I wonder if we should have an explicit DEF_HELPER_FLAGS_2. Looking at > the flags though: > > /* call flags */ > /* Helper does not read globals (either directly or through an exception). It > implies TCG_CALL_NO_WRITE_GLOBALS. */ > #define TCG_CALL_NO_READ_GLOBALS 0x0010 > /* Helper does not write globals */ > #define TCG_CALL_NO_WRITE_GLOBALS 0x0020 > /* Helper can be safely suppressed if the return value is not used. */ > #define TCG_CALL_NO_SIDE_EFFECTS 0x0040 > > /* convenience version of most used call flags */ > #define TCG_CALL_NO_RWG TCG_CALL_NO_READ_GLOBALS > #define TCG_CALL_NO_WG TCG_CALL_NO_WRITE_GLOBALS > #define TCG_CALL_NO_SE TCG_CALL_NO_SIDE_EFFECTS > #define TCG_CALL_NO_RWG_SE (TCG_CALL_NO_RWG | TCG_CALL_NO_SE) > #define TCG_CALL_NO_WG_SE (TCG_CALL_NO_WG | TCG_CALL_NO_SE) > > I guess no flags means machine state is fully consistent before we make > the helper call? > > > diff --git a/plugins/plugins.c b/plugins/plugins.c > > index eabc931..5a08e71 100644 > > --- a/plugins/plugins.c > > +++ b/plugins/plugins.c > > @@ -1,8 +1,13 @@ > > #include "qemu/osdep.h" > > #include "qemu-common.h" > > +#include "cpu.h" > > #include "qemu/error-report.h" > > #include "qemu/plugins.h" > > +#include "qemu/instrument.h" > > +#include "tcg/tcg.h" > > +#include "tcg/tcg-op.h" > > #include "qemu/queue.h" > > +#include "qemu/option.h" > > #include <gmodule.h> > > > > typedef bool (*PluginInitFunc)(const char *); > > @@ -80,6 +85,40 @@ void qemu_plugin_load(const char *filename, const char *args) > > return; > > } > > > > +bool plugins_need_before_insn(target_ulong pc, CPUState *cpu) > > +{ > > + QemuPluginInfo *info; > > + QLIST_FOREACH(info, &qemu_plugins, next) { > > + if (info->needs_before_insn && info->needs_before_insn(pc, cpu)) { > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > +void plugins_instrument_before_insn(target_ulong pc, CPUState *cpu) > > +{ > > + TCGv t_pc = tcg_const_tl(pc); > > + TCGv_ptr t_cpu = tcg_const_ptr(cpu); > > + /* We will dispatch plugins' callbacks in our own helper below */ > > + gen_helper_before_insn(t_pc, t_cpu); > > + tcg_temp_free(t_pc); > > + tcg_temp_free_ptr(t_cpu); > > +} > > OK I see what is happening here - but I worry about pushing off all > plugins into a single helper call with a fairly fixed amount of > information. > > Would it be better to expose the generate helper API and maybe a few TCG > constants to the plugin function itself. It could then either emit > additional TCG operations or call to the helper - and deal with the > logic about if it should or shouldn't in a single call rather than doing > the need_foo call first. The idea is to avoid TCG dependency. We even can use an interpreter without changing the API of the plugins. Therefore the helpers is the only way to do instrumentation. Pavel Dovgalyuk
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index 0f9dca9..48773ac 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -17,6 +17,7 @@ #include "exec/gen-icount.h" #include "exec/log.h" #include "exec/translator.h" +#include "qemu/instrument.h" /* Pairs with tcg_clear_temp_count. To be called by #TranslatorOps.{translate_insn,tb_stop} if @@ -89,6 +90,10 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, } } + if (plugins_need_before_insn(db->pc_next, cpu)) { + plugins_instrument_before_insn(db->pc_next, cpu); + } + /* Disassemble one instruction. The translate_insn hook should update db->pc_next and db->is_jmp to indicate what should be done next -- either exiting this loop or locate the start of diff --git a/include/qemu/instrument.h b/include/qemu/instrument.h new file mode 100644 index 0000000..e8f279f --- /dev/null +++ b/include/qemu/instrument.h @@ -0,0 +1,7 @@ +#ifndef INSTRUMENT_H +#define INSTRUMENT_H + +bool plugins_need_before_insn(target_ulong pc, CPUState *cpu); +void plugins_instrument_before_insn(target_ulong pc, CPUState *cpu); + +#endif /* INSTRUMENT_H */ diff --git a/plugins/helper.h b/plugins/helper.h new file mode 100644 index 0000000..007b395 --- /dev/null +++ b/plugins/helper.h @@ -0,0 +1 @@ +DEF_HELPER_2(before_insn, void, tl, ptr) diff --git a/plugins/plugins.c b/plugins/plugins.c index eabc931..5a08e71 100644 --- a/plugins/plugins.c +++ b/plugins/plugins.c @@ -1,8 +1,13 @@ #include "qemu/osdep.h" #include "qemu-common.h" +#include "cpu.h" #include "qemu/error-report.h" #include "qemu/plugins.h" +#include "qemu/instrument.h" +#include "tcg/tcg.h" +#include "tcg/tcg-op.h" #include "qemu/queue.h" +#include "qemu/option.h" #include <gmodule.h> typedef bool (*PluginInitFunc)(const char *); @@ -80,6 +85,40 @@ void qemu_plugin_load(const char *filename, const char *args) return; } +bool plugins_need_before_insn(target_ulong pc, CPUState *cpu) +{ + QemuPluginInfo *info; + QLIST_FOREACH(info, &qemu_plugins, next) { + if (info->needs_before_insn && info->needs_before_insn(pc, cpu)) { + return true; + } + } + + return false; +} + +void plugins_instrument_before_insn(target_ulong pc, CPUState *cpu) +{ + TCGv t_pc = tcg_const_tl(pc); + TCGv_ptr t_cpu = tcg_const_ptr(cpu); + /* We will dispatch plugins' callbacks in our own helper below */ + gen_helper_before_insn(t_pc, t_cpu); + tcg_temp_free(t_pc); + tcg_temp_free_ptr(t_cpu); +} + +void helper_before_insn(target_ulong pc, void *cpu) +{ + QemuPluginInfo *info; + QLIST_FOREACH(info, &qemu_plugins, next) { + if (info->needs_before_insn && info->needs_before_insn(pc, cpu)) { + if (info->before_insn) { + info->before_insn(pc, cpu); + } + } + } +} + void qemu_plugins_init(void) { QemuPluginInfo *info; @@ -88,4 +127,6 @@ void qemu_plugins_init(void) info->init(info->args); } } + +#include "exec/helper-register.h" }