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.hender...@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);
