Re: [Qemu-devel] [PATCH v3 13/15] tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write

[Paolo Bonzini]

On 03/10/2016 11:48, Alex Bennée wrote:
> 
> Paolo Bonzini <address@hidden> 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?

Only if there are no concurrent atomic writes---but there shouldn't be, 
since those three fields are immutable after tb_alloc, and in turn 
tb_alloc's assignments are not visible until qht publishes them.

It's actually a bit more complex, and if I understand the 
ramifications correctly the "barrier" in 

#define smp_wmb()  ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); })

might not be required at all.  I need to run this by the memory model
heavyweights.  So it's possible that this is a sanitizer bug.

Paolo

> Unfortunately the sanitizer isn't able to see the indirect acquires
> effect on the other accesses.
> 
>>
>> Paolo
> 
> 
> --
> Alex Bennée
>