Page MenuHomeFreeBSD
Differential D44274

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

Authored by tpearson_raptorengineering.com on Mar 7 2024, 9:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 9, 12:10 AM
Unknown Object (File)
Tue, Sep 8, 11:40 PM
Unknown Object (File)
Tue, Sep 8, 9:23 PM
Unknown Object (File)
Tue, Sep 8, 7:32 AM
Unknown Object (File)
Tue, Sep 8, 4:51 AM
Unknown Object (File)
Mon, Sep 7, 9:15 PM
Unknown Object (File)
Mon, Aug 24, 8:03 PM
Unknown Object (File)
Sun, Aug 23, 1:28 PM
View All Files
Subscribers
imp
jhb
jhibbits
lwhsu
markj
pkubaj
shawn_anastas.io
View All 9 Subscribers

Details

Reviewers
jhb
jhibbits
markj
andrew
sanastasio_raptorengineering.com
Group Reviewers
PowerPC
Commits
rGead69828206e: ossl: Add GCM support on powerpc64/powerpc64le (POWER8+)
rG5daf8ed625af: ossl: Add GCM support on powerpc64/powerpc64le (POWER8+)
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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

sanastasio_raptorengineering.com requested review of this revision.Mar 7 2024, 9:02 PM
sanastasio_raptorengineering.com created this revision.
sanastasio_raptorengineering.com updated this revision to Diff 135611.Mar 11 2024, 5:32 PM
  • 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 added a comment.Apr 4 2024, 9:54 PM

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 added a comment.Apr 9 2024, 5:51 PM
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 added a subscriber: markj.Apr 12 2024, 7:50 PM

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

markj added a comment.Apr 15 2024, 1:41 PM

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.

119

Please fix the style here.

sys/modules/ossl/Makefile
29

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 marked 3 inline comments as done.Jun 4 2024, 1:46 PM
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
29

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 updated this revision to Diff 139440.Jun 4 2024, 1:47 PM
  • Update style as suggested
sanastasio_raptorengineering.com added a comment.Aug 27 2024, 6:51 PM

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

lwhsu added reviewers: markj, PowerPC.Sep 3 2024, 5:10 PM
jhibbits added inline comments.Sep 9 2024, 1:18 PM
sys/modules/ossl/Makefile
29

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 updated this revision to Diff 143658.Sep 24 2024, 2:58 AM
  • Rename ARM's ossl_aes_gcm.c to ossl_aes_gcm_neon.c and update references
Herald added a reviewer: andrew. · View Herald TranscriptSep 24 2024, 2:58 AM
sanastasio_raptorengineering.com marked an inline comment as done.Sep 24 2024, 2:58 AM
sanastasio_raptorengineering.com added inline comments.
sys/modules/ossl/Makefile
29

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

markj added a comment.Sep 24 2024, 8:51 AM

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 marked an inline comment as done.Jan 24 2025, 5:30 PM
sanastasio_raptorengineering.com added inline comments.
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 added a comment.Jun 2 2025, 6:42 PM

Is there anything blocking this now?

tpearson_raptorengineering.com added a subscriber: tpearson_raptorengineering.com.Jun 29 2025, 4:52 PM

@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 added inline comments.Jul 11 2025, 6:46 PM
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 added a comment.Jul 15 2025, 7:07 PM

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 added subscribers: shawn_anastas.io, pkubaj.Aug 12 2025, 8:11 AM

@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 updated this revision to Diff 161591.Sep 5 2025, 4:00 PM
tpearson_raptorengineering.com commandeered this revision from sanastasio_raptorengineering.com.
tpearson_raptorengineering.com added a reviewer: sanastasio_raptorengineering.com.

Rebase for 15.0

tpearson_raptorengineering.com added a comment.Sep 5 2025, 6:16 PM
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 accepted this revision.Sep 9 2025, 11:33 PM

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.

This revision is now accepted and ready to land.Sep 9 2025, 11:33 PM
tpearson_raptorengineering.com marked 3 inline comments as done.Sep 10 2025, 4:39 PM
tpearson_raptorengineering.com added inline comments.
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 marked an inline comment as done.Sep 10 2025, 4:51 PM
tpearson_raptorengineering.com added inline comments.
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 added inline comments.Sep 11 2025, 2:47 PM
sys/crypto/openssl/ossl_aes_gcm.c
59

Ah, I missed that. Ok then.

Closed by commit rG5daf8ed625af: ossl: Add GCM support on powerpc64/powerpc64le (POWER8+) (authored by tpearson_raptorengineering.com, committed by markj). · Explain WhySep 11 2025, 4:08 PM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rG5daf8ed625af: ossl: Add GCM support on powerpc64/powerpc64le (POWER8+).
markj added a commit: rGead69828206e: ossl: Add GCM support on powerpc64/powerpc64le (POWER8+).Oct 15 2025, 1:27 PM

Revision Contents
Changeset List

PathSize
sys/
conf/
3 lines
2 lines
3 lines
crypto/
openssl/
amd64/
232 lines
arm/
ossl_aes_gcm_neon.c
ossl_aes_gcm.c
amd64/
399 lines
9 lines
6 lines
modules/
ossl/
7 lines

Diff 161591

View Options

sys/conf/files.amd64

Loading...
View Options

sys/conf/files.arm

Loading...
View Options

sys/conf/files.powerpc

Loading...
View Options

sys/crypto/openssl/amd64/ossl_aes_gcm.c

Loading...
View Options

sys/crypto/openssl/amd64/ossl_aes_gcm_avx512.c

Loading...
View Options

sys/crypto/openssl/arm/ossl_aes_gcm.c

Loading...
View Options

sys/crypto/openssl/arm/ossl_aes_gcm_neon.c

Loading...
View Options

sys/crypto/openssl/ossl_aes_gcm.c

Loading...
View Options

sys/crypto/openssl/ossl_ppc.c

Loading...
View Options

sys/crypto/openssl/ossl_x86.c

Loading...
View Options

sys/modules/ossl/Makefile

Loading...