Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.7\))
Subject: Re: [PATCH 07/12] RISC-V: crypto: add Zvkg accelerated GCM GHASH
 implementation
From:   Jerry Shih <jerry.shih@sifive.com>
In-Reply-To: <20231122014216.GI2172@sol.localdomain>
Date:   Mon, 27 Nov 2023 10:49:05 +0800
Cc:     Paul Walmsley <paul.walmsley@sifive.com>, palmer@dabbelt.com,
        Albert Ou <aou@eecs.berkeley.edu>, herbert@gondor.apana.org.au,
        davem@davemloft.net, andy.chiu@sifive.com, greentime.hu@sifive.com,
        conor.dooley@microchip.com, guoren@kernel.org, bjorn@rivosinc.com,
        heiko@sntech.de, ardb@kernel.org, phoebe.chen@sifive.com,
        hongrong.hsu@sifive.com, linux-riscv@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <D1C10635-7799-4400-B85E-746D156D714A@sifive.com>
References: <20231025183644.8735-1-jerry.shih@sifive.com>
 <20231025183644.8735-8-jerry.shih@sifive.com>
 <20231122014216.GI2172@sol.localdomain>
To:     Eric Biggers <ebiggers@kernel.org>
Precedence: bulk

On Nov 22, 2023, at 09:42, Eric Biggers <ebiggers@kernel.org> 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;
>> +};
>=20
> 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();
>=20
> The indirection to ghash_func is unnecessary, since the only value is
> gcm_ghash_rv64i_zvkg.
>=20
> 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 >=3D GHASH_BLOCK_SIZE) {
>> +			crypto_xor((u8 *)&dctx->shash, src, =
GHASH_BLOCK_SIZE);
>> +			gf128mul_lle(&dctx->shash, &ctx->key);
>> +			srclen -=3D GHASH_BLOCK_SIZE;
>> +			src +=3D GHASH_BLOCK_SIZE;
>> +		}
>> +	}
>=20
> The assembly code uses the equivalent of the following do-while loop =
instead:
>=20
>        do {
>                srclen -=3D GHASH_BLOCK_SIZE;
>        } while (srclen);
>=20
> I.e., it assumes the length here is nonzero and a multiple of 16, =
which it is.
>=20
> To avoid confusion, I recommend making the C code use the same =
do-while loop.

Fixed.

>>       const struct riscv64_ghash_context *ctx =3D
>>              crypto_tfm_ctx(crypto_shash_tfm(desc->tfm));
>=20
> 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 =3D
>> +		crypto_tfm_ctx(crypto_shash_tfm(desc->tfm));
>> +	struct riscv64_ghash_desc_ctx *dctx =3D shash_desc_ctx(desc);
>> +	int i;
>> +
>> +	if (dctx->bytes) {
>> +		for (i =3D dctx->bytes; i < GHASH_BLOCK_SIZE; i++)
>> +			dctx->buffer[i] =3D 0;
>> +
>> +		ghash_blocks(ctx, dctx, dctx->buffer, GHASH_BLOCK_SIZE, =
func);
>> +		dctx->bytes =3D 0;
>> +	}
>> +
>=20
> Setting dctx->bytes above is unnecessary.

Fixed.

>> +static int ghash_init(struct shash_desc *desc)
>> +{
>> +	struct riscv64_ghash_desc_ctx *dctx =3D shash_desc_ctx(desc);
>> +
>> +	*dctx =3D (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 =3D
>> +		crypto_tfm_ctx(crypto_shash_tfm(tfm));
>> +
>> +	if (keylen !=3D GHASH_BLOCK_SIZE)
>> +		return -EINVAL;
>> +
>> +	memcpy(&ctx->key, key, GHASH_BLOCK_SIZE);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct shash_alg riscv64_ghash_alg_zvkg =3D {
>> +	.digestsize =3D GHASH_DIGEST_SIZE,
>> +	.init =3D ghash_init,
>> +	.update =3D ghash_update_zvkg,
>> +	.final =3D ghash_final_zvkg,
>> +	.setkey =3D ghash_setkey,
>=20
> IMO it's helpful to order the shash functions as follows, both in =
their
> definitions and their fields in struct shash_alg:
>=20
>    setkey
>    init
>    update
>    final
>=20
> 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=