target/riscv: manage triggers available to OpenOCD for internal use #1111

en-sc · 2024-08-07T19:49:37Z

Before the change, if the user wrote to any tdata* register, OpenOCD would sometimes start to disable all the triggers (by writing zeroes to tdata1) and re-enable them again (by witing all trigger registers to the values read before for each tselect value), e.g. on step (see disable/enable_triggers()).

There are a couple of issues with such approach:

RISC-V Debug Specification does not require custom register types to support re-enabling by such sequence of writes (e.g. some custom trigger type may require writing a custom CSR to enable it).
OpenOCD may still overwrite these triggers when a user asks to set a new WP.

This commit introduces riscv reserve_trigger ... command to explicitly mark the triggers OpenOCD should not touch.

Such approach allows to separate management of custom triggers and offload it onto the user (e.g. disable/enable such triggers by setting up an event handler on step-related events).

Change-Id: I3339000445185ab221368442a070f412bf44bfab

After riscv-collab/riscv-openocd#1111 is merged, the registers a user wishes to have direct control of should be reserved. This is the case in `HwbpManual`. The test still works with older OpenOCD versions, since no exception is generated when a command (`riscv reserve_trigger` in this case) is not found.

en-sc · 2024-08-07T19:53:46Z

This one needs a patch to riscv-tests: riscv-software-src/riscv-tests#575

MarekVCodasip

Thanks for the patch, but I do think at least one of the changes needs further discussion.

MarekVCodasip · 2024-08-08T12:09:15Z

src/target/riscv/riscv-013_reg.c

-	if (reg->number == GDB_REGNO_TDATA1 ||
-			reg->number == GDB_REGNO_TDATA2) {
-		r->manual_hwbp_set = true;
-		/* When enumerating triggers, we clear any triggers with DMODE set,
-		 * assuming they were left over from a previous debug session. So make
-		 * sure that is done before a user might be setting their own triggers.
-		 */
-		if (riscv_enumerate_triggers(target) != ERROR_OK)
-			return ERROR_FAIL;
-	}
-


I think we should reserve the trigger (together with an INFO or a WARNING print if it isn't already reserved) if the user touches it via reg_set()

Trough, this raises a question if a watchpoint is set on said trigger, if we should remove it from memory.

What do others think?

I'd like to avoid it.

This will require reading tselect to know which trigger to reserve. IMHO reg command should strive to minimize the number of operations it does.

TBH, I think Trigger Module registers should probably be marked as hidden and should not be accessible to user by default.

What do others think?

My vote is to always print big ugly error message when user tries to access trigger module registers explicitly.

I would go as far as to render trigger module registers as unavailable to the user by default.

Otherwise it may create way too much hassle because of some obscure corner case when a user wants to mess up with the trigger module while having bp/wp set. I don't think any sane person want to do that. If user is crazy enough to mess with this stuff - he can unlock them an do whatever she/he wants (without any baby-sitting from OpenOCD side).

That being said I would recommend to file an issue for this functionality and merge this patch as is, since in my opinion this patch makes things a little bit better compared to what we have now.

I am happy to see the manual_hwbp_set heuristics removed, since it silently changed the behavior of enabling/disabling triggers when the user touched any of the trigger registers.

I think we should reserve the trigger if (...) What do others think?

My preference would be to keep the trigger CSRs normally visible to the user, and don't perform any extra action if the user modifies any of the trigger CSRs. OpenOCD is a low-level debug tool, and as such it gives the user generally a lot of freedom about what it can do, and at the same time it requires cautious use, otherwise things may go awry.

I'd apply the same logic to the trigger manipulation, and not try to make OpenOCD any more "clever" - just do what the user says. :)

src/target/riscv/riscv.c

JanMatCodasip

Overall, I like the reserve_trigger feature. Please, if you can take a look at my comments. Thank you.

doc/openocd.texi

src/target/riscv/riscv.c

doc/openocd.texi

JanMatCodasip · 2024-08-14T14:14:22Z

src/target/riscv/riscv-013_reg.c

-	if (reg->number == GDB_REGNO_TDATA1 ||
-			reg->number == GDB_REGNO_TDATA2) {
-		r->manual_hwbp_set = true;
-		/* When enumerating triggers, we clear any triggers with DMODE set,
-		 * assuming they were left over from a previous debug session. So make
-		 * sure that is done before a user might be setting their own triggers.
-		 */
-		if (riscv_enumerate_triggers(target) != ERROR_OK)
-			return ERROR_FAIL;
-	}
-


I am happy to see the manual_hwbp_set heuristics removed, since it silently changed the behavior of enabling/disabling triggers when the user touched any of the trigger registers.

I think we should reserve the trigger if (...) What do others think?

My preference would be to keep the trigger CSRs normally visible to the user, and don't perform any extra action if the user modifies any of the trigger CSRs. OpenOCD is a low-level debug tool, and as such it gives the user generally a lot of freedom about what it can do, and at the same time it requires cautious use, otherwise things may go awry.

I'd apply the same logic to the trigger manipulation, and not try to make OpenOCD any more "clever" - just do what the user says. :)

JanMatCodasip

Ah, I had few review comments that I thought I submitted but I forgot. I'm sorry and sending them now.

src/target/riscv/riscv.c

doc/openocd.texi

JanMatCodasip

Overall, I like the reserve_triggers feature very much. This merge request looks very close to completion.

When I read the code again, I made some minor comments and suggestions. If you can please look at them. I don't expect to have any further review findings than what I posted so far.

Thank you.

src/target/riscv/riscv.h

src/target/riscv/riscv.c

doc/openocd.texi

src/target/riscv/riscv.c

doc/openocd.texi

src/target/riscv/riscv.c

Before the change, if the user wrote to any `tdata*` register, OpenOCD would sometimes start to disable all the triggers (by writing zeroes to `tdata1`) and re-enable them again (by witing all trigger registers to the values read before for each `tselect` value), e.g. on `step` (see `disable/enable_triggers()`). There are a couple of issues with such approach: 1. RISC-V Debug Specification does not require custom register types to support re-enabling by such sequence of writes (e.g. some custom trigger type may require writing a custom CSR to enable it). 2. OpenOCD may still overwrite these triggers when a user asks to set a new WP. This commit introduces `riscv reserve_trigger ...` command to explicitly mark the triggers OpenOCD should not touch. Such approach allows to separate management of custom triggers and offload it onto the user (e.g. disable/enable such triggers by setting up an event handler on `step`-related events). Change-Id: I3339000445185ab221368442a070f412bf44bfab Signed-off-by: Evgeniy Naydanov <[email protected]>

riscv-collab/riscv-openocd#1111 introduces a change in OpenOCD behavior: a manual trigger should be manually removed to step/resume from it. This was not concidered in previous stop-gap solutions. This commit on the other hand: 1. Determins if `reserve trigger` is supported by the target. 2. Changes `HwbpManual` test's behavior depending on that. 3. Cleans up some minor mistakes in `HwbpManual`

riscv-collab/riscv-openocd#1111 introduces a change in OpenOCD behavior: a manual trigger should be manually removed to step/resume from it. This was not concidered in previous stop-gap solutions (76ff703 and 8cc4918) This commit on the other hand: 1. Determines if `reserve trigger` is supported by the target. 2. Changes `HwbpManual` test's behavior depending on that. 3. Cleans up some minor mistakes in `HwbpManual`

riscv-collab/riscv-openocd#1111 introduces a change in OpenOCD behavior: a manual trigger should be manually removed to step/resume from it. This was not concidered in previous stop-gap solutions (76ff703 and 8cc4918) This commit: 1. Determines if `reserve trigger` is supported by the target. This can be removed once riscv-collab/riscv-openocd#1111 is merged. 2. Marks `HwbpManual` test as not applicable in case `reserve trigger` is not supported. 3. Accounts for the change in OpenOCD's behavior when stepping from a manual BP. 4. Cleans up some minor mistakes in `HwbpManual`

en-sc requested review from JanMatCodasip and MarekVCodasip August 7, 2024 19:49

en-sc self-assigned this Aug 7, 2024

en-sc mentioned this pull request Aug 7, 2024

[debug] Reserve the trigger in HwbpManual riscv-software-src/riscv-tests#575

Merged

MarekVCodasip reviewed Aug 8, 2024

View reviewed changes

JanMatCodasip reviewed Aug 14, 2024

View reviewed changes

en-sc force-pushed the en-sc/ref-reg-manual-hwbp branch from 9a935fc to bb02b5f Compare August 14, 2024 16:57

en-sc requested review from JanMatCodasip and MarekVCodasip August 14, 2024 16:58

JanMatCodasip reviewed Sep 3, 2024

View reviewed changes

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

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

doc/openocd.texi Outdated Show resolved Hide resolved

en-sc force-pushed the en-sc/ref-reg-manual-hwbp branch from bb02b5f to 2ec79ce Compare September 4, 2024 16:35

JanMatCodasip reviewed Sep 5, 2024

View reviewed changes

en-sc force-pushed the en-sc/ref-reg-manual-hwbp branch from 2ec79ce to 5a8697b Compare September 5, 2024 09:59

en-sc requested a review from JanMatCodasip September 5, 2024 10:12

JanMatCodasip approved these changes Sep 5, 2024

View reviewed changes

en-sc mentioned this pull request Sep 6, 2024

[debug] Reserve triggers propperly in HwbpManual riscv-software-src/riscv-tests#581

Merged

en-sc merged commit d58c656 into riscv-collab:riscv Sep 6, 2024
4 checks passed

en-sc deleted the en-sc/ref-reg-manual-hwbp branch September 6, 2024 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/riscv: manage triggers available to OpenOCD for internal use #1111

target/riscv: manage triggers available to OpenOCD for internal use #1111

en-sc commented Aug 7, 2024

en-sc commented Aug 7, 2024

MarekVCodasip left a comment

MarekVCodasip Aug 8, 2024

en-sc Aug 8, 2024

aap-sc Aug 8, 2024

JanMatCodasip Aug 14, 2024 •

edited

Loading

JanMatCodasip left a comment

JanMatCodasip Aug 14, 2024 •

edited

Loading

JanMatCodasip left a comment

JanMatCodasip left a comment •

edited

Loading

target/riscv: manage triggers available to OpenOCD for internal use #1111

target/riscv: manage triggers available to OpenOCD for internal use #1111

Conversation

en-sc commented Aug 7, 2024

en-sc commented Aug 7, 2024

MarekVCodasip left a comment

Choose a reason for hiding this comment

MarekVCodasip Aug 8, 2024

Choose a reason for hiding this comment

en-sc Aug 8, 2024

Choose a reason for hiding this comment

aap-sc Aug 8, 2024

Choose a reason for hiding this comment

JanMatCodasip Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

JanMatCodasip left a comment

Choose a reason for hiding this comment

JanMatCodasip Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

JanMatCodasip left a comment

Choose a reason for hiding this comment

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

JanMatCodasip Aug 14, 2024 •

edited

Loading

JanMatCodasip Aug 14, 2024 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading