[v1] target/riscv: fix amocas with rd = x0 followed by beq involving x0 is not taken when it should be

[PATCH] target/riscv: fix amocas with rd = x0 followed by beq involving x0 is not taken when it should be

Posted by Danielle Lam 3 weeks, 2 days ago

amocas.{w,d,q,b,h} implement compare and swap by reusing rd current value as both comparator input and as the output that tcg_gen_atomic_cmpxchg_* writes the old memory into. Before the fix, ret == cmpv and we name that variable dest. When rd = x0, dest was ctx->zero. Then since ret == cmpv in the first place, the atomic op overwrites ret with ctx->zero which is not the same zero value. Any later instructions that refer back to ret for the initial zero value which is now changed, resulting in 0xbaaaaaad00000000.

The fix is to use different variables for ret and cmpv and assign that to atomic op to run.

**TESTING**
gen_cmpxchg
.d was tested and it passed. .w/.b/.h use the same logic so the fix was similar.

gen_cmpxchg64
.d was not tested

trans_amocas_q
.q had its own custom test: test_amocasq.S

.section .text
.globl _start
_start:
    la      t0, scratch
    li      t1, 0x1111
    li      t2, 0x2222
    sd      t1, 0(t0)
    sd      t2, 8(t0)

    li      a0, 0x3333
    li      a1, 0x4444
    amocas.q x0, a0, (t0)

    addi    a2, x0, 0

spin:
    j       spin

.section .bss
.align 4
scratch:
    .space 16

Then searching the trace log:
grep -n "x12/a2" amocasq-test.trace.log

I found that all matches read zero.
10612696: x12/a2   0 x13/a3   0 x14/a4   0 x15/a5   0
...
10613892: x12/a2   0 x13/a3   0 x14/a4   0 x15/a5   0

Run against the unpatched code, a2 read back as 0x2222. This confirms the test run as intended and the fix works.

Signed-off-by: Danielle Lam <danlam@g.hmc.edu>
---
 target/riscv/insn_trans/trans_rvzacas.c.inc | 13 ++++++++-----
 target/riscv/translate.c                    |  5 +++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvzacas.c.inc b/target/riscv/insn_trans/trans_rvzacas.c.inc
index 79bca1e957..867f5e5ade 100644
--- a/target/riscv/insn_trans/trans_rvzacas.c.inc
+++ b/target/riscv/insn_trans/trans_rvzacas.c.inc
@@ -72,13 +72,14 @@ static bool gen_cmpxchg64(DisasContext *ctx, arg_atomic *a, MemOp mop)
         return false;
     }
 
-    TCGv_i64 dest = get_gpr_pair(ctx, a->rd);
     TCGv src1 = get_address(ctx, a->rs1, 0);
     TCGv_i64 src2 = get_gpr_pair(ctx, a->rs2);
+    TCGv_i64 cmpv = get_gpr_pair(ctx, a->rd);
+    TCGv_i64 dest = tcg_temp_new_i64();
 
     mop |= ctx->mo_endianness;
     decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
-    tcg_gen_atomic_cmpxchg_i64(dest, src1, dest, src2, ctx->mem_idx, mop);
+    tcg_gen_atomic_cmpxchg_i64(dest, src1, cmpv, src2, ctx->mem_idx, mop);
 
     gen_set_gpr_pair(ctx, a->rd, dest);
     return true;
@@ -117,13 +118,15 @@ static bool trans_amocas_q(DisasContext *ctx, arg_amocas_q *a)
     TCGv_i128 src2 = tcg_temp_new_i128();
     TCGv_i64 src2l = get_gpr(ctx, a->rs2, EXT_NONE);
     TCGv_i64 src2h = get_gpr(ctx, a->rs2 == 0 ? 0 : a->rs2 + 1, EXT_NONE);
-    TCGv_i64 destl = get_gpr(ctx, a->rd, EXT_NONE);
-    TCGv_i64 desth = get_gpr(ctx, a->rd == 0 ? 0 : a->rd + 1, EXT_NONE);
+    TCGv_i64 cmpl = get_gpr(ctx, a->rd, EXT_NONE);
+    TCGv_i64 cmph = get_gpr(ctx, a->rd == 0 ? 0 : a->rd + 1, EXT_NONE);
+    TCGv_i64 destl = tcg_temp_new_i64();
+    TCGv_i64 desth = tcg_temp_new_i64();
     MemOp memop = MO_ALIGN | MO_UO;
 
     memop |= ctx->mo_endianness;
     tcg_gen_concat_i64_i128(src2, src2l, src2h);
-    tcg_gen_concat_i64_i128(dest, destl, desth);
+    tcg_gen_concat_i64_i128(dest, cmpl, cmph);
     decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
     tcg_gen_atomic_cmpxchg_i128(dest, src1, dest, src2, ctx->mem_idx, memop);
 
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 9684dbe752..62a9af9b56 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1166,13 +1166,14 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
 
 static bool gen_cmpxchg(DisasContext *ctx, arg_atomic *a, MemOp mop)
 {
-    TCGv dest = get_gpr(ctx, a->rd, EXT_NONE);
     TCGv src1 = get_address(ctx, a->rs1, 0);
     TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
+    TCGv cmpv = get_gpr(ctx, a->rd, EXT_NONE);
+    TCGv dest = dest_gpr(ctx, a->rd);
 
     mop |= ctx->mo_endianness;
     decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
-    tcg_gen_atomic_cmpxchg_tl(dest, src1, dest, src2, ctx->mem_idx, mop);
+    tcg_gen_atomic_cmpxchg_tl(dest, src1, cmpv, src2, ctx->mem_idx, mop);
 
     gen_set_gpr(ctx, a->rd, dest);
     return true;
-- 
2.47.3

Re: [PATCH] target/riscv: fix amocas with rd = x0 followed by beq involving x0 is not taken when it should be

Posted by Alistair Francis 1 week, 6 days ago

On Fri, 2026-07-03 at 13:51 +0700, Danielle Lam wrote:
> amocas.{w,d,q,b,h} implement compare and swap by reusing rd current
> value as both comparator input and as the output that
> tcg_gen_atomic_cmpxchg_* writes the old memory into. Before the fix,
> ret == cmpv and we name that variable dest. When rd = x0, dest was
> ctx->zero. Then since ret == cmpv in the first place, the atomic op
> overwrites ret with ctx->zero which is not the same zero value. Any
> later instructions that refer back to ret for the initial zero value
> which is now changed, resulting in 0xbaaaaaad00000000.

I'm having some trouble following what the issue is. Do you mind
clearing up this commit title and message to be really clear of what
the issue is and what you are fixing?

Also, the lines should wrap at around 70 charecters

> 
> The fix is to use different variables for ret and cmpv and assign
> that to atomic op to run.
> 
> **TESTING**
> gen_cmpxchg
> .d was tested and it passed. .w/.b/.h use the same logic so the fix
> was similar.
> 
> gen_cmpxchg64
> .d was not tested
> 
> trans_amocas_q
> .q had its own custom test: test_amocasq.S
> 
> .section .text
> .globl _start
> _start:
>     la      t0, scratch
>     li      t1, 0x1111
>     li      t2, 0x2222
>     sd      t1, 0(t0)
>     sd      t2, 8(t0)
> 
>     li      a0, 0x3333
>     li      a1, 0x4444
>     amocas.q x0, a0, (t0)
> 
>     addi    a2, x0, 0
> 
> spin:
>     j       spin
> 
> .section .bss
> .align 4
> scratch:
>     .space 16
> 
> Then searching the trace log:
> grep -n "x12/a2" amocasq-test.trace.log
> 
> I found that all matches read zero.
> 10612696: x12/a2   0 x13/a3   0 x14/a4   0 x15/a5   0
> ...
> 10613892: x12/a2   0 x13/a3   0 x14/a4   0 x15/a5   0
> 
> Run against the unpatched code, a2 read back as 0x2222. This confirms
> the test run as intended and the fix works.

Could you add a test case to tests/tcg/riscv64/ instead? This is hard
to verify that it fixes something

Alistair

> 
> Signed-off-by: Danielle Lam <danlam@g.hmc.edu>
> ---
>  target/riscv/insn_trans/trans_rvzacas.c.inc | 13 ++++++++-----
>  target/riscv/translate.c                    |  5 +++--
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvzacas.c.inc
> b/target/riscv/insn_trans/trans_rvzacas.c.inc
> index 79bca1e957..867f5e5ade 100644
> --- a/target/riscv/insn_trans/trans_rvzacas.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzacas.c.inc
> @@ -72,13 +72,14 @@ static bool gen_cmpxchg64(DisasContext *ctx,
> arg_atomic *a, MemOp mop)
>          return false;
>      }
>  
> -    TCGv_i64 dest = get_gpr_pair(ctx, a->rd);
>      TCGv src1 = get_address(ctx, a->rs1, 0);
>      TCGv_i64 src2 = get_gpr_pair(ctx, a->rs2);
> +    TCGv_i64 cmpv = get_gpr_pair(ctx, a->rd);
> +    TCGv_i64 dest = tcg_temp_new_i64();
>  
>      mop |= ctx->mo_endianness;
>      decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> -    tcg_gen_atomic_cmpxchg_i64(dest, src1, dest, src2, ctx->mem_idx,
> mop);
> +    tcg_gen_atomic_cmpxchg_i64(dest, src1, cmpv, src2, ctx->mem_idx,
> mop);
>  
>      gen_set_gpr_pair(ctx, a->rd, dest);
>      return true;
> @@ -117,13 +118,15 @@ static bool trans_amocas_q(DisasContext *ctx,
> arg_amocas_q *a)
>      TCGv_i128 src2 = tcg_temp_new_i128();
>      TCGv_i64 src2l = get_gpr(ctx, a->rs2, EXT_NONE);
>      TCGv_i64 src2h = get_gpr(ctx, a->rs2 == 0 ? 0 : a->rs2 + 1,
> EXT_NONE);
> -    TCGv_i64 destl = get_gpr(ctx, a->rd, EXT_NONE);
> -    TCGv_i64 desth = get_gpr(ctx, a->rd == 0 ? 0 : a->rd + 1,
> EXT_NONE);
> +    TCGv_i64 cmpl = get_gpr(ctx, a->rd, EXT_NONE);
> +    TCGv_i64 cmph = get_gpr(ctx, a->rd == 0 ? 0 : a->rd + 1,
> EXT_NONE);
> +    TCGv_i64 destl = tcg_temp_new_i64();
> +    TCGv_i64 desth = tcg_temp_new_i64();
>      MemOp memop = MO_ALIGN | MO_UO;
>  
>      memop |= ctx->mo_endianness;
>      tcg_gen_concat_i64_i128(src2, src2l, src2h);
> -    tcg_gen_concat_i64_i128(dest, destl, desth);
> +    tcg_gen_concat_i64_i128(dest, cmpl, cmph);
>      decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
>      tcg_gen_atomic_cmpxchg_i128(dest, src1, dest, src2, ctx-
> >mem_idx, memop);
>  
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 9684dbe752..62a9af9b56 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1166,13 +1166,14 @@ static bool gen_amo(DisasContext *ctx,
> arg_atomic *a,
>  
>  static bool gen_cmpxchg(DisasContext *ctx, arg_atomic *a, MemOp mop)
>  {
> -    TCGv dest = get_gpr(ctx, a->rd, EXT_NONE);
>      TCGv src1 = get_address(ctx, a->rs1, 0);
>      TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +    TCGv cmpv = get_gpr(ctx, a->rd, EXT_NONE);
> +    TCGv dest = dest_gpr(ctx, a->rd);
>  
>      mop |= ctx->mo_endianness;
>      decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> -    tcg_gen_atomic_cmpxchg_tl(dest, src1, dest, src2, ctx->mem_idx,
> mop);
> +    tcg_gen_atomic_cmpxchg_tl(dest, src1, cmpv, src2, ctx->mem_idx,
> mop);
>  
>      gen_set_gpr(ctx, a->rd, dest);
>      return true;