Re: [PATCH for-7.2 14/21] accel/tcg: Hoist get_page_addr_code out of tb_lookup

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

On 8/16/22 18:43, Ilya Leoshkevich wrote:
> On Fri, 2022-08-12 at 11:07 -0700, Richard Henderson wrote:
>> We will want to re-use the result of get_page_addr_code
>> beyond the scope of tb_lookup.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++----------
>>   1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index a9b7053274..889355b341 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -209,13 +209,12 @@ static bool tb_lookup_cmp(const void *p, const
>> void *d)
>>   }
>>   
>>   /* Might cause an exception, so have a longjmp destination ready */
>> -static TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>> -                                   target_ulong cs_base,
>> +static TranslationBlock *tb_lookup(CPUState *cpu, tb_page_addr_t
>> phys_pc,
>> +                                   target_ulong pc, target_ulong
>> cs_base,
>>                                      uint32_t flags, uint32_t cflags)
>>   {
>>       CPUArchState *env = cpu->env_ptr;
>>       TranslationBlock *tb;
>> -    tb_page_addr_t phys_pc;
>>       struct tb_desc desc;
>>       uint32_t jmp_hash, tb_hash;
>>   
>> @@ -240,11 +239,8 @@ static TranslationBlock *tb_lookup(CPUState
>> *cpu, target_ulong pc,
>>       desc.cflags = cflags;
>>       desc.trace_vcpu_dstate = *cpu->trace_dstate;
>>       desc.pc = pc;
>> -    phys_pc = get_page_addr_code(desc.env, pc);
>> -    if (phys_pc == -1) {
>> -        return NULL;
>> -    }
>>       desc.phys_page1 = phys_pc & TARGET_PAGE_MASK;
>> +
>>       tb_hash = tb_hash_func(phys_pc, pc, flags, cflags, *cpu-
>>> trace_dstate);
>>       tb = qht_lookup_custom(&tb_ctx.htable, &desc, tb_hash,
>> tb_lookup_cmp);
>>       if (tb == NULL) {
>> @@ -371,6 +367,7 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState
>> *env)
>>       TranslationBlock *tb;
>>       target_ulong cs_base, pc;
>>       uint32_t flags, cflags;
>> +    tb_page_addr_t phys_pc;
>>   
>>       cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
>>   
>> @@ -379,7 +376,12 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState
>> *env)
>>           cpu_loop_exit(cpu);
>>       }
>>   
>> -    tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>> +    phys_pc = get_page_addr_code(env, pc);
>> +    if (phys_pc == -1) {
>> +        return tcg_code_gen_epilogue;
>> +    }
>> +
>> +    tb = tb_lookup(cpu, phys_pc, pc, cs_base, flags, cflags);
>>       if (tb == NULL) {
>>           return tcg_code_gen_epilogue;
>>       }
>> @@ -482,6 +484,7 @@ void cpu_exec_step_atomic(CPUState *cpu)
>>       TranslationBlock *tb;
>>       target_ulong cs_base, pc;
>>       uint32_t flags, cflags;
>> +    tb_page_addr_t phys_pc;
>>       int tb_exit;
>>   
>>       if (sigsetjmp(cpu->jmp_env, 0) == 0) {
>> @@ -504,7 +507,12 @@ void cpu_exec_step_atomic(CPUState *cpu)
>>            * Any breakpoint for this insn will have been recognized
>> earlier.
>>            */
>>   
>> -        tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>> +        phys_pc = get_page_addr_code(env, pc);
>> +        if (phys_pc == -1) {
>> +            tb = NULL;
>> +        } else {
>> +            tb = tb_lookup(cpu, phys_pc, pc, cs_base, flags,
>> cflags);
>> +        }
>>           if (tb == NULL) {
>>               mmap_lock();
>>               tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
>> @@ -949,6 +957,7 @@ int cpu_exec(CPUState *cpu)
>>               TranslationBlock *tb;
>>               target_ulong cs_base, pc;
>>               uint32_t flags, cflags;
>> +            tb_page_addr_t phys_pc;
>>   
>>               cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base,
>> &flags);
>>   
>> @@ -970,7 +979,12 @@ int cpu_exec(CPUState *cpu)
>>                   break;
>>               }
>>   
>> -            tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>> +            phys_pc = get_page_addr_code(cpu->env_ptr, pc);
>> +            if (phys_pc == -1) {
>> +                tb = NULL;
>> +            } else {
>> +                tb = tb_lookup(cpu, phys_pc, pc, cs_base, flags,
>> cflags);
>> +            }
>>               if (tb == NULL) {
>>                   mmap_lock();
>>                   tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
> 
> This patch did not make it into v2, but having get_page_addr_code()
> before tb_lookup() in helper_lookup_tb_ptr() helped raise the exception
> when trying to execute a no-longer-executable TB.
> 
> Was it dropped for performance reasons?

Ah, yes.  I dropped it because I ran into some regression, and started minimizing the 
tree.  Because of the extra lock that needed to be held (next patch, also dropped), I 
couldn't prove this actually helped.

I think the bit that's causing your user-only failure at the moment is the jump cache. 
This patch hoisted the page table check before the jmp_cache.  For system, cputlb.c takes 
care of flushing the jump cache with page table changes; we still don't have anything in 
user-only that takes care of that.


r~