[sw,otbnsim] Add KMAC interface to OTBNsim #29025
Conversation
h-filali
requested review from
andrea-caforio,
andreaskurth,
etterli and
nasahlpa
h-filali
force-pushed
the
otbnsim-kmac
branch
8 times, most recently
from
93a47e0 to
860684f
Compare
etterli
left a comment
etterli
left a comment
There was a problem hiding this comment.
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.
h-filali
force-pushed
the
otbnsim-kmac
branch
2 times, most recently
from
ad88033 to
2e98ad2
Compare
|
Thanks @etterli for your comments. Nice finds! I added some TODOs to make sure your comments aren't forgotten. I also adapted the documentation. |
h-filali
force-pushed
the
otbnsim-kmac
branch
4 times, most recently
from
ec230c6 to
1c22c51
Compare
h-filali
force-pushed
the
otbnsim-kmac
branch
from
40376cb to
57a97ad
Compare
hw/ip/otbn/dv/otbnsim/sim/wsr.py
Outdated
| from typing import List, Optional, Tuple | ||
| from .ext_regs import OTBNExtRegs | ||
| from .ispr import ISPR, DumbISPR, ISPRChange | ||
| from .kmac_ispr import KmacDataWSR |
There was a problem hiding this comment.
This kmac_ispr import is not supposed to be in this commit. It should come in the next one.
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
Lots and lots of comments, sorry. I guess this is the danger of an enormous PR :-/
hw/ip/otbn/dv/otbnsim/sim/wsr.py
Outdated
| Assumes that idx is a valid index (call check_idx to ensure this). | ||
|
|
||
| ''' | ||
| # KMAC_DATA_S0/1 should only be through the wrapper class. |
| if idx == 0x8: | ||
| return self.KMAC_DATA.read_unsigned(share_idx=0) | ||
|
|
||
| elif idx == 0x9: | ||
| return self.KMAC_DATA.read_unsigned(share_idx=1) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()).
| # 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() |
There was a problem hiding this comment.
| # 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() |
hw/ip/otbn/dv/otbnsim/sim/kmac.py
Outdated
| entry = None | ||
| if mode is not None and strength is not None: | ||
| entry = MODE_STRENGTH_TABLE.get((mode, strength)) |
There was a problem hiding this comment.
The .get() will return None by default if there is no match. You could specify another default if desired with .get(<key>, <YourDefault>).
| 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
force-pushed
the
otbnsim-kmac
branch
4 times, most recently
from
c0c2c71 to
cefe00b
Compare
|
Thanks @rswarbrick @etterli please LMK if this LGTY now! |
h-filali
force-pushed
the
otbnsim-kmac
branch
from
cefe00b to
b8d2f8a
Compare
h-filali
force-pushed
the
otbnsim-kmac
branch
from
b8d2f8a to
387b661
Compare
|
Thanks @rswarbrick for having another look. Your feedback should be addressed now. PTAL |
h-filali
force-pushed
the
otbnsim-kmac
branch
from
387b661 to
cffde69
Compare
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]>
h-filali
force-pushed
the
otbnsim-kmac
branch
2 times, most recently
from
33cdedd to
ea3d56c
Compare
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
One minor nit, but I'm very happy for this to land otherwise.
This has been a big PR: well done for getting it to this state!
hw/ip/otbn/data/wsr.yml
Outdated
| For masked operations, provide the first share here and the second share in KMAC_DATA_S1. | ||
|
|
||
| If masking is not required: | ||
| - Set one this share to the plaintext data. |
There was a problem hiding this comment.
Nit: I think this should probably be "Set this share" (and similarly for kmac_data_s1)
There was a problem hiding this comment.
Thanks for catching this one!
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
force-pushed
the
otbnsim-kmac
branch
from
ea3d56c to
49835fe
Compare
|
Thanks @rswarbrick for your thorough review. I'll merge this piece of work once CI passes! |
5 participants
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.