[prim_fifo_sync,rtl] Specialize for Depth == 1

This case is pretty simple to reason about, and avoids needing proper read and write addresses (because there is only one element). Unfortunately, it's quite a big commit! This is because we end up changing some of the duplicated counters inside the fifo. Items in the commit: - Update the RTL in prim_fifo_sync.sv to specialize as you'd expect. - Add an interface that can be bound in to prim_fifo_sync.sv and will register a way for sec_cm tests to use forced signals to simulate fault injections. - Bind that interface into the prim_fifo_sync instances - Wrap up the ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT calls that are used for prim_fifo_sync instances. After the change, the two macro calls that were previously used (for the read and write pointer) are now handled by a single macro. In instances with Depth >= 2, this turns into ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT. For singleton instances, it's ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1. I don't *think* you can make the dispatch between the two options automatic (because the pre-processor can't know the values of the Depth parameter). - Use these new macros in the various blocks that were using prim_fifo_sync in secure mode with Depth = 1. This is CSRNG, OTBN and flash_ctrl. Signed-off-by: Rupert Swarbrick <[email protected]>
lowRISC · Dec 5, 2024 · 2461469 · 2461469
1 parent 16b5876
commit 2461469
Show file tree

Hide file tree

Showing 15 changed files with 315 additions and 218 deletions.
diff --git a/hw/dv/sv/sec_cm/prim_singleton_fifo_if.sv b/hw/dv/sv/sec_cm/prim_singleton_fifo_if.sv
@@ -0,0 +1,74 @@
+// Copyright lowRISC contributors (OpenTitan project).
+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+
+// Countermeasure interface for the hardened 1-bit counter in prim_fifo_sync if Depth=1.
+//
+// This interface can be bound into a prim_fifo_sync instance. Many instances of the interface will
+// not do anything, because Depth > 1 or or Secure = 0. However if Depth = 1 and Secure = 1, the
+// nested prim_singleton_fifo_if_proxy class will register the instance in sec_cm_pkg and can
+// inject/restore faults.
+interface prim_singleton_fifo_if #(
+  parameter int Depth = 1,
+  parameter bit Secure = 1'b1
+) (
+  input clk_i,
+  input rst_ni
+);
+
+  `include "dv_macros.svh"
+  `include "uvm_macros.svh"
+  import uvm_pkg::*;
+
+  string msg_id = $sformatf("%m");
+
+  string path = dv_utils_pkg::get_parent_hier($sformatf("%m"));
+  string signal_forced;
+
+  class prim_singleton_fifo_if_proxy extends sec_cm_pkg::sec_cm_base_if_proxy;
+    logic orig_value;
+
+    function new(string name="");
+      super.new(name);
+    endfunction
+
+    virtual task automatic inject_fault();
+      logic force_value;
+
+      @(negedge clk_i);
+      `DV_CHECK(uvm_hdl_read(signal_forced, orig_value))
+      `DV_CHECK(uvm_hdl_force(signal_forced, ~orig_value))
+      `uvm_info(msg_id,
+                $sformatf("Forcing %s from %0d to %0d", signal_forced, orig_value, ~orig_value),
+                UVM_LOW)
+
+      @(negedge clk_i);
+      `DV_CHECK(uvm_hdl_release(signal_forced))
+    endtask
+
+    virtual task automatic restore_fault();
+      `DV_CHECK(uvm_hdl_deposit(signal_forced, orig_value))
+      `uvm_info(msg_id, $sformatf("Forcing %s to original value %0d", signal_forced, orig_value),
+                UVM_LOW)
+    endtask
+  endclass
+
+  if (Depth == 1 && Secure) begin : gen_secure_singleton
+    prim_singleton_fifo_if_proxy if_proxy;
+    initial begin
+      string local_signal;
+      local_signal = $urandom_range(0, 1) ? "inv_full" : "full_q";
+      signal_forced = $sformatf("%s.%s", path, local_signal);
+      `DV_CHECK_FATAL(uvm_hdl_check_path(signal_forced),, msg_id)
+
+      // Store the proxy object for TB to use
+      if_proxy = new("if_proxy");
+      if_proxy.sec_cm_type = sec_cm_pkg::SecCmSingletonFifo;
+      if_proxy.path = path;
+      sec_cm_pkg::sec_cm_if_proxy_q.push_back(if_proxy);
+
+      `uvm_info(msg_id, $sformatf("Interface proxy class is added for %s", path), UVM_HIGH)
+    end
+  end
+
+endinterface
diff --git a/hw/dv/sv/sec_cm/sec_cm.core b/hw/dv/sv/sec_cm/sec_cm.core
@@ -21,10 +21,12 @@ filesets:
       - prim_count_if.sv
       - prim_sparse_fsm_flop_if.sv
       - prim_double_lfsr_if.sv
+      - prim_singleton_fifo_if.sv
       - sec_cm_prim_onehot_check_bind.sv
       - sec_cm_prim_count_bind.sv
       - sec_cm_prim_sparse_fsm_flop_bind.sv
       - sec_cm_prim_double_lfsr_bind.sv
+      - sec_cm_prim_singleton_fifo_bind.sv
     file_type: systemVerilogSource
 
 targets:

diff --git a/hw/dv/sv/sec_cm/sec_cm_pkg.sv b/hw/dv/sv/sec_cm/sec_cm_pkg.sv
@@ -18,7 +18,8 @@ package sec_cm_pkg;
     SecCmPrimCount,
     SecCmPrimSparseFsmFlop,
     SecCmPrimDoubleLfsr,
-    SecCmPrimOnehot
+    SecCmPrimOnehot,
+    SecCmSingletonFifo
   } sec_cm_type_e;
 
   `include "sec_cm_base_if_proxy.sv"

diff --git a/hw/dv/sv/sec_cm/sec_cm_prim_singleton_fifo_bind.sv b/hw/dv/sv/sec_cm/sec_cm_prim_singleton_fifo_bind.sv
@@ -0,0 +1,11 @@
+// Copyright lowRISC contributors (OpenTitan project).
+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+
+module sec_cm_prim_singleton_fifo_bind ();
+  // Bind prim_singleton_fifo_if into each instance of prim_fifo_sync. This will have no effect
+  // unless the fifo has Depth=1 and Secure=1.
+  bind
+    prim_fifo_sync
+    prim_singleton_fifo_if #(.Depth(Depth), .Secure(Secure)) u_prim_singleton_fifo_if (.*);
+endmodule
diff --git a/hw/ip/csrng/dv/tb.sv b/hw/ip/csrng/dv/tb.sv
@@ -141,22 +141,22 @@ module tb;
   // v is released into the ctr_drbg_gen/update blocks.
   `define BLOCK_ENCRYPT_PATH tb.dut.u_csrng_core.u_csrng_block_encrypt
   `define CTR_DRBG_GEN tb.dut.u_csrng_core.u_csrng_ctr_drbg_gen
-  `define CTR_DRBG_GEN_FIFO `CTR_DRBG_GEN.u_prim_fifo_sync_bencack.gen_normal_fifo
+  `define CTR_DRBG_GEN_FIFO `CTR_DRBG_GEN.u_prim_fifo_sync_bencack.gen_singleton_fifo
   `ASSERT_INIT(CsrngCtrDrbgGenFifoDepth1, `CTR_DRBG_GEN.BlkEncAckFifoDepth == 1)
   `ASSERT(CsrngSecCmAesCipherDataRegLocalEscGen,
-      $past(`CTR_DRBG_GEN_FIFO.fifo_incr_wptr) &&
+      $rose(`CTR_DRBG_GEN_FIFO.full_q) &&
       `BLOCK_ENCRYPT_PATH.block_encrypt_aes_cipher_sm_err_o |=>
-      $past(`CTR_DRBG_GEN_FIFO.storage[0]
+      $past(`CTR_DRBG_GEN_FIFO.storage
           [`CTR_DRBG_GEN.BlkEncAckFifoWidth-1 -: `CTR_DRBG_GEN.BlkLen]) !=
       $past(`BLOCK_ENCRYPT_PATH.cipher_data_out, 2), clk, !rst_n)
 
   `define CTR_DRBG_UPD tb.dut.u_csrng_core.u_csrng_ctr_drbg_upd
-  `define CTR_DRBG_UPD_FIFO `CTR_DRBG_UPD.u_prim_fifo_sync_bencack.gen_normal_fifo
+  `define CTR_DRBG_UPD_FIFO `CTR_DRBG_UPD.u_prim_fifo_sync_bencack.gen_singleton_fifo
   `ASSERT_INIT(CsrngCtrDrbgUpdFifoDepth1, `CTR_DRBG_UPD.BlkEncAckFifoDepth == 1)
   `ASSERT(CsrngSecCmAesCipherDataRegLocalEscUpd,
-      $past(`CTR_DRBG_UPD_FIFO.fifo_incr_wptr) &&
+      $rose(`CTR_DRBG_UPD_FIFO.full_q) &&
       `BLOCK_ENCRYPT_PATH.block_encrypt_aes_cipher_sm_err_o |=>
-      $past(`CTR_DRBG_UPD_FIFO.storage[0]
+      $past(`CTR_DRBG_UPD_FIFO.storage
           [`CTR_DRBG_UPD.BlkEncAckFifoWidth-1 -: `CTR_DRBG_UPD.BlkLen]) !=
       $past(`BLOCK_ENCRYPT_PATH.cipher_data_out, 2), clk, !rst_n)
 

diff --git a/hw/ip/otbn/dv/uvm/env/seq_lib/otbn_common_vseq.sv b/hw/ip/otbn/dv/uvm/env/seq_lib/otbn_common_vseq.sv
@@ -10,34 +10,6 @@ class otbn_common_vseq extends otbn_base_vseq;
   }
   `uvm_object_new
 
-  // Write rubbish to the storage backing memory for a prim_fifo_sync
-  function void splat_fifo_storage(string fifo_path, int unsigned depth);
-    for (int unsigned i = 0; i < depth; i++) begin
-      string storage_path = $sformatf("%0s.gen_normal_fifo.storage[%0d]", fifo_path, i);
-      bit [31:0] value;
-      randcase
-        1: value = 0;
-        1: value = '1;
-        1: value = $urandom;
-      endcase
-      `DV_CHECK_FATAL(uvm_hdl_deposit(storage_path, value))
-    end
-  endfunction
-
-  task dut_init(string reset_kind = "HARD");
-    // Zero the contents of the DMEM/IMEM request fifos if we're about to do fault injection on
-    // their counters. This avoids a problem where we generate a spurious request when the FIFO was
-    // actually empty and lots of signals in the design become X. This includes OTBN's status signal
-    // and we would never get to the "LOCKED" status because we're stuck at X. Zeroing the backing
-    // memory avoids that problem.
-    splat_fifo_storage("tb.dut.u_tlul_adapter_sram_dmem.u_reqfifo", 1);
-    splat_fifo_storage("tb.dut.u_tlul_adapter_sram_dmem.u_sramreqfifo", 1);
-    splat_fifo_storage("tb.dut.u_tlul_adapter_sram_imem.u_reqfifo", 1);
-    splat_fifo_storage("tb.dut.u_tlul_adapter_sram_imem.u_sramreqfifo", 1);
-
-    super.dut_init(reset_kind);
-  endtask
-
   virtual task body();
     enable_base_alert_checks = 1'b1;
 

diff --git a/hw/ip/otbn/rtl/otbn.sv b/hw/ip/otbn/rtl/otbn.sv
@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 `include "prim_assert.sv"
+`include "prim_fifo_assert.sv"
 
 /**
  * OpenTitan Big Number Accelerator (OTBN)
@@ -1427,41 +1428,23 @@ module otbn
       u_otbn_core.u_otbn_rf_bignum.gen_rf_bignum_ff.u_otbn_rf_bignum_inner.u_prim_onehot_check,
       alert_tx_o[AlertFatal])
 
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(DmemRspFifoWptrCheck_A,
-      u_tlul_adapter_sram_dmem.u_rspfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(DmemRspFifoRptrCheck_A,
-      u_tlul_adapter_sram_dmem.u_rspfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(DmemSramReqFifoWptrCheck_A,
-      u_tlul_adapter_sram_dmem.u_sramreqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(DmemSramReqFifoRptrCheck_A,
-      u_tlul_adapter_sram_dmem.u_sramreqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,
-      alert_tx_o[AlertFatal])
-    `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(DmemReqFifoWptrCheck_A,
-      u_tlul_adapter_sram_dmem.u_reqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(DmemReqFifoRptrCheck_A,
-      u_tlul_adapter_sram_dmem.u_reqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,
-      alert_tx_o[AlertFatal])
-
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(ImemRspFifoWptrCheck_A,
-      u_tlul_adapter_sram_imem.u_rspfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(ImemRspFifoRptrCheck_A,
-      u_tlul_adapter_sram_imem.u_rspfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(ImemSramReqFifoWptrCheck_A,
-      u_tlul_adapter_sram_imem.u_sramreqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(ImemSramReqFifoRptrCheck_A,
-      u_tlul_adapter_sram_imem.u_sramreqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(ImemReqFifoWptrCheck_A,
-      u_tlul_adapter_sram_imem.u_reqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,
-      alert_tx_o[AlertFatal])
-  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(ImemReqFifoRptrCheck_A,
-      u_tlul_adapter_sram_imem.u_reqfifo.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,
-      alert_tx_o[AlertFatal])
+  `ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(DmemRspFifo,
+                                               u_tlul_adapter_sram_dmem.u_rspfifo,
+                                               alert_tx_o[AlertFatal])
+  `ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(DmemSramReqFifo,
+                                               u_tlul_adapter_sram_dmem.u_sramreqfifo,
+                                               alert_tx_o[AlertFatal])
+  `ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(DmemReqFifo,
+                                               u_tlul_adapter_sram_dmem.u_reqfifo,
+                                               alert_tx_o[AlertFatal])
+
+  `ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(ImemRspFifo,
+                                               u_tlul_adapter_sram_imem.u_rspfifo,
+                                               alert_tx_o[AlertFatal])
+  `ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(ImemSramReqFifo,
+                                               u_tlul_adapter_sram_imem.u_sramreqfifo,
+                                               alert_tx_o[AlertFatal])
+  `ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(ImemReqFifo,
+                                               u_tlul_adapter_sram_imem.u_reqfifo,
+                                               alert_tx_o[AlertFatal])
 endmodule
diff --git a/hw/ip/prim/prim_fifo.core b/hw/ip/prim/prim_fifo.core
@@ -17,6 +17,7 @@ filesets:
       - rtl/prim_fifo_async.sv
       - rtl/prim_fifo_sync.sv
       - rtl/prim_fifo_sync_cnt.sv
+      - rtl/prim_fifo_assert.sv : {is_include_file : true}
     file_type: systemVerilogSource
 
   files_verilator_waiver:

diff --git a/hw/ip/prim/rtl/prim_assert_sec_cm.svh b/hw/ip/prim/rtl/prim_assert_sec_cm.svh
@@ -61,6 +61,9 @@
   `ASSERT_PRIM_ONEHOT_ERROR_TRIGGER_ALERT(NAME_, \
     REG_TOP_HIER_.u_prim_reg_we_check.u_prim_onehot_check, ALERT_, GATE_, MAX_CYCLES_)
 
+`define ASSERT_PRIM_FIFO_SYNC_SINGLETON_ERROR_TRIGGER_ALERT(NAME, HIER_, ALERT_, GATE_ = 0, MAX_CYCLES_ = `_SEC_CM_ALERT_MAX_CYC) \
+  `ASSERT_ERROR_TRIGGER_ALERT(NAME_, HIER_, ALERT_, GATE_, MAX_CYCLES_, err_o)
+
 ////////////////////////////////////////////////////////////////////////////////
 //
 // Assertions for CMs that trigger some other form of error

diff --git a/hw/ip/prim/rtl/prim_fifo_assert.sv b/hw/ip/prim/rtl/prim_fifo_assert.sv
@@ -0,0 +1,47 @@
+// Copyright lowRISC contributors (OpenTitan project).
+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+//
+// Macros that define assertions that relate to FIFO implementations
+
+`ifndef PRIM_FIFO_ASSERT_SV
+`define PRIM_FIFO_ASSERT_SV
+
+// Use PRIM_COUNT_ERROR_TRIGGER_ALERT appropriately to check that the three prim_counts generated by
+// a prim_fifo_sync with depth at least two do indeed generate an alert if they detect an error.
+//
+// - NAME_ is used as the root of the names of the generated assertions.
+// - HIER_ is a hierarchical path to the prim_fifo_sync in question.
+// - ALERT_ is the name of the alert that should be generated.
+// - GATE_ is a signal that, if true, will cause an error to be ignored.
+// - MAX_CYCLES_ is the number of cycles allowed until the alert must be generated.
+`define ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT(NAME_, HIER_, ALERT_, GATE_ = 0, MAX_CYCLES_ = `_SEC_CM_ALERT_MAX_CYC) \
+  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(``NAME_``WptrCheck_A,                                                            \
+                                         HIER_.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_wptr,                         \
+                                         ALERT_,                                                                          \
+                                         GATE_,                                                                           \
+                                         MAX_CYCLES_)                                                                     \
+  `ASSERT_PRIM_COUNT_ERROR_TRIGGER_ALERT(``NAME_``RptrCheck_A,                                                            \
+                                         HIER_.gen_normal_fifo.u_fifo_cnt.gen_secure_ptrs.u_rptr,                         \
+                                         ALERT_,                                                                          \
+                                         GATE_,                                                                           \
+                                         MAX_CYCLES_)
+
+// An analagous assertion to PRIM_COUNT_ERROR_TRIGGER_ALERT, but specialised for the case where the
+// fifo has depth 1, which means there aren't actually three prim_count instances but instead there
+// is just a single error signal.
+//
+// - NAME_ is used as a root for the name of the generated assertion.
+// - HIER_ is a hierarchical path to the prim_fifo_sync in question.
+// - ALERT_ is the name of the alert that should be generated.
+// - GATE_ is a signal that, if true, will cause an error to be ignored.
+// - MAX_CYCLES_ is the number of cycles allowed until the alert must be generated.
+`define ASSERT_PRIM_FIFO_SYNC_ERROR_TRIGGERS_ALERT1(NAME_, HIER_, ALERT_, GATE_ = 0, MAX_CYCLES_ = `_SEC_CM_ALERT_MAX_CYC) \
+  `ASSERT_ERROR_TRIGGER_ALERT(``NAME_``FullCheck_A,                                                                        \
+                              HIER_,                                                                                       \
+                              ALERT_,                                                                                      \
+                              GATE_,                                                                                       \
+                              MAX_CYCLES_,                                                                                 \
+                              err_o)
+
+`endif // PRIM_FIFO_ASSERT_SV