added zicsrh coverpoints

[AnonymousVikram]

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.

[SadhviNarayanan]

should there be another cross with an illegal instruction exception?

[SadhviNarayanan]

might need to cross these with an illegal instruction exception

[davidharrishmc]

@AnonymousVikram there is still a lot that needs to clean up on this PR. I've done a partial review. When it is ready, turn off draft and let me know that it is ready to review.

[davidharrishmc]

How do we know we are in hs mode, not vs mode?

[davidharrishmc]

I don't see this file in riscv-arch-test act4 branch.

[davidharrishmc]

Same queries as above. These apply many places.

[davidharrishmc]

Exception is not needed here. Exception is an expected outcome, not an input condition.

[davidharrishmc]

Applies many places.

[davidharrishmc]

not needed

[davidharrishmc]

This should be vs mode, not priv_mode_s.

[davidharrishmc]

The coverpoint is named cp_hcsr_virtualinstructionfault

[davidharrishmc]

I think this should be mhcsr rather than hcsr.

[davidharrishmc]

Distinguish u and vu modes.

[Copilot]

Pull request overview

This PR adds functional coverage infrastructure for the RISC-V Hypervisor extension (ZicsrH), introducing comprehensive coverage points for hypervisor CSRs and privileged instructions.

• Adds coverage groups for H-extension CSR access patterns, bit-walking tests, and replica testing
• Implements coverage for privilege mode interactions (M, HS, VS, U, VU modes)
• Covers hypervisor-specific behaviors like TVM/VTVM traps, virtual instruction faults, and special CSR fields

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
coverpoints/priv/ZicsrH_coverage_init.svh	Initializes all ZicsrH coverage group instances with appropriate names
coverpoints/priv/ZicsrH_coverage.svh	Defines 9 comprehensive coverage groups for H-extension CSRs, privilege modes, traps, and hypervisor instructions

---

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

[Copilot]

The VTVM cross coverage appears to have incorrect filtering logic. Line 578 uses ignore_bins to exclude hgatp (12'h680) from the VTVM test, but the comment on line 576 states "VTVM trap in VS-mode accessing satp". However, according to the RISC-V privileged specification, VTVM controls traps for SATP accesses from VS-mode, so the filtering is correct. But the cross coverage is using mstatus_tvm which controls TVM, not VTVM. VTVM is a separate bit in hstatus. This coverpoint should check hstatus.VTVM instead of mstatus.TVM for the VS-mode case.

[Copilot]

The comment on line 159 appears to be redundant or incorrectly placed. Line 154 already has a comment describing "VS-mode (hstatus.V=1): HS CSRs should cause virtual instruction fault", and line 159 repeats "VS-mode (hstatus.V=1): VS CSRs should cause virtual instruction fault". However, this is describing a different coverpoint (cp_vscsr_virtualinstructionfault_vs vs cp_hscsr_virtualinstructionfault_vs), so while not incorrect, the duplicate phrasing on consecutive lines reduces clarity. Consider condensing these comments or making them more distinct.

Suggested change

	// VS-mode (hstatus.V=1): HS CSRs should cause virtual instruction fault
	cp_hscsr_virtualinstructionfault_vs: cross priv_mode_s, hscsrname, csrop, hstatus_v_bit, exception {
	ignore_bins not_vs_mode = binsof(hstatus_v_bit) intersect {hstatus_v_bit.hs_mode}; // Exclude HS mode (hstatus.V=0)
	ignore_bins no_virtual_instr_fault_trap = binsof(exception) intersect {0}; // Expect a trap (virtual instruction fault)
	}
	// VS-mode (hstatus.V=1): VS CSRs should cause virtual instruction fault
	// In VS-mode (hstatus.V=1), accesses to HS CSRs (cp_hscsr_virtualinstructionfault_vs) should cause a virtual instruction fault
	cp_hscsr_virtualinstructionfault_vs: cross priv_mode_s, hscsrname, csrop, hstatus_v_bit, exception {
	ignore_bins not_vs_mode = binsof(hstatus_v_bit) intersect {hstatus_v_bit.hs_mode}; // Exclude HS mode (hstatus.V=0)
	ignore_bins no_virtual_instr_fault_trap = binsof(exception) intersect {0}; // Expect a trap (virtual instruction fault)
	}
	// In VS-mode (hstatus.V=1), accesses to VS CSRs (cp_vscsr_virtualinstructionfault_vs) should cause a virtual instruction fault

[Copilot]

Inconsistent spacing in the initialization statements. Lines 13-16 have proper alignment with consistent spacing before set_inst_name, but lines 17-21 have inconsistent spacing (some have more spaces, some fewer). For better readability and maintainability, all lines should use consistent alignment.

Suggested change

	ZicsrH_vsstatus_cg = new(); ZicsrH_vsstatus_cg.set_inst_name("obj_ZicsrH_vsstatus");
	ZicsrH_tvm_cg = new(); ZicsrH_tvm_cg.set_inst_name("obj_ZicsrH_tvm");
	ZicsrH_mtval_cg = new(); ZicsrH_mtval_cg.set_inst_name("obj_ZicsrH_mtval");
	ZicsrH_hprivinst_cg = new(); ZicsrH_hprivinst_cg.set_inst_name("obj_ZicsrH_hprivinst");
	ZicsrH_vsstatus_cg = new(); ZicsrH_vsstatus_cg.set_inst_name("obj_ZicsrH_vsstatus");
	ZicsrH_tvm_cg = new(); ZicsrH_tvm_cg.set_inst_name("obj_ZicsrH_tvm");
	ZicsrH_mtval_cg = new(); ZicsrH_mtval_cg.set_inst_name("obj_ZicsrH_mtval");
	ZicsrH_hprivinst_cg = new(); ZicsrH_hprivinst_cg.set_inst_name("obj_ZicsrH_hprivinst");

[Copilot]

The coverpoints include both cp_vsstatus_sd, cp_vsstatus_fs, cp_vsstatus_vs, and cp_vsstatus_xs but these are defined without any bins. Empty coverpoints without bins will capture all possible values, which may not be the intended behavior. Consider adding explicit bins to define the specific values or states that should be covered for each field.

Suggested change

	cp_vsstatus_sd: coverpoint ins.current.rs1_val[XLEN-1] {
	}
	// FS field [14:13]
	cp_vsstatus_fs: coverpoint ins.current.rs1_val[14:13] {
	}
	// VS field [10:9]
	cp_vsstatus_vs: coverpoint ins.current.rs1_val[10:9] {
	}
	// XS field [16:15] - read-only, reflects extension state
	cp_vsstatus_xs: coverpoint ins.current.rs1_val[16:15] {
	cp_vsstatus_sd: coverpoint ins.current.rs1_val[XLEN-1] {
	bins sd_0 = {0};
	bins sd_1 = {1};
	}
	// FS field [14:13]
	cp_vsstatus_fs: coverpoint ins.current.rs1_val[14:13] {
	bins fs_off = {2'b00};
	bins fs_initial = {2'b01};
	bins fs_clean = {2'b10};
	bins fs_dirty = {2'b11};
	}
	// VS field [10:9]
	cp_vsstatus_vs: coverpoint ins.current.rs1_val[10:9] {
	bins vs_0 = {2'b00};
	bins vs_1 = {2'b01};
	bins vs_2 = {2'b10};
	bins vs_3 = {2'b11};
	}
	// XS field [16:15] - read-only, reflects extension state
	cp_vsstatus_xs: coverpoint ins.current.rs1_val[16:15] {
	bins xs_all_clean = {2'b00};
	bins xs_initial = {2'b01};
	bins xs_clean = {2'b10};
	bins xs_dirty = {2'b11};

[Copilot]

The cross coverage cp_hv_loadstore_hs and cp_hfence_hs use ignore_bins to filter priv_mode_s values, but the filtering logic intersects with hardcoded values {2'b00, 2'b11}. This assumes specific encodings for privilege modes. Consider using named bins (e.g., priv_mode_s.u_mode, priv_mode_s.m_mode) for clarity and to ensure the ignore_bins correctly excludes non-HS modes. This would make the code more maintainable and less prone to errors if privilege mode encodings change.

Suggested change

	ignore_bins not_hs = binsof(priv_mode_s) intersect {2'b00, 2'b11};
	}
	cp_hfence_m: cross priv_mode_m, hfence;
	cp_hfence_hs: cross priv_mode_s, hfence {
	ignore_bins not_hs = binsof(priv_mode_s) intersect {2'b00, 2'b11};
	ignore_bins not_hs = binsof(priv_mode_s) intersect {priv_mode_s.u_mode, priv_mode_s.m_mode};
	}
	cp_hfence_m: cross priv_mode_m, hfence;
	cp_hfence_hs: cross priv_mode_s, hfence {
	ignore_bins not_hs = binsof(priv_mode_s) intersect {priv_mode_s.u_mode, priv_mode_s.m_mode};

[Copilot]

The hfence coverpoint bins are defined with specific full 32-bit instruction encodings (e.g., 32'h22000073, 32'h62000073) which only match when rs1=x0 and rs2=x0. However, HFENCE.VVMA and HFENCE.GVMA instructions can have non-zero rs1 and rs2 operands to specify address ranges and ASIDs/VMIDs. This coverage will miss testing these instructions with different operand values. Consider using wildcard bins to cover the full range of valid operand combinations.

Suggested change

	hfence: coverpoint ins.current.insn {
	bins hfence_vvma = {32'h22000073}; // rs1=x0, rs2=x0
	bins hfence_gvma = {32'h62000073}; // rs1=x0, rs2=x0
	// HFENCE instructions: match all valid rs1/rs2 combinations
	hfence: coverpoint ins.current.insn {
	wildcard bins hfence_vvma = {32'b00100010??????????00000001110011}; // HFENCE.VVMA: opcode=SYSTEM, funct7=0x22, funct3=0, rs1/rs2=any
	wildcard bins hfence_gvma = {32'b01100010??????????00000001110011}; // HFENCE.GVMA: opcode=SYSTEM, funct7=0x62, funct3=0, rs1/rs2=any

[davidharrishmc]

@AnonymousVikram there is still a lot to do here. We seem to have a disconnect because you seem to think it is done. Let's communicate more over Slack or f2f. Please reply to the comments with "DONE" when you feel they are done. Same with comments from Copilot.