[kmac,dv] Rewrite kmac_app_agent to be more general

This work adapts the agent to match the newly defined interface with
minimal changes. This should allow OTBN to interact with KMAC through
the interface in a more complicated way than existing users.

The agent that was in place isn't really able to handle the tweaked
protocol, so this commit rewrites it to allow extending things
smoothly.

Upsettingly, the commit is enormous! I couldn't figure out how to
split it up into smaller pieces. :-(

Important changes:

 - The agent gets split in two ("kmac_app_device_agent" and
   "kmac_app_host_agent"). These have different sequencers,
   corresponding to the fact that they fundamentally drive different
   sorts of items.

 - We no longer use push_pull_agent. It was already a slightly awkward
   fit (I think) and can't really support the new protocol.

 - kmac_app_device_driver now watches app requests and creates
   sequence items to represent these requests. They get
   broadcast through m_req_port. That port is connected to the
   sequencer's m_req_fifo, which can be consumed by the sequence that
   sends responses.

 - Sequence items are a bit more structured:

     + A request is sent in multiple beats, which each get a
       kmac_app_req_item where the last has m_last set.

     + The items representing the beats of a request are grouped into
       a "packet", represented by a kmac_app_req_packet_item. This
       contains a queue with the kmac_app_req_item objects for the
       beats.

     + A response is represented by a kmac_app_rsp_item.

     + The monitor sees complete transactions (a request followed by a
       response) and represents this as a kmac_app_mon_item, which
       contains a kmac_app_req_packet_item and a kmac_app_rsp_item.

 - The signals in kmac_app_if are now driven through clocking blocks
   in both directions, depending on an if_mode variable.

 - We don't use phase sequences any more. I think this is not really
   recommended and we can just start the "device responder" sequences
   from the test: probably a bit clearer. For example, see
   keymgr_base_test.sv (three lines of code).

Minor ("while we're at it") changes:

 - The interface gets a more conventional "_if" name.

 - It also exposes the request and response as inout ports, which
   makes it a bit easier to use. See hw/ip/keymgr/dv/tb.sv for an
   example of the change.

 - Fixed dubious function names in kmac_app_agent_cfg: things that
   acted on a digest, composed of two shares, were previously called
   e.g. add_user_digest_share. But they acted on both shares! Drop the
   _share" part.

 - Any new or dramatically-altered classes use out-of-block
   definitions and I've added meaningful documentation comments to
   functions, tasks and class variables.

 - The imports in kmac_app_agent_pkg are a bit more precise (mainly
    because I was concentrating to make sure I got the right thing
    from the right place).

 - The StrbAlignLSB_A assertions in kmac_app_intf were more
   complicated than needed. Group them into a single assertion. Also,
   express the assertions with default clocking/disable to make them
   easier to understand.

 - Rather than using field macros for the new classes, I've defined
   do_print/do_copy manually. This is probably a bit more efficient at
   runtime and (more importantly) lets us be explicit about things
   like radixes. For example, kmac_app_req_item always uses
   hexadecimal for its m_data field.

 - Since the strobe line is always required to be contiguous and
   aligned to the lsb, kmac_app_req_item represents it as just the
   number of valid bytes (m_num_bytes). This makes some of the code
   deailing with the item a bit simpler.

   For example, see kmac_app_host_seq: handling partial words when
   cfg.inject_zero_in_host_strb is now rather less mysterious.

 - I've renamed kmac_app_rsp_item::get_is_kmac_rsp_data_invalid to
   just is_kmac_rsp_data_invalid. There's no need for the "double
   accessor name".

 - Since I was having to touch the code anyway, I've made the way
   scoreboards consume items from the agent a bit simpler. For
   example, keymgr_scoreboard now has two imp imports (for requests
   and transactions) and doesn't have to manually maintain fifos in
   the same way.

 - I renamed the agents that connect to KMAC in the keymgr and
   keymgr_dpe environments to just m_kmac_agent: there's not much
   point in cluttering the code with longer names.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

Author: rswarbrick

hw/dv/sv/kmac_app_agent/kmac_app_agent.core

@@ -9,25 +9,24 @@ filesets:
     depend:
       - lowrisc:dv:dv_utils
       - lowrisc:dv:dv_lib
-      - lowrisc:dv:push_pull_agent
       - lowrisc:ip:keymgr_pkg
       - lowrisc:ip:kmac_pkg
     files:
+      - kmac_app_if.sv
       - kmac_app_agent_pkg.sv
-      - kmac_app_intf.sv
-      - kmac_app_item.sv: {is_include_file: true}
       - kmac_app_agent_cfg.sv: {is_include_file: true}
-      - kmac_app_sequencer.sv: {is_include_file: true}
-      - kmac_app_agent_cov.sv: {is_include_file: true}
-      - kmac_app_driver.sv: {is_include_file: true}
-      - kmac_app_host_driver.sv: {is_include_file: true}
+      - kmac_app_device_agent.sv: {is_include_file: true}
       - kmac_app_device_driver.sv: {is_include_file: true}
+      - kmac_app_device_sequencer.sv: {is_include_file: true}
+      - kmac_app_host_agent.sv: {is_include_file: true}
+      - kmac_app_host_driver.sv: {is_include_file: true}
       - kmac_app_monitor.sv: {is_include_file: true}
-      - kmac_app_agent.sv: {is_include_file: true}
-      - seq_lib/kmac_app_base_seq.sv: {is_include_file: true}
+      - kmac_app_mon_item.sv: {is_include_file: true}
+      - kmac_app_req_item.sv: {is_include_file: true}
+      - kmac_app_req_packet_item.sv: {is_include_file: true}
+      - kmac_app_rsp_item.sv: {is_include_file: true}
       - seq_lib/kmac_app_host_seq.sv: {is_include_file: true}
       - seq_lib/kmac_app_device_seq.sv: {is_include_file: true}
-      - seq_lib/kmac_app_seq_list.sv: {is_include_file: true}
     file_type: systemVerilogSource
 
 targets:

hw/dv/sv/kmac_app_agent/kmac_app_agent.sv

@@ -2,72 +2,98 @@
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
-class kmac_app_agent_cfg extends dv_reactive_agent_cfg;
+class kmac_app_agent_cfg extends dv_base_agent_cfg;
+  // Handle to the agent's interface
+  virtual kmac_app_if vif;
 
-  // interface handle used by driver, monitor & the sequencer, via cfg handle
-  virtual kmac_app_intf vif;
+  // Used in Device mode. The maximum length of time that the device is not ready when it is not
+  // processing a request.
+  int unsigned max_idle_time_not_ready = 8;
 
+  // Used in Device mode. The percentage chance for the device to be ready to receive a request on a
+  // cycle (assuming the device is idle and there haven't already been max_idle_time_not_ready
+  // cycles of not being ready)
+  int unsigned idle_ready_pct = 50;
+
+  // Bounds used by the host sequence for the m_delay value in the kmac_app_req_item objects it
+  // generates. This is the number of cycles to wait before sending the request.
   int unsigned req_delay_min = 0;
-  int unsigned req_delay_max = 100;
+  int unsigned req_delay_max = 10;
 
-  // delay between last message request and first digest response
+  // Bounds used by the device sequence for the number of cycles between when a request is accepted
+  // and when a response is sent.
   int unsigned rsp_delay_min = 0;
   int unsigned rsp_delay_max = 100;
 
-  // Enables/disable all protocol delays.
+  // If this is set then the device and response sequence will both constrain delays to be zero.
   rand bit zero_delays;
 
-  // Enable starting the device auto-response sequence by default if configured in Device mode.
-  bit start_default_device_seq = 1;
-
   // Knob to enable percentage of error response in auto-response sequence.
   int unsigned error_rsp_pct = 0;
 
-  // Knob to control whether an error response can be caused by all zeros or all ones in one of the
-  // shares.
+  // Knob to control whether a response where a share is '0 or '1 is considered an error response
   bit constant_share_means_error = 1'b1;
 
   // Knob to allow injecting zeros in strb.
   bit inject_zero_in_host_strb = 0;
 
-  rand push_pull_agent_cfg#(`CONNECT_DATA_WIDTH) m_data_push_agent_cfg;
+  // True if this app interface is capable of masking
+  bit has_masking = 1;
 
-  // KMAC digest share0/1 that can be set from test env.
-  kmac_pkg::rsp_digest_t rsp_digest_hs[$];
+  // A queue of digest responses. If the agent is in Device mode, this can be filled by calling
+  // add_user_digest. Once that has been done, the automatic device-mode sequence will respond the
+  // data in this queue.
+  rsp_digest_t rsp_digest_hs[$];
 
-  // Bias randomization in favor of enabling zero delays less often.
-  constraint zero_delays_c {
-    zero_delays dist { 0 := 8,
-                       1 := 2 };
-  }
-
-  // Setter method for the user digest share queues - must be called externally to place specific user-digest_share0
-  // to be sent by the driver.
-  function void add_user_digest_share(kmac_pkg::rsp_digest_t rsp_digest_h);
-    rsp_digest_hs.push_back(rsp_digest_h);
-  endfunction
-  // Getter method for the user digest share - returns the first data entry.
-  function kmac_pkg::rsp_digest_t get_user_digest_share();
-    `DV_CHECK_NE_FATAL(has_user_digest_share(), 0, "rsp_digest_share0 is empty!")
-    return rsp_digest_hs.pop_front();
-  endfunction
-  // Getter method for the user digest share - must be called externally to check whether there is
-  // any user data in the queues.
-  function bit has_user_digest_share();
-    return (rsp_digest_hs.size() > 0);
-  endfunction
+  extern function new (string name = "");
+
+  // Add a digest to the user digest queue.
+  //
+  // This only makes sense in Device mode.
+  extern function void add_user_digest(rsp_digest_t rsp_digest_h);
+
+  // Pop the first item from the user digest queue. This will fail with an error if the queue is
+  // empty.
+  extern function rsp_digest_t pop_user_digest();
+
+  // Returns true if the user digest queue is nonempty
+  extern function bit has_user_digest();
 
   `uvm_object_utils_begin(kmac_app_agent_cfg)
+    `uvm_field_int(req_delay_min,            UVM_DEFAULT)
+    `uvm_field_int(req_delay_max,            UVM_DEFAULT)
     `uvm_field_int(rsp_delay_min,            UVM_DEFAULT)
     `uvm_field_int(rsp_delay_max,            UVM_DEFAULT)
     `uvm_field_int(zero_delays,              UVM_DEFAULT)
-    `uvm_field_int(start_default_device_seq, UVM_DEFAULT)
   `uvm_object_utils_end
 
-  function new (string name = "");
-    super.new(name);
-    m_data_push_agent_cfg = push_pull_agent_cfg#(`CONNECT_DATA_WIDTH)::type_id::create(
-        "m_data_push_agent_cfg");
-  endfunction : new
-
+  // Bias randomization in favor of enabling zero delays less often.
+  extern constraint zero_delays_c;
 endclass
+
+function kmac_app_agent_cfg::new (string name = "");
+  super.new(name);
+endfunction : new
+
+function void kmac_app_agent_cfg::add_user_digest(rsp_digest_t rsp_digest_h);
+  if (if_mode != Device) begin
+    `uvm_fatal(get_name(), $sformatf("Cannot add a digest: agent is in %0s mode.", if_mode.name()))
+  end
+
+  rsp_digest_hs.push_back(rsp_digest_h);
+endfunction
+
+function rsp_digest_t kmac_app_agent_cfg::pop_user_digest();
+  if (!has_user_digest()) begin
+    `uvm_fatal(get_name(), "Cannot get a user digest: the queue is empty.")
+  end
+  return rsp_digest_hs.pop_front();
+endfunction
+
+function bit kmac_app_agent_cfg::has_user_digest();
+  return (rsp_digest_hs.size() > 0);
+endfunction
+
+constraint kmac_app_agent_cfg::zero_delays_c {
+  zero_delays dist { 0 := 8, 1 := 2 };
+}

hw/dv/sv/kmac_app_agent/kmac_app_agent_cfg.sv

@@ -5,33 +5,44 @@
 package kmac_app_agent_pkg;
   // dep packages
   import uvm_pkg::*;
-  import dv_utils_pkg::*;
-  import dv_base_agent_pkg::*;
-  import dv_lib_pkg::*;
-  import keymgr_pkg::*;
-  import push_pull_agent_pkg::*;
+
+  import dv_utils_pkg::Host, dv_utils_pkg::Device;
+
+  import dv_base_agent_pkg::dv_base_agent_cfg;
+  import dv_base_agent_pkg::dv_base_agent_cov;
+  import dv_base_agent_pkg::dv_base_driver;
+  import dv_base_agent_pkg::dv_base_monitor;
+  import dv_base_agent_pkg::dv_base_sequencer;
+  import dv_base_agent_pkg::dv_base_agent;
+  import dv_base_agent_pkg::dv_base_seq;
+
+  import kmac_pkg::AppDigestW, kmac_pkg::MsgWidth;
+  import kmac_pkg::rsp_digest_t;
 
   // macro includes
   `include "uvm_macros.svh"
   `include "dv_macros.svh"
 
-  // parameters
-  parameter int KMAC_REQ_DATA_WIDTH = 2 * kmac_pkg::MsgWidth   // share0 + share1
-                                      + kmac_pkg::MsgWidth / 8 // strobe
-                                      + 2;                     // req_last + rsp_ready
+  `include "kmac_app_req_item.sv"
+  `include "kmac_app_req_packet_item.sv"
+  `include "kmac_app_rsp_item.sv"
+  `include "kmac_app_mon_item.sv"
 
-  `define CONNECT_DATA_WIDTH .HostDataWidth(kmac_app_agent_pkg::KMAC_REQ_DATA_WIDTH)
-
-  // package sources
-  `include "kmac_app_item.sv"
   `include "kmac_app_agent_cfg.sv"
-  `include "kmac_app_sequencer.sv"
-  `include "kmac_app_agent_cov.sv"
-  `include "kmac_app_driver.sv"
-  `include "kmac_app_host_driver.sv"
-  `include "kmac_app_device_driver.sv"
+  typedef dv_base_agent_cov #(.CFG_T (kmac_app_agent_cfg)) kmac_app_agent_cov;
   `include "kmac_app_monitor.sv"
-  `include "kmac_app_seq_list.sv"
-  `include "kmac_app_agent.sv"
+
+  `include "kmac_app_device_driver.sv"
+  `include "kmac_app_device_sequencer.sv"
+
+  `include "kmac_app_host_driver.sv"
+  typedef dv_base_sequencer #(.ITEM_T (kmac_app_req_item),
+                              .CFG_T (kmac_app_agent_cfg)) kmac_app_host_sequencer;
+
+  `include "seq_lib/kmac_app_device_seq.sv"
+  `include "seq_lib/kmac_app_host_seq.sv"
+
+  `include "kmac_app_device_agent.sv"
+  `include "kmac_app_host_agent.sv"
 
 endpackage: kmac_app_agent_pkg

hw/dv/sv/kmac_app_agent/kmac_app_agent_cov.sv

@@ -0,0 +1,51 @@
+// Copyright lowRISC contributors (OpenTitan project).
+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+
+class kmac_app_device_agent extends dv_base_agent #(.CFG_T       (kmac_app_agent_cfg),
+                                                    .DRIVER_T    (kmac_app_device_driver),
+                                                    .SEQUENCER_T (kmac_app_device_sequencer),
+                                                    .MONITOR_T   (kmac_app_monitor),
+                                                    .COV_T       (kmac_app_agent_cov));
+  `uvm_component_utils(kmac_app_device_agent)
+
+  extern function new(string name, uvm_component parent);
+  extern function void build_phase(uvm_phase phase);
+  extern function void connect_phase(uvm_phase phase);
+
+  // A task that runs the standard kmac_app_device_seq to respond to app requests
+  //
+  // This sequence never completes
+  extern task run_device_seq();
+endclass
+
+function kmac_app_device_agent::new(string name, uvm_component parent);
+  super.new(name, parent);
+endfunction
+
+function void kmac_app_device_agent::build_phase(uvm_phase phase);
+  super.build_phase(phase);
+
+  // Get the handle to the interface
+  if (!uvm_config_db#(virtual kmac_app_if)::get(this, "", "vif", cfg.vif)) begin
+    `uvm_fatal(`gfn, "failed to get kmac_app_if handle from uvm_config_db")
+  end
+
+  // Configure the interface to match the agent.
+  cfg.vif.if_mode = Device;
+endfunction
+
+function void kmac_app_device_agent::connect_phase(uvm_phase phase);
+  super.connect_phase(phase);
+
+  if (cfg.is_active) begin
+    // Connect requests from the driver to the sequencer
+    driver.m_req_port.connect(sequencer.m_req_fifo.analysis_export);
+  end
+endfunction
+
+task kmac_app_device_agent::run_device_seq();
+  kmac_app_device_seq seq = kmac_app_device_seq::type_id::create("seq");
+  if (!seq.randomize()) `uvm_fatal(get_full_name(), "Failed to randomize device seq")
+  seq.start(sequencer);
+endtask