by Eric Biggers

[permalink] [raw]

Subject: Re: [PATCH v5 4/8] crypto: x86/aesni-xctr: Add accelerated implementation of XCTR

On Wed, Apr 27, 2022 at 12:37:55AM +0000, Nathan Huckleberry wrote:
> Add hardware accelerated versions of XCTR for x86-64 CPUs with AESNI
> support. These implementations are modified versions of the CTR
> implementations found in aesni-intel_asm.S and aes_ctrby8_avx-x86_64.S.

Just one implementation now, using aes_ctrby8_avx-x86_64.S.

> +/* Note: the "x" prefix in these aliases means "this is an xmm register". The
> + * alias prefixes have no relation to XCTR where the "X" prefix means "XOR
> + * counter".
> + */

Block comments look like:

/*
* text
*/

> + .if !\xctr
> + vpshufb xbyteswap, xcounter, xdata0
> + .set i, 1
> + .rept (by - 1)
> + club XDATA, i
> + vpaddq (ddq_add_1 + 16 * (i - 1))(%rip), xcounter, var_xdata
> + vptest ddq_low_msk(%rip), var_xdata
> + jnz 1f
> + vpaddq ddq_high_add_1(%rip), var_xdata, var_xdata
> + vpaddq ddq_high_add_1(%rip), xcounter, xcounter
> + 1:
> + vpshufb xbyteswap, var_xdata, var_xdata
> + .set i, (i +1)
> + .endr
> + .else
> + movq counter, xtmp
> + .set i, 0
> + .rept (by)
> + club XDATA, i
> + vpaddq (ddq_add_1 + 16 * i)(%rip), xtmp, var_xdata
> + .set i, (i +1)
> + .endr
> + .set i, 0
> + .rept (by)
> + club XDATA, i
> + vpxor xiv, var_xdata, var_xdata
> + .set i, (i +1)
> + .endr
> + .endif

I'm not a fan of 'if !condition ... else ...', as the else clause is
double-negated. It's more straightforward to do 'if condition ... else ...'.

> + .if !\xctr
> + vmovdqa byteswap_const(%rip), xbyteswap
> + vmovdqu (p_iv), xcounter
> + vpshufb xbyteswap, xcounter, xcounter
> + .else
> + andq $(~0xf), num_bytes
> + shr $4, counter
> + vmovdqu (p_iv), xiv
> + .endif

Isn't the 'andq $(~0xf), num_bytes' instruction unnecessary? If it is
necessary, I'd expect it to be necessary for CTR too.

Otherwise this file looks good.

Note, the macros in this file all expand to way too much code, especially due to
the separate cases for AES-128, AES-192, and AES-256, and for each one every
partial stride length 1..7. Of course, this is true for the existing CTR code
too, so I don't think you have to fix this... But maybe think about addressing
this later. Changing the handling of partial strides might be the easiest way
to save a lot of code without hurting any micro-benchmarks too much. Also maybe
some or all of the AES key sizes could be combined.

> +#ifdef CONFIG_X86_64
> +/*
> + * XCTR does not have a non-AVX implementation, so it must be enabled
> + * conditionally.
> + */
> +static struct skcipher_alg aesni_xctr = {
> + .base = {
> + .cra_name = "__xctr(aes)",
> + .cra_driver_name = "__xctr-aes-aesni",
> + .cra_priority = 400,
> + .cra_flags = CRYPTO_ALG_INTERNAL,
> + .cra_blocksize = 1,
> + .cra_ctxsize = CRYPTO_AES_CTX_SIZE,
> + .cra_module = THIS_MODULE,
> + },
> + .min_keysize = AES_MIN_KEY_SIZE,
> + .max_keysize = AES_MAX_KEY_SIZE,
> + .ivsize = AES_BLOCK_SIZE,
> + .chunksize = AES_BLOCK_SIZE,
> + .setkey = aesni_skcipher_setkey,
> + .encrypt = xctr_crypt,
> + .decrypt = xctr_crypt,
> +};
> +
> +static struct simd_skcipher_alg *aesni_simd_xctr;
> +#endif

Comment the #endif above:

#endif /* CONFIG_X86_64 */

> @@ -1180,8 +1274,19 @@ static int __init aesni_init(void)
> if (err)
> goto unregister_skciphers;
>
> +#ifdef CONFIG_X86_64
> + if (boot_cpu_has(X86_FEATURE_AVX))
> + err = simd_register_skciphers_compat(&aesni_xctr, 1,
> + &aesni_simd_xctr);
> + if (err)
> + goto unregister_aeads;
> +#endif
> +
> return 0;
>
> +unregister_aeads:
> + simd_unregister_aeads(aesni_aeads, ARRAY_SIZE(aesni_aeads),
> + aesni_simd_aeads);

This will cause a compiler warning in 32-bit builds because the
'unregister_aeads' label won't be used.

- Eric

2022-05-03 01:06:22

by Eric Biggers

[permalink] [raw]

Subject: Re: [PATCH v5 3/8] crypto: hctr2 - Add HCTR2 support

On Wed, Apr 27, 2022 at 12:37:54AM +0000, Nathan Huckleberry wrote:
> +static int hctr2_create_common(struct crypto_template *tmpl,
> + struct rtattr **tb,
> + const char *xctr_name,
> + const char *polyval_name)
> +{
> + u32 mask;
> + struct skcipher_instance *inst;
> + struct hctr2_instance_ctx *ictx;
> + struct skcipher_alg *xctr_alg;
> + struct crypto_alg *blockcipher_alg;
> + struct shash_alg *polyval_alg;
> + char blockcipher_name[CRYPTO_MAX_ALG_NAME];
> + int len;
> + int err;
> +
> + err = crypto_check_attr_type(tb, CRYPTO_ALG_TYPE_SKCIPHER, &mask);
> + if (err)
> + return err;
> +
> + inst = kzalloc(sizeof(*inst) + sizeof(*ictx), GFP_KERNEL);
> + if (!inst)
> + return -ENOMEM;
> + ictx = skcipher_instance_ctx(inst);
> +
> + /* Stream cipher, xctr(block_cipher) */
> + err = crypto_grab_skcipher(&ictx->xctr_spawn,
> + skcipher_crypto_instance(inst),
> + xctr_name, 0, mask);
> + if (err)
> + goto err_free_inst;
> + xctr_alg = crypto_spawn_skcipher_alg(&ictx->xctr_spawn);
> +
> + err = -EINVAL;
> + if (strncmp(xctr_alg->base.cra_name, "xctr(", 5))
> + goto err_free_inst;
> + len = strscpy(blockcipher_name, xctr_name + 5,
> + sizeof(blockcipher_name));

Found a bug here; 'xctr_name' in the strscpy() statement needs to be
xctr_alg->base.cra_name. Otherwise something like
'hctr2_base(xctr-aes-aesni,polyval-clmulni)' doesn't work, as it will try to
extract the block cipher name from the driver name "xctr-aes-aesni" instead of
from the algorithm name "xctr(aes)" as intended.

- Eric