Re: [Qemu-devel] [PATCH] tcg: consistently access cpu->tb_jmp_cache atomically

[Paolo Bonzini <pbonzini@redhat.com>]

On 15/06/2017 02:36, Emilio G. Cota wrote:
> Some code paths can lead to atomic accesses racing with memset()
> on cpu->tb_jmp_cache, which can result in torn reads/writes
> and is undefined behaviour in C11.
> 
> These torn accesses are unlikely to show up as bugs, but from code
> inspection they seem possible. For example, tb_phys_invalidate does:
>     /* remove the TB from the hash list */
>     h = tb_jmp_cache_hash_func(tb->pc);
>     CPU_FOREACH(cpu) {
>         if (atomic_read(&cpu->tb_jmp_cache[h]) == tb) {
>             atomic_set(&cpu->tb_jmp_cache[h], NULL);
>         }
>     }
> Here atomic_set might race with a concurrent memset (such as the
> ones scheduled via "unsafe" async work, e.g. tlb_flush_page) and
> therefore we might end up with a torn pointer (or who knows what,
> because we are under undefined behaviour).

Also atomic_read could, which is worse (though in this case it would
only lead to a useless NULL write).

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> 
> This patch converts parallel accesses to cpu->tb_jmp_cache to use
> atomic primitives, thereby bringing these accesses back to defined
> behaviour. The price to pay is to potentially execute more instructions
> when clearing cpu->tb_jmp_cache, but given how infrequently they happen
> and the small size of the cache, the performance impact I have measured
> is within noise range when booting debian-arm.
> 
> Note that under "safe async" work (e.g. do_tb_flush) we could use memset
> because no other vcpus are running. However I'm keeping these accesses
> atomic as well to keep things simple and to avoid confusing analysis
> tools such as ThreadSanitizer.
> 
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  cputlb.c          |  4 ++--
>  include/qom/cpu.h | 11 ++++++++++-
>  qom/cpu.c         |  5 +----
>  translate-all.c   | 25 ++++++++++++-------------
>  4 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 743776a..e85b6d3 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -118,7 +118,7 @@ static void tlb_flush_nocheck(CPUState *cpu)
>  
>      memset(env->tlb_table, -1, sizeof(env->tlb_table));
>      memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table));
> -    memset(cpu->tb_jmp_cache, 0, sizeof(cpu->tb_jmp_cache));
> +    cpu_tb_jmp_cache_clear(cpu);
>  
>      env->vtlb_index = 0;
>      env->tlb_flush_addr = -1;
> @@ -183,7 +183,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, run_on_cpu_data data)
>          }
>      }
>  
> -    memset(cpu->tb_jmp_cache, 0, sizeof(cpu->tb_jmp_cache));
> +    cpu_tb_jmp_cache_clear(cpu);
>  
>      tlb_debug("done\n");
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 89ddb68..2fe7cff 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -346,7 +346,7 @@ struct CPUState {
>  
>      void *env_ptr; /* CPUArchState */
>  
> -    /* Writes protected by tb_lock, reads not thread-safe  */
> +    /* Accessed in parallel; all accesses must be atomic */
>      struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
>  
>      struct GDBRegisterState *gdb_regs;
> @@ -422,6 +422,15 @@ extern struct CPUTailQ cpus;
>  
>  extern __thread CPUState *current_cpu;
>  
> +static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < TB_JMP_CACHE_SIZE; i++) {
> +        atomic_set(&cpu->tb_jmp_cache[i], NULL);
> +    }
> +}
> +
>  /**
>   * qemu_tcg_mttcg_enabled:
>   * Check whether we are running MultiThread TCG or not.
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 5069876..585419b 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -274,7 +274,6 @@ void cpu_reset(CPUState *cpu)
>  static void cpu_common_reset(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -    int i;
>  
>      if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>          qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
> @@ -292,9 +291,7 @@ static void cpu_common_reset(CPUState *cpu)
>      cpu->crash_occurred = false;
>  
>      if (tcg_enabled()) {
> -        for (i = 0; i < TB_JMP_CACHE_SIZE; ++i) {
> -            atomic_set(&cpu->tb_jmp_cache[i], NULL);
> -        }
> +        cpu_tb_jmp_cache_clear(cpu);
>  
>  #ifdef CONFIG_SOFTMMU
>          tlb_flush(cpu, 0);
> diff --git a/translate-all.c b/translate-all.c
> index b3ee876..af3c4df 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -923,11 +923,7 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
>      }
>  
>      CPU_FOREACH(cpu) {
> -        int i;
> -
> -        for (i = 0; i < TB_JMP_CACHE_SIZE; ++i) {
> -            atomic_set(&cpu->tb_jmp_cache[i], NULL);
> -        }
> +        cpu_tb_jmp_cache_clear(cpu);
>      }
>  
>      tcg_ctx.tb_ctx.nb_tbs = 0;
> @@ -1806,19 +1802,22 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>      cpu_loop_exit_noexc(cpu);
>  }
>  
> -void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr)
> +static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
>  {
> +    unsigned int i0 = tb_jmp_cache_hash_page(page_addr);
>      unsigned int i;
>  
> +    for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
> +        atomic_set(&cpu->tb_jmp_cache[i0 + i], NULL);
> +    }
> +}
> +
> +void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr)
> +{
>      /* Discard jump cache entries for any tb which might potentially
>         overlap the flushed page.  */
> -    i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> -    memset(&cpu->tb_jmp_cache[i], 0,
> -           TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> -
> -    i = tb_jmp_cache_hash_page(addr);
> -    memset(&cpu->tb_jmp_cache[i], 0,
> -           TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> +    tb_jmp_cache_clear_page(cpu, addr - TARGET_PAGE_SIZE);
> +    tb_jmp_cache_clear_page(cpu, addr);
>  }
>  
>  static void print_qht_statistics(FILE *f, fprintf_function cpu_fprintf,
>