Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RISC-V support for the SHA256 #16710

Closed
wants to merge 1 commit into from
Closed

RISC-V support for the SHA256 #16710

wants to merge 1 commit into from

Conversation

marcfedorow
Copy link
Contributor

No description provided.

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

LGTM

@paulidale paulidale added approval: review pending This pull request needs review by a committer branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Sep 29, 2021
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 30, 2021
@t8m
Copy link
Member

t8m commented Sep 30, 2021

I know we already approved and merged the SHA-512 support. My question is - for other architectures that add new instructions for algorithm acceleration or any other extra instruction sets we usually support building "full featured" assembler code which contains fallback code paths for CPUs which do not support these instructions. Do these RISC-V PRs effectively make the asm build for RISC-V depending on these special instructions being supported?

Please note I do not know almost anything about RISC-V so it is possible that these instructions are widely available and for the rest building with no-asm is sufficient.

@marcfedorow
Copy link
Contributor Author

@t8m
I do not think that I can produce efficient assembler code like this.
However sha* instructions are not the only that increase performance.
I did not add inline assembler for instructions that can be automatically emitted by the compiler. They are mostly bit manipulation instructions (such as rotations, zero counting, etc).
If riscv-bitmanip instructions are present, compiler should emit them regardless of presence of zkn instructions.

To some point compiler generates more or less efficient code depending on what exact subset is supported. My patches should help it in that cases where it can not generate the most efficient code by itself.

Sorry if I misunderstood your question.

@t8m
Copy link
Member

t8m commented Sep 30, 2021

I try to ask differently - Other asm implementations for different architectures (such as x86, arm, ppc, or s390x) do runtime selection of the codepath (and thus extra instructions) used. So the code can be built once and run on different CPU models with different extra instructions (AVX2, SSSE3,....). Your patches do not do that, if I understand them correctly. They are just build-time and once you build so the instructions are used you cannot use the binaries on CPUs which do not support these sha512 or sha256 instructions.
I am saying that just for consideration.

@marcfedorow
Copy link
Contributor Author

marcfedorow commented Sep 30, 2021

Yes, you understand my patches correctly.
E.g. this code compiled with -march=rv64izkn will fail with cause 2 (illegal instruction) if run on hardware with no ZKN-extension.

The problem is: RISC-V currently has no consensus about the discovery mechanism. Previously it should be done by checking the machine-ISA register. Currently, both bit-manipulation and cryptographic extensions marked their MISA bits as deprecated (they now should be hard-wired to zero).

Here is the quote from the pre-release version:
The misa.K bit is no longer used to identify the scalar cryptography extension. Feature detection of scalar cryptography extensions will be done using the work-in-progress discovery mechanisms for RISC-V.
This applies to the release version too.

So now there is no consensual adequate discovery mechanism at this exact moment.
Previously I could use something like

shl x1 1 10 # reg1 = 1 << ('K' - 'A')
csrr x2 misa # read a control/status register w/ info about supported extensions 
and x1 x1 x2  # reg1 &= misa
beqz x1 fallback # if (!reg1) goto: fallback

But now it is impossible.

The only way it is possible to provide run-time fallbacks is to execute some instruction and then to catch an exception.
Probably another way will be developed -- if so (or if traps become the recommended way to check the extension presence) I will probably implement assembler with run-time fallbacks.
I do not think that it should be done now because the discovery mechanism is not frozen yet.

@t8m
Copy link
Member

t8m commented Sep 30, 2021

OK, thanks for the explanation. I think I can approve this now.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 30, 2021
@paulidale
Copy link
Contributor

The only way it is possible to provide run-time fallbacks is to execute some instruction and then to catch an exception.

Some platforms do exactly this. It's hideous.

@t8m
Copy link
Member

t8m commented Oct 1, 2021

Some platforms do exactly this. It's hideous.

Yeah that approach is causing multiple problems, let's not do it here.

@openssl-machine
Copy link
Collaborator

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.

@kroeckx
Copy link
Member

kroeckx commented Oct 2, 2021

At least one approach we use on some platforms, is that the OS can provide that information. On linux this would be using getauxval().

@paulidale
Copy link
Contributor

Good point @kroeckx

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 4, 2021
openssl-machine pushed a commit that referenced this pull request Oct 4, 2021
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16710)
@t8m
Copy link
Member

t8m commented Oct 4, 2021

Merged to master. Thanks for the contribution.

@t8m t8m closed this Oct 4, 2021
@marcfedorow marcfedorow deleted the sha256 branch October 4, 2021 09:30
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 14, 2022
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#16710)

(cherry picked from commit 657d192)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2022
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16710)

(cherry picked from commit 657d192)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants