From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cjl1p-0000gH-BE for qemu-devel@nongnu.org; Fri, 03 Mar 2017 06:05:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cjl1m-000242-6L for qemu-devel@nongnu.org; Fri, 03 Mar 2017 06:05:41 -0500 Received: from mail-wr0-x231.google.com ([2a00:1450:400c:c0c::231]:33585) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cjl1l-00023n-W2 for qemu-devel@nongnu.org; Fri, 03 Mar 2017 06:05:38 -0500 Received: by mail-wr0-x231.google.com with SMTP id u48so71308915wrc.0 for ; Fri, 03 Mar 2017 03:05:37 -0800 (PST) References: <20170302195337.31558-1-alex.bennee@linaro.org> <20170302195337.31558-5-alex.bennee@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Fri, 03 Mar 2017 11:05:39 +0000 Message-ID: <87pohy1uyk.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Richard Henderson , Paolo Bonzini , QEMU Developers , MTTCG Devel , KONRAD =?utf-8?B?RnLDqWTDqXJpYw==?= , Alvise Rigo , "Emilio G. Cota" , Pranith Kumar , Nikunj A Dadhania , Peter Crosthwaite Peter Maydell writes: > On 2 March 2017 at 19:53, Alex Bennée wrote: >> While on MTTCG hosts it is very important that updates to >> cpu->interrupt_request are protected by the BQL not all guests have >> been converted to the correct locking yet. As a result we are seeing >> breaking on non-MTTCG enabled guests in production builds. >> >> The locking in the guests needs to be fixed but while running single >> threaded they will continue to work. By moving the asserts to >> tcg_debug_asserts() they will still be useful during conversion >> work (much like the existing assert_memory_lock/assert_tb_lock >> asserts). >> >> Signed-off-by: Alex Bennée >> --- >> translate-all.c | 2 +- >> translate-common.c | 3 ++- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/translate-all.c b/translate-all.c >> index 9bac061c9b..7ee273410d 100644 >> --- a/translate-all.c >> +++ b/translate-all.c >> @@ -1928,7 +1928,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf) >> >> void cpu_interrupt(CPUState *cpu, int mask) >> { >> - g_assert(qemu_mutex_iothread_locked()); >> + tcg_debug_assert(qemu_mutex_iothread_locked()); > > If CONFIG_DEBUG_TCG isn't enabled then tcg_debug_assert() > turns into "if (!(X)) { __builtin_unreachable(); }", which > means that instead of asserting we now run straight > into compiler undefined behaviour, don't we? According to the commit that added it (c552d6c038f7cf4058d1fd5987118ffd41e0e050) it is meant to be a hint to the compiler. Reading the GCC notes however seems to contradict that. FWIW I did test it in both builds and we do used tese for a bunch of other asserts and they haven't blown up. > If what we want is "don't actually check this condition in > the non-tcg-debug config" then we should do something > that means we don't actually check the condition... Hmm: 28 intptr_t qemu_real_host_page_mask; 29 30 #ifndef CONFIG_USER_ONLY 31 /* mask must never be zero, except for A20 change call */ 32 static void tcg_handle_interrupt(CPUState *cpu, int mask) 33 { 34 int old_mask; 35 tcg_debug_assert(qemu_mutex_iothread_locked()); 36 37 old_mask = cpu->interrupt_request; Line 34 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0a but contains no code. Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a and ends at 0x24db0f . Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f but contains no code. Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f and ends at 0x24db15 . 0x24db0a : callq 0x27a570 0x24db0f : mov 0xa8(%rbx),%ebp 0x24db15 : mov %r12d,%eax 0x24db18 : mov %rbx,%rdi 0x24db1b : or %ebp,%eax 0x24db1d : mov %eax,0xa8(%rbx) 0x24db23 : callq 0x27a530 It certainly looks as though it makes the call but ignores the result? > > thanks > -- PMM -- Alex Bennée