[v4] target/riscv: Fix pointer mask related support

[PATCH v4 6/8] accel/tcg: Fix tb mis-matched problem when CF_PCREL is enabled

Posted by Weiwei Li 2 years, 10 months ago

A corner case is triggered  when tb block with first_pc = 0x80000008
and first_pc = 0x800000200 has the same jump cache hash, and share
the same tb entry with the same tb information except PC.
The executed sequence is as follows:
tb(0x80000008) -> tb(0x80000008)-> tb(0x800000200) -> tb(0x80000008)

1. At the first time tb for 0x80000008 is loaded, tb in jmp_cache is
filled, however pc is not updated.
2. At the second time tb for 0x80000008 is looked up in tb_lookup(),
pc in jmp cache is set to 0x80000008.
3. when tb for 0x800000200 is loaded, tb for jmp cache is updated to
this block, however pc is not updated, and remains to be 0x80000008.
4. Finally at the last time tb for 0x80000008 is looked up, tb for
0x800000200 is mismatched.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 accel/tcg/cpu-exec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index c815f2dbfd..faff413f42 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -983,6 +983,9 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
                 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);
+                if (cflags & CF_PCREL) {
+                    qatomic_set(&cpu->tb_jmp_cache->array[h].pc, pc);
+                }
             }
 
 #ifndef CONFIG_USER_ONLY
-- 
2.25.1

Re: [PATCH v4 6/8] accel/tcg: Fix tb mis-matched problem when CF_PCREL is enabled

Posted by Richard Henderson 2 years, 10 months ago

On 3/31/23 08:06, Weiwei Li wrote:
> A corner case is triggered  when tb block with first_pc = 0x80000008
> and first_pc = 0x800000200 has the same jump cache hash, and share
> the same tb entry with the same tb information except PC.
> The executed sequence is as follows:
> tb(0x80000008) -> tb(0x80000008)-> tb(0x800000200) -> tb(0x80000008)
> 
> 1. At the first time tb for 0x80000008 is loaded, tb in jmp_cache is
> filled, however pc is not updated.
> 2. At the second time tb for 0x80000008 is looked up in tb_lookup(),
> pc in jmp cache is set to 0x80000008.
> 3. when tb for 0x800000200 is loaded, tb for jmp cache is updated to
> this block, however pc is not updated, and remains to be 0x80000008.
> 4. Finally at the last time tb for 0x80000008 is looked up, tb for
> 0x800000200 is mismatched.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   accel/tcg/cpu-exec.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index c815f2dbfd..faff413f42 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -983,6 +983,9 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
>                   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);
> +                if (cflags & CF_PCREL) {
> +                    qatomic_set(&cpu->tb_jmp_cache->array[h].pc, pc);
> +                }

Good catch on the bug, but incorrect fix.  Need

if (cflags & CF_PCREL) {
     qatomic_set(&cpu->tb_jmp_cache->array[h].pc, pc);
     qatomic_store_release(&cpu->tb_jmp_cache->array[h].tb, tb);
} else {
     /* Use the pc value already stored in tb->pc. */
     qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
}


r~

Re: [PATCH v4 6/8] accel/tcg: Fix tb mis-matched problem when CF_PCREL is enabled

Posted by Richard Henderson 2 years, 10 months ago

On 3/31/23 18:26, Richard Henderson wrote:
> On 3/31/23 08:06, Weiwei Li wrote:
>> A corner case is triggered  when tb block with first_pc = 0x80000008
>> and first_pc = 0x800000200 has the same jump cache hash, and share
>> the same tb entry with the same tb information except PC.
>> The executed sequence is as follows:
>> tb(0x80000008) -> tb(0x80000008)-> tb(0x800000200) -> tb(0x80000008)
>>
>> 1. At the first time tb for 0x80000008 is loaded, tb in jmp_cache is
>> filled, however pc is not updated.
>> 2. At the second time tb for 0x80000008 is looked up in tb_lookup(),
>> pc in jmp cache is set to 0x80000008.
>> 3. when tb for 0x800000200 is loaded, tb for jmp cache is updated to
>> this block, however pc is not updated, and remains to be 0x80000008.
>> 4. Finally at the last time tb for 0x80000008 is looked up, tb for
>> 0x800000200 is mismatched.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   accel/tcg/cpu-exec.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index c815f2dbfd..faff413f42 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -983,6 +983,9 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
>>                   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);
>> +                if (cflags & CF_PCREL) {
>> +                    qatomic_set(&cpu->tb_jmp_cache->array[h].pc, pc);
>> +                }
> 
> Good catch on the bug, but incorrect fix.  Need
> 
> if (cflags & CF_PCREL) {
>      qatomic_set(&cpu->tb_jmp_cache->array[h].pc, pc);
>      qatomic_store_release(&cpu->tb_jmp_cache->array[h].tb, tb);
> } else {
>      /* Use the pc value already stored in tb->pc. */
>      qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
> }

Queuing the fix to tcg-next.

r~

[PATCH v4 1/8] target/riscv: Fix pointer mask transformation for vector address
[PATCH v4 2/8] target/riscv: Update cur_pmmask/base when xl changes
[PATCH v4 3/8] target/riscv: Fix target address to update badaddr
[PATCH v4 4/8] target/riscv: Add support for PC-relative translation
[PATCH v4 5/8] accel/tcg: Fix overwrite problems of tcg_cflags
[PATCH v4 6/8] accel/tcg: Fix tb mis-matched problem when CF_PCREL is enabled
[PATCH v4 7/8] target/riscv: Enable PC-relative translation in system mode
[PATCH v4 8/8] target/riscv: Add pointer mask support for instruction fetch