Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[edn] Improve FSM coverage #23092

Merged
merged 3 commits into from
May 14, 2024
Merged

[edn] Improve FSM coverage #23092

merged 3 commits into from
May 14, 2024

Conversation

vogelpi
Copy link
Contributor

@vogelpi vogelpi commented May 13, 2024

This PR contains two commits that help improving FSM coverage:

  1. [edn/rtl] Make Error main SM state terminal again: This is actually a security-relevant bug fix. Previously, the hardware could be made to leave the terminal error state upon receiving an error status signal from CSRNG.
  2. [edn/dv] Fix FSM coverage hole: There were some systematic coverage holes in the err_test of EDN DV. This commit fixes those.

A third commit has been appended to fix Verilator lint warnings and errors.

Previously, it was possible to move out of the "terminal" error state
upon receiving a CSRNG ack error. However, we don't verify this and
it should not be possible. The Error state needs to be terminal.

Signed-off-by: Pirmin Vogel <[email protected]>
@vogelpi vogelpi requested a review from a team as a code owner May 13, 2024 22:38
@vogelpi vogelpi requested review from marnovandermaas, andreaskurth and h-filali and removed request for a team May 13, 2024 22:38
@vogelpi
Copy link
Contributor Author

vogelpi commented May 14, 2024

Regression results with coverage below. The FSM coverage is indeed 5% higher now but we also need to work on the alert test as discussed in #22352.

EDN Simulation Results

Monday May 13 2024 22:35:07 UTC

GitHub Revision: 6021c33c18

Branch: edn-fsm-cov

Testplan

Simulator: VCS

Test Results

Stage Name Tests Max Job Runtime Simulated Time Passing Total Pass Rate
V1 smoke edn_smoke 3.250s 25.534us 50 50 100.00 %
V1 csr_hw_reset edn_csr_hw_reset 0.740s 46.623us 5 5 100.00 %
V1 csr_rw edn_csr_rw 0.610s 15.685us 20 20 100.00 %
V1 csr_bit_bash edn_csr_bit_bash 3.740s 259.066us 5 5 100.00 %
V1 csr_aliasing edn_csr_aliasing 1.130s 113.478us 5 5 100.00 %
V1 csr_mem_rw_with_rand_reset edn_csr_mem_rw_with_rand_reset 1.050s 24.852us 20 20 100.00 %
V1 regwen_csr_and_corresponding_lockable_csr edn_csr_rw 0.610s 15.685us 20 20 100.00 %
edn_csr_aliasing 1.130s 113.478us 5 5 100.00 %
V1 TOTAL 105 105 100.00 %
V2 firmware edn_genbits 1.738m 4.491ms 300 300 100.00 %
V2 csrng_commands edn_genbits 1.738m 4.491ms 300 300 100.00 %
V2 genbits edn_genbits 1.738m 4.491ms 300 300 100.00 %
V2 interrupts edn_intr 3.350s 22.737us 50 50 100.00 %
V2 alerts edn_alert 4.340s 90.683us 50 50 100.00 %
V2 errs edn_err 3.690s 310.242us 100 100 100.00 %
V2 disable edn_disable 12.180s 500.000us 49 50 98.00 %
edn_disable_auto_req_mode 3.440s 35.655us 50 50 100.00 %
V2 stress_all edn_stress_all 15.560s 624.554us 50 50 100.00 %
V2 intr_test edn_intr_test 0.730s 12.215us 50 50 100.00 %
V2 alert_test edn_alert_test 3.310s 16.727us 50 50 100.00 %
V2 tl_d_oob_addr_access edn_tl_errors 3.260s 2.075ms 20 20 100.00 %
V2 tl_d_illegal_access edn_tl_errors 3.260s 2.075ms 20 20 100.00 %
V2 tl_d_outstanding_access edn_csr_hw_reset 0.740s 46.623us 5 5 100.00 %
edn_csr_rw 0.610s 15.685us 20 20 100.00 %
edn_csr_aliasing 1.130s 113.478us 5 5 100.00 %
edn_same_csr_outstanding 1.080s 114.978us 20 20 100.00 %
V2 tl_d_partial_access edn_csr_hw_reset 0.740s 46.623us 5 5 100.00 %
edn_csr_rw 0.610s 15.685us 20 20 100.00 %
edn_csr_aliasing 1.130s 113.478us 5 5 100.00 %
edn_same_csr_outstanding 1.080s 114.978us 20 20 100.00 %
V2 TOTAL 789 790 99.87 %
V2S tl_intg_err edn_sec_cm 4.850s 932.199us 5 5 100.00 %
edn_tl_intg_err 1.850s 158.174us 20 20 100.00 %
V2S sec_cm_config_regwen edn_regwen 1.000s 17.023us 10 10 100.00 %
V2S sec_cm_config_mubi edn_alert 4.340s 90.683us 50 50 100.00 %
V2S sec_cm_main_sm_fsm_sparse edn_sec_cm 4.850s 932.199us 5 5 100.00 %
V2S sec_cm_ack_sm_fsm_sparse edn_sec_cm 4.850s 932.199us 5 5 100.00 %
V2S sec_cm_fifo_ctr_redun edn_sec_cm 4.850s 932.199us 5 5 100.00 %
V2S sec_cm_ctr_redun edn_sec_cm 4.850s 932.199us 5 5 100.00 %
V2S sec_cm_main_sm_ctr_local_esc edn_alert 4.340s 90.683us 50 50 100.00 %
edn_sec_cm 4.850s 932.199us 5 5 100.00 %
V2S sec_cm_cs_rdata_bus_consistency edn_alert 4.340s 90.683us 50 50 100.00 %
V2S sec_cm_tile_link_bus_integrity edn_tl_intg_err 1.850s 158.174us 20 20 100.00 %
V2S TOTAL 35 35 100.00 %
V3 stress_all_with_rand_reset edn_stress_all_with_rand_reset 1.473h 162.188ms 50 50 100.00 %
V3 TOTAL 50 50 100.00 %
TOTAL 979 980 99.90 %

Coverage Results

Coverage Dashboard

SCORE LINE COND TOGGLE FSM BRANCH ASSERT GROUP
94.79 % 98.24 % 93.78 % 97.02 % 85.47 % 96.62 % 99.77 % 92.66 %

Failure Buckets

  • UVM_FATAL (uvm_phase.svh:1512) [PH_TIMEOUT] Explicit timeout of * ps hit, indicating a probable testbench issue has 1 failures:
    • Test edn_disable has 1 failures.
      • 21.edn_disable.28745602482555654232114957843993826852934868471929246159080802735941029427796
        Line 73, in log /home/pirmin/ot/opentitan/scratch/edn-fsm-cov/edn-sim-vcs/21.edn_disable/latest/run.log

          UVM_FATAL @ 500000000 ps: (uvm_phase.svh:1512) [PH_TIMEOUT] Explicit timeout of 500000000 ps hit, indicating a probable testbench issue
          UVM_INFO @ 500000000 ps: (uvm_report_catcher.svh:705) [UVM/REPORT/CATCHER]
          --- UVM Report catcher Summary ---
        

INFO: [FlowCfg] [scratch_path]: [edn] [/home/pirmin/ot/opentitan/scratch/edn-fsm-cov/edn-sim-vcs]
ERROR: [dvsim] Errors were encountered in this run.

      [   legend    ]: [Q: queued, D: dispatched, P: passed, F: failed, K: killed, T: total]                                                                                             

00:00:27 [ build ]: [Q: 0, D: 0, P: 2, F: 0, K: 0, T: 2] 100%
01:45:16 [ run ]: [Q: 0, D: 0, P: 979, F: 1, K: 0, T: 980] 100%
01:45:22 [ cov_merge ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
01:45:25 [ cov_report ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%

Copy link

@h-filali h-filali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +184 to +186
// Either move into RejectCsrngEntropy or Error but don't move out of Error as it's terminal.
state_d = local_escalate_i ? Error :
state_q == Error ? Error : RejectCsrngEntropy;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that you caught that.

Copy link

@h-filali h-filali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, accidentally clicked on comment instead of approve...

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@andreaskurth andreaskurth merged commit 7c92d49 into lowRISC:master May 14, 2024
32 checks passed
@vogelpi vogelpi mentioned this pull request Jun 7, 2024
@vogelpi vogelpi deleted the edn-fsm-cov branch June 13, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants