Re: [Qemu-devel] [PATCH v3 06/11] tcg: Introduce tb_mark_invalid() and tb_is_invalid()

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> On 14/07/16 15:53, Alex Bennée wrote:
>> Sergey Fedorov <address@hidden> writes:
>>
>>> From: Sergey Fedorov <address@hidden>
>>>
>>> These functions will be used to make translation block invalidation safe
>>> with concurrent lockless lookup in the global hash table.
>>>
>>> Most targets don't use 'cs_base'; so marking TB as invalid is as simple
>>> as assigning -1 to 'cs_base'. SPARC target stores the next program
>>> counter into 'cs_base', and -1 is a fine invalid value since PC must bet
>>> a multiple of 4 in SPARC. The only odd target is i386, for which a
>>> special flag is introduced in place of removed 'HF_SOFTMMU_MASK'.
>>>
>>> Suggested-by: Paolo Bonzini <address@hidden>
>>> Signed-off-by: Sergey Fedorov <address@hidden>
>>> Signed-off-by: Sergey Fedorov <address@hidden>
>>> ---
>>>  include/exec/exec-all.h  | 10 ++++++++++
>>>  target-alpha/cpu.h       | 14 ++++++++++++++
>>>  target-arm/cpu.h         | 14 ++++++++++++++
>>>  target-cris/cpu.h        | 14 ++++++++++++++
>>>  target-i386/cpu.h        | 17 +++++++++++++++++
>>>  target-lm32/cpu.h        | 14 ++++++++++++++
>>>  target-m68k/cpu.h        | 14 ++++++++++++++
>>>  target-microblaze/cpu.h  | 14 ++++++++++++++
>>>  target-mips/cpu.h        | 14 ++++++++++++++
>>>  target-moxie/cpu.h       | 14 ++++++++++++++
>>>  target-openrisc/cpu.h    | 14 ++++++++++++++
>>>  target-ppc/cpu.h         | 14 ++++++++++++++
>>>  target-s390x/cpu.h       | 14 ++++++++++++++
>>>  target-sh4/cpu.h         | 14 ++++++++++++++
>>>  target-sparc/cpu.h       | 14 ++++++++++++++
>>>  target-sparc/translate.c |  1 +
>>>  target-tilegx/cpu.h      | 14 ++++++++++++++
>>>  target-tricore/cpu.h     | 14 ++++++++++++++
>>>  target-unicore32/cpu.h   | 14 ++++++++++++++
>>>  target-xtensa/cpu.h      | 14 ++++++++++++++
>>>  20 files changed, 266 insertions(+)
>>>
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index db79ab65cebe..61cc3a1fb8f7 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -256,6 +256,16 @@ void tb_free(TranslationBlock *tb);
>>>  void tb_flush(CPUState *cpu);
>>>  void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
>>>
>>> +static inline void tb_mark_invalid(TranslationBlock *tb)
>>> +{
>>> +    cpu_get_invalid_tb_cpu_state(&tb->pc, &tb->cs_base, &tb->flags);
>> Hmmm are we getting something here? Maybe cpu_tb_invalidate_cpu_state?
>
> Just to be similar to cpu_get_tb_cpu_state().

I guess if you squint just right. I'll drop the objection.

>
>>
>>> +}
>>> +
>>> +static inline bool tb_is_invalid(TranslationBlock *tb)
>>> +{
>>> +    return cpu_tb_cpu_state_is_invalidated(tb->pc, tb->cs_base, tb->flags);
>>> +}
>> Also why are we passing three pointers to parts of TranslationBlock? Why
>> not just pass tb directly and be done with it?
>
> I'm not sure we want to include exec/exec-all.h in target-*/cpu.h

Fair enough.

>
>>
>> I'm sure the compiler does something sensible but seems overly verbose
>> to me.
>>
>>> +
>>>  #if defined(USE_DIRECT_JUMP)
>>>
>>>  #if defined(CONFIG_TCG_INTERPRETER)
> (snip)
>> Otherwise:
>>
>> Reviewed-by: Alex Bennée <address@hidden>
>
> Kind regards,
> Sergey


--
Alex Bennée