Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 2 March 2017 at 19:53, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>
>> ---
>>  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 <tcg_handle_interrupt+10> but contains no code.
Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a <tcg_handle_interrupt+10> and ends at 0x24db0f <tcg_handle_interrupt+15>.
Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f <tcg_handle_interrupt+15> but contains no code.
Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f <tcg_handle_interrupt+15> and ends at 0x24db15 <tcg_handle_interrupt+21>.
   0x24db0a <tcg_handle_interrupt+10>:	callq  0x27a570 <qemu_mutex_iothread_locked>
   0x24db0f <tcg_handle_interrupt+15>:	mov    0xa8(%rbx),%ebp
   0x24db15 <tcg_handle_interrupt+21>:	mov    %r12d,%eax
   0x24db18 <tcg_handle_interrupt+24>:	mov    %rbx,%rdi
   0x24db1b <tcg_handle_interrupt+27>:	or     %ebp,%eax
   0x24db1d <tcg_handle_interrupt+29>:	mov    %eax,0xa8(%rbx)
   0x24db23 <tcg_handle_interrupt+35>:	callq  0x27a530 <qemu_cpu_is_self>

It certainly looks as though it makes the call but ignores the result?

>
> thanks
> -- PMM


--
Alex Bennée