On 10/7/19 11:28 AM, Alex Bennée wrote:
> +void HELPER(inc_exec_freq)(void *ptr)
> +{
> + TBStatistics *stats = (TBStatistics *) ptr;
> + g_assert(stats);
> + atomic_inc(&stats->executions.normal);
> +}
tcg_debug_assert.
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 114ebe48bf..b7dd1a78e5 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1784,10 +1784,17 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> /*
> * We want to fetch the stats structure before we start code
> * generation so we can count interesting things about this
> - * generation.
> + * generation. If dfilter is in effect we will only collect stats
> + * for the specified range.
> */
> - if (tb_stats_collection_enabled()) {
> + if (tb_stats_collection_enabled() &&
> + qemu_log_in_addr_range(tb->pc)) {
> + uint32_t flag = get_default_tbstats_flag();
> tb->tb_stats = tb_get_stats(phys_pc, pc, cs_base, flags);
> +
> + if (flag & TB_EXEC_STATS) {
> + tb->tb_stats->stats_enabled |= TB_EXEC_STATS;
> + }
Is this intended to be
tb->tb_stats->stats_enabled =
(tb->tb_stats->stats_enabled & ~TB_EXEC_STATS)
| (flag & TB_EXEC_STATS);
so that the flag eventually gets cleared? I also don't understand placing
stats_enabled within a structure that is shared between multiple TB.
I can only imagine that TB_EXEC_STATS should really be a bit in tb->cflags. It
wouldn't need to be in CF_HASH_MASK, but that seems to be the logical place to
put it.
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 70c66c538c..ec6bd829a0 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -46,6 +46,7 @@ void translator_loop(const TranslatorOps *ops,
> DisasContextBase *db,
>
> ops->init_disas_context(db, cpu);
> tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */
> + gen_tb_exec_count(tb);
Too early. This should go after gen_tb_start().
> /* Reset the temp count so that we can identify leaks */
> tcg_clear_temp_count();
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index 822c43cfd3..be006383b9 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -32,6 +32,15 @@ static inline void gen_io_end(void)
> tcg_temp_free_i32(tmp);
> }
>
> +static inline void gen_tb_exec_count(TranslationBlock *tb)
> +{
> + if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
> + TCGv_ptr ptr = tcg_const_ptr(tb->tb_stats);
> + gen_helper_inc_exec_freq(ptr);
> + tcg_temp_free_ptr(ptr);
> + }
> +}
I think this could go into translator.c, instead of gen-icount.h; it shouldn't
be used anywhere else.
> +#define TB_NOTHING (1 << 0)
What's this?
r~
