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();
+
/*
* 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
--
2.34.1On 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
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~
© 2016 - 2026 Red Hat, Inc.