Re: [Qemu-devel] [RFC v2 05/11] tcg: protect TBContext with tb_lock.

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> On 05/04/16 18:32, Alex Bennée wrote:
> (snip)
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index 74065d9..bd50fef 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -205,18 +205,24 @@ static void cpu_exec_nocache(CPUState *cpu, int 
>> max_cycles,
>>      if (max_cycles > CF_COUNT_MASK)
>>          max_cycles = CF_COUNT_MASK;
>>
>> +    tb_lock();
>>      cpu->tb_invalidated_flag = false;
>>      tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
>>                       max_cycles | CF_NOCACHE
>>                           | (ignore_icount ? CF_IGNORE_ICOUNT : 0));
>>      tb->orig_tb = cpu->tb_invalidated_flag ? NULL : orig_tb;
>>      cpu->current_tb = tb;
>> +    tb_unlock();
>> +
>>      /* execute the generated code */
>>      trace_exec_tb_nocache(tb, tb->pc);
>> -    cpu_tb_exec(cpu, tb);
>> +    cpu_tb_exec(cpu, tb->tc_ptr);
>
> Very suspicious change. I can't even find which patch changes
> cpu_tb_exec() accordingly.

I think that came from a patch this series was based on.
It's gone now.

>
>> +
>> +    tb_lock();
>>      cpu->current_tb = NULL;
>>      tb_phys_invalidate(tb, -1);
>>      tb_free(tb);
>> +    tb_unlock();
>>  }
>>  #endif
>>
>> diff --git a/exec.c b/exec.c
>> index 17f390e..c46c123 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2111,6 +2111,9 @@ static void check_watchpoint(int offset, int len, 
>> MemTxAttrs attrs, int flags)
>>                      continue;
>>                  }
>>                  cpu->watchpoint_hit = wp;
>> +
>> +                /* Unlocked by cpu_loop_exit or cpu_resume_from_signal.  */
>
> In fact, neither cpu_resume_from_signal() nor cpu_loop_exit() unlocks
> the lock by itself, it gets unlocked after sigsetjmp() returns via
> siglongjmp() back to cpu_exec(). So maybe it would be more clear to say
> something like "'tb_lock' gets unlocked after siglongjmp()"?


"Locks are reset when we longjmp back to the main cpu_exec loop"?

Looking at where the patch is though I think I need to bring that bit
forward from the main series.

>
>> +                tb_lock();
>>                  tb_check_watchpoint(cpu);
>>                  if (wp->flags & BP_STOP_BEFORE_ACCESS) {
>>                      cpu->exception_index = EXCP_DEBUG;
> (snip)
>> diff --git a/translate-all.c b/translate-all.c
>> index a7ff5e7..935d24c 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -834,7 +834,9 @@ static void page_flush_tb(void)
>>  }
>>
>>  /* flush all the translation blocks */
>> -/* XXX: tb_flush is currently not thread safe */
>> +/* XXX: tb_flush is currently not thread safe.  System emulation calls it 
>> only
>> + * with tb_lock taken or from safe_work, so no need to take tb_lock here.
>> + */
>
> "System emulation"? What about user-mode emulation?

It's still not thread safe ;-)

It's a harder problem to solve because we can't just suspend all
threads to reset the translation buffer. I'm not sure we want to try and
fix it in this series.

>
>>  void tb_flush(CPUState *cpu)
>>  {
>>  #if defined(DEBUG_FLUSH)
>> @@ -1350,6 +1352,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t 
>> start, tb_page_addr_t end,
>>      /* we remove all the TBs in the range [start, end[ */
>>      /* XXX: see if in some cases it could be faster to invalidate all
>>         the code */
>> +    tb_lock();
>
> Don't we need also protect a call to page_find() above? page_find()
> calls page_find_alloc() which is noted to be called with 'tb_lock' held.
> However, it might depend on the way we treat 'mmap_lock' in system mode
> emulation. We might also consider taking the lock outside of
> tb_invalidate_phys*() functions because they can be called after
> page_find().
>
>>      tb = p->first_tb;
>>      while (tb != NULL) {
>>          n = (uintptr_t)tb & 3;
>> @@ -1417,12 +1420,13 @@ void tb_invalidate_phys_page_range(tb_page_addr_t 
>> start, tb_page_addr_t end,
>>      if (current_tb_modified) {
>>          /* we generate a block containing just the instruction
>>             modifying the memory. It will ensure that it cannot modify
>> -           itself */
>> +           itself.  cpu_resume_from_signal unlocks tb_lock.  */
>>          cpu->current_tb = NULL;
>>          tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
>>          cpu_resume_from_signal(cpu, NULL);
>>      }
>>  #endif
>> +    tb_unlock();
>>  }
>>
>>  #ifdef CONFIG_SOFTMMU
> (snip)
>> @ -1627,6 +1636,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>>      target_ulong pc, cs_base;
>>      uint64_t flags;
>>
>> +    tb_lock();
>
> We don't have to take 'tb_lock' for nether tb_find_pc() nor
> cpu_restore_state_from_tb() because the lock does not protect from
> tb_flush() anyway. I think the lock should be taken just before the
> first call to tb_phys_invalidate() in this function.
>
>>      tb = tb_find_pc(retaddr);
>>      if (!tb) {
>>          cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
>> @@ -1678,11 +1688,15 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t 
>> retaddr)
>>      /* FIXME: In theory this could raise an exception.  In practice
>>         we have already translated the block once so it's probably ok.  */
>>      tb_gen_code(cpu, pc, cs_base, flags, cflags);
>> -    /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
>> -       the first in the TB) then we end up generating a whole new TB and
>> -       repeating the fault, which is horribly inefficient.
>> -       Better would be to execute just this insn uncached, or generate a
>> -       second new TB.  */
>> +
>> +    /* This unlocks the tb_lock.
>> +     *
>> +     * TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
>> +     * the first in the TB) then we end up generating a whole new TB and
>> +     * repeating the fault, which is horribly inefficient.
>> +     * Better would be to execute just this insn uncached, or generate a
>> +     * second new TB.
>> +     */
>>      cpu_resume_from_signal(cpu, NULL);
>>  }
> (snip)
>
> Kind regards,
> Sergey


--
Alex Bennée