[dv,membkdr] Updating membackdoor loading utilities #20864

neeraj-rv · 2024-01-17T23:11:43Z

Updates contain:

completed handling of multiple subwords within a work (memory entry) for mem during (a) randomization routines (b) data generations routines
Added support for interleaved memory bit handling
Added support for sram scrambing before integrity

github-actions · 2024-01-17T23:12:48Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+            // Compute the ECC Code
+            raw_subword_with_ecc = get_ecc_computed_data(raw_subword);
+            `uvm_info(`gfn,
+                      $sformatf("Randomizing mem contents: \n\


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
659:46: The lines can't be continued with '', use concatenation operator with braces [Style: forbid-line-continuations] [forbid-line-continuations]

github-actions · 2024-01-17T23:12:48Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

      end
+       `uvm_info(`gfn, $sformatf("Randomizing mem contents: \n\


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
672:47: The lines can't be continued with '', use concatenation operator with braces [Style: forbid-line-continuations] [forbid-line-continuations]

Updates contain: 1. completed handling of multiple subwords within a work (memory entry) for mem during (a) randomization routines (b) data generations routines 2. Added support for interleaved memory bit handling 3. Added support for sram scrambing before integrity cleanup Signed-off-by: Neeraj Upasani <[email protected]>

rswarbrick

Hi there.

I've reviewed some of this code, but think we should probably think a little about the structure we want. The mem_bkdr_util code at the moment is pretty simple (I should know! I wrote most of it, some time ago!) This proposed change adds quite a lot of functionality, with corresponding complexity. Is there a way that this could be implemented by writing a subclass that has the extra functionality and instantiating that instead of the base class in the place you need it? (I suspect most of the code would just need moving)

rswarbrick · 2024-01-18T22:15:20Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+    this.interleave_bits_in_entry = interleave_bits_in_entry;
+    this.scramble_before_integ    = scramble_before_integ;
+    this.redundant_bits_per_entry = redundant_bits_per_entry;
+    this.width                    = (n_bits / depth) - redundant_bits_per_entry;


We've already got a check that depth divides n_bits. But could you add one to make sure that redundant_bits_per_entry - (n_bits / depth) is positive?

rswarbrick · 2024-01-19T09:51:26Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

@@ -37,6 +37,13 @@ class mem_bkdr_util extends uvm_object;
  protected uint32_t addr_lsb;
  protected uint32_t addr_width;
  protected uint32_t byte_addr_width;
+  protected uint32_t non_ecc_bits_per_subword;
+  protected uint32_t ecc_bits_per_subword;


I don't think we actually use this variable (except where it gets its value). Maybe it could be a local there?

rswarbrick · 2024-01-19T09:51:58Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+      bits_per_subword         = non_ecc_bits_per_subword + ecc_bits_per_subword +
+                                 extra_bits_per_subword;


I had to read this twice to get the syntax tree! :-) Could you add some parens around the broken line?

rswarbrick · 2024-01-19T09:53:11Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+      ecc_bits_per_subword     = get_ecc_parity_width(secded_eds);
+      bits_per_subword         = non_ecc_bits_per_subword + ecc_bits_per_subword +
+                                 extra_bits_per_subword;
+    //int subwords_per_word;


You've moved this variable to a field in the class. Probably need to delete this line too.

rswarbrick · 2024-01-19T09:58:04Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+  protected uint32_t ecc_bits_per_subword;
+  protected uint32_t bits_per_subword;
+  protected uint32_t subwords_per_word;
+  protected uint32_t redundant_bits_per_entry;


Can you add a doc comment somewhere (maybe above the constructor) which explains what some of these numbers mean?

Also, is there a difference in meaning between "entry" and "word" here? If not, it would probably be clearer to standardise on one of them.

rswarbrick · 2024-01-19T10:35:52Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+    if (interleave_bits_in_entry) begin
+      raw_data = data;
+      for (int subword_idx = 0; subword_idx < subwords_per_word; subword_idx++) begin
+         for (int b= 0; b < bits_per_subword; b++) begin


Nit: Missing space before the '='

rswarbrick · 2024-01-19T10:37:03Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+             int true_col_pos;
+             int intrlv_col_pos;
+             intrlv_col_pos       = (subword_idx + (((b * 4) + 1) * 4));
+             true_col_pos         = (subword_idx * bits_per_subword) + b;


I'd probably suggest folding these lines together, so you end up with something like:

int true_col_pos = (subword_idx + (((b * 4) + 1) * 4)); int intrlv_col_pos = (subword_idx * bits_per_subword) + b;

rswarbrick · 2024-01-19T10:38:49Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

+    // Adjust the row data for interleaving. Was interleaved, beg back to true position
+    if (interleave_bits_in_entry) begin
+      raw_data = data;
+      for (int subword_idx = 0; subword_idx < subwords_per_word; subword_idx++) begin


This loop is inverse with the one in write(), I think. (In fact, are they self-inverse so they are identical?)

I'd suggest moving the contents into a helper function. I think it would probably make things a bit easier to follow.

rswarbrick · 2024-01-19T10:41:24Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

@@ -379,9 +428,26 @@ class mem_bkdr_util extends uvm_object;

  // this is used to write 32bit of data plus 7 raw integrity bits.
  virtual function void write39integ(bit [bus_params_pkg::BUS_AW-1:0] addr, logic [38:0] data);
+    uvm_hdl_data_t  row_data;


Nit: Double space.

rswarbrick · 2024-01-19T10:48:09Z

hw/dv/sv/mem_bkdr_util/mem_bkdr_util.sv

    `_ACCESS_CHECKS(addr, 32) // this is essentially an aligned 32bit access.
    if (!check_addr_valid(addr)) return;
-    write(addr, data);
+
+    // read-modify-write:


Hmm, this needs a bit of thought. We're adding RMW functionality which wasn't there before. (Notice that there isn't any read() call in the old code of this function!)

Can you explain (possibly in a comment) why write39integ needs read-modify-write code and write doesn't? In the original implementation, this was essentially just a call to write.

Razer6 · 2024-01-26T13:43:46Z

.bazelrc

@@ -139,6 +139,9 @@ build:ubsan --linkopt -fsanitize=undefined
 # Enable the rust nightly toolchain
 build --@rules_rust//rust/toolchain/channel=nightly

+#Add Centos bin link
+build --@rules_rust//:extra_rustc_toolchain_dirs=/tools/foss/llvm/13.0.1/x86_64/centos.7/bin


This needs to be removed as it is needed in the local environment

github-actions bot reviewed Jan 17, 2024

View reviewed changes

neeraj-rv added the Component:Darjeeling label Jan 17, 2024

neeraj-rv force-pushed the membkdr branch from d7c04e2 to 716b727 Compare January 18, 2024 07:25

neeraj-rv force-pushed the membkdr branch from 716b727 to 908a69c Compare January 18, 2024 07:29

neeraj-rv requested review from andreaskurth and msfschaffner January 18, 2024 17:01

neeraj-rv marked this pull request as ready for review January 18, 2024 17:01

neeraj-rv requested review from a team and cfrantz as code owners January 18, 2024 17:01

neeraj-rv requested review from rswarbrick and removed request for a team January 18, 2024 17:01

rswarbrick reviewed Jan 19, 2024

View reviewed changes

Razer6 reviewed Jan 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dv,membkdr] Updating membackdoor loading utilities #20864

[dv,membkdr] Updating membackdoor loading utilities #20864

neeraj-rv commented Jan 17, 2024

github-actions bot Jan 17, 2024

github-actions bot Jan 17, 2024

rswarbrick left a comment

rswarbrick Jan 18, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

rswarbrick Jan 19, 2024

Razer6 Jan 26, 2024

		bits_per_subword = non_ecc_bits_per_subword + ecc_bits_per_subword +
		extra_bits_per_subword;

[dv,membkdr] Updating membackdoor loading utilities #20864

Are you sure you want to change the base?

[dv,membkdr] Updating membackdoor loading utilities #20864

Conversation

neeraj-rv commented Jan 17, 2024

github-actions bot Jan 17, 2024

Choose a reason for hiding this comment

github-actions bot Jan 17, 2024

Choose a reason for hiding this comment

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

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