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

[Sergey Fedorov <serge.fdrv@gmail.com>]

On 01/06/16 13:30, Alex Bennée wrote:
> Sergey Fedorov <serge.fdrv@gmail.com> writes:
>
>> On 05/04/16 18:32, Alex Bennée wrote:
>> (snip)
>>> 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"?

Yes, it this looks fine.

> 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.

I think it could be possible to do something like start_exclusive() to
achieve this in user-only emulation.

>>>  void tb_flush(CPUState *cpu)
>>>  {
>>>  #if defined(DEBUG_FLUSH)

Kind regards,
Sergey