Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong

[Alex Bennée]

Paolo Bonzini <address@hidden> writes:

> On 30/09/2016 23:30, Alex Bennée wrote:
>> Commit b480d9b74 converted tb_page_addr_t to abi_ulong which while the
>> right size imposes additional alignment restrictions on the type. This
>> gets in the way of using atomic accesses on certain guest platforms
>> which allow finer alignments. As tb_page_addr_t isn't actually visible
>> to the guest we can revert the change.
>>
>> This is potentially less efficient for ILP32 style guests but it is the
>> simpler change to make.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>> ---
>>  include/exec/exec-all.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 336a57c..c3596a6 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -30,7 +30,7 @@
>>     addresses in userspace mode.  Define tb_page_addr_t to be an appropriate
>>     type.  */
>>  #if defined(CONFIG_USER_ONLY)
>> -typedef abi_ulong tb_page_addr_t;
>> +typedef target_ulong tb_page_addr_t;
>>  #else
>>  typedef ram_addr_t tb_page_addr_t;
>>  #endif
>>
>
> I think target_ulong is in general not accessible atomically, because we
> still haven't dropped 64-on-32 support.

True. I've been thinking about our options here.

1. For atomic_read/set fall back to plain access for sizeof(*ptr) >
sizeof(void *)

This would throw up warnings in ThreadSanitizer on 64-on-32 but
practically generate the same code as before.

2. Split sizeof(*ptr) > sizeof(void *) accesses over two

This would keep the sanitizer happy but be incorrect behaviour, you
could get tears.

3. Add a generic 64-on-32 lock for these accesses

It would be a global lock for any oversized access which could end up
being highly contended but at least any behaviour would be always
correct.

It's tricky because pretty much all of the atomic_set/read use is purely
for C11 correctness, actual sequential correctness is guaranteed by
using more restrictive primitives and/or additional locking. I'm not
suggesting we support 64-on-32 for any of the more strict primitives.

However the series as a whole does have value. As you can see from the
other patches there are some real races being picked up by the sanitizer
which only really become visible when a) you remove the noise of the
"false" positives and b) run the test many many times. For example this
one:

==================
WARNING: ThreadSanitizer: data race (pid=24906)
  Read of size 8 at 0x7db4000261f0 by thread T3 (mutexes: write M8203):
    #0 do_tb_flush /home/alex/lsrc/qemu/qemu.git/translate-all.c:872 
(qemu-arm+0x00006000ce68)
    #1 process_queued_cpu_work /home/alex/lsrc/qemu/qemu.git/cpus-common.c:337 
(qemu-arm+0x000060116712)
    #2 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:654 
(qemu-arm+0x000060052213)
    #3 clone_func /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6070 
(qemu-arm+0x0000600686fb)
    #4 <null> <null> (libtsan.so.0+0x0000000230d9)

  Previous write of size 8 at 0x7db4000261f0 by main thread (mutexes: write M8):
    #0 cpu_list_add /home/alex/lsrc/qemu/qemu.git/cpus-common.c:87 
(qemu-arm+0x000060115b7a)
    #1 cpu_exec_init /home/alex/lsrc/qemu/qemu.git/exec.c:641 
(qemu-arm+0x000060009900)
    #2 arm_cpu_initfn /home/alex/lsrc/qemu/qemu.git/target-arm/cpu.c:447 
(qemu-arm+0x0000600f833b)
    #3 object_init_with_type qom/object.c:339 (qemu-arm+0x000060156c73)
    #4 object_init_with_type qom/object.c:335 (qemu-arm+0x000060156c35)
    #5 object_initialize_with_type qom/object.c:370 (qemu-arm+0x000060156f35)
    #6 object_new_with_type qom/object.c:478 (qemu-arm+0x00006015763a)
    #7 object_new qom/object.c:488 (qemu-arm+0x00006015769e)
    #8 cpu_generic_init qom/cpu.c:76 (qemu-arm+0x0000601537f4)
    #9 cpu_arm_init /home/alex/lsrc/qemu/qemu.git/target-arm/helper.c:5101 
(qemu-arm+0x0000600eed19)
    #10 cpu_copy /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:3744 
(qemu-arm+0x000060053516)
    #11 do_fork /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6107 
(qemu-arm+0x000060068832)
    #12 do_syscall /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:9489 
(qemu-arm+0x000060073bcc)
    #13 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 
(qemu-arm+0x00006005305e)
    #14 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 
(qemu-arm+0x0000600555c9)

  Location is heap block of size 37200 at 0x7db40001e000 allocated by main 
thread:
    #0 malloc <null> (libtsan.so.0+0x0000000254a3)
    #1 g_malloc <null> (libglib-2.0.so.0+0x00000004f728)
    #2 object_new qom/object.c:488 (qemu-arm+0x00006015769e)
    #3 cpu_generic_init qom/cpu.c:76 (qemu-arm+0x0000601537f4)
    #4 cpu_arm_init /home/alex/lsrc/qemu/qemu.git/target-arm/helper.c:5101 
(qemu-arm+0x0000600eed19)
    #5 cpu_copy /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:3744 
(qemu-arm+0x000060053516)
    #6 do_fork /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6107 
(qemu-arm+0x000060068832)
    #7 do_syscall /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:9489 
(qemu-arm+0x000060073bcc)
    #8 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 
(qemu-arm+0x00006005305e)
    #9 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 
(qemu-arm+0x0000600555c9)

  Mutex M8203 (0x0000604ad638) created at:
    #0 pthread_mutex_init <null> (libtsan.so.0+0x0000000280b5)
    #1 qemu_mutex_init util/qemu-thread-posix.c:46 (qemu-arm+0x0000601be220)
    #2 code_gen_alloc /home/alex/lsrc/qemu/qemu.git/translate-all.c:739 
(qemu-arm+0x00006000c7ce)
    #3 tcg_exec_init /home/alex/lsrc/qemu/qemu.git/translate-all.c:757 
(qemu-arm+0x00006000c845)
    #4 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4250 
(qemu-arm+0x000060054aca)

  Mutex M8 (0x000060508920) created at:
    #0 pthread_mutex_init <null> (libtsan.so.0+0x0000000280b5)
    #1 qemu_mutex_init util/qemu-thread-posix.c:46 (qemu-arm+0x0000601be220)
    #2 qemu_init_cpu_list /home/alex/lsrc/qemu/qemu.git/cpus-common.c:42 
(qemu-arm+0x000060115973)
    #3 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4161 
(qemu-arm+0x000060054842)

  Thread T3 (tid=24910, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x000000027577)
    #1 do_fork /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6147 
(qemu-arm+0x000060068c1a)
    #2 do_syscall /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:9489 
(qemu-arm+0x000060073bcc)
    #3 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 
(qemu-arm+0x00006005305e)
    #4 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 
(qemu-arm+0x0000600555c9)

SUMMARY: ThreadSanitizer: data race 
/home/alex/lsrc/qemu/qemu.git/translate-all.c:872 do_tb_flush
==================

which showed up on run 619 of a 1000 run test...


>
> Paolo


--
Alex Bennée