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

sfifo_observe_gate_q is stuck at reset in entropy_src #503

Closed
Nitsirks opened this issue Apr 17, 2024 · 3 comments · Fixed by #506
Closed

sfifo_observe_gate_q is stuck at reset in entropy_src #503

Nitsirks opened this issue Apr 17, 2024 · 3 comments · Fixed by #506
Labels
bug Something isn't working

Comments

@Nitsirks
Copy link
Contributor

This register sfifo_observe_gate_q never gets out of reset because the set condition can't ever be true. The observe push is qualified by the observe full already, so we never detect when there was an attempt to push the fifo while it was full.

@moidx
Copy link
Contributor

moidx commented Apr 17, 2024

Hi @vogelpi, can we get your input here?

sfifo_observe_push is qualified by !sfifo_observe_full

// fifo controls
assign sfifo_observe_push = fw_ov_mode && pfifo_postht_pop && !sfifo_observe_full &&
(sfifo_observe_gate_q || !sfifo_observe_not_empty);

but then used in a condition with sfifo_observe_full, which triggers a linter error:

assign sfifo_observe_gate_d = (sfifo_observe_push && sfifo_observe_full) ? 1'b0 :
!sfifo_observe_not_empty ? 1'b1 :
sfifo_observe_gate_q;

This condition was updated in lowRISC/opentitan#21799, but the team is wondering if there is an incremental way to solve this problem without having to rebase the entire block.

@howardtr, it may be worth discussing the changes made to the entropy complex in the past six months. There have been many fixes introduced for issues found during DV and silicon testing. We can try to summarize the issues so that you can make a call on whether or not to cherry pick changes.

@howardtr
Copy link
Collaborator

@moidx - Thanks for summarizing the issue above

We'll certainly pick up the changes for the entropy block for caliptra 2.0 (next release), but for caliptra 1.1, we're trying to find an incremental set of changes as you mentioned

@vogelpi
Copy link

vogelpi commented Apr 18, 2024

Hi,
21799 added another FIFO to deal with internal backpressure. Since this FIFO is inserted before the Observe FIFO, the gating condition needed to be updated. But it's a rather big change not easy to port over.

However, there was another PR before 21799 which fixed the gating condition issue. See lowRISC/opentitan#21640 . This one should be easy to cherry-pick over.

For your next release, I definitely recommend taking everything over including the big PR. As @moidx pointed out, there have been many changes improving the functionality and the verification of the module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants