Add SM2 implementation in generic riscv64 asm #25918
Conversation
t8m
added
branch: master
geliyaz
force-pushed
the
master
branch
2 times, most recently
from
b724adb to
2af3a2a
Compare
Sure! |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
how is the review going? |
|
Please drop the merge commit - |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
We still need one of the RISC-V savvy developers to have a look at this. |
crypto/ec/asm/ecp_sm2p256-riscv64.pl
Outdated
| add $b0, $t8, $b0 | ||
|
|
||
| // Select based on carry | ||
| bltu $c0, $b0, ${mod}_mod_add |
There was a problem hiding this comment.
(Don't know much about either SM2 or RISC-V, but daring to ask) Is this branch a side-channel? Or is there no requirement for constant-time in this function?
There was a problem hiding this comment.
Thank you for pointing that out! We've made adjustments to ensure the function operates in constant-time, reducing the risk of side-channel attacks. Here's the updated code:
slt $c0, $c0, $c3
negw $c1, $c0
addw $c0, $c0, -1
and $t0, $t0, $c1
and $t1, $t1, $c1
and $t2, $t2, $c1
and $t3, $t3, $c1
and $t4, $t4, $c0
and $t5, $t5, $c0
and $t6, $t6, $c0
and $t7, $t7, $c0
or $t0, $t0, $t4
or $t1, $t1, $t5
or $t2, $t2, $t6
or $t3, $t3, $t7
Let me know if you have any further concerns or suggestions!
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
Can you provide benchmark results run on a real RISC-V board for comparison, like in #20754 ? |
Yes, I've updated the benchmark results run on a real RISC-V board(SG2042) |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
Pinging some RISC-V code developers for code review @ZenithalHourlyRate @xicilion @HeliC829 @JerryShih @cyyself |
|
Kicked off the tests.. |
| # Function arguments | ||
| # Input block pointer, output block pointer, key pointer | ||
| my ($i0, $i1, $i2) = use_regs(10 .. 12); | ||
| my ($t0, $t1, $t2, $t3, $t4, $t5, $t6, $t7, $c0, $c1, $c2, $c3) = use_regs(5 .. 7, 13 ... 17, 28 .. 31); |
There was a problem hiding this comment.
What is the reason for reassigning t and s variables here to different values than riscv.pm?
There was a problem hiding this comment.
The reassignment of the t and s variables (primarily t here) is to manage register usage more clearly during assembly code generation, specifically to distinguish which registers are temporary (and do not need to be saved) from those that need to be saved (note that s registers are not used here, indicating that this function likely does not use any callee-saved registers).
github-actions
bot
added
the
severity: fips change
crypto/ec/asm/ecp_sm2p256-riscv64.pl
Outdated
| ld $t6, 16($i2) | ||
| ld $t7, 24($i2) | ||
|
|
||
| sltu $c2, $c0, $c3 |
There was a problem hiding this comment.
Where does $c3 come from (it doesnt look like its set here)
There was a problem hiding this comment.
The comparison instruction is unnecessary in this context, so it is redundant. I have modified it.
| } | ||
|
|
||
| sub bn_mod_div_by_2() { | ||
| my $mod = shift; |
There was a problem hiding this comment.
There is a long history in openssl of minimal comments in assembler files. This might be great for the author, but its probably not good for long term maintainability after it has been sitting in the code repository for many years.
Is there any chance that you could document this file a bit more to make it easier to follow and review?
e.g. for this function
// The template function bn_mod_div_by_2(a)
// returns (if a_is_odd ? a + mod : a) >> 1
// where mod is one of ...
I see the an EC implementation for a similar function that seems to do this with or's and shifts.
Maybe a few words explaining what is does would be useful.. Since it loks like you shift a first and then start adding to it..
Even the bn_sub function is hard to follow (at least for RISC-V).
The Additions of the carries hurt my brain...
There was a problem hiding this comment.
I‘ve added more comments, kindly conduct a re-review.
github-actions
bot
removed
the
severity: fips change
This PR adds the support of SM2 implementation for RISC-V 64 platform with base integer instruction set.
Testing
For testing, Built-in test suite was run using user-mode qemu. All tests pass.
First, configuring the build
Next, I ran the built-in test suite using make test. The summary is below:
benchmark results run on SG2042：
Checklist