-
Notifications
You must be signed in to change notification settings - Fork 777
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
[sram_ctrl,dv] Add sec_cm_mem_readback test #24699
Conversation
dfa746b
to
ecbfec0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nasahlpa for working on this. In my view, this looks mostly good. I have some comments related to the timing of the forcing/releasing though.
addr_faulty = {addr[TL_AW-1:2], ~addr[1], addr[0]}; | ||
`DV_CHECK(uvm_hdl_force(fi_path, addr_faulty)) | ||
// Release the faulty signal after one clock cycle. | ||
cfg.clk_rst_vif.wait_clks(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be cfg.clk_rst_vif.wait_n_clks(1);
In most cases, we force and release on the negative edge to not interfere with the DUT. The DUT evaluates the signals after the posedge.
// TODO: add list of signals instead of using a single signal. Currently only | ||
// the read or write address is faulted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should actually do this now as we're targeting V3 with this. But it doesn't need to be exhaustive of course. A couple of interesting signals are sufficient. The address is certainly one. Maybe also the write enable and the data?
I think we should also explain here that the list isn't exhaustive and that it doesn't need to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ecbfec0
to
cc0cece
Compare
This commit provides a new sec_cm_readback test that tests the SRAM readback FI countermeasure. In the test, a fault is injected during one of the read/write SRAM requests. The test checks, whether the expected alert fires. Closes lowRISC#23322. Signed-off-by: Pascal Nasahl <[email protected]>
cc0cece
to
8c05c3c
Compare
Thanks for your review @vogelpi and the offline discussion regarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @nasahlpa , this all looks great to me, many thanks!
This commit provides a new sec_cm_readback test that tests the SRAM readback FI countermeasure. In the test, a fault is injected during one of the read/write SRAM requests. The test checks, whether the expected alert fires.
Closes #23322.