[prim_fifo_sync,rtl] Specialize for Depth == 1 #25461
Conversation
rswarbrick
force-pushed
the
singleton-fifo
branch
from
b04b8ba to
9ded42f
Compare
|
Force-push wires up the |
rswarbrick
force-pushed
the
singleton-fifo
branch
from
9ded42f to
1cf96ea
Compare
|
Force-push wires up the other output that I'd missed... Thinking about |
rswarbrick
force-pushed
the
singleton-fifo
branch
from
1cf96ea to
76bb460
Compare
|
Thanks for the review, @matutem. I've addressed the comments (I hope) and force-pushed. I think there's still a question about security hardening and I'm hoping someone who knows more about security than me (@andreaskurth? @vogelpi?) would be able to weigh in. |
There was a problem hiding this comment.
I am not sure I understand the motivation of this PR. It doesn't mean I cannot support it but I would first like to understand before spending time on reviewing this. As noted in the discussion, this can have security impact, so a careful review is needed here.
@rswarbrick can you please clarify? My understanding is that with the current design and a depth of 0, we instantiate the prim_fifo_sync_cnt modules even though they are not used - there is a single module storage entry anyway and we always write that one and read from it. The prim_fifo_sync_cnt modules mean some waste of area due to the prim_flops inside and coverage cannot be closed for this as is. Is this correct?
|
Yes, I think that is correct, which is the reason for @matutem's original issue (I think). |
There was a problem hiding this comment.
The unhardened implementation looks good to me but I think adding a second flop (using prim_flop) in the case Secure is set to 1 would make sense.
Can you please also check if there are any instances in the design with Depth == 1 and Secure == 1? Because for blocks using such instantiations we have very likely some FI tests that will break with this PR. Because the faults are typically injected into either the read or the write pointers which are no longer there with this PR.
|
I've just worked through the tree a bit to search for examples of this. And I've found at least some! The three fifos in |
rswarbrick
force-pushed
the
singleton-fifo
branch
from
76bb460 to
9354116
Compare
|
I've just pushed up a version with the duplicated 1-bit counter that I think we'd need. Looking closely, I don't think this will actually break any testing. But that's because we do the tests by injecting things into instances of Clearly, it would cause the testing to miss security holes! So I'm going to poke around now and see whether I can replicate the testing sensibly in this case. |
rswarbrick
force-pushed
the
singleton-fifo
branch
from
9354116 to
2461469
Compare
rswarbrick
requested review from
marnovandermaas
and removed request for
a team
|
Well, that was harder than I expected! I've just force pushed with a much larger commit (sorry) that I'm hoping will fix lots of the build failures with the previous version. Frustratingly, I can't really see an easy way to split it up. |
rswarbrick
force-pushed
the
singleton-fifo
branch
3 times, most recently
from
2fce3e1 to
8fddc7a
Compare
There was a problem hiding this comment.
Thanks @rswarbrick for going the extra miles with the automated countermeasure tests, this helps keeping the nightlies healthy and avoids someone else debugging afterwards.
I think for the entropy complex (EDN, CSRNG, ENTROPY_SRC) we do have non-automated tests injecting errors into all FIFOs, see e.g. csrng_err_vseq.sv. I am not sure but I believe these sequences will now fail as the paths into the depth == 1 FIFOs no longer exists. This is only detected at runtime. Can you please run full regressions for these three blocks to be sure? I think a 10% regression should be sufficient to hit these cases.
|
Ah and for the new DV interfaces and assertions, I would appreciate if @matutem could take another look please. |
rswarbrick
force-pushed
the
singleton-fifo
branch
from
8fddc7a to
e662912
Compare
This case is pretty simple to reason about, and avoids needing proper read and write addresses (because there is only one element). Unfortunately, it's quite a big commit! This is because we end up changing some of the duplicated counters inside the fifo. Items in the commit: - Update the RTL in prim_fifo_sync.sv to specialize as you'd expect. - Add an interface that can be bound in to prim_fifo_sync.sv and will register a way for sec_cm tests to use forced signals to simulate fault injections. - Bind that interface into the prim_fifo_sync instances - Wrap up the ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT calls that are used for prim_fifo_sync instances. After the change, the two macro calls that were previously used (for the read and write pointer) are now handled by a single macro. In instances with Depth >= 2, this turns into ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT. For singleton instances, it's ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1. I don't *think* you can make the dispatch between the two options automatic (because the pre-processor can't know the values of the Depth parameter). - Use these new macros in the various blocks that were using prim_fifo_sync in secure mode with Depth = 1. This is CSRNG, OTBN and flash_ctrl. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
singleton-fifo
branch
from
e662912 to
a796984
Compare
This case is pretty simple to reason about, and avoids needing proper read and write addresses (because there is only one element).
This has seen some FPV testing (
util/dvsim/dvsim.py hw/top_earlgrey/formal/top_earlgrey_fpv_prim_cfgs.hjson --select-cfg=prim_fifo_sync_fpv) and passes happily. This was inspired by #25419 (or, more precisely, I was!) so I thought it made sense to implement.Closes #25419.