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

[Ilya Leoshkevich]

On Tue, 2022-08-16 at 20:42 -0500, Richard Henderson wrote:
> 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~
> 

Would something like this be okay?

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 27435b97dbd..9421c84d991 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1152,6 +1152,27 @@ static inline void
tb_jmp_unlink(TranslationBlock *dest)
     qemu_spin_unlock(&dest->jmp_lock);
 }
 
+static void cpu_tb_jmp_cache_remove(TranslationBlock *tb)
+{
+    CPUState *cpu;
+    uint32_t h;
+
+    /* remove the TB from the hash list */
+    if (TARGET_TB_PCREL) {
+        /* Any TB may be at any virtual address */
+        CPU_FOREACH(cpu) {
+            cpu_tb_jmp_cache_clear(cpu);
+        }
+    } else {
+        h = tb_jmp_cache_hash_func(tb_pc(tb));
+        CPU_FOREACH(cpu) {
+            if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
+                qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
+            }
+        }
+    }
+}
+
 /*
  * In user-mode, call with mmap_lock held.
  * In !user-mode, if @rm_from_page_list is set, call with the TB's
pages'
@@ -1159,7 +1180,6 @@ static inline void tb_jmp_unlink(TranslationBlock
*dest)
  */
 static void do_tb_phys_invalidate(TranslationBlock *tb, bool
rm_from_page_list)
 {
-    CPUState *cpu;
     PageDesc *p;
     uint32_t h;
     tb_page_addr_t phys_pc;
@@ -1190,20 +1210,7 @@ static void
do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
         }
     }
 
-    /* remove the TB from the hash list */
-    if (TARGET_TB_PCREL) {
-        /* Any TB may be at any virtual address */
-        CPU_FOREACH(cpu) {
-            cpu_tb_jmp_cache_clear(cpu);
-        }
-    } else {
-        h = tb_jmp_cache_hash_func(tb_pc(tb));
-        CPU_FOREACH(cpu) {
-            if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
-                qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
-            }
-        }
-    }
+    cpu_tb_jmp_cache_remove(tb);
 
     /* suppress this TB from the two jump lists */
     tb_remove_from_jmp_list(tb, 0);
@@ -2243,6 +2250,13 @@ void page_set_flags(target_ulong start,
target_ulong end, int flags)
             (flags & PAGE_WRITE) &&
             p->first_tb) {
             tb_invalidate_phys_page(addr, 0);
+        } else {
+            TranslationBlock *tb;
+            int n;
+
+            PAGE_FOR_EACH_TB(p, tb, n) {
+                cpu_tb_jmp_cache_remove(tb);
+            }
         }
         if (reset_target_data) {
             g_free(p->target_data);