Tell external debugger whether mcontrol6.hit bits are supported

[JanMatCodasip]

The 1.0 version of the debug specification gives a lot of freedom to the trigger implementers if and how to implement support for mcontrol6.hit{0,1} bits. At the same time it is not clearly reported back to the external debugger if hit bits are supported.

As an extreme case, the current version of the spec permits that a given mcontrol6 trigger supports the hit bits for certain combination of fields of mcontrol6 and but does not support them for other combination of the fields of that very same trigger.

For these reasons, it is difficult for the external debugger to distinguish what hit[1:0] == 0b00 actually means:

• a) Hit bits are not supported on this trigger, and we do not know if the trigger fired.
• b) Hit bits are supported on this trigger, and the trigger has not fired.

In order to reliably tell these two cases apart, the external debugger currently needs to perform a lengthy procedure:

for each mcontrol6 trigger whose hit[1:0] == 0b00:
    try writing 0b01, 0b10 and 0b11 to hit[1:0], and retain other tdata1 bits; 
    if any of the cases results in hit[1:0] non-zero:
        restore hit[1:0] to 0b00;
        return this_trigger_is_not_hit;
    else:
        return this_trigger_does_not_support_hit;

With the above in mind, I propose that 1.1 version of the spec introduces a clear mechanism that will tell external debuggers whether mcontrol6.hit bits are supported in the hardware.

I can think of these options:

Option 1: Make mcontrol6.hit bits mandatory for all trigger implementations and increment tinfo.version.

Option 2: Introduce new mcontrol6.hit2 bit which would allow to encode all the outcomes below. Don't allow partial implementation of some but not all of the hit bits. Make the hit bits read only, similarly to dcsr.cause. Increment tinfo.version.

• mcontrol.hit[2:0] == 0b000 - Trigger not hit
• mcontrol.hit[2:0] == 0b001 - Trigger hit before
• mcontrol.hit[2:0] == 0b010 - Trigger hit sometime after
• mcontrol.hit[2:0] == 0b011 - Trigger hit immediately after
• mcontrol.hit[2:0] == 0b100 - Hit information not supported
• mcontrol.hit[2:0] == 0b101, 0b110, 0b111 - Reserved for future use

What is your opinion on this? Thank you.

[en-sc]

@JanMatCodasip, thank you for the proposal!

I'm also deeply concerned by the current state of hit bits, and the options you suggest seem reasonable.
I'm inclined towards option 2, since option 1 seems too restrictive to me.

I have a couple of comments:

Make the hit bits read only

Hit bits should be cleared by SW (only SW knows when it got all the necessary info), so at least the hit field should be declared as W1C, and considering that writes of 0b111 to 0b100 should leave 0b100 the field should be WARL.

Don't allow partial implementation of some but not all of the hit bits.

I don't get what it means in case of a read-only field.
E.g.:
Imagine a simple system (without OOO execution etc.) on which the triggers fire only before or immediately after. So "sometime after" is never observed in the value read from hit field.
Even if the field is WARL I don't see an issue if such system decides to ignore writes of "sometime after" to the hit field.

[en-sc]

In order to reliably tell these two cases apart, the external debugger currently needs to perform a lengthy procedure:

There is also an alternative solution to deriving support of hit field values that doesn't require that much work.

When setting the trigger check if the desired timing is the only one supported (based on Table 15. Suggested Trigger Timings) by looping through mcontrol6.hit values.

There are a number of benefits to this approach:

• Trigger is configured once (it may be deleted and then re-set multiple times, but there is no need to re-discover the set of valid hit field values).
• A meaningful diagnostic can be returned to user based on the action they just took. Meaning:

> wp ...
WARNING: unexpected `hit` support (not the recommended one)
or
WARNING: `hit` not supported.

vs

> wp ...
> ...
> resume
...
WARNING: wp on address and `hit == before`
or
WARNING: unable to derive which trigger has fired.

There is a caveat -- nothing prevents hit field support from changing over time (i.e. when the trigger has fired the set of valid hit field values may differ).

[JanMatCodasip]

@en-sc, thank you for your replies.

JM: Make the hit bits read only

en-sc: Hit bits should be cleared by SW (only SW knows when it got all the necessary info)

I was thinking that hit bits could be read-only and reflect the current state - the state at the moment of last halt. This would be similar to the behavior of dcsr.cause which also does not need to be cleared by the external debugger.

But anyway, that would be just an unrelated side-improvement, and I don't feel strongly about it.

JM: Don't allow partial implementation of some but not all of the hit bits.

en-sc: I don't get what it means in case of a read-only field.

You're right, that sentence does not make too much sense. I have removed it from my original comment.

JM: to reliably tell these two cases apart, the external debugger currently needs to perform a lengthy procedure [...]

en-sc: There is also an alternative solution [...] that doesn't require that much work. [...] There is a caveat -- nothing prevents hit field support from changing over time.

Thank you for mentioning that approach. I expect it will work for virtually all existing targets. That is, I am not too worried about the caveat you mentioned (it is technically possible, but in my opinion very improbable in practice).

[en-sc]

JM: Make the hit bits read only

en-sc: Hit bits should be cleared by SW (only SW knows when it got all the necessary info)
JM: [...] This would be similar to the behavior of dcsr.cause which also does not need to be cleared by the external debugger. [...]

But what about other actions (e.g. breakpoint exception or start tracing)? Native (self-hosted) debugger has no equivalent to writing dmcontrol.resumereq.

[JanMatCodasip]

@en-sc, you're right, I forgot about other trigger actions than "enter debug mode".

I have removed the sentence about making hit bits read-only from my original comment.

[rtwfroody]

The traditional answer to these kinds of questions has been that it could go in the https://github.com/riscv/configuration-structure. That project seems to have moved away from describing implementation details like this, but it might be added before Debug 1.1 happens.

[JanMatCodasip]

The traditional answer to these kinds of questions has been that it could go in the https://github.com/riscv/configuration-structure. [...]

Still, my preference would be to inform the external debugger about the hit bits support by the means of the trigger extension itself, and not use the configuration structure.

My reasoning for that is:

• Hit bits support is an implementation detail of Sdtrig, and so keeping that information outside of Sdtrig (in the configuration structure) does not seem to be a good fit.
• The debug spec itself promises that it allows the debugger autodetect what is actually implemented (Section 1.5, point 5: "A debugger can discover almost [1] everything it needs to know itself, without user configuration.").