provide riscv-specific controls to disable triggers from being used for watchpoints

[kr-sc]

Add a new riscv specific commands to disable triggers

[JanMatCodasip]

I have done only a superficial review so far.

What is the motivation for having these new configuration options, please? If the target supports the given trigger types, why not to always use them? Why would someone force-disable that functionality when the target provides it?

[kr-sc]

What is the motivation for having these new configuration options, please? If the target supports the given trigger types, why not to always use them? Why would someone force-disable that functionality when the target provides it?

I think, it will be usefull for hardware debugging (for testing particular type of trigger).

[JanMatCodasip]

I think, it will be useful for hardware debugging (for testing particular type of trigger).

If you wish to test the HW with the help of OpenOCD, I recommend to stick with the existing low-level commands (poll off, riscv dmi_read, riscv dmi_write, reg, ...). You can for example write own TCL script that will perform the requested operations (e.g. set some triggers) and check the outcome. The tests can even be scripted in python -- you can connect through the telnet or TCL channels and execute commands this way. This approach provides more flexibility and allows to test virtually any debug feature.

I would rather not add any TCL commands that have no other use case than testing.

[kr-sc]

If you wish to test the HW with the help of OpenOCD, I recommend to stick with the existing low-level commands (poll off, riscv dmi_read, riscv dmi_write, reg, ...). You can for example write own TCL script that will perform the requested operations (e.g. set some triggers) and check the outcome. The tests can even be scripted in python -- you can connect through the telnet or TCL channels and execute commands this way. This approach provides more flexibility and allows to test virtually any debug feature.

If we think like that we can write everything with low-level jtag commands (and it is possible, I think), but I think that we want to extend existing codebase to simplify control of debugged unit.

We have a problem with our hardware: equality_match trigger is supported, but it compare only first byte of address of memory access request: for example, if you set a wp on 0x100 with size 4 and try to read from 0x98 with size 4, wp will not hit. So, we want to add an option to turn off usage of this trigger for wp (and at the same time we made a same options for other types of triggers).

[JanMatCodasip]

Thank you for sharing the motivation for this feature.

If we think like that we can write everything with low-level jtag commands (and it is possible, I think) ...

If it would be a hw-testing-only feature, I maintain the opinion that these should not be added to OpenOCD. Test cases for hardware should be developed separately. And if someone wishes to execute them through OpenOCD, the interface to do so is already available (as mentioned in the previous comment).

... but I think that we want to extend existing codebase to simplify control of debugged unit.

I agree as long as there are real use cases for those commands or options (beyond testing).

We have a problem with our hardware: equality_match trigger is supported, but ...

Known issues in existing hardware - that is IMO a good argument to include the feature 👍

[JanMatCodasip]

This looks fine to me overall. I have few last suggestions.

I will not be able to make another review of this since I am out of office until Oct 9. This should not be an issue as this merge request is very close to completion and only minor items are left.

Previous review dismissed.

[timsifive]

I finally read this. General comments:

1. You can set custom triggers simply by writing tselect, tdata1, etc. OpenOCD won't overwrite triggers that are already set. No need to mess around with poll or dmi_read/dmi_write.
2. Work-around for buggy hardware is a good reason for configuration options, assuming they don't make the code too complex.

[timsifive]

The implemented feature applies only to watchpoints, but the documentation and command names don't say anything about that.

Instead of making the command names even more cumbersome, I propose:
riscv set_enable_eq_match_trigger [r][w][x]

This lets a user set those trigger types for any combination of read, write, and execute. It might make the implementation a bit more straightforward even. I think you can simply modify try_setup_single_match_trigger() and try_setup_chained_match_triggers() to return error when a type is passed that doesn't fit with the r/w/x bits that are requested.

[timsifive]

Can you wrap this line? No need to be strictly 80 columns, but 166 is a bit much.
(I know there are a few exceptions to this already, but going forward we're trying to keep the line lengths reasonable.)

[kr-sc]

Addressed

[kr-sc]

The implemented feature applies only to watchpoints, but the documentation and command names don't say anything about that.

When on (default), allow OpenOCD to use exact-match triggers in watchpoints. - description of eq_match_trigger_enable command from doc/openocd.texi

When on, allow OpenOCD to use equality match trigger in wp - help message of eq_match_trigger_enable command

[kr-sc]

Instead of making the command names even more cumbersome, I propose:
riscv set_enable_eq_match_trigger [r][w][x]

As I know, OpenOCD use only equality match trigger for bp. Do we really need command that disables, for example, NAPOT trigger for bp (e mode)?

[kr-sc]

This lets a user set those trigger types for any combination of read, write, and execute. It might make the implementation a bit more straightforward even. I think you can simply modify try_setup_single_match_trigger() and try_setup_chained_match_triggers() to return error when a type is passed that doesn't fit with the r/w/x bits that are requested.

We don't now type of trigger inside of try_setup_single_match_trigger, because we only pass trigger_request_info struct in it, so I think that it's better not to modify this function.

[kr-sc]

Change output of set_ebable_*_trigger comamnds when it is called without args (like in #918 (comment))

[JanMatCodasip]

I have re-checked the code visually and it looks all right.

[timsifive]

We don't know type of trigger inside of try_setup_single_match_trigger, because we only pass trigger_request_info struct in it, so I think that it's better not to modify this function.

The type is encoded in tdata1 of the trigger_request_info, though. It would be trivial to add a line there that checks the type and match fields, and then compares it against whatever the user configured.

Regardless. Since you've already implemented this, I guess it's good enough. In the future, can you open an issue or something when you're planning on adding new commands? That way we can have a little discussion on what commands make the most sense. I think my idea would have been just as much work to implement, and would have left us with a command that is more useful in the future, when OpenOCD implementation changes, or when we encounter hardware with different bugs.

[kr-sc]

In the future, can you open an issue or something when you're planning on adding new commands?

Yes, next time I'll do it.

[aap-sc]

@kr-sc , I'd like to request few additional improvements:

1. enable_ge_lt_trigger should be renamed to wp_enable_ge_lt_trigger (this applies to other controls as well). This is to clearly identify that these controls are for watchpoints only.
2. when trigger->length == 0 we should allow for equality match to be used regardless of this control (otherwise we won't be able to set such wathcpoints)

[aap-sc]

@timsifive , @JanMatCodasip FYI