[sw, otbn] Align OTBNsim with the latest MAI RTL implementation by h-filali · Pull Request #30623 · lowRISC/opentitan

h-filali · 2026-07-02T21:12:40Z

This PR aligns the OTBN simuator with the new RTL implementation of the Mask Accelerator.
This PR also re enables the otbn smoke test lines that test the MAI.

etterli

Great work for such a complicated topic :)

I have a rather drastic simplification idea. Right now the masking accelerator exactly models the RTL. But couldn't we do the following which requires almost no RTL modeling:

The main idea is that the simulator only checks whether the final shared result produced by the RTL is correct. This means the simulator does:

Capture the input values when an element is dispatched. This is similar to as it was. The shared input values are just captured in a list.
The simulator then unmasks the inputs and performs the desired operation. It then masks the result with a fixed mask. The result is stored for later.
The simulator models the latency the same way as it already did.
In the last cycle:
- The simulator gets the result which RTL wants to write to the output WSRs.
- The simulator unmasks the result and compares it to the computed "golden" result.
- If both secrets match, we know that the RTL did the correct computations. The simulator then stores the values from the RTL in the WSRs.
- If not, something went wrong.

I see the following advantages with this approach

The MA model is much simpler.
The MA model automatically checks the correctness of the operation. I.e., it checks that secAdd performed the correct addition. There is no masking circuit which could be wrong both in RTL and simulator.
In the standalone mode, the simulator can simply return the "golden" result which was masked with a fixed value. This would simplify debugging OTBN applications.

etterli · 2026-07-03T05:58:31Z

        super().on_start()
        # On start, the default operation is set.
        self._operation = MaiOperation.A2B
+        self._raw_op: int = int(MaiOperation.A2B)


Why do we need a raw value? Does this model the operation value before the validity check is done?

I'm not sure yet but I think previously the simulator did raise a MAI_ERROR as soon as an invalid operation value was written to the CSR. The hardware however only raises the error when an execution is started with an invalid operation?

Yes because there's values that don't map to any operation. The raw bits are needed to check for invalid operations.

etterli · 2026-07-03T06:03:09Z

        else:
            self._delayed_insn_cnt_zero(1)

+        if self.state.wipe_cycles == WIPE_CYCLES - 97:


This magic number is prone to errors if WIPE_CYCLES ever changes. And I think this error is hard to catch. Can we solve this in a different way?

Could we just check for = 3? Because IIUC the update/cnt capture happens always in the 3rd last wipe cycle? This would remove the dependency on WIPE_CYCLES and would make this independent of the secure wipe length.

The wipe_cycles are counting down. So I wrote it this way such that if WIPE_CYCLES changes this is still correct.

etterli · 2026-07-03T06:11:25Z

+            MaiOperation.A2B: _MaskingAccelerator(self.wsrs.MOD, MaiOperation.A2B),
+            MaiOperation.B2A: _MaskingAccelerator(self.wsrs.MOD, MaiOperation.B2A),
+            MaiOperation.SECADD: _MaskingAccelerator(self.wsrs.MOD, MaiOperation.SECADD),
+            MaiOperation.SECADDMOD: _MaskingAccelerator(self.wsrs.MOD, MaiOperation.SECADDMOD),


Do we even need 4 objects anymore?

You're right this isn't necessary anymore. I'll remove it.

etterli · 2026-07-03T06:38:56Z

+        if self._writeback_idx >= _MaskingAccelerator.VEC_SIZE:
            self._writeback_idx = 0
-            # Reset the busy bit in the cycle where the last result is set.
-            self.csrs.MAI_STATUS.update_busy_bit(False)


ok to use _pending_busy_clear but why not just rely on update_busy_bit()?

Ah because update is immediately but we need to do the update one cycle later.

etterli · 2026-07-03T06:40:57Z

+            # _cnt was already captured from the wipe phase via on_sec_wipe_zero_step()
+            # (matching RTL's in_cnt_load_val_q set by sec_wipe_mai_i). Do NOT re-capture
+            # it here from the current URND — that would read the wrong cycle.
+            # Begin pushing inputs in the dispatch logic.


Suggested change

# _cnt was already captured from the wipe phase via on_sec_wipe_zero_step()

# (matching RTL's in_cnt_load_val_q set by sec_wipe_mai_i). Do NOT re-capture

# it here from the current URND — that would read the wrong cycle.

# Begin pushing inputs in the dispatch logic.

# _cnt was already captured from the wipe phase via on_sec_wipe_zero_step().

# Begin pushing inputs in the dispatch logic.

etterli · 2026-07-03T06:42:59Z

+            # Mirror RTL in_cnt_done path: reload _cnt from current URND.cnt so
+            # that a subsequent MAI operation within the same EXEC uses the right offset.
+            _, _, _, new_cnt = _urnd_fields(self.wsrs.URND._value)
+            self._cnt = new_cnt
+            # Immediately set the ready bit as the input WSRs can be overwritten in this cycle.


Suggested change

# Mirror RTL in_cnt_done path: reload _cnt from current URND.cnt so

# that a subsequent MAI operation within the same EXEC uses the right offset.

_, _, _, new_cnt = _urnd_fields(self.wsrs.URND._value)

self._cnt = new_cnt

# Immediately set the ready bit as the input WSRs can be overwritten in this cycle.

# Latch the random dispatch index for the next execution.

_, _, _, new_cnt = _urnd_fields(self.wsrs.URND._value)

self._cnt = new_cnt

# Immediately set the input-ready bit as the input WSRs can be overwritten in this

# cycle.

etterli · 2026-07-03T06:44:28Z

+        '''Return whether writing value to the MAI_CTRL CSR is currently allowed.

-        # We only allow setting the operation to valid options.
-        if not self.csrs.MAI_CTRL.is_valid_operation(value):
+        Mirrors RTL ispr_mai_sw_err logic in otbn_mai.sv.


Suggested change

'''Return whether writing value to the MAI_CTRL CSR is currently allowed.

# We only allow setting the operation to valid options.

if not self.csrs.MAI_CTRL.is_valid_operation(value):

Mirrors RTL ispr_mai_sw_err logic in otbn_mai.sv.

'''Return whether writing value to the MAI_CTRL CSR is currently allowed.

Direct references to RTL are hard to maintain.

etterli · 2026-07-03T06:47:14Z

+        # When start fires, the MAI must be idle and have a valid previously-registered op.
+        # Mirrors: ma_start & busy → sw_err; invalid_op = !(ma_mask_op_q inside {valid}) & ma_start


Please remove any RTL code copies (also at other locations) as it makes hard to maintain these parts. This is already outdated compared to master. See #30505. It depends on ma_mask_op_d and not _q anymore.

NIT: I suggest to use "must not be busy" as we have a busy flag.

Suggested change

# When start fires, the MAI must be idle and have a valid previously-registered op.

# Mirrors: ma_start & busy → sw_err; invalid_op = !(ma_mask_op_q inside {valid}) & ma_start

# When start fires, the MAI must not be busy and the next operation value must be a valid choice.

etterli · 2026-07-03T06:52:15Z

+        # NOTE: _cnt is intentionally NOT reset here.  It is captured during the
+        # preceding secure wipe via on_sec_wipe_zero_step() and must survive the
+        # on_start() call so that the first MAI operation uses the correct element
+        # ordering (matching RTL's in_cnt_load_val_q).


Suggested change

# NOTE: _cnt is intentionally NOT reset here. It is captured during the

# preceding secure wipe via on_sec_wipe_zero_step() and must survive the

# on_start() call so that the first MAI operation uses the correct element

# ordering (matching RTL's in_cnt_load_val_q).

# NOTE: _cnt is intentionally NOT reset here. It is captured during the

# preceding secure wipe via on_sec_wipe_zero_step() and must survive the

# on_start() call so that the first MAI operation uses the correct element

# ordering.

andrea-caforio

Thanks @h-filali. This is great!

This commit changes the MAI part of the OTBNsim to align it with the current state of the RTL. Signed-off-by: Hakim Filali <hfilali@lowrisc.org>

h-filali · 2026-07-07T10:33:27Z

Thanks @etterli and @andrea-caforio for having a look. I addressed all the comments raised above.

To address your main point @etterli I made the MAI model much faster. It is now vectorized and doesn't operate on a bit by bit level anymore. I also added a couple of new functions to align the randomness with the vectorized share bits (this includes a pre compute function that computes helper values once at the start). Lastly I created a fast path for the case where MAI is not in use or only part of the pipeline needs to be computed.

nasahlpa

Thanks Hakim for the implementation and addressing Pascals feedback. To unblock work, I'd suggest merging this - if there is additional feedback from Pascal once he is back from vacation we still can address it in a follow-up.

nasahlpa · 2026-07-07T12:58:07Z

But before merging, we should look into why
//sw/device/tests:otbn_isa_test_fpga_cw340_sival_rom_ext
is failing

Signed-off-by: Hakim Filali <hfilali@lowrisc.org>

h-filali requested review from andrea-caforio, etterli and nasahlpa July 2, 2026 21:12

h-filali requested review from a team as code owners July 2, 2026 21:12

h-filali requested review from engdoreis and hcallahan-lowrisc and removed request for a team July 2, 2026 21:12

h-filali force-pushed the otbn-sim-mask-acc branch 2 times, most recently from b1a6951 to 4206f76 Compare July 2, 2026 22:01

etterli reviewed Jul 3, 2026

View reviewed changes

h-filali force-pushed the otbn-sim-mask-acc branch 5 times, most recently from f7dd906 to 0be7249 Compare July 6, 2026 17:10

andrea-caforio approved these changes Jul 7, 2026

View reviewed changes

[sw,otbnsim] Adapt OTBNsim to the new mask accelerator RTL

2184855

This commit changes the MAI part of the OTBNsim to align it with the current state of the RTL. Signed-off-by: Hakim Filali <hfilali@lowrisc.org>

h-filali force-pushed the otbn-sim-mask-acc branch from 0be7249 to 7d5d7c3 Compare July 7, 2026 10:27

nasahlpa approved these changes Jul 7, 2026

View reviewed changes

h-filali force-pushed the otbn-sim-mask-acc branch 2 times, most recently from 6df33a6 to 5fc07cb Compare July 7, 2026 15:25

[sw,otbn] Re-enable secAdd for OTBN smoke test

f10a675

Signed-off-by: Hakim Filali <hfilali@lowrisc.org>

h-filali force-pushed the otbn-sim-mask-acc branch from 5fc07cb to f10a675 Compare July 7, 2026 15:39

		# When start fires, the MAI must be idle and have a valid previously-registered op.
		# Mirrors: ma_start & busy → sw_err; invalid_op = !(ma_mask_op_q inside {valid}) & ma_start

	# When start fires, the MAI must be idle and have a valid previously-registered op.
	# Mirrors: ma_start & busy → sw_err; invalid_op = !(ma_mask_op_q inside {valid}) & ma_start
	# When start fires, the MAI must not be busy and the next operation value must be a valid choice.

Uh oh!

Conversation

h-filali commented Jul 2, 2026

Uh oh!

etterli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrea-caforio left a comment

Choose a reason for hiding this comment

Uh oh!

h-filali commented Jul 7, 2026

Uh oh!

nasahlpa left a comment

Choose a reason for hiding this comment

Uh oh!

nasahlpa commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants