remedy endless feedback loop

[majbthrd]

As reported in #80, there is an endless simulation loop in ./src/dm_mem.sv.

In the existing code:

• always_comb "p_hart_ctrl_queue" has a dependency on "halted_aligned" and produces "go", "resume", and "cmdbusy_o"
• always_comb "p_rw_logic" has dependencies on "go", "resume", and "cmdbusy_o" and produces "halted_aligned"

As a result, each always_comb re-evaluates after the other ad infinitum.

Since "go", "resume", and "cmdbusy_o" are simply combinatorial logic dependent only on "state_q", this PR moves this logic to its own always_comb.

[bluewww]

Hardware wise there is no logical loop, but I guess the simulator somehow gets erroneously triggered anyway. Synthesizers don't have a problem with this piece of code.

In xcelium there is actually an option to work around this problem by using the -delay_trigger switch.

I'll see if this breaks anything. I'm not a fan of working around buggy simulators.

[majbthrd]

I am of two minds on this. Ideally, the simulator would have synthesizer-like logic to infer there is not a loop. However, the existing grouping together all these signals into a single always_comb causes there to be an implicit directive that the simulator should re-evaluate when any one of the dependencies in the always_comb changes, so I can understand why the loop is happening.

Thanks.

[bluewww]

Ok I think I know why this is breaking. Some simulators consider the default assignment to a signal at the top of the always_comb block and the (potential) subsequent override in the case statement of it a "event" that triggers other sensitive (to that signal) always_comb blocks. If you do that in two always_comb blocks such that they trigger each other these simulators hit the iteration limit and think its a combinatorial loop (despite not being one).

The solution to this issue is simply not doing a default assignment to these variables, but defining them in each branch of the case statement instead.

[majbthrd]

For what it is worth, I updated the PR to follow in the spirit of what you suggested.

It seems the most minimal change to avoid default values would be to move "go", "resume", and "cmdbusy_o" into the case statement. A healthy subset of these were already set in the case statement, so it seems fairly painless as changes go.

It could still be judged as 'working around buggy simulators', but it is there for your perusal.

[bluewww]

Thanks this looks good I'll give it spin

[christian-lanius]

This pull request fixes some issue observed in the CVA6 project, specifically the one discussed in https://github.com/openhwgroup/cva6/issues/800

Is there any reason not to include this pull request?

[bluewww]

@christian-lanius which simulator and version did you run? I was under the impression this was fixed (in the simulators I looked at)

[christian-lanius]

I am using the master branch of CVA6, which uses submit e19d69efe7 of riscv-dbg. However the latest master branch looks the same in this part of the code.

The issue occurs in QuestaSim 2020.4 and 2021.3 (I have also tried 2018, but that crashes beforehand with an internal error in vlog). The issue does not occur in VCS. The exact infinite loop I get is shown below:

#############  Autofindloop Analysis  ###############
#############  Loop found at time 11390 ns ###############
#   Active process: /ariane_tb/dut/i_dm_top/i_dm_mem/#ASSIGN#114 @ sub-iteration 0
#     Source: /home/lanius/code/cva6_new/corev_apu/riscv-dbg/src/dm_top.sv:191
#   Active process: /ariane_tb/dut/i_dm_top/i_dm_mem/#IMPLICIT-WIRE(cmdbusy_o)#49 @ sub-iteration 1
#     Source: /home/lanius/code/cva6_new/corev_apu/riscv-dbg/src/dm_top.sv:191
#     Assigning reg to (val=0)
#   Active process: /ariane_tb/dut/i_dm_top/i_dm_mem/#ASSIGN#114 @ sub-iteration 2
#     Source: /home/lanius/code/cva6_new/corev_apu/riscv-dbg/src/dm_top.sv:191
################# END OF LOOP #################
# ** Error (suppressible): (vsim-3601) Iteration limit 10000000 reached at time 11390 ns.