mbox series

[00/17] crypto/cipher: Class hierarchy cleanups

Message ID 20200813032537.2888593-1-richard.henderson@linaro.org
Headers show
Series crypto/cipher: Class hierarchy cleanups | expand

Message

Richard Henderson Aug. 13, 2020, 3:25 a.m. UTC 
Mostly this is intended to cleanup the class hierarchy
used for the ciphers.  We currently have multiple levels
of dispatch, and multiple separate allocations.  The final
patches rearrange this to one level of indirect call, and
all memory allocated contiguously.

But on the way there are a number of other misc cleanups.

I know those final patches are somewhat big, but I don't
immediately see how to split them apart.

I noticed this while profiling patches to make ARM PAUTH
use the crypto subsystem.  The qcrypto_cipher_* dispatch
routines were consuming a noticeable portion of the runtime,
and with these changes they were down below 1% where they
ought to be.

While I did not continue with PAUTH using AES, I still think
these are good cleanups.


r~


Richard Henderson (17):
  crypto: Move QCryptoCipher typedef to qemu/typedefs.h
  crypto: Move QCryptoCipherDriver typedef to qemu/typedefs.h
  crypto: Assume blocksize is a power of 2
  crypto: Rename cipher include files to .inc.c
  crypto: Remove redundant includes
  crypto/nettle: Fix xts_encrypt arguments
  crypto: Use the correct const type for driver
  crypto: Allocate QCryptoCipher with the subclass
  crypto: Move cipher->driver init to qcrypto_*_cipher_ctx_new
  crypto: Constify cipher data tables
  crypto/builtin: Remove odd-sized AES block handling
  crypto/builtin: Merge qcrypto_cipher_aes_{ecb,xts}_{en,de}crypt
  crypto/builtin: Move AES_cbc_encrypt into cipher-builtin.inc.c
  crypto/builtin: Split and simplify AES_encrypt_cbc
  crypto/builtin: Split QCryptoCipherBuiltin into subclasses
  crypto/nettle: Split QCryptoCipherNettle into subclasses
  crypto/gcrypt: Split QCryptoCipherGcrypt into subclasses

 crypto/afalgpriv.h                            |   3 +
 crypto/cipherpriv.h                           |   6 +-
 include/crypto/aes.h                          |   4 -
 include/crypto/cipher.h                       |   5 +-
 include/qemu/typedefs.h                       |   2 +
 crypto/aes.c                                  |  51 --
 crypto/cipher-afalg.c                         |  25 +-
 crypto/cipher-builtin.c                       | 532 ------------
 crypto/cipher-builtin.inc.c                   | 425 ++++++++++
 .../{cipher-gcrypt.c => cipher-gcrypt.inc.c}  | 522 ++++++------
 crypto/cipher-nettle.c                        | 733 -----------------
 crypto/cipher-nettle.inc.c                    | 756 ++++++++++++++++++
 crypto/cipher.c                               |  44 +-
 13 files changed, 1477 insertions(+), 1631 deletions(-)
 delete mode 100644 crypto/cipher-builtin.c
 create mode 100644 crypto/cipher-builtin.inc.c
 rename crypto/{cipher-gcrypt.c => cipher-gcrypt.inc.c} (51%)
 delete mode 100644 crypto/cipher-nettle.c
 create mode 100644 crypto/cipher-nettle.inc.c

Comments

Philippe Mathieu-Daudé Aug. 13, 2020, 8:54 a.m. UTC | #1 
On 8/13/20 5:25 AM, Richard Henderson wrote:
> Mostly this is intended to cleanup the class hierarchy
> used for the ciphers.  We currently have multiple levels
> of dispatch, and multiple separate allocations.  The final
> patches rearrange this to one level of indirect call, and
> all memory allocated contiguously.
> 
> But on the way there are a number of other misc cleanups.
> 
> I know those final patches are somewhat big, but I don't
> immediately see how to split them apart.
> 
> I noticed this while profiling patches to make ARM PAUTH
> use the crypto subsystem.  The qcrypto_cipher_* dispatch
> routines were consuming a noticeable portion of the runtime,
> and with these changes they were down below 1% where they
> ought to be.
> 
> While I did not continue with PAUTH using AES, I still think
> these are good cleanups.
> 
> 
> r~
> 
> 
> Richard Henderson (17):
>   crypto: Move QCryptoCipher typedef to qemu/typedefs.h
>   crypto: Move QCryptoCipherDriver typedef to qemu/typedefs.h
>   crypto: Assume blocksize is a power of 2
>   crypto: Rename cipher include files to .inc.c
>   crypto: Remove redundant includes
>   crypto/nettle: Fix xts_encrypt arguments
>   crypto: Use the correct const type for driver
>   crypto: Allocate QCryptoCipher with the subclass
>   crypto: Move cipher->driver init to qcrypto_*_cipher_ctx_new
>   crypto: Constify cipher data tables
>   crypto/builtin: Remove odd-sized AES block handling
>   crypto/builtin: Merge qcrypto_cipher_aes_{ecb,xts}_{en,de}crypt
>   crypto/builtin: Move AES_cbc_encrypt into cipher-builtin.inc.c
>   crypto/builtin: Split and simplify AES_encrypt_cbc
>   crypto/builtin: Split QCryptoCipherBuiltin into subclasses
>   crypto/nettle: Split QCryptoCipherNettle into subclasses
>   crypto/gcrypt: Split QCryptoCipherGcrypt into subclasses
> 
>  crypto/afalgpriv.h                            |   3 +
>  crypto/cipherpriv.h                           |   6 +-
>  include/crypto/aes.h                          |   4 -
>  include/crypto/cipher.h                       |   5 +-
>  include/qemu/typedefs.h                       |   2 +
>  crypto/aes.c                                  |  51 --
>  crypto/cipher-afalg.c                         |  25 +-
>  crypto/cipher-builtin.c                       | 532 ------------
>  crypto/cipher-builtin.inc.c                   | 425 ++++++++++
>  .../{cipher-gcrypt.c => cipher-gcrypt.inc.c}  | 522 ++++++------
>  crypto/cipher-nettle.c                        | 733 -----------------
>  crypto/cipher-nettle.inc.c                    | 756 ++++++++++++++++++
>  crypto/cipher.c                               |  44 +-
>  13 files changed, 1477 insertions(+), 1631 deletions(-)
>  delete mode 100644 crypto/cipher-builtin.c
>  create mode 100644 crypto/cipher-builtin.inc.c
>  rename crypto/{cipher-gcrypt.c => cipher-gcrypt.inc.c} (51%)
>  delete mode 100644 crypto/cipher-nettle.c
>  create mode 100644 crypto/cipher-nettle.inc.c
> 

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
no-reply@patchew.org Aug. 13, 2020, 10:48 a.m. UTC | #2 
Patchew URL: https://patchew.org/QEMU/20200813032537.2888593-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200813032537.2888593-1-richard.henderson@linaro.org
Subject: [PATCH 00/17] crypto/cipher: Class hierarchy cleanups

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200813032537.2888593-1-richard.henderson@linaro.org -> patchew/20200813032537.2888593-1-richard.henderson@linaro.org
Switched to a new branch 'test'
a39d494 crypto/gcrypt: Split QCryptoCipherGcrypt into subclasses
ca787d9 crypto/nettle: Split QCryptoCipherNettle into subclasses
81bf19b crypto/builtin: Split QCryptoCipherBuiltin into subclasses
11aed74 crypto/builtin: Split and simplify AES_encrypt_cbc
c628150 crypto/builtin: Move AES_cbc_encrypt into cipher-builtin.inc.c
3707370 crypto/builtin: Merge qcrypto_cipher_aes_{ecb, xts}_{en, de}crypt
efe1005 crypto/builtin: Remove odd-sized AES block handling
628aa8a crypto: Constify cipher data tables
01f1215 crypto: Move cipher->driver init to qcrypto_*_cipher_ctx_new
93dc38a crypto: Allocate QCryptoCipher with the subclass
81aaa54 crypto: Use the correct const type for driver
8c43775 crypto/nettle: Fix xts_encrypt arguments
f7c0f04 crypto: Remove redundant includes
490fd1b crypto: Rename cipher include files to .inc.c
4ba136f crypto: Assume blocksize is a power of 2
bf305e2 crypto: Move QCryptoCipherDriver typedef to qemu/typedefs.h
680954e crypto: Move QCryptoCipher typedef to qemu/typedefs.h

=== OUTPUT BEGIN ===
1/17 Checking commit 680954e19b44 (crypto: Move QCryptoCipher typedef to qemu/typedefs.h)
2/17 Checking commit bf305e28fb65 (crypto: Move QCryptoCipherDriver typedef to qemu/typedefs.h)
3/17 Checking commit 4ba136fd50f5 (crypto: Assume blocksize is a power of 2)
4/17 Checking commit 490fd1bf6368 (crypto: Rename cipher include files to .inc.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16: 
rename from crypto/cipher-builtin.c

total: 0 errors, 1 warnings, 14 lines checked

Patch 4/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/17 Checking commit f7c0f04877bd (crypto: Remove redundant includes)
6/17 Checking commit 8c4377523504 (crypto/nettle: Fix xts_encrypt arguments)
7/17 Checking commit 81aaa54855c5 (crypto: Use the correct const type for driver)
8/17 Checking commit 93dc38a2a468 (crypto: Allocate QCryptoCipher with the subclass)
9/17 Checking commit 01f121573c8b (crypto: Move cipher->driver init to qcrypto_*_cipher_ctx_new)
10/17 Checking commit 628aa8ac5fbc (crypto: Constify cipher data tables)
11/17 Checking commit efe100595b61 (crypto/builtin: Remove odd-sized AES block handling)
12/17 Checking commit 3707370f4f52 (crypto/builtin: Merge qcrypto_cipher_aes_{ecb, xts}_{en, de}crypt)
13/17 Checking commit c6281504f1df (crypto/builtin: Move AES_cbc_encrypt into cipher-builtin.inc.c)
14/17 Checking commit 11aed74bf453 (crypto/builtin: Split and simplify AES_encrypt_cbc)
15/17 Checking commit 81bf19bde54b (crypto/builtin: Split QCryptoCipherBuiltin into subclasses)
16/17 Checking commit ca787d9c34d4 (crypto/nettle: Split QCryptoCipherNettle into subclasses)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#257: FILE: crypto/cipher-nettle.inc.c:255:
+#define DEFINE_XTS(NAME, TYPE, BLEN, ENCRYPT, DECRYPT)          \
+    QEMU_BUILD_BUG_ON(BLEN != XTS_BLOCK_SIZE);                  \
+    DEFINE__XTS(NAME, TYPE, BLEN, ENCRYPT, DECRYPT)             \
+static const struct QCryptoCipherDriver NAME##_driver_xts = {   \
+    .cipher_encrypt = NAME##_encrypt_xts,                       \
+    .cipher_decrypt = NAME##_decrypt_xts,                       \
+    .cipher_setiv = NAME##_setiv,                               \
+    .cipher_free = qcrypto_cipher_ctx_free,                     \
+};

total: 1 errors, 0 warnings, 973 lines checked

Patch 16/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

17/17 Checking commit a39d494a93ce (crypto/gcrypt: Split QCryptoCipherGcrypt into subclasses)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200813032537.2888593-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
no-reply@patchew.org Aug. 13, 2020, 11:11 a.m. UTC | #3 
Patchew URL: https://patchew.org/QEMU/20200813032537.2888593-1-richard.henderson@linaro.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      crypto/pbkdf-nettle.o
In file included from /tmp/qemu-test/src/crypto/cipher.c:156:0:
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes128_xts_wrape':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes128_encrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void aes128_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes128_xts_wrapd':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes128_decrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void aes128_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes192_xts_wrape':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes192_encrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void aes192_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes192_xts_wrapd':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes192_decrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void aes192_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes256_xts_wrape':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes256_encrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void aes256_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes256_xts_wrapd':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes256_decrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void aes256_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_serpent_xts_wrape':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'serpent_encrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void serpent_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_serpent_xts_wrapd':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'serpent_decrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void serpent_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_twofish_xts_wrape':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'twofish_encrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void twofish_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_twofish_xts_wrapd':
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'twofish_decrypt_native' discards 'const' qualifier from pointer target type [-Werror]
 static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
                     ^
/tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'
---
 static void twofish_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
             ^
cc1: all warnings being treated as errors
make: *** [crypto/cipher.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 709, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=73d17a80a3a34b9caef4371277896653', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-k265h_76/src/docker-src.2020-08-13-07.09.18.27263:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=73d17a80a3a34b9caef4371277896653
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-k265h_76/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m20.871s
user    0m8.035s


The full log is available at
http://patchew.org/logs/20200813032537.2888593-1-richard.henderson@linaro.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Daniel P. Berrangé Aug. 17, 2020, 4:45 p.m. UTC | #4 
On Thu, Aug 13, 2020 at 04:11:40AM -0700, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200813032537.2888593-1-richard.henderson@linaro.org/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   CC      crypto/pbkdf-nettle.o
> In file included from /tmp/qemu-test/src/crypto/cipher.c:156:0:
> /tmp/qemu-test/src/crypto/cipher-nettle.inc.c: In function 'qcrypto_nettle_aes128_xts_wrape':
> /tmp/qemu-test/src/crypto/cipher-nettle.inc.c:185:21: error: passing argument 1 of 'aes128_encrypt_native' discards 'const' qualifier from pointer target type [-Werror]
>  static const struct QCryptoCipherDriver NAME##_driver_ctr = {           \
>                      ^
> /tmp/qemu-test/src/crypto/cipher-nettle.inc.c:270:5: note: in expansion of macro 'DEFINE_CTR'

Older versions of nettle had a different API declaration for various
functions. This failure suggests the code changes in this series only
work for the modern nettle.


Regards,
Daniel
Daniel P. Berrangé Aug. 17, 2020, 5:09 p.m. UTC | #5 
On Wed, Aug 12, 2020 at 08:25:20PM -0700, Richard Henderson wrote:
> Mostly this is intended to cleanup the class hierarchy
> used for the ciphers.  We currently have multiple levels
> of dispatch, and multiple separate allocations.  The final
> patches rearrange this to one level of indirect call, and
> all memory allocated contiguously.
> 
> But on the way there are a number of other misc cleanups.
> 
> I know those final patches are somewhat big, but I don't
> immediately see how to split them apart.

Yeah, I can't see a better way off hand.

> I noticed this while profiling patches to make ARM PAUTH
> use the crypto subsystem.  The qcrypto_cipher_* dispatch
> routines were consuming a noticeable portion of the runtime,
> and with these changes they were down below 1% where they
> ought to be.
> 
> While I did not continue with PAUTH using AES, I still think
> these are good cleanups.

They'll probably improve the LUKS block driver performance too.

What were you measuring performance with ?  Did you use the
benchmark-crypto-cipher  program or something else ?

Regards,
Daniel
Richard Henderson Aug. 17, 2020, 8:49 p.m. UTC | #6 
On 8/17/20 10:09 AM, Daniel P. Berrangé wrote:
> What were you measuring performance with ?  Did you use the
> benchmark-crypto-cipher  program or something else ?

Perf of a boot of an aarch64 kernel, which

  * debug enabled for regression testing,
  * the v8.3-pauth instructions enabled,
  * a local qemu patch to use aes128 for pauth.

I can dig up pointers if you want, but fairly niche.

Because of all the little debug-enabled functions, it meant we were doing a 1
block aes128 encrypt approximately every 40 guest instructions.


r~