[csrng/rtl] add reseed intervall sts err #22883

h-filali · 2024-04-29T16:00:14Z

Please see the commit messages for info.

Resolves #16499

I still need to do some cleanup and debug the last part of the alert test vseq.
There seems to be an issue in DV where the wait for an acknowledgement doesn't behave as
expected.
I also need to rebase.
The first commit can be ignored since it is already merged.

h-filali · 2024-04-30T13:06:39Z

@vogelpi I rebased now. I think this can be reviewed now, although I still get some failing tests. I'm working on resolving the failures.
I also removed part of the DV that doesn't need to be reviewed yet.

vogelpi

Thanks for working on this @h-filali . There are a couple of issues in this PR I would like to discuss with you 1 by 1. Let's set up a meeting.

hw/ip/csrng/data/csrng.hjson

hw/ip/csrng/dv/env/csrng_scoreboard.sv

hw/ip/edn/doc/registers.md

vogelpi · 2024-04-30T13:32:52Z

hw/ip/csrng/rtl/csrng_cmd_stage.sv

+          if (cmd_sts_err_q) begin
+            state_d = Idle;
+            cmd_sts_err_release = 1'b1;


I think you should only release the error when getting the ack. Right now you drop the error upon entering the CmdAck state. What's worse, cmd_sts_err_release doesn't seem to have an effect other than delaying the branch checking cmd_ack_i by one clock cycle. Does this make sense?

I changed it such that the check now happens directly in the cmd_stage. I think it should be better now.

vogelpi · 2024-04-30T13:35:16Z

hw/ip/csrng/rtl/csrng_ctr_drbg_cmd.sv

-         (cmdreq_ccmd == INS) ? {{(CtrLen-1){1'b0}},1'b1} :
-         (cmdreq_ccmd == RES) ? {{(CtrLen-1){1'b0}},1'b1} :
+         (cmdreq_ccmd == INS) ? {{(CtrLen-1){1'b0}},1'b0} :
+         (cmdreq_ccmd == RES) ? {{(CtrLen-1){1'b0}},1'b0} :


As discussed in our 1:1 this is needed for the reseed_interval values to make any sense. Otherwise a value of 2 for reseed_interval would only allow 1 generate command between two reseeds (instead of two).

Yes, I agree that this makes sense.

hw/ip/csrng/rtl/csrng_pkg.sv

vogelpi

Thanks for the update @h-filali . This is going in the right direction but it's not yet working as expected. Please see my comments for details.

vogelpi · 2024-04-30T20:17:32Z

hw/ip/csrng/rtl/csrng_core.sv

+    // Set reseed_cnt_reached_d to true if the max number of generate requests between reseeds
+    // has been reached for the respective counter.
+    assign reseed_cnt_reached_d[ai] = (shid_q == ai) ?
+        (state_db_rd_rc == reg2hw.reseed_interval.q) :
+        reseed_cnt_reached_q[ai];
+


I fear this is not going to work properly when more than 1 instance is busy. Because:

shid_q always updates 1 cycle after acmd_sop which itself is high 1 cycle after the command stage of interest has won arbitration.

But inside the command stage we check reseed_cnt_reached before initiating arbitration.

As a result, the command stage might use reseed_cnt_reached of the wrong instance.

To solve this, I see the following two options:

Feed the reseed counter values for all instances out of the state database to compare them to the limit "live" inside the command stage. This means you need 3 comparators instead of 1, but no muxes and no FFs. It's easy to do and will help us to with upcoming changes for M4.

The check is performed upon writing to the state database instead. This has the same area requirements but there is a risk of the comparison getting out of sync when SW updates the reseed limit in the meantime.

I would go for Option 1.

There are 3 different reseed count reached values. One for each endpoint. They all start off at 0 and for them to be increased, a generate command must have been sent at some point, which means shid_q == ai must have been true at some point and the value must have been updated. I probably should have added a comment explaining this or maybe I'm missing something here. What do you think @vogelpi

Hm, yes thanks for clarifying. However, there is still an issue that the comparison can be out of date. What can happen:

The last Generate command ends and the state database is updated.

The next Generate is received, the command stage checks reseed_cnt_reached_q. It's showing value from when that reseed counter was last read, not when it was written.

It's below the limit, so it's requesting arbitration.

It wins arbitration, shid_q is updated and we read the state database and update reseed_cnt_reached_q.

Now it reached the limit but the Generate is already in the pipeline and we will ack it at the end.

So we are doing one Generate more than we should.

I think an example for when the state is read, is for the error response or the acknowledgement. The command stage needs to read from the state db every time a command is acked, so the reseed_cnt_reached_d variables will be updated any time this happens. For a second generate to be processed, the previous one has to have read from state db first.

I'm now also convinced, that doing it on state_db writes instead of state_db reads is safer. I now changed it to writes.

Thanks @h-filali !

vogelpi · 2024-04-30T20:22:51Z

hw/ip/csrng/rtl/csrng_cmd_stage.sv

@@ -433,13 +449,18 @@ module csrng_cmd_stage import csrng_pkg::*; #(

  assign cmd_stage_ack_o = cmd_ack_q;


What's missing here is to also set cmd_stage_ack_o in case of cmd_err_ack. Right now, just the status is set but the ack is not going out. As a result CSRNG just sucks the command without doing anything. The app interface dies.

Ah yes totally missed that one. Let me fix that.

h-filali · 2024-04-30T20:42:12Z

@vogelpi Thanks for reviewing, I addressed your points. Hopefully this works now

vogelpi

This looks now good, thanks @h-filali ! Let's see what CI has to say to this :-)

marnovandermaas

Looks very reasonable to me. Just a small question on the condition in which the exceeded reseed is triggered.

marnovandermaas · 2024-05-01T08:56:44Z

hw/ip/csrng/rtl/csrng_core.sv

+    // has been reached for the respective counter.
+    assign reseed_cnt_reached_d[ai] =
+        state_db_wr_req && state_db_wr_req_rdy && (state_db_wr_inst_id == ai) ?
+            (state_db_wr_rc == reg2hw.reseed_interval.q) :


Do we want this to be >=? Maybe that case can be reached if we are reseeding based on a large reseed value and then when a lower reseed value is put in the reseed count is already beyond it.

This count goes up for each generate command that is completed. When the reseed_interval is reached no further generates should be accepted. I guess we could add an assertion for that to make sure it doesn't happen. However, I already go another potential PR cooking that tests this.

Ok, it's not a huge deal. I was just thinking it might happen if your counter already has a higher value when you write your reseed interval. We can just put some text in the reseed interval register description saying you should reseed the block to guarantee the new reseed interval takes effect.

Ah now I see, thanks for clarifying. I guess I can do it in a follow up PR once this one is merged.

Yup, that's fine!

h-filali · 2024-05-01T10:07:11Z

Looks very reasonable to me. Just a small question on the condition in which the exceeded reseed is triggered.

Thanks @marnovandermaas for having a look!

vogelpi · 2024-05-02T07:59:00Z

clkmgr_jitter_test_fpga_cw310_sival_rom_ext seems to constantly and reliably timeout with this PR (but not with any others) we should probably investigate this before merging. The test itself is very simple and I suspect the test isn't even reached.

This commit adds a new status error response, that is triggered whenever the number of generates between reseeds exceeds the reseed_interval. Signed-off-by: Hakim Filali <[email protected]>

h-filali requested a review from vogelpi April 29, 2024 16:00

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch 2 times, most recently from 156427c to 79f56a5 Compare April 30, 2024 13:05

vogelpi requested changes Apr 30, 2024

View reviewed changes

vogelpi mentioned this pull request Apr 30, 2024

[csrng] Investigate reseed interval #16499

Closed

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch 3 times, most recently from edda763 to e2247c6 Compare April 30, 2024 19:34

vogelpi reviewed Apr 30, 2024

View reviewed changes

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch from e2247c6 to 078f877 Compare April 30, 2024 20:39

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch from 078f877 to 1ca97d0 Compare April 30, 2024 21:09

vogelpi self-requested a review April 30, 2024 21:12

vogelpi approved these changes Apr 30, 2024

View reviewed changes

vogelpi marked this pull request as ready for review April 30, 2024 21:18

vogelpi requested review from a team as code owners April 30, 2024 21:18

vogelpi requested review from marnovandermaas and pamaury and removed request for a team April 30, 2024 21:18

vogelpi mentioned this pull request Apr 30, 2024

[csrng/rtl] csrng move cmd checks into the cmd stage #22906

Merged

marnovandermaas reviewed May 1, 2024

View reviewed changes

h-filali added the kokoro:rebuild label May 1, 2024

opentitan-github-bot removed the kokoro:rebuild label May 1, 2024

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch from 1ca97d0 to 0417776 Compare May 1, 2024 15:51

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch from 0417776 to a9e28b6 Compare May 2, 2024 14:45

[csrng/rtl] Add reseed interval status error

7b67a48

This commit adds a new status error response, that is triggered whenever the number of generates between reseeds exceeds the reseed_interval. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the csrng-add-reseed-intervall-sts-err branch from a9e28b6 to 7b67a48 Compare May 2, 2024 15:00

vogelpi merged commit 7f84f7c into lowRISC:master May 2, 2024
32 checks passed

timothytrippel mentioned this pull request May 3, 2024

[keymgr] Modify PRNG reseeding #22951

Merged

vogelpi mentioned this pull request Jun 25, 2024

[csrng] V2S Signoff #22467

Closed

h-filali deleted the csrng-add-reseed-intervall-sts-err branch October 7, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[csrng/rtl] add reseed intervall sts err #22883

[csrng/rtl] add reseed intervall sts err #22883

h-filali commented Apr 29, 2024 •

edited

Loading

h-filali commented Apr 30, 2024

vogelpi left a comment

vogelpi Apr 30, 2024

h-filali Apr 30, 2024

vogelpi Apr 30, 2024

h-filali Apr 30, 2024

vogelpi Apr 30, 2024

vogelpi left a comment

vogelpi Apr 30, 2024

h-filali Apr 30, 2024

vogelpi Apr 30, 2024

h-filali Apr 30, 2024

h-filali Apr 30, 2024

vogelpi Apr 30, 2024

vogelpi Apr 30, 2024

h-filali Apr 30, 2024

h-filali commented Apr 30, 2024

vogelpi left a comment

marnovandermaas left a comment

marnovandermaas May 1, 2024

h-filali May 1, 2024 •

edited

Loading

marnovandermaas May 1, 2024

h-filali May 1, 2024

marnovandermaas May 2, 2024

h-filali commented May 1, 2024

vogelpi commented May 2, 2024

		@@ -433,13 +449,18 @@ module csrng_cmd_stage import csrng_pkg::*; #(

		assign cmd_stage_ack_o = cmd_ack_q;

[csrng/rtl] add reseed intervall sts err #22883

[csrng/rtl] add reseed intervall sts err #22883

Conversation

h-filali commented Apr 29, 2024 • edited Loading

h-filali commented Apr 30, 2024

vogelpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-filali commented Apr 30, 2024

vogelpi left a comment

Choose a reason for hiding this comment

marnovandermaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-filali May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-filali commented May 1, 2024

vogelpi commented May 2, 2024

h-filali commented Apr 29, 2024 •

edited

Loading

h-filali May 1, 2024 •

edited

Loading