target/riscv:Perform single step before resume if necessary

Two cases where single step is needed before resume:
1. ebreak used in software breakpoint;
2. a trigger that is taken just before the instruction
   that triggered it is retired.

Signed-off-by: Songhe Zhu <[email protected]> Co-developed-by: Fei Gao <[email protected]> Co-developed-by: xiatianyi <[email protected]>

Author: zhusonghe

src/target/riscv/riscv.c

@@ -623,12 +623,12 @@ static int find_first_trigger_by_id(struct target *target, int unique_id)
 
 static unsigned int count_trailing_ones(riscv_reg_t reg)
 {
-	assert(sizeof(riscv_reg_t) * 8 == 64);
-	for (unsigned int i = 0; i < 64; i++) {
+	const unsigned int riscv_reg_bits = sizeof(riscv_reg_t) * CHAR_BIT;
+	for (unsigned int i = 0; i < riscv_reg_bits; i++) {
 		if ((1 & (reg >> i)) == 0)
 			return i;
 	}
-	return 64;
+	return riscv_reg_bits;
 }
 
 static int set_trigger(struct target *target, unsigned int idx, riscv_reg_t tdata1, riscv_reg_t tdata2)
@@ -1561,13 +1561,69 @@ int riscv_remove_watchpoint(struct target *target,
 	return ERROR_OK;
 }
 
+typedef enum {
+	M6_HIT_ERROR,
+	M6_HIT_NOT_SUPPORTED,
+	M6_NOT_HIT,
+	M6_HIT_BEFORE,
+	M6_HIT_AFTER,
+	M6_HIT_IMM_AFTER
+} mctrl6hitstatus;
+
+/* TODO Record info:mcontrol6_supports_hit, This way
+ * the number of reads and writes of TDATA1 will be
+ * greatly reduced.
+ */
+static mctrl6hitstatus check_mcontrol6_hit_status(struct target *target,
+		riscv_reg_t tdata1, uint64_t hit_mask)
+{
+	const uint32_t hit0 = get_field(tdata1, CSR_MCONTROL6_HIT0);
+	const uint32_t hit1 = get_field(tdata1, CSR_MCONTROL6_HIT1);
+	const uint32_t hit_info = (hit1 << 1) | hit0;
+	if (hit_info == CSR_MCONTROL6_HIT0_BEFORE)
+		return M6_HIT_BEFORE;
+
+	if (hit_info == CSR_MCONTROL6_HIT0_AFTER)
+		return M6_HIT_AFTER;
+
+	if (hit_info == CSR_MCONTROL6_HIT0_IMMEDIATELY_AFTER)
+		return M6_HIT_IMM_AFTER;
+
+	if (hit_info == CSR_MCONTROL6_HIT0_FALSE) {
+		/* hit[1..0] equals 0, which can mean one of the following:
+		 * - "hit" bits are supported and this trigger has not fired
+		 * - "hit" bits are not supported on this trigger
+		 * To distinguish these two cases, try writing all non-zero bit
+		 * patterns to hit[1..0] to determine if the "hit" bits are supported:
+		 */
+		riscv_reg_t tdata1_tests[] = {
+			set_field(tdata1, CSR_MCONTROL6_HIT0, 1),
+			set_field(tdata1, CSR_MCONTROL6_HIT1, 1),
+			set_field(tdata1, CSR_MCONTROL6_HIT0, 1) | field_value(CSR_MCONTROL6_HIT1, 1)
+		};
+		riscv_reg_t tdata1_test_rb;
+		for (uint64_t i = 0; i < ARRAY_SIZE(tdata1_tests); ++i) {
+			if (riscv_reg_set(target, GDB_REGNO_TDATA1, tdata1_tests[i]) != ERROR_OK)
+				return M6_HIT_ERROR;
+			if (riscv_reg_get(target, &tdata1_test_rb, GDB_REGNO_TDATA1) != ERROR_OK)
+				return M6_HIT_ERROR;
+			if (riscv_reg_set(target, GDB_REGNO_TDATA1, tdata1_test_rb & ~hit_mask) != ERROR_OK)
+				return M6_HIT_ERROR;
+			if (tdata1_test_rb == tdata1_tests[i])
+				return M6_NOT_HIT;
+		}
+	}
+	return M6_HIT_NOT_SUPPORTED;
+}
+
 /**
  * Look at the trigger hit bits to find out which trigger is the reason we're
  * halted.  Sets *unique_id to the unique ID of that trigger. If *unique_id is
  * RISCV_TRIGGER_HIT_NOT_FOUND, no match was found.
  */
 
-static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_id)
+static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_id,
+		bool *need_single_step)
 {
 	/* FIXME: this function assumes that we have only one trigger that can
 	 * have hit bit set. Debug spec allows hit bit to bit set if a trigger has
@@ -1601,9 +1657,21 @@ static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_
 				break;
 			case CSR_TDATA1_TYPE_MCONTROL:
 				hit_mask = CSR_MCONTROL_HIT;
+				r->need_single_step = true;
 				break;
 			case CSR_TDATA1_TYPE_MCONTROL6:
 				hit_mask = CSR_MCONTROL6_HIT0 | CSR_MCONTROL6_HIT1;
+				if (r->trigger_tinfo_version[i] == CSR_TINFO_VERSION_0) {
+					r->need_single_step = true;
+				} else if (r->trigger_tinfo_version[i] == ERROR_TARGET_RESOURCE_NOT_AVAILABLE
+					|| r->trigger_tinfo_version[i] == CSR_TINFO_VERSION_1) {
+					mctrl6hitstatus hits_status = check_mcontrol6_hit_status(target,
+								tdata1, hit_mask);
+					if (hits_status == M6_HIT_ERROR)
+						return ERROR_FAIL;
+					if (hits_status == M6_HIT_BEFORE || hits_status == M6_HIT_NOT_SUPPORTED)
+						r->need_single_step = true;
+				}
 				break;
 			case CSR_TDATA1_TYPE_ICOUNT:
 				hit_mask = CSR_ICOUNT_HIT;
@@ -1622,8 +1690,9 @@ static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_
 		/* FIXME: this logic needs to be changed to ignore triggers that are not
 		 * the last one in the chain. */
 		if (tdata1 & hit_mask) {
-			LOG_TARGET_DEBUG(target, "Trigger %u (unique_id=%" PRIi64 ") has hit bit set.",
-				i, r->trigger_unique_id[i]);
+			LOG_TARGET_DEBUG(target, "Trigger %u (unique_id=%" PRIi64 ""
+			", need_single_step=%" PRId32 ") has hit bit set.",
+				i, r->trigger_unique_id[i], r->need_single_step);
 			if (riscv_reg_set(target, GDB_REGNO_TDATA1, tdata1 & ~hit_mask) != ERROR_OK)
 				return ERROR_FAIL;
 
@@ -2285,13 +2354,15 @@ static int set_debug_reason(struct target *target, enum riscv_halt_reason halt_r
 {
 	RISCV_INFO(r);
 	r->trigger_hit = -1;
+	r->need_single_step = false;
 	switch (halt_reason) {
 		case RISCV_HALT_EBREAK:
 			target->debug_reason = DBG_REASON_BREAKPOINT;
 			break;
 		case RISCV_HALT_TRIGGER:
 			target->debug_reason = DBG_REASON_UNDEFINED;
-			if (riscv_trigger_detect_hit_bits(target, &r->trigger_hit) != ERROR_OK)
+			if (riscv_trigger_detect_hit_bits(target, &r->trigger_hit,
+					&r->need_single_step) != ERROR_OK)
 				return ERROR_FAIL;
 			// FIXME: handle multiple hit bits
 			if (r->trigger_hit != RISCV_TRIGGER_HIT_NOT_FOUND) {
@@ -2553,10 +2624,19 @@ static int resume_prep(struct target *target, int current,
 	if (handle_breakpoints) {
 		/* To be able to run off a trigger, we perform a step operation and then
 		 * resume. If handle_breakpoints is true then step temporarily disables
-		 * pending breakpoints so we can safely perform the step. */
-		if (old_or_new_riscv_step_impl(target, current, address, handle_breakpoints,
-				false /* callbacks are not called */) != ERROR_OK)
-			return ERROR_FAIL;
+		 * pending breakpoints so we can safely perform the step.
+		 *
+		 * Two cases where single step is needed before resuming:
+		 * 1. ebreak used in software breakpoint;
+		 * 2. a trigger that is taken just before the instruction that triggered it is retired.
+		 */
+		if (target->debug_reason == DBG_REASON_BREAKPOINT
+		    || (target->debug_reason == DBG_REASON_WATCHPOINT
+			&& r->need_single_step)) {
+			if (old_or_new_riscv_step_impl(target, current, address, handle_breakpoints,
+					false /* callbacks are not called */) != ERROR_OK)
+				return ERROR_FAIL;
+		}
 	}
 
 	if (r->get_hart_state) {
@@ -5698,19 +5778,21 @@ static int check_if_trigger_exists(struct target *target, unsigned int index)
  * to determine trigger types supported by a trigger.
  * It is assumed that the trigger is already selected via writing `tselect`.
  */
-static int get_trigger_types(struct target *target, unsigned int *trigger_tinfo,
-		riscv_reg_t tdata1)
+static int get_trigger_types_and_version(struct target *target, unsigned int *trigger_tinfo,
+		int64_t *trigger_tinfo_version, riscv_reg_t tdata1)
 {
 	assert(trigger_tinfo);
 	riscv_reg_t tinfo;
 	if (riscv_reg_get(target, &tinfo, GDB_REGNO_TINFO) == ERROR_OK) {
+		*trigger_tinfo_version = get_field(tinfo, CSR_TINFO_VERSION);
 		/* tinfo.INFO == 1: trigger doesn’t exist
 		 * tinfo == 0 or tinfo.INFO != 1 and tinfo LSB is set: invalid tinfo */
 		if (tinfo == 0 || tinfo & 0x1)
 			return ERROR_TARGET_RESOURCE_NOT_AVAILABLE;
 		*trigger_tinfo = tinfo;
 		return ERROR_OK;
 	}
+	*trigger_tinfo_version = ERROR_TARGET_RESOURCE_NOT_AVAILABLE;
 	const unsigned int type = get_field(tdata1, CSR_TDATA1_TYPE(riscv_xlen(target)));
 	if (type == 0)
 		return ERROR_TARGET_RESOURCE_NOT_AVAILABLE;
@@ -5794,7 +5876,8 @@ int riscv_enumerate_triggers(struct target *target)
 		if (riscv_reg_get(target, &tdata1, GDB_REGNO_TDATA1) != ERROR_OK)
 			return ERROR_FAIL;
 
-		result = get_trigger_types(target, &r->trigger_tinfo[t], tdata1);
+		result = get_trigger_types_and_version(target, &r->trigger_tinfo[t],
+				&r->trigger_tinfo_version[t], tdata1);
 		if (result == ERROR_FAIL)
 			return ERROR_FAIL;
 		if (result == ERROR_TARGET_RESOURCE_NOT_AVAILABLE)

src/target/riscv/riscv.h

@@ -152,6 +152,17 @@ struct riscv_info {
 	/* record the tinfo of each trigger */
 	unsigned int trigger_tinfo[RISCV_MAX_TRIGGERS];
 
+	/* record the tinfo_version of each trigger */
+	int64_t trigger_tinfo_version[RISCV_MAX_TRIGGERS];
+
+	/* Record if single-step is needed prior to resuming
+	 * from a software breakpoint or trigger.
+	 * Single-step is needed if the instruction that
+	 * caused the halt was not retired. That is,
+	 * when we halted "before" that instruction.
+	 */
+	bool need_single_step;
+
 	/* For each physical trigger contains:
 	 * -1: the hwbp is available
 	 * -4: The trigger is used by the itrigger command