RISC-V: GCM: Implement .ghash() #20078

cmuellner · 2023-01-18T18:30:36Z

The current RISC-V GCM implementation does not include a .ghash() callback. This is an issue for future improvements (i.e. adding new GCM implementations for the upcoming vector crypto ISA extensions). Therefore, this PR adds this missing piece.

Unfortunately, this was a bit more work than expected because the current implementation needed to be replaced by an improved .gmult() code sequence to free up registers so the loop in .ghash() could be realized without stack utilization.

During testing, it was observed that no test calls .ghash() with more than 1 block (i.e. 16 bytes) of data. Therefore, I wrote a new one. Plus, yet another one which also triggers another code path in gcm128.c (running into the GHASH_CHUNK limitation).

During the work it was observed that the RISC-V extension test macros deserve some clean-ups to make them easier to read (e.g. extension tests are only relevant for run-time decisions) and extend. The related changes are included in this series.

The .ghash() implementation itself is straightforward. It just includes the XOR-loop into the gmult() sequence.

All code has been tested using upstream QEMU.

Test instructions:

OPENSSL_riscvcap_base="rv64gc"
OPENSSL_riscvcap_zbc="rv64gc_zbc"
OPENSSL_riscvcap_zbc_zbb="rv64gc_zbc_zbb"
OPENSSL_riscvcap_zbc_zbkb="rv64gc_zbc_zbkb"

QEMU_CPU_BASE="rv64"
QEMU_CPU_ZBC="rv64,zbc=true"
QEMU_CPU_ZBC_ZBB="rv64,zbc=true,zbb=true"
QEMU_CPU_ZBC_ZBKB="rv64,zbc=true,zbkb=true"

# Must succeed
OPENSSL_riscvcap=$OPENSSL_riscvcap_base QEMU_CPU=$QEMU_CPU_BASE ../build.sh test build noclean
OPENSSL_riscvcap=$OPENSSL_riscvcap_zbc QEMU_CPU=$QEMU_CPU_ZBC ../build.sh test build noclean
OPENSSL_riscvcap=$OPENSSL_riscvcap_zbc_zbb QEMU_CPU=$QEMU_CPU_ZBC_ZBB ../build.sh test build noclean
OPENSSL_riscvcap=$OPENSSL_riscvcap_zbc_zbkb QEMU_CPU=$QEMU_CPU_ZBC_ZBKB ../build.sh test build noclean

# Must fail (illegal instr)
OPENSSL_riscvcap=$OPENSSL_riscvcap_zbc QEMU_CPU=$QEMU_CPU_BASE ../build.sh test build noclean
OPENSSL_riscvcap=$OPENSSL_riscvcap_zbc_zbb QEMU_CPU=$QEMU_CPU_ZBC ../build.sh test build noclean
OPENSSL_riscvcap=$OPENSSL_riscvcap_zbc_zbkb QEMU_CPU=$QEMU_CPU_ZBC ../build.sh test build noclean

cmuellner · 2023-01-18T19:03:30Z

Updated PR to address CI errors.

cmuellner · 2023-01-18T19:39:06Z

Updated PR for yet another CI error (before we had warnings because of dynamic stack arrays, now zero-initializers were not allowed).

I've already sent the ICLA.

paulidale

Looks pretty good. Just a few small items.

I've not reviewed the RISC-V assembly, I'm not well enough acquainted with the architecture to do this.

include/crypto/riscv_arch.h

providers/implementations/ciphers/cipher_aes_xts_hw.c

test/aesgcmtest.c

crypto/modes/asm/ghash-riscv64.pl

cmuellner · 2023-01-20T14:14:36Z

Updated the PR:

removed the copyright date change
reverted the feature test macro renaming (back to RISCV_HAS_*)
removed the new test

cmuellner · 2023-01-24T22:03:38Z

Rebased this PR on top of 7b49d6a, which introduces cross-compile tests that trigger this PR. See #20107 for more details.

cmuellner · 2023-01-24T23:09:47Z

The build was failing because of the use Devel::StackTrace; in the Perl code: https://github.com/openssl/openssl/pull/20078/files#diff-e66a71edbf0eff8e8b1c5afbceac9ddf1e4b41990a8780befd6e47c91770e087R9

The error message was:

Can't locate Devel/StackTrace.pm in @inc (you may need to install the Devel::StackTrace module) (@inc contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at crypto/modes/asm/ghash-riscv64.pl line 9.

I've updated the PR to fail gracefully if the package perl-stacktrace is not present (there will be no helping stacktrace printed in this case).

t8m · 2023-01-25T09:24:24Z

It would be nice to get some independent review from someone who knows RISC-V assembly.

cmuellner · 2023-01-25T11:09:07Z

Ok, understood. I will try to find someone in the RISC-V community to do this review.

cmuellner · 2023-02-01T16:06:57Z

Updated the PR:

moved common Perl-Asm code into a Perl module (like other architectures have one as well)
make Perl-Asm running with use strict
rebased on upstream/master

ptomsich · 2023-02-07T14:54:03Z

Note that a specification change for GCM is in progress: riscv/riscv-crypto@19d044f

We will update this PR (over the course of the next days) as soon as the specification has settled.

paulidale · 2023-02-08T00:02:00Z

At what point should we expect the standardisation to be completed?

OpenSSL generally doesn't accept algorithms before they are published as a national or international standard.
I don't know if this would apply to instruction sets or not but will ask the question of the management committee.

ptomsich · 2023-02-08T00:17:21Z

Once the standard goes to the 45-day public review (called the "FREEZE"-milestone in RISC-V parlance), no semantic changes are expected. This is the point when the Linux kernel, GCC, etc. accept changes as well.
We expect FREEZE/45-day public review to happen by end of this quarter and we will then update the PRs appropriately with a link to the published specification.

However, to make it to FREEZE, prototype patches against upstream projects need to exist (not merged, but publicly available) and have been reviewed to ensure that the proposed extension can be supported in software (i.e., as an end-to-end proof-of-concept requirement).

Just a side-note: the current changes to the ghash instruction (creating a second instruction that is more gmult-like) have been triggered by the experience with exactly this pull-request.

openssl-machine · 2023-03-16T02:00:21Z

This pull request is ready to merge

paulidale · 2023-03-16T02:12:57Z

Merged, thanks for the contribution.

In RISC-V we have multiple extensions, that can be used to accelerate processing. The known extensions are defined in riscv_arch.def. From that file test functions of the following form are generated: RISCV_HAS_$ext(). In recent commits new ways to define the availability of these test macros have been defined. E.g.: #define RV32I_ZKND_ZKNE_CAPABLE \ (RISCV_HAS_ZKND() && RISCV_HAS_ZKNE()) [...] #define RV64I_ZKND_ZKNE_CAPABLE \ (RISCV_HAS_ZKND() && RISCV_HAS_ZKNE()) This leaves us with two different APIs to test capabilities. Further, creating the same macros for RV32 and RV64 results in duplicated code (see example above). This inconsistent situation makes it hard to integrate further code. So let's clean this up with the following steps: * Replace RV32I_* and RV64I_* macros by RICSV_HAS_* macros * Move all test macros into riscv_arch.h * Use "AND" and "OR" to combine tests with more than one extension * Rename include files for accelerated processing (remove extension postfix). We end up with compile time tests for RV32/RV64 and run-time tests for available extensions. Adding new routines (e.g. for vector crypto instructions) should be straightforward. Testing showed no regressions. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20078)

Move helper functions and instruction encoding functions into a riscv.pm Perl module to avoid pointless code duplication. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20078)

On systems where Devel::StackTrace is available, we can use this module to create more usable error messages. Further, don't print error messages in case of official register aliases, but simply accept them. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20078)

A recent commit introduced a Perl module for common code. This patch changes the GCM code to use this module, removes duplicated code, and moves the instruction encoding functions into the module. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20078)

The existing GCM calculation provides some potential for further optimizations. Let's use the demo code from the RISC-V cryptography extension groups (https://github.com/riscv/riscv-crypto), which represents the extension architect's intended use of the clmul instruction. The GCM calculation depends on bit and byte reversal. Therefore, we use the corresponding instructions to do that (if available at run-time). The resulting computation becomes quite compact and passes all tests. Note, that a side-effect of this change is a reduced register usage in .gmult(), which opens the door for an efficient .ghash() implementation. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20078)

RISC-V currently only offers a GMULT() callback for accelerated processing. Let's implement the missing piece to have GHASH() available as well. Like GMULT(), we provide a variant for systems with the Zbkb extension (including brev8). The integration follows the existing pattern for GMULT() in RISC-V. We keep the C implementation as we need to decide if we can call an optimized routine at run-time. The C implementation is the fall-back in case we don't have any extensions available that can be used to accelerate the calculation. Tested with all combinations of possible extensions on QEMU (limiting the available instructions accordingly). No regressions observed. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20078)

With different sets of available extensions a number of different implementation variants are possible. Quite a number of them are already implemented in openSSL or are in the process of being implemented, so pick the relevant openSSL coden and add suitable glue code similar to arm64 and powerpc to use it for kernel-specific cryptography. The prioritization of the algorithms follows the ifdef chain for the assembly callbacks done in openssl but here algorithms will get registered separately so that all of them can be part of the crypto selftests. The crypto subsystem will select the most performant of all registered algorithms on the running system but will selftest all registered ones. In a first step this adds scalar variants using the Zbc, Zbb and possible Zbkb (bitmanip crypto extension) and the perl implementation stems from openSSL pull request on openssl/openssl#20078 Co-developed-by: Christoph Müllner <christoph.muellner@vrull.eu> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 18, 2023

cmuellner force-pushed the riscv-ghash branch from 251807a to 91543f9 Compare January 18, 2023 19:01

cmuellner force-pushed the riscv-ghash branch from 91543f9 to 54a96e3 Compare January 18, 2023 19:37

paulidale reviewed Jan 19, 2023

View reviewed changes

include/crypto/riscv_arch.h Outdated Show resolved Hide resolved

providers/implementations/ciphers/cipher_aes_xts_hw.c Outdated Show resolved Hide resolved

test/aesgcmtest.c Outdated Show resolved Hide resolved

crypto/modes/asm/ghash-riscv64.pl Outdated Show resolved Hide resolved

t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Jan 19, 2023

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 19, 2023

paulidale mentioned this pull request Jan 20, 2023

Expand our RISC-V testing #20091

Open

cmuellner force-pushed the riscv-ghash branch from 54a96e3 to d668395 Compare January 20, 2023 14:06

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 20, 2023

cmuellner force-pushed the riscv-ghash branch from d668395 to 8f2ec58 Compare January 24, 2023 22:00

cmuellner force-pushed the riscv-ghash branch from 8f2ec58 to eb65344 Compare January 24, 2023 23:06

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Jan 25, 2023

t8m added the help wanted label Jan 25, 2023

cmuellner mentioned this pull request Jan 26, 2023

RISC-V vector crypto support #20149

Closed

cmuellner force-pushed the riscv-ghash branch from eb65344 to e385f3b Compare February 1, 2023 16:05

paulidale added the hold: need omc decision The OMC needs to make a decision label Feb 8, 2023

paulidale added approval: done This pull request has the required number of approvals tests: exempted The PR is exempt from requirements for testing and removed approval: review pending This pull request needs review by a committer tests: present The PR has suitable tests present labels Mar 15, 2023

openssl-machine 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 Mar 16, 2023

paulidale closed this Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RISC-V: GCM: Implement .ghash() #20078

RISC-V: GCM: Implement .ghash() #20078

cmuellner commented Jan 18, 2023 •

edited

cmuellner commented Jan 18, 2023

cmuellner commented Jan 18, 2023

paulidale left a comment

cmuellner commented Jan 20, 2023

cmuellner commented Jan 24, 2023

cmuellner commented Jan 24, 2023

t8m commented Jan 25, 2023

cmuellner commented Jan 25, 2023

cmuellner commented Feb 1, 2023

ptomsich commented Feb 7, 2023

paulidale commented Feb 8, 2023

ptomsich commented Feb 8, 2023

openssl-machine commented Mar 16, 2023

paulidale commented Mar 16, 2023

RISC-V: GCM: Implement .ghash() #20078

RISC-V: GCM: Implement .ghash() #20078

Conversation

cmuellner commented Jan 18, 2023 • edited

cmuellner commented Jan 18, 2023

cmuellner commented Jan 18, 2023

paulidale left a comment

Choose a reason for hiding this comment

cmuellner commented Jan 20, 2023

cmuellner commented Jan 24, 2023

cmuellner commented Jan 24, 2023

t8m commented Jan 25, 2023

cmuellner commented Jan 25, 2023

cmuellner commented Feb 1, 2023

ptomsich commented Feb 7, 2023

paulidale commented Feb 8, 2023

ptomsich commented Feb 8, 2023

openssl-machine commented Mar 16, 2023

paulidale commented Mar 16, 2023

cmuellner commented Jan 18, 2023 •

edited