lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.55k stars 761 forks source link

[alert_handler] spurious alert/esc? #2321

Closed cindychip closed 4 years ago

cindychip commented 4 years ago

Hi Michael.

One question I have regarding spurious alert error. I would like to confirm if this spurious alert is expected. My sequence is (please refer to the screenshot):

  1. alert ping timeout set to 1. (1 clk cycle)
  2. ping comes, and after one clk cycle, alert p/n followed.
  3. the ping response fail will fire, because alert p/n comes in cycle 2.
  4. spurious alert also fired, from the RTL, because alert receiver detected an alert_ping, and categorized this alert as ping response, but already in alert_ping_timer, it has already timeout.

image

msfschaffner commented 4 years ago

Hey Cindy,

I think this is correct behavior, since every ping arriving outside of the expected window is per definition a spurious ping, see: https://github.com/lowRISC/opentitan/blob/e4e90cf757302a0491bf5515a578bf4c049fc9ff/hw/ip/alert_handler/rtl/alert_handler_ping_timer.sv#L144-L145

RE when this can occur, I think what you say is correct, yes. I.e., due to the ping_pending_q register here https://github.com/lowRISC/opentitan/blob/e4e90cf757302a0491bf5515a578bf4c049fc9ff/hw/ip/prim/rtl/prim_alert_receiver.sv#L96 the alert receiver modules will qualify differential handshakes as ping responses up to one cycle after ping_en_i has been deasserted.

Btwy, due to this ping_pending_q qualification in the receivers I believe this corner case is the only way how you can trigger a "spurious ping" pingfail without tampering with the design. However, if you where to randomly force any of the ping_ok_o signals during simulation, that should always trigger a pingfail via spurious ping detection, if the corresponding ping_en_i / ping_pending_q is not set.

cindychip commented 4 years ago

Thanks Michael for replying to this corner case. I will align it on the DV side then :) Yeah I personally think so long as this alert triggered some sort of interrupt/escalation, should be okay how we categorize it.

cindychip commented 4 years ago

PR #3107 is merged and the rerun passed. Close this issue.