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

- Update the trace description by removing the interface signal details #53

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

AoteJin
Copy link
Collaborator

@AoteJin AoteJin commented Aug 6, 2024

Per issue #48

chapter2.adoc Outdated

[mtrcctl]
==== M-Mode Trace Control
For each hart i, an input port, mtrcen[i], controls M-mode trace availability. Setting mtrcen[i] to 1 enables M-mode and supervisor domain trace by clearing the sec_check[i] signal to 0 across all privilege levels. Conversely, if mtrcen[i] is set to 0, the sec_check[i] signal cannot be cleared when the hart i runs in M-mode.
For each hart i, the input port mtrcen[i] controls the availability M-mode tracing. Setting mtrcen[i] to 1 enables trace for both M-mode and the supervisor domain. Conversely, setting mtrcen[i] to 0 disables trace output when hart i is running in M-mode.

Choose a reason for hiding this comment

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

mtrcen is another signal. I thought the intent was to remove references to signals and focus on the behavioural requirements (hence removing references to sec_check). How is mtrcen supposed to be controlled? This is what should be described in my view.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spec defines the mtrcen to indicate that the M-mode trace capability is granted by higher-privileged entity than M-mode SW (e.g. a fuse or BROM to toggle the signal and lock it). To avoid ambiguity, we explicitly name the signal. I will add a note to describe how it is controlled, as you suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good point by Iain. Is there a requirement that the hart implements this mtrcen signal interface, or does there just need to be a way to disable tracing in M-mode? When I look at the RV debug spec, it never specifies a haltreq signal from the DM to the hart, it could instead be transported by a bus request, or something else. Those are implementation details. In this case, mtrcen could be established via a handshake between the hart and the RoT, rather than via a wire routed to each hart, right? Obviously all of this would apply to mdbgen too.

Also, there's a typo:

controls the availability of M-mode tracing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, the intention here is not to define an interface, but rather to explicitly highlight that the debug/trace capability of M-mode is provided by the RoT. I agree that this could potentially be broadcasted via a protocol, depending on the implementation. The key point is to convey this idea in a straightforward manner, similar to how MEIP/SEIP interrupts are described. By using terms like "signal" or "port," the reader can easily visualize the concept.
What do you think about adding a non-normative description to explain how this is controlled and how it might be implemented differently up to the HW choice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest replacing line 99 with something like:

Each hart will add a new state element, mtrcen, which controls the availability M-mode tracing. mtrcen may be controlled via a new input port to the hart, or by handshake with the system Root of Trust, or by other methods. Setting mtrcen to 1 enables trace for both M-mode and the supervisor domain. Conversely, setting mtrcen to 0 disables trace output when hart i is running in M-mode.

The same could be done for mdbgen. This allows using these m*en names to simplify references to the enables, without appearing to dictate the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I updated it accordingly in latest commit below.

chapter2.adoc Outdated

[mtrcctl]
==== M-Mode Trace Control
For each hart i, an input port, mtrcen[i], controls M-mode trace availability. Setting mtrcen[i] to 1 enables M-mode and supervisor domain trace by clearing the sec_check[i] signal to 0 across all privilege levels. Conversely, if mtrcen[i] is set to 0, the sec_check[i] signal cannot be cleared when the hart i runs in M-mode.
Each hart must add a new state element, `mtrcen`, which controls the availability of M-mode tracing. Setting `mtrcen` to 1 enables trace for both M-mode and the supervisor domain. Conversely, setting `mtrcen` to 0 disables trace output when hart i is running in M-mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

One nit (a mistake in my suggested text), I would replace "hart i is running" with "the hart is". And the "Conversely, " seems unnecessary. Otherwise all looks good!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. Updated and merging

@AoteJin AoteJin merged commit a4cd020 into main Aug 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants