Skip to content

[crypto/mlkem] Add main subroutines for ml-kem1024 - #30719

Merged
andrea-caforio merged 1 commit into
lowRISC:masterfrom
siemen11:mlkemphase1
Aug 3, 2026
Merged

[crypto/mlkem] Add main subroutines for ml-kem1024#30719
andrea-caforio merged 1 commit into
lowRISC:masterfrom
siemen11:mlkemphase1

Conversation

@siemen11

Copy link
Copy Markdown
Contributor

Add the main arithmetic and subroutines for the ml-kem1024 in the otbn.

@siemen11
siemen11 force-pushed the mlkemphase1 branch 17 times, most recently from 4b0ff34 to af2819f Compare July 13, 2026 07:30
@siemen11 siemen11 added the CI:Rerun Rerun failed CI jobs label Jul 13, 2026
@github-actions github-actions Bot removed the CI:Rerun Rerun failed CI jobs label Jul 13, 2026

@andrea-caforio andrea-caforio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @siemen11. ML-KEM is on the map now. :-) I did an initial review, haven't checked some of the logic-heavy parts yet.


/* Basecase even products c0 = a0*b0 + a1*b1*gamma */
bn.mulvm.8S w3, w0, w1
bn.mulvm.8S w4, w3, w2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work without a dummy addition in between? Our Montgomery multiplier does not do the conditional subtraction, so 2 subsequent multiplications might be incorrect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that ML-KEM works on 12 bit values, not like ML-DSA with 24 bits, so we can leave the overflow unhandled for a bit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The following addition is not a problem either I guess? Just to verify for myself because the operands for the addition are together larger than 2q but the addition is only well-defined for < 2q.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I understand it is that the outputs from bn.mulvm are strictly bounded by q, so the sum of any two multiplication outputs is at most 2q

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output of the Montgomery mutiplier used to be in [0, q[ but that was removed in later versions of the SIMD extension.

https://opentitan.org/book/hw/ip/otbn/doc/isa.html#bnmulvmhttps://opentitan.org/book/hw/ip/otbn/doc/isa.html#bnmulvm

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way that I understand it is that, we have a, b smaller than q and q = 3329, a 12 bit prime.

When we call bn.mulvm.8S, we calculate
r = (ab + mq)/2^32 <= floor((a*b - q)/2^32) + q (because m <= w^32 - 1, the Mont constant)
But because a, b < q, that floor is actually equal to 0, so r stays still smaller than q

Specifically, this is also mentioned in the docu:
"Note that when chaining multiplications, the conditional subtraction can be postponed until after the last multiplication in case the initial inputs are in [0, 2q[ and q < (2^d)/4 holds."

We have d = 32, and we have (2^d) / 4 = (2^32) / 4 = 2^30 and q = 2^12

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some digging and I think the explanation of @siemen11 is correct (the w in m <= w^32 - 1 should be a 2). For ML-DSA is does not work because q is too big and the floor() is not 0. Because a*b = (q-1)^2 at most and s = (a*b - q) / 2^32 is > 1. This means that r <= q + s.

Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_sample.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_sample.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_sample.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_ntt.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_expand.s Outdated
@siemen11
siemen11 marked this pull request as draft July 13, 2026 22:34
@siemen11
siemen11 force-pushed the mlkemphase1 branch 5 times, most recently from 80cbc75 to faa95fb Compare July 15, 2026 09:58
@siemen11
siemen11 force-pushed the mlkemphase1 branch 2 times, most recently from 18cb044 to 222df8a Compare July 17, 2026 09:07
@siemen11
siemen11 force-pushed the mlkemphase1 branch 2 times, most recently from 25040d5 to dd8b9d8 Compare July 17, 2026 22:46

@etterli etterli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @siemen11 for bringing ML-KEM alive. I had a first view into the decode functions. Mainly added some comments about some specifics of OTBN instructions.

Comment thread sw/otbn/crypto/mlkem1024/tests/mlkem1024_arith_testgen.py Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
@siemen11
siemen11 force-pushed the mlkemphase1 branch 3 times, most recently from 0c52ce7 to a8bafd2 Compare July 22, 2026 13:45
@siemen11
siemen11 marked this pull request as ready for review July 22, 2026 13:54

@etterli etterli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements on the decoding. I now reviewed also the encoding functions.

Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_decoding.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_encoding.s
@siemen11
siemen11 force-pushed the mlkemphase1 branch 2 times, most recently from 7b96c79 to 8e6346a Compare July 24, 2026 10:00

@etterli etterli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the polynomial arithmetic operations. Nicely optimized computations.

Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_arith.s Outdated

/* Basecase even products c0 = a0*b0 + a1*b1*gamma */
bn.mulvm.8S w3, w0, w1
bn.mulvm.8S w4, w3, w2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some digging and I think the explanation of @siemen11 is correct (the w in m <= w^32 - 1 should be a 2). For ML-DSA is does not work because q is too big and the floor() is not 0. Because a*b = (q-1)^2 at most and s = (a*b - q) / 2^32 is > 1. This means that r <= q + s.

Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_arith.s Outdated
Comment on lines +118 to +119
/* Zero w31 to guarantee zero register for mask generation */
bn.xor w31, w31, w31

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this or is in ML-KEM defined, similar to ML-DSA, that w31 is always 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decaps uses w31... I need to optimize this still and see whether I can move it out

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the w31 there for now, maybe later we can optimize it

Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_arith.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_arith.s Outdated
Comment thread sw/otbn/crypto/mlkem1024/mlkem1024_arith.s Outdated
@siemen11
siemen11 force-pushed the mlkemphase1 branch 6 times, most recently from 9a3fb9a to 63ef84e Compare July 25, 2026 14:02
@siemen11
siemen11 requested a review from nasahlpa July 27, 2026 18:50
@siemen11 siemen11 added the CI:Rerun Rerun failed CI jobs label Jul 27, 2026
@github-actions github-actions Bot removed the CI:Rerun Rerun failed CI jobs label Jul 27, 2026
Add the main arithmetic and subroutines for the ml-kem1024 in the otbn.

With thanks to Andrea Caforio <andrea.caforio@lowrisc.org>

Signed-off-by: Siemen Dhooghe <sdhooghe@google.com>
@siemen11 siemen11 added the CI:Rerun Rerun failed CI jobs label Jul 30, 2026
@github-actions github-actions Bot removed the CI:Rerun Rerun failed CI jobs label Jul 30, 2026

@andrea-caforio andrea-caforio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go. :-)

@andrea-caforio
andrea-caforio added this pull request to the merge queue Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants