Re: [PATCH v5 14/17] include/hw/core: Create struct CPUJumpCache

[Richard Henderson <richard.henderson@linaro.org>]

On 9/29/22 06:46, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Wrap the bare TranslationBlock pointer into a structure.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Signed-off-by: Richard Henderson <richard.henderson@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.


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);
>>           }
>>       }
> 
>