Skip to content
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.

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

Add support for Smdbltrp. #998

Merged
merged 5 commits into from
May 1, 2024
Merged

Add support for Smdbltrp. #998

merged 5 commits into from
May 1, 2024

Conversation

rtwfroody
Copy link
Collaborator

@rtwfroody rtwfroody commented Apr 1, 2024

2 changes:

  1. Add extcause field, which gives is room for a double trap cause plus a few others in case of future need.
  2. Add cetrig bit to let a debugger control behavior when a double trap occurs.

We expect this to be approved together with #988, but I want to keep them separate so we can easily have one approved/merged without the other.

@rtwfroody
Copy link
Collaborator Author

We can't merge this until ARC also reviews the change.

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

I went back and forth about whether we should proactively reserve more bits. In the end, I think that this is fine for the foreseeable future and any Debug 1.0 follow-on extensions like double trap or secure debug. Debug 2.0 (which would have a different tinfo.version) can consider the two cause fields to be a unified 6-bit field with 64 encodings rather than the current 7+8 encodings.

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

I just saw your latest change. Why 26:24 rather than left-justifying or right-justifying in that reserved area?

@rtwfroody
Copy link
Collaborator Author

rtwfroody commented Apr 3, 2024 via email

@rtwfroody rtwfroody changed the title Add extcause field. Add support for Smdbltrp. Apr 18, 2024
This will make space for Sddbltrp, and possibly other extensions that
need more causes.
@pdonahue-ventana
Copy link
Collaborator

@ved-rivos: Why bit 19 instead of 18?

@ved-rivos
Copy link
Contributor

@pdonahue-ventana - bit 18 has been allocated for previous ELP - pelp - please see #855

@pdonahue-ventana
Copy link
Collaborator

OK. That's from a while ago so I forgot about it.

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

I previously approved this. But I'll explicitly do it again now that it also contains the extcause change.

more specific halt reason than "other." Otherwise it contains 0.

<value v="0" name="dbltrp">
An exception or interrupt occurred while the trap handler was in
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
An exception or interrupt occurred while the trap handler was in
The hart entered a critical error state. Part of the ((Smdbltrp)) extension.

Reworded to:

  1. Use the critical error term to tie it back to the cetrig
  2. Such an exception/interrupt may not always lead to a critical error (e.g., at S-mode or VS-mode it does not) and in M-mode it does not if it was not the RNMI handler and RNMI was implemented. So this would avoid needing to put all conditions in here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK.

When {dcsr-cause} is 7, this optional field contains the value of a
more specific halt reason than "other." Otherwise it contains 0.

<value v="0" name="dbltrp">
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call it critical error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@ved-rivos ved-rivos left a comment

Choose a reason for hiding this comment

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

Looks good to me. I left a couple of comments.

@ved-rivos
Copy link
Contributor

@rtwfroody Thanks. LGTM.

more specific halt reason than "other." Otherwise it contains 0.

<value v="0" name="critical error">
The hart entered a critical error state. Part of the
Copy link
Contributor

@ved-rivos ved-rivos Apr 23, 2024

Choose a reason for hiding this comment

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

Suggested change
The hart entered a critical error state. Part of the
The hart entered a critical error state, as defined in the ((Smdbltrp)) extension.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

====
When {dcsr-cetrig} is enabled, resuming from Debug Mode
following an entry due to a critical error will result in an
immediate re-entry into Debug Mode due to the critical error.
Copy link
Contributor

Choose a reason for hiding this comment

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

The debugger may resume with cetrig set to 0 to allow the platform defined actions on critical-error signal to occur. Other possible actions include initiating a hart or platform reset using the Debug Module reset control.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@rtwfroody
Copy link
Collaborator Author

Per https://lists.riscv.org/g/tech-chairs/message/1898 this was approved by ARC.

@rtwfroody rtwfroody merged commit 7a39e67 into main May 1, 2024
1 check passed
@rtwfroody rtwfroody deleted the extcause branch May 1, 2024 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants