[otbn,dv] Sort out timing for done/status signals in ISS

Now that otbn_core_model exposes a STATUS register, we can use it to
derive the "done" signal. Using the status register directly, rather
than reconstructing it from the done signal, models the OTBN block
more closely and actually simplifies a load of tracking DV
code (mainly because the two things that we're comparing Just Match,
so we don't need to convert between the two views of the world).

Unfortunately, we can't get rid of the "done" signal entirely, because
otbn.sv can choose to instantiate the model instead of the RTL to
represent the core. The timing isn't *quite* right here, because the
core flops STATUS but not its done_o output. But this shouldn't matter
for the chip-level sims where we use the model like this.

We *do* check that the RTL's done signal is as predicted in the UVM
and verilator testbenches.

This patch also sorts out the timing of the start of execution in the
UVM testbench: the ISS was probing the wrong start signal, so started
a cycle late. This didn't matter too much, because it and the design
were both waiting on the EDN, which took more than one cycle.

Finally, we fix the assertion that checks the model and RTL have
matching STATUS. This was broken before (using "rst_n", rather than
"!rst_n"), which is why we didn't notice the off-by-one at the start.

Signed-off-by: Rupert Swarbrick <[email protected]>

hw/ip/otbn/dv/model/otbn_core_model.sv

@@ -35,9 +35,8 @@ module otbn_core_model
   input  logic                     rst_edn_ni,
 
   input  logic                     start_i, // start the operation
-  output bit                       done_o,  // operation done
 
-  output err_bits_t                err_bits_o, // valid when done_o is asserted
+  output err_bits_t                err_bits_o, // updated when STATUS switches to idle
 
   input  edn_pkg::edn_rsp_t        edn_rnd_i, // EDN response interface
   input  logic                     edn_rnd_cdc_done_i, // RND from EDN is valid (DUT perspective)
@@ -46,6 +45,8 @@ module otbn_core_model
   output bit [7:0]       status_o,   // STATUS register
   output bit [31:0]      insn_cnt_o, // INSN_CNT register
 
+  output bit             done_r_o,
+
   output bit             err_o // something went wrong
 );
 
@@ -101,9 +102,7 @@ module otbn_core_model
 
   always_ff @(posedge clk_i or negedge rst_ni) begin
     if (!rst_ni) begin
-      if (model_state != 0) begin
-        otbn_model_reset(model_handle);
-      end
+      otbn_model_reset(model_handle);
       model_state <= 0;
       status_q <= 0;
       insn_cnt_q <= 0;
@@ -140,17 +139,6 @@ module otbn_core_model
   assign status_o = status_q;
   assign insn_cnt_o = insn_cnt_q;
 
-  // Track negedges of running_q and expose that as a "done" output.
-  bit running_r = 1'b0;
-  always_ff @(posedge clk_i or negedge rst_ni) begin
-    if (!rst_ni) begin
-      running_r <= 1'b0;
-    end else begin
-      running_r <= running;
-    end
-  end
-  assign done_o = running_r & ~running;
-
   // If DesignScope is not empty, we have a design to check. Bind a copy of otbn_rf_snooper_if into
   // each register file. The otbn_model_check() function will use these to extract memory contents.
   if (DesignScope != "") begin: g_check_design
@@ -182,6 +170,20 @@ module otbn_core_model
 
   assign err_o = failed_step | failed_cmp;
 
+  // Derive a "done" signal. This should trigger for a single cycle when OTBN finishes its work.
+  // It's analogous to the done_o signal on otbn_core, but this signal is delayed by a single cycle
+  // (hence its name is done_r_o).
+  bit otbn_model_busy, otbn_model_busy_r;
+  assign otbn_model_busy = (status_q != StatusIdle) && (status_q != StatusLocked);
+  always_ff @(posedge clk_i or negedge rst_ni) begin
+    if (!rst_ni) begin
+      otbn_model_busy_r <= 1'b0;
+    end else begin
+      otbn_model_busy_r <= otbn_model_busy;
+    end
+  end
+  assign done_r_o = otbn_model_busy_r & ~otbn_model_busy;
+
   // Make stop_pc available over DPI. This is handy for Verilator simulations (where the top-level
   // is in C++).
   export "DPI-C" function otbn_core_get_stop_pc;

hw/ip/otbn/dv/otbnsim/sim/sim.py

@@ -120,8 +120,9 @@ def step(self, verbose: bool) -> Tuple[Optional[OTBNInsn], List[Trace]]:
             # Zero INSN_CNT the cycle after we are told to start (and every
             # cycle after that until we start executing instructions, but that
             # doesn't really matter)
+            changes = self._on_stall(verbose, fetch_next=False)
             self.state.ext_regs.write('INSN_CNT', 0, True)
-            return (None, self._on_stall(verbose, fetch_next=False))
+            return (None, changes)
 
         insn = self._next_insn
         if insn is None:

hw/ip/otbn/dv/otbnsim/sim/state.py

@@ -157,8 +157,9 @@ def commit(self, sim_stalled: bool) -> None:
         if self.rnd_cdc_pending:
             self.rnd_cdc_counter += 1
 
+        self.ext_regs.commit()
+
         if self._urnd_stall:
-            self.ext_regs.commit()
             if self._urnd_reseed_complete:
                 self._urnd_stall = False
                 self.non_insn_stall = False

hw/ip/otbn/dv/uvm/env/otbn_scoreboard.sv

@@ -42,11 +42,20 @@ class otbn_scoreboard extends cip_base_scoreboard #(
   // array, mapping the transaction source ID (a_source in the TL transaction) to an expected value.
   otbn_exp_read_data_t exp_read_values [tl_source_t];
 
-  bit saw_start_tl_trans = 1'b0;
-  bit waiting_for_model = 1'b0;
+  // A flag that tracks the fact that we've seen a TL write to the CMD register that we expect to
+  // start OTBN. We track this because we derive the "start" signal in the model from an internal
+  // DUT signal, so need to make sure it stays in sync with the TL side.
+  //
+  // If false, there are no transactions pending. This gets set in process_tl_addr when we see a
+  // write to CMD. This runs on a posedge of the clock. We expect to see the model start on the
+  // following negedge. When we process a change to model status that shows things starting (in
+  // process_model_fifo), we check that the flag is set and then clear it. When setting the flag, we
+  // queue a check to run on the following posedge of the clock to make sure the flag isn't still
+  // set.
+  bit pending_start_tl_trans = 1'b0;
 
   // The mirrored STATUS register from the ISS.
-  bit [7:0] model_status = 1'b0;
+  bit [7:0] model_status = 8'd0;
 
   // The "locked" field is used to track whether OTBN is "locked". For most operational state
   // tracking, we go through the ISS, but OTBN can become locked without actually starting an
@@ -72,7 +81,6 @@ class otbn_scoreboard extends cip_base_scoreboard #(
     fork
       process_model_fifo();
       process_trace_fifo();
-      check_start();
     join_none
   endtask
 
@@ -83,15 +91,8 @@ class otbn_scoreboard extends cip_base_scoreboard #(
     // transaction, the reset will have caused them to be forgotten.
     exp_read_values.delete();
 
-    // Clear waiting_for_model. This handles a corner case where a TL transaction comes in to start
-    // OTBN just before a reset and the reset hits after the negedge of the clock (so we have set
-    // waiting_for_model) but before the following clock cycle (which would have generated the
-    // OtbnModelStart transaction).
-    waiting_for_model = 1'b0;
-
-    // Clear saw_start_tl_trans. Any "start" TL transaction will have been discarded by the reset,
-    // so we shouldn't still be tracking it.
-    saw_start_tl_trans = 1'b0;
+    model_status = otbn_pkg::StatusIdle;
+    pending_start_tl_trans = 1'b0;
 
     // Clear the locked bit (this is modelling RTL state that should be cleared on reset)
     locked = 1'b0;
@@ -128,17 +129,22 @@ class otbn_scoreboard extends cip_base_scoreboard #(
       end
 
       case (csr.get_name())
-        // Spot writes to the "cmd" register, which tell us to start
-        // These start the processor. We don't pass those to the model through UVM, because it's
-        // difficult to get the timing right (you only recognise the transaction on the clock edge
-        // after you needed to set the signal!), so the testbench actually grabs the model's start
-        // signal from the DUT internals. Of course, we need to check this is true exactly when we
-        // expect it to be. Here, we set a flag to say that we expect the "start" signal to be high.
-        // See the check_start() task, which checks it's true at the right time.
+        // Spot writes to the "cmd" register that tell us to start
         "cmd": begin
-          // We start the execution when we see a write of the EXECUTE command.
+          // We start the execution when we see a write of the EXECUTE command. See the comment
+          // above pending_start_tl_trans to see how this tracking works.
           if (item.a_data == otbn_pkg::CmdExecute) begin
-            saw_start_tl_trans = 1'b1;
+            // Set a flag: we're expecting the model to start on the next negedge. Also, spawn off a
+            // checking thread that will make sure it has been cleared again by the next posedge.
+            // Note that the reset() method is only called in the DV base class on the following
+            // posedge of rst_n, so we have to check whether we're still in reset here.
+            pending_start_tl_trans = 1'b1;
+            fork begin
+              @(cfg.clk_rst_vif.cb);
+              `DV_CHECK_FATAL(!cfg.clk_rst_vif.rst_n || !pending_start_tl_trans,
+                              "Model ignored a write of EXECUTE to the CMD register.")
+            end
+            join_none;
           end
         end
 
@@ -257,14 +263,18 @@ class otbn_scoreboard extends cip_base_scoreboard #(
       `uvm_info(`gfn, $sformatf("received model transaction:\n%0s", item.sprint()), UVM_HIGH)
 
       case (item.item_type)
-        OtbnModelStart: begin
-          // We should only see the model start if it was started by a TL transaction on the
-          // previous cycle.
-          `DV_CHECK_FATAL(waiting_for_model, "model started unbidden!")
-          waiting_for_model = 1'b0;
-        end
-
         OtbnModelStatus: begin
+          // Has the status changed from idle to busy? If so, we should have seen a write to the
+          // command register on the previous posedge. See comment above pending_start_tl_trans for
+          // the details.
+          if (model_status == otbn_pkg::StatusIdle &&
+              item.status inside {otbn_pkg::StatusBusyExecute,
+                                  otbn_pkg::StatusBusySecWipeDmem,
+                                  otbn_pkg::StatusBusySecWipeImem}) begin
+            `DV_CHECK_FATAL(pending_start_tl_trans,
+                            "Saw start transaction without corresponding write to CMD")
+            pending_start_tl_trans = 1'b0;
+          end
           model_status = item.status;
         end
 
@@ -300,36 +310,6 @@ class otbn_scoreboard extends cip_base_scoreboard #(
     end
   endtask
 
-  // We track TL writes that should start the processor and the model transaction that says it has
-  // started. There should be a single cycle delay between the two: on the negedge of each clock,
-  // convert a "there was a TL write" event (saw_start_tl_trans) to "we expect to see the model
-  // start" (waiting_for_model).
-  //
-  // If we get to a negedge where waiting_for_model is true, that means that the model didn't start
-  // when we expected it to.
-  task check_start();
-    forever begin
-      @(cfg.clk_rst_vif.cbn);
-
-      if (!cfg.clk_rst_vif.rst_n) begin
-        // We're in reset. Wait until we start again.
-        @(posedge cfg.clk_rst_vif.rst_n);
-      end else begin
-        // We're not in reset. Check that waiting_for_model is false. If not, we've had a cycle
-        // since the model should have started.
-        `DV_CHECK(!waiting_for_model, "model didn't start when we expected it to")
-
-        // If we've just seen a write to the CMD register that should start OTBN, set the
-        // waiting_for_model flag. This gives a single cycle delay (on the next cycle, we'll check
-        // it worked properly using the DV_CHECK above).
-        if (saw_start_tl_trans) begin
-          waiting_for_model = 1'b1;
-          saw_start_tl_trans = 1'b0;
-        end
-      end
-    end
-  endtask
-
   // Pop from iss_trace_queue and rtl_trace_queue while they both contain an entry
   function void pop_trace_queues();
     while ((iss_trace_queue.size() > 0) && (rtl_trace_queue.size() > 0)) begin

hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_agent_pkg.sv

@@ -12,7 +12,6 @@ package otbn_model_agent_pkg;
     otbn_trace_checker_pop_iss_insn(output bit [31:0] insn_addr, output string mnemonic);
 
   typedef enum {
-    OtbnModelStart,
     OtbnModelStatus,
     OtbnModelInsn
   } otbn_model_item_type_e;

hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_if.sv

@@ -16,7 +16,6 @@ interface otbn_model_if #(
   logic                     start;        // Start the operation
 
   // Outputs from DUT
-  bit                       done;         // Operation done
   bit                       err;          // Something went wrong
   bit [31:0]                stop_pc;      // PC at end of operation
 

hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_monitor.sv

@@ -21,32 +21,12 @@ class otbn_model_monitor extends dv_base_monitor #(
 
   protected task collect_trans(uvm_phase phase);
     fork
-      collect_start();
       collect_status();
       // TODO: Only run when coverage is enabled.
       collect_insns();
     join
   endtask
 
-  protected task collect_start();
-    otbn_model_item trans;
-
-    forever begin
-      // Collect transactions on each clock edge when reset is high.
-      @(posedge cfg.vif.clk_i);
-      if (cfg.vif.rst_ni === 1'b1) begin
-        if (cfg.vif.start) begin
-          trans = otbn_model_item::type_id::create("trans");
-          trans.item_type = OtbnModelStart;
-          trans.status    = 0;
-          trans.err       = 0;
-          trans.mnemonic  = "";
-          analysis_port.write(trans);
-        end
-      end
-    end
-  endtask
-
   protected task collect_status();
     otbn_model_item trans;
 

hw/ip/otbn/dv/uvm/tb.sv

@@ -158,7 +158,7 @@ module tb;
   // to grab the decoded signal from TL transactions on the cycle it happens. We have an explicit
   // check in the scoreboard to ensure that this gets asserted at the time we expect (to spot any
   // decoding errors).
-  assign model_if.start = dut.start_q;
+  assign model_if.start = dut.start_d;
 
   // Valid signals below are set when DUT finishes processing incoming 32b packages and constructs
   // 256b EDN data. Model checks if the processing of the packages are done in maximum of 5 cycles
@@ -183,7 +183,6 @@ module tb;
     .rst_edn_ni   (edn_rst_n),
 
     .start_i      (model_if.start),
-    .done_o       (model_if.done),
 
     .err_bits_o   (),
 
@@ -195,6 +194,8 @@ module tb;
     .status_o     (model_if.status),
     .insn_cnt_o   (model_insn_cnt),
 
+    .done_r_o     (),
+
     .err_o        (model_if.err)
   );
 
@@ -224,12 +225,11 @@ module tb;
   // These are the sort of checks that are easier to state at the design level than in terms of UVM
   // transactions.
 
-  // Check that the idle output from the DUT is the inverse of the model's "done" signal.
-  // Separately, the code in otbn_idle_checker.sv checks that the idle output from the DUT is also
-  // the inverse of the "done" signal that we expect. Combining the two tells us that the RTL and
-  // model agree about whether they are running or not.
-  `ASSERT(MatchingDone_A, idle == !model_if.done, clk, rst_n)
-
+  // Check that the status output from the model exactly matches the register from the dut. In
+  // theory, we could see mismatches by probing the STATUS register over the TL bus, but we have to
+  // be lucky with exactly when the reads happen if we want to see off-by-one cycle errors. This
+  // assertion gives a continuous check.
+  `ASSERT(MatchingStatus_A, dut.hw2reg.status.d == model_if.status, clk, !rst_n)
 
   initial begin
     mem_bkdr_util imem_util, dmem_util;

hw/ip/otbn/dv/verilator/otbn_top_sim.sv

@@ -282,9 +282,9 @@ module otbn_top_sim (
 
   localparam string DesignScope = "..u_otbn_core";
 
-  logic      otbn_model_done;
   err_bits_t otbn_model_err_bits;
   bit [31:0] otbn_model_insn_cnt;
+  bit        otbn_model_done_r;
   bit        otbn_model_err;
 
   otbn_core_model #(
@@ -299,17 +299,18 @@ module otbn_top_sim (
     .rst_edn_ni            ( IO_RST_N ),
 
     .start_i               ( otbn_start ),
-    .done_o                ( otbn_model_done ),
 
     .err_bits_o            ( otbn_model_err_bits ),
 
     .edn_rnd_i             ( rnd_rsp ),
     .edn_rnd_cdc_done_i    ( edn_rnd_data_valid ),
     .edn_urnd_data_valid_i ( edn_urnd_data_valid ),
 
-    .status_o              (),
+    .status_o              ( ),
     .insn_cnt_o            ( otbn_model_insn_cnt ),
 
+    .done_r_o              ( otbn_model_done_r ),
+
     .err_o                 ( otbn_model_err )
   );
 
@@ -323,12 +324,17 @@ module otbn_top_sim (
       cnt_mismatch_latched      <= 1'b0;
       model_err_latched         <= 1'b0;
     end else begin
-      if (otbn_done_q != otbn_model_done) begin
-        $display("ERROR: At time %0t, otbn_done_q != otbn_model_done (%0d != %0d).",
-                 $time, otbn_done_q, otbn_model_done);
+      // Check that the 'done_o' output from the RTL matches the 'done_r_o' output from the model
+      // (with one cycle delay).
+      if (otbn_done_q && !otbn_model_done_r) begin
+        $display("ERROR: At time %0t, RTL done on previous cycle, but model still busy.", $time);
+        done_mismatch_latched <= 1'b1;
+      end
+      if (otbn_model_done_r && !otbn_done_q) begin
+        $display("ERROR: At time %0t, model finished, but RTL not done in time.", $time);
         done_mismatch_latched <= 1'b1;
       end
-      if (otbn_done_q && otbn_model_done) begin
+      if (otbn_model_done_r && otbn_done_q) begin
         if (otbn_err_bits_q != otbn_model_err_bits) begin
           $display("ERROR: At time %0t, otbn_err_bits != otbn_model_err_bits (0x%0x != 0x%0x).",
                    $time, otbn_err_bits_q, otbn_model_err_bits);