Page MenuHomeFreeBSD

Add AES-GCM H/W acceleration for kTLS on ARMv8 architecture
AcceptedPublic

Authored by gonzo on Dec 3 2020, 2:48 AM.

Details

Summary

Add ktls_armv8 kernel modules that implements AES-GCM encryption
usign ARMv8-accelerated crypto primitives from OpenSSL.

The module supports:

  • AES128-GCM encryption
  • AES256-GCM encryption
  • TLS v1.2
  • TLS v1.3
Test Plan

Tested on Ampere Computing Altra platform

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 36193
Build 33082: arc lint + arc unit

Event Timeline

gonzo requested review of this revision.Dec 3 2020, 2:48 AM

This is an almost ready version to get the initial discussion going. I'd like to get it in shape for HEAD before 13.0 freeze. There are still some issues.

The requirement of AQUIRE_CTX/RELEASE_CTX is not exactly clear to me, the driver seems to work fine without locking on an 80-CPU platform under heavy load. I'd appreciate some help on this topic. What is the failure mode for a race like this?

Also, the driver wasn't tested on the big-endian platform yet.

You would be better off adding AES-GCM support to an OCF driver for arm64 instead. I'm currently looking at retiring the software interface for KTLS and instead only using OCF for software KTLS. In addition, AES-GCM support in OCF would also benefit other use cases like IPsec and ZFS.

That said, my thoughts for arm64 AES-GCM support was to extend ossl(4) to support AES-GCM and use that. However, you could take your AES-GCM implementation and add it to armv8crypto(4).

In D27454#613768, @jhb wrote:

You would be better off adding AES-GCM support to an OCF driver for arm64 instead. I'm currently looking at retiring the software interface for KTLS and instead only using OCF for software KTLS. In addition, AES-GCM support in OCF would also benefit other use cases like IPsec and ZFS.

That said, my thoughts for arm64 AES-GCM support was to extend ossl(4) to support AES-GCM and use that. However, you could take your AES-GCM implementation and add it to armv8crypto(4).

OK, I'll switch it to OCF and armv8crypto. I wrote it as a kTLS interface to avoid extra indirection levels (kTLS/GCM instead of kTLS/OCF/GCM), do you think switching to OCF can have any performance penalties?

In D27454#613768, @jhb wrote:

You would be better off adding AES-GCM support to an OCF driver for arm64 instead. I'm currently looking at retiring the software interface for KTLS and instead only using OCF for software KTLS. In addition, AES-GCM support in OCF would also benefit other use cases like IPsec and ZFS.

That said, my thoughts for arm64 AES-GCM support was to extend ossl(4) to support AES-GCM and use that. However, you could take your AES-GCM implementation and add it to armv8crypto(4).

OK, I'll switch it to OCF and armv8crypto. I wrote it as a kTLS interface to avoid extra indirection levels (kTLS/GCM instead of kTLS/OCF/GCM), do you think switching to OCF can have any performance penalties?

No, I think the performance should be about the same, and if anything I'm trying to minimize OCF overhead as much as possible in my current set of changes,

Move AES-GCM support to armv8crypto acceleration module

Hi @gonzo,

I'm looking at D21017, which adds AES-XTS support to armv8_crypto, and has been sitting in review for some time.

I am hoping to get it committed soon, but wanted to get your attention here as it will create some small conflicts with this patch. Let me know if you have any major concerns about that, I'm happy to accommodate so that we might get both of these patches in before 13 branches.

Hi @gonzo,

I'm looking at D21017, which adds AES-XTS support to armv8_crypto, and has been sitting in review for some time.

I am hoping to get it committed soon, but wanted to get your attention here as it will create some small conflicts with this patch. Let me know if you have any major concerns about that, I'm happy to accommodate so that we might get both of these patches in before 13 branches.

Hi @mhorne

No concerns, please go ahead with your patch. I'll resolve conflicts later.

sys/crypto/armv8/armv8_crypto.c
212

This should also check csp_flags and only accept the flags it supports (for KTLS you will want CSP_F_SEPARATE_AAD and CSP_F_SEPARATE_OUTPUT IIRC). This makes it future-proof against new flags being added in the future.

235

It seems like you support separate output buffers with AES-CBC in your changes, so you should perhaps just keep the csp_flags check in one place and whitelist the flags supported there the way aesni(4) does?

317

Should some of this logic be conditional on the session? E.g. only initializing H for GCM sessions, and only initializing a decrypt key for AES-CBC (and XTS). AES-CTR (which GCM and CCM both use for their cipher side) does not use a separate decryption key schedule but instead uses the encryption key to generate a keystream xored with the cipher/plain text in both directions.

326

Perhaps call this error as that is more common in the kernel?

450

I think you can instead remove the err == 0 check before the copy for the output buffer and set 'err = 0' just before the out label since all the places that set err jump to out. I would probably also spell it error.

474

It's really nicer to reject this in the probesession callback instead. Then you are only checking once per session (and can fall back to another driver) rather than checking once per-op (and failing every request sent if the check fails).

479

Same, though I think you already check this in probesession, so there's no need to re-check it here.

sys/modules/armv8crypto/arm_arch.h
1 ↗(On Diff #80684)

This file is already present in sys/crypto/openssl/aarch64/arm_arch.h for use by ossl(4). You should reuse that header rather than adding a new copy, plus it shouldn't live in sys/modules anyway to support compiling the driver into a kernel. Note that you will want to update sys/conf/files.arm64 as well as I believe we plan to add device armv8crypto to GENERIC shortly.

gonzo marked 7 inline comments as done.

Sync to the latest HEAD:

  • Update with the allanjude's patch rebased on the latest main
  • Address jhb's comments
markj added inline comments.
sys/modules/armv8crypto/Makefile
34

Doesn't sys/conf/files.arm64 need to be updated as well? With this patch a kernel compiled with "device armv8crypto" will fail to link.

Remove unused include dir and fix armv8crypto build when included in a kernel config

jhb added inline comments.
sys/crypto/armv8/armv8_crypto.c
116

This can just use 'device_set_desc'. It's not a dynamic string, so a copy isn't required. That is an old bug, but not worth perpetuating further.

This revision is now accepted and ready to land.Wed, Jan 20, 4:31 PM