Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.7\))
Subject: Re: [PATCH v2 07/13] RISC-V: crypto: add accelerated
 AES-CBC/CTR/ECB/XTS implementations
From:   Jerry Shih <jerry.shih@sifive.com>
In-Reply-To: <20231129201601.GA1174@sol.localdomain>
Date:   Sat, 2 Dec 2023 21:20:42 +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, conor.dooley@microchip.com, ardb@kernel.org,
        heiko@sntech.de, 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: <67F5B520-ED93-4CF2-AACB-D0B64F5B38A1@sifive.com>
References: <20231127070703.1697-1-jerry.shih@sifive.com>
 <20231127070703.1697-8-jerry.shih@sifive.com>
 <20231128040716.GI1463@sol.localdomain>
 <7DFBB20D-B8D4-409B-8562-4C60E67FD279@sifive.com>
 <20231129201601.GA1174@sol.localdomain>
To:     Eric Biggers <ebiggers@kernel.org>
Precedence: bulk

On Nov 30, 2023, at 04:16, Eric Biggers <ebiggers@kernel.org> wrote:
> On Wed, Nov 29, 2023 at 03:57:25PM +0800, Jerry Shih wrote:
>> On Nov 28, 2023, at 12:07, Eric Biggers <ebiggers@kernel.org> wrote:
>>> On Mon, Nov 27, 2023 at 03:06:57PM +0800, Jerry Shih wrote:
>>>> +typedef void (*aes_xts_func)(const u8 *in, u8 *out, size_t length,
>>>> +			     const struct crypto_aes_ctx *key, u8 *iv,
>>>> +			     int update_iv);
>>>=20
>>> There's no need for this indirection, because the function pointer =
can only have
>>> one value.
>>>=20
>>> Note also that when Control Flow Integrity is enabled, assembly =
functions can
>>> only be called indirectly when they use SYM_TYPED_FUNC_START.  =
That's another
>>> reason to avoid indirect calls that aren't actually necessary.
>>=20
>> We have two function pointers for encryption and decryption.
>> 	static int xts_encrypt(struct skcipher_request *req)
>> 	{
>> 		return xts_crypt(req, =
rv64i_zvbb_zvkg_zvkned_aes_xts_encrypt);
>> 	}
>>=20
>> 	static int xts_decrypt(struct skcipher_request *req)
>> 	{
>> 		return xts_crypt(req, =
rv64i_zvbb_zvkg_zvkned_aes_xts_decrypt);
>> 	}
>> The enc and dec path could be folded together into `xts_crypt()`, but =
we will have
>> additional branches for enc/decryption path if we don't want to have =
the indirect calls.
>> Use `SYM_TYPED_FUNC_START` in asm might be better.
>>=20
>=20
> Right.  Normal branches are still more efficient and straightforward =
than
> indirect calls, though, and they don't need any special considerations =
for CFI.
> So I'd just add a 'bool encrypt' or 'bool decrypt' argument to =
xts_crypt(), and
> make xts_crypt() call the appropriate assembly function based on that.

Fixed.
The xts_crypt() now has an additional bool argument for enc/decryption.

>>> Did you consider writing xts_crypt() the way that arm64 and x86 do =
it?  The
>>> above seems to reinvent sort of the same thing from first =
principles.  I'm
>>> wondering if you should just copy the existing approach for now.  =
Then there
>>> would be no need to add the scatterwalk_next() function, and also =
the handling
>>> of inputs that don't need ciphertext stealing would be a bit more =
streamlined.
>>=20
>> I will check the arm and x86's implementations.
>> But the `scatterwalk_next()` proposed in this series does the same =
thing as the
>> call `scatterwalk_ffwd()` in arm and x86's implementations.
>> The scatterwalk_ffwd() iterates from the beginning of =
scatterlist(O(n)), but the=20
>> scatterwalk_next() is just iterates from the end point of the last =
used
>> scatterlist(O(1)).
>=20
> Sure, but your scatterwalk_next() only matters when there are multiple
> scatterlist entries and the AES-XTS message length isn't a multiple of =
the AES
> block size.  That's not an important case, so there's little need to
> micro-optimize it.  The case that actually matters for AES-XTS is a =
single-entry
> scatterlist containing a whole number of AES blocks.

The v3 patch will remove the `scatterwalk_next()` and use =
`scatterwalk_ffwd()`
instead.

-Jerry=