[otbn,dv] Pass mnemonic in trace from ISS
This is a partial change, in the direction of adding some functional
coverage to OTBN. The problem is that we want to track things like
"did I execute ADDI?" but don't want to write (yet another) decoder in
the coverage model, which might be buggy or get out of sync with the
actual RTL and ISS, yielding bogus coverage data.
To solve this, the plan is to pass mnemonics from the ISS to the trace
comparison and then (over DPI) to the coverage model. We know that the
RTL trace items will be in sync with the ISS sync items (otherwise the
test fails), so we can just build two queues on the SystemVerilog side
to merge together instruction data as seen by the RTL trace interface
and information about decoded instructions as seen by the ISS.
This patch doesn't do any of the later work: it just adds a mnemonic
to the output from the ISS in stepped mode and teaches the C++ code
that receives it to understand what it's seeing.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/otbn/dv/model/otbn_trace_checker.cc b/hw/ip/otbn/dv/model/otbn_trace_checker.cc
index 5cc3dba..bb16e55 100644
--- a/hw/ip/otbn/dv/model/otbn_trace_checker.cc
+++ b/hw/ip/otbn/dv/model/otbn_trace_checker.cc
@@ -5,6 +5,7 @@
#include "otbn_trace_checker.h"
#include <cassert>
+#include <cstring>
#include <iostream>
#include <memory>
@@ -17,7 +18,8 @@
rtl_stall_(false),
iss_pending_(false),
done_(true),
- seen_err_(false) {
+ seen_err_(false),
+ last_data_vld_(false) {
OtbnTraceSource::get().AddListener(this);
}
@@ -148,8 +150,12 @@
return true;
}
- OtbnTraceEntry trace_entry;
- trace_entry.from_iss_trace(lines);
+ OtbnIssTraceEntry trace_entry;
+ if (!trace_entry.from_iss_trace(lines)) {
+ // Error parsing ISS trace. This has already printed a message to stderr.
+ // Just return false to pass the error code along.
+ return false;
+ }
done_ = false;
if (iss_pending_) {
@@ -194,6 +200,14 @@
return true;
}
+const OtbnIssTraceEntry::IssData *OtbnTraceChecker::PopIssData() {
+ if (!last_data_vld_)
+ return nullptr;
+
+ last_data_vld_ = false;
+ return &last_data_;
+}
+
bool OtbnTraceChecker::MatchPair() {
if (!(rtl_pending_ && iss_pending_)) {
return true;
@@ -210,5 +224,36 @@
seen_err_ = true;
return false;
}
+
+ // We've got a matching pair. Move the ISS data out of the (now defunct)
+ // iss_entry_ and into last_data_.
+ last_data_ = std::move(iss_entry_.data_);
+ last_data_vld_ = true;
+
return true;
}
+
+// Exposed over DPI as:
+//
+// import "DPI-C" function bit
+// otbn_trace_checker_pop_iss_insn(output string mnemonic);
+//
+// Any string output argument will stay valid until the next call to this
+// function.
+
+extern "C" unsigned char otbn_trace_checker_pop_iss_insn(
+ const char **mnemonic) {
+ static char mnemonic_buf[16];
+
+ const OtbnIssTraceEntry::IssData *iss_data =
+ OtbnTraceChecker::get().PopIssData();
+ if (!iss_data)
+ return 0;
+
+ assert(iss_data->mnemonic.size() + 1 <= sizeof mnemonic_buf);
+ memcpy(mnemonic_buf, iss_data->mnemonic.c_str(),
+ iss_data->mnemonic.size() + 1);
+
+ *mnemonic = mnemonic_buf;
+ return 1;
+}
diff --git a/hw/ip/otbn/dv/model/otbn_trace_checker.h b/hw/ip/otbn/dv/model/otbn_trace_checker.h
index 12220db..886c933 100644
--- a/hw/ip/otbn/dv/model/otbn_trace_checker.h
+++ b/hw/ip/otbn/dv/model/otbn_trace_checker.h
@@ -67,6 +67,10 @@
// mismatch.
bool Finish();
+ // Return and clear the ISS data for the last pair of trace entries that went
+ // through MatchPair if there is any.
+ const OtbnIssTraceEntry::IssData *PopIssData();
+
private:
// If rtl_pending_ and iss_pending_ are not both true, return true
// immediately with no other change. Otherwise, compare the two pending trace
@@ -80,10 +84,15 @@
OtbnTraceEntry rtl_stalled_entry_;
bool iss_pending_;
- OtbnTraceEntry iss_entry_;
+ OtbnIssTraceEntry iss_entry_;
bool done_;
bool seen_err_;
+
+ // The ISS entry for the last pair of trace entries that went through
+ // MatchPair.
+ bool last_data_vld_;
+ OtbnIssTraceEntry::IssData last_data_;
};
#endif // OPENTITAN_HW_IP_OTBN_DV_MODEL_OTBN_TRACE_CHECKER_H_
diff --git a/hw/ip/otbn/dv/model/otbn_trace_entry.cc b/hw/ip/otbn/dv/model/otbn_trace_entry.cc
index f802660..b4eb6e5 100644
--- a/hw/ip/otbn/dv/model/otbn_trace_entry.cc
+++ b/hw/ip/otbn/dv/model/otbn_trace_entry.cc
@@ -5,6 +5,8 @@
#include "otbn_trace_entry.h"
#include <algorithm>
+#include <cassert>
+#include <iostream>
#include <sstream>
void OtbnTraceEntry::from_rtl_trace(const std::string &trace) {
@@ -23,24 +25,6 @@
std::sort(writes_.begin(), writes_.end());
}
-void OtbnTraceEntry::from_iss_trace(const std::vector<std::string> &lines) {
- bool first = true;
- for (const std::string &line : lines) {
- if (first) {
- hdr_ = line;
- } else {
- // Ignore '!' lines (which are used to tell the simulation about external
- // register changes, not tracked by the RTL core simulation)
- bool is_bang = (line.size() > 0 && line[0] == '!');
- if (!is_bang) {
- writes_.push_back(line);
- }
- }
- first = false;
- }
- std::sort(writes_.begin(), writes_.end());
-}
-
bool OtbnTraceEntry::operator==(const OtbnTraceEntry &other) const {
return hdr_ == other.hdr_ && writes_ == other.writes_;
}
@@ -71,3 +55,56 @@
return 0 ==
hdr_.compare(1, std::string::npos, prev.hdr_, 1, std::string::npos);
}
+
+bool OtbnIssTraceEntry::from_iss_trace(const std::vector<std::string> &lines) {
+ // Read FSM. state 0 = read header; state 1 = read mnemonic (for E
+ // lines); state 2 = read writes
+ int state = 0;
+
+ for (const std::string &line : lines) {
+ switch (state) {
+ case 0:
+ hdr_ = line;
+ state = (!line.empty() && line[0] == 'E') ? 1 : 2;
+ break;
+
+ case 1:
+ // This some "special" extra data from the ISS that we use for
+ // functional coverage calculations. The line should be of the form
+ //
+ // # MNEMONIC
+ //
+ if (line.size() <= 2 || line[0] != '#' || line[1] != ' ') {
+ std::cerr << "Bad 'special' line for ISS trace with header `" << hdr_
+ << "': `" << line << "'.\n";
+ return false;
+ }
+ data_.mnemonic = line.substr(2);
+
+ state = 2;
+ break;
+
+ default: {
+ assert(state == 2);
+ // Ignore '!' lines (which are used to tell the simulation about
+ // external register changes, not tracked by the RTL core simulation)
+ bool is_bang = (line.size() > 0 && line[0] == '!');
+ if (!is_bang) {
+ writes_.push_back(line);
+ }
+ break;
+ }
+ }
+ }
+
+ // We shouldn't be in state 1 here: that would mean an E line with no
+ // follow-up '#' line.
+ if (state == 1) {
+ std::cerr << "No 'special' line for ISS trace with header `" << hdr_
+ << "'.\n";
+ return false;
+ }
+
+ std::sort(writes_.begin(), writes_.end());
+ return true;
+}
diff --git a/hw/ip/otbn/dv/model/otbn_trace_entry.h b/hw/ip/otbn/dv/model/otbn_trace_entry.h
index f730778..21715f3 100644
--- a/hw/ip/otbn/dv/model/otbn_trace_entry.h
+++ b/hw/ip/otbn/dv/model/otbn_trace_entry.h
@@ -9,8 +9,9 @@
class OtbnTraceEntry {
public:
+ virtual ~OtbnTraceEntry(){};
+
void from_rtl_trace(const std::string &trace);
- void from_iss_trace(const std::vector<std::string> &lines);
bool operator==(const OtbnTraceEntry &other) const;
void print(const std::string &indent, std::ostream &os) const;
@@ -30,9 +31,21 @@
// have been a stall)
bool is_compatible(const OtbnTraceEntry &other) const;
- private:
+ protected:
std::string hdr_;
std::vector<std::string> writes_;
};
+class OtbnIssTraceEntry : public OtbnTraceEntry {
+ public:
+ bool from_iss_trace(const std::vector<std::string> &lines);
+
+ // Fields that are populated from the "special" line for ISS entries
+ struct IssData {
+ std::string mnemonic;
+ };
+
+ IssData data_;
+};
+
#endif // OPENTITAN_HW_IP_OTBN_DV_MODEL_OTBN_TRACE_ENTRY_H_
diff --git a/hw/ip/otbn/dv/otbnsim/stepped.py b/hw/ip/otbn/dv/otbnsim/stepped.py
index 1fa4604..8c07c7a 100755
--- a/hw/ip/otbn/dv/otbnsim/stepped.py
+++ b/hw/ip/otbn/dv/otbnsim/stepped.py
@@ -93,10 +93,11 @@
insn, changes = sim.step(verbose=False, collect_stats=False)
if insn is None:
- hdr = 'STALL'
+ print('STALL')
else:
- hdr = 'E PC: {:#010x}, insn: {:#010x}'.format(pc, insn.raw)
- print(hdr)
+ print(f'E PC: {pc:#010x}, insn: {insn.raw:#010x}')
+ print(f'# {insn.insn.mnemonic}')
+
for change in changes:
entry = change.rtl_trace()
if entry is not None:
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 b0a3fde..2bf5ab6 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_env_cov.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_env_cov.sv
@@ -11,20 +11,73 @@
class otbn_env_cov extends cip_base_env_cov #(.CFG_T(otbn_env_cfg));
`uvm_component_utils(otbn_env_cov)
- // the base class provides the following handles for use:
- // otbn_env_cfg: cfg
+ covergroup insn_cg
+ with function sample(mnem_str_t mnemonic);
- // covergroups
- // [add covergroups here]
+ // A coverpoint for the instruction mnemonic. There's a manually specified bin for each valid
+ // instruction name. The bins are listed separately like this (rather than having a single bins
+ // line with each value) so that the names appear in the coverage report. The "mnem_" prefix is
+ // just to avoid SystemVerilog reserved words like "and".
+ mnemonic_cp: coverpoint mnemonic {
+ bins mnem_add = {mnem_str_t'("add")};
+ bins mnem_addi = {mnem_str_t'("addi")};
+ bins mnem_lui = {mnem_str_t'("lui")};
+ bins mnem_sub = {mnem_str_t'("sub")};
+ bins mnem_sll = {mnem_str_t'("sll")};
+ bins mnem_slli = {mnem_str_t'("slli")};
+ bins mnem_srl = {mnem_str_t'("srl")};
+ bins mnem_srli = {mnem_str_t'("srli")};
+ bins mnem_sra = {mnem_str_t'("sra")};
+ bins mnem_srai = {mnem_str_t'("srai")};
+ bins mnem_and = {mnem_str_t'("and")};
+ bins mnem_andi = {mnem_str_t'("andi")};
+ bins mnem_or = {mnem_str_t'("or")};
+ bins mnem_ori = {mnem_str_t'("ori")};
+ bins mnem_xor = {mnem_str_t'("xor")};
+ bins mnem_xori = {mnem_str_t'("xori")};
+ bins mnem_lw = {mnem_str_t'("lw")};
+ bins mnem_sw = {mnem_str_t'("sw")};
+ bins mnem_beq = {mnem_str_t'("beq")};
+ bins mnem_bne = {mnem_str_t'("bne")};
+ bins mnem_jal = {mnem_str_t'("jal")};
+ bins mnem_jalr = {mnem_str_t'("jalr")};
+ bins mnem_csrrs = {mnem_str_t'("csrrs")};
+ bins mnem_csrrw = {mnem_str_t'("csrrw")};
+ bins mnem_ecall = {mnem_str_t'("ecall")};
+ bins mnem_loop = {mnem_str_t'("loop")};
+ bins mnem_loopi = {mnem_str_t'("loopi")};
+ bins mnem_bn_add = {mnem_str_t'("bn.add")};
+ bins mnem_bn_addc = {mnem_str_t'("bn.addc")};
+ bins mnem_bn_addi = {mnem_str_t'("bn.addi")};
+ bins mnem_bn_addm = {mnem_str_t'("bn.addm")};
+ bins mnem_bn_mulqacc = {mnem_str_t'("bn.mulqacc")};
+ bins mnem_bn_mulqacc_wo = {mnem_str_t'("bn.mulqacc.wo")};
+ bins mnem_bn_mulqacc_so = {mnem_str_t'("bn.mulqacc.so")};
+ bins mnem_bn_sub = {mnem_str_t'("bn.sub")};
+ bins mnem_bn_subb = {mnem_str_t'("bn.subb")};
+ bins mnem_bn_subi = {mnem_str_t'("bn.subi")};
+ bins mnem_bn_subm = {mnem_str_t'("bn.subm")};
+ bins mnem_bn_and = {mnem_str_t'("bn.and")};
+ bins mnem_bn_or = {mnem_str_t'("bn.or")};
+ bins mnem_bn_not = {mnem_str_t'("bn.not")};
+ bins mnem_bn_xor = {mnem_str_t'("bn.xor")};
+ bins mnem_bn_rshi = {mnem_str_t'("bn.rshi")};
+ bins mnem_bn_sel = {mnem_str_t'("bn.sel")};
+ bins mnem_bn_cmp = {mnem_str_t'("bn.cmp")};
+ bins mnem_bn_cmpb = {mnem_str_t'("bn.cmpb")};
+ bins mnem_bn_lid = {mnem_str_t'("bn.lid")};
+ bins mnem_bn_sid = {mnem_str_t'("bn.sid")};
+ bins mnem_bn_mov = {mnem_str_t'("bn.mov")};
+ bins mnem_bn_movr = {mnem_str_t'("bn.movr")};
+ bins mnem_bn_wsrr = {mnem_str_t'("bn.wsrr")};
+ bins mnem_bn_wsrw = {mnem_str_t'("bn.wsrw")};
+ }
+ endgroup
function new(string name, uvm_component parent);
super.new(name, parent);
- // [instantiate covergroups here]
- endfunction : new
- virtual function void build_phase(uvm_phase phase);
- super.build_phase(phase);
- // [or instantiate covergroups here]
+ insn_cg = new;
endfunction
endclass
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_env_pkg.sv b/hw/ip/otbn/dv/uvm/env/otbn_env_pkg.sv
index e55e94b..702de92 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_env_pkg.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_env_pkg.sv
@@ -35,6 +35,9 @@
// typedefs
typedef virtual pins_if #(1) idle_vif;
+ parameter int unsigned MNEM_STR_LEN = 16;
+ typedef bit [MNEM_STR_LEN*8-1:0] mnem_str_t;
+
// A very simple wrapper around a word that has been loaded from the input binary and needs
// storing to OTBN's IMEM or DMEM.
typedef struct packed {
diff --git a/hw/ip/otbn/dv/uvm/env/otbn_scoreboard.sv b/hw/ip/otbn/dv/uvm/env/otbn_scoreboard.sv
index 378c7c3..bfa8057 100644
--- a/hw/ip/otbn/dv/uvm/env/otbn_scoreboard.sv
+++ b/hw/ip/otbn/dv/uvm/env/otbn_scoreboard.sv
@@ -98,6 +98,7 @@
task process_model_fifo();
otbn_model_item item;
+
forever begin
model_fifo.get(item);
`uvm_info(`gfn, $sformatf("received model transaction:\n%0s", item.sprint()), UVM_HIGH)
@@ -106,9 +107,18 @@
OtbnModelStart: begin
this.expect_start_counter--;
end
+
OtbnModelDone: begin
// TODO: Handle this signal
end
+
+ OtbnModelInsn: begin
+ if (cfg.en_cov) begin
+ `DV_CHECK_FATAL(item.mnemonic.len() <= MNEM_STR_LEN)
+ cov.insn_cg.sample(mnem_str_t'(item.mnemonic));
+ end
+ end
+
default: `uvm_fatal(`gfn, $sformatf("Bad item type %0d", item.item_type))
endcase
end
diff --git a/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_agent_pkg.sv b/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_agent_pkg.sv
index 1671e5c..25d4001 100644
--- a/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_agent_pkg.sv
+++ b/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_agent_pkg.sv
@@ -8,9 +8,12 @@
import dv_utils_pkg::*;
import dv_lib_pkg::*;
+ import "DPI-C" function bit otbn_trace_checker_pop_iss_insn(output string mnemonic);
+
typedef enum {
OtbnModelStart,
- OtbnModelDone
+ OtbnModelDone,
+ OtbnModelInsn
} otbn_model_item_type_e;
// macro includes
diff --git a/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_item.sv b/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_item.sv
index a55ff12..d03bc82 100644
--- a/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_item.sv
+++ b/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_item.sv
@@ -10,9 +10,13 @@
// Only valid when item_type == OtbnModelDone.
bit err;
+ // Only valid when item_type == OtbnModelInsn
+ string mnemonic;
+
`uvm_object_utils_begin(otbn_model_item)
- `uvm_field_enum (otbn_model_item_type_e, item_type, UVM_DEFAULT)
- `uvm_field_int (err, UVM_DEFAULT)
+ `uvm_field_enum (otbn_model_item_type_e, item_type, UVM_DEFAULT)
+ `uvm_field_int (err, UVM_DEFAULT)
+ `uvm_field_string (mnemonic, UVM_DEFAULT)
`uvm_object_utils_end
`uvm_object_new
diff --git a/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_monitor.sv b/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_monitor.sv
index bfe380e..f497011 100644
--- a/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_monitor.sv
+++ b/hw/ip/otbn/dv/uvm/otbn_model_agent/otbn_model_monitor.sv
@@ -23,6 +23,8 @@
fork
collect_start();
collect_done();
+ // TODO: Only run when coverage is enabled.
+ collect_insns();
join
endtask
@@ -37,6 +39,7 @@
trans = otbn_model_item::type_id::create("trans");
trans.item_type = OtbnModelStart;
trans.err = 0;
+ trans.mnemonic = "";
analysis_port.write(trans);
end
end
@@ -55,6 +58,7 @@
trans = otbn_model_item::type_id::create("trans");
trans.item_type = OtbnModelDone;
trans.err = cfg.vif.err;
+ trans.mnemonic = "";
analysis_port.write(trans);
cfg.vif.wait_not_done();
end else begin
@@ -65,4 +69,25 @@
end
endtask
+ protected task collect_insns();
+ otbn_model_item trans;
+
+ string insn_mnemonic;
+
+ // Collect transactions on each clock edge when we are not in reset
+ forever begin
+ @(posedge cfg.vif.clk_i);
+ if (cfg.vif.rst_ni === 1'b1) begin
+ // Ask the trace checker for any ISS instruction that has come in since last cycle.
+ if (otbn_trace_checker_pop_iss_insn(insn_mnemonic)) begin
+ trans = otbn_model_item::type_id::create("trans");
+ trans.item_type = OtbnModelInsn;
+ trans.err = 0;
+ trans.mnemonic = insn_mnemonic;
+ analysis_port.write(trans);
+ end
+ end
+ end
+ endtask
+
endclass
