Skip to content

Add dcsr cetrig control #1255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: riscv
Choose a base branch
from
Open

Add dcsr cetrig control #1255

wants to merge 3 commits into from

Conversation

lz-bro
Copy link
Contributor

@lz-bro lz-bro commented May 13, 2025

Introduce RISC-V-sepecific configure parameter -cetrig

en-sc and others added 2 commits May 7, 2025 09:37
@en-sc @lz-bro
Update image in build32
8d5f2fe 
Ubuntu 20.04 is no longer available.
See actions/runner-images#11101

Checkpatch-ignore: BAD_SIGN_OFF
Change-Id: I0ec3e3342f9212a2a79d8dca6274e7db62ecedab
Signed-off-by: Evgeniy Naydanov <evgeniy.naydanov@syntacore.com>
@berolinux @lz-bro
target/riscv: Regenerate debug_defines.{c,h} from current riscv-debug…
6a30484 
…-spec

This pulls in some improvements from the riscv-debug-spec repo, and
cleans up the licensing (riscv-debug-spec changed the license to dual
CC-BY-4.0 and BSD-2-Clause specifically to make sure the generated
debug_defines files don't conflict with OpenOCD licensing)

Signed-off-by: Bernhard Rosenkränzer <bero@baylibre.com>
@lz-bro lz-bro force-pushed the add-dcsr-cetrig-control branch 3 times, most recently from 6b88574 to 605e820 Compare May 13, 2025 09:33
en-sc
en-sc reviewed May 13, 2025
View reviewed changes
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch!

Overall it looks great, the only concern I have is with the new return from riscv013_halt_reason(). I'm not sure RISCV_HALT_ERROR is the proper return code. I'd suggest adding a new status and handling it differently at the callsite.

Also, how did you test the changes? Is this at all possible to extend riscv-tests to include relevant tests?

src/target/riscv/riscv-013.c
@@ -5367,6 +5368,14 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target)
return RISCV_HALT_INTERRUPT;
case CSR_DCSR_CAUSE_GROUP:
return RISCV_HALT_GROUP;
case CSR_DCSR_CAUSE_OTHER:
if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {
LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");
LOG_TARGET_INFO(target, "halted because of hart in a critical error state.");

I believe INFO is more appropriate here, since this is completely normal from the debugger's point of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry but would like to disagree.

Even though this is not an error of OpenOCD or of the debug transport (JTAG adapter or similar), it is situation that is clearly exceptional and requires user attention. As such, it should be prominently displayed to the user. So a warning is in my opinion warranted here.

Copy link
Collaborator

@en-sc en-sc Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JanMatCodasip, I will insist)

  1. IMHO a similar example will be an assertion failure. GDB does not warn about an assertion failure -- it just notifies you.
  2. To be honest, this seems like an issue with the spec, but: what if the Smdbltrp extension is not implemented? The cause may still be other but this is not an exceptional situation.

@lz-bro lz-bro force-pushed the add-dcsr-cetrig-control branch 2 times, most recently from d1d2b0a to aed2254 Compare May 14, 2025 07:14
@lz-bro
Copy link
Contributor Author

lz-bro commented May 14, 2025

Thank you for the patch!

Overall it looks great, the only concern I have is with the new return from riscv013_halt_reason(). I'm not sure RISCV_HALT_ERROR is the proper return code. I'd suggest adding a new status and handling it differently at the callsite.

Also, how did you test the changes? Is this at all possible to extend riscv-tests to include relevant tests?

  1. Now, I have tested the changes on XiangShan-KunMingHu platform and spike by enabling smdbltrp extension.
    You can find the corresponding ISA extension from the XiangShan Github repository: https://github.com/OpenXiangShan/XiangShan/blob/master/src/main/scala/xiangshan/Parameters.scala
  2. I don't have any good ideas yet to extend riscv-tests to include relevant tests.

JanMatCodasip
JanMatCodasip reviewed May 23, 2025
View reviewed changes
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. I have managed a partial review and am sending my comments so far.

@lz-bro lz-bro force-pushed the add-dcsr-cetrig-control branch 2 times, most recently from d368717 to 31e3406 Compare May 26, 2025 07:03
@JanMatCodasip JanMatCodasip mentioned this pull request Jun 6, 2025
Open
JanMatCodasip
JanMatCodasip reviewed Jun 6, 2025
View reviewed changes
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have few more comments, otherwise the changes look good.

src/target/riscv/riscv-013.c
@@ -5367,6 +5368,14 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target)
return RISCV_HALT_INTERRUPT;
case CSR_DCSR_CAUSE_GROUP:
return RISCV_HALT_GROUP;
case CSR_DCSR_CAUSE_OTHER:
if (get_field(dcsr, CSR_DCSR_EXTCAUSE) == 0) {
LOG_TARGET_WARNING(target, "halted because of hart in a critical error state.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry but would like to disagree.

Even though this is not an error of OpenOCD or of the debug transport (JTAG adapter or similar), it is situation that is clearly exceptional and requires user attention. As such, it should be prominently displayed to the user. So a warning is in my opinion warranted here.

@lz-bro lz-bro force-pushed the add-dcsr-cetrig-control branch from 31e3406 to fb914e0 Compare June 9, 2025 01:37
@lz-bro
target/riscv: add cetrig control
65b6aab 
Introduce RISC-V-sepecific `configure` parameter `-cetrig`
@lz-bro lz-bro force-pushed the add-dcsr-cetrig-control branch from fb914e0 to 65b6aab Compare June 9, 2025 01:44
JanMatCodasip
JanMatCodasip approved these changes Jun 13, 2025
View reviewed changes
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@en-sc en-sc en-sc left review comments

@JanMatCodasip JanMatCodasip JanMatCodasip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lz-bro @JanMatCodasip @en-sc @berolinux