target/riscv: cleanup trigger setup

* Add a warning when eq trigger is setup and it's behavior is different from other triggers. * Make eq trigger's behavior consistent with other triggers in case of length == 1. * Fix a bug in setting chained triggers (LT, GT case). * Improve logging. Change-Id: Id1ed0d11971b8ed875afbb979e6c8a8b51dd3818 Signed-off-by: Evgeniy Naydanov <[email protected]>
riscv-collab · Jul 17, 2023 · b5730cc · b5730cc
1 parent 92c0319
commit b5730cc
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 12 deletions.
diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c
@@ -655,9 +655,18 @@ struct trigger_request_info {
 	riscv_reg_t tdata1_ignore_mask;
 };
 
+static void log_trigger_request_info(struct trigger_request_info trig_info)
+{
+	LOG_DEBUG("tdata1=%" PRIx64 ", tdata2=%" PRIx64 ", tdata1_ignore_mask=%" PRIx64,
+			trig_info.tdata1, trig_info.tdata2, trig_info.tdata1_ignore_mask);
+};
+
 static int try_setup_single_match_trigger(struct target *target,
 		struct trigger *trigger, struct trigger_request_info trig_info)
 {
+	LOG_TARGET_DEBUG(target, "trying to set up a match trigger");
+	log_trigger_request_info(trig_info);
+
 	int trigger_type =
 		get_field(trig_info.tdata1, CSR_MCONTROL_TYPE(riscv_xlen(target)));
 	int ret = ERROR_TARGET_RESOURCE_NOT_AVAILABLE;
@@ -683,6 +692,9 @@ static int try_setup_chained_match_triggers(struct target *target,
 		struct trigger *trigger, struct trigger_request_info t1,
 		struct trigger_request_info t2)
 {
+	LOG_TARGET_DEBUG(target, "trying to set up a chain of match triggers");
+	log_trigger_request_info(t1);
+	log_trigger_request_info(t2);
 	int trigger_type =
 		get_field(t1.tdata1, CSR_MCONTROL_TYPE(riscv_xlen(target)));
 	int ret = ERROR_TARGET_RESOURCE_NOT_AVAILABLE;
@@ -717,8 +729,10 @@ static int try_setup_chained_match_triggers(struct target *target,
 struct match_triggers_tdata1_fields {
 	riscv_reg_t common;
 	struct {
+		/* Other values are available for this field,
+		 * but currently only `any` is needed.
+		 */
 		riscv_reg_t any;
-		riscv_reg_t s8bit;
 	} size;
 	struct {
 		riscv_reg_t enable;
@@ -752,10 +766,7 @@ static struct match_triggers_tdata1_fields fill_match_triggers_tdata1_fields_t2(
 		.size = {
 			.any =
 				field_value(CSR_MCONTROL_SIZELO, CSR_MCONTROL_SIZELO_ANY & 3) |
-				field_value(CSR_MCONTROL_SIZEHI, (CSR_MCONTROL_SIZELO_ANY >> 2) & 3),
-			.s8bit =
-				field_value(CSR_MCONTROL_SIZELO, CSR_MCONTROL_SIZELO_8BIT & 3) |
-				field_value(CSR_MCONTROL_SIZEHI, (CSR_MCONTROL_SIZELO_8BIT >> 2) & 3)
+				field_value(CSR_MCONTROL_SIZEHI, (CSR_MCONTROL_SIZELO_ANY >> 2) & 3)
 		},
 		.chain = {
 			.enable = field_value(CSR_MCONTROL_CHAIN, CSR_MCONTROL_CHAIN_ENABLED),
@@ -793,8 +804,7 @@ static struct match_triggers_tdata1_fields fill_match_triggers_tdata1_fields_t6(
 			field_value(CSR_MCONTROL6_LOAD, trigger->is_read) |
 			field_value(CSR_MCONTROL6_STORE, trigger->is_write),
 		.size = {
-			.any = field_value(CSR_MCONTROL6_SIZE, CSR_MCONTROL6_SIZE_ANY),
-			.s8bit = field_value(CSR_MCONTROL6_SIZE, CSR_MCONTROL6_SIZE_8BIT)
+			.any = field_value(CSR_MCONTROL6_SIZE, CSR_MCONTROL6_SIZE_ANY)
 		},
 		.chain = {
 			.enable = field_value(CSR_MCONTROL6_CHAIN, CSR_MCONTROL6_CHAIN_ENABLED),
@@ -852,24 +862,38 @@ static int maybe_add_trigger_t2_t6(struct target *target,
 		struct trigger_request_info lt_1 = {
 			.tdata1 = fields.common | fields.size.any | fields.chain.enable |
 				fields.match.lt,
-			.tdata2 = trigger->address,
+			.tdata2 = trigger->address + trigger->length,
 			.tdata1_ignore_mask = fields.tdata1_ignore_mask
 		};
 		struct trigger_request_info ge_2 = {
 			.tdata1 = fields.common | fields.size.any | fields.chain.disable |
 				fields.match.ge,
-			.tdata2 = trigger->address + trigger->length,
+			.tdata2 = trigger->address,
 			.tdata1_ignore_mask = fields.tdata1_ignore_mask
 		};
 		ret = try_setup_chained_match_triggers(target, trigger, lt_1, ge_2);
 		if (ret != ERROR_TARGET_RESOURCE_NOT_AVAILABLE)
 			return ret;
 	}
 	LOG_TARGET_DEBUG(target, "trying to setup equality match trigger");
+	if (trigger->length > 1) {
+		LOG_TARGET_DEBUG(target, "Trigger will match accesses at address 0x%" TARGET_PRIxADDR
+				", but may not match accesses at addresses in the inclusive range from 0x%"
+				TARGET_PRIxADDR " to 0x%" TARGET_PRIxADDR ".", trigger->address,
+				trigger->address + 1, trigger->address + trigger->length - 1);
+		RISCV_INFO(info);
+		if (!info->inconsistent_trigger_warning_was_displayed)
+			/* This message is displayed only once per target to avoid
+			 * overwhelming the user with such messages on resume.
+			 */
+			LOG_TARGET_WARNING(target,
+					"This and some other triggers will match accesses at the specifyed address, "
+					"but may not match accesses into the range [address, address + length]");
+		info->inconsistent_trigger_warning_was_displayed = true;
+	}
 	struct trigger_request_info eq = {
-		.tdata1 = fields.common |
-			(trigger->length == 1 ? fields.size.s8bit : fields.size.any) |
-			fields.chain.disable | fields.match.eq,
+		.tdata1 = fields.common | fields.size.any | fields.chain.disable |
+			fields.match.eq,
 		.tdata2 = trigger->address,
 		.tdata1_ignore_mask = fields.tdata1_ignore_mask
 	};

diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h
@@ -292,6 +292,8 @@ struct riscv_info {
 	int64_t last_activity;
 
 	yes_no_maybe_t vsew64_supported;
+
+	bool inconsistent_trigger_warning_was_displayed;
 };
 
 COMMAND_HELPER(riscv_print_info_line, const char *section, const char *key,