diff mbox

[v3,13/15] tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write

Message ID 20160930213106.20186-14-alex.bennee@linaro.org
State New
Headers show

Commit Message

Alex Bennée Sept. 30, 2016, 9:31 p.m. UTC 
To meet C11 semantics for shared data access we need to use relaxed
atomic accesses. While the completion of data writes w.r.t reads is
ensured by QHT's explicit barriers when a newly generated TB is inserted
ThreadSanitizer will still complain. By using the relaxed accesses the
same code gets generated but instrumentation does not have to worry
about a potentially undefined interaction between plain loads/stores.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 cpu-exec.c      | 6 +++---
 translate-all.c | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.9.3

Comments

Alex Bennée Oct. 3, 2016, 9:48 a.m. UTC | #1 
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/09/2016 23:31, Alex Bennée wrote:

>>      tb = atomic_rcu_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)]);

>> -    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||

>> -                 tb->flags != flags)) {

>> +    if (unlikely(!tb || atomic_read(&tb->pc) != pc || atomic_read(&tb->cs_base) != cs_base ||

>> +                 atomic_read(&tb->flags) != flags)) {

>

> This should not be necessary (and is responsible for the 64-on-32

> compilation failure).  The load of tb from the cache is an acquire

> operation, and synchronizes with the corresponding store in

> cpu->tb_jmp_cache.


Is the C11 spec happy with "plain" accesses after the acquire operation?
Unfortunately the sanitizer isn't able to see the indirect acquires
effect on the other accesses.

>

> Paolo



--
Alex Bennée
diff mbox

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index e114fcd..99c906b 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -140,7 +140,7 @@  static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb)
     uintptr_t ret;
     TranslationBlock *last_tb;
     int tb_exit;
-    uint8_t *tb_ptr = itb->tc_ptr;
+    uint8_t *tb_ptr = atomic_read(&itb->tc_ptr);
 
     qemu_log_mask_and_addr(CPU_LOG_EXEC, itb->pc,
                            "Trace %p [" TARGET_FMT_lx "] %s\n",
@@ -291,8 +291,8 @@  static inline TranslationBlock *tb_find(CPUState *cpu,
        is executed. */
     cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
     tb = atomic_rcu_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)]);
-    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
-                 tb->flags != flags)) {
+    if (unlikely(!tb || atomic_read(&tb->pc) != pc || atomic_read(&tb->cs_base) != cs_base ||
+                 atomic_read(&tb->flags) != flags)) {
         tb = tb_htable_lookup(cpu, pc, cs_base, flags);
         if (!tb) {
 
diff --git a/translate-all.c b/translate-all.c
index 8ca393c..0f13d4d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1198,10 +1198,10 @@  TranslationBlock *tb_gen_code(CPUState *cpu,
     }
 
     gen_code_buf = tcg_ctx.code_gen_ptr;
-    tb->tc_ptr = gen_code_buf;
-    tb->cs_base = cs_base;
-    tb->flags = flags;
-    tb->cflags = cflags;
+    atomic_set(&tb->tc_ptr, gen_code_buf);
+    atomic_set(&tb->cs_base, cs_base);
+    atomic_set(&tb->flags, flags);
+    atomic_set(&tb->cflags, cflags);
 
 #ifdef CONFIG_PROFILER
     tcg_ctx.tb_count1++; /* includes aborted translations because of