Fix: Prevent CSR access if illegal CSR instruction occurs

dpretet · Oct 24, 2023 · 8c12593 · 8c12593
1 parent 95ea57b
commit 8c12593
Show file tree

Hide file tree

Showing 15 changed files with 124 additions and 87 deletions.
diff --git a/doc/TODO.md b/doc/TODO.md
@@ -1,11 +1,4 @@
-# User Mode
-
-- write tests for interrupts
-    - enhance existing, existing tests are very poor
-    - add test for vector table
-    - Test des exception load/store misaligned
-- add FIFO for memory exceptions
-    - avoid to loose a trap if highest interrupt trap occurs
+# User Mode Design
 
 - [X] Support U-mode:
     - Previous privilege mode interrupt is stored in xPP to support nested trap
@@ -23,29 +16,32 @@
     - PMP checks are applied to all accesses whose effective privilege mode is S or U, including
       instruction fetches and data accesses in S and U mode, and data accesses in M-mode when the
       MPRV bit in mstatus is set and the MPP field in mstatus contains S or U (page 56 & page 23)
-- [ ] Study PMA (Physical Memory Attribute) (section 3.6)
+- [X] Study PMA (Physical Memory Attribute) (section 3.6)
     - define R/W/X et l'address matching
     - le PMA ne permet pas de définir des zones d'IO et/ou si une region peut etre cohérente
-- [ ] Support cycle registers per mode
-- [ ] WFI:
-    - [ ] if MIE/SIE=1, wait for one of them and trap to m-mode. Resume to mepc=pc+4
-    - [ ] if MIE/SIE=0, wait for any intp and move forward
-    - [ ] Support MSTATUS.TW (timeout platform-dependant)
+- [X] WFI:
+    - [X] if MIE/SIE=1, wait for one of them and trap to m-mode. Resume to mepc=pc+4
+    - [X] if MIE/SIE=0, wait for any intp and move forward
+    - [X] Support MSTATUS.TW (timeout platform-dependent)
+- [X] add FIFO for memory exceptions
 - [ ] mcounteren: accessibility to lower privilege modes
-- [ ] mcountinhibit: stop a specific counter
-- [ ] Machine Environment Configuration Registers (menvcfg and menvcfgh)
+    - Bit x = 1, lower privilege mode can read the counter
+    - Bit x = 0, lower privilege mode access is forbidden and raise an illegal instruction exception
 
-TESTS
+# Testcases
 
 U-mode
-- pass from/to m-mode/u-mode
-- try mret in u-mode, needs to fail
-- try to access m-mode only CSRs
+- [X] pass from/to m-mode/u-mode
+- [X] try mret in u-mode, needs to fail
+- [X] try to access m-mode only CSRs
 
 Interrupt
-- do something wothin a loop with interrupt enabled, data needs to be OK
-- WFI in u-mode, interrupt enabled, trapped in m-mode
-- WFI in u-mode, interrupt disabled, NOP
+- [X] Do something within a loop with interrupt enabled, data needs to be OK
+- [ ] WFI in u-mode, interrupt enabled, trapped in m-mode
+- [ ] WFI in u-mode, interrupt disabled, NOP
+- [ ] Add test for vector table
+- [ ] Test des exception load/store misaligned
+- [ ] test MSTATUS.TW
 
 MPU:
 - [X] configure registers
@@ -60,7 +56,7 @@ MPU:
     - [ ] read/write data in M-mode with MPRV + MPP w/ U-mode
 - [ ] locked access to change configuration
 - [ ] locked region accessed wrongly by m-mode- pass xIE & xIP to the control
-- [ ] test MSTATUS.TW
 
 - Pass compliance with U-mode
-- Re-parse the documentation
+- Review testcases
+- Parse again the documentation
diff --git a/doc/project_mgt_hw.md b/doc/project_mgt_hw.md
@@ -60,6 +60,8 @@ Any new features should be carefully study to ensure a proper exception and inte
 
 ## Miscellanous
 
+- [ ] mcountinhibit: stop a specific counter
+- [ ] Machine Environment Configuration Registers (menvcfg and menvcfgh)
 - [ ] Machine Configuration Pointer Register (mconfigptr)
 - [ ] Create a HW test platform
     - [ ] Analogue pocket

diff --git a/rtl/friscv_control.sv b/rtl/friscv_control.sv
@@ -190,6 +190,7 @@ module friscv_control
     logic [XLEN       -1:0] sb_mepc;
     logic [XLEN       -1:0] sb_mtvec;
     logic [XLEN       -1:0] sb_mstatus;
+    logic [XLEN       -1:0] sb_mcounteren;
     logic                   sb_mie;
     logic                   sb_mtip;
     logic                   sb_msip;
@@ -218,6 +219,7 @@ module friscv_control
     logic                   store_misaligned;
     logic                   inst_access_fault;
     logic                   illegal_instruction;
+    logic                   illegal_csr;
     logic                   wfi_tw;
     logic                   trap_occuring;
     logic                   sync_trap_occuring;
@@ -293,9 +295,9 @@ module friscv_control
         else if (cause=='hB)  get_mcause_desc = "Environment call (M-mode)";
         else if (cause=='h2)  get_mcause_desc = "Illegal instruction";
         // Asynchronous Trap
-        else if (cause=='h80000003)  get_mcause_desc = "Machine Software Interrupt";
-        else if (cause=='h80000007)  get_mcause_desc = "Machine Timer Interrupt";
-        else if (cause=='h8000000B)  get_mcause_desc = "Machine External Interrupt";
+        else if (cause=='h80000003) get_mcause_desc = "Machine Software Interrupt";
+        else if (cause=='h80000007) get_mcause_desc = "Machine Timer Interrupt";
+        else if (cause=='h8000000B) get_mcause_desc = "Machine External Interrupt";
         // All other unknown interrupts
         else get_mcause_desc = "Unknown Trap Cause";
     endfunction
@@ -339,16 +341,17 @@ module friscv_control
     // CSR Shared bus extraction
     ///////////////////////////////////////////////////////////////////////////
 
-    assign sb_mtvec   = csr_sb[`CSR_SB_MTVEC   +: XLEN];
-    assign sb_mstatus = csr_sb[`CSR_SB_MSTATUS +: XLEN];
-    assign sb_mepc    = csr_sb[`CSR_SB_MEPC    +: XLEN];
-    assign sb_mie     = csr_sb[`CSR_SB_MIE];
-    assign sb_meip    = csr_sb[`CSR_SB_MEIP];
-    assign sb_mtip    = csr_sb[`CSR_SB_MTIP];
-    assign sb_msip    = csr_sb[`CSR_SB_MSIP];
-    assign sb_meie    = csr_sb[`CSR_SB_MEIE];
-    assign sb_mtie    = csr_sb[`CSR_SB_MTIE];
-    assign sb_msie    = csr_sb[`CSR_SB_MSIE];
+    assign sb_mtvec      = csr_sb[`CSR_SB_MTVEC   +: XLEN];
+    assign sb_mstatus    = csr_sb[`CSR_SB_MSTATUS +: XLEN];
+    assign sb_mepc       = csr_sb[`CSR_SB_MEPC    +: XLEN];
+    assign sb_mcounteren = csr_sb[`CSR_SB_MCOUNTEREN +: XLEN];
+    assign sb_mie        = csr_sb[`CSR_SB_MIE];
+    assign sb_meip       = csr_sb[`CSR_SB_MEIP];
+    assign sb_mtip       = csr_sb[`CSR_SB_MTIP];
+    assign sb_msip       = csr_sb[`CSR_SB_MSIP];
+    assign sb_meie       = csr_sb[`CSR_SB_MEIE];
+    assign sb_mtie       = csr_sb[`CSR_SB_MTIE];
+    assign sb_msie       = csr_sb[`CSR_SB_MSIE];
 
     assign ctrl_sb = {instret, clr_meip,
                       mtval_wr, mtval,
@@ -482,7 +485,7 @@ module friscv_control
 
     assign proc_valid = inst_ready & processing & (cfsm==FETCH) & csr_ready & !trap_occuring;
 
-    assign csr_en = inst_ready && sys[`IS_CSR] & (cfsm==FETCH) & !proc_busy;
+    assign csr_en = inst_ready && sys[`IS_CSR] & (cfsm==FETCH) & !proc_busy & !illegal_csr;
 
     assign proc_instbus[`OPCODE   +: `OPCODE_W ] = opcode;
     assign proc_instbus[`FUNCT3   +: `FUNCT3_W ] = funct3;
@@ -1268,13 +1271,24 @@ module friscv_control
     // User mode is trying to execute an instruction reserved for M-mode
     generate
     if (USER_MODE) begin: UMODE_ILLEGAL_INST
-        assign illegal_instruction = (priv_mode==`MMODE)                 ? '0         :
-                                     (sys[`IS_MRET])                     ? inst_ready :
-                                     (sys[`IS_CSR] && csr[9:8] != 2'b00) ? inst_ready :
-                                     (wfi_tw)                            ? 1'b1       :
-                                                                           '0;
+
+        assign illegal_instruction = (priv_mode==`MMODE)  ? '0         :
+                                     (sys[`IS_MRET])      ? inst_ready :
+                                     (illegal_csr)        ? inst_ready :
+                                     (wfi_tw)             ? 1'b1       :
+                                                            '0         ;
+
+        assign illegal_csr = (priv_mode==`MMODE || !sys[`IS_CSR])    ? '0   :
+                             (csr[11:0]=='hC00 && !sb_mcounteren[0]) ? 1'b1 : // Cycle  
+                             (csr[11:0]=='hC01 && !sb_mcounteren[1]) ? 1'b1 : // Time  
+                             (csr[11:0]=='hC02 && !sb_mcounteren[2]) ? 1'b1 : // Instret  
+                             (csr[11:4]=='hFC)                       ? 1'b1 : // Custom perf. registers  
+                             (csr[ 9:8]!=2'b00)                      ? 1'b1 : // M-Mode only registers 
+                                                                       1'b0 ;
+
     end else begin : NO_UMODE
         assign illegal_instruction = '0;
+        assign illegal_csr = '0;
     end
     endgenerate
 

diff --git a/rtl/friscv_csr.sv b/rtl/friscv_csr.sv
@@ -566,6 +566,7 @@ module friscv_csr
         else if (csr==RDTIMEH)         oldval = rdtime[32+:32];
         else if (csr==RDINSTRETH)      oldval = rdinstret[32+:32];
         else if (csr==MHART_ID)        oldval = mhartid;
+        else if (csr==MCOUNTEREN)      oldval = mcounteren;
         else if (csr==INSTREQ_ACTIVE)  oldval = instreq_perf_active;
         else if (csr==INSTREQ_SLEEP)   oldval = instreq_perf_sleep;
         else if (csr==INSTREQ_STALL)   oldval = instreq_perf_stall;
@@ -837,6 +838,23 @@ module friscv_csr
         end
     end
 
+    ///////////////////////////////////////////////////////////////////////////
+    // MCOUNTEREN - 0x306
+    ///////////////////////////////////////////////////////////////////////////
+    always @ (posedge aclk or negedge aresetn) begin
+        if (!aresetn) begin
+            mcounteren <= {XLEN{1'b0}};
+        end else if (srst) begin
+            mcounteren <= {XLEN{1'b0}};
+        end else begin
+            if (csr_wren) begin
+                if (csr==MCOUNTEREN) begin
+                    mcounteren <= newval;
+                end
+            end
+        end
+    end
+
     ///////////////////////////////////////////////////////////////////////////
     // PMPCFG0/3 - 0x3A0-0x3A3
     ///////////////////////////////////////////////////////////////////////////
@@ -989,6 +1007,7 @@ module friscv_csr
     assign csr_sb[`CSR_SB_MTVEC+:XLEN] = mtvec;
     assign csr_sb[`CSR_SB_MEPC+:XLEN] = mepc;
     assign csr_sb[`CSR_SB_MSTATUS+:XLEN] = mstatus;
+    assign csr_sb[`CSR_SB_MCOUNTEREN+:XLEN] = mcounteren;
 
     assign csr_sb[`CSR_SB_MIE] = mstatus[3];
     assign csr_sb[`CSR_SB_MEIP] = mip[11];

diff --git a/rtl/friscv_h.sv b/rtl/friscv_h.sv
@@ -226,7 +226,8 @@
 `define CSR_SB_MEPC         `CSR_SB_MTVEC + `XLEN
 `define CSR_SB_MSTATUS      `CSR_SB_MEPC + `XLEN
 `define CSR_SB_MIE          `CSR_SB_MSTATUS + `XLEN
-`define CSR_SB_MEIP         `CSR_SB_MIE + 1
+`define CSR_SB_MCOUNTEREN   `CSR_SB_MIE + `XLEN
+`define CSR_SB_MEIP         `CSR_SB_MCOUNTEREN + `XLEN
 `define CSR_SB_MTIP         `CSR_SB_MEIP + 1
 `define CSR_SB_MSIP         `CSR_SB_MTIP + 1
 `define CSR_SB_MEIE         `CSR_SB_MSIP + 1

diff --git a/rtl/friscv_mpu.sv b/rtl/friscv_mpu.sv
@@ -117,7 +117,7 @@ module friscv_mpu
                 )
                 pmp_region 
                 (
-                    .pmp_addr    (csr_sb[4*XLEN+i*XLEN+:XLEN]), // 4*XLEN = 4 * PMPCFG
+                    .pmp_addr    (csr_sb[4*XLEN+i*XLEN+:XLEN]), // 4*XLEN = 4 * PMPCFG bc is on first position on the bus
                     .pmp_cfg     (csr_sb[i*8+:8]),
                     .pmp_base    (pmp_base[i*RLEN+:RLEN]),
                     .pmp_mask    (pmp_mask[i*RLEN+:RLEN])

diff --git a/test/common/debug_platform_verilator.gtkw b/test/common/debug_platform_verilator.gtkw
@@ -1,15 +1,15 @@
 [*]
 [*] GTKWave Analyzer v3.4.0 (w)1999-2022 BSI
-[*] Sun Oct 22 18:08:09 2023
+[*] Sun Oct 22 18:40:10 2023
 [*]
-[dumpfile] "/Users/damien/workspace/hdl/friscv/test/wba_testsuite/friscv_testbench.vcd"
-[dumpfile_mtime] "Sun Oct 22 18:07:49 2023"
-[dumpfile_size] 1961741
+[dumpfile] "/Users/damien/workspace/hdl/friscv/test/priv_sec_testsuite/friscv_testbench.vcd"
+[dumpfile_mtime] "Sun Oct 22 18:10:23 2023"
+[dumpfile_size] 2015157
 [savefile] "/Users/damien/workspace/hdl/friscv/test/priv_sec_testsuite/debug_platform_verilator.gtkw"
-[timestart] 1937
+[timestart] 2212
 [size] 1440 900
 [pos] -1 -1
-*-5.496682 2106 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1
+*-4.496682 2287 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1
 [treeopen] friscv_testbench.
 [treeopen] friscv_testbench.friscv_testbench.
 [treeopen] friscv_testbench.friscv_testbench.genblk2.
@@ -21,7 +21,7 @@
 [treeopen] friscv_testbench.friscv_testbench.genblk2.dut.cpu0.processing.
 [treeopen] friscv_testbench.friscv_testbench.genblk2.dut.cpu0.USE_ICACHE.icache.cache_blocks.
 [sst_width] 255
-[signals_width] 263
+[signals_width] 237
 [sst_expanded] 1
 [sst_vpaned_height] 234
 @c00200
@@ -93,16 +93,16 @@ friscv_testbench.friscv_testbench.genblk2.axi4l_ram.p1_bvalid
 -
 @1401200
 -Interconnect
-@c00201
+@c00200
 -CSRs
-@201
+@200
 -
 @23
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpcfg0[31:0]
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpcfg1[31:0]
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpcfg2[31:0]
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpcfg3[31:0]
-@201
+@200
 -
 @23
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpaddr0[31:0]
@@ -121,17 +121,17 @@ friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpaddr12[31:0]
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpaddr13[31:0]
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpaddr14[31:0]
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.pmpaddr15[31:0]
-@201
+@200
 -
 @23
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.ctrl_mstatus[31:0]
 @29
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.ctrl_mstatus_wr
-@201
+@200
 -
 @23
 friscv_testbench.friscv_testbench.genblk2.dut.cpu0.csrs.mstatus[31:0]
-@1401201
+@1401200
 -CSRs
 @c00200
 -ISA Regsiters

diff --git a/test/priv_sec_testsuite/tests/Makefile b/test/priv_sec_testsuite/tests/Makefile
@@ -43,11 +43,11 @@ vpath %.S $(src_dir)
 define compile_template
 
 $$($(1)_p_tests): $(1)-p-%: $(1)/%.S
-	$$(RISCV_GCC) $(2) $$(RISCV_GCC_OPTS) -I$(env_dir)/p -I$(src_dir)/macros/scalar -T$(env_dir)/p/link.ld $$< -o $$@.elf
+	$$(RISCV_GCC) $(2) $$(RISCV_GCC_OPTS) -I$(env_dir) -I$(env_dir)/p -I$(src_dir)/macros/scalar -T$(env_dir)/p/link.ld $$< -o $$@.elf
 $(1)_tests += $$($(1)_p_tests)
 
 $$($(1)_v_tests): $(1)-v-%: $(1)/%.S
-	$$(RISCV_GCC) $(2) $$(RISCV_GCC_OPTS) -DENTROPY=0x$$(shell echo \$$@ | md5sum | cut -c 1-7) -std=gnu99 -O2 -I$(env_dir)/v -I$(src_dir)/macros/scalar -T$(env_dir)/v/link.ld $(env_dir)/v/entry.S $(env_dir)/v/*.c $$< -o $$@.elf
+	$$(RISCV_GCC) $(2) $$(RISCV_GCC_OPTS) -DENTROPY=0x$$(shell echo \$$@ | md5sum | cut -c 1-7) -std=gnu99 -O2 -I$(env_dir) -I$(env_dir)/v -I$(src_dir)/macros/scalar -T$(env_dir)/v/link.ld $(env_dir)/v/entry.S $(env_dir)/v/*.c $$< -o $$@.elf
 $(1)_tests += $$($(1)_v_tests)
 
 $(1)_tests_dump = $$(addsuffix .asm, $$($(1)_tests))

diff --git a/test/priv_sec_testsuite/tests/env/p/riscv_test.h b/test/priv_sec_testsuite/tests/env/p/riscv_test.h
@@ -265,7 +265,8 @@ reset_vector:                                                           \
         li a7, 93;                                                      \
         addi a0, TESTNUM, 0;                                            \
         add x31, x31, 1;                                                \
-        ebreak
+        ebreak;                                                         \
+        ebreak;                                                         \
 
 //-----------------------------------------------------------------------
 // Data Section Macro

diff --git a/test/priv_sec_testsuite/tests/rv32ui-p-test0.v b/test/priv_sec_testsuite/tests/rv32ui-p-test0.v
@@ -33,11 +33,11 @@ B3 FE CE 01 63 1E D0 05 F3 2E 10 34 93 8E 4E 00
 F3 2E 10 34 93 8E 4E 00 73 90 1E 34 73 00 20 30
 73 00 00 00 93 00 10 00 63 14 1A 02 73 00 20 30
 93 80 10 00 63 1E 1A 00 F3 2E 00 30 93 80 10 00
-63 18 1A 00 73 00 10 00 6F 00 80 02 63 12 30 02
+63 18 1A 00 73 00 10 00 6F 00 C0 02 63 14 30 02
 0F 00 F0 0F 63 80 01 00 93 91 11 00 93 E1 11 00
 93 08 D0 05 13 85 01 00 93 8F 1F 00 73 00 10 00
-0F 00 F0 0F 93 01 10 00 93 08 D0 05 13 05 00 00
-73 00 10 00 73 00 10 00 73 10 00 C0 00 00 00 00
+73 00 10 00 0F 00 F0 0F 93 01 10 00 93 08 D0 05
+13 05 00 00 73 00 10 00 73 00 10 00 73 10 00 C0
 00 00 00 00 00 00 00 00 00 00 00 00
 @00011000
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

diff --git a/test/priv_sec_testsuite/tests/rv32ui-p-test1.v b/test/priv_sec_testsuite/tests/rv32ui-p-test1.v
@@ -36,11 +36,11 @@ F3 2E 10 34 93 8E 4E 00 73 90 1E 34 73 00 20 30
 93 08 90 00 73 00 00 00 93 02 00 00 13 03 00 00
 93 03 40 06 93 82 12 00 23 20 50 00 03 23 00 00
 E3 9A 72 FE 63 9E 62 00 13 0A 00 00 13 03 10 00
-73 00 50 10 63 16 6A 00 6F 00 80 02 63 12 30 02
+73 00 50 10 63 16 6A 00 6F 00 C0 02 63 14 30 02
 0F 00 F0 0F 63 80 01 00 93 91 11 00 93 E1 11 00
 93 08 D0 05 13 85 01 00 93 8F 1F 00 73 00 10 00
-0F 00 F0 0F 93 01 10 00 93 08 D0 05 13 05 00 00
-73 00 10 00 73 00 10 00 73 10 00 C0 00 00 00 00
+73 00 10 00 0F 00 F0 0F 93 01 10 00 93 08 D0 05
+13 05 00 00 73 00 10 00 73 00 10 00 73 10 00 C0
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 00 00 00 00 00 00 00 00 00 00 00 00
 @00011000

diff --git a/test/priv_sec_testsuite/tests/rv32ui-p-test2.v b/test/priv_sec_testsuite/tests/rv32ui-p-test2.v
@@ -115,14 +115,18 @@ F3 2E 10 34 93 8E 4E 00 73 90 1E 34 73 00 20 30
 93 08 10 00 73 00 00 00 13 05 00 00 B7 05 10 00
 13 06 00 00 93 06 F0 08 93 08 80 00 73 00 00 00
 EF 00 80 01 B7 02 10 00 23 A0 02 00 93 02 10 00
-63 1E 54 02 6F 00 80 05 93 02 00 00 13 03 A0 00
+63 1E 54 02 6F 00 C0 05 93 02 00 00 13 03 A0 00
 93 03 00 00 13 0E 00 00 B3 83 53 00 23 20 7E 00
 93 83 13 00 83 23 0E 00 93 82 12 00 13 9E 22 00
-E3 94 62 FE 67 80 00 00 63 12 30 02 0F 00 F0 0F
+E3 94 62 FE 67 80 00 00 63 14 30 02 0F 00 F0 0F
 63 80 01 00 93 91 11 00 93 E1 11 00 93 08 D0 05
-13 85 01 00 93 8F 1F 00 73 00 10 00 0F 00 F0 0F
-93 01 10 00 93 08 D0 05 13 05 00 00 73 00 10 00
-73 00 10 00 73 10 00 C0
+13 85 01 00 93 8F 1F 00 73 00 10 00 73 00 10 00
+0F 00 F0 0F 93 01 10 00 93 08 D0 05 13 05 00 00
+73 00 10 00 73 00 10 00 73 10 00 C0 00 00 00 00
+00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+00 00 00 00 00 00 00 00
 @00011000
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00