Pavel Dovgalyuk <pavel.dovga...@ispras.ru> writes:
> From: Pavel Dovgalyuk <pavel.dovga...@gmail.com>
>
> This is a samples of the instrumenting interface and implementation
> of some instruction tracing tasks.
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@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
