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 Zicfilp and the hart ELP state #855

Closed
wants to merge 1 commit into from

Conversation

sorear
Copy link

@sorear sorear commented Jul 8, 2023

Zicfilp adds a new ELP state to each hart, which is handled like the privilege mode: it affects all instructions, is not visible in any CSR, but is saved into CSR fields by traps and restored by trap returns.

Add it to the debug spec in the same fashion as the privilege mode, and specify that it does not affect execution in Debug Mode.

(Draft because Zicfilp is not frozen.)

@sorear
Copy link
Author

sorear commented Jul 9, 2023

@ved-rivos

Copy link
Contributor

@timsifive timsifive left a comment

Choose a reason for hiding this comment

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

Overall looks good and thorough.

I wonder if we should rename priv to reflect its more general use now, or if we should leave it to not make any unnecessary waves.

@@ -67,7 +67,7 @@ \section{Execution Based} \label{execution_based}
not executing from the Program Buffer.
Its recommended encoding is 0x7b200073.
When {\tt dret} is executed, \Rpc is restored from \RcsrDpc and normal execution resumes at the
privilege set by \FcsrDcsrPrv and \FcsrDcsrV.
privilege set by \FcsrDcsrPrv and \FcsrDcsrV and the ELP state set by \FcsrDcsrElp.
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
privilege set by \FcsrDcsrPrv and \FcsrDcsrV and the ELP state set by \FcsrDcsrElp.
privilege set by \FcsrDcsrPrv and \FcsrDcsrV, and the ELP state set by \FcsrDcsrElp.

Comment on lines +42 to +55
\begin{table}
\centering
\caption{Expected Landing Pad Encoding}
\label{tab:elpmode}
\begin{tabular}{|c|l|}
\hline
elp & Name \\
\hline
0 & {\tt NO\_LP\_EXPECTED} \\
\hline
1 & {\tt LP\_EXPECTED} \\
\hline
\end{tabular}
\end{table}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer these values inlined in dcsr and priv using the XML value tags. v is a bit different since a single table contains v, prv, and what they mean depending on whether H is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could also just reference Zicfilp for the syntax and semantics of ELP

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a different virtual register instead of being part of the priv virtual register?

Copy link
Author

Choose a reason for hiding this comment

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

If it's literally just a gdb user interface thing then we might as well split out $elp from $priv but I wasn't sure how much of the stack splitting the two would touch and whether the new register might cause problems with debug middleware that doesn't otherwise need changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressing just OpenOCD/gdb, either change would require modifications to OpenOCD but not to gdb. The changes required to OpenOCD are pretty minimal for either option.

@ved-rivos
Copy link
Contributor

@rtwfroody - this PR can be closed without merge as the #1071 that integrates the Zicfilp extension is merged.

@rtwfroody rtwfroody closed this Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants