[csrng/rtl] Propagate csrng errors to edn through cmd sts signal #21981

h-filali · 2024-03-12T16:27:07Z

This PR adds functionality to signal status errors along a command acknowledgement.
For further details have a look at the individual commit messages.

Resolves #16516

andreaskurth

Thanks for this work, @h-filali. I think this is almost ready, just a few questions and suggestions from my side

andreaskurth · 2024-03-25T07:08:33Z

hw/ip/csrng/rtl/csrng_ctr_drbg_cmd.sv

+  // Return a status error when the state is not zeroized properly during a UNI command.
+  assign ctr_drbg_cmd_sts_err = sfifo_keyvrc_pop && (ctr_drbg_cmd_ccmd_o == UNI) &&
         ((KeyLen == '0) && (BlkLen == '0) && (CtrLen == '0));


I don't really understand this condition. Which part checks the 'not zeroized properly'? And why are we comparing the length synthesis parameters to zero?

I tried to change as little as possible but I agree with your point. I'll adapt the check such that it actually checks if we are zeroizing properly.

andreaskurth · 2024-03-25T07:09:43Z

hw/ip/csrng/rtl/csrng_ctr_drbg_gen.sv

+  // Return a status error when the genbits FIFO is popped while ctr_drbg_gen_ccmd_o is not
+  // equal to GEN.
+  assign ctr_drbg_gen_sts_err = sfifo_genbits_pop && (
         (ctr_drbg_gen_ccmd_o == INV) ||
         (ctr_drbg_gen_ccmd_o == INS) ||
         (ctr_drbg_gen_ccmd_o == RES) ||
         (ctr_drbg_gen_ccmd_o == UPD) ||
         (ctr_drbg_gen_ccmd_o == UNI));


Shouldn't we have ctr_drbg_gen_ccmd_o != GEN, in line with the comment?

Good point! I'll change it.

andreaskurth · 2024-03-25T07:22:44Z

hw/ip/csrng/rtl/csrng_pkg.sv

@@ -22,17 +22,24 @@ package csrng_pkg;
    logic                       genbits_ready;
  } csrng_req_t;

+  typedef enum logic [1:0] {


Suggest introducing a localparam that defines the width of this enum (reason: see next comment)

andreaskurth · 2024-03-25T07:23:30Z

hw/ip/csrng/rtl/csrng_state_db.sv

  output logic [StateId-1:0] state_db_sts_id_o
 );

  localparam int InternalStateWidth = 2+KeyLen+BlkLen+CtrLen;
  localparam int RegInternalStateWidth = 30+InternalStateWidth;
  localparam int RegW = 32;
-  localparam int StateWidth = 1+1+KeyLen+BlkLen+CtrLen+StateId+1;
+  localparam int StateWidth = 1+1+KeyLen+BlkLen+CtrLen+StateId+2;


Suggest using the (newly introduced) localparam that defines the width of csrng_cmd_sts_e as last summand

Excellent point, I totally missed that.

andreaskurth · 2024-03-25T08:22:09Z

hw/ip/edn/data/edn.hjson

+                0b00: Request completed successfully.
+                0b01: Request completed with an invalid application command error.
+                      This error indicates that the issued application command doesn't represent a valid operation.
+                      If this error appears, the main state machine will hang and the entropy complex has to be restarted.
+                0b10: Request completed with an invalid state parameter error.
+                      This error indicates that the state wasn't zeroized properly after an uninstantiate command.
+                      In this case the entropy complex should be restarted to properly zeroize the state.
+                0b11: Request completed with an invalid counter drbg generation command error.
+                      This error indicates that CSRNG entropy was generated for a command that is not a generate command.
+                      In this case the entropy should not be considered as valid.


Instead of duplicating this information from CSRNG, would it makes sense to refer to the CSRNG doc (like for HW_CMD_STS.CMD_TYPE below)?

Yes it would.

andreaskurth · 2024-03-25T08:23:02Z

hw/ip/edn/data/edn.hjson

-                0b1: Request completed with an error.
+                This field contains the application command type of the hardware controlled command issued last.
+                The application command selects one of five operations to perform.
+                A description of the application command types can be found [here](https://opentitan.org/book/hw/ip/csrng/doc/theory_of_operation.html#command-description).


Suggest to use relative paths (like ../../csrng/doc/theory_of_operation.md#command-description) instead, because that will work with local files, and it will work if we change the web interface to the documentation

andreaskurth · 2024-03-25T08:23:21Z

hw/ip/edn/data/edn.hjson

+                0b00: Request completed successfully.
+                0b01: Request completed with an invalid application command error.
+                      This error indicates that the issued application command doesn't represent a valid operation.
+                      If this error appears, the main state machine will hang and the entropy complex has to be restarted.
+                0b10: Request completed with an invalid state parameter error.
+                      This error indicates that the state wasn't zeroized properly after an uninstantiate command.
+                      In this case the entropy complex should be restarted to properly zeroize the state.
+                0b11: Request completed with an "invalid counter drbg generation command" error.
+                      This error indicates that CSRNG entropy was generated for a command that is not a generate command.
+                      In this case the entropy should not be considered as valid.


Ditto referencing the CSRNG doc?

andreaskurth · 2024-03-25T08:34:30Z

hw/dv/sv/csrng_agent/csrng_agent_cfg.sv

-  bit    cmd_zero_delays;
-  bit    under_reset;
-  bit    rsp_sts_err;
+  uint              min_cmd_ack_dly, max_cmd_ack_dly,


Wouldn't it be better to combine this commit with the first one, so that tests continue to pass despite the RTL changes?

Sure, I'll squash.

andreaskurth · 2024-03-25T08:35:16Z

hw/ip/csrng/doc/registers.md

@@ -157,30 +157,51 @@ Command request register
 ## SW_CMD_STS
 Application interface command status register
 - Offset: `0x1c`
- Reset default: `0x1`
- Reset mask: `0x3`
+- Reset default: `0x0`


I'd also combine this commit with the first one because otherwise documentation will be outdated

andreaskurth · 2024-03-25T08:35:53Z

sw/device/lib/crypto/drivers/entropy.c

@@ -450,9 +450,9 @@ static status_t csrng_send_app_cmd(uint32_t base_address,
        reg = abs_mmio_read32(kBaseCsrng + CSRNG_INTR_STATE_REG_OFFSET);
      } while (!bitfield_bit32_read(reg, CSRNG_INTR_STATE_CS_CMD_REQ_DONE_BIT));

-      // Check the "status" bit, which will be 1 only if there was an error.
+      // Check the "status" bit, which will be 0 unless there was an error.


I'd also combine this commit with the first one, because otherwise SW tests will fail

andreaskurth · 2024-03-25T09:59:32Z

hw/ip/csrng/data/csrng.hjson

-                0b1: Request completed with an error
+                To check whether a command was succesful, wait for !!INTR_STATE.CS_CMD_REQ_DONE or
+                !!SW_CMD_STS.CMD_ACK to be high and then check the value of this field.
+                0b00: Request completed successfully.


I just saw in #22114 that you're changing the representation of the values from binary to hex, which I think makes a lot of sense and could already be done in this PR

Sure, done!

h-filali · 2024-03-25T10:11:38Z

@andreaskurth I addressed all your points and rebased to the current master. Thanks a lot for reviewing!

This commit changes the sw_cmd_sts and hw_cmd_sts signals to multiple bits. 0 represents a successful acknowledgement, whereas any value other than 0 represents an error status. The sts signal is now also able to anounce to the EDN that the CSRNG main SM is hanging due to an invalid acmd. This commit also aligns the EDN rtl with the new sts response. Furthermore a new csrng interface pkg is created to avoid circular dependencies. This commit also aligns the EDN and CSRNG DV, documentation and SW with the new STS signal. Signed-off-by: Hakim Filali <[email protected]>

This commit removes the csrng_fsm_idle_wait() in csrng_send_app_cmd(). This was needed because the cmd_rdy signal was high whenever the CSRNG was turned off. Due to a fix in lowRISC#21981 this is no longer needed. Signed-off-by: Hakim Filali <[email protected]>

vogelpi

Thanks for the PR @h-filali ! It looks mostly good to me but I have some questions. Regarding the zeroizing check, I am more in favor of removing that entirely but let's discuss once you're back from vacation and exams.

vogelpi · 2024-04-01T21:36:02Z

sw/device/lib/dif/dif_edn.c

-      bit = EDN_SW_CMD_STS_CMD_STS_BIT;
+      field_val = bitfield_field32_read(reg, EDN_SW_CMD_STS_CMD_STS_FIELD);
+      *set = field_val ? true : false;


IIUC, here we fold all non-zero values to True, meaning the reported status becomes binary again. I believe this is okay for now as we should move forward but it probably makes sense to file an issue.

Yes I think we can do that in a separate issue. Let's talk about it and create an issue.

Discussed and agreed to create a separate software issue for this. This isn't critical for the TO.

vogelpi · 2024-04-01T21:39:00Z

sw/device/lib/dif/dif_csrng.c

    status->kind = kDifCsrngCmdStatusError;
-    uint32_t reg =


That seems to be quite a change. Do we need to re-align the corresponding function description in dif_csrng.h?

I think the function description should be fine but let's check in person.

Checked and agreed that no further changes are needed here.

vogelpi · 2024-04-01T21:46:18Z

hw/ip/csrng/rtl/csrng_pkg.sv

+    CMD_STS_INVALID_ACMD         = 2'h1,
+    CMD_STS_INVALID_STATE_PARAM  = 2'h2,
+    CMD_STS_INVALID_GEN_CMD      = 2'h3,
+    CMD_STS_UNDRIVEN             = 'z


Why do we need this?

I think you mean the undriven value here not the entire enum type.
I spent quite some time getting the CSRNG interface to work. Xcelium doesn't like it if we assign 'z to a struct that has a substruct. There's different ways you can do this and this is the only way I found that works both in the RTL and for the block level simulation.

Discussed and agreed in person that this is the way to handle this. Already previously, something very similar was done but only now we need to define an enum value for this case. This is okay.

vogelpi · 2024-04-01T22:00:38Z

hw/ip/csrng/rtl/csrng_ctr_drbg_cmd.sv

-  assign ctr_drbg_cmd_sts_o = sfifo_keyvrc_pop && (ctr_drbg_cmd_ccmd_o == UNI) &&
-         ((KeyLen == '0) && (BlkLen == '0) && (CtrLen == '0));
+  // Return a status error when the state is not zeroized properly during a UNI command.
+  assign ctr_drbg_cmd_sts_err = sfifo_keyvrc_pop && (ctr_drbg_cmd_ccmd_o == UNI) &&
+         (sfifo_keyvrc_wdata >> (KeyVRCFifoWidth-UniZeroizeWidth)) == '0;


I am not 100% sure but I think this is another error condition in the design which doesn't really make sense as it's never going to happen (similarly to receiving an INS command on an instance which is not instantiated in the RTL ). Here we have in the same file:

assign sfifo_keyvrc_wdata = (rcstage_ccmd == UNI) ? {{(KeyLen+BlkLen+CtrLen+1+SeedLen){1'b0}},rcstage_glast,upd_cmd_inst_id_i,upd_cmd_ccmd_i} :

meaning when the command seen is UNI, we set the relevant wdata bits to 0 anyway. This might be a coverage hole and synthesis might optimize this away. Things like that should be tested with SVAs. Did I miss something @h-filali ?

Ah, now I understood your comment in another discussion with @andreaskurth about "doing minimal changes". I think agree with you but I think in the case of CSRNG we should maybe sit together and discuss what we really want.

Yes, let's discuss this in person.

Discussed in person to remove this in a separate PR to unblock this one.

This commit removes the csrng_fsm_idle_wait() in csrng_send_app_cmd(). This was needed because the cmd_rdy signal was high whenever the CSRNG was turned off. Due to a fix in lowRISC#21981 this is no longer needed. Signed-off-by: Hakim Filali <[email protected]>

vogelpi

LGTM after discussing in person.

This commit removes the csrng_fsm_idle_wait() in csrng_send_app_cmd(). This was needed because the cmd_rdy signal was high whenever the CSRNG was turned off. Due to a fix in lowRISC#21981 this is no longer needed. Signed-off-by: Hakim Filali <[email protected]>

This commit removes the csrng_fsm_idle_wait() in csrng_send_app_cmd(). This was needed because the cmd_rdy signal was high whenever the CSRNG was turned off. Due to a fix in #21981 this is no longer needed. Signed-off-by: Hakim Filali <[email protected]>

lowRISC#21981 reworked the status signaling of CSRNG and changed the definition of the ack_sts signal from 1-bit to a multi-bit enum which broke the csrng_intr vseq trying to force the ack and ack_sts signals. This commit gets the sequence working again until it's reworked to properly test the error signaling without forcing signals. This is related to lowRISC#22869. Signed-off-by: Pirmin Vogel <[email protected]>

#21981 reworked the status signaling of CSRNG and changed the definition of the ack_sts signal from 1-bit to a multi-bit enum which broke the csrng_intr vseq trying to force the ack and ack_sts signals. This commit gets the sequence working again until it's reworked to properly test the error signaling without forcing signals. This is related to #22869. Signed-off-by: Pirmin Vogel <[email protected]>

The underlying hardware issue got fixed with lowRISC#21981 and the software workaround got disabled with lowRISC#22263. This commit now removes the unused software function. This is related to lowRISC#19568. Signed-off-by: Pirmin Vogel <[email protected]>

The underlying hardware issue got fixed with #21981 and the software workaround got disabled with #22263. This commit now removes the unused software function. This is related to #19568. Signed-off-by: Pirmin Vogel <[email protected]>

The underlying hardware issue got fixed with lowRISC#21981 and the software workaround got disabled with lowRISC#22263. This commit now removes the unused software function. This is related to lowRISC#19568. Signed-off-by: Pirmin Vogel <[email protected]>

h-filali requested review from a team as code owners March 12, 2024 16:27

h-filali requested review from marnovandermaas, engdoreis, andreaskurth and vogelpi and removed request for a team, marnovandermaas and engdoreis March 12, 2024 16:27

h-filali force-pushed the csrng-sts-resp branch 5 times, most recently from b47e116 to 65fb31d Compare March 19, 2024 12:56

h-filali mentioned this pull request Mar 19, 2024

[csrng/rtl] Disallow out of sequence application commands #22114

Merged

h-filali force-pushed the csrng-sts-resp branch from 65fb31d to 8cefcf7 Compare March 25, 2024 08:32

andreaskurth reviewed Mar 25, 2024

View reviewed changes

h-filali force-pushed the csrng-sts-resp branch 3 times, most recently from d7b809d to 0c9c6d6 Compare March 25, 2024 10:10

h-filali force-pushed the csrng-sts-resp branch 5 times, most recently from f42f36e to 6a423db Compare March 25, 2024 11:12

h-filali force-pushed the csrng-sts-resp branch from 6a423db to 3d165f8 Compare March 25, 2024 11:22

h-filali mentioned this pull request Mar 26, 2024

[csrng] SW instance instantiate command hangs after entropy complex initialization #19568

Closed

vogelpi mentioned this pull request Mar 30, 2024

[csrng] D2S Signoff #20976

Closed

vogelpi reviewed Apr 1, 2024

View reviewed changes

vogelpi approved these changes Apr 8, 2024

View reviewed changes

vogelpi merged commit 042e0b9 into lowRISC:master Apr 8, 2024
32 checks passed

h-filali mentioned this pull request Apr 8, 2024

[csrng/rtl] Remove zeroize check when uninstantiating #22466

Closed

vogelpi mentioned this pull request Apr 28, 2024

[csrng/dv] Adjust csrng_intr test to properly test sw/hw_cmd_ack_sts #22869

Closed

vogelpi mentioned this pull request Jun 7, 2024

[edn] V2S Signoff #22468

Closed

vogelpi mentioned this pull request Jun 25, 2024

[csrng] V2S Signoff #22467

Closed

h-filali deleted the csrng-sts-resp branch October 7, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[csrng/rtl] Propagate csrng errors to edn through cmd sts signal #21981

[csrng/rtl] Propagate csrng errors to edn through cmd sts signal #21981

h-filali commented Mar 12, 2024

andreaskurth left a comment

andreaskurth Mar 25, 2024

h-filali Mar 25, 2024

andreaskurth Mar 25, 2024

h-filali Mar 25, 2024

andreaskurth Mar 25, 2024

andreaskurth Mar 25, 2024

h-filali Mar 25, 2024

andreaskurth Mar 25, 2024

h-filali Mar 25, 2024

andreaskurth Mar 25, 2024

andreaskurth Mar 25, 2024

andreaskurth Mar 25, 2024

h-filali Mar 25, 2024

andreaskurth Mar 25, 2024

andreaskurth Mar 25, 2024

andreaskurth Mar 25, 2024

h-filali Mar 25, 2024

h-filali commented Mar 25, 2024

vogelpi left a comment

vogelpi Apr 1, 2024

h-filali Apr 8, 2024

vogelpi Apr 8, 2024

vogelpi Apr 1, 2024

h-filali Apr 8, 2024

vogelpi Apr 8, 2024

vogelpi Apr 1, 2024

h-filali Apr 8, 2024

vogelpi Apr 8, 2024

vogelpi Apr 1, 2024

vogelpi Apr 1, 2024

h-filali Apr 8, 2024

vogelpi Apr 8, 2024

vogelpi left a comment

[csrng/rtl] Propagate csrng errors to edn through cmd sts signal #21981

[csrng/rtl] Propagate csrng errors to edn through cmd sts signal #21981

Conversation

h-filali commented Mar 12, 2024

andreaskurth 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

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 Mar 25, 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

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

vogelpi left a comment

Choose a reason for hiding this comment