CSRH Implementation #1568

AnonymousVikram · 2025-11-20T19:37:44Z

Cleaned up version of the PR

…nto mux, improve other csrh handling

Copilot

Pull request overview

This PR implements the CSRH (Hypervisor CSR) module as part of adding support for the RISC-V Hypervisor Extension. The implementation adds a new csrh.sv module to handle hypervisor-specific control and status registers, along with virtualization mode tracking in privmode.sv and trap routing logic in trap.sv.

Key changes:

New csrh.sv module implementing all hypervisor CSRs (hstatus, hedeleg, hideleg, hie, hvip, hepc, hcause, htval, htinst, hgatp, etc.)
Virtualization mode (V-bit) state tracking in privmode.sv with proper state transitions on traps and returns
Trap target routing logic (TrapToM, TrapToHS, TrapToVS) added to trap.sv for determining trap destinations

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/privileged/csrh.sv	New module implementing all hypervisor CSRs with read/write logic, trap handling, and access control
src/privileged/trap.sv	Added trap target outputs (TrapToM, TrapToHS, TrapToVS) to support hypervisor trap delegation
src/privileged/privmode.sv	Added virtualization mode (V-bit) tracking with state machine for trap and return handling
src/privileged/privileged.sv	Added hypervisor signal declarations and updated module instantiations with new ports
src/privileged/csr.sv	Added CSRHWriteM, HSTrapM, PrivReturnHSM signals and conditional csrh module instantiation
src/cvw.sv	Added H_SUPPORTED parameter to cvw_t configuration struct
config/shared/parameter-defs.vh	Added H_SUPPORTED parameter to configuration parameter list
config/rv64i/config.vh	Added H_SUPPORTED parameter (set to 0)
config/rv64gc/config.vh	Added H_SUPPORTED parameter (set to 0)
config/rv32imc/config.vh	Added H_SUPPORTED parameter (set to 0)
config/rv32i/config.vh	Added H_SUPPORTED parameter (set to 0)
config/rv32gc/config.vh	Added H_SUPPORTED parameter (set to 0)
config/rv32e/config.vh	Added H_SUPPORTED parameter (set to 0)

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

Copilot · 2025-12-09T22:26:18Z

src/privileged/csrh.sv

+
+  // HCAUSE: Written by CSR instructions and by hardware on traps
+  assign NextHCAUSE = HSTrapM ? {{(P.XLEN-5){1'b0}}, NextCauseM} : CSRWriteValM;
+  flopenr #(P.XLEN) HCAUSEreg (clk, reset, (WriteHCAUSSEM | HSTrapM), NextHCAUSE, HCAUSE_REGW);


The signal name WriteHCAUSSEM is inconsistent with the naming pattern. It should be WriteHCAUSEM to match the register name HCAUSE.

Suggested change

flopenr #(P.XLEN) HCAUSEreg (clk, reset, (WriteHCAUSSEM | HSTrapM), NextHCAUSE, HCAUSE_REGW);

flopenr #(P.XLEN) HCAUSEreg (clk, reset, (WriteHCAUSEM | HSTrapM), NextHCAUSE, HCAUSE_REGW);

Copilot · 2025-12-09T22:26:18Z

src/privileged/csrh.sv

+
+    if (~LegalHAccess) begin : illegalaccess
+      IllegalCSRHAccessM = 1'b1;
+    end else begin : legalacess_mux


Typo in block label: "legalacess_mux" should be "legalaccess_mux" (missing 'c').

Suggested change

end else begin : legalacess_mux

end else begin : legalaccess_mux

Copilot · 2025-12-09T22:26:19Z

src/privileged/csr.sv

  assign CSRUWriteM = CSRWriteM  & InstrValidNotFlushedM;
  assign MTrapM = TrapM & (NextPrivilegeModeM == P.M_MODE);
  assign STrapM = TrapM & (NextPrivilegeModeM == P.S_MODE) & P.S_SUPPORTED;
+  assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;


Inconsistent use of logical NOT (!VirtModeW) versus bitwise NOT (~VirtModeW on line 212). Since VirtModeW is a single bit, both work, but for consistency with line 212 and line 217, consider using ~VirtModeW.

Suggested change

assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;

assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & ~VirtModeW) & P.H_SUPPORTED;

Copilot · 2025-12-09T22:26:19Z

src/privileged/csr.sv

+  assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;
+  assign PrivReturnHSM = sretM & (PrivilegeModeW == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;


Inconsistent use of logical NOT (!VirtModeW) versus bitwise NOT (~VirtModeW on line 212). Since VirtModeW is a single bit, both work, but for consistency with line 212, consider using ~VirtModeW.

Suggested change

assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;

assign PrivReturnHSM = sretM & (PrivilegeModeW == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;

assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & ~VirtModeW) & P.H_SUPPORTED;

assign PrivReturnHSM = sretM & (PrivilegeModeW == P.S_MODE & ~VirtModeW) & P.H_SUPPORTED;

Copilot · 2025-12-09T22:26:19Z

src/privileged/trap.sv


+  assign TrapToVS = 1'b0; // until hedeleg/hideleg are implemented
+  assign TrapToHS = DelegateM;
+  assign TrapToM  = TrapM & ~TrapToHS; // and not VS


The comment says "and not VS" but the logic only excludes TrapToHS. For future completeness when TrapToVS is implemented, this should be TrapM & ~TrapToHS & ~TrapToVS. Since TrapToVS is currently hardcoded to 1'b0, the current logic works but will need updating when hedeleg/hideleg are implemented.

Suggested change

assign TrapToM = TrapM & ~TrapToHS; // and not VS

assign TrapToM = TrapM & ~TrapToHS & ~TrapToVS; // and not VS

Copilot · 2025-12-09T22:26:20Z

src/privileged/csrh.sv

+  logic WriteHIEM, WriteHTIMEDELTAM, WriteHCOUNTERENM;
+  logic WriteHGEIEM, WriteHENVCFGM, WriteHENVCFGHM, WriteHTVALM;
+  logic WriteHVIPM, WriteHTINSTM, WriteHGATPM;
+  logic WriteHEPCM, WriteHCAUSSEM;


The signal name WriteHCAUSSEM is inconsistent with the naming pattern used for other CSRs. It should be WriteHCAUSEM to match the register name HCAUSE (following the pattern: WriteHEPCM for HEPC, WriteHSTATUSM for HSTATUS, etc.).

Suggested change

logic WriteHEPCM, WriteHCAUSSEM;

logic WriteHEPCM, WriteHCAUSEM;

Copilot · 2025-12-09T22:26:20Z

src/privileged/csrh.sv

+  // henvcfgh only exists and is writable for RV32
+  assign WriteHENVCFGHM   = (P.XLEN == 32) & (ValidWrite & (CSRAdrM == HENVCFGH));
+  assign WriteHEPCM       = ValidWrite & (CSRAdrM == HEPC);
+  assign WriteHCAUSSEM    = ValidWrite & (CSRAdrM == HCAUSE);


The signal name WriteHCAUSSEM is inconsistent with the naming pattern. It should be WriteHCAUSEM to match the register name HCAUSE.

Suggested change

assign WriteHCAUSSEM = ValidWrite & (CSRAdrM == HCAUSE);

assign WriteHCAUSEM = ValidWrite & (CSRAdrM == HCAUSE);

davidharrishmc · 2025-12-10T11:08:30Z

Aren't you putting this work into a hypervisor branch for now, instead of main?
Did you run regression and did this break anything?
The Copilot suggestions are good.
Eventually you will need virtual-instruction exceptions on some of these accesses. Ok to defer that.

davidharrishmc · 2025-12-10T11:06:18Z

src/privileged/privmode.sv

+      end else if (mretM) begin
+        NextVirtModeM = MSTATUS_MPV;
+      end else if (sretM) begin
+        NextVirtModeM = (VirtModeW == 1'b0) ? HSTATUS_SPV : 1'b1;


Rewrite as HSTATUS_SPV | VirtModeW

davidharrishmc · 2025-12-10T11:13:56Z

src/privileged/csrh.sv

+  input  logic              CSRHWriteM,       // High if operation is a write
+  input  logic [11:0]       CSRAdrM,
+  input  logic [P.XLEN-1:0] CSRWriteValM,
+  input  logic [1:0]        PrivilegeModeW,   // Current privilege mode (U, S, M)


Keep indenting of comments clean now that our code is pretty mature.

davidharrishmc · 2025-12-10T11:14:27Z

src/privileged/csrh.sv

+  input  logic              PrivReturnHSM,   // Privilege return (sret) from HS-mode
+  input  logic [P.XLEN-1:0] NextEPCM,         // Value for hepc on trap
+  input  logic [4:0] NextCauseM,      // Value for hcause on trap
+  input  logic [P.XLEN-1:0] NextMtvalM,       // Value for htval on trap


name is Mtval but comment is htval

davidharrishmc · 2025-12-10T11:15:47Z

src/privileged/csrh.sv

+  logic WriteHVIPM, WriteHTINSTM, WriteHGATPM;
+  logic WriteHEPCM, WriteHCAUSSEM;
+
+  // Internal register for htimedelta


Put the comment on same line with the signal declaration and delete white space to improve density.

davidharrishmc · 2025-12-10T11:17:13Z

src/privileged/csrh.sv

+  // H-CSRs are accessible in M-Mode or HS-Mode.
+  // HS-Mode is S-Mode when VirtModeW is 0.
+  // Access is ILLEGAL in U-Mode (U/VU) and VS-Mode (S-Mode when VirtModeW=1).
+  assign LegalHAccess = (PrivilegeModeW == P.M_MODE) |


Add pipeline stage suffixes to every signal.

davidharrishmc · 2025-12-10T11:32:37Z

src/privileged/csrh.sv

+  // Configuration & Timers
+  flopenr #(P.XLEN) HCOUNTERENreg(clk, reset, WriteHCOUNTERENM, CSRWriteValM, HCOUNTEREN_REGW);
+  flopenr #(P.XLEN) HENVCFGreg(clk, reset, WriteHENVCFGM, CSRWriteValM, HENVCFG_REGW);
+  flopenr #(P.XLEN) HENVCFGHreg(clk, reset, WriteHENVCFGHM, CSRWriteValM, HENVCFGH_REGW);


davidharrishmc · 2025-12-10T11:35:19Z

src/privileged/csrh.sv

+        HCOUNTEREN: CSRHReadValM = HCOUNTEREN_REGW;
+        HGEIE:      CSRHReadValM = HGEIE_REGW;
+        HENVCFG:    CSRHReadValM = HENVCFG_REGW;
+        HENVCFGH:   CSRHReadValM = HENVCFGH_REGW; // Only exists for RV32, reads 0 for RV64


See other CSR modules for how to handle h registers with RV32. Need to throw illegal instruction trap instead of reading 0 on RV64. Applies to all h registers.

davidharrishmc · 2025-12-10T11:37:08Z

src/privileged/privmode.sv

+      end
+    end
+
+    flopenl #(1) virtmodereg(clk, reset, ~StallW, NextVirtModeM, 1'b0, VirtModeW);


Can this be a flopenr?

davidharrishmc · 2025-12-10T11:37:42Z

src/privileged/privmode.sv

+    always_comb begin
+      NextVirtModeM = VirtModeW;
+      if (TrapM) begin
+        if (TrapToVS)  NextVirtModeM = 1'b1;


Replace two lines with NextVitModeM = TraptoVS;

davidharrishmc · 2025-12-10T11:38:38Z

src/privileged/privmode.sv

+  if (P.H_SUPPORTED) begin:virtmode
+    always_comb begin
+      NextVirtModeM = VirtModeW;
+      if (TrapM) begin


Remove these begin/end s and simplify the code structure.

resolved prof. harris comments

TnnsBeast and others added 7 commits November 20, 2025 11:28

Add v bit, add csrh

52a6b5d

Big cleanup, fix NextCauseM length, pass lint?

8536325

Newline to pass lint?

193ba30

Pass lint (kind of), probably use this as the new base

2aaa25c

Remove csrh temporarily

2ba8fad

lint passing with initial csrh

833ec09

Change order of virtmode signals, syntax, pull ? signals from flops i…

2038b3b

…nto mux, improve other csrh handling

AnonymousVikram force-pushed the main branch from 9aafc30 to 2038b3b Compare November 20, 2025 20:44

AnonymousVikram marked this pull request as ready for review December 9, 2025 22:14

Copilot AI review requested due to automatic review settings December 9, 2025 22:14

Copilot started reviewing on behalf of AnonymousVikram December 9, 2025 22:15 View session

AnonymousVikram changed the title ~~[WIP] CSRH Implementation~~ CSRH Implementation Dec 9, 2025

Copilot AI reviewed Dec 9, 2025

View reviewed changes

davidharrishmc reviewed Dec 10, 2025

View reviewed changes

juliacygong added 2 commits December 10, 2025 11:51

resolved prof. harris comments

15ddb03

Merge pull request #2 from juliacygong/h2

46e1e29

resolved prof. harris comments

	flopenr #(P.XLEN) HCAUSEreg (clk, reset, (WriteHCAUSSEM \| HSTrapM), NextHCAUSE, HCAUSE_REGW);
	flopenr #(P.XLEN) HCAUSEreg (clk, reset, (WriteHCAUSEM \| HSTrapM), NextHCAUSE, HCAUSE_REGW);

	end else begin : legalacess_mux
	end else begin : legalaccess_mux

	assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;
	assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & ~VirtModeW) & P.H_SUPPORTED;

		assign HSTrapM = TrapM & (NextPrivilegeModeM == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;
		assign PrivReturnHSM = sretM & (PrivilegeModeW == P.S_MODE & !VirtModeW) & P.H_SUPPORTED;

	assign TrapToM = TrapM & ~TrapToHS; // and not VS
	assign TrapToM = TrapM & ~TrapToHS & ~TrapToVS; // and not VS

	logic WriteHEPCM, WriteHCAUSSEM;
	logic WriteHEPCM, WriteHCAUSEM;

	assign WriteHCAUSSEM = ValidWrite & (CSRAdrM == HCAUSE);
	assign WriteHCAUSEM = ValidWrite & (CSRAdrM == HCAUSE);

CSRH Implementation #1568

Are you sure you want to change the base?

CSRH Implementation #1568

Conversation

AnonymousVikram commented Nov 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

davidharrishmc commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidharrishmc commented Dec 10, 2025 •

edited

Loading