From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqzcQ-0005Vs-9y for qemu-devel@nongnu.org; Mon, 03 Oct 2016 05:33:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bqzcJ-0004XX-Qu for qemu-devel@nongnu.org; Mon, 03 Oct 2016 05:33:05 -0400 Received: from mail-lf0-x22b.google.com ([2a00:1450:4010:c07::22b]:34486) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqzcJ-0004WO-2D for qemu-devel@nongnu.org; Mon, 03 Oct 2016 05:32:59 -0400 Received: by mail-lf0-x22b.google.com with SMTP id b81so30279648lfe.1 for ; Mon, 03 Oct 2016 02:32:58 -0700 (PDT) References: <20160930213106.20186-1-alex.bennee@linaro.org> <20160930213106.20186-4-alex.bennee@linaro.org> <3cb98ad6-1832-57af-e06d-450a031c15f1@redhat.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <3cb98ad6-1832-57af-e06d-450a031c15f1@redhat.com> Date: Mon, 03 Oct 2016 10:32:55 +0100 Message-ID: <87h98tu69k.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, mttcg@greensocs.com, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, nikunj@linux.vnet.ibm.com, mark.burton@greensocs.com, jan.kiszka@siemens.com, serge.fdrv@gmail.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite Paolo Bonzini 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 >> --- >> 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 (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 (libtsan.so.0+0x0000000254a3) #1 g_malloc (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 (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 (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 (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