[aes/dv] Randomly drive invalid AES operation values #24623

vogelpi · 2024-09-23T10:32:14Z

This PR contains a couple of commits to enable closing a small coverage hole.

The main commit is the last one. It properly randomizes the operation values. Previously, the operation field of the main control register was only written with valid one-hot encoded values but not with invalid values leading to a small DV coverage hole.

This resolves #20941.

vogelpi · 2024-09-23T10:33:02Z

Thanks @martin-velay for your help with debugging this :-)

rswarbrick

Lots of nitty comments/questions but they needn't really gate this PR.

hw/ip/aes/dv/env/seq_lib/aes_base_vseq.sv

rswarbrick · 2024-09-23T15:03:45Z

hw/ip/aes/dv/env/aes_env_cfg.sv

+  //   [1]: reseed error
+  //   [2]: mode error
+  //   [3]: key_len
+  cfg_error_type_t   config_error_type_en       = 4'b1111;


This needn't gate the PR, but can't you do this with something like ... = '{op = 1'b1, rsd_rate = 1'b1, ...}; ? It might be a bit easier to read and wouldn't need the comment either.

Good idea! I've implemented the change for lines / types touches in this PR.

rswarbrick · 2024-09-23T15:07:15Z

hw/ip/aes/dv/env/aes_message_item.sv

+    solve has_config_error before cfg_error_type;
+    if (has_config_error) {
+      cfg_error_type inside {[1:15]};
+      config_error_type_en[0] == 0 -> cfg_error_type[0] == 0;


Can't this be expressed with names? So something like config_error_type_en.op == 0 -> cfg_error_type.op == 0?

Even more tersely, I think you can do something like !(cfg_error_type & ~config_error_type_en), but that's probably getting ridiculously hard to understand...

FOLLOW-UP: Ah! I've just seen the lines this came from! I think I stand by my comments, but they needn't gate this PR.

I went with the first proposal to be consistent and improve readability.

hw/ip/aes/dv/env/aes_message_item.sv

rswarbrick · 2024-09-23T15:13:11Z

hw/ip/aes/dv/env/aes_message_item.sv

+    end else begin
+      this.aes_operation = 2'b01; // resovle to AES-ENC
+      `uvm_info(`gfn,
+          $sformatf("\n\t ---| Illegal operation value detected, resolving to default AES-ENC"),


Nit: I think you probably want "... detected. Resolving ..." to fix the grammar.

Yep, thanks!

martin-velay

Your changes are looking good, this PR can be approved according to me.

Note: the DV seems to be complicated to debug because of some bad practices, particularly regarding the sequence items which are gathering more information than it should. These objects should be broken up into multiple sequence items in the future. And also using enumerated values for some variables might help.

martin-velay · 2024-09-23T15:26:17Z

hw/ip/aes/dv/env/aes_message_item.sv

+          $sformatf("\n\t ---| Illegal key length value detected, resolving to default AES-256"),
+          UVM_MEDIUM)
+    end
+    if (item.reseed_rate inside {3'b001, 3'b010, 3'b100}) begin


That's not really related to this PR, but in general I think it would be better to move to enumerated values. This would make reading easier. But it's not blocking for this PR of course.

I totally agree! I've changed this now for lines / member variables touches in this PR.

martin-velay · 2024-09-23T15:42:53Z

hw/ip/aes/dv/env/aes_message_item.sv

    return str;
  endfunction // conver2string

+  function string cfg_error_string();


As the sequence item is not properly split originally, I agree with your change. But FYI, this convert2string function is defined by the UVM. That's convenient when you want to debug a sequence from anybody, you know that you can call this and print all the relevant info in a relevant format. In the future, we should break down the sequence items into sub-items and eventually gathering them into another sequence item.

I agree with you. Thanks for providing some background on this Martin.

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

Previously, the reseed rate wasn't correctly passed, resulting in confusing debug statements during configuration error tests. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

This commit properly randomizes the operation values to fix a coverage hole. Previously, the operation field of the main control register was only written with valid one-hot encoded values but not with invalid values leading to a small DV coverage hole. This resolves lowRISC#20941. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

Previously, the basic convert2string() function called in different contexts (scoreboard, base sequence, etc.) would print information that is not available / meaningful in the scoreboard context such as the configuration error data and distributions. This commits splits the original function into multiple functions to enable controlling what to print in which context. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

vogelpi · 2024-09-25T00:45:45Z

Thanks @martin-velay and @rswarbrick for your review and the suggestions! I've implemented most of your feedback (see discussions). If you're happy with the changes, please feel free to approve & merge this @rswarbrick .

rswarbrick

This looks good to me. A couple of (rather nitty) tweaks that might be worth doing, but I'm happy either way.

rswarbrick · 2024-09-25T11:09:29Z

hw/ip/aes/dv/env/aes_env_cfg.sv

+  //   [0]: op_error
+  //   [1]: reseed error
+  //   [2]: mode error
+  //   [3]: key_len


Nit: I don't think you need the comment about the indices any more?

Similarly for cfg_error_type below.

rswarbrick · 2024-09-25T11:13:04Z

hw/ip/aes/dv/env/aes_seq_item.sv

@@ -7,7 +7,7 @@ class aes_seq_item extends uvm_sequence_item;
  `uvm_object_utils(aes_seq_item)

  aes_item_type_e  item_type;
-  aes_op_e operation;
+  bit [1:0] operation;


Nit: I know that this is unchanged with this commit, but the indentation looks a bit weird with the previous line. Maybe we should either be dropping a space on line 9 or adding some more spaces before "operation"?

vogelpi · 2024-09-25T21:18:35Z

Thanks @rswarbrick for your approval. I am merging this now as I need to move on.

vogelpi requested a review from a team as a code owner September 23, 2024 10:32

vogelpi requested review from matutem, rswarbrick and martin-velay and removed request for a team and matutem September 23, 2024 10:32

vogelpi mentioned this pull request Sep 23, 2024

[aes,dv] Randomly drive invalid operation values to fix coverage hole #20941

Closed

vogelpi force-pushed the aes-cov-fix branch 2 times, most recently from 4383da6 to 71249fe Compare September 23, 2024 14:28

rswarbrick reviewed Sep 23, 2024

View reviewed changes

martin-velay approved these changes Sep 23, 2024

View reviewed changes

vogelpi added 4 commits September 25, 2024 01:41

[aes/dv] Correct name of reseed rate weight env config variable

f72cbdf

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

[aes/dv] Clean up invalid value checking/resolving, also handle rsd_rate

c006aff

Previously, the reseed rate wasn't correctly passed, resulting in confusing debug statements during configuration error tests. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

vogelpi force-pushed the aes-cov-fix branch from 71249fe to c07ec6c Compare September 25, 2024 00:43

rswarbrick approved these changes Sep 25, 2024

View reviewed changes

vogelpi merged commit 1cb1c3d into lowRISC:master Sep 25, 2024
38 checks passed

vogelpi added the ManuallyCherryPick:earlgrey_1.0.0 This PR should be cherry-picked to the earlgrey_1.0.0 branch (no automation, manual coordination). label Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aes/dv] Randomly drive invalid AES operation values #24623

[aes/dv] Randomly drive invalid AES operation values #24623

vogelpi commented Sep 23, 2024

vogelpi commented Sep 23, 2024

rswarbrick left a comment

rswarbrick Sep 23, 2024

vogelpi Sep 25, 2024

rswarbrick Sep 23, 2024

vogelpi Sep 25, 2024

rswarbrick Sep 23, 2024

vogelpi Sep 25, 2024

martin-velay left a comment

martin-velay Sep 23, 2024

vogelpi Sep 25, 2024

martin-velay Sep 23, 2024

vogelpi Sep 25, 2024

vogelpi commented Sep 25, 2024

rswarbrick left a comment

rswarbrick Sep 25, 2024

rswarbrick Sep 25, 2024

vogelpi commented Sep 25, 2024

[aes/dv] Randomly drive invalid AES operation values #24623

[aes/dv] Randomly drive invalid AES operation values #24623

Conversation

vogelpi commented Sep 23, 2024

vogelpi commented Sep 23, 2024

rswarbrick 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

martin-velay 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

vogelpi commented Sep 25, 2024

rswarbrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi commented Sep 25, 2024