ossl: Add GCM support on powerpc64/powerpc64le (POWER8+)

[tpearson_raptorengineering.com]

Summary

Separate ossl's existing AES-NI GCM implementation into a common
ossl_aes_gcm.c and add conditionals to switch between OpenSSL's AES-NI
and POWER8 GCM routines depending on the architecture. Since the
existing AVX-512 implementation is less agnostic, move it into a
separate ossl_aes_gcm_avx512.c.

Additionally, import the required POWER8 GCM routines for both powerpc64
and powerpc64le from OpenSSL 3.1.2.

Test Plan

Tested using tools/tools/cryptocheck as well as through an IPSec VPN configured for aes-gcm

[sanastasio_raptorengineering.com]

• Whitespace/formatting cleanup
• Use powerpc64 compiler conditional instead of PPC
• Drop ppc_aes_gcm_{de,en}crypt_ wrapper functions in favor of macros

[sanastasio_raptorengineering.com]

Hi all, sorry for the ping. If anybody had time to review this, it would be greatly appreciated. We've deployed this patch internally on some of our production systems and in addition to the massive performance improvement, it has proven stable under various workloads.

[jhibbits]

In D44274#1017742, @sanastasio_raptorengineering.com wrote:

Hi all, sorry for the ping. If anybody had time to review this, it would be greatly appreciated. We've deployed this patch internally on some of our production systems and in addition to the massive performance improvement, it has proven stable under various workloads.

Hi @sanastasio_raptorengineering.com It looks fine from my (rather limited) perspective, but I would really like @jhb to review it, since he's played a lot in the OSSL module space.

[jhb]

@markj worked on the avx512 bits for ossl for AES-GCM so probably should look as well

[markj]

It looks like the ARM NEON implementation could be deduplicated too, but it isn't. Is there a reason?

sys/crypto/openssl/ossl_aes_gcm.c

54

83

Please follow style(9) for the prototype, i.e., the return type should be its own line and continuing lines should be indented by four spaces.

120

Please fix the style here.

sys/modules/ossl/Makefile

28

Do we now have two ossl_aes_gcm.c files? i.e., the one you added and the one under arm/?

The latter should perhaps be called ossl_aes_gcm_neon.c or similar.

[sanastasio_raptorengineering.com]

In D44274#1021011, @markj wrote:

It looks like the ARM NEON implementation could be deduplicated too, but it isn't. Is there a reason?

This implementation was started before the ARM gcm implementation was merged. Additionally, I don't have any ARM hardware on hand that could be used to test it and ensure that nothing breaks (which I was able to do for X86 despite our primary focus being PPC). I'd prefer it if someone already set up for FreeBSD/ARM development handles consolidation of the NEON GCM implementation if desired.

sys/modules/ossl/Makefile

28

Is this required by the kernel build system or some other tooling? As I mentioned, I don't have any ARM hardware on hand to test any changes to that side of things, so I'd prefer to avoid changing anything ARM-related if possible.

[sanastasio_raptorengineering.com]

• Update style as suggested

[sanastasio_raptorengineering.com]

Hi all, sorry for the ping. Could anybody take another look at this patch set?

[jhibbits]

sys/modules/ossl/Makefile

28

I agree with Mark, ossl_aes_gcm.c in arm should be renamed to ossl_aes_gcm_neon.c; if you look at sys/modules/ossl/Makefile, you see at the top both sys/crypto/openssl and sys/crypto/openssl/${MACHINE_CPUARCH} are included, so if you want sys/crypto/openssl/ossl_aes_gcm.c to be ignored for arm, it's best to rename the arm-specific file and reference that directly.

[sanastasio_raptorengineering.com]

• Rename ARM's ossl_aes_gcm.c to ossl_aes_gcm_neon.c and update references

[sanastasio_raptorengineering.com]

sys/modules/ossl/Makefile

28

Got it, that makes sense. I've renamed ARM's version as you recommend in the latest revision.

[markj]

Sorry for the delay. I'm traveling until the beginning of october and won't be able to do much serious review or testing until then, so it'll take me a bit longer to get back to this.

sys/crypto/openssl/powerpc64/aes-gcm-ppc.S

3 ↗

(On Diff #143658)

How was this file generated? It's missing the header included in other asm files here. It must be deterministically reproducible from the in-tree openssl implementation.

[sanastasio_raptorengineering.com]

sys/crypto/openssl/powerpc64/aes-gcm-ppc.S

3 ↗

(On Diff #143658)

This file was generated from upstream openssl-3.1.2 sources. The in-tree openssl implementation at src/crypto/openssl is at version 3.0.13 whereas the P9 GCM implementation was added in 3.1.0.

What would be the best route to get the upstream P9 GCM implementation into the in-tree OpenSSL? If it would be acceptable, I could simply backport the P9 GCM implementation to the in-tree OpenSSL and submit another patch for that.

[jhibbits]

Is there anything blocking this now?

[tpearson_raptorengineering.com]

@jhibbits I'm wondering the same thing. On my side this seems good, any chance we can get it merged? Raptor is still carrying this as a downstream patch in its OPNsense builds. Thanks!

[jhibbits]

sys/crypto/openssl/powerpc64/aes-gcm-ppc.S

3 ↗

(On Diff #143658)

I think the best route would be to bring in the minimal patch to the in-tree OpenSSL for adding what you need here. That should be a separate patch and review, then base this change on top of that.

Sorry for the back and forth over a long time, I do want to see this get in before 15. @markj may have a different opinion on how to best get this in, but I think the two-stage (openssl then this) is the most appropriate.

[jhb]

Another developer is also actively working on importing OpenSSL 3.5 which should bring in the necessary asm changes as well (I think it's building on amd64 currently and being tested)? At that point merging this will presumably be simpler?

[pkubaj]

@shawn_anastas.io
FYI, OpenSSL 3.5 has been merged, so you should probably rebase this review. I hope it can finally be pushed before stable/15.

[tpearson_raptorengineering.com]

Rebase for 15.0

[tpearson_raptorengineering.com]

In D44274#1185321, @pkubaj wrote:

@shawn_anastas.io
FYI, OpenSSL 3.5 has been merged, so you should probably rebase this review. I hope it can finally be pushed before stable/15.

New version uploaded and tested. Note cryptocheck itself needs D52399 and D52400 in order to function, but this series does pass the cryptocheck checks once applied.

[markj]

I tested amd64, both the AVX512 and AES-NI variants. I think this is ok: please fix the nits and email me a git-format patch and I'll apply it.

sys/crypto/openssl/ossl_aes_gcm.c

59

Why include the parameter lists here? They're omitted in the amd64/i386 case above.

107

There should be no space after the first cast.

556

The indentation here should be left unmodified.

[tpearson_raptorengineering.com]

sys/crypto/openssl/ossl_aes_gcm.c

59

Looks like they moved to crypto/openssl/include/crypto/aes_platform.h. Will update.

[tpearson_raptorengineering.com]

sys/crypto/openssl/ossl_aes_gcm.c

59

I take that back, got the functions mixed up.

The functions aren't parameter compatible as-is; there is precedent in aes_platform.h for the parameter shuffling so it seems to have been acceptable at some point. Can we just take this as-is?

[markj]

sys/crypto/openssl/ossl_aes_gcm.c

59

Ah, I missed that. Ok then.