Skip to content

U-mode control by localparam (critical and secondary)#333

Open
MikeOpenHWGroup wants to merge 1 commit into
openhwgroup:mainfrom
MikeOpenHWGroup:umode
Open

U-mode control by localparam (critical and secondary)#333
MikeOpenHWGroup wants to merge 1 commit into
openhwgroup:mainfrom
MikeOpenHWGroup:umode

Conversation

@MikeOpenHWGroup

Copy link
Copy Markdown
Member

This PR include a number of fixes for CVE2 Issue #332.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses CVE2 Issue #332 by further disabling latent U-mode behavior in the CSR block via a localparam gate, ensuring the core cannot enter U-mode and that U-related CSR behavior is suppressed.

Changes:

  • Added a UmodeEnabled localparam and umode_control signal to centrally gate U-mode-related behavior.
  • Gated mstatus/dcsr WARL behavior and MRET update logic to prevent transitions to PRIV_LVL_U when U-mode is disabled.
  • Updated reset/state defaults and debug trigger matching (tdata0) to reflect U-mode being disabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cairo-caplan cairo-caplan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for the PR, @MikeOpenHWGroup

My only comment for now would be to use only UmodeEnabled instead of also umode_control.

Comment thread rtl/cve2_cs_registers.sv
localparam int unsigned UmodeEnabled = 0;

logic umode_control;
assign umode_control = logic'(UmodeEnabled);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After analyzing all the modifications they are sound, pass the basic UVM tests and SEC fails only for the modified signals i.e.:
cve2_top.u_cve2_core.cs_registers_i.u_mstatus_csr.wr_data_i cve2_top.u_cve2_core.cs_registers_i.u_mstack_csr.rdata_q cve2_top.u_cve2_core.cs_registers_i.u_dcsr_csr.wr_data_i cve2_top.u_cve2_core.cs_registers_i.tmatch_control_rdata.3 cve2_top.u_cve2_core.cs_registers_i.csr_rdata_o

But then, how about using only UmodeEnabled instead of UmodeEnabled and umode_control ?
They both have the same value and should not change at simulation time. To be more clear, UmodeEnabled could be declared as below, and all occurrences of umode_control would be substituted by UmodeEnabled .

localparam logic UmodeEnabled = 1'b0;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I knew that this would break SEC. AFAIK, there is no way to avoid this as we are changing the logic by disabling U-mode. @davideschiavone can you comment?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes total sense. We are breaking SEC because this is a real bug as we were not supposed to support UMODE

@MikeOpenHWGroup

Copy link
Copy Markdown
Member Author

My only comment for now would be to use only UmodeEnabled instead of also umode_control.

I did this intentionally. My reasoning is that the localparams already declared in the module are all unsigned int. I was not sure that this would work for synthesis in all cases, so I also declared umode_control as a single logic bit and assigned its value by casting. @davideschiavone, what do you think about this?

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.

4 participants