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 <[email protected]> > --- > 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;
