[otbn,dv] Add loop state to coverage tracking
This fills in the holes in the existing coverage tracking for branch
and jump instructions: now we can see whether the instruction was at
the end of a loop or not.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/otbn/doc/dv/index.md b/hw/ip/otbn/doc/dv/index.md
index a92af34..4c4eee2 100644
--- a/hw/ip/otbn/doc/dv/index.md
+++ b/hw/ip/otbn/doc/dv/index.md
@@ -392,7 +392,7 @@
- Branch backwards to an invalid address (wrapping past zero)
Tracked as `eq_neg_cross`.
- Branch instruction at end of a loop.
- **TODO: Loop state tracking**
+ Tracked as `eq_at_loop_end_cross` (which also crosses with whether the branch was taken or not).
The "branch to current address" item is problematic if we want to take the branch.
Probably we need some tests with short timeouts to handle this properly.
@@ -413,7 +413,7 @@
- Branch backwards to an invalid address (wrapping past zero)
Tracked as `eq_neg_cross`.
- Branch instruction at end of a loop.
- **TODO: Loop state tracking**
+ Tracked as `eq_at_loop_end_cross` (which also crosses with whether the branch was taken or not).
The "branch to current address" item is problematic if we want to take the branch.
Probably we need some tests with short timeouts to handle this properly.
@@ -434,7 +434,7 @@
- Jump when the current PC is the top word in IMEM.
Tracked as `from_top_cp`.
- Jump instruction at end of a loop.
- **TODO: Loop state tracking**
+ Tracked as `at_loop_end_cp`.
Note that the "jump to current address" item won't be a problem to test since it will quickly overflow the call stack.
@@ -460,7 +460,7 @@
- Jump when the current PC is the top word in IMEM.
Tracked as `from_top_cp`.
- Jump instruction at end of a loop.
- **TODO: Loop state tracking**
+ Tracked as `at_loop_end_cp`.
Note that the "jump to current address" item won't be a problem to test since it will quickly over- or underflow the call stack, provided `<grd>` and `<grs1>` aren't both `x1`.
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_env.core b/hw/ip/otbn/dv/uvm/env/otbn_env.core
index c94ba2f..5ea0d52 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_env.core
+++ b/hw/ip/otbn/dv/uvm/env/otbn_env.core
@@ -16,6 +16,7 @@
- lowrisc:dv:otbn_model_agent
files:
- otbn_env_pkg.sv
+ - otbn_loop_if.sv
- otbn_trace_item.sv: {is_include_file: true}
- otbn_trace_monitor.sv: {is_include_file: true}
- otbn_env_cfg.sv: {is_include_file: true}
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_env.sv b/hw/ip/otbn/dv/uvm/env/otbn_env.sv
index 42f9091..82504fb 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_env.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_env.sv
@@ -28,6 +28,9 @@
if (!uvm_config_db#(virtual otbn_trace_if)::get(this, "", "trace_vif", cfg.trace_vif)) begin
`uvm_fatal(`gfn, "failed to get otbn_trace_if handle from uvm_config_db")
end
+ if (!uvm_config_db#(virtual otbn_loop_if)::get(this, "", "loop_vif", cfg.loop_vif)) begin
+ `uvm_fatal(`gfn, "failed to get otbn_loop_if handle from uvm_config_db")
+ end
trace_monitor = otbn_trace_monitor::type_id::create("trace_monitor", this);
trace_monitor.cfg = cfg;
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_env_cfg.sv b/hw/ip/otbn/dv/uvm/env/otbn_env_cfg.sv
index dc98b30..4d40bca 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_env_cfg.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_env_cfg.sv
@@ -12,8 +12,9 @@
`uvm_object_new
- // Handle used by otbn_trace_monitor
+ // Handles used by otbn_trace_monitor
virtual otbn_trace_if trace_vif;
+ virtual otbn_loop_if loop_vif;
// The directory in which to look for ELF files (set by the test in build_phase; controlled by the
// +otbn_elf_dir plusarg).
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_env_cov.sv b/hw/ip/otbn/dv/uvm/env/otbn_env_cov.sv
index ab6298e..3fedc19 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_env_cov.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_env_cov.sv
@@ -990,7 +990,8 @@
logic [31:0] insn_addr,
logic [11:0] offset,
logic [31:0] operand_a,
- logic [31:0] operand_b);
+ logic [31:0] operand_b,
+ logic at_current_loop_end_insn);
mnemonic_cp: coverpoint mnemonic {
`DEF_MNEM_BIN(mnem_beq);
@@ -1011,11 +1012,15 @@
`DEF_SEEN_CP(neg_cp, $signed(insn_addr) + $signed(offset) < 0)
`DEF_MNEM_CROSS2(eq, neg)
+
+ at_loop_end_cp: coverpoint at_current_loop_end_insn;
+ `DEF_MNEM_CROSS2(eq, at_loop_end)
endgroup
covergroup insn_jal_cg
with function sample(logic [31:0] insn_addr,
- logic [20:0] offset);
+ logic [20:0] offset,
+ logic at_current_loop_end_insn);
`DEF_SIGN_CP(dir_cp, offset, 21)
offset_align_cp: coverpoint offset[1:0] {
@@ -1026,12 +1031,15 @@
`DEF_SEEN_CP(oob_cp, $signed(insn_addr) + $signed(offset) > ImemSizeByte)
`DEF_SEEN_CP(neg_cp, $signed(insn_addr) + $signed(offset) < 0)
`DEF_SEEN_CP(from_top_cp, insn_addr == ImemSizeByte - 4)
+
+ at_loop_end_cp: coverpoint at_current_loop_end_insn;
endgroup
covergroup insn_jalr_cg
with function sample(logic [31:0] insn_addr,
logic [11:0] offset,
- logic [31:0] operand_a);
+ logic [31:0] operand_a,
+ logic at_current_loop_end_insn);
`DEF_SIGN_CP(off_dir_cp, offset, 12)
@@ -1068,6 +1076,9 @@
// Jump when the current PC is the top word in IMEM.
`DEF_SEEN_CP(from_top_cp, insn_addr == ImemSizeByte - 4)
+
+ // Is this jump the last instruction of the current loop?
+ at_loop_end_cp: coverpoint at_current_loop_end_insn;
endgroup
covergroup insn_csrrs_cg
@@ -1320,13 +1331,18 @@
rtl_item.insn_addr,
{insn_data[31], insn_data[7], insn_data[30:25], insn_data[11:8]},
rtl_item.gpr_operand_a,
- rtl_item.gpr_operand_b);
+ rtl_item.gpr_operand_b,
+ rtl_item.at_current_loop_end_insn);
mnem_jal:
insn_jal_cg.sample(rtl_item.insn_addr,
{insn_data[31], insn_data[19:12],
- insn_data[20], insn_data[30:21], 1'b0});
+ insn_data[20], insn_data[30:21], 1'b0},
+ rtl_item.at_current_loop_end_insn);
mnem_jalr:
- insn_jalr_cg.sample(rtl_item.insn_addr, insn_data[31:20], rtl_item.gpr_operand_a);
+ insn_jalr_cg.sample(rtl_item.insn_addr,
+ insn_data[31:20],
+ rtl_item.gpr_operand_a,
+ rtl_item.at_current_loop_end_insn);
mnem_csrrs:
insn_csrrs_cg.sample(insn_data, rtl_item.gpr_operand_a);
mnem_csrrw:
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_loop_if.sv b/hw/ip/otbn/dv/uvm/env/otbn_loop_if.sv
new file mode 100644
index 0000000..3855659
--- /dev/null
+++ b/hw/ip/otbn/dv/uvm/env/otbn_loop_if.sv
@@ -0,0 +1,15 @@
+// Copyright lowRISC contributors.
+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+//
+// Bound into the otbn_loop_controller and used to help collect loop information for coverage.
+
+interface otbn_loop_if (
+ input clk_i,
+ input rst_ni,
+
+ input logic [31:0] insn_addr,
+ input logic at_current_loop_end_insn
+);
+
+endinterface
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_trace_item.sv b/hw/ip/otbn/dv/uvm/env/otbn_trace_item.sv
index ba0a7f8..990b25d 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_trace_item.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_trace_item.sv
@@ -21,15 +21,20 @@
// GPR write data
logic [31:0] gpr_write_data;
+ // Flag which is true if the current instruction is at the end of the innermost loop. This will
+ // cause an if it's a jump, branch or another loop/loopi.
+ logic at_current_loop_end_insn;
+
`uvm_object_utils_begin(otbn_trace_item)
- `uvm_field_int (insn_addr, UVM_DEFAULT | UVM_HEX)
- `uvm_field_int (insn_data, UVM_DEFAULT | UVM_HEX)
- `uvm_field_int (gpr_operand_a, UVM_DEFAULT | UVM_HEX)
- `uvm_field_int (gpr_operand_b, UVM_DEFAULT | UVM_HEX)
- `uvm_field_int (wdr_operand_a, UVM_DEFAULT | UVM_HEX)
- `uvm_field_int (wdr_operand_b, UVM_DEFAULT | UVM_HEX)
- `uvm_field_sarray_int (flags_write_data, UVM_DEFAULT | UVM_HEX)
- `uvm_field_int (gpr_write_data, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (insn_addr, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (insn_data, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (gpr_operand_a, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (gpr_operand_b, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (wdr_operand_a, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (wdr_operand_b, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_sarray_int (flags_write_data, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (gpr_write_data, UVM_DEFAULT | UVM_HEX)
+ `uvm_field_int (at_current_loop_end_insn, UVM_DEFAULT)
`uvm_object_utils_end
`uvm_object_new
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_trace_monitor.sv b/hw/ip/otbn/dv/uvm/env/otbn_trace_monitor.sv
index a28fd34..0fba62a 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_trace_monitor.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_trace_monitor.sv
@@ -24,6 +24,10 @@
@(posedge cfg.trace_vif.clk_i);
if (cfg.trace_vif.rst_ni === 1'b1) begin
if (cfg.trace_vif.insn_valid && !cfg.trace_vif.insn_stall) begin
+ // Belt-and-braces check to make sure that the trace_vif and loop_vif are talking about
+ // the same instruction.
+ `DV_CHECK_EQ_FATAL(cfg.trace_vif.insn_addr, cfg.loop_vif.insn_addr)
+
item = otbn_trace_item::type_id::create("item");
item.insn_addr = cfg.trace_vif.insn_addr;
item.insn_data = cfg.trace_vif.insn_data;
@@ -33,6 +37,7 @@
item.wdr_operand_b = cfg.trace_vif.rf_bignum_rd_data_b;
item.flags_write_data = cfg.trace_vif.flags_write_data;
item.gpr_write_data = cfg.trace_vif.rf_base_wr_data;
+ item.at_current_loop_end_insn = cfg.loop_vif.at_current_loop_end_insn;
`uvm_info(`gfn, $sformatf("saw trace item:\n%0s", item.sprint()), UVM_HIGH)
analysis_port.write(item);
diff --git a/hw/ip/otbn/dv/uvm/tb.sv b/hw/ip/otbn/dv/uvm/tb.sv
index 474e228..8fd90d5 100644
--- a/hw/ip/otbn/dv/uvm/tb.sv
+++ b/hw/ip/otbn/dv/uvm/tb.sv
@@ -84,6 +84,16 @@
bind dut.u_otbn_core otbn_tracer u_otbn_tracer(.*, .otbn_trace(i_otbn_trace_if));
+ bind dut.u_otbn_core.u_otbn_controller.u_otbn_loop_controller
+ otbn_loop_if i_otbn_loop_if (
+ .clk_i (clk_i),
+ .rst_ni (rst_ni),
+ // The insn_addr_i signal in the loop controller is of width ImemAddrWidth. We expand it to a
+ // 32-bit address here to avoid having to parameterise the type of the interface.
+ .insn_addr (32'(insn_addr_i)),
+ .at_current_loop_end_insn (at_current_loop_end_insn)
+ );
+
// OTBN model, wrapping an ISS.
//
// Note that we pull the "start" signal out of the DUT. This is because it's much more difficult
@@ -132,6 +142,9 @@
uvm_config_db#(virtual otbn_trace_if)::set(null, "*.env", "trace_vif",
dut.u_otbn_core.i_otbn_trace_if);
+ uvm_config_db#(virtual otbn_loop_if)::set(
+ null, "*.env", "loop_vif",
+ dut.u_otbn_core.u_otbn_controller.u_otbn_loop_controller.i_otbn_loop_if);
$timeformat(-12, 0, " ps", 12);
run_test();
