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

[rom_ctrl, dv] Conditional coverage hole of rom_ctrl_mux #25798

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

KinzaQamar
Copy link
Contributor

rom_ctrl coverage report contains a hole occurred in rom_ctrl_mux.sv by a conditional statement (alert_d = sel_invalid | sel_reverted | sel_q_reverted) as 001 not covered.

This PR fixes corrupt_sig_fatal_vseq to force sel_bus_qq in rom_ctrl_mux to an invalid value to get sel_q_reverted = 1. Then to make sure that we inject something bad to sel_bus_qq before ROM check finishes, the fix adds force_early variable.

Lastly, changed the name err_point to inject_after_done for better readibility.

@KinzaQamar KinzaQamar marked this pull request as ready for review January 7, 2025 16:23
@KinzaQamar KinzaQamar requested a review from a team as a code owner January 7, 2025 16:23
@KinzaQamar KinzaQamar requested review from eshapira and rswarbrick and removed request for a team and eshapira January 7, 2025 16:23
rom_ctrl coverage report contains a hole occurred in rom_ctrl_mux.sv by
a conditional statement (alert_d = sel_invalid | sel_reverted |
sel_q_reverted) as 001 not covered.

This PR fixes corrupt_sig_fatal_vseq to force sel_bus_qq in rom_ctrl_mux
to an invalid value to get sel_q_reverted = 1. Then to make sure that we
inject something bad to sel_bus_qq before ROM check finishes, the fix
adds force_early variable.

Lastly, changed the name err_point to inject_after_done for better
readibility.

Signed-off-by: Kinza Qamar <[email protected]>
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

This looks sensible, but don't we need to add a new possible value for cm_id? With this change on its own, we are no longer going to force sel_bus_q?

@rswarbrick
Copy link
Contributor

Duh, sorry! I'm convinced by this change after all. (For future me: There are three values A,B,C and we check A==B and B==C. We want to see each of those checks failing separately, so poking B won't help)

@rswarbrick rswarbrick merged commit 81ebe5f into lowRISC:master Jan 9, 2025
38 checks passed
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.

2 participants