On Nov 2, 2023, at 12:51, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> On Thu, Oct 26, 2023 at 02:36:36AM +0800, Jerry Shih wrote:
>> diff --git a/arch/riscv/crypto/Kconfig b/arch/riscv/crypto/Kconfig
>> index 10d60edc0110..500938317e71 100644
>> --- a/arch/riscv/crypto/Kconfig
>> +++ b/arch/riscv/crypto/Kconfig
>> @@ -2,4 +2,16 @@
>>
>> menu "Accelerated Cryptographic Algorithms for CPU (riscv)"
>>
>> +config CRYPTO_AES_RISCV64
>> + default y if RISCV_ISA_V
>> + tristate "Ciphers: AES"
>> + depends on 64BIT && RISCV_ISA_V
>> + select CRYPTO_AES
>> + select CRYPTO_ALGAPI
>> + help
>> + Block ciphers: AES cipher algorithms (FIPS-197)
>> +
>> + Architecture: riscv64 using:
>> + - Zvkned vector crypto extension
>
> kconfig options should default to off.
>
> I.e., remove the line "default y if RISCV_ISA_V"
Fixed.
>> + *
>> + * All zvkned-based functions use encryption expending keys for both encryption
>> + * and decryption.
>> + */
>
> The above comment is a bit confusing. It's describing the 'key' field of struct
> aes_key; maybe there should be a comment there instead:
>
> struct aes_key {
> u32 key[AES_MAX_KEYLENGTH_U32]; /* round keys in encryption order */
> u32 rounds;
> };
I have updated the asm implementation to use `crypto_aes_ctx` struct.
>> +int riscv64_aes_setkey(struct riscv64_aes_ctx *ctx, const u8 *key,
>> + unsigned int keylen)
>> +{
>> + /*
>> + * The RISC-V AES vector crypto key expending doesn't support AES-192.
>> + * We just use the generic software key expending here to simplify the key
>> + * expending flow.
>> + */
>
> expending => expanding
Thx.
Fixed.
>> + u32 aes_rounds;
>> + u32 key_length;
>> + int ret;
>> +
>> + ret = aes_expandkey(&ctx->fallback_ctx, key, keylen);
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> + /*
>> + * Copy the key from `crypto_aes_ctx` to `aes_key` for zvkned-based AES
>> + * implementations.
>> + */
>> + aes_rounds = aes_round_num(keylen);
>> + ctx->key.rounds = aes_rounds;
>> + key_length = AES_BLOCK_SIZE * (aes_rounds + 1);
>> + memcpy(ctx->key.key, ctx->fallback_ctx.key_enc, key_length);
>> +
>> + return 0;
>> +}
>
> Ideally this would use the same crypto_aes_ctx for both the fallback and the
> assembly code. I suppose we don't want to diverge from the OpenSSL code (unless
> it gets rewritten), though. So I guess this is fine for now.
I have updated the asm implementation to use `crypto_aes_ctx` struct.
>> void riscv64_aes_encrypt_zvkned(const struct riscv64_aes_ctx *ctx, u8 *dst,
>> const u8 *src)
>
> These functions can be called from a different module (aes-block-riscv64), so
> they need EXPORT_SYMBOL_GPL.
Fixed.
>> +static inline bool check_aes_ext(void)
>> +{
>> + return riscv_isa_extension_available(NULL, ZVKNED) &&
>> + riscv_vector_vlen() >= 128;
>> +}
>> +
>> +static int __init riscv64_aes_mod_init(void)
>> +{
>> + if (check_aes_ext())
>> + return crypto_register_alg(&riscv64_aes_alg_zvkned);
>> +
>> + return -ENODEV;
>> +}
>> +
>> +static void __exit riscv64_aes_mod_fini(void)
>> +{
>> + if (check_aes_ext())
>> + crypto_unregister_alg(&riscv64_aes_alg_zvkned);
>> +}
>> +
>> +module_init(riscv64_aes_mod_init);
>> +module_exit(riscv64_aes_mod_fini);
>
> module_exit can only run if module_init succeeded. So, in cases like this it's
> not necessary to check for CPU features before unregistering the algorithm.
>
> - Eric
Fixed.
-Jerry
