Coverpoints for EndianH, SvinvalH, ZicntrH, Zihpm #715

juliacygong · 2025-11-01T20:48:39Z

Description

Provide a detailed description of the changes performed by the PR.

Related Issues

Please list all the issues related to this PR. Use NA if no issues exist

Ratified/Unratified Extensions

Ratified
Unratified

List Extensions

List the extensions that your PR affects. In case of unratified extensions, please provide a link to the spec draft that was referred to make this PR.

Reference Model Used

SAIL
Spike
Other - < SPECIFY HERE >

Mandatory Checklist:

All tests are compliant with the test-format spec present in this repo ?
Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ?
Ran the new tests on RISCOF in coverage mode
Link to Google-Drive folder containing the new coverage reports (See this for more info): < SPECIFY HERE >

Optional Checklist:

Were the tests hand-written/modified ?
Have you run these on any hard DUT model ? Please specify name and provide link if possible in the description
If you have modified arch_test.h Please provide a detailed description of the changes in the Description section above.

coreyqh

Overall good work, a few minor changes needed across the board and some major changes needed for ZicntrH

coreyqh · 2025-11-08T02:04:56Z

coverpoints/priv/EndianH_coverage.svh

+        hstatus_vsbe: coverpoint ins.current.csr[12'h600][5] { // vsbe is hstatus[5] in RV64
+        }
+    `else
+        hstatus_vsbe: coverpoint ins.current.csr[12'h600][5] { // vsbe is hstatush[5] in RV32


Looks like the hstatus_vsbe helper coverpoint is the same regardless of XLEN. Need to update with the address of hstatush for the RV32 version.

coreyqh · 2025-11-08T02:07:48Z

coverpoints/priv/EndianH_coverage.svh

+covergroup EndianH_endian_cg with function sample(ins_t ins);
+    option.per_instance = 0;
+    `include "coverage/RISCV_coverage_standard_coverpoints.svh"
+    // "Endianness tests in machine mode"


This comment should likely be "endianness tests in hypervisor mode" or similar

coreyqh · 2025-11-08T02:12:26Z

coverpoints/priv/EndianH_coverage.svh

+            wildcard bins lwu = {32'b????????????_?????_110_?????_0000011};
+        }
+        cp_doubleoffset: coverpoint ins.current.imm[2:0] iff (ins.current.rs1_val[2:0] == 3'b000)  {
+            bins zero = {3'b000};


Personally, I think it would be more readable to define these up with the lw, sw, etc instructions in a separate `ifdef block.

coreyqh · 2025-11-08T02:20:32Z

coverpoints/priv/SvinvalH_coverage.svh

+// ---------------------
+function void svinvalH_sample(int hart, int issue, ins_t ins);
+
+    //$display("Svinval coverage: ins_str %s ins,prev.mode %b tvm %b", ins.ins_str, ins.prev.mode, ins.prev.csr[12'h300][20]);


Consider removing this debugging print statement

coreyqh · 2025-11-08T02:22:18Z

coverpoints/priv/ZicntrH_coverage.svh

+//
+///////////////////////////////////////////
+
+define COVER_ZICNTRH


should be `define (missing the "tick")

coreyqh · 2025-11-08T02:23:05Z

coverpoints/priv/ZicntrH_coverage.svh

+covergroup ZicntrH_scounters_cg with function sample(ins_t ins);
+    option.per_instance = 0;
+    `include "coverage/RISCV_coverage_standard_coverpoints.svh"
+    // counter access in supervisor mode


Hypervisor mode

coreyqh · 2025-11-08T02:38:18Z

coverpoints/priv/ZicntrH_coverage.svh

+    `endif
+
+    // M mode coverpoints
+    cp_mhscounteren_access_m: cross csrr, counters_mcounteren, counters_hcounteren, counters_scounteren, priv_mode_m;


counters_*counteren coverpoints all include parts of the 32-bit machine code instruction.

This means that cross products of these will have mutually exclusive bins since the value of that part of the instruction cannot have multiple values simultaneously. You will need to separate this into different cross products.

coreyqh · 2025-11-08T02:42:52Z

coverpoints/priv/Zihpm_coverage.svh

+        `endif
+    }
+
+    cp_hmp_write: cross cp_hpmh_count, priv_mode_m;


Should be hpm not hpmh

coreyqh · 2025-11-08T08:55:38Z

Looks amazing @juliacygong

Just a few picky things: there is still a display statement in there, and access is misspelled as acces in a few places.

After those are fixed, this has my endorsement.

Signed-off-by: juliacygong <[email protected]>

coreyqh

Great work

jordancarlin

More comments to come later, but here is a first round of feedback.

jordancarlin · 2025-11-09T18:18:35Z

coverpoints/priv/EndianH_coverage.svh

+///////////////////////////////////////////
+//
+// RISC-V Architectural Functional Coverage Covergroups Initialization File
+//
+///////////////////////////////////////////


Review headers in all files. Currently missing license and this is not an initialization file. See other coverpoint files for an example.

jordancarlin · 2025-11-09T18:19:08Z

coverpoints/priv/EndianH_coverage.svh

+    // "Endianness tests in hypervisor mode"
+
+    // building blocks for the main coverpoints
+        // ENDIANNESS COVERPOINTS: check writes and reads with various endianness


Strange indentation

jordancarlin · 2025-11-09T18:19:24Z

coverpoints/priv/EndianH_coverage.svh

+//
+///////////////////////////////////////////
+
+`define COVER_ENDIANS


Suggested change

`define COVER_ENDIANS

`define COVER_ENDIANH

jordancarlin · 2025-11-09T18:19:54Z

coverpoints/priv/EndianH_coverage.svh

+    cp_sd: coverpoint ins.current.insn {
+            wildcard bins sd = {32'b????????????_?????_011_?????_0100011};
+        }


Indentation issue

jordancarlin · 2025-11-09T18:21:23Z

coverpoints/priv/EndianH_coverage.svh

+    hstatus_vsbe: coverpoint ins.current.csr[12'h600][5] { // vsbe is hstatus[5] in RV64
+        }


Indentation

jordancarlin · 2025-11-09T18:25:02Z

coverpoints/priv/SvinvalH_coverage.svh

+        wildcard bins hinval.vvma     = {32'b0010001_?????_?????_000_00000_1110011};
+        wildcard bins hinval.gvma     = {32'b0110001_?????_?????_000_00000_1110011};


Use underscores in place of . in instruction names.

jordancarlin · 2025-11-09T18:26:09Z

coverpoints/priv/SvinvalH_coverage.svh

+        bins vtvm_0 = {0};
+        bins vtvm_1 = {1};


No need to explicitly define the bins since we want all of the values and the default names of 0/1 will be clear enough.

jordancarlin · 2025-11-09T18:27:04Z

coverpoints/priv/Zihpm_coverage.svh

+    option.per_instance = 0;
+    `include "coverage/RISCV_coverage_standard_coverpoints.svh"
+
+cp_hpm_count: coverpoint {ins.current.insn[31:20], ins.current.csr[12'hB03][31:0] } {


Indentation

jordancarlin · 2025-11-09T18:27:54Z

coverpoints/priv/Zihpm_coverage.svh

+        bins cycle_enabled         = {44'b110000000000_00000000000000000000000000000001};
+        bins time_enabled          = {44'b110000000001_00000000000000000000000000000010};
+        bins instret_enabled       = {44'b110000000010_00000000000000000000000000000100};


cycle, time, and instret are part of Zicntr, not Zihpm.

jordancarlin · 2025-11-09T18:28:48Z

coverpoints/priv/ZicntrH_coverage.svh

+    }
+
+    counters_scounteren: coverpoint {ins.current.insn[31:20], ins.current.csr[12'h106][31:0]} {
+    bins cycle_enabled         = {44'b110000000000_00000000000000000000000000000001};


Indentation

davidharrishmc

Finally reviewed. Looking good; most fixes should be easy.

davidharrishmc · 2025-11-24T14:54:43Z

coverpoints/priv/EndianH_coverage.svh

+        wildcard bins sd = {32'b????????????_?????_011_?????_0100011};
+    }
+    cp_ld: coverpoint ins.current.insn {
+        wildcard bins ld = {32'b????????????_?????_001_?????_0000011};


funct3 = 011

Fixed funct3 for ld. I also noticed that funct3 for ld is also incorrect in EndianM, EndianS, and EndianU and fixed them.

davidharrishmc · 2025-11-24T15:15:13Z

coverpoints/priv/EndianH_coverage.svh

+    cp_hstatus_vbe_endianness_lb:  cross priv_mode_vs, hstatus_vsbe, cp_lb,  cp_byteoffset;
+    cp_hstatus_vbe_endianness_lhu: cross priv_mode_vs, hstatus_vsbe, cp_lhu, cp_halfoffset;
+    cp_hstatus_vbe_endianness_lbu: cross priv_mode_vs, hstatus_vsbe, cp_lbu, cp_byteoffset;
+    cp_mstatus_mprv_vsbe_endianness_sw:  cross priv_mode_m, hstatus_vsbe, cp_sw,  cp_wordoffset, mstatus_mprv, mstatus_mbe;


Testplan calls for mpp={01/11}, mpv = {0/1}, mbe={0/1} that don't seem to be in these crosses.

davidharrishmc · 2025-11-24T15:18:54Z

coverpoints/priv/EndianH_coverage.svh

+    // VS and VU modes
+    priv_mode_vs: coverpoint {ins.current.VirtMode, ins.current.mode} {
+        bins VS_mode = {3'b101}; 
+    }
+
+    priv_mode_vu: coverpoint {ins.current.VirtMode, ins.current.mode} {
+        bins VU_mode = {3'b100};
+    }


This applies to the other covergroups as well.

davidharrishmc · 2025-11-24T15:21:47Z

coverpoints/priv/SvinvalH_coverage.svh

+        wildcard bins sfence_w_inval  = {32'b0001100_00000_00000_000_00000_1110011};
+        wildcard bins sinval_vma      = {32'b0001011_?????_?????_000_00000_1110011};
+        wildcard bins hinval_vvma     = {32'b0010001_?????_?????_000_00000_1110011};
+        wildcard bins hinval_gvma     = {32'b0110001_?????_?????_000_00000_1110011};


Looks like funct7 should be 0010011 for hinval_vvma and 0110011 for hinval_gvma per Figure 125 of the priv spec.

davidharrishmc · 2025-11-24T15:29:00Z

coverpoints/priv/ZicntrH_coverage.svh

+
+    cp_htimedelta: coverpoint {ins.current.csr[12'h605][63:0]} {
+        bins htimedelta_zero  = {64'd0};
+        bins htimedelta_2p30  = {64'd1073741824};           //  2^30


Write this in hex with a 1 in bit 30.

davidharrishmc · 2025-11-24T15:33:37Z

coverpoints/priv/ZicntrH_coverage.svh

+
+    `ifdef XLEN32
+
+    cp_htimedeltah: coverpoint {ins.current.csr[12'h615][31:0]}1 {


Something is garbled. For RV32, I think you want a htimedelta coverpoint as in RV64, but that it involve {htimedeltah, htimedelta} to give a 64-bit value. I believe there should be an `if/else to define htimedelta differently for RV64 vs 32.

davidharrishmc · 2025-11-24T15:35:59Z

coverpoints/priv/ZicntrH_coverage.svh

+
+    cp_htimedeltah: coverpoint {ins.current.csr[12'h615][31:0]}1 {
+        bins htimedeltah_zero = {{32'd0, 32'd0}};
+        bins htimedeltah_2p30 = {{32'd1073741824, 32'd0}};   //  2^30


It would be more readable to put the upper bits in the upper half. Use hex rather than decimal. The encoding for 2p60 and n2p60 looks incorrect.

davidharrishmc · 2025-11-24T15:40:21Z

coverpoints/priv/ZicntrH_coverage.svh

+    cp_mhscounteren_access_hcnt_m: cross csrr, counters_hcounteren, priv_mode_m;
+    cp_mhscounteren_access_scnt_m: cross csrr, counters_scounteren, priv_mode_m;
+    cp_delta_m: cross csrr, cp_htimedelta, priv_mode_m;
+    `ifdef XLEN32


If htimedelta coverpoint is defined to depend on XLEN, then this coverpoint is not needed because the 64-bit value is subsumed in htimedelta.

davidharrishmc · 2025-11-24T15:41:25Z

coverpoints/priv/ZicntrH_coverage.svh

+    cp_mhscounteren_access_scnt_m: cross csrr, counters_scounteren, priv_mode_m;
+    cp_delta_m: cross csrr, cp_htimedelta, priv_mode_m;
+    `ifdef XLEN32
+    cp_deltah_m: cross csrr, cp_htimedeltah, priv_mode_m;


Can this be more specific about reading time, rather than just a csrr?

Same comments about htimedelta and csrr time apply to all delta coverpoints.

davidharrishmc · 2025-11-24T16:00:55Z

coverpoints/priv/ZicntrH_coverage.svh

+        bins VU_mode = {3'b100};
+    }
+
+    counters_hcounteren: coverpoint {ins.current.insn[31:20], ins.current.csr[12'h606][31:0]}


juliacygong added 4 commits October 31, 2025 20:25

coverpoint drafts

0ea5f3e

remove unnecessary

aa1aea7

fixing cp

7d1cec1

cp syntax edits

253c9f6

juliacygong changed the title ~~coverpoints draft~~ Draft Coverpoints for EndianH, SvinvalH, ZicntrH, Zihpm Nov 4, 2025

coreyqh suggested changes Nov 8, 2025

View reviewed changes

cp draft edits

b0f174d

fixed ZicntrH cp syntax

5eaee02

Signed-off-by: juliacygong <[email protected]>

coreyqh approved these changes Nov 8, 2025

View reviewed changes

jordancarlin requested changes Nov 9, 2025

View reviewed changes

juliacygong added 3 commits November 10, 2025 12:14

cp review riscv-non-isa#2 edits

21914aa

cp review 2 syntax fixes

2341653

fixed initialization headers

8dda9b6

juliacygong marked this pull request as ready for review November 14, 2025 19:01

juliacygong changed the title ~~Draft Coverpoints for EndianH, SvinvalH, ZicntrH, Zihpm~~ Coverpoints for EndianH, SvinvalH, ZicntrH, Zihpm Nov 14, 2025

davidharrishmc requested changes Nov 24, 2025

View reviewed changes

coverpoints revision 3

08c35a5

		hstatus_vsbe: coverpoint ins.current.csr[12'h600][5] { // vsbe is hstatus[5] in RV64
		}

		wildcard bins hinval.vvma = {32'b0010001_?????_?????_000_00000_1110011};
		wildcard bins hinval.gvma = {32'b0110001_?????_?????_000_00000_1110011};


		`ifdef XLEN32

		cp_htimedeltah: coverpoint {ins.current.csr[12'h615][31:0]}1 {

Coverpoints for EndianH, SvinvalH, ZicntrH, Zihpm #715

Are you sure you want to change the base?

Coverpoints for EndianH, SvinvalH, ZicntrH, Zihpm #715

Uh oh!

Conversation

juliacygong commented Nov 1, 2025

Description

Related Issues

Ratified/Unratified Extensions

List Extensions

Reference Model Used

Mandatory Checklist:

Optional Checklist:

Uh oh!

coreyqh left a comment

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!

coreyqh commented Nov 8, 2025

Uh oh!

coreyqh left a comment

Choose a reason for hiding this comment

Uh oh!

jordancarlin left a comment

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!

Choose a reason for hiding this comment

Uh oh!

davidharrishmc left a comment

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!