Re: [Qemu-devel] tcg/translate-all.c:169: tb_lock: Assertion `!have_tb_lock' failed when doing cpu_restore_state in usermode

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/10/2017 12:52, Peter Maydell wrote:
> On 10 October 2017 at 11:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> I've seen the same on x86.  Using the program counter from translate.c
>> here looks very fishy:
>>
>>     /* Now we have a real cpu fault.  Since this is the exact location of
>>      * the exception, we must undo the adjustment done by cpu_restore_state
>>      * for handling call return addresses.  */
>>     cpu_restore_state(cpu, pc + GETPC_ADJ);
> 
> This is the right thing if the signal happened directly from
> translated code (as it will for guest reads/writes). This
> bit of code expects that cpu_restore_state() will just do nothing
> if the pc value isn't actually in a TB.

Yes, it is right if it happens from translated code.  But
cpu_restore_code currently expects either retaddr == 0 or retaddr from a
TB.  And generalizing those expectations...

>> and cpu_restore_state would just return false because tb_find_pc fails.  Maybe
>> something like this?
>>
>> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>> index 492ea0826c..66a4351b96 100644
>> --- a/accel/tcg/user-exec.c
>> +++ b/accel/tcg/user-exec.c
>> @@ -119,10 +119,13 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
>>          return 1; /* the MMU fault was handled without causing real CPU fault */
>>      }
>>
>> -    /* Now we have a real cpu fault.  Since this is the exact location of
>> -     * the exception, we must undo the adjustment done by cpu_restore_state
>> -     * for handling call return addresses.  */
>> -    cpu_restore_state(cpu, pc + GETPC_ADJ);
>> +    if (pc >= (uintptr_t)tcg_ctx.code_gen_buffer &&
>> +        pc < (uintptr_t)tcg_ctx.code_gen_ptr) {
>> +        /* Now we have a real cpu fault.  Since this is the exact location of
>> +         * the exception, we must undo the adjustment done by cpu_restore_state
>> +         * for handling call return addresses.  */
>> +        cpu_restore_state(cpu, pc + GETPC_ADJ);
>> +    }
> 
> I think that would be better inside cpu_restore_state(), because it's
> an internal detail of the TCG accelerator that it happens to put
> all its code inside those bounds.

... makes sense too, and it can replace the existing check for !retaddr
(introduced in commit d8b2239bcd, "translate-all: exit cpu_restore_state
early if translating", 2017-03-09) which only works for softmmu.

Thanks,

Paolo