[otbn] Register local escalation signals for timing optimization

[etterli]

This implements the proposed optimization in #28345.

Signals contributing to the local controller escalation signal directly factor into the insn_executing signal inside the otbn_controller.sv. This then drives various commit signals as well as the DMEM request signal (lsu_load_req_o). By cutting these escalation signals, these paths are shortened which results in better timing/area results. Modeling this delayed escalation required changes in the otbnsim as well as in most of the vseqs.

The regression results are very similar to the last weekly run.
I tested all 11 failing seeds also on the current master and they fail there as well. As far as I understand, these fail due to general DV problems of how things are modeled. The pass rate for these tests heavily depend on the seed.

Let me know whether I should squash the commits adapting the vseqs (I left them separated for ease of reviewing).

[vogelpi]

Thanks @etterli , the RTL looks good to me. I've left some comments.

Please let me know when you think the PR is ready for review.

[etterli]

I integrated the feedback from @vogelpi and adapted the DV tests to match the delayed escalation behaviour. See also updated PR description.

[vogelpi]

Thanks @etterli for the PR, this looks good!

I've carefully reviewed the RTL and we would need a DV expert to review the DV code. Most of the commits are very well structured and accurately described. But I would collapse the last 6 commits into a single one. As far as I understand, you have created separate commits for the different vseqs. But this is not really required as we want to fix all the vseqs anyway.

[vogelpi]

CHANGE AUTHORIZED: hw/ip/otbn/rtl/otbn_core.sv

This PR implements an RTL change to relax the timing in OTBN. It has previously been discussed in the Security WG. This is fine.

[rswarbrick]

This is kind of cool, but I think it's a bit unconventional to define a clocking block like this. Would something like this work instead? (Completely untested!)

  clocking error_cb @(posedge clk_i);
    input logic software_err       = u_otbn_controller.software_err;
    input logic fatal_software_err = u_otbn_controller.fatal_software_err;
  endclocking

In otbn_base_vseq, you've currently got

          @(cfg.controller_vif.wait_sw_error_rises);

I think this could then be:

          wait(cfg.controller_vif.error_cb.software_error);

(assuming you don't care about if it's already set)

[etterli]

See comment above where this clocking block is used. It cannot be clocked on posedge of clk_i.

[rswarbrick]

I think I understand what you're describing now (after reading the existing code more carefully).

Imagining that there's a posedge of the clock at each integer time, I think the timing looks something like this with the current code:

• At time 0 a bad thing happens, causing u_otbn_controller.software_err to go high.
• The clocking block fires, causing @(cfg.controller_vif.wait_sw_error_rises); to complete.
• We send an error escalation on the vif (also at time zero)
• The send_err_escalation task sets the wakeup_iss flag then waits until the next clock edge (time 1)
• We then wait until time 2.

My suggestion wouldn't work! The problem is that we'd only see u_otbn_controller.software_err going high at time 1, which would be too late to send the escalation (because the model might have been stepped already by a fluke of scheduling).

However, it would work if we switched my proposed clocking block to the negedge. Then I think the vseq can become just this:

    fork begin : isolation_fork
      fork
        begin
          otbn_pkg::err_bits_t err_bits;
          string err_type;

          // Wait for a SW error of some sort (we only expect one to happen at once). The clocking
          // block is on the negedge of the clock.
          wait(cfg.controller_vif.error_cb.software_error or
               cfg.controller_vif.error_cb.fatal_software_error);

          if (cfg.controller_vif.error_cb.fatal_software_error) begin
            err_bits = '{fatal_software: 1'b1, default: 1'b0};
            err_type = "fatal";
          end else begin
            err_bits = '{illegal_insn: 1'b1, default: 1'b0};
            err_type = "non-fatal";
          end

          // Tell the model about the injected error
          `uvm_info(`gfn, $sformatf("The injection led to a %0s SW error", err_type), UVM_LOW)
          cfg.model_agent_cfg.vif.send_err_escalation(err_bits);
        end
        begin
          // Handle HW based escalation
          // Do nothing yet. We signal the error in the next cycle.
          @(cfg.clk_rst_vif.cb);
        end
      join_any
      disable fork; // Kill the SW handling forks if the HW case completes first.
    end join

[etterli]

Actually the timing is slightly different (injection happens already on negedge) and both SW errors can occur at the same time. The fatal SW error is triggered in addition in case any SW error should be treated as fatal error defined by the CTRL register. The model then requires both notifications.

Let me explain the timing using otbn_rf_base_intg_err_vseq as example.

• This test waits on run_until_use() until the RF is read. The test will continue at the negedge of clk in the cycle the RF is used.
• We then inject the error which will lead to a SW error (i.e., because the modified register value will result in an invalid branch target address).
• We notify the model with cfg.model_agent_cfg.vif.tolerate_result_mismatch(1); that the next committed result may be wrong. Should be irrelevant to the timing (as long no delta cycle is advanced).
• Now we have to wait until the next clk posedge to then send the HW escalation triggered by the violated integrity. At the same time we must send any SW error to the model before the model steps for the current cycle. I did this waiting with my unconventional clocking block.

I tried your waiting on the negedge but then the clocking block is too late and it does not fire a SW error.

Honestly, I'm now also surprised that my clocking on the actual signal does work. I think it only works because there is nothing (no delta cycle) between the error injection and my clocking block..? But it seems very susceptible to race conditions. Especially as the event happens before we "wait" (@cfg.controller_vif.wait_sw_error_rises) on the event after we triggered the event (Is event the correct wording in SV?). The same also applies in a similar way to your solution or am I missing something?

[etterli]

So instead of creating a clocking block, we could also wait directly on the actual signal.

@(cfg.controller_vif.software_err);

This requires to add the software_err signal to the otbn_controller_if port but that is easy.
Using wait instead of @ would also work.

See the newly pushed version.

[rswarbrick]

This all looks rather complicated to me, and I'm surprised if it works. I think the third thread will always complete first and there's actually a race at the moment between that and the middles of the other two. Assuming that u_otbn_controller.software_err changes on the clock, the earliest that line 985 can complete is racing against line 1002. If you're unlucky, line 1002 will win and we'll never see the error.

I'm very happy to sketch out something robust, but I'm not certain I can figure out what's supposed to be happening. Can you leave a note that explains the timings you're trying to model?

[etterli]

No the trick is that u_otbn_controller.software_err changes immediately when a SW error occurs / we inject an error which results in a SW error (it is combinatorial). What I want to do is to reuse the RTL to decide whether the injected error leads to a SW error or not. Otherwise the DV code would have to do this and this depends highly on the actual RTL.

In context: This task is called after the error injection and the @cfg.controller_vif.wait_sw_error_rises basically just checks in the next simulator delta cycle whether the injection led to a SW error or not. This check is required because a SW error will still lead to an immediate escalation (not delayed) whereas a fatal error like a predecode error will escalate in the next cycle (so when @(cfg.clk_rst_vif.cb) triggers).

We also need to distinguish between a fatal and non fatal SW as we either do or don't lock up OTBN at the end of the secure wipe (different error bits are set).

Does this explain what I want to achieve with this task? I agree it is probably not the cleanest way as I directly probe RTL signals..

[rswarbrick]

I think I understand (and thanks for explaining things). I think that my proposal (added to a previous note) with something on the negedge might be a bit simpler?

[etterli]

I incorporated the review from @rswarbrick. A full regression is still ongoing.

[etterli]

No the trick is that u_otbn_controller.software_err changes immediately when a SW error occurs / we inject an error which results in a SW error (it is combinatorial). What I want to do is to reuse the RTL to decide whether the injected error leads to a SW error or not. Otherwise the DV code would have to do this and this depends highly on the actual RTL.

In context: This task is called after the error injection and the @cfg.controller_vif.wait_sw_error_rises basically just checks in the next simulator delta cycle whether the injection led to a SW error or not. This check is required because a SW error will still lead to an immediate escalation (not delayed) whereas a fatal error like a predecode error will escalate in the next cycle (so when @(cfg.clk_rst_vif.cb) triggers).

We also need to distinguish between a fatal and non fatal SW as we either do or don't lock up OTBN at the end of the secure wipe (different error bits are set).

Does this explain what I want to achieve with this task? I agree it is probably not the cleanest way as I directly probe RTL signals..

[etterli]

See comment above where this clocking block is used. It cannot be clocked on posedge of clk_i.

[rswarbrick]

Thanks for all the explanation and changes. I think there are still a couple of small improvements to go, but it's looking really good!

[rswarbrick]

I think I understand what you're describing now (after reading the existing code more carefully).

Imagining that there's a posedge of the clock at each integer time, I think the timing looks something like this with the current code:

• At time 0 a bad thing happens, causing u_otbn_controller.software_err to go high.
• The clocking block fires, causing @(cfg.controller_vif.wait_sw_error_rises); to complete.
• We send an error escalation on the vif (also at time zero)
• The send_err_escalation task sets the wakeup_iss flag then waits until the next clock edge (time 1)
• We then wait until time 2.

My suggestion wouldn't work! The problem is that we'd only see u_otbn_controller.software_err going high at time 1, which would be too late to send the escalation (because the model might have been stepped already by a fluke of scheduling).

However, it would work if we switched my proposed clocking block to the negedge. Then I think the vseq can become just this:

    fork begin : isolation_fork
      fork
        begin
          otbn_pkg::err_bits_t err_bits;
          string err_type;

          // Wait for a SW error of some sort (we only expect one to happen at once). The clocking
          // block is on the negedge of the clock.
          wait(cfg.controller_vif.error_cb.software_error or
               cfg.controller_vif.error_cb.fatal_software_error);

          if (cfg.controller_vif.error_cb.fatal_software_error) begin
            err_bits = '{fatal_software: 1'b1, default: 1'b0};
            err_type = "fatal";
          end else begin
            err_bits = '{illegal_insn: 1'b1, default: 1'b0};
            err_type = "non-fatal";
          end

          // Tell the model about the injected error
          `uvm_info(`gfn, $sformatf("The injection led to a %0s SW error", err_type), UVM_LOW)
          cfg.model_agent_cfg.vif.send_err_escalation(err_bits);
        end
        begin
          // Handle HW based escalation
          // Do nothing yet. We signal the error in the next cycle.
          @(cfg.clk_rst_vif.cb);
        end
      join_any
      disable fork; // Kill the SW handling forks if the HW case completes first.
    end join

[rswarbrick]

I think I understand (and thanks for explaining things). I think that my proposal (added to a previous note) with something on the negedge might be a bit simpler?

[etterli]

@vogelpi @rswarbrick

I added an explanation on why sampling the SW error signals from the RTL model to control the behaviour of the DV model is ok. See this comment.

[vogelpi]

CHANGE AUTHORIZED: hw/ip/otbn/rtl/otbn_core.sv

This PR changes the RTL of OTBN to fix a whole set of critical timing paths. The DV is aligned in sync with these changes. This is fine.

[vogelpi]

@vogelpi @rswarbrick

I added an explanation on why sampling the SW error signals from the RTL model to control the behaviour of the DV model is ok. See this comment.

Thanks for your message and the additional comment @etterli. After our discussion last week, I thought more about this and I also think the approach is okay. As you explain in your newly added comment, the DV change does not detect when we're expected to see an error and then instructs the model accordingly, it just aligns the timing between model and RTL.

To make this more obvious, I would suggest the following:

1. Rename the function to convey the message that the purpose of this function is just to keep the timing of the model and the RTL aligned. Right now, the function name is handle_delayed_escalation() which is a lot more generic. Maybe optionally_delay_model() would work better. The name should not suggest that the DV code here changes the model behavior with respect to whether we see or not see an escalation. The escalation is always triggered.
2. Shorten the big comment before the function and move the parts describing the different forks/branches into the function. Before the function, you just need to outline what the higher-level purpose of the function is (i.e. ensure the model remains in sync with RTL depending on the type of error seen).
3. Create a separate GitHub issue where we can re-discuss whether we should change the model. We discussed a couple of approaches together and concluded that this approach here is okay. But we should also involve DV folks in that discussion. However, @rswarbrick is extremely busy with other critical work right now. My view is that the current approach is good to proceed for now and unblock other RTL work. If we decide to change the DV approach again for this, we can do so in the future. The problem is now well understood and required changes will mostly be in the model and in the now introduced function. This should be doable.

[vogelpi]

Also, we would need another CHANGE AUTHORIZE comment for this PR. Maybe @andreaskurth or @rswarbrick could help out please?

[rswarbrick]

CHANGE AUTHORIZED: hw/ip/otbn/rtl/otbn_core.sv

I'm convinced that this change doesn't contain a risk to the design (so it should be authorized). I believe that the DV side of thing could probably have been done more easily (as described in reviews), but that shouldn't gate the PR.

[etterli]

@vogelpi @rswarbrick

I incorporated the feedback from @vogelpi and created the issue #29026 to track the discussed special RTL probing.

I also improved the regression results by rebasing onto the latest master (now includes the otbn_sec_cm fix) as well as implementing some improvements in the DV framework. Especially for tests like otbn_ctrl_redun.

Now only few seeds for the tests otbn_partial_wipe and otbn_stress_all_with_rand_reset fail which do pass on the current master. However, these failures are mostly due to DV framework errors (like the simulator does not perfectly model the RTL). I will create issues describing the remaining DV issues later.

As reference for later:
Seeds which do fail with the delayed escalation but pass on the master:

• otbn_partial_wipe
  • 102652375855186875685574724665002460857039337799073695044744087342108108650176
• otbn_stress_all_with_rand_reset
  • 77796259482280258487775019503527232235529693308155587808744077118562121958794