Re: [Qemu-devel] [PATCH 5/5] tcg: move tb_invalidated_flag to CPUState

["Alex Bennée" <alex.bennee@linaro.org>]

sergey.fedorov@linaro.org writes:

> From: Paolo Bonzini <pbonzini@redhat.com>
>
> This is a baby step towards making tb_flush thread safe.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  cpu-exec.c              | 11 +++++------
>  include/exec/exec-all.h |  2 --
>  include/qom/cpu.h       |  2 ++
>  translate-all.c         |  3 +--
>  4 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index f90482eff778..07545aa91082 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -195,10 +195,11 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
>      if (max_cycles > CF_COUNT_MASK)
>          max_cycles = CF_COUNT_MASK;
>
> +    cpu->tb_invalidated_flag = 0;

We've declared as bool so lets use true/false instead of 1/0's

>      tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
>                       max_cycles | CF_NOCACHE
>                           | (ignore_icount ? CF_IGNORE_ICOUNT : 0));
> -    tb->orig_tb = tcg_ctx.tb_ctx.tb_invalidated_flag ? NULL : orig_tb;
> +    tb->orig_tb = cpu->tb_invalidated_flag ? NULL : orig_tb;
>      cpu->current_tb = tb;
>      /* execute the generated code */
>      trace_exec_tb_nocache(tb, tb->pc);
> @@ -219,8 +220,6 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
>      tb_page_addr_t phys_pc, phys_page1;
>      target_ulong virt_page2;
>
> -    tcg_ctx.tb_ctx.tb_invalidated_flag = 0;
> -
>      /* find translated block using physical mappings */
>      phys_pc = get_page_addr_code(env, pc);
>      phys_page1 = phys_pc & TARGET_PAGE_MASK;
> @@ -288,6 +287,7 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>  #endif
>
>      /* if no translated code available, then translate it now */
> +    cpu->tb_invalidated_flag = 0;
>      tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
>
>  #ifdef CONFIG_USER_ONLY
> @@ -493,12 +493,11 @@ int cpu_exec(CPUState *cpu)
>                  tb = tb_find_fast(cpu);
>                  /* Note: we do it here to avoid a gcc bug on Mac OS X when
>                     doing it in tb_find_slow */
> -                if (tcg_ctx.tb_ctx.tb_invalidated_flag) {
> +                if (cpu->tb_invalidated_flag) {
>                      /* as some TB could have been invalidated because
> -                       of memory exceptions while generating the code, we
> +                       of a tb_flush while generating the code, we
>                         must recompute the hash index here */
>                      next_tb = 0;
> -                    tcg_ctx.tb_ctx.tb_invalidated_flag = 0;
>                  }
>                  if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
>                      qemu_log("Trace %p [" TARGET_FMT_lx "] %s\n",
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 05a151da4a54..0ef6ea5cf6dc 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -288,8 +288,6 @@ struct TBContext {
>      /* statistics */
>      int tb_flush_count;
>      int tb_phys_invalidate_count;
> -
> -    int tb_invalidated_flag;
>  };
>
>  void tb_free(TranslationBlock *tb);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7052eee7b78a..9538f9cc2af3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -240,6 +240,7 @@ struct kvm_run;
>   * @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
>   * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
>   *           CPU and return to its top level loop.
> + * @tb_invalidated_flag: Set to tell TCG that tb_flush has been called.
>   * @singlestep_enabled: Flags for single-stepping.
>   * @icount_extra: Instructions until next timer event.
>   * @icount_decr: Number of cycles left, with interrupt flag in high bit.
> @@ -291,6 +292,7 @@ struct CPUState {
>      bool stopped;
>      bool crash_occurred;
>      bool exit_request;
> +    bool tb_invalidated_flag;

s/_flag// would save a few characters given it should be obvious it is a
flag from the setting of true and false?

>      uint32_t interrupt_request;
>      int singlestep_enabled;
>      int64_t icount_extra;
> diff --git a/translate-all.c b/translate-all.c
> index 1db5a914d9a3..8e1edd6bb633 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -843,6 +843,7 @@ void tb_flush(CPUState *cpu)
>      tcg_ctx.tb_ctx.nb_tbs = 0;
>
>      CPU_FOREACH(cpu) {
> +        cpu->tb_invalidated_flag = 1;
>          memset(cpu->tb_jmp_cache, 0, sizeof(cpu->tb_jmp_cache));
>      }
>
> @@ -1079,8 +1080,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>          /* cannot fail at this point */
>          tb = tb_alloc(pc);
>          assert(tb != NULL);
> -        /* Don't forget to invalidate previous TB info.  */
> -        tcg_ctx.tb_ctx.tb_invalidated_flag = 1;
>      }
>
>      gen_code_buf = tcg_ctx.code_gen_ptr;

I also note there is some code motion about where these flags are set
and cleared which should probably be mentioned in the commit message.

--
Alex Bennée