lowRISC / lowRISC/opentitan

Race condition if use clk_rst_if, so clock not guaranteed to be activated.

Open
#27,719 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

### Description

The initial block at hw/dv/sv/common_ifs/clk_rst_if.sv:298, is written assuming it runs before whatever code makes the call to set_active().

Simulators are allowed to run initial blocks in any order, however.

So if the clk_rst_if's set_active() is also called in an initial block, there is no guarantee that the event set_active_called won't already be triggered before the clk_rst_if instance is set up to wait for it, thus leaving the initial statement blocking forever and the clock unactivated.

This isn't just a hypothetical situation. I found it when I ran the chip_sw_aes_smoketest using the DSim simulator. The race condition in the call to tb.passive_clk_rst_if.set_active() (hw/top_earlgrey/dv/tb/tb.sv:59) resulted in tb.passive_clk_rst_if.clk not being activated and the test never finishing cleanly. There are other tb.sv files that look like they may have a similar clock startup problems.

Simply changing the "@set_active_called" to "wait(set_active_called.triggered) fixed the problem with chip_sw_aes_smoketest.

If consistent with your design intent, please consider the following change:

```
diff --git a/hw/dv/sv/common_ifs/clk_rst_if.sv b/hw/dv/sv/common_ifs/clk_rst_if.sv
index ae99c0a4dc..b8d63af51d 100644
--- a/hw/dv/sv/common_ifs/clk_rst_if.sv
+++ b/hw/dv/sv/common_ifs/clk_rst_if.sv
@@ -300,7 +300,7 @@ interface clk_rst_if #(
// wait a whole number of clock periods, which means it's possible for the clock to synchronise
// with the "expected" timestamps.
bit done;
- @set_active_called;
+ wait(set_active_called.triggered);
if (drive_clk) begin
fork
begin

```

(Using current master - hash 89baefa2c64de902c2e38f138020e1ff1df5484b - as reference)

Contributor guide

Open the contributing guide

Research direction

Start in hw/dv/sv/common_ifs/clk_rst_if.sv around line 298 and compare the clock startup with tb.sv, especially hw/top_earlgrey/dv/tb/tb.sv:59. Run chip_sw_aes_smoketest with the relevant simulator to reproduce the startup race. Done means the passive clock is activated reliably and the test finishes cleanly.

Written by the indexing model from the issue text.

Assessment

Domain
embedded-iot
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.