[rtl] IbexSimpleSystem missing instruction address check

[ZhongYic00]

Observed Behavior

Cosim reports a kind of mismatch that DUT executed instruction at an address which should be invalid.
Below is a concrete scenario where DUT continued to execute at 0x2002b2, which overflows the RAM boundary [0x100000,+0x100000]

FAILURE: Co-simulation mismatch at time                11360
Synchronous trap was expected at ISS PC: 100000 but the DUT didn't report one at PC 2002b2
[11360] %Error: ibex_simple_system_cosim_checker.sv:74: Assertion failed in TOP.ibex_simple_sy
stem.u_ibex_simple_system_cosim_checker_bind: Co-simulation mismatch seen

Look into the RTL, this is due to a missing address check in the icache access path of ram2p.
Source and corresponding fix:

diff --git a/examples/simple_system/rtl/ibex_simple_system.sv b/examples/simple_system/rtl/ibex_simple_system.sv
index e466ac2..b34eaed 100644
--- a/examples/simple_system/rtl/ibex_simple_system.sv
+++ b/examples/simple_system/rtl/ibex_simple_system.sv
@@ -115,7 +115,7 @@ module ibex_simple_system (
   logic instr_err;
 
   assign instr_gnt = instr_req;
-  assign instr_err = '0;
+  assign instr_err = (instr_addr & cfg_device_addr_mask[Ram]) != cfg_device_addr_base[Ram];

Expected Behavior

Instruction access address should be checked like done in data path (in the Bus module), so as to eliminate unwanted mismatches.

Steps to reproduce the issue

1. Build lowrisc:ibex:ibex_simple_system_cosim
2. Run Cosim with the provided testcase binary (which may need conversion? bin is not officially supported, I run with my modified simulating c++ code) build/lowrisc_ibex_ibex_simple_system_cosim_0/sim-verilator/Vibex_simple_system -c 10000 --meminit=ram,errors/3d1ff274213239e43245fca8b28c8867
3. Mismatch reported

ibex_instr_access_unchecked.zip

My Environment

master (commit 587e9fe)

EDA tool and version:
Verilator 5.006 2023-01-22 rev (Debian 5.006-3)

Operating system:

Version of the Ibex source code:

[rswarbrick]

Thanks for reporting this. The existing code looks wrong to me and is in an "example" folder, so there's a good chance we've never looked this corner case carefully.

For the proposed fix: I think something like that makes sense, but I'd probably suggest moving the assign line to be next to the u_ram instantiation, to keep everything about the RAM in the same place. I'd also suggest a second commit that moves the magic numbers that define signals like cfg_device_addr_mask[Ram] into localparams and that uses those parameters when instantiating u_ram.

Would you be up for creating a PR?

[ZhongYic00]

Of course very willing to do this 😊

[ZhongYic00]

Drafted. I'm new to participate in open RTL projects, don't know if everything is compliant.
For the 2nd commit, I just use localparam for two Ram magic nums, unsure if this meets your advice (restructure other params, or use global param to align with C++ part?)

[ZhongYic00]

Drafted. I'm new to participate in open RTL projects, don't know if everything is compliant. For the 2nd commit, I just use localparam for two Ram magic nums, unsure if this meets your advice (restructure other params, or use global param to align with C++ part?)

What's more, I noticed another series of mismatch events owing to the hardcoded RAM in ref model. It's larger than RTL side, so out-of-bound accesses raise invalid_access trap in RTL but messy bytes (illegal_instn) in cospike.

ibex/dv/verilator/simple_system_cosim/simple_system_cosim.cc

Line 32 in 587e9fe

_cosim->add_memory(0x00000000, 0xFFFF0000);