Resubmitted DCLS val updates, updates to regress scripts and Makefile by samipmodi323 · Pull Request #468 · chipsalliance/Cores-VeeR-EL2

samipmodi323 · 2026-06-23T16:01:22Z

Added dcls coverage under testbench/coverage/fcov - Samip
Updates Makefile - Jinal
added ability to print sim time even in c code - Jinal
updated .github/scripts/regression scripts - Samip/Jinal

github-actions · 2026-06-25T17:26:46Z

Coverage report for this PR is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/coverage_dashboard/all, documentation is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/docs_rendered/html

github-actions · 2026-06-26T16:29:48Z

Coverage report for this PR is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/coverage_dashboard/all, documentation is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/docs_rendered/html

nasahlpa

Thanks @samipmodi323. A couple of things that I personally would change:

A git commit message on each commit would help the reviewer to understand why the commit is needed
Separating changes that are related into multiple commits
Rebasing the branch instead of merging it

nasahlpa · 2026-06-29T05:39:29Z

+                76: force `LOCKSTEP.dma_hresp = '1;
+                // --- END ADDED AHB FORCES ---
+                // --- ADDED UNCONDITIONAL FORCES (AHB) ---
+                77: force `LOCKSTEP.o_cpu_halt_ack = '1;


This fore already exists for the AXI case as well. Instead of duplicating it here, could we use force only once?

Forces consolidated in the latest push from 29-June

nasahlpa · 2026-06-29T05:41:47Z

+#don't override here.  TODO: a better place to randomize DELAY/MUBI params
+#ifneq ($(findstring dcls,$(TEST)),)
+#lockstep_enable ?= 1
+#lockstep_regfile_enable ?= 1
+#endif
+#
+#ifdef lockstep_enable
+# override CONF_PARAMS += -set lockstep_enable=1
+# ifdef lockstep_regfile_enable
+#  override CONF_PARAMS += -set lockstep_regfile_enable=1
+# endif
+# ifndef lockstep_delay
+#	MIN:= 2
+#	MAX:= 4
+#  lockstep_delay := $(shell bash -c 'echo $$(( ( $$RANDOM % ($(MAX) - $(MIN) + 1) ) + $(MIN) ))')
+# endif
+# override CONF_PARAMS += -set lockstep_delay=$(lockstep_delay)
+#endif


Is this needed? As it is commented out probably not - so I would recommend deleting it.

Removed in the latest push from 29-June

github-actions · 2026-06-30T04:58:12Z

Coverage report for this PR is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/coverage_dashboard/all, documentation is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/docs_rendered/html

moidx

Once you address the comments, please amend the last commit and force push instead of adding one more commit. Thanks.

nasahlpa · 2026-06-30T16:23:57Z

@samipmodi323 would it be possible to just have a single commit in this PR and add a descriptive commit message?

nasahlpa

I think this PR needs a bit more work. The covergroups currently do not consider the multi-bit encoding.

nasahlpa · 2026-06-30T16:33:34Z

+    .corruption_detected(corruption_detected),
+`ifdef RV_LOCKSTEP_REGFILE_ENABLE


Does this code work for the case when If RV_LOCKSTEP_REGFILE_ENABLE is not set? I think then we have a stray comma at line 20.

Thank for this review. I have pushed change for this. And as mentioned overall, the fcov will change in my follow on turning with Mubi added

nasahlpa · 2026-06-30T16:36:29Z

        if (old_boot_count == (2*2) || old_boot_count == (3*2) || old_boot_count == (6*2) ||
            old_boot_count == (9*2) || old_boot_count == (10*2) || old_boot_count == (13*2) ||
            old_boot_count == (18*2) || old_boot_count == (19*2) || old_boot_count == (28*2) ||
+            (old_boot_count >= (33*2) && old_boot_count <= (33*2) && (old_boot_count % 2 == 0)) ||  //skip VEER side Unconditional forces to prevent breakage in code exectuion


Can't this be simplified to old_boot_count == (33*2) ?

My intent was just to keep the flow same as few other places I have this >= & <=.

updated in my latest push

nasahlpa · 2026-06-30T16:39:08Z

    echo -e "${COLOR_WHITE}==================== running test '${NAME}' ====================${COLOR_CLEAR}"

-    for COVERAGE in branch toggle functional; do
+#    for COVERAGE in branch toggle functional; do


Would it be possible to please not comment out code? Same below at line 35.

Done in my latest push from July1

nasahlpa · 2026-06-30T16:46:25Z

+    input logic core_rst_l,
+    input logic lockstep_err_injection_en_i,
+    input logic disable_corruption_detection_i,
+    input logic corruption_detected_o,


I've mentioned this already in my previous review, aren't those signals multi-bit encoded signals?

In my opinion, this could should work with different multi bit encodings.

Hi, I have the updates in my follow on turnin...https://github.com/chipsalliance/Cores-VeeR-EL2/pull/479/changes#diff-58450bb2dc27f6c231cec64be3969c2fb25634ac43ddef3de0f7c758a9635802

I would not recommend merging code into the repository that does not work. I would recommend either fixing it here in this pull request or removing the functional coverage code and pushing it in the other PR.

In my latest PR, I have removed the fcov completely. In my next PR, it will be added with MUBI. Thanks

nasahlpa · 2026-06-30T16:47:22Z

+
+  // Error injection and corruption enable path
+  err_injection_vs_detection: coverpoint {lockstep_err_injection_en_i, disable_corruption_detection_i, corruption_detected_o} {
+    bins inject_detected = {3'b111};


This is something I already have flagged in my previous review - those signals are multi-bit signals. Simply using 1 here does not work.

Hi, I have the updates in my follow on turnin...https://github.com/chipsalliance/Cores-VeeR-EL2/pull/479/changes#diff-58450bb2dc27f6c231cec64be3969c2fb25634ac43ddef3de0f7c758a9635802

nasahlpa · 2026-06-30T17:00:15Z

    # USER_MODE - '1' for user mode, '0' for without user mode
    # CACHE WAYPACK -
-    check_args_count $# 6
+    # SIMULATOR - (Optional) 'verilator' (default) or 'vcs'


As this script is meant to be run in CI - is the intention of this change to run the regression with vcs at a later point?

No such intention, but the script is updated to also support VCS. I am using this script also to run tests locally with VCS, thus the support is needed.

…date axi/ahb forces and also updated dcls.c to reflect it

github-actions · 2026-07-01T23:20:23Z

Coverage report for this PR is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/coverage_dashboard/all, documentation is available at https://chipsalliance.github.io/Cores-VeeR-EL2//html/dev/468/docs_rendered/html

samipmodi323 mentioned this pull request Jun 23, 2026

DCLS val updates, Makefile/regress scripts update #461

Closed

samipmodi323 force-pushed the dev/samipmodi323/dcls_val_updates2 branch from 590f2b7 to 57d2ee6 Compare June 24, 2026 02:49

samipmodi323 marked this pull request as ready for review June 26, 2026 19:33

samipmodi323 requested review from moidx and nasahlpa June 26, 2026 19:33

nasahlpa reviewed Jun 29, 2026

View reviewed changes

Resubmitted DCLS val updates, updates to regress scripts and Makefile

27c6acc

samipmodi323 force-pushed the dev/samipmodi323/dcls_val_updates2 branch from 3f10d3f to abef2ca Compare June 29, 2026 18:41

moidx reviewed Jun 30, 2026

View reviewed changes

Comment thread testbench/coverage/fcov/dcls_fcov.sv Outdated

Comment thread testbench/coverage/fcov/dcls_fcov_bind.sv Outdated

samipmodi323 force-pushed the dev/samipmodi323/dcls_val_updates2 branch 3 times, most recently from 5e55989 to 3a8d00b Compare June 30, 2026 14:44

moidx previously approved these changes Jun 30, 2026

View reviewed changes

moidx requested a review from nasahlpa June 30, 2026 14:55

nasahlpa reviewed Jun 30, 2026

View reviewed changes

samipmodi323 dismissed moidx’s stale review via f18cfee June 30, 2026 21:49

samipmodi323 force-pushed the dev/samipmodi323/dcls_val_updates2 branch from 3a8d00b to f18cfee Compare June 30, 2026 21:49

Makefile cleanup to remove comments, updated tb_top to remove consoli…

54b60a8

…date axi/ahb forces and also updated dcls.c to reflect it

samipmodi323 force-pushed the dev/samipmodi323/dcls_val_updates2 branch from f18cfee to 54b60a8 Compare July 1, 2026 13:53

		.corruption_detected(corruption_detected),
		`ifdef RV_LOCKSTEP_REGFILE_ENABLE

Uh oh!

Conversation

samipmodi323 commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

github-actions Bot commented Jun 26, 2026

Uh oh!

nasahlpa 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!

Uh oh!

github-actions Bot commented Jun 30, 2026

Uh oh!

moidx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nasahlpa commented Jun 30, 2026

Uh oh!

nasahlpa 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!

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!

github-actions Bot commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants