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

Danielle Lam posted 1 patch 3 weeks, 2 days ago
Failed in applying to current master (apply log)
There is a newer version of this series
target/riscv/insn_trans/trans_rvzacas.c.inc | 13 ++++++++-----
target/riscv/translate.c                    |  5 +++--
2 files changed, 11 insertions(+), 7 deletions(-)
[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
From 048ca4fcaa826f0ba025b5da5087534939dc74bd Mon Sep 17 00:00:00 2001
From: Danielle Lam <danlam@g.hmc.edu>
Date: Fri, 3 Jul 2026 03:34:29 +0100
Subject: [PATCH] target/riscv: fix amocas with rd = x0 followed by beq
 involving x0 is not taken when it should be

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