diff mbox series

[v3] sim: riscv: Fix some issues with class-a instructions

Message ID AS8P193MB12855C4859FB6C421808222AE4162@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM
State New
Headers
Series [v3] sim: riscv: Fix some issues with class-a instructions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Bernd Edlinger April 26, 2024, 7:49 a.m. UTC 
  This fixes some issues with atomic instruction handling.  First the
instructions may have AQ and/or RL bits set, but the emulator has no
such concept, so we have to ignore those.

According to the spec the memory must be naturally aligned, otherwise
an exception shall be thrown, so do the sim_core read/write aligned.
In the case of riscv64 target, there were the LR_D and SC_D
64bit load and store instructions missing, so add those.
Additionally the LR_W instruction was missing the sign extension.

Also the AMOMIN/AMOMAX[U]_W instructions were not correct for riscv64
because the upper half word of the input registers were not ignored
as they should, so use explicit type-casts to uint32_t and int32_t
for those.

Furthermore the read value was written directly to the rd register,
but that can overwrite the value in rs1 or rs2.  In that case the
previous value must be still available and used instead.

And finally make the class-a instruction set only executable if a
riscv cpu model with A extension is selected.
---
 sim/riscv/sim-main.c | 84 ++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 27 deletions(-)

v2: use sim_core_read/write_unaligned, but override current_alignment,
to raise an exception when the address is not aligned.

v3: fix an issue with instructions where the output value overwrites
the input register when both are the same register.  Typically
seen with instructions like amoswap.w a0, a0, (a1)
diff mbox series

Patch

diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
index e4b15b533ba..f6f6e2384e8 100644
--- a/sim/riscv/sim-main.c
+++ b/sim/riscv/sim-main.c
@@ -841,6 +841,9 @@  execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 static sim_cia
 execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 {
+  unsigned_word mask_aq = OP_MASK_AQ << OP_SH_AQ;
+  unsigned_word mask_rl = OP_MASK_RL << OP_SH_RL;
+  unsigned_word mask_aqrl = mask_aq | mask_rl;
   struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
   SIM_DESC sd = CPU_STATE (cpu);
   struct riscv_sim_state *state = RISCV_SIM_STATE (sd);
@@ -851,17 +854,25 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
   const char *rs1_name = riscv_gpr_names_abi[rs1];
   const char *rs2_name = riscv_gpr_names_abi[rs2];
   struct atomic_mem_reserved_list *amo_prev, *amo_curr;
-  unsigned_word tmp;
+  unsigned_word tmp, res;
   sim_cia pc = riscv_cpu->pc + 4;
+  int prev_alignment = current_alignment;
+
+  if (current_alignment != FORCED_ALIGNMENT)
+    current_alignment = STRICT_ALIGNMENT;
 
   /* Handle these two load/store operations specifically.  */
-  switch (op->match)
+  switch (op->match & ~mask_aqrl)
     {
+    case MATCH_LR_D:
     case MATCH_LR_W:
       TRACE_INSN (cpu, "%s %s, (%s);", op->name, rd_name, rs1_name);
-      store_rd (cpu, rd,
-	sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
-				   riscv_cpu->regs[rs1]));
+      if (op->xlen_requirement == 64)
+	res = sim_core_read_unaligned_8 (cpu, riscv_cpu->pc, read_map,
+					 riscv_cpu->regs[rs1]);
+      else
+	res = EXTEND32 (sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
+						   riscv_cpu->regs[rs1]));
 
       /* Walk the reservation list to find an existing match.  */
       amo_curr = state->amo_reserved_list;
@@ -878,6 +889,7 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
       amo_curr->next = state->amo_reserved_list;
       state->amo_reserved_list = amo_curr;
       goto done;
+    case MATCH_SC_D:
     case MATCH_SC_W:
       TRACE_INSN (cpu, "%s %s, %s, (%s);", op->name, rd_name, rs2_name,
 		  rs1_name);
@@ -889,15 +901,20 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 	  if (amo_curr->addr == riscv_cpu->regs[rs1])
 	    {
 	      /* We found a reservation, so operate it.  */
-	      sim_core_write_unaligned_4 (cpu, riscv_cpu->pc, write_map,
-					  riscv_cpu->regs[rs1],
-					  riscv_cpu->regs[rs2]);
-	      store_rd (cpu, rd, 0);
+	      if (op->xlen_requirement == 64)
+		sim_core_write_unaligned_8 (cpu, riscv_cpu->pc, write_map,
+					    riscv_cpu->regs[rs1],
+					    riscv_cpu->regs[rs2]);
+	      else
+		sim_core_write_unaligned_4 (cpu, riscv_cpu->pc, write_map,
+					    riscv_cpu->regs[rs1],
+					    riscv_cpu->regs[rs2]);
 	      if (amo_curr == state->amo_reserved_list)
 		state->amo_reserved_list = amo_curr->next;
 	      else
 		amo_prev->next = amo_curr->next;
 	      free (amo_curr);
+	      res = 0;
 	      goto done;
 	    }
 	  amo_prev = amo_curr;
@@ -905,7 +922,7 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 	}
 
       /* If we're still here, then no reservation exists, so mark as failed.  */
-      store_rd (cpu, rd, 1);
+      res = 1;
       goto done;
     }
 
@@ -913,46 +930,49 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
   TRACE_INSN (cpu, "%s %s, %s, (%s);",
 	      op->name, rd_name, rs2_name, rs1_name);
   if (op->xlen_requirement == 64)
-    tmp = sim_core_read_unaligned_8 (cpu, riscv_cpu->pc, read_map,
+    res = sim_core_read_unaligned_8 (cpu, riscv_cpu->pc, read_map,
 				     riscv_cpu->regs[rs1]);
   else
-    tmp = EXTEND32 (sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
+    res = EXTEND32 (sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
 					       riscv_cpu->regs[rs1]));
-  store_rd (cpu, rd, tmp);
 
-  switch (op->match)
+  switch (op->match & ~mask_aqrl)
     {
     case MATCH_AMOADD_D:
     case MATCH_AMOADD_W:
-      tmp = riscv_cpu->regs[rd] + riscv_cpu->regs[rs2];
+      tmp = res + riscv_cpu->regs[rs2];
       break;
     case MATCH_AMOAND_D:
     case MATCH_AMOAND_W:
-      tmp = riscv_cpu->regs[rd] & riscv_cpu->regs[rs2];
+      tmp = res & riscv_cpu->regs[rs2];
       break;
     case MATCH_AMOMAX_D:
+      tmp = max ((signed_word) res, (signed_word) riscv_cpu->regs[rs2]);
+      break;
     case MATCH_AMOMAX_W:
-      tmp = max ((signed_word) riscv_cpu->regs[rd],
-		 (signed_word) riscv_cpu->regs[rs2]);
+      tmp = max ((int32_t) res, (int32_t) riscv_cpu->regs[rs2]);
       break;
     case MATCH_AMOMAXU_D:
+      tmp = max ((unsigned_word) res, (unsigned_word) riscv_cpu->regs[rs2]);
+      break;
     case MATCH_AMOMAXU_W:
-      tmp = max ((unsigned_word) riscv_cpu->regs[rd],
-		 (unsigned_word) riscv_cpu->regs[rs2]);
+      tmp = max ((uint32_t) res, (uint32_t) riscv_cpu->regs[rs2]);
       break;
     case MATCH_AMOMIN_D:
+      tmp = min ((signed_word) res, (signed_word) riscv_cpu->regs[rs2]);
+      break;
     case MATCH_AMOMIN_W:
-      tmp = min ((signed_word) riscv_cpu->regs[rd],
-		 (signed_word) riscv_cpu->regs[rs2]);
+      tmp = min ((int32_t) res, (int32_t) riscv_cpu->regs[rs2]);
       break;
     case MATCH_AMOMINU_D:
+      tmp = min ((unsigned_word) res, (unsigned_word) riscv_cpu->regs[rs2]);
+      break;
     case MATCH_AMOMINU_W:
-      tmp = min ((unsigned_word) riscv_cpu->regs[rd],
-		 (unsigned_word) riscv_cpu->regs[rs2]);
+      tmp = min ((uint32_t) res, (uint32_t) riscv_cpu->regs[rs2]);
       break;
     case MATCH_AMOOR_D:
     case MATCH_AMOOR_W:
-      tmp = riscv_cpu->regs[rd] | riscv_cpu->regs[rs2];
+      tmp = res | riscv_cpu->regs[rs2];
       break;
     case MATCH_AMOSWAP_D:
     case MATCH_AMOSWAP_W:
@@ -960,7 +980,7 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
       break;
     case MATCH_AMOXOR_D:
     case MATCH_AMOXOR_W:
-      tmp = riscv_cpu->regs[rd] ^ riscv_cpu->regs[rs2];
+      tmp = res ^ riscv_cpu->regs[rs2];
       break;
     default:
       TRACE_INSN (cpu, "UNHANDLED INSN: %s", op->name);
@@ -975,6 +995,8 @@  execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 				riscv_cpu->regs[rs1], tmp);
 
  done:
+  store_rd (cpu, rd, res);
+  current_alignment = prev_alignment;
   return pc;
 }
 
@@ -1307,7 +1329,15 @@  execute_one (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
   switch (op->insn_class)
     {
     case INSN_CLASS_A:
-      return execute_a (cpu, iw, op);
+      /* Check whether model with A extension is selected.  */
+      if (riscv_cpu->csr.misa & 1)
+	return execute_a (cpu, iw, op);
+      else
+	{
+	  TRACE_INSN (cpu, "UNHANDLED EXTENSION: %d", op->insn_class);
+	  sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_signalled,
+			   SIM_SIGILL);
+	}
     case INSN_CLASS_C:
       /* Check whether model with C extension is selected.  */
       if (riscv_cpu->csr.misa & 4)