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

kr-sc · 2023-09-14T15:17:57Z

Add a new riscv specific commands to disable triggers

src/target/riscv/riscv.c

JanMatCodasip · 2023-09-15T07:52:09Z

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 · 2023-09-18T09:13:13Z

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 · 2023-09-18T13:18:58Z

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 · 2023-09-18T13:51:46Z

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 · 2023-09-19T07:50:25Z

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 👍

src/target/riscv/riscv.c

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.

doc/openocd.texi

src/target/riscv/riscv.c

Previous review dismissed.

timsifive · 2023-09-25T18:48:01Z

I finally read this. General comments:

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.
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 · 2023-09-25T18:51:17Z

doc/openocd.texi

@@ -10871,6 +10871,20 @@ Control dcsr.ebreaku. When on (default), U-mode ebreak instructions trap to
 OpenOCD. When off, they generate a breakpoint exception handled internally.
 @end deffn

+The commands below can be used to prevent OpenOCD from using certain RISC-V trigger features, for example in cases when there are known issues in the target hardware.


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.)

src/target/riscv/riscv.c

kr-sc · 2023-09-26T08:02:27Z

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 · 2023-09-26T08:06:39Z

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 · 2023-09-26T08:10:19Z

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 · 2023-09-26T09:09:50Z

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

…for watchpoints Add a new riscv specific commands to disable triggers Change-Id: Ic1842085aa66851c740e0abcbfbe0adbe930920e Signed-off-by: Kirill Radkin <[email protected]>

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 · 2023-10-11T08:48:29Z

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 · 2023-10-18T20:15:30Z

src/target/riscv/riscv.c

-			.tdata1_ignore_mask = fields.tdata1_ignore_mask
-		};
-		struct trigger_request_info ge_2 = {
+		if (r->enable_ge_lt_trigger) {


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

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.

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)

@timsifive , @JanMatCodasip FYI

JanMatCodasip reviewed Sep 15, 2023

View reviewed changes

src/target/riscv/riscv.c Outdated Show resolved Hide resolved

kr-sc force-pushed the kr-sc/disable-triggers-option branch 2 times, most recently from a541283 to 27839b1 Compare September 19, 2023 11:50

JanMatCodasip previously requested changes Sep 20, 2023

View reviewed changes

kr-sc force-pushed the kr-sc/disable-triggers-option branch from 27839b1 to fe16177 Compare September 20, 2023 08:26

JanMatCodasip reviewed Sep 21, 2023

View reviewed changes

doc/openocd.texi Outdated Show resolved Hide resolved

src/target/riscv/riscv.c Outdated Show resolved Hide resolved

JanMatCodasip changed the title ~~provide riscv-specific controls to disable triggers from beeing used for watchpoints~~ provide riscv-specific controls to disable triggers from being used for watchpoints Sep 21, 2023

kr-sc force-pushed the kr-sc/disable-triggers-option branch from fe16177 to fb9486a Compare September 25, 2023 10:22

timsifive requested changes Sep 25, 2023

View reviewed changes

kr-sc force-pushed the kr-sc/disable-triggers-option branch 2 times, most recently from 63547f4 to 4c12f4a Compare September 26, 2023 08:42

provide riscv-specific controls to disable triggers from beeing used …

e76a9b7

…for watchpoints Add a new riscv specific commands to disable triggers Change-Id: Ic1842085aa66851c740e0abcbfbe0adbe930920e Signed-off-by: Kirill Radkin <[email protected]>

kr-sc force-pushed the kr-sc/disable-triggers-option branch from 4c12f4a to e76a9b7 Compare October 2, 2023 08:54

JanMatCodasip approved these changes Oct 5, 2023

View reviewed changes

timsifive approved these changes Oct 10, 2023

View reviewed changes

timsifive merged commit beb7059 into riscv-collab:riscv Oct 11, 2023
5 checks passed

aap-sc reviewed Oct 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kr-sc commented Sep 14, 2023 •

edited

Loading

JanMatCodasip commented Sep 15, 2023

kr-sc commented Sep 18, 2023

JanMatCodasip commented Sep 18, 2023

kr-sc commented Sep 18, 2023 •

edited

Loading

JanMatCodasip commented Sep 19, 2023 •

edited

Loading

JanMatCodasip left a comment

timsifive commented Sep 25, 2023

timsifive left a comment

timsifive Sep 25, 2023

kr-sc Sep 26, 2023

kr-sc commented Sep 26, 2023

kr-sc commented Sep 26, 2023 •

edited

Loading

kr-sc commented Sep 26, 2023 •

edited

Loading

kr-sc commented Sep 26, 2023

JanMatCodasip left a comment

timsifive left a comment

kr-sc commented Oct 11, 2023

aap-sc Oct 18, 2023 •

edited

Loading

aap-sc Oct 18, 2023

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

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

Conversation

kr-sc commented Sep 14, 2023 • edited Loading

JanMatCodasip commented Sep 15, 2023

kr-sc commented Sep 18, 2023

JanMatCodasip commented Sep 18, 2023

kr-sc commented Sep 18, 2023 • edited Loading

JanMatCodasip commented Sep 19, 2023 • edited Loading

JanMatCodasip left a comment

Choose a reason for hiding this comment

timsifive commented Sep 25, 2023

timsifive left a comment

Choose a reason for hiding this comment

timsifive Sep 25, 2023

Choose a reason for hiding this comment

kr-sc Sep 26, 2023

Choose a reason for hiding this comment

kr-sc commented Sep 26, 2023

kr-sc commented Sep 26, 2023 • edited Loading

kr-sc commented Sep 26, 2023 • edited Loading

kr-sc commented Sep 26, 2023

JanMatCodasip left a comment

Choose a reason for hiding this comment

timsifive left a comment

Choose a reason for hiding this comment

kr-sc commented Oct 11, 2023

aap-sc Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

aap-sc Oct 18, 2023

Choose a reason for hiding this comment

kr-sc commented Sep 14, 2023 •

edited

Loading

kr-sc commented Sep 18, 2023 •

edited

Loading

JanMatCodasip commented Sep 19, 2023 •

edited

Loading

kr-sc commented Sep 26, 2023 •

edited

Loading

kr-sc commented Sep 26, 2023 •

edited

Loading

aap-sc Oct 18, 2023 •

edited

Loading