[ACT] ACTs and coverage points for machine mode software and timer interrupts #567

Ali-Faraz-10xe · 2024-11-21T15:46:25Z

Description

This pull request introduces tests and covergroups to verify machine mode software and timer interrupts. The tests and covergroups were developed following the guidelines outlined in the Test Plan

Related Issues

This pull request relies on the changes in the following PRs for full functionality and compatibility:

Ratified/Unratified Extensions

Ratified
Unratified

List Extensions

N/A

Reference Model Used

SAIL
Spike
Other - < SPECIFY HERE >

DUT 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 available here
Link to PR in RISCV-ISAC from which the reports were generated available here

Optional Checklist:

Were the tests hand-written/modified ?

allenjbaum

In general, equations with constants (e.g. mip>>3 & 0x1!= 0x1) are not satisfactory. What would be satisfactory would be ((mip & MIP_MSIP)==0), where MIP_MSIP is #defined - and you don't even have to define it yourself because it is already defined in encoding.h which is #included. Note that this applies of almost CSR fields . The comments are quite appreciated, on the other hand.

coverage/header_file.yaml

riscof-plugins/rv32/sail_cSim/env/model_test.h

allenjbaum

Almost all tests need to initialize MIP.MSIP/MTIP before doing anything else.
The MAX_VALE should be renamed TEST_VALUE, since it isn't a max value at all.
I think you could combine a lot of the 32 and 64b tests, except for the cases that need to write all 64bits of MTIME and MTiMECMP (which I would do with a macro to and read them differently depending on XLEN)

riscv-test-suite/env/arch_test.h

riscv-test-suite/env/encoding.h

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-M-INTR-PRIORITY.S

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-MSIP-FUNCTIONALITY.S

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-MTIME-LENGTH.S

riscv-test-suite/rv32i_m/m_interrupts_32/src/DIS-GLB-MIE.S

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-MTIP-FUNCTIONALITY.S

…tine execution

…ftware and timer interrupts

…test.h to mitigate the issue of spurious interrupt

…framework

riscv-test-suite/env/arch_test.h

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-M-SOFTWARE-INTR.S

…P into vector signature Signed-off-by: Umer Shahid <[email protected]>

Signed-off-by: Umer Shahid <[email protected]>

…tart of the test Signed-off-by: Umer Shahid <[email protected]>

…tests

…ncoding file compatibility

Signed-off-by: Umer Shahid <[email protected]>

UmerShahidengr

All requested changes have been done

allenjbaum

This is the first batch of overall changes that are needed. I'll come back and do another review when these are fixed (I haven't made it all the way through yet.

allenjbaum · 2025-04-02T06:26:27Z

riscv-test-suite/env/arch_test.h

-sv_\__MODE__\()ip:                      // note: clear has no effect on MxIP
-        SREG    T4, 2*REGWIDTH(T1)      // save 3rd sig value, (xip)
+        csrrc   T2, CSR_MIDELEG, x0     // read machine interrupt delegation
+        nop	                // if mideleg is not used and trap occur


You need to document the reason for this. This is setting up the trap handler to take an exception whenever it interrupts into Smode (because it can't read the mideleg CSR that caused it to go to Smode?)
The problem here is that the trap signature for an illegal instruction won't store mideleg - so the test can never tell whether mideleg was set or not. It would be best to have the test (not the trap handler) explicitly store mideleg whenever it changes it, and avoid the extra trap.

allenjbaum · 2025-04-02T06:41:03Z

riscv-test-suite/env/arch_test.h

+        srl     T4, T4, T2              // move xIE to LSB
+        andi    T4, T4, 0b10001         // mask xIE and xPIE
+        SREG    T4, 2*REGWIDTH(T1)      // save 3rd sig value, (xstatus.XIE and xstatus.XPIE)
+        nop


Why is there a nop here? This shouldn't ever trap. Also: you're storing individual bits from Xstatus, XIE, and XIP; you should collect the 3 bits and store them in the first signature entry, (label sv__MODE_()vect, the first word of the signature entry; there are lots of free bits in that entry. This avoids changing the size of the interrupt trap signature entry. That size is pre-calculated, is used to update the trap signature pointer, and is stored in this entry. Changing the size introduces a bug because the pointer is pre-incremented with it, and the next trap will overwrite the extra word of this entry.

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-M-INTR-PRIORITY.S

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-M-SOFTWARE-INTR.S

allenjbaum · 2025-05-09T05:40:45Z

coverage/m_interrupts/rv32_m_interrupts.cgf

+  val_comb:
+      # Verify MSIP set and clear mechanisms
+      '(call_type != "interrupt") and ((mstatus & ${MSTATUS_MIE}) == 0) and ((mip & ${MIP_MSIP}) == ${MIP_MSIP})' : 0 # Checks software interrupt pending mechanism when writing 1 to memory-mapped MSIP control register without immediate interrupt service.
+      '(call_type != "interrupt") and ((mstatus & ${MSTATUS_MIE}) == 0) and ((mip & ${MIP_MSIP}) == 0)' : 0 # Validates software interrupt clearing mechanism when writing 0 to memory-mapped MSIP control register.


This one will get hit right out of reset. This needs to be conditioned on the old value of MIP_MSIP

allenjbaum · 2025-05-09T05:54:40Z

coverage/m_interrupts/rv32_m_interrupts.cgf

+  val_comb:
+      # Verify MTIP set and clear mechanisms
+      '(call_type != "interrupt") and ((mstatus & ${MSTATUS_MIE}) == 0) and ((mip & ${MIP_MTIP}) == ${MIP_MTIP})' : 0 # Verifies Machine Timer Interrupt Pending (MTIP) bit setting mechanism during non-interrupt execution.
+      '(call_type != "interrupt") and ((mstatus & ${MSTATUS_MIE}) == 0) and ((mip & ${MIP_MTIP}) == 0)' : 0 # Checks MTIP bit clearing mechanism when no interrupt is being processed.


I think this will generate false positives unless you can check for edges rather than levels, i.e. mip=MTIP and old_mip!=MTIP

allenjbaum · 2025-05-09T05:55:51Z

coverage/m_interrupts/rv32_m_interrupts.cgf

+      csrrc: 0
+      sw   : 0
+  val_comb:
+      '(call_type != "interrupt") and ((mstatus & ${MSTATUS_MIE}) == 0) and ((mie & ${MIE_MSIE}) == ${MIE_MSIE}) and ((mip & ${MIP_MSIP}) == ${MIP_MSIP})' : 0 # Confirms that no software interrupt is taken when global machine interrupt is disabled, despite local interrupt enable being active.


Anytime you are checking for call_type != "interrupt" you'll get false positives; but edge checking in all of them.

allenjbaum · 2025-05-09T05:58:20Z

coverage/m_interrupts/rv32_m_interrupts.cgf

+      mret : 0
+  val_comb:
+      # Validate software interrupt handling
+      '(call_type == "interrupt") and ((mstatus & ${MSTATUS_MPP}) == ${MSTATUS_MPP_M}) and ((mstatus & ${MSTATUS_MPIE}) == ${MSTATUS_MPIE}) and ((mstatus & ${MSTATUS_MIE}) == 0) and ((mcause & ${INTERRUPT_MASK}) == ${CAUSE_M_SOFT_INTR}) and ((mie & ${MIE_MSIE}) == ${MIE_MSIE}) and ((mip & ${MIP_MSIP}) == ${MIP_MSIP})': 0 # Validates software interrupt processing when global machine interrupt is enabled, verifying all required interrupt conditions.


Is this testing only trapping from Mmode? Don't we also want to test from Umode and Smode? Pretty tricky because you wo't necessarily have access to Mmode registers.

allenjbaum · 2025-05-09T06:15:06Z

riscof-plugins/rv32/sail_cSim/env/model_test.h

@@ -52,13 +56,32 @@ RVMODEL_DATA_SECTION			;\
 //RVTEST_IO_ASSERT_DFPR_EQ
 #define RVMODEL_IO_ASSERT_DFPR_EQ(_D, _R, _I)

-#define RVMODEL_SET_MSW_INT
+#define RVMODEL_SET_MSW_INT             ;\


The RVMODEL_Set/Clr_M/H/Sxxx_INT macros need to be redefined so that the aren't placed inline, but are called as simple subroutines with a JAL, and return to the test with a JR. The should be instantiated (not defined) in model_test.h as they are #defined, but with the name as a label at the start (instead of #define), and a JR at the end with a fixed link register (that tests will have to accomodate).

jamesbeyond changed the title ~~ACTs and coverage points for machine mode software and timer interrupts~~ [ACT] ACTs and coverage points for machine mode software and timer interrupts Dec 2, 2024

allenjbaum requested changes Dec 4, 2024

View reviewed changes

allenjbaum reviewed Dec 4, 2024

View reviewed changes

coverage/header_file.yaml Outdated Show resolved Hide resolved

allenjbaum reviewed Dec 4, 2024

View reviewed changes

riscof-plugins/rv32/sail_cSim/env/model_test.h Outdated Show resolved Hide resolved

Ali-Faraz-10xe requested a review from allenjbaum February 21, 2025 05:55

allenjbaum requested changes Feb 22, 2025

View reviewed changes

jamesbeyond added privileged ratified ratified specs labels Feb 24, 2025

Ali-Faraz-10xe added 16 commits April 2, 2025 05:42

Merge branch 'dev' into m_interrupts

07652be

Description Updated

1820257

Added ifndef,endif in all machine interrupts tests

7218801

Write Global MIE value in signatures during the interrupt service rou…

e7ae3d4

…tine execution

Removed mstatus signature after mret in EN-GLB-MIE test

24b0bdc

Added macros definitions in all model_test.h plugins files for the so…

72b0ac0

…ftware and timer interrupts

Add comments in covergroups and improve formatting

dc1d1f3

Add mret support in isac and covergroups

acddf50

Add nop in the end of each timer/software interrupts macros in model_…

2a17450

…test.h to mitigate the issue of spurious interrupt

Refactor: Improve RISC-V interrupt and CSR handling consistency

3bc02de

Add 'check_requirements.sh' file to check system requirement for the …

60a256b

…framework

modify mret support in isac

0575706

remove trailing space

3722acf

Removed Trailing Spaces

07a0a72

change label name from init_mtimecmp to init_\__MODE__\()timecmp

4de9800

remove redundant line

6a00d29

Ali-Faraz-10xe force-pushed the m_interrupts branch from e9d4d12 to 6a00d29 Compare April 2, 2025 00:48

allenjbaum reviewed Apr 2, 2025

View reviewed changes

riscv-test-suite/env/arch_test.h Outdated Show resolved Hide resolved

Ali-Faraz-10xe commented Apr 3, 2025

View reviewed changes

riscv-test-suite/rv32i_m/m_interrupts_32/src/CHK-M-SOFTWARE-INTR.S Outdated Show resolved Hide resolved

Ali-Faraz-10xe force-pushed the m_interrupts branch 3 times, most recently from d1bcf98 to d4efbfb Compare April 9, 2025 12:32

Ali-Faraz-10xe force-pushed the m_interrupts branch 2 times, most recently from 70d8faf to 6a00d29 Compare April 9, 2025 12:40

UmerShahidengr and others added 9 commits April 9, 2025 18:04

Updated arch_test.h : Converted Xstatus.IE, Xstatus.PIE, XxIE and XxI…

fbd8e05

…P into vector signature Signed-off-by: Umer Shahid <[email protected]>

Update CHK-M-INTR-PRIORITY.S : Removed nop after SREG inst

45b2608

Signed-off-by: Umer Shahid <[email protected]>

Update CHK-M-SOFTWARE-INTR.S : Disabling Ext & SW Interrupts at the s…

13cbd30

…tart of the test Signed-off-by: Umer Shahid <[email protected]>

Resolve merge conflicts

78cab2d

Update CHK-M-INTR-PRIORITY.S: Enabling global interrupt on last of test

94580bd

Update ALL-TESTS: Resetting/Disabling Interrupts at the start of the …

8c86afe

…tests

Update ALL-TESTS: Removed nop after SREG inst

0bfde62

Update encoding.h: Changed MIE_xxIP to MIE_xxIE for header_file and e…

72dd887

…ncoding file compatibility

Removed Some Trailing Spaces

d7bc642

Ali-Faraz-10xe requested a review from UmerShahidengr April 18, 2025 11:31

UmerShahidengr and others added 2 commits April 18, 2025 18:30

Merge branch 'dev' into m_interrupts

c243d1b

Signed-off-by: Umer Shahid <[email protected]>

Update All_TESTS: Removed macros from the start of each test

bfda014

Ali-Faraz-10xe requested a review from allenjbaum April 21, 2025 05:04

Merge branch 'dev' into m_interrupts

bf918a0

Signed-off-by: Umer Shahid <[email protected]>

UmerShahidengr approved these changes Apr 29, 2025

View reviewed changes

Ali-Faraz-10xe added 2 commits April 29, 2025 22:10

Merge branch 'dev' into m_interrupts

4e64499

Merge branch 'dev' into m_interrupts

ebb1a35

allenjbaum requested changes May 9, 2025

View reviewed changes

UmerShahidengr and others added 2 commits May 23, 2025 09:58

Merge branch 'dev' into m_interrupts

5992038

Merge branch 'dev' into m_interrupts

0a45f55

[ACT] ACTs and coverage points for machine mode software and timer interrupts #567

Are you sure you want to change the base?

[ACT] ACTs and coverage points for machine mode software and timer interrupts #567

Uh oh!

Conversation

Ali-Faraz-10xe commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Ratified/Unratified Extensions

List Extensions

Reference Model Used

DUT Model Used

Mandatory Checklist:

Optional Checklist:

Uh oh!

allenjbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

allenjbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UmerShahidengr left a comment

Choose a reason for hiding this comment

Uh oh!

allenjbaum left a comment

Choose a reason for hiding this comment

Uh oh!

allenjbaum Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

allenjbaum Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allenjbaum May 9, 2025

Choose a reason for hiding this comment

Uh oh!

allenjbaum May 9, 2025

Choose a reason for hiding this comment

Uh oh!

allenjbaum May 9, 2025

Choose a reason for hiding this comment

Uh oh!

allenjbaum May 9, 2025

Choose a reason for hiding this comment

Uh oh!

allenjbaum May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ali-Faraz-10xe commented Nov 21, 2024 •

edited

Loading