[EDN] V2 Review Items

[senelson7]

This issue captures AIs that came up in the V2 meeting for EDN:

• Steve: Fix formatting/link errors in the dv doc
• Steve: Implement all covergroups listed in testplan
• Steve: Add cross of num_endpoints/mode in edn_cfg_cg
• Steve: Remove "endpoint requests completed at various times" from edn_endpoints_cg
• Steve: Add cross between coverpoints in cs_cmds_cg
• Steve: Reformulate edn_genbits_cg to cover random delays on CSRNG/EDN interfaces (host/device) and add to agent if not already in PP_agent
• Steve: Ensure CSRNG/EDN (host/device) delays are enabled in agent
• Sri/MSF/Martin: Create issue for chip-level tests [entropy_complex] Corner case tests at the chip level #10970
• Muqing: Modify err_test_cg to use enum vs int and implement
• Steve: Verify with Mark/Martin that min/max delays are appropriate
• Weicai: Make fix for instruction type in TL-UL (Done via [dv] Add a TL error case - write with instr_type = True #12015)
• Steve: Randomize csrng_req_bus to increase coverage
• Steve: Add test to toggle rsp_sts bit
• Rasmus/Weicai: Add common exclusion for alert ping lines (Done via [tools/xcelium] updated common coverage exclusions  #11021)
• Weicai: Clean up common testplans (Done via [dv] Clean up enable_reg_testplan #10972)
• Muqing/Steve: Verify EDN.MAIN_SM.CTR.LOCAL_ESC, EDN.CS_RDATA.BUS.CONSISTENCY are covered with error_test
• Mark/Steve: Newly discovered issue, see [edn] EDN doesn't support backpressure from CSRNG on RES/GEN commands #15469, see also [edn/rtl/dv] EDN handle cmd ready de-assert #15561
  All valid and ready combinations are covered in TB now: https://reports.opentitan.org/hw/ip/edn/dv/latest/cov_report/grp15.html
• Mark/Steve: Behavior when edn endpoints are disabled, see [edn] End point behavior when edn disabled #15349
  Two sequences: edn_disable, edn_disable_auto_req_mode
• Code coverage not at V2 levels (90%)

[moidx]

The latest nightly regression shows > 90% coverage for this block. Is there anything else pending to sign-off?

[moidx]

I see FSM coverage is still below 90%.

[msfschaffner]

This might be related to branches that we can exclude (due to escalation), but I don't know whether the coverage exclusions have been applied to this block yet.

[weicaiyang]

If the FSM coverage is due to D2S related features, we could just waive it and address during V2S.

[msfschaffner]

I quickly glanced over the coverage results and saw that the uncovered arcs in edn_main_sm.sv and edn_ack_sm.sv are almost exclusively due to *-> Error and *-> Idle transitions.

@weicaiyang didn't we discuss an option that would automatically exclude reset transitions from coverage collection?
But yes, it feels like we can wave FSM coverage closure to V2S.

[weicaiyang]

@weicaiyang didn't we discuss an option that would automatically exclude reset transitions from coverage collection? But yes, it feels like we can wave FSM coverage closure to V2S.

@msfschaffner
We exclude transitions triggered by reset only. Looks like it works.
In edn_ack_sm , the only missing transition is DataWait-> Idle, which can be caused by disabling the enable_i.
And Error -> Idle is gone, which can be triggered by reset only.

  always_comb begin
    state_d = state_q;
    ack_o = 1'b0;
    fifo_pop_o = 1'b0;
    ack_sm_err_o = 1'b0;
    unique case (state_q)
      Idle: begin
        if (enable_i) begin
          if (req_i) begin
            if (fifo_not_empty_i) begin
              fifo_pop_o = 1'b1;
            end
            state_d = DataWait;
          end
        end
      end
      DataWait: begin
        if (!enable_i) begin
          state_d = Idle;
        end else begin
          if (fifo_not_empty_i) begin
            state_d = AckPls;
          end
        end
      end
      AckPls: begin
        ack_o = 1'b1;
        state_d = Idle;
      end
      Error: begin
        ack_sm_err_o = 1'b1;
      end
      default: state_d = Error;
    endcase

Similar to edn_main_sm . *-> Idle can be triggered by disabling edn_enable_i.

Disabling EDN in the middle of the seq may be not very important. Perhaps we can leave it for V3.

[tjaychen]

actually that's quite important.
A lot of our entropy complexes get started by ROM and need to later be initialized by software. To do that we have to disable and make sure everything can move back to a clean state.

So we definitely should test these. @senelson7 previously merged #14534 for this, so it's probably something that should be tested as well.

[weicaiyang]

actually that's quite important. A lot of our entropy complexes get started by ROM and need to later be initialized by software. To do that we have to disable and make sure everything can move back to a clean state.

So we definitely should test these. @senelson7 previously merged #14534 for this, so it's probably something that should be tested as well.

Make sense. Thanks for pointing this out.

[senelson7]

I had code in a PR that forced the *->Error and *->Idle transitions but the consensus was it wasn't necessary just to get FSM transition coverage up. #14407 highlights this. Maybe this was just wrt reset and *->Error but I removed it all. Are you indicating you want the *->Idle due to not enabled to be put back? I probably had only done it for csrng so far but also planned to do it for edn.

[martin-lueker]

Hi @senelson7,
My understanding was that there would be sim exclusions paired with formal approaches to close the gap, and these would be done consistently across all IPs. My assumption though is that this is still pending. (All: is this just a problem for CSRNG/EDN or are there other gaps as well?)
Meanwhile, I take it as a really exciting sign that have just the FSM coverage left. This is actually really close!

[weicaiyang]

I had code in a PR that forced the *->Error and *->Idle transitions but the consensus was it wasn't necessary just to get FSM transition coverage up. #14407 highlights this. Maybe this was just wrt reset and *->Error but I removed it all. Are you indicating you want the *->Idle due to not enabled to be put back? I probably had only done it for csrng so far but also planned to do it for edn.

For *-> error, we could keep idle -> error as the common sec_cm V2S test can cover it. And add exclusions for non-idle -> error.

#14407 only exclude transition that only happens on reset. So it does't exclude all *->idle, as some can be triggered by enable=0. If I recall correctly, you had a PR that forced FSM to any state and do a reset, in order to hit these transition, which seems a bit hacky and didn't really test the enable function. Perhaps it's better to program EDN_ENABLE=0 in the middle of the operation and re-enable the EDN to ensure it works again.

[weicaiyang]

My understanding was that there would be sim exclusions paired with formal approaches to close the gap, and these would be done consistently across all IPs. My assumption though is that this is still pending. (All: is this just a problem for CSRNG/EDN or are there other gaps as well?)

The DV portion in #14407 is done. The transition that only happens on reset are automatically excluded now. It works for EDN. AES FSM coverage is improved to 90%+.

Meanwhile, I take it as a really exciting sign that have just the FSM coverage left. This is actually really close!

Indeed!

[msfschaffner]

The lint policy update in #14407 is pending (we have to wait for an updated policy file bundled with the next AscentLint release from RealIntent). However, I ran the new policy already locally and all FSMs in the entire chip conform to the new rule.

[msfschaffner]

Hence it sounds like the only thing that we still need to do for EDN is covering functional arcs to IDLE that are not due to the reset. Would the suggestion from @weicaiyang RE programming EDN_ENABLE=0 in the middle of ongoing operations work?

[senelson7]

Maybe, but it'll be totally random which state we are in when we disable. Maybe we can define what edn will be doing when it is disabled? If not, there are several cases to try to hit, i.e. no/some/all endpoints requesting, waiting for entropy, entropy delivered, etc. Same for csrng?

[weicaiyang]

there are several cases to try to hit, i.e. no/some/all endpoints requesting, waiting for entropy, entropy delivered, etc. Same for csrng?

These sound very interesting cases to hit. @martin-lueker may have better idea on what cases are good to test.

[weicaiyang]

As discussed in the last Si meeting, here are the takeaway.

1. it's good to have this transition (*->Idle) covered before signed off V2, as we may program EDN_ENABLE to disable EDN in real use cases.
2. randomly programming EDN_ENABLE=0 may not be able to hit all the cases. We may need to probe the internal state before programming it, which sounds like a reasonable approach.

@senelson7 Let me know if you think we need more discussion on this issue.

[cindychip]

All items covered. Close this issue