[3/3] target/riscv: fix the trap generation for conditional store

Commit Message

Frederic Konrad Dec. 11, 2024, 9:19 p.m. UTC

From Unpriviledged ISA manual:

"For LR and SC, the Zalrsc extension requires that the address held in rs1 be
naturally aligned to the size of the operand (i.e., eight-byte aligned for
doublewords and four-byte aligned for words). If the address is not naturally
aligned, an address-misaligned exception or an access-fault exception will be
generated. The access-fault exception can be generated for a memory access that
would otherwise be able to complete except for the misalignment, if the
misaligned access should not be emulated."

Here nothing checks that the address is naturally aligned, so this fixes that
wrong behavior by raising address-misaligned exception if the address in rs1
is not naturally aligned.

Signed-off-by: Frederic Konrad <[e-mail]>
---
 target/riscv/insn_trans/trans_rva.c.inc | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Richard Henderson Dec. 11, 2024, 9:43 p.m. UTC | #1

On 12/11/24 15:19, Frederic Konrad wrote:
> +    /*
> +     * A misaligned store trap should be triggered even if the store should
> +     * fail due to the reservation.
> +     */
> +    tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));

The constant is incorrect for testing the low bits.

> +    tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);

Best to make the fallthrough path be the common case, as we will optimize across the 
extended basic block.

Use test-style comparison:

     tcg_gen_brcondi_tl(TCG_COND_TSTNE, src1, memop_size(mop) - 1, l_misalign);



r~

Patch

diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index 9cf3ae8019..30a047164c 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -58,11 +58,30 @@  static bool gen_lr(DisasContext *ctx, arg_atomic *a, MemOp mop)
 static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
 {
     TCGv dest, src1, src2;
+    TCGv tmp = tcg_temp_new();
+    TCGLabel *l3 = gen_new_label();
     TCGLabel *l1 = gen_new_label();
     TCGLabel *l2 = gen_new_label();
 
     decode_save_opc(ctx, 0);
     src1 = get_address(ctx, a->rs1, 0);
+    /*
+     * A misaligned store trap should be triggered even if the store should
+     * fail due to the reservation.
+     */
+    tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));
+    tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);
+
+    /*
+     * Store the faulty address, and the actual PC.  Then generate the
+     * exception.
+     */
+    tcg_gen_st_tl(src1, tcg_env, offsetof(CPURISCVState, badaddr));
+    gen_pc_plus_diff(cpu_pc, ctx, 0);
+    gen_helper_raise_exception(tcg_env,
+                               tcg_constant_i32(RISCV_EXCP_STORE_AMO_ADDR_MIS));
+
+    gen_set_label(l3);
     tcg_gen_brcond_tl(TCG_COND_NE, load_res, src1, l1);
 
     /*