Repository navigation
Conversation
t8m
left a comment
t8m
left a comment
There was a problem hiding this comment.
Could we get an independent review from another RISC-V developer?
t8m
added
branch: master
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. |
| 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 |
|
You should be able to see it... Not sure how this relates to your PR. As you are just doing asm changes you can ignore it.. It will get skipped when it runs next as I added a label to skip it. |
Not sure why, but on my end I'm only seeing a generic 'Error' message without detailed logs. |
|
Click on the "Style report" item about the error. |
This appears to be a Chrome-specific logging issue (detailed logs were visible when I switched to Firefox) I've investigated the style check failure and confirmed it's unrelated to my changes: |
|
Ping @vdukhovni for review. |
|
@vdukhovni Could you please take a look when you have time? Let me know if you have any questions. Thanks! |
|
@t8m @paulidale gentle ping for admin help on this one. |
|
I squashed the two commits and updated to the current master which has the improved RISC-V coverage testing. |
slontis
added
approval: done
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #25918)
|
Merged back to master.. Thanks for your contribution and patience. |
The ecp_sm2p256-riscv64.pl generator switches to .section .rodata to emit constant data (.Lpoly, .Lord, .Lpoly_div_2, .Lord_div_2), but never switches back before emitting function code. As a result, the function symbols defined by this file are assembled into .rodata instead of executable .text. On systems enforcing NX for .rodata, calling the RISC-V64 SM2 assembly faults immediately. Fix this by adding .previous after the constant block, restoring the initial .text section before function emission. Verified with readelf: before the fix, .text is empty and the function symbols are in .rodata; after the fix, constants remain in .rodata and the function symbols are in executable .text. Introduced in commit 05301b1 (PR #25918). Reviewed-by: Paul Yang <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> MergeDate: Mon Jul 20 09:47:49 2026 (Merged from #31874)
The ecp_sm2p256-riscv64.pl generator switches to .section .rodata to emit constant data (.Lpoly, .Lord, .Lpoly_div_2, .Lord_div_2), but never switches back before emitting function code. As a result, the function symbols defined by this file are assembled into .rodata instead of executable .text. On systems enforcing NX for .rodata, calling the RISC-V64 SM2 assembly faults immediately. Fix this by adding .previous after the constant block, restoring the initial .text section before function emission. Verified with readelf: before the fix, .text is empty and the function symbols are in .rodata; after the fix, constants remain in .rodata and the function symbols are in executable .text. Introduced in commit 05301b1 (PR #25918). Reviewed-by: Paul Yang <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> MergeDate: Mon Jul 20 09:47:35 2026 (Merged from #31874)
The ecp_sm2p256-riscv64.pl generator switches to .section .rodata to emit constant data (.Lpoly, .Lord, .Lpoly_div_2, .Lord_div_2), but never switches back before emitting function code. As a result, the function symbols defined by this file are assembled into .rodata instead of executable .text. On systems enforcing NX for .rodata, calling the RISC-V64 SM2 assembly faults immediately. Fix this by adding .previous after the constant block, restoring the initial .text section before function emission. Verified with readelf: before the fix, .text is empty and the function symbols are in .rodata; after the fix, constants remain in .rodata and the function symbols are in executable .text. Introduced in commit 05301b1 (PR openssl#25918). Reviewed-by: Paul Yang <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> MergeDate: Mon Jul 20 09:47:35 2026 (Merged from openssl#31874)
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