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

Update montgomery multiplication to use s2n-bignum's verified scalar bignum functions #1135

Merged
merged 21 commits into from
Aug 17, 2023

Conversation

aqjune-aws
Copy link
Contributor

@aqjune-aws aqjune-aws commented Aug 3, 2023

Description of changes:

This patch updates aws-lc's montgomery multiplication to use s2n-bignum's verified bignum functions.
This is a follow-up of #1114, and is splitted from #1108.
To selectively apply s2n-bignum to Graviton 2, this patch invokes CRYPTO_is_ARMv8_wide_multiplier_capable() and runs the s2n-bignum functions only when wide multipliers are not capable.

The performance numbers of RSA signing are as follows. Graviton 2 is used, and tool/bssl speed -filter RSA has been used. (Unit: ops/sec).

Bits Operation baseline AWS-LC s2n-bignum speedup vs baseline
2048 RSA sign 299.3 399 33.31%
  verify (fresh key) 10736.3 15491 44.29%
3072 RSA sign 95.4 113.2 18.66%
  verify (fresh key) 4917.7 6001.7 22.04%
4096 RSA sign 41.7 63.2 51.56%
  verify (fresh key) 2781.6 3451 24.07%

This is only adopted for AArch64 that has narrow multiplication instruction bandwidths.

Testing:

Tested via bssl speed -filter RSA

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@aqjune-aws aqjune-aws requested a review from a team as a code owner August 3, 2023 16:02
@aqjune-aws aqjune-aws changed the title Adopt s2n-bignum's verified scalar bignum functions to aws-lc's montgomery multiplication Update montgomery multiplication to use s2n-bignum's verified scalar bignum functions Aug 3, 2023
@dkostic
Copy link
Contributor

dkostic commented Aug 7, 2023

nice improvements, can we get perf numbers for GV3 and possibly Apple M1/M2?

crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
@aqjune-aws
Copy link
Contributor Author

aqjune-aws commented Aug 7, 2023

@dkostic For microarchitectures supporting fast enough multiplications (such as Graviton 3 and M1), the current version of s2n-bignum didn't bring effective performance improvement. To selectively apply s2n-bignum to Graviton 2, this patch invokes CRYPTO_is_ARMv8_wide_multiplier_capable() and runs the s2n-bignum functions only when wide multipliers are not capable.
This is the relevant part in arm_arch.h:

// The Neoverse V1 and Apple M1 micro-architectures are detected to enable
// high unrolling factor of AES-GCM and other algorithms that leverage a
// wide crypto pipeline and fast multiplier.
#define ARMV8_NEOVERSE_V1 (1 << 12)
#define ARMV8_APPLE_M1 (1 << 13)

crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Show resolved Hide resolved
@torben-hansen torben-hansen self-requested a review August 8, 2023 17:14
@darylmartin100 darylmartin100 requested review from nebeid and removed request for torben-hansen August 9, 2023 19:07
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
@aqjune-aws
Copy link
Contributor Author

aqjune-aws commented Aug 10, 2023

From the CI failure, it seems in ARM the s2n-bignum assembly files are not linked to executables if MY_ASSEMBLER_IS_TOO_OLD_FOR_AVX is set.

Relevant CMakeLists.txt lines: https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/CMakeLists.txt#L170-L171

Should we update the CMakeLists.txt lines in this PR?

=> After a discussion, crypto/fipsmodule/CMakeLists.txt is updated to do so.

@nebeid
Copy link
Contributor

nebeid commented Aug 10, 2023

From the CI failure, it seems in ARM the s2n-bignum assembly files are not linked to executables if MY_ASSEMBLER_IS_TOO_OLD_FOR_AVX is set.

Relevant CMakeLists.txt lines: https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/CMakeLists.txt#L170-L171

Should we update the CMakeLists.txt lines in this PR?

Thank you, aqjune-aws.
I think we always had the same functions from s2n-bignum supported for x86 and aarch64, that’s why we didn’t see this fix.

nebeid
nebeid previously approved these changes Aug 11, 2023
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Show resolved Hide resolved
crypto/fipsmodule/bn/montgomery.c Outdated Show resolved Hide resolved
crypto/curve25519/curve25519.c Show resolved Hide resolved
crypto/curve25519/curve25519.c Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/p384.c Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/p521.c Outdated Show resolved Hide resolved
@nebeid nebeid merged commit b706d7e into aws:main Aug 17, 2023
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.

6 participants