Re: [PATCH v4 5/8] crypto: arm64/aes-xctr: Add accelerated implementation of XCTR

From: Eric Biggers <ebiggers@kernel.org>
To: Nathan Huckleberry <nhuck@google.com>
Cc: linux-crypto@vger.kernel.org,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	linux-arm-kernel@lists.infradead.org,
	Paul Crowley <paulcrowley@google.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH v4 5/8] crypto: arm64/aes-xctr: Add accelerated implementation of XCTR
Date: Mon, 18 Apr 2022 21:33:50 -0700	[thread overview]
Message-ID: <Yl47rptSdSetygym@sol.localdomain> (raw)
In-Reply-To: <20220412172816.917723-6-nhuck@google.com>

On Tue, Apr 12, 2022 at 05:28:13PM +0000, Nathan Huckleberry wrote:
> diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
> index dc35eb0245c5..ac37e2f7ca84 100644
> --- a/arch/arm64/crypto/aes-modes.S
> +++ b/arch/arm64/crypto/aes-modes.S
> @@ -479,6 +479,140 @@ ST5(	mov		v3.16b, v4.16b			)
>  	b		.Lctrout
>  AES_FUNC_END(aes_ctr_encrypt)
>  
> +	/*
> +	 * aes_xctr_encrypt(u8 out[], u8 const in[], u8 const rk[], int rounds,
> +	 *		   int bytes, u8 const ctr[], u8 finalbuf[], int
> +	 *		   byte_ctr)
> +	 */
> +

What is the 'finalbuf' parameter for?  It is never used.

Why is byte_ctr an 'int' here but an 'unsigned int' in the .c file?

It looks like 'ctr' is actually the IV; perhaps it should be called 'iv' to
distinguish it from the byte_ctr?

As mentioned elsewhere, please don't have a line break between a parameter's
type and name.

Generally, comments and register aliases would be super helpful throughout the
code.  As-is, this is much harder to read than the x86 version...

Also, this function is heavily duplicated with aes_ctr_encrypt.  Did you
consider generating both from a single macro, like you did with the x86 version?

> +	umov		x12, vctr.d[0]		/* keep ctr in reg */

/* keep first 8 bytes of IV in reg */

> +	lsr		x7, x7, #4

x7 needs to be w7, since it corresponds to a 32-bit parameter ('int byte_ctr').
The upper 32 bits of the register are not guaranteed to be zero.

> +	sub		x7, x11, #MAX_STRIDE
> +	eor		x7, x12, x7
> +	ins		v0.d[0], x7
> +	sub		x7, x11, #MAX_STRIDE - 1
> +	sub		x8, x11, #MAX_STRIDE - 2
> +	eor		x7, x7, x12
> +	sub		x9, x11, #MAX_STRIDE - 3
> +	mov		v1.d[0], x7
> +	eor		x8, x8, x12
> +	eor		x9, x9, x12
> +ST5(	sub		x10, x11, #MAX_STRIDE - 4)
> +	mov		v2.d[0], x8
> +	eor		x10, x10, x12
> +	mov		v3.d[0], x9
> +ST5(	mov		v4.d[0], x10			)

There seem to be some unnecessarily tight instruction dependencies here.  E.g.,
the first 3 instructions are all sequential.  Are there not enough free
registers to write it otherwise?  I.e. do all the sub's first, then the eor's,
then the mov's.

The trailing parenthesis after #MAX_STRIDE - 4 should be indented another level.
As-is it looks like a typo.

Why does one place use 'ins' and the others use 'mov'?

- Eric

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel