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.
> +
> +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().
> + } 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.
> 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)
> +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.
> +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.
- Eric
On Nov 22, 2023, at 09:42, Eric Biggers <[email protected]> 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