| Message ID | 20200813032537.2888593-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | crypto/cipher: Class hierarchy cleanups | expand |
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>
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
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
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 -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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 -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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~
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 -- 2.25.1