Re: [RFC PATCH 00/13] RISC-V crypto with reworked asm files

[Jerry Shih <jerry.shih@sifive.com>]

On Jan 3, 2024, at 22:35, Eric Biggers <ebiggers@kernel.org> wrote:
> On Wed, Jan 03, 2024 at 03:00:29PM +0100, Ard Biesheuvel wrote:
>> On Tue, 2 Jan 2024 at 07:50, Eric Biggers <ebiggers@kernel.org> wrote:
>>> 
>>> As discussed previously, the proposed use of the so-called perlasm for
>>> the RISC-V crypto assembly files makes them difficult to read, and these
>>> files have some other issues such extensively duplicating source code
>>> for the different AES key lengths and for the unrolled hash functions.
>>> There is/was a desire to share code with OpenSSL, but many of the files
>>> have already diverged significantly; also, for most of the algorithms
>>> the source code can be quite short anyway, due to the native support for
>>> them in the RISC-V vector crypto extensions combined with the way the
>>> RISC-V vector extension naturally scales to arbitrary vector lengths.
>>> 
>>> Since we're still waiting for prerequisite patches to be merged anyway,
>>> we have a bit more time to get this cleaned up properly.  So I've had a
>>> go at cleaning up the patchset to use standard .S files, with the code
>>> duplication fixed.  I also made some tweaks to make the different
>>> algorithms consistent with each other and with what exists in the kernel

Do you mean the xts gluing part only? Do we have the different algorithms
for the actual implementation in .S? 

>>> already for other architectures, and tried to improve comments.

The improved comments are better. Thanks.

>>> The result is this series, which passes all tests and is about 2400
>>> lines shorter than the latest version with the perlasm
>>> (https://lore.kernel.org/linux-crypto/20231231152743.6304-1-jerry.shih@sifive.com/).

For the unrolled hash functions case(sha256/512), the .S implementation uses
macro instead. But I think the macro expanded code will be the same. Do we
care about the source code size instead of the final binary code size?

For the AES variants part, the .S uses branches inside the inner loop. Does the
branch inside the inner loop better than the bigger code size?

>>> All the same functionality and general optimizations are still included,
>>> except for some minor optimizations in XTS that I dropped since it's not
>>> clear they are worth the complexity.  (Note that almost all users of XTS
>>> in the kernel only use it with block-aligned messages, so it's not very
>>> important to optimize the ciphertext stealing case.)

The aesni/neon are SIMD, so I think the rvv version could have the different
design. And I think my implementation is very similar to x86-xts except the
tail block numbers for ciphertext stealing case.
The x86-xts-like implementation uses the fixed 2 block for the ciphertext
stealing case.

+		int xts_blocks = DIV_ROUND_UP(req->cryptlen,
+					      AES_BLOCK_SIZE) - 2;

>>> I'd appreciate people's thoughts on this series.  Jerry, I hope I'm not
>>> stepping on your toes too much here, but I think there are some big
>>> improvements here.
>>> 
>> 
>> As I have indicated before, I fully agree with Eric here that avoiding
>> perlasm is preferable: sharing code with OpenSSL is great if we can
>> simply adopt the exact same code (and track OpenSSL as its upstream)
>> but this never really worked that well for skciphers, due to API
>> differences. (The SHA transforms can be reused a bit more easily)

In my opinion, I would prefer the perlasm with the usage of asm mnemonics.
I could see the expanded asm source from perlsm. I don't know how to dump the
expanded asm source when we use asm `.macro` directives. I use objdump to
see the final asm.
And we could use function to modularize the asm implementations. The macro
might do the same things, but it's more clear for me for the argument passing
and more powerful string manipulation.
And I could have scope for the register name binding. It's more clear to me
comparing with a long list of `#define xxx`.

If the pure .S is still more preferable in kernel, let's do that.

-Jerry

>> I will also note that perlasm is not as useful for RISC-V as it is for
>> other architectures: in OpenSSL, perlasm is also used to abstract
>> differences in calling conventions between, e.g., x86_64 on Linux vs
>> Windows, or to support building with outdated [proprietary]
>> toolchains.
>> 
>> I do wonder if we could also use .req directives for register aliases
>> instead of CPP defines? It shouldn't matter for working code, but the
>> diagnostics tend to be a bit more useful if the aliases are visible to
>> the assembler.
> 
> .req unfortunately isn't an option since it is specific to AArch64 and ARM
> assembly.  So we have to use #defines like x86 does.  Ultimately, the effect is
> about the same.
> 
> - Eric