Richard Henderson <richard.hender...@linaro.org> writes:
> On 9/29/22 06:46, Alex Bennée wrote:
>> Richard Henderson <richard.hender...@linaro.org> writes:
>>
>>> Wrap the bare TranslationBlock pointer into a structure.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
>>> ---
>>> include/hw/core/cpu.h | 8 ++++++--
>>> accel/tcg/cpu-exec.c | 9 ++++++---
>>> accel/tcg/cputlb.c | 2 +-
>>> accel/tcg/translate-all.c | 4 ++--
>>> 4 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index 9e47184513..ee5b75dea0 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -232,6 +232,10 @@ struct hvf_vcpu_state;
>>> #define TB_JMP_CACHE_BITS 12
>>> #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
>>> +typedef struct {
>>> + TranslationBlock *tb;
>>> +} CPUJumpCache;
>>> +
>> I don't quite follow whats going on here. I see we add vaddr pc in a
>> later patch but I don't quite see why a cache for looking up TBs gets a
>> sidechan value added later.
>> Is this because the vaddr will no longer match the tb->pc? Maybe a
>> comment on the structure is needed?
>
> Correct, there will be no tb->pc, so the cpu has to remember the virtual
> address itself.
>
> This patch only wraps the current pointer into a structure.
Sure - however we don't expand the comment when vaddr is added later.
I'm also concerned that now we have two fields there is scope for skew.
I guess the acquire/release semantics are to ensure we may fail safe but
never get a false positive?
>
>
> r~
>
>>
>>> /* work queue */
>>> /* The union type allows passing of 64 bit target pointers on
>>> 32 bit
>>> @@ -361,7 +365,7 @@ struct CPUState {
>>> IcountDecr *icount_decr_ptr;
>>> /* Accessed in parallel; all accesses must be atomic */
>>> - TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
>>> + CPUJumpCache tb_jmp_cache[TB_JMP_CACHE_SIZE];
>>> struct GDBRegisterState *gdb_regs;
>>> int gdb_num_regs;
>>> @@ -452,7 +456,7 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
>>> unsigned int i;
>>> for (i = 0; i < TB_JMP_CACHE_SIZE; i++) {
>>> - qatomic_set(&cpu->tb_jmp_cache[i], NULL);
>>> + qatomic_set(&cpu->tb_jmp_cache[i].tb, NULL);
>>> }
>>> }
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index dd58a144a8..c6283d5798 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -252,7 +252,7 @@ static inline TranslationBlock *tb_lookup(CPUState
>>> *cpu, target_ulong pc,
>>> tcg_debug_assert(!(cflags & CF_INVALID));
>>> hash = tb_jmp_cache_hash_func(pc);
>>> - tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash]);
>>> + tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash].tb);
>>> if (likely(tb &&
>>> tb->pc == pc &&
>>> @@ -266,7 +266,7 @@ static inline TranslationBlock *tb_lookup(CPUState
>>> *cpu, target_ulong pc,
>>> if (tb == NULL) {
>>> return NULL;
>>> }
>>> - qatomic_set(&cpu->tb_jmp_cache[hash], tb);
>>> + qatomic_set(&cpu->tb_jmp_cache[hash].tb, tb);
>>> return tb;
>>> }
>>> @@ -987,6 +987,8 @@ int cpu_exec(CPUState *cpu)
>>> tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>>> if (tb == NULL) {
>>> + uint32_t h;
>>> +
>>> mmap_lock();
>>> tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
>>> mmap_unlock();
>>> @@ -994,7 +996,8 @@ int cpu_exec(CPUState *cpu)
>>> * We add the TB in the virtual pc hash table
>>> * for the fast lookup
>>> */
>>> -
>>> qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
>>> + h = tb_jmp_cache_hash_func(pc);
>>> + qatomic_set(&cpu->tb_jmp_cache[h].tb, tb);
>>> }
>>> #ifndef CONFIG_USER_ONLY
>>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>>> index f5e6ca2da2..fb8f3087f1 100644
>>> --- a/accel/tcg/cputlb.c
>>> +++ b/accel/tcg/cputlb.c
>>> @@ -103,7 +103,7 @@ static void tb_jmp_cache_clear_page(CPUState *cpu,
>>> target_ulong page_addr)
>>> unsigned int i, i0 = tb_jmp_cache_hash_page(page_addr);
>>> for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
>>> - qatomic_set(&cpu->tb_jmp_cache[i0 + i], NULL);
>>> + qatomic_set(&cpu->tb_jmp_cache[i0 + i].tb, NULL);
>>> }
>>> }
>>> diff --git a/accel/tcg/translate-all.c
>>> b/accel/tcg/translate-all.c
>>> index f429d33981..efa479ccf3 100644
>>> --- a/accel/tcg/translate-all.c
>>> +++ b/accel/tcg/translate-all.c
>>> @@ -1187,8 +1187,8 @@ static void do_tb_phys_invalidate(TranslationBlock
>>> *tb, bool rm_from_page_list)
>>> /* remove the TB from the hash list */
>>> h = tb_jmp_cache_hash_func(tb->pc);
>>> CPU_FOREACH(cpu) {
>>> - if (qatomic_read(&cpu->tb_jmp_cache[h]) == tb) {
>>> - qatomic_set(&cpu->tb_jmp_cache[h], NULL);
>>> + if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
>>> + qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
>>> }
>>> }
>>
--
Alex Bennée
