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

[Richard Henderson <rth@twiddle.net>]

On 06/14/2017 05:36 PM, 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).
> 
> 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(-)

Applied, thanks.


r~