Re: [PATCH 07/12] RISC-V: crypto: add Zvkg accelerated GCM GHASH implementation

[Jerry Shih]

On Nov 22, 2023, at 09:42, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> On Thu, Oct 26, 2023 at 02:36:39AM +0800, Jerry Shih wrote:
>> +struct riscv64_ghash_context {
>> +	be128 key;
>> +};
>> +
>> +struct riscv64_ghash_desc_ctx {
>> +	be128 shash;
>> +	u8 buffer[GHASH_BLOCK_SIZE];
>> +	u32 bytes;
>> +};
> 
> I recommend calling the first struct 'riscv64_ghash_tfm_ctx', and making the
> pointers to it be named 'tctx'.  That would more clearly distinguish it from the
> desc_ctx / dctx.

Fixed.

>> +
>> +typedef void (*ghash_func)(be128 *Xi, const be128 *H, const u8 *inp,
>> +			   size_t len);
>> +
>> +static inline void ghash_blocks(const struct riscv64_ghash_context *ctx,
>> +				struct riscv64_ghash_desc_ctx *dctx,
>> +				const u8 *src, size_t srclen, ghash_func func)
>> +	if (crypto_simd_usable()) {
>> +		kernel_vector_begin();
>> +		func(&dctx->shash, &ctx->key, src, srclen);
>> +		kernel_vector_end();
> 
> The indirection to ghash_func is unnecessary, since the only value is
> gcm_ghash_rv64i_zvkg.
> 
> This also means that ghash_update() should be folded into ghash_update_zvkg(),
> and ghash_final() into ghash_final_zvkg().

Fixed. The `gcm_ghash_rv64i_zvkg()` is folded into `ghash_update_zvkg()` and
`ghash_final_zvkg()`.

>> +	} else {
>> +		while (srclen >= GHASH_BLOCK_SIZE) {
>> +			crypto_xor((u8 *)&dctx->shash, src, GHASH_BLOCK_SIZE);
>> +			gf128mul_lle(&dctx->shash, &ctx->key);
>> +			srclen -= GHASH_BLOCK_SIZE;
>> +			src += GHASH_BLOCK_SIZE;
>> +		}
>> +	}
> 
> The assembly code uses the equivalent of the following do-while loop instead:
> 
>        do {
>                srclen -= GHASH_BLOCK_SIZE;
>        } while (srclen);
> 
> I.e., it assumes the length here is nonzero and a multiple of 16, which it is.
> 
> To avoid confusion, I recommend making the C code use the same do-while loop.

Fixed.

>>       const struct riscv64_ghash_context *ctx =
>>              crypto_tfm_ctx(crypto_shash_tfm(desc->tfm));
> 
> crypto_tfm_ctx(crypto_shash_tfm(tfm)) should be crypto_shash_ctx(tfm)

Fixed.
But the original code do the same thing.

>> +static int ghash_final(struct shash_desc *desc, u8 *out, ghash_func func)
>> +{
>> +	const struct riscv64_ghash_context *ctx =
>> +		crypto_tfm_ctx(crypto_shash_tfm(desc->tfm));
>> +	struct riscv64_ghash_desc_ctx *dctx = shash_desc_ctx(desc);
>> +	int i;
>> +
>> +	if (dctx->bytes) {
>> +		for (i = dctx->bytes; i < GHASH_BLOCK_SIZE; i++)
>> +			dctx->buffer[i] = 0;
>> +
>> +		ghash_blocks(ctx, dctx, dctx->buffer, GHASH_BLOCK_SIZE, func);
>> +		dctx->bytes = 0;
>> +	}
>> +
> 
> Setting dctx->bytes above is unnecessary.

Fixed.

>> +static int ghash_init(struct shash_desc *desc)
>> +{
>> +	struct riscv64_ghash_desc_ctx *dctx = shash_desc_ctx(desc);
>> +
>> +	*dctx = (struct riscv64_ghash_desc_ctx){};
>> +
>> +	return 0;
>> +}
>> +
>> +static int ghash_update_zvkg(struct shash_desc *desc, const u8 *src,
>> +			     unsigned int srclen)
>> +{
>> +	return ghash_update(desc, src, srclen, gcm_ghash_rv64i_zvkg);
>> +}
>> +
>> +static int ghash_final_zvkg(struct shash_desc *desc, u8 *out)
>> +{
>> +	return ghash_final(desc, out, gcm_ghash_rv64i_zvkg);
>> +}
>> +
>> +static int ghash_setkey(struct crypto_shash *tfm, const u8 *key,
>> +			unsigned int keylen)
>> +{
>> +	struct riscv64_ghash_context *ctx =
>> +		crypto_tfm_ctx(crypto_shash_tfm(tfm));
>> +
>> +	if (keylen != GHASH_BLOCK_SIZE)
>> +		return -EINVAL;
>> +
>> +	memcpy(&ctx->key, key, GHASH_BLOCK_SIZE);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct shash_alg riscv64_ghash_alg_zvkg = {
>> +	.digestsize = GHASH_DIGEST_SIZE,
>> +	.init = ghash_init,
>> +	.update = ghash_update_zvkg,
>> +	.final = ghash_final_zvkg,
>> +	.setkey = ghash_setkey,
> 
> IMO it's helpful to order the shash functions as follows, both in their
> definitions and their fields in struct shash_alg:
> 
>    setkey
>    init
>    update
>    final
> 
> That matches the order in which they're called.

I have different opinion. I reorder the initialization in the order declared.
That will help us to check whether the function/member is missed.

> - Eric


-Jerry