Re: [PATCH 3/3] accel/tcg: Fix jump cache set in cpu_exec_loop

[liweiwei]

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