| Message ID | 20230401045106.3885562-4-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | last minute tcg fixes | expand |
On 2023/4/1 12:51, Richard Henderson wrote: > Assign pc and use store_release to assign tb. > > Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller") > Reported-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/cpu-exec.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index c815f2dbfd..8370c92c05 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -257,7 +257,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc, > > if (cflags & CF_PCREL) { > /* Use acquire to ensure current load of pc from jc. */ > - tb = qatomic_load_acquire(&jc->array[hash].tb); > + tb = qatomic_load_acquire(&jc->array[hash].tb); > > if (likely(tb && > jc->array[hash].pc == pc && > @@ -272,7 +272,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc, > return NULL; > } > jc->array[hash].pc = pc; > - /* Use store_release on tb to ensure pc is written first. */ > + /* Ensure pc is written first. */ > qatomic_store_release(&jc->array[hash].tb, tb); > } else { > /* Use rcu_read to ensure current load of pc from *tb. */ > @@ -971,18 +971,27 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc) > > tb = tb_lookup(cpu, pc, cs_base, flags, cflags); > if (tb == NULL) { > + CPUJumpCache *jc; > uint32_t h; > > mmap_lock(); > tb = tb_gen_code(cpu, pc, cs_base, flags, cflags); > mmap_unlock(); > + Blank line. > /* > * We add the TB in the virtual pc hash table > * for the fast lookup > */ > h = tb_jmp_cache_hash_func(pc); > - /* Use the pc value already stored in tb->pc. */ > - qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb); > + jc = cpu->tb_jmp_cache; > + if (cflags & CF_PCREL) { > + jc->array[h].pc = pc; > + /* Ensure pc is written first. */ > + qatomic_store_release(&jc->array[h].tb, tb); Whether we should add a qatomic_load_require() before this? Regards, Weiwei Li > + } else { > + /* Use the pc value already stored in tb->pc. */ > + qatomic_set(&jc->array[h].tb, tb); > + } > } > > #ifndef CONFIG_USER_ONLY
On 4/1/23 04:03, liweiwei wrote: >> mmap_unlock(); >> + > Blank line. Yes, adding separation. >> /* >> * We add the TB in the virtual pc hash table >> * for the fast lookup >> */ >> h = tb_jmp_cache_hash_func(pc); >> - /* Use the pc value already stored in tb->pc. */ >> - qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb); >> + jc = cpu->tb_jmp_cache; >> + if (cflags & CF_PCREL) { >> + jc->array[h].pc = pc; >> + /* Ensure pc is written first. */ >> + qatomic_store_release(&jc->array[h].tb, tb); > > Whether we should add a qatomic_load_require() before this? The load_acquire is already present in tb_lookup. r~
On Sat, 1 Apr 2023 at 05:52, Richard Henderson <richard.henderson@linaro.org> wrote: > > Assign pc and use store_release to assign tb. > > Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller") > Reported-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/cpu-exec.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) I don't fully understand the code here, but before 2dd5b7a1b91 this was a callsite for tb_jmp_cache_set(), 2dd5b7a1b91 expanded out the call but forgot the CF_PCREL half of the if, and this patch restores it. Which all seems logical, so Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c815f2dbfd..8370c92c05 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -257,7 +257,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc, if (cflags & CF_PCREL) { /* Use acquire to ensure current load of pc from jc. */ - tb = qatomic_load_acquire(&jc->array[hash].tb); + tb = qatomic_load_acquire(&jc->array[hash].tb); if (likely(tb && jc->array[hash].pc == pc && @@ -272,7 +272,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc, return NULL; } jc->array[hash].pc = pc; - /* Use store_release on tb to ensure pc is written first. */ + /* Ensure pc is written first. */ qatomic_store_release(&jc->array[hash].tb, tb); } else { /* Use rcu_read to ensure current load of pc from *tb. */ @@ -971,18 +971,27 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc) tb = tb_lookup(cpu, pc, cs_base, flags, cflags); if (tb == NULL) { + CPUJumpCache *jc; uint32_t h; mmap_lock(); tb = tb_gen_code(cpu, pc, cs_base, flags, cflags); mmap_unlock(); + /* * We add the TB in the virtual pc hash table * for the fast lookup */ h = tb_jmp_cache_hash_func(pc); - /* Use the pc value already stored in tb->pc. */ - qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb); + jc = cpu->tb_jmp_cache; + if (cflags & CF_PCREL) { + jc->array[h].pc = pc; + /* Ensure pc is written first. */ + qatomic_store_release(&jc->array[h].tb, tb); + } else { + /* Use the pc value already stored in tb->pc. */ + qatomic_set(&jc->array[h].tb, tb); + } } #ifndef CONFIG_USER_ONLY
Assign pc and use store_release to assign tb. Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller") Reported-by: Weiwei Li <liweiwei@iscas.ac.cn> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/cpu-exec.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)