[sw,otbnsim] Add KMAC interface to OTBNsim by h-filali · Pull Request #29025 · lowRISC/opentitan

h-filali · 2026-01-05T13:38:10Z

This PR adds a new KMAC interface to OTBNsim.

This PR is rebased on #29165 so please review the first commit there.
The second commit unifies the classes for the CSRs and WSRs.
The third commit adds an implementation for the model.
The fourth commit connects the KMAC model to OTBN.
The fifth commit contains a working assembly example/test.

etterli

Thanks for this first implementation! It looks pretty good.

I reviewed the CSR/WSR and integration part but not yet completely the kmac.py as well as the software test. My feedback is mostly about error handling, otherwise the concept looks good to me.

hw/ip/otbn/dv/otbnsim/sim/csr.py

hw/ip/otbn/dv/otbnsim/sim/kmac.py

hw/ip/otbn/dv/otbnsim/sim/wsr.py

h-filali · 2026-01-06T14:24:03Z

Thanks @etterli for your comments. Nice finds! I added some TODOs to make sure your comments aren't forgotten. I also adapted the documentation.

sw/otbn/crypto/tests/kmac_test.s

hw/ip/otbn/dv/otbnsim/sim/wsr.py

hw/ip/otbn/dv/otbnsim/sim/csr.py

etterli · 2026-01-13T15:26:24Z

We should also check what must be changed for the iflow (information flow) declarations from the CSR / WSR instructions. See here or here. There are also some other fields like lsu in the instruction definition files which maybe must be adpated.

This can probably be postponed.

rswarbrick

I've only read the "Generalize WSRs" commit so far, so here are just some partial comments. But I really like this! I like the way that csr.py and wsr.py can be merged so much.

hw/ip/otbn/dv/otbnsim/sim/ispr.py

rswarbrick

I've only read part of the "Add new KMAC model" commit. I think it could be dramatically simpler, and I'm sorry for not having reviewed it a few weeks ago.

hw/ip/otbn/dv/otbnsim/sim/kmac.py

h-filali · 2026-02-02T15:39:17Z

Thanks @rswarbrick for your review. I changed the following things to address your feedback:

Changed TraceISPR to ISPRChange
Added the _ prefixes
Moved the KMAC_DATA_S0/1 logic from kmac.py to wsr.py
Addressed the nits

Do you think you could take another look @rswarbrick ?

etterli · 2026-02-02T17:19:01Z

hw/ip/otbn/dv/otbnsim/sim/wsr.py

+from typing import List, Optional, Tuple
+from .ext_regs import OTBNExtRegs
+from .ispr import ISPR, DumbISPR, ISPRChange
+from .kmac_ispr import KmacDataWSR


This kmac_ispr import is not supposed to be in this commit. It should come in the next one.

rswarbrick

Lots and lots of comments, sorry. I guess this is the danger of an enormous PR :-/

hw/ip/otbn/dv/otbnsim/sim/ispr.py

rswarbrick · 2026-02-02T16:57:10Z

hw/ip/otbn/dv/otbnsim/sim/wsr.py

+from typing import List, Optional, Tuple
+from .ext_regs import OTBNExtRegs
+from .ispr import ISPR, DumbISPR, ISPRChange
+from .kmac_ispr import KmacDataWSR


I don't think that file exists in this commit :-)

This still needs fixing, I think. And Pascal pointed it out too.

hw/ip/otbn/dv/otbnsim/sim/kmac.py

hw/ip/otbn/dv/otbnsim/sim/kmac_ispr.py

etterli

Thanks @h-filali for the update. I have some minor feedback but LGTM otherwise.

etterli · 2026-02-02T17:58:08Z

hw/ip/otbn/dv/otbnsim/sim/wsr.py

        Assumes that idx is a valid index (call check_idx to ensure this).

        '''
+        # KMAC_DATA_S0/1 should only be through the wrapper class.


NIT: Incomplete sentence?

etterli · 2026-02-02T18:08:05Z

hw/ip/otbn/dv/otbnsim/sim/wsr.py

+        if idx == 0x8:
+            return self.KMAC_DATA.read_unsigned(share_idx=0)
+
+        elif idx == 0x9:
+            return self.KMAC_DATA.read_unsigned(share_idx=1)


If we here extract the read for the KMAC data WSRs (which makes sense) then I think we should not have self.KMAC_DATA.shares[0] and self.KMAC_DATA.shares[1] in _by_idx. Otherwise the comment above saying that these should only be accessed via the wrapper is a little bit contradictory.

Ah, now I see why it is in _by_idx. It is there to simplify the on_start() and also we need these WSRs indexes to be present for check_idx() and has_value_at_idx(). These two are used in the BNWSRR and BNWSRW instructions to check whether the access is legal.

As this read_at_idx() is the only place where _by_idx is accessed to read the WSRs I agree that these should stay in _by_idx (same reasoning for write_at_idx()).

etterli · 2026-02-02T18:21:59Z

hw/ip/otbn/dv/otbnsim/sim/kmac.py

+        # Set the message write error if an illegal write to KMAC_DATA_S0/1 happened.
+        if self._wsrs.KMAC_DATA.shares_dirty() and \
+                not self._csrs.KMAC_IF_STATUS.get_msg_write_rdy():
+            self._csrs.KMAC_IF_STATUS.set_msg_write_error()


Suggested change

# Set the message write error if an illegal write to KMAC_DATA_S0/1 happened.

if self._wsrs.KMAC_DATA.shares_dirty() and \

not self._csrs.KMAC_IF_STATUS.get_msg_write_rdy():

self._csrs.KMAC_IF_STATUS.set_msg_write_error()

# Set the message write error if an illegal write to KMAC_DATA_S0/1 happened.

if (self._wsrs.KMAC_DATA.shares_dirty() and

not self._csrs.KMAC_IF_STATUS.get_msg_write_rdy()):

self._csrs.KMAC_IF_STATUS.set_msg_write_error()

etterli · 2026-02-02T18:28:18Z

hw/ip/otbn/dv/otbnsim/sim/kmac.py

+        entry = None
+        if mode is not None and strength is not None:
+            entry = MODE_STRENGTH_TABLE.get((mode, strength))


The .get() will return None by default if there is no match. You could specify another default if desired with .get(<key>, <YourDefault>).

Suggested change

entry = None

if mode is not None and strength is not None:

entry = MODE_STRENGTH_TABLE.get((mode, strength))

entry = MODE_STRENGTH_TABLE.get((mode, strength))

h-filali · 2026-02-05T14:43:11Z

Thanks @rswarbrick @etterli please LMK if this LGTY now!

rswarbrick · 2026-02-06T09:39:59Z

hw/ip/otbn/dv/otbnsim/sim/kmac.py

+            self._keccak_round_ctr.decrement()
+
+        # Handle KMAC_MSG_SEND command.
+        kmac_msg_send = self._csrs.KMAC_MSG_SEND.read_unsigned()


Thanks for the tweak (after this comment). I think you can get rid of the variable too: if self._csrs.KMAC_MSG_SEND.read_unsigned(): ?

The variable is reused later in the code, where it is useful for not going over the 100 char limit.

rswarbrick · 2026-02-06T09:43:23Z

hw/ip/otbn/dv/otbnsim/sim/kmac.py

+    This prevents updates within a cycle from taking effect until end_cycle() is called.
+    """
+
+    def __init__(self, max_val: Optional[int] = None, min_val: int = 0) -> None:


Do you ever supply min_val? If not, I'd probably remove the argument (for now, at least).

rswarbrick · 2026-02-06T09:45:24Z

hw/ip/otbn/dv/otbnsim/sim/kmac.py

+class Counter():
+    """A hardware-like counter model with separate Next (D) and Current (Q) states.
+
+    This prevents updates within a cycle from taking effect until end_cycle() is called.


It might be worth adding a note to the documentation that the value of the counter will always stay in the bounds?

hw/ip/otbn/dv/otbnsim/sim/kmac.py

rswarbrick · 2026-02-06T09:55:04Z

hw/ip/otbn/dv/otbnsim/sim/kmac.py

+        # Determine how many bytes are valid for this word from BYTE_STROBE.
+        byte_strobe = self._csrs.KMAC_BYTE_STROBE.read_unsigned()
+        byte_mask = (byte_strobe >> strobe_shift) & strobe_mask
+        num_bytes = byte_mask.bit_count()


The change looks good to me. Could you open a "what should happen for non-contiguous masks" issue? The behaviour needs documenting (even if we change our mind in the future).

rswarbrick · 2026-02-06T10:01:17Z

hw/ip/otbn/dv/otbnsim/sim/kmac_ispr.py

+        # In case of a write, commit the next state.
+        if self._next_value is not None:
+            self._value = self._next_value
+            # In case we are writing a command it needs to be cleared in the next cycle.


I'm really sorry: I know that I basically said the same thing three lines earlier: I must have missed this comment. It needs a comma after commend. There's also no really need for "in case": just this, maybe?

# If we are writing a command, it needs to be cleared in the next cycle.

(Aargh! I even copy-pasted the text in my proposed reordering. Sorry)

No worries, I'll fix it!

rswarbrick · 2026-02-06T10:09:37Z

hw/ip/otbn/data/csr.yml

+- name: kmac_if_status
+  address: 0x7d9
+  doc: |
+    Write a 1 to bits 1 or 2 to clear the error bits.


The fields of this register need to explicitly say what happens on a write. (I'd hope to see "w1c" or "write-one-clear" in the text somewhere, I think)

rswarbrick · 2026-02-06T10:11:36Z

hw/ip/otbn/data/csr.yml

+- name: kmac_intr
+  address: 0x7da
+  doc: |
+    Writing 1 to bit 0 of this register clears the interrupt.


Probably better to move this information into the field and just leave the second line at the register level.

rswarbrick · 2026-02-06T10:13:28Z

hw/ip/otbn/data/csr.yml

+- name: kmac_cfg
+  address: 0x7db
+  doc: |
+    This register allows OTBN to set the KMAC hashing mode (mode = SHAKE / cSHAKE / SHA3), enable or disable keyed KMAC mode (kmac_en = 0/1), and select the desired security strength (kstrength = L128 / L224 / L256 / L384 / L512).


This is essentially a long sentence with a list of the fields :-) I think the sets of allowed values belong on the fields below. This line could probably be much shorter. It could probably be really short, I think? "A configuration register for the KMAC interface."

rswarbrick · 2026-02-06T10:14:21Z

hw/ip/otbn/data/csr.yml

+- name: kmac_msg_send
+  address: 0x7dc
+  doc: |
+    This CSR consists of a single bit that, if set to one, will trigger OTBN to send the contents of KMAC_DATA_S0 and KMAC_DATA_S1 to KMAC.


I think this can be shorter: you've defined the field below anyway, haven't you?

rswarbrick · 2026-02-06T10:15:03Z

hw/ip/otbn/data/csr.yml

+- name: kmac_cmd
+  address: 0x7dd
+  doc: |
+    The register allows OTBN to issue START, PROCESS, RUN and DONE commands to KMAC.


Probably better to move the list of commands to the CMD field below?

rswarbrick · 2026-02-06T10:21:19Z

hw/ip/otbn/data/csr.yml

+  doc: |
+    Writes to this register are ignored.
+    This register exposes the error code from the KMAC HWIP ERR_CODE register.
+    The additional debug information from the KMAC HWIP ERR_CODE register is not displayed.


Nit: I'd probably use the same verb as the previous line (because I'm not sure what "display" means). I think we can also avoid a double negative with something like "No other information from the KMAC HWIP ERR_CODE register is exposed."

rswarbrick · 2026-02-06T10:22:43Z

hw/ip/otbn/data/wsr.yml

+
+    If masking is not required:
+    - Set one share (e.g., KMAC_DATA_S0) to the plaintext data.
+    - Set the corresponding share (e.g., KMAC_DATA_S1) to all-zeros.


I think you probably want "other share" here (and the same for KMAC_DATA_S1 below)

rswarbrick · 2026-02-06T10:28:21Z

hw/ip/otbn/dv/otbnsim/sim/csr.py

            # RND_PREFETCH register
            return 0

+        if idx in {0x7d9, 0x7da, 0x7db, 0x7dc, 0x7dd, 0x7de, 0xfc2, 0xfc3}:


It might be cleaner to do it like this, avoiding repeating the indices and lookup:

kmac_reg = self._by_idx.get(idx) if kmac_reg is not None: return kmac_reg.read_unsigned()

The same goes for the writes below.

rswarbrick · 2026-02-06T10:29:21Z

hw/ip/otbn/dv/otbnsim/sim/csr.py

+            0xfc3,  # KMAC_ERROR
+        }
+
+        self._by_idx = {


I think this name could be more informative. Maybe something like _idx_to_kmac_reg or something?

rswarbrick · 2026-02-06T10:44:54Z

hw/ip/otbn/dv/otbnsim/sim/csr.py

+        self.KMAC_CMD = KmacCommandCSR('KMAC_CMD', write_mask=0x3f)
+        self.KMAC_BYTE_STROBE = DumbISPR('KMAC_BYTE_STROBE', width=32)
+
+        self._known_indices = {


It's a pity that we have to repeat the addresses below when we're doing the lookups. Couldn't this be something like the following?

self._idx_to_csr = { 0x7c0: self.flags.groups[0] # FG0 0x7c1: self.flags.groups[1] # FG1 0x7c8: self.flags # The typing is wrong here! But I think we just # need FlagGroups to derive from ISPR # 0x7d0 - 0x7d8 really aren't standard CSRs. Probably they need # special-casing in check_idx / read_unsigned / write_unsigned 0x7d8: ??? # I'd suggest modelling RND_PREFETCH behaviour # explicitly (since the modelling is currently split # between read_unsigned and write_unsigned 0x7d9: self.KMAC_IF_STATUS, 0x7da: self.KMAC_INTR, 0x7db: self.KMAC_CFG, 0x7dc: self.KMAC_MSG_SEND, 0x7dd: self.KMAC_CMD, 0x7de: self.KMAC_BYTE_STROBE, 0xfc0: ??? 0xfc1: ??? # Similarly, I'd suggest modelling the CSR interface to # RND and URND explicitly. To avoid the circular # dependency, I'd suggest defining CSRs in terms of WSRs # and making WSRFile an argument to the CSRFile # constructor. 0xfc2: self.KMAC_STATUS, 0xfc3: self.KMAC_ERROR, }

I tried to find a generic solution that works for all the "special" registers. Please check if that fits your vision.

h-filali · 2026-02-06T15:09:53Z

Thanks @rswarbrick for having another look. Your feedback should be addressed now. PTAL

This commit changes the WSR and DumbWSR classes to be more generic. This allows code sharing between the WSR classes and potential new classes with different register size but the same functionality. Signed-off-by: Hakim Filali <[email protected]>

This commit adds a new KMAC model that can be used to model a KMAC-OBTN interface. This model can be used to interface KMAC from OTBN. This model should behave like the KMAC HW IP. Signed-off-by: Hakim Filali <[email protected]>

Use the new KMAC OTBNsim model to extend OTBNsim. This commit adds new WSRs and CSRs to OTBNsim and connects them to the new KMAC model. Signed-off-by: Hakim Filali <[email protected]>

This commit adds an assembly test that tests all allowed combinations of KMAC modes and kstrengths. This commit shall also serve as a first example of how to use the interface. Signed-off-by: Hakim Filali <[email protected]>

h-filali requested review from andrea-caforio, andreaskurth, etterli and nasahlpa January 5, 2026 13:38

nasahlpa requested a review from vogelpi January 5, 2026 13:51

h-filali force-pushed the otbnsim-kmac branch 8 times, most recently from 93a47e0 to 860684f Compare January 5, 2026 16:42

etterli reviewed Jan 6, 2026

View reviewed changes

h-filali force-pushed the otbnsim-kmac branch 2 times, most recently from ad88033 to 2e98ad2 Compare January 6, 2026 14:22

h-filali added the CI:Rerun Rerun failed CI jobs label Jan 6, 2026

github-actions bot removed the CI:Rerun Rerun failed CI jobs label Jan 6, 2026

andrea-caforio reviewed Jan 6, 2026

View reviewed changes

sw/otbn/crypto/tests/kmac_test.s Show resolved Hide resolved

andrea-caforio reviewed Jan 7, 2026

View reviewed changes

sw/otbn/crypto/tests/kmac_test.s Show resolved Hide resolved

etterli reviewed Jan 7, 2026

View reviewed changes

hw/ip/otbn/dv/otbnsim/sim/wsr.py Outdated Show resolved Hide resolved

etterli reviewed Jan 8, 2026

View reviewed changes

hw/ip/otbn/dv/otbnsim/sim/wsr.py Outdated Show resolved Hide resolved

etterli reviewed Jan 8, 2026

View reviewed changes

hw/ip/otbn/dv/otbnsim/sim/csr.py Outdated Show resolved Hide resolved

h-filali force-pushed the otbnsim-kmac branch from 2e98ad2 to d3c611d Compare January 9, 2026 10:34

h-filali force-pushed the otbnsim-kmac branch 4 times, most recently from ec230c6 to 1c22c51 Compare January 20, 2026 10:51

h-filali force-pushed the otbnsim-kmac branch 2 times, most recently from bed7fb1 to b12d29d Compare January 28, 2026 10:11

rswarbrick reviewed Jan 29, 2026

View reviewed changes

hw/ip/otbn/dv/otbnsim/sim/ispr.py Show resolved Hide resolved

hw/ip/otbn/dv/otbnsim/sim/ispr.py Show resolved Hide resolved

rswarbrick reviewed Jan 29, 2026

View reviewed changes

h-filali force-pushed the otbnsim-kmac branch 3 times, most recently from 1459d48 to 40376cb Compare February 2, 2026 15:25

etterli mentioned this pull request Feb 2, 2026

[otbn] Define SIMD instructions #29231

Open

h-filali force-pushed the otbnsim-kmac branch from 40376cb to 57a97ad Compare February 2, 2026 17:09

etterli reviewed Feb 2, 2026

View reviewed changes

rswarbrick reviewed Feb 2, 2026

View reviewed changes

etterli approved these changes Feb 2, 2026

View reviewed changes

h-filali force-pushed the otbnsim-kmac branch 4 times, most recently from c0c2c71 to cefe00b Compare February 5, 2026 14:36

h-filali force-pushed the otbnsim-kmac branch from cefe00b to b8d2f8a Compare February 6, 2026 09:54

rswarbrick reviewed Feb 6, 2026

View reviewed changes

h-filali force-pushed the otbnsim-kmac branch from b8d2f8a to 387b661 Compare February 6, 2026 15:08

h-filali force-pushed the otbnsim-kmac branch from 387b661 to cffde69 Compare February 6, 2026 15:11

h-filali added 2 commits February 6, 2026 16:29

[sw,otbnsim] Add new KMAC model for OTBNsim

8916607

This commit adds a new KMAC model that can be used to model a KMAC-OBTN interface. This model can be used to interface KMAC from OTBN. This model should behave like the KMAC HW IP. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the otbnsim-kmac branch from cffde69 to 33cdedd Compare February 6, 2026 15:31

h-filali added 2 commits February 6, 2026 16:32

[sw,otbn] Add KMAC interface to OTBNsim

91efd64

Use the new KMAC OTBNsim model to extend OTBNsim. This commit adds new WSRs and CSRs to OTBNsim and connects them to the new KMAC model. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the otbnsim-kmac branch from 33cdedd to ea3d56c Compare February 6, 2026 15:32

Conversation

h-filali commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etterli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

h-filali commented Jan 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etterli commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rswarbrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rswarbrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

h-filali commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rswarbrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

h-filali commented Feb 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-filali commented Jan 5, 2026 •

edited

Loading

etterli commented Jan 13, 2026 •

edited

Loading