[alert_handler] prim_alert_sender ignores a ping that arrives very shortly after the previous one
Nobody has claimed this yet.
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
### Description
I just spent a bit of time diagnosing an (initially mysterious) local test failure, but I think I eventually got to the bottom of things. The stimulus that causes the failure is that the testbench changes the value of an alert interface's `ping` signal very shortly after the corresponding ack drops for the previous ping.
The following screenshot shows the situation ("dut ports" gives the signals at the ports of a block that includes a `prim_alert_sender`; "prim_alert_sender ports" is the ports of the sender itself; the final section gives signals internal to the sender):
In fact, the ack and new ping are so close in time that they happen on the same clock cycle in the relevant `prim_alert_sender` instance (see `ack_p` and `ping_p` in the `prim_alert_sender ports` group and notice how the red bars are both in a single clock cycle).
The RTL in `prim_alert_sender` registers the "I'm handling a ping at the moment" state in a signal called `ping_trigger` (a confusing name!), which gets asserted when a change of ping value is seen (`ping_event`) and cleared again after the ack (`ping_clr`).
In the current design, `ping_clr` trumps `ping_event` and the `prim_alert_sender` will ignore the extra ping. This eventually causes a test failure because the testbench was sending the ping and doesn't expect the alert sender to fail to respond.
Why isn't this noticed by our existing FPV tests? Looking at `prim_alert_rxtx_assert_fpv.sv`, the first plausible assertion you find is `AlertPingIgnored_A`:
```
`ASSERT(AlertPingIgnored_A, (prim_alert_rxtx_tb.i_prim_alert_sender.state_q inside {
prim_alert_rxtx_tb.i_prim_alert_sender.PingHsPhase1,
prim_alert_rxtx_tb.i_prim_alert_sender.PingHsPhase2}) && $rose(ping_req_i) |->
ping_ok_o == 0 throughout ping_req_i [->1],
clk_i, !rst_ni || error_present || mubi4_test_true_strict(init_trig_i))
```
This doesn't actually say anything useful though! Indeed, I propose we remove it in #29647.
A more interesting assertion is `AlertPingOk_A`, which is currently checked in as:
```systemverilog
`ASSERT(AlertPingOk_A, !(prim_alert_rxtx_tb.i_prim_alert_sender.state_q inside {
prim_alert_rxtx_tb.i_prim_alert_sender.PingHsPhase1,
prim_alert_rxtx_tb.i_prim_alert_sender.PingHsPhase2}) && $rose(ping_req_i) |->
##[1:9] ping_ok_o,
clk_i, !rst_ni || error_present || init_pending)
```
That's hard to read, but it boils down to the following (a tidy-up in my local work branch):
```systemverilog
assign sender_is_pinging = i_prim_alert_sender.state_q inside
{i_prim_alert_sender.PingHsPhase1, i_prim_alert_sender.PingHsPhase2};
...
// If the alert receiver is asked to ping the alert sender when that block isn't already
// responding to a ping, the ping transaction will go through and the receiver will respond with
// ping_ok_o in at most 10 cycles.
AlertPingOk_A: assert property (!sender_is_pinging && $rose(ping_req_i) |-> ##[1:9] ping_ok_o);
```
Figuring out the exact timing is tricky here (because `ping_req_i` is an input to the receiver), but I think the point is that `prim_alert_receiver` never sends back-to-back ping requests like this, because of the `Pause0` and `Pause1` states in its FSM.
One possible path forwards would be to explicitly say that we expect this behaviour. The changes:
- Say something in the alert_handler documentation to mention this corner of the protocol. (This is a bit tricky, because the protocol is only really described from the receiver's point of view: tidying that up would be nice!)
- Add a "usage assertion" to `prim_alert_sender` that fails if the ping signal changes in the middle (or the very end) of an existing ping transaction.
Another option would be to make the protocol allow the situation. Allowing exactly this example is easy (a 1-line change in `prim_alert_sender.sv:184` to make `ping_event` trump `ping_clr`), but I'm not convinced this is the right thing to do: we just end up with another edge case where the ping is asserted at the same time as the ack drops.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with prim_alert_sender.sv around line 184 and trace how ping_event and ping_clr interact when an ack and new ping occur in one cycle. Read prim_alert_rxtx_assert_fpv.sv, especially AlertPingIgnored_A and AlertPingOk_A, then review the alert_handler documentation and #29647. Done means the protocol expectation or RTL behavior is explicitly resolved and covered by an appropriate assertion or test.
Written by the indexing model from the issue text.
Assessment
- Domain
- embedded-iot, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100