Add dcsr cetrig control #1255

lz-bro · 2025-05-13T09:20:05Z

Introduce RISC-V-sepecific configure parameter -cetrig

Ubuntu 20.04 is no longer available. See actions/runner-images#11101 Checkpatch-ignore: BAD_SIGN_OFF Change-Id: I0ec3e3342f9212a2a79d8dca6274e7db62ecedab Signed-off-by: Evgeniy Naydanov <[email protected]>

…-spec This pulls in some improvements from the riscv-debug-spec repo, and cleans up the licensing (riscv-debug-spec changed the license to dual CC-BY-4.0 and BSD-2-Clause specifically to make sure the generated debug_defines files don't conflict with OpenOCD licensing) Signed-off-by: Bernhard Rosenkränzer <[email protected]>

en-sc

Thank you for the patch!

Overall it looks great, the only concern I have is with the new return from riscv013_halt_reason(). I'm not sure RISCV_HALT_ERROR is the proper return code. I'd suggest adding a new status and handling it differently at the callsite.

Also, how did you test the changes? Is this at all possible to extend riscv-tests to include relevant tests?

en-sc · 2025-05-13T15:48:19Z

src/target/riscv/riscv-013.c

 	/* When false, we need to set dcsr.ebreak*, halting the target if that's
 	 * necessary. */
-	bool dcsr_ebreak_is_set;
+	bool dcsr_configs_is_set;


Please, adjust the comment to drop the mention of ebreak.
Also, this is a nitpick, but dcsr_configs_is_set does not feel right (plural with is). Maybe dcsr_config_is_set?

en-sc · 2025-05-13T15:55:13Z

src/target/riscv/riscv-013.c

 	return ERROR_OK;
 }

-static int halt_set_dcsr_ebreak(struct target *target)
+static int halt_set_dcsr_configs(struct target *target)


Suggested change

static int halt_set_dcsr_configs(struct target *target)

static int halt_set_dcsr_config(struct target *target)

For consistency with dcsr_config_is_set (if you decide to adopt the suggestion).

en-sc · 2025-05-13T15:55:32Z

src/target/riscv/riscv-013.c

@@ -2939,13 +2940,13 @@ static int assert_reset(struct target *target)
 	return riscv013_invalidate_cached_progbuf(target);
 }

-static bool dcsr_ebreak_config_equals_reset_value(const struct target *target)
+static bool dcsr_configs_equals_reset_value(const struct target *target)


Suggested change

static bool dcsr_configs_equals_reset_value(const struct target *target)

static bool dcsr_config_equals_reset_value(const struct target *target)

Same as above

en-sc · 2025-05-13T15:59:34Z

src/target/riscv/riscv-013.c

+	case CSR_DCSR_CAUSE_OTHER:
+		if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {
+			LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");
+			return RISCV_HALT_ERROR;


I believe this should be a separate entry in enum riscv_halt_reason.
Also, AFAIU, RISCV_HALT_ERROR is currently unused. Maybe a simple rename would suffice.

en-sc · 2025-05-13T16:01:09Z

src/target/riscv/riscv-013.c

@@ -5367,6 +5368,14 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target)
 		return RISCV_HALT_INTERRUPT;
 	case CSR_DCSR_CAUSE_GROUP:
 		return RISCV_HALT_GROUP;
+	case CSR_DCSR_CAUSE_OTHER:
+		if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {
+			LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");


Suggested change

LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");

LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");

I believe INFO is more appropriate here, since this is completely normal from the debugger's point of view.

en-sc · 2025-05-13T16:02:50Z

src/target/riscv/riscv.c

@@ -525,6 +527,15 @@ static struct jim_nvp nvp_ebreak_mode_opts[] = {
 	{ .name = NULL, .value = RISCV_EBREAK_MODE_INVALID }
 };

+
+#define RISCV_CETRIG_INVALID -1


I believe this can be inlined.

To be consistent with RISCV_EBREAK_MODE_INVALID, should this be kept?

en-sc · 2025-05-13T16:05:18Z

src/target/riscv/riscv.c

+{
+	if (goi->argc == 0) {
+		Jim_WrongNumArgs(goi->interp, 1, goi->argv - 1,
+				"?disable?or?enable?");


Suggested change

"?disable?or?enable?");

"?disable|enable?");

en-sc · 2025-05-13T16:09:11Z

src/target/riscv/riscv.c

+static int jim_report_cetrig_config(const struct riscv_private_config *config,
+		Jim_Interp *interp)
+{
+	const char * const cetrig_opt = jim_nvp_value2name_simple(nvp_cetrig_opts,


Suggested change

const char * const cetrig_opt = jim_nvp_value2name_simple(nvp_cetrig_opts,

const char *cetrig_opt = jim_nvp_value2name_simple(nvp_cetrig_opts,

I'd suggest dropping this const. AFAIU, the styleguide will be updated sometime to include this rule.
See https://review.openocd.org/c/openocd/+/8750/comment/984b8414_f6f804e4/

Introduce RISC-V-sepecific `configure` parameter `-cetrig`

lz-bro · 2025-05-14T08:18:54Z

Thank you for the patch!

Overall it looks great, the only concern I have is with the new return from riscv013_halt_reason(). I'm not sure RISCV_HALT_ERROR is the proper return code. I'd suggest adding a new status and handling it differently at the callsite.

Also, how did you test the changes? Is this at all possible to extend riscv-tests to include relevant tests?

Now, I have tested the changes on XiangShan-KunMingHu platform and spike by enabling smdbltrp extension.
You can find the corresponding ISA extension from the XiangShan Github repository: https://github.com/OpenXiangShan/XiangShan/blob/master/src/main/scala/xiangshan/Parameters.scala
I don't have any good ideas yet to extend riscv-tests to include relevant tests.

JanMatCodasip

Overall this looks good. I have managed a partial review and am sending my comments so far.

JanMatCodasip · 2025-05-23T12:54:21Z

src/target/riscv/riscv.h

@@ -75,6 +75,7 @@ enum riscv_halt_reason {
 	RISCV_HALT_TRIGGER,
 	RISCV_HALT_UNKNOWN,
 	RISCV_HALT_GROUP,
+	RISCV_HALT_CRITERR,


If you like - this will be more clear to code readers:

Suggested change

RISCV_HALT_CRITERR,

RISCV_HALT_CRITICAL_ERROR,

JanMatCodasip · 2025-05-23T12:55:41Z

src/target/riscv/riscv-013.c

@@ -5367,6 +5368,14 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target)
 		return RISCV_HALT_INTERRUPT;
 	case CSR_DCSR_CAUSE_GROUP:
 		return RISCV_HALT_GROUP;
+	case CSR_DCSR_CAUSE_OTHER:
+		if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {
+			LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");


Since this is an error condition of the target, I would use at least warning level. Perhaps even an error?

Suggested change

LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");

LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");

JanMatCodasip · 2025-05-23T12:58:32Z

src/target/riscv/riscv-013.c

 	dcsr = set_field(dcsr, CSR_DCSR_EBREAKU, config->dcsr_ebreak_fields[RISCV_MODE_U]);
 	dcsr = set_field(dcsr, CSR_DCSR_EBREAKVS, config->dcsr_ebreak_fields[RISCV_MODE_VS]);
 	dcsr = set_field(dcsr, CSR_DCSR_EBREAKVU, config->dcsr_ebreak_fields[RISCV_MODE_VU]);
+	dcsr = set_field(dcsr, CSR_DCSR_CETRIG, config->dcsr_cetrig);
 	if (dcsr != original_dcsr &&
 			riscv_reg_set(target, GDB_REGNO_DCSR, dcsr) != ERROR_OK)
 		return ERROR_FAIL;
-	info->dcsr_ebreak_is_set = true;
+	info->dcsr_config_is_set = true;
 	return ERROR_OK;


Would it make sense to read DCSR back and see if these WARL bits were really set as the user wanted? If not, then a log info print could be made.

If you think this is a useful improvement, please open an issue or make a TODO comment into the code. I don't insist on making it in this PR. It'd better to do this separately.

JanMatCodasip · 2025-05-23T13:01:11Z

src/target/riscv/riscv-013.c

+		if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {
+			LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");
+			return RISCV_HALT_CRITERR;
+		}
+		LOG_TARGET_ERROR(target, "Unknown DCSR extcause field: 0x%"
+				PRIx64, get_field(dcsr, CSR_DCSR_EXTCAUSE));
+		return RISCV_HALT_UNKNOWN;


If you like, this could be a switch instead of if cascade.

This will encourage better coding style in future, when there are more EXTCAUSE values.

Suggested change

if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {

LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");

return RISCV_HALT_CRITERR;

}

LOG_TARGET_ERROR(target, "Unknown DCSR extcause field: 0x%"

PRIx64, get_field(dcsr, CSR_DCSR_EXTCAUSE));

return RISCV_HALT_UNKNOWN;

switch (get_field(dcsr, CSR_DCSR_EXTCAUSE))

{

case 0:

/* ... */

return RISCV_HALT_CRITICAL_ERROR;

default:

/* ... */

return RISCV_HALT_UNKNOWN;

}

JanMatCodasip · 2025-05-23T13:01:49Z

src/target/riscv/riscv-013.c

 	 * necessary. */
-	bool dcsr_ebreak_is_set;
+	bool dcsr_config_is_set;


Suggested change

bool dcsr_config_is_set;

bool dcsr_register_is_set;

JanMatCodasip · 2025-05-23T13:04:01Z

src/target/riscv/riscv-013.c

+	/* When false, we need to set dcsr config, halting the target if that's
 	 * necessary. */


Suggested change

/* When false, we need to set dcsr config, halting the target if that's

* necessary. */

/* When false, we need to configure certain bits in the dcsr register.

* To do that, we may momentarily halt the target, if necessary. */

en-sc and others added 2 commits May 7, 2025 09:37

Update image in build32

8d5f2fe

Ubuntu 20.04 is no longer available. See actions/runner-images#11101 Checkpatch-ignore: BAD_SIGN_OFF Change-Id: I0ec3e3342f9212a2a79d8dca6274e7db62ecedab Signed-off-by: Evgeniy Naydanov <[email protected]>

lz-bro force-pushed the add-dcsr-cetrig-control branch 3 times, most recently from 6b88574 to 605e820 Compare May 13, 2025 09:33

en-sc reviewed May 13, 2025

View reviewed changes

lz-bro force-pushed the add-dcsr-cetrig-control branch from 605e820 to d1d2b0a Compare May 14, 2025 07:10

target/riscv: add cetrig control

aed2254

Introduce RISC-V-sepecific `configure` parameter `-cetrig`

lz-bro force-pushed the add-dcsr-cetrig-control branch from d1d2b0a to aed2254 Compare May 14, 2025 07:14

JanMatCodasip reviewed May 23, 2025

View reviewed changes

	static int halt_set_dcsr_configs(struct target *target)
	static int halt_set_dcsr_config(struct target *target)

	static bool dcsr_configs_equals_reset_value(const struct target *target)
	static bool dcsr_config_equals_reset_value(const struct target *target)

	LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");
	LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");

	const char * const cetrig_opt = jim_nvp_value2name_simple(nvp_cetrig_opts,
	const char *cetrig_opt = jim_nvp_value2name_simple(nvp_cetrig_opts,

		/* When false, we need to set dcsr config, halting the target if that's
		* necessary. */

Add dcsr cetrig control #1255

Are you sure you want to change the base?

Add dcsr cetrig control #1255

Uh oh!

Conversation

lz-bro commented May 13, 2025

Uh oh!

en-sc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lz-bro commented May 14, 2025

Uh oh!

JanMatCodasip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!