target/riscv: clean-up register invalidation

* Registers were not invalidated if the hart became unavailable. * Improved logging in the case register invalidation involves loss of information. Change-Id: Icfb5e190dd6dcb1a97e4d314d802466cab7a01e4 Signed-off-by: Evgeniy Naydanov <[email protected]>
riscv-collab · Dec 10, 2024 · a3e1409 · a3e1409
1 parent ea8f9d5
commit a3e1409
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 21 deletions.
diff --git a/src/target/riscv/riscv-011.c b/src/target/riscv/riscv-011.c
@@ -1136,7 +1136,7 @@ static int execute_resume(struct target *target, bool step)
 	}
 
 	target->state = TARGET_RUNNING;
-	register_cache_invalidate(target->reg_cache);
+	riscv_reg_cache_invalidate_all(target);
 
 	return ERROR_OK;
 }

diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c
@@ -2728,6 +2728,13 @@ static int handle_became_unavailable(struct target *target,
 		enum riscv_hart_state previous_riscv_state)
 {
 	RISCV013_INFO(info);
+
+	if (riscv_reg_cache_any_dirty(target, LOG_LVL_WARNING))
+		LOG_TARGET_WARNING(target, "Discarding values of dirty registers "
+				"(due to target becoming unavailable).");
+
+	riscv_reg_cache_invalidate_all(target);
+
 	info->dcsr_ebreak_is_set = false;
 	return ERROR_OK;
 }
@@ -5435,6 +5442,8 @@ static int riscv013_step_or_resume_current_hart(struct target *target,
 	if (riscv_reg_flush_all(target) != ERROR_OK)
 		return ERROR_FAIL;
 
+	riscv_reg_cache_invalidate_all(target);
+
 	dm013_info_t *dm = get_dm(target);
 	/* Issue the resume command, and then wait for the current hart to resume. */
 	uint32_t dmcontrol = DM_DMCONTROL_DMACTIVE | DM_DMCONTROL_RESUMEREQ;

diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c
@@ -290,7 +290,6 @@ static const virt2phys_info_t sv57x4 = {
 
 static enum riscv_halt_reason riscv_halt_reason(struct target *target);
 static void riscv_info_init(struct target *target, struct riscv_info *r);
-static void riscv_invalidate_register_cache(struct target *target);
 static int riscv_step_rtos_hart(struct target *target);
 
 static void riscv_sample_buf_maybe_add_timestamp(struct target *target, bool before)
@@ -2485,10 +2484,15 @@ static int riscv_halt_go_all_harts(struct target *target)
 				return ERROR_FAIL;
 		}
 	} else {
+		// Safety check:
+		if (riscv_reg_cache_any_dirty(target, LOG_LVL_ERROR))
+			LOG_TARGET_INFO(target, "BUG: Registers should not be dirty while "
+					"the target is not halted!");
+
+		riscv_reg_cache_invalidate_all(target);
+
 		if (r->halt_go(target) != ERROR_OK)
 			return ERROR_FAIL;
-
-		riscv_invalidate_register_cache(target);
 	}
 
 	return ERROR_OK;
@@ -2572,7 +2576,11 @@ static int riscv_assert_reset(struct target *target)
 	struct target_type *tt = get_target_type(target);
 	if (!tt)
 		return ERROR_FAIL;
-	riscv_invalidate_register_cache(target);
+
+	if (riscv_reg_cache_any_dirty(target, LOG_LVL_INFO))
+		LOG_TARGET_INFO(target, "Discarding values of dirty registers.");
+
+	riscv_reg_cache_invalidate_all(target);
 	return tt->assert_reset(target);
 }
 
@@ -2699,7 +2707,11 @@ static int resume_go(struct target *target, int current,
 static int resume_finish(struct target *target, int debug_execution)
 {
 	assert(target->state == TARGET_HALTED);
-	register_cache_invalidate(target->reg_cache);
+	if (riscv_reg_cache_any_dirty(target, LOG_LVL_ERROR))
+		LOG_TARGET_ERROR(target,
+				"BUG: registers should have been flushed by this point.");
+
+	riscv_reg_cache_invalidate_all(target);
 
 	target->state = debug_execution ? TARGET_DEBUG_RUNNING : TARGET_RUNNING;
 	target->debug_reason = DBG_REASON_NOTHALTED;
@@ -4001,7 +4013,15 @@ static int riscv_openocd_step_impl(struct target *target, int current,
 		LOG_TARGET_ERROR(target, "Unable to step rtos hart.");
 	}
 
-	register_cache_invalidate(target->reg_cache);
+	if (riscv_reg_cache_any_dirty(target, LOG_LVL_ERROR)) {
+		/* If this happens, it means there is a bug in the previous
+		 * register-flushing algorithm: not all registers were flushed
+		 * back to the target prior to single-step. */
+		LOG_TARGET_ERROR(target,
+				"BUG: registers should have been flushed by this point.");
+	}
+
+	riscv_reg_cache_invalidate_all(target);
 
 	if (info->isrmask_mode == RISCV_ISRMASK_STEPONLY)
 		if (riscv_interrupts_restore(target, current_mstatus) != ERROR_OK) {
@@ -5177,7 +5197,7 @@ COMMAND_HANDLER(riscv_exec_progbuf)
 	if (riscv_reg_flush_all(target) != ERROR_OK)
 		return ERROR_FAIL;
 	int error = riscv_program_exec(&prog, target);
-	riscv_invalidate_register_cache(target);
+	riscv_reg_cache_invalidate_all(target);
 
 	if (error != ERROR_OK) {
 		LOG_TARGET_ERROR(target, "exec_progbuf: Program buffer execution failed.");
@@ -5731,8 +5751,6 @@ static int riscv_resume_go_all_harts(struct target *target)
 	} else {
 		LOG_TARGET_DEBUG(target, "Hart requested resume, but was already resumed.");
 	}
-
-	riscv_invalidate_register_cache(target);
 	return ERROR_OK;
 }
 
@@ -5826,17 +5844,6 @@ unsigned int riscv_vlenb(const struct target *target)
 	return r->vlenb;
 }
 
-static void riscv_invalidate_register_cache(struct target *target)
-{
-	/* Do not invalidate the register cache if it is not yet set up
-	 * (e.g. when the target failed to get examined). */
-	if (!target->reg_cache)
-		return;
-
-	LOG_TARGET_DEBUG(target, "Invalidating register cache.");
-	register_cache_invalidate(target->reg_cache);
-}
-
 int riscv_get_hart_state(struct target *target, enum riscv_hart_state *state)
 {
 	RISCV_INFO(r);

diff --git a/src/target/riscv/riscv_reg.c b/src/target/riscv/riscv_reg.c
@@ -888,6 +888,34 @@ static int riscv_set_or_write_register(struct target *target,
 	return ERROR_OK;
 }
 
+bool riscv_reg_cache_any_dirty(const struct target *target, int log_level)
+{
+	bool any_dirty = false;
+
+	if (!target->reg_cache)
+		return any_dirty;
+
+	for (unsigned int number = 0; number < target->reg_cache->num_regs; ++number) {
+		const struct reg * const reg = riscv_reg_impl_cache_entry(target, number);
+		assert(riscv_reg_impl_is_initialized(reg));
+		if (reg->dirty) {
+			log_printf_lf(log_level, __FILE__, __LINE__, __func__,
+					"[%s] Register %s is dirty!", target_name(target), reg->name);
+			any_dirty = true;
+		}
+	}
+	return any_dirty;
+}
+
+void riscv_reg_cache_invalidate_all(struct target *target)
+{
+	if (!target->reg_cache)
+		return;
+
+	LOG_TARGET_DEBUG(target, "Invalidating register cache.");
+	register_cache_invalidate(target->reg_cache);
+}
+
 /**
  * This function is used to change the value of a register. The new value may
  * be cached, and may not be written until the hart is resumed.

diff --git a/src/target/riscv/riscv_reg.h b/src/target/riscv/riscv_reg.h
@@ -21,6 +21,17 @@ void riscv_reg_free_all(struct target *target);
 
 /** Write all dirty registers to the target. */
 int riscv_reg_flush_all(struct target *target);
+/**
+ * Check whether there are any dirty registers in the OpenOCD's register cache.
+ * In addition, all dirty registers will be reported to the log using the
+ * supplied "log_level".
+ */
+bool riscv_reg_cache_any_dirty(const struct target *target, int log_level);
+/**
+ * Invalidate all registers - forget their cached register values.
+ * WARNING: If a register was dirty, its walue will be silently lost!
+ */
+void riscv_reg_cache_invalidate_all(struct target *target);
 /**
  * Set the register value. For cacheable registers, only the cache is updated
  * (write-back mode).