target/riscv:Perform single step before resume if necessary

[sunnyzhu-learning]

Reason for modification：

1. When wfi in starting, this scene does not require single step behavior;
2. When dpc equal to Trigger address, this scene will cause hart not halted.

@en-sc @JanMatCodasip please code review~

[en-sc]

@sunnyzhu-learning, thank you for the patch.

Please note that hit bits are optional and therefore the behavior of targets that don't support them should also be considered.

[sunnyzhu-learning]

Q: Please note that hit bits are optional and therefore the behavior of targets that don't support them should also be considered.
A: internal discussion a lot, modified as above, considering hit bits are optional.

[sunnyzhu-learning]

1. Single step is needed if any mcontrol trigger has timing before. This may cause unnecessary single step in case like trigger0(timing before) not fire and trigger1(timing after) fires, but it's a conservative handling to make sure single step is performed in timing before case.
2. hit bits 0 stands for trigger not firing or hit not support by HW. Single step is needed if any mcontrol6 trigger has hit0[1] before, or hit not support by HW, or even not firing if hit is supported. This may cause unnecessary single step, but we have to consider the case where hit is not support by HW.
3. this patch will benefit in cases like
   a. all tirggers have timing/hit0[1] after for mcontrol[6] trigger.
   b. trigger0 is timing after and hit is implenmented.

[sunnyzhu-learning]

I don't have a best solution to solve this question, above patch is a optimize solution, Please help with code review,thanks~
@en-sc

[sunnyzhu-learning]

update patch, internal code review done, @en-sc please code review(1062402) thanks~

[en-sc]

I don't like this change in it's current state. The reason is:

1. There are too many operations to derive hit field support that are performed in the most common case -- a trigger is not hit.
2. Handling mcontrol type does not consider the fact that mcontrol.hit field can be set for a matched trigger that did not fire. Therefore there can be a matched mcontrol trigger and another trigger that fired with a different timing that lacks hit field support.

My suggestion is:
When looking into which trigger was hit (riscv_trigger_detect_hit_bits()):

• r->need_single_step is initializes to false.
• In case there is a mcontrol trigger (regardless if it has fired or not): r->need_single_step = true due to the fact that there can be another mcontrol or mcontrol6 trigger that lacks hit field support and was hit before.
• In case there is a mcontrol6 trigger: if hit == after || hit == immed_after r->need_single_step |= false. This condition can be detected without any additional reads from the target.
• In case of itrigger, etrigger or tmexttrigger trigger r->need_single_step = true for the same reason as mcontrol.
• In case there is a trigger of some other type (if I'm correct only icount is unaccounted for at this point) that has it's hit field set, r->need_single_step = false.

IMHO this will cover the most common cases without the need for excessive investigation of hit field support.

[en-sc]

Please note: analyzing timings for mcontrol6 depends on tinfo.verison.

[sunnyzhu-learning]

Q:

1. There are too many operations to derive hit field support that are performed in the most common case -- a trigger is not hit.

When halt_reason is RISCV_HALT_TRIGGER, then enter riscv_trigger_detect_hit_bits(), I think this checking operations acceptably.

2. Handling mcontrol type does not consider the fact that mcontrol.hit field can be set for a matched trigger that did not fire. Therefore there can be a matched mcontrol trigger and another trigger that fired with a different timing that lacks hit field support.

Yeah，you are right，mcontrol.hit Description：

If this bit is implemented then it must become set 
when this trigger fires and may become set when this trigger matches.

your suggestion:

• In case there is a mcontrol trigger (regardless if it has fired or not): r->need_single_step = true due to the fact that there can be another mcontrol or mcontrol6 trigger that lacks hit field support and was hit before.

The solution is too crude, but I agree your suggestion,

example:
A CPU support two triggers, T0(mcontrol.hit=1[match but not fire], mcontrol.timing=after), then r->need_single_step = false, break, T1(mcontrol.hit=0[not support], mcontrol.timing=before) fire，now due to

riscv-openocd/src/target/riscv/riscv.c

Line 1649 in f9a1292

if (tdata1 & hit_mask) {

break, T1 is not needed single step.

next patch changes to:

case CSR_TDATA1_TYPE_MCONTROL:
	hit_mask = CSR_MCONTROL_HIT;
	r->need_single_step = true;
	break;

• In case there is a mcontrol6 trigger: if hit == after || hit == immed_after r->need_single_step |= false. This condition can be detected without any additional reads from the target.

Internal discussion a lot, Disagree your suggestion, hit == after || hit == immed_after included in function check_mcontrol6_hit_status logic is ok，other releated function can call it directly, because it is fully mcontrol6.hit0[1] info.

• In case of itrigger, etrigger or tmexttrigger trigger r->need_single_step = true for the same reason as mcontrol.

I don't agree your suggestion, reference Debug spec 5.7.14 Description:

When the trigger fires, all CSRs are updated for the interrupt trap as defined by the Privileged Spec,
and the requested action is taken just before the first instruction of the trap handler is executed.
If the trigger fires with action =0 then zero is written to the tval CSR on the breakpoint trap (see
5.4)

this can ensure itrigger, etrigger or tmexttrigger trigger is timing.after ? r->need_single_step should be used false by default when other type triggers not support.

• In case there is a trigger of some other type (if I'm correct only icount is unaccounted for at this point) that has it's hit field set, r->need_single_step = false.

I agree, use default is ok.

Q: If a trigger supports a subset of hit field values, at this point the value of the hit field may be not 0. (tdata1_test_rb != tdata1_test does not mean tdata1.hit is zero in such case).

this case first considering hit_info == CSR_MCONTROL6_HIT0_FALSE, I do not understand this comment.

Q: Please, consider looping over an array of tdata1 values to check:

I agree this change, next patch addressed it.

Q: analyzing timings for mcontrol6 depends on tinfo.verison.

I agree this change, next patch addressed it.

Last, Thanks a lot your suggestions, Helped me a lot~

[en-sc]

When halt_reason is RISCV_HALT_TRIGGER, then enter riscv_trigger_detect_hit_bits(), I think this checking operations acceptably.

The thing is, even when riscv_halt_reason() is RISCV_HALT_TRIGGER there can be plenty of triggers on the platform and only one of them is hit. This is what I mean by "too many operations to derive hit field support that are performed in the most common case".

this case first considering hit_info == CSR_MCONTROL6_HIT0_FALSE, I do not understand this comment.

See the comment in the discussion: #1144 (comment)

this can ensure itrigger, etrigger or tmexttrigger trigger is timing.after ? r->need_single_step should be used false by default when other type triggers not support.

Yes, you are right. My mistake.

[JanMatCodasip]

Overall I like the idea and thank you for this contribution.

Care needs to be taken that the feature is implemented correctly and that the code remains clear and understandable after the change.

Please, take a look at my comments.

[sunnyzhu-learning]

please, update your code to read tinfo.version just once.
Please, also print the obtained tinfo.version value to the log (LOG_TARGET_DEBUG) so that it becomes easier to troubleshoot issues.

addressed，move tinfo.version value to add_trigger() @JanMatCodasip Please code review,thanks~

[JanMatCodasip]

LGTM.

@sunnyzhu-learning Thank you for the contribution and for keeping up with the review suggestions.

[en-sc]

LGTM

[en-sc]

@sunnyzhu-learning, I don't quite get it:
Why RISCV_TINFO_VERSION_UNKNOWN is handled the same way as CSR_TINFO_VERSION_1 and not the CSR_TINFO_VERSION_0?

Shouldn't this be rewritten as:

				if (r->tinfo_version == CSR_TINFO_VERSION_0
					|| r->tinfo_version == RISCV_TINFO_VERSION_UNKNOWN) {
					*need_single_step = true;
				} else if (r->tinfo_version == CSR_TINFO_VERSION_1) {

[sunnyzhu-learning]

Considering RISCV_TINFO_VERSION_UNKNOWN equal to tinfo register is not implemented, and mcontrol6 is implemented, so RISCV_TINFO_VERSION_UNKNOWN is handled the same way as CSR_TINFO_VERSION_1, this is my understand.