Re: [PATCH v3] arm64/crypto: Accelerated CRC T10 DIF computation

From: Ard Biesheuvel
Date: Tue Nov 22 2016 - 07:53:21 EST

Next message: Lee Jones: "Re: [PATCH v2 1/3] mfd: arizona: Correctly clean up after IRQs"
Previous message: Andrew Jones: "Re: [kvm-unit-tests PATCH v10 3/3] arm: pmu: Add CPI checking"
In reply to: YueHaibing: "[PATCH v3] arm64/crypto: Accelerated CRC T10 DIF computation"
Next in thread: Ard Biesheuvel: "Re: [PATCH v3] arm64/crypto: Accelerated CRC T10 DIF computation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 22 November 2016 at 10:14, YueHaibing <yuehaibing@xxxxxxxxxx> wrote:
> This is the ARM64 CRC T10 DIF transform accelerated with the ARMv8
> NEON instruction.The config CRYPTO_CRCT10DIF_NEON should be turned
> on to enable the feature.The crc_t10dif crypto library function will
> use this faster algorithm when crct10dif_neon module is loaded.
>

What is this algorithm commonly used for? In other words, why is it a
good idea to add support for this algorithm to the kernel?

> Tcrypt benchmark results:
>
> HIP06 (mode=320 sec=2)
>
> The ratio of bytes/sec crct10dif-neon Vs. crct10dif-generic:
>
> TEST neon generic ratio
> 16 byte blocks, 16 bytes per update, 1 updates 214506112 171095400 1.25
> 64 byte blocks, 16 bytes per update, 4 updates 139385312 119036352 1.17
> 64 byte blocks, 64 bytes per update, 1 updates 671523712 198945344 3.38
> 256 byte blocks, 16 bytes per update, 16 updates 157674880 125146752 1.26
> 256 byte blocks, 64 bytes per update, 4 updates 491888128 175764096 2.80
> 256 byte blocks, 256 bytes per update, 1 updates 2123298176 206995200 10.26
> 1024 byte blocks, 16 bytes per update, 64 updates 161243136 126460416 1.28
> 1024 byte blocks, 256 bytes per update, 4 updates 1643020800 200027136 8.21
> 1024 byte blocks, 1024 bytes per update, 1 updates 4238239232 209106432 20.27
> 2048 byte blocks, 16 bytes per update, 128 updates 162079744 126953472 1.28
> 2048 byte blocks, 256 bytes per update, 8 updates 1693587456 200867840 8.43
> 2048 byte blocks, 1024 bytes per update, 2 updates 3424323584 206330880 16.60
> 2048 byte blocks, 2048 bytes per update, 1 updates 5228207104 208620544 25.06
> 4096 byte blocks, 16 bytes per update, 256 updates 162304000 126894080 1.28
> 4096 byte blocks, 256 bytes per update, 16 updates 1731862528 201197568 8.61
> 4096 byte blocks, 1024 bytes per update, 4 updates 3668625408 207003648 17.72
> 4096 byte blocks, 4096 bytes per update, 1 updates 5551239168 209127424 26.54
> 8192 byte blocks, 16 bytes per update, 512 updates 162779136 126984192 1.28
> 8192 byte blocks, 256 bytes per update, 32 updates 1753702400 201420800 8.71
> 8192 byte blocks, 1024 bytes per update, 8 updates 3760918528 207351808 18.14
> 8192 byte blocks, 4096 bytes per update, 2 updates 5483655168 208928768 26.25
> 8192 byte blocks, 8192 bytes per update, 1 updates 5623377920 209108992 26.89
>
> Signed-off-by: YueHaibing <yuehaibing@xxxxxxxxxx>
> Signed-off-by: YangShengkai <yangshengkai@xxxxxxxxxx>
> Signed-off-by: Ding Tianhong <dingtianhong@xxxxxxxxxx>
> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>
> ---
> arch/arm64/crypto/Kconfig | 5 +
> arch/arm64/crypto/Makefile | 4 +
> arch/arm64/crypto/crct10dif-neon-asm_64.S | 751 ++++++++++++++++++++++++++++++
> arch/arm64/crypto/crct10dif-neon_glue.c | 115 +++++
> 4 files changed, 875 insertions(+)
> create mode 100644 arch/arm64/crypto/crct10dif-neon-asm_64.S
> create mode 100644 arch/arm64/crypto/crct10dif-neon_glue.c
>
> diff --git a/arch/arm64/crypto/Kconfig b/arch/arm64/crypto/Kconfig
> index 2cf32e9..2e450bf 100644
> --- a/arch/arm64/crypto/Kconfig
> +++ b/arch/arm64/crypto/Kconfig
> @@ -23,6 +23,11 @@ config CRYPTO_GHASH_ARM64_CE
> depends on ARM64 && KERNEL_MODE_NEON
> select CRYPTO_HASH
>
> +config CRYPTO_CRCT10DIF_NEON
> + tristate "CRCT10DIF hardware acceleration using NEON instructions"
> + depends on ARM64 && KERNEL_MODE_NEON
> + select CRYPTO_HASH
> +

Could you please follow the existing pattern:

config CRYPTO_CRCT10DIF_ARM64_NEON

> config CRYPTO_AES_ARM64_CE
> tristate "AES core cipher using ARMv8 Crypto Extensions"
> depends on ARM64 && KERNEL_MODE_NEON
> diff --git a/arch/arm64/crypto/Makefile b/arch/arm64/crypto/Makefile
> index abb79b3..6c9ff2c 100644
> --- a/arch/arm64/crypto/Makefile
> +++ b/arch/arm64/crypto/Makefile
> @@ -29,6 +29,10 @@ aes-ce-blk-y := aes-glue-ce.o aes-ce.o
> obj-$(CONFIG_CRYPTO_AES_ARM64_NEON_BLK) += aes-neon-blk.o
> aes-neon-blk-y := aes-glue-neon.o aes-neon.o
>
> +obj-$(CONFIG_CRYPTO_CRCT10DIF_NEON) += crct10dif-neon.o
> +crct10dif-neon-y := crct10dif-neon-asm_64.o crct10dif-neon_glue.o
> +AFLAGS_crct10dif-neon-asm_64.o := -march=armv8-a+crypto
> +

Please drop this line, and add

.cpu generic+crypto

to the .S file

> AFLAGS_aes-ce.o := -DINTERLEAVE=4
> AFLAGS_aes-neon.o := -DINTERLEAVE=4
>
> diff --git a/arch/arm64/crypto/crct10dif-neon-asm_64.S b/arch/arm64/crypto/crct10dif-neon-asm_64.S
> new file mode 100644
> index 0000000..2ae3033
> --- /dev/null
> +++ b/arch/arm64/crypto/crct10dif-neon-asm_64.S
> @@ -0,0 +1,751 @@
> +/*
> + * Copyright (c) 2016-2017 Hisilicon Limited.
> + *

Please drop the 2017 here.

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/assembler.h>
> +
> +.global crc_t10dif_neon

Please drop this .global, and use ENTRY() below

> +.text
> +
> +/* X0 is initial CRC value
> + * X1 is data buffer
> + * X2 is the length of buffer
> + * X3 is the backup buffer(for extend)
> + * X4 for other extend parameter(for extend)
> + * Q0, Q1, Q2, Q3 maybe as parameter for other functions,
> + * the value of Q0, Q1, Q2, Q3 maybe modified.
> + *
> + * suggestion:
> + * 1. dont use general purpose register for calculation
> + * 2. set data endianness outside of the kernel
> + * 3. use ext as shifting around
> + * 4. dont use LD3/LD4, ST3/ST4
> + */
> +

Whose suggestions are these, and why? I do appreciate comments like
this, but only if I can learn something from it

> +crc_t10dif_neon:

ENTRY()

> + /* push the register to stack that CRC16 will use */
> + STP X5, X6, [sp, #-0x10]!

Please use an ordinary stack frame, i.e.,

stp x29, x30, [sp, #-xxx]!
mov x29, sp

where xxx is the entire allocation you need for stacking callee save registers

> + STP X7, X8, [sp, #-0x10]!
> + STP X9, X10, [sp, #-0x10]!
> + STP X11, X12, [sp, #-0x10]!
> + STP X13, X14, [sp, #-0x10]!

These are not callee save, so no need to stack them

> + STP Q10, Q11, [sp, #-0x20]!
> + STP Q12, Q13, [sp, #-0x20]!
> + STP Q4, Q5, [sp, #-0x20]!
> + STP Q6, Q7, [sp, #-0x20]!
> + STP Q8, Q9, [sp, #-0x20]!
> + STP Q14, Q15, [sp, #-0x20]!
> + STP Q16, Q17, [sp, #-0x20]!
> + STP Q18, Q19, [sp, #-0x20]!
> +

What is the point of stacking the NEON registers? Also, as a general
note, could you switch to lower case throughout the file?

> + SUB sp,sp,#0x20
> +

Please account for locals in the allocation above. Only outgoing
arguments should be allocated below the frame pointer

> + MOV X11, #0 // PUSH STACK FLAG
> +

What does this comment mean?

> + CMP X2, #0x80
> + B.LT 2f // _less_than_128, <128
> +

Redundant comment

> + /* V10/V11/V12/V13 is 128bit.
> + * we get data 512bit( by cacheline ) each time
> + */
> + LDP Q10, Q11, [X1], #0x20
> + LDP Q12, Q13, [X1], #0x20
> +
> + /* move the initial value to V6 register */
> + LSL X0, X0, #48
> + EOR V6.16B, V6.16B, V6.16B
> + MOV V6.D[1], X0
> +
> + /* big-little end change. because the data in memory is little-end,
> + * we deal the data for bigend
> + */
> +

What if I am using a big endian kernel? Hint: you probably need to
wrap these in CPU_LE()

> + REV64 V10.16B, V10.16B
> + REV64 V11.16B, V11.16B
> + REV64 V12.16B, V12.16B
> + REV64 V13.16B, V13.16B
> + EXT V10.16B, V10.16B, V10.16B, #8
> + EXT V11.16B, V11.16B, V11.16B, #8
> + EXT V12.16B, V12.16B, V12.16B, #8
> + EXT V13.16B, V13.16B, V13.16B, #8
> +
> + EOR V10.16B, V10.16B, V6.16B
> +
> + SUB X2, X2, #0x80
> + ADD X5, X1, #0x20
> +
> + /* deal data when the size of buffer bigger than 128 bytes */
> + /* _fold_64_B_loop */
> + LDR Q6,=0xe658000000000000044c000000000000

Could you move all these non-trivial constants to a separate location
(after the end of the function), and name them?

> +1:
> +
> + LDP Q16, Q17, [X1] ,#0x40
> + LDP Q18, Q19, [X5], #0x40
> +
> + /* carry-less multiply.
> + * V10 high-64bits carry-less multiply
> + * V6 high-64bits(PMULL2)
> + * V11 low-64bits carry-less multiply V6 low-64bits(PMULL)
> + */
> +
> + PMULL2 V4.1Q, V10.2D, V6.2D
> + PMULL V10.1Q, V10.1D, V6.1D
> + PMULL2 V5.1Q, V11.2D, V6.2D
> + PMULL V11.1Q, V11.1D, V6.1D
> +

These instructions are only available if you have the PMULL extension,
so this algorithm is not plain NEON.

> + REV64 V16.16B, V16.16B
> + REV64 V17.16B, V17.16B
> + REV64 V18.16B, V18.16B
> + REV64 V19.16B, V19.16B
> +

Endian swap on LE only?

> + PMULL2 V14.1Q, V12.2D, V6.2D
> + PMULL V12.1Q, V12.1D, V6.1D
> + PMULL2 V15.1Q, V13.2D, V6.2D
> + PMULL V13.1Q, V13.1D, V6.1D
> +
> + EXT V16.16B, V16.16B, V16.16B, #8
> + EOR V10.16B, V10.16B, V4.16B
> +
> + EXT V17.16B, V17.16B, V17.16B, #8
> + EOR V11.16B, V11.16B, V5.16B
> +
> + EXT V18.16B, V18.16B, V18.16B, #8
> + EOR V12.16B, V12.16B, V14.16B
> +
> + EXT V19.16B, V19.16B, V19.16B, #8
> + EOR V13.16B, V13.16B, V15.16B
> +
> + SUB X2, X2, #0x40
> +
> +
> + EOR V10.16B, V10.16B, V16.16B
> + EOR V11.16B, V11.16B, V17.16B
> +
> + EOR V12.16B, V12.16B, V18.16B
> + EOR V13.16B, V13.16B, V19.16B
> +
> + CMP X2, #0x0
> + B.GE 1b // >=0
> +
> + LDR Q6, =0x06df0000000000002d56000000000000
> + MOV V4.16B, V10.16B
> + /* V10 carry-less 0x06df000000000000([127:64]*[127:64]) */
> + PMULL V4.1Q, V4.1D, V6.1D //switch PMULL & PMULL2 order
> + PMULL2 V10.1Q, V10.2D, V6.2D
> + EOR V11.16B, V11.16B, V4.16B
> + EOR V11.16B, V11.16B, V10.16B
> +
> + MOV V4.16B, V11.16B
> + PMULL V4.1Q, V4.1D, V6.1D //switch PMULL & PMULL2 order
> + PMULL2 V11.1Q, V11.2D, V6.2D
> + EOR V12.16B, V12.16B, V4.16B
> + EOR V12.16B, V12.16B, V11.16B
> +
> + MOV V4.16B, V12.16B
> + PMULL V4.1Q, V4.1D, V6.1D //switch PMULL & PMULL2 order
> + PMULL2 V12.1Q, V12.2D, V6.2D
> + EOR V13.16B, V13.16B, V4.16B
> + EOR V13.16B, V13.16B, V12.16B
> +
> + ADD X2, X2, #48
> + CMP X2, #0x0
> + B.LT 3f // _final_reduction_for_128, <0
> +
> + /* _16B_reduction_loop */
> +4:
> + /* unrelated load as early as possible*/
> + LDR Q10, [X1], #0x10
> +
> + MOV V4.16B, V13.16B
> + PMULL2 V13.1Q, V13.2D, V6.2D
> + PMULL V4.1Q, V4.1D, V6.1D
> + EOR V13.16B, V13.16B, V4.16B
> +
> + REV64 V10.16B, V10.16B
> + EXT V10.16B, V10.16B, V10.16B, #8
> +
> + EOR V13.16B, V13.16B, V10.16B
> +
> + SUB X2, X2, #0x10
> + CMP X2, #0x0
> + B.GE 4b // _16B_reduction_loop, >=0
> +
> + /* _final_reduction_for_128 */
> +3: ADD X2, X2, #0x10
> + CMP X2, #0x0
> + B.EQ 5f // _128_done, ==0
> +
> + /* _get_last_two_xmms */

Bogus comment. I guess you ported this code from x86, are you sure you
don't need to credit the original author?

> +6: MOV V12.16B, V13.16B
> + SUB X1, X1, #0x10
> + ADD X1, X1, X2
> + LDR Q11, [X1], #0x10
> + REV64 V11.16B, V11.16B
> + EXT V11.16B, V11.16B, V11.16B, #8
> +
> + CMP X2, #8
> + B.EQ 50f
> + B.LT 51f
> + B.GT 52f
> +
> +50:
> + /* dont use X register as temp one */
> + FMOV D14, D12
> + MOVI D12, #0
> + MOV V12.D[1],V14.D[0]
> + B 53f
> +51:
> + MOV X9, #64
> + LSL X13, X2, #3 // <<3 equal x8
> + SUB X9, X9, X13
> + MOV X5, V12.D[0] // low 64-bit
> + MOV X6, V12.D[1] // high 64-bit
> + LSR X10, X5, X9 // high bit of low 64-bit
> + LSL X7, X5, X13
> + LSL X8, X6, X13
> + ORR X8, X8, X10 // combination of high 64-bit
> + MOV V12.D[1], X8
> + MOV V12.D[0], X7
> +
> + B 53f
> +52:
> + LSL X13, X2, #3 // <<3 equal x8
> + SUB X13, X13, #64
> +
> + DUP V18.2D, X13
> + FMOV D16, D12
> + USHL D16, D16, D18
> + EXT V12.16B, V16.16B, V16.16B, #8
> +
> +53:
> + MOVI D14, #0 //add one zero constant
> +
> + CMP X2, #0
> + B.EQ 30f
> + CMP X2, #1
> + B.EQ 31f
> + CMP X2, #2
> + B.EQ 32f
> + CMP X2, #3
> + B.EQ 33f
> + CMP X2, #4
> + B.EQ 34f
> + CMP X2, #5
> + B.EQ 35f
> + CMP X2, #6
> + B.EQ 36f
> + CMP X2, #7
> + B.EQ 37f
> + CMP X2, #8
> + B.EQ 38f
> + CMP X2, #9
> + B.EQ 39f
> + CMP X2, #10
> + B.EQ 40f
> + CMP X2, #11
> + B.EQ 41f
> + CMP X2, #12
> + B.EQ 42f
> + CMP X2, #13
> + B.EQ 43f
> + CMP X2, #14
> + B.EQ 44f
> + CMP X2, #15
> + B.EQ 45f
> +

This looks awful. If you make the snippets below a fixed size, you
could use a computed goto instead

> + // >> 128bit
> +30:
> + EOR V13.16B, V13.16B, V13.16B
> + EOR V8.16B, V8.16B, V8.16B
> + LDR Q9,=0xffffffffffffffffffffffffffffffff

Shouldn't you initialize q8 here as well. And in general, couldn't you
use some kind of shift to generate these constants (in all cases
below)?
> + B 46f
> +
> + // >> 120bit
> +31:
> + USHR V13.2D, V13.2D, #56
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xff
> + LDR Q9,=0xffffffffffffffffffffffffffffff00
> + B 46f
> +
> + // >> 112bit
> +32:
> + USHR V13.2D, V13.2D, #48
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffff
> + LDR Q9,=0xffffffffffffffffffffffffffff0000
> + B 46f
> +
> + // >> 104bit
> +33:
> + USHR V13.2D, V13.2D, #40
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffffff
> + LDR Q9,=0xffffffffffffffffffffffffff000000
> + B 46f
> +
> + // >> 96bit
> +34:
> + USHR V13.2D, V13.2D, #32
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffffffff
> + LDR Q9,=0xffffffffffffffffffffffff00000000
> + B 46f
> +
> + // >> 88bit
> +35:
> + USHR V13.2D, V13.2D, #24
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffffffffff
> + LDR Q9,=0xffffffffffffffffffffff0000000000
> + B 46f
> +
> + // >> 80bit
> +36:
> + USHR V13.2D, V13.2D, #16
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffffffffffff
> + LDR Q9,=0xffffffffffffffffffff000000000000
> + B 46f
> +
> + // >> 72bit
> +37:
> + USHR V13.2D, V13.2D, #8
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffffffffffffff
> + LDR Q9,=0xffffffffffffffffff00000000000000
> + B 46f
> +
> + // >> 64bit
> +38:
> + EXT V13.16B, V13.16B, V14.16B, #8
> + LDR Q8,=0xffffffffffffffff
> + LDR Q9,=0xffffffffffffffff0000000000000000
> + B 46f
> +
> + // >> 56bit
> +39:
> + EXT V13.16B, V13.16B, V13.16B, #7
> + MOV V13.S[3], V14.S[0]
> + MOV V13.H[5], V14.H[0]
> + MOV V13.B[9], V14.B[0]
> +
> + LDR Q8,=0xffffffffffffffffff
> + LDR Q9,=0xffffffffffffff000000000000000000
> + B 46f
> +
> + // >> 48bit
> +40:
> + EXT V13.16B, V13.16B, V13.16B, #6
> + MOV V13.S[3], V14.S[0]
> + MOV V13.H[5], V14.H[0]
> +
> + LDR Q8,=0xffffffffffffffffffff
> + LDR Q9,=0xffffffffffff00000000000000000000
> + B 46f
> +
> + // >> 40bit
> +41:
> + EXT V13.16B, V13.16B, V13.16B, #5
> + MOV V13.S[3], V14.S[0]
> + MOV V13.B[11], V14.B[0]
> +
> + LDR Q8,=0xffffffffffffffffffffff
> + LDR Q9,=0xffffffffff0000000000000000000000
> + B 46f
> +
> + // >> 32bit
> +42:
> + EXT V13.16B, V13.16B, V13.16B, #4
> + MOV V13.S[3], V14.S[0]
> +
> + LDR Q8,=0xffffffffffffffffffffffff
> + LDR Q9,=0xffffffff000000000000000000000000
> + B 46f
> +
> + // >> 24bit
> +43:
> + EXT V13.16B, V13.16B, V13.16B, #3
> + MOV V13.H[7], V14.H[0]
> + MOV V13.B[13], V14.B[0]
> +
> + LDR Q8,=0xffffffffffffffffffffffffff
> + LDR Q9,=0xffffff00000000000000000000000000
> + B 46f
> +
> + // >> 16bit
> +44:
> + EXT V13.16B, V13.16B, V13.16B, #2
> + MOV V13.H[7], V14.H[0]
> +
> + LDR Q8,=0xffffffffffffffffffffffffffff
> + LDR Q9,=0xffff0000000000000000000000000000
> + B 46f
> +
> + // >> 8bit
> +45:
> + EXT V13.16B, V13.16B, V13.16B, #1
> + MOV V13.B[15], V14.B[0]
> +
> + LDR Q8,=0xffffffffffffffffffffffffffffff
> + LDR Q9,=0xff000000000000000000000000000000
> +
> + // backup V12 first
> + // pblendvb xmm1, xmm2

Another remnant of the x86 version, please remove

> +46:
> + AND V12.16B, V12.16B, V9.16B
> + AND V11.16B, V11.16B, V8.16B
> + ORR V11.16B, V11.16B, V12.16B
> +
> + MOV V12.16B, V11.16B
> + MOV V4.16B, V13.16B
> + PMULL2 V13.1Q, V13.2D, V6.2D
> + PMULL V4.1Q, V4.1D, V6.1D
> + EOR V13.16B, V13.16B, V4.16B
> + EOR V13.16B, V13.16B, V12.16B
> +
> + /* _128_done. we change the Q6 D[0] and D[1] */
> +5: LDR Q6, =0x2d560000000000001368000000000000
> + MOVI D14, #0
> + MOV V10.16B, V13.16B
> + PMULL2 V13.1Q, V13.2D, V6.2D
> +
> + MOV V10.D[1], V10.D[0]
> + MOV V10.D[0], V14.D[0] //set zero
> +
> + EOR V13.16B, V13.16B, V10.16B
> +
> + MOV V10.16B, V13.16B
> + LDR Q7, =0x00000000FFFFFFFFFFFFFFFFFFFFFFFF
> + AND V10.16B, V10.16B, V7.16B
> +
> + MOV S13, V13.S[3]
> +
> + PMULL V13.1Q, V13.1D, V6.1D
> + EOR V13.16B, V13.16B, V10.16B
> +
> + /* _barrett */

What does '_barrett' mean?

> +7: LDR Q6, =0x00000001f65a57f8000000018bb70000
> + MOVI D14, #0
> + MOV V10.16B, V13.16B
> + PMULL2 V13.1Q, V13.2D, V6.2D
> +
> + EXT V13.16B, V13.16B, V13.16B, #12
> + MOV V13.S[0], V14.S[0]
> +
> + EXT V6.16B, V6.16B, V6.16B, #8
> + PMULL2 V13.1Q, V13.2D, V6.2D
> +
> + EXT V13.16B, V13.16B, V13.16B, #12
> + MOV V13.S[0], V14.S[0]
> +
> + EOR V13.16B, V13.16B, V10.16B
> + MOV X0, V13.D[0]
> +
> + /* _cleanup */
> +8: MOV X14, #48
> + LSR X0, X0, X14

Why the temp x14?

> +99:
> + ADD sp, sp, #0x20
> +
> + LDP Q18, Q19, [sp], #0x20
> + LDP Q16, Q17, [sp], #0x20
> + LDP Q14, Q15, [sp], #0x20
> +
> + LDP Q8, Q9, [sp], #0x20
> + LDP Q6, Q7, [sp], #0x20
> + LDP Q4, Q5, [sp], #0x20
> + LDP Q12, Q13, [sp], #0x20
> + LDP Q10, Q11, [sp], #0x20
> + LDP X13, X14, [sp], #0x10
> + LDP X11, X12, [sp], #0x10
> + LDP X9, X10, [sp], #0x10
> + LDP X7, X8, [sp], #0x10
> + LDP X5, X6, [sp], #0x10
> +

None of these registers need to be restored. The only thing you need
(to mirror the prologue)

ldp x29, x30, [sp], #xxx
ret

where xxx is the same value you used at the beginning.

> + RET
> +
> + /* _less_than_128 */
> +2: CMP X2, #32
> + B.LT 9f // _less_than_32
> + LDR Q6, =0x06df0000000000002d56000000000000
> +
> + LSL X0, X0, #48
> + LDR Q10, =0x0

Please use movi here

> + MOV V10.D[1], X0
> + LDR Q13, [X1], #0x10
> + REV64 V13.16B, V13.16B
> + EXT V13.16B, V13.16B, V13.16B, #8
> +
> + EOR V13.16B, V13.16B, V10.16B
> +
> + SUB X2, X2, #32
> + B 4b
> +
> + /* _less_than_32 */
> +9: CMP X2, #0
> + B.EQ 99b // _cleanup

You can use CBZ here

> + LSL X0, X0, #48
> + LDR Q10,=0x0

Please use movi here

> + MOV V10.D[1], X0
> +
> + CMP X2, #16
> + B.EQ 10f // _exact_16_left
> + B.LE 11f // _less_than_16_left
> + LDR Q13, [X1], #0x10
> +
> + REV64 V13.16B, V13.16B
> + EXT V13.16B, V13.16B, V13.16B, #8
> +
> + EOR V13.16B, V13.16B, V10.16B
> + SUB X2, X2, #16
> + LDR Q6, =0x06df0000000000002d56000000000000
> + B 6b // _get_last_two_xmms

Another bogus comment

> +
> + /* _less_than_16_left */
> +11: CMP X2, #4
> + B.LT 13f // _only_less_than_4
> +
> + /* backup the length of data, we used in _less_than_2_left */
> + MOV X8, X2
> + CMP X2, #8
> + B.LT 14f // _less_than_8_left
> +
> + LDR X14, [X1], #8
> + /* push the data to stack, we backup the data to V10 */
> + STR X14, [sp, #0]
> + SUB X2, X2, #8
> + ADD X11, X11, #8
> +
> + /* _less_than_8_left */
> +14: CMP X2, #4
> + B.LT 15f // _less_than_4_left
> +
> + /* get 32bit data */
> + LDR W5, [X1], #4
> +
> + /* push the data to stack */
> + STR W5, [sp, X11]
> + SUB X2, X2, #4
> + ADD X11, X11, #4
> +
> + /* _less_than_4_left */
> +15: CMP X2, #2
> + B.LT 16f // _less_than_2_left
> +
> + /* get 16bits data */
> + LDRH W6, [X1], #2
> +
> + /* push the data to stack */
> + STRH W6, [sp, X11]
> + SUB X2, X2, #2
> + ADD X11, X11, #2
> +
> + /* _less_than_2_left */
> +16:
> + /* get 8bits data */
> + LDRB W7, [X1], #1
> + STRB W7, [sp, X11]
> + ADD X11, X11, #1
> +
> + /* POP data from stack, store to V13 */
> + LDR Q13, [sp]
> + MOVI D14, #0
> + REV64 V13.16B, V13.16B
> + MOV V8.16B, V13.16B
> + MOV V13.D[1], V8.D[0]
> + MOV V13.D[0], V8.D[1]
> +
> + EOR V13.16B, V13.16B, V10.16B
> + CMP X8, #15
> + B.EQ 80f
> + CMP X8, #14
> + B.EQ 81f
> + CMP X8, #13
> + B.EQ 82f
> + CMP X8, #12
> + B.EQ 83f
> + CMP X8, #11
> + B.EQ 84f
> + CMP X8, #10
> + B.EQ 85f
> + CMP X8, #9
> + B.EQ 86f
> + CMP X8, #8
> + B.EQ 87f
> + CMP X8, #7
> + B.EQ 88f
> + CMP X8, #6
> + B.EQ 89f
> + CMP X8, #5
> + B.EQ 90f
> + CMP X8, #4
> + B.EQ 91f
> + CMP X8, #3
> + B.EQ 92f
> + CMP X8, #2
> + B.EQ 93f
> + CMP X8, #1
> + B.EQ 94f
> + CMP X8, #0
> + B.EQ 95f
> +

Again, please use a computed goto instead

> +80:
> + EXT V13.16B, V13.16B, V13.16B, #1
> + MOV V13.B[15], V14.B[0]
> + B 5b
> +
> +81:
> + EXT V13.16B, V13.16B, V13.16B, #2
> + MOV V13.H[7], V14.H[0]
> + B 5b
> +
> +82:
> + EXT V13.16B, V13.16B, V13.16B, #3
> + MOV V13.H[7], V14.H[0]
> + MOV V13.B[13], V14.B[0]
> + B 5b
> +83:
> +
> + EXT V13.16B, V13.16B, V13.16B, #4
> + MOV V13.S[3], V14.S[0]
> + B 5b
> +
> +84:
> + EXT V13.16B, V13.16B, V13.16B, #5
> + MOV V13.S[3], V14.S[0]
> + MOV V13.B[11], V14.B[0]
> + B 5b
> +
> +85:
> + EXT V13.16B, V13.16B, V13.16B, #6
> + MOV V13.S[3], V14.S[0]
> + MOV V13.H[5], V14.H[0]
> + B 5b
> +
> +86:
> + EXT V13.16B, V13.16B, V13.16B, #7
> + MOV V13.S[3], V14.S[0]
> + MOV V13.H[5], V14.H[0]
> + MOV V13.B[9], V14.B[0]
> + B 5b
> +
> +87:
> + MOV V13.D[0], V13.D[1]
> + MOV V13.D[1], V14.D[0]
> + B 5b
> +
> +88:
> + EXT V13.16B, V13.16B, V13.16B, #9
> + MOV V13.D[1], V14.D[0]
> + MOV V13.B[7], V14.B[0]
> + B 5b
> +
> +89:
> + EXT V13.16B, V13.16B, V13.16B, #10
> + MOV V13.D[1], V14.D[0]
> + MOV V13.H[3], V14.H[0]
> + B 5b
> +
> +90:
> + EXT V13.16B, V13.16B, V13.16B, #11
> + MOV V13.D[1], V14.D[0]
> + MOV V13.H[3], V14.H[0]
> + MOV V13.B[5], V14.B[0]
> + B 5b
> +
> +91:
> + MOV V13.S[0], V13.S[3]
> + MOV V13.D[1], V14.D[0]
> + MOV V13.S[1], V14.S[0]
> + B 5b
> +
> +92:
> + EXT V13.16B, V13.16B, V13.16B, #13
> + MOV V13.D[1], V14.D[0]
> + MOV V13.S[1], V14.S[0]
> + MOV V13.B[3], V14.B[0]
> + B 5b
> +
> +93:
> + MOV V15.H[0], V13.H[7]
> + MOV V13.16B, V14.16B
> + MOV V13.H[0], V15.H[0]
> + B 5b
> +
> +94:
> + MOV V15.B[0], V13.B[15]
> + MOV V13.16B, V14.16B
> + MOV V13.B[0], V15.B[0]
> + B 5b
> +
> +95:
> + LDR Q13,=0x0

movi

> + B 5b // _128_done
> +
> + /* _exact_16_left */
> +10:
> + LD1 { V13.2D }, [X1], #0x10
> +
> + REV64 V13.16B, V13.16B
> + EXT V13.16B, V13.16B, V13.16B, #8
> + EOR V13.16B, V13.16B, V10.16B
> + B 5b // _128_done
> +
> + /* _only_less_than_4 */
> +13: CMP X2, #3
> + MOVI D14, #0
> + B.LT 17f //_only_less_than_3
> +
> + LDR S13, [X1], #4
> + MOV V13.B[15], V13.B[0]
> + MOV V13.B[14], V13.B[1]
> + MOV V13.B[13], V13.B[2]
> + MOV V13.S[0], V13.S[1]
> +
> + EOR V13.16B, V13.16B, V10.16B
> +
> + EXT V13.16B, V13.16B, V13.16B, #5
> +
> + MOV V13.S[3], V14.S[0]
> + MOV V13.B[11], V14.B[0]
> +
> + B 7b // _barrett
> + /* _only_less_than_3 */
> +17:
> + CMP X2, #2
> + B.LT 18f // _only_less_than_2
> +
> + LDR H13, [X1], #2
> + MOV V13.B[15], V13.B[0]
> + MOV V13.B[14], V13.B[1]
> + MOV V13.H[0], V13.H[1]
> +
> + EOR V13.16B, V13.16B, V10.16B
> +
> + EXT V13.16B, V13.16B, V13.16B, #6
> + MOV V13.S[3], V14.S[0]
> + MOV V13.H[5], V14.H[0]
> +
> + B 7b // _barrett
> +
> + /* _only_less_than_2 */
> +18:
> + LDRB W7, [X1], #1
> + LDR Q13, = 0x0
> + MOV V13.B[15], W7
> +
> + EOR V13.16B, V13.16B, V10.16B
> +
> + EXT V13.16B, V13.16B, V13.16B, #7
> + MOV V13.S[3], V14.S[0]
> + MOV V13.H[5], V14.H[0]
> + MOV V13.B[9], V14.B[0]
> +
> + B 7b // _barrett

Please end with ENDPROC()

> diff --git a/arch/arm64/crypto/crct10dif-neon_glue.c b/arch/arm64/crypto/crct10dif-neon_glue.c
> new file mode 100644
> index 0000000..a6d8c5c
> --- /dev/null
> +++ b/arch/arm64/crypto/crct10dif-neon_glue.c
> @@ -0,0 +1,115 @@
> +/*
> + * Copyright (c) 2016-2017 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/crc-t10dif.h>
> +#include <crypto/internal/hash.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +
> +asmlinkage __u16 crc_t10dif_neon(__u16 crc, const unsigned char *buf,
> + size_t len);
> +
> +struct chksum_desc_ctx {
> + __u16 crc;
> +};
> +
> +/*
> + * Steps through buffer one byte at at time, calculates reflected
> + * crc using table.
> + */
> +

Where is the table?

> +static int chksum_init(struct shash_desc *desc)
> +{
> + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> + ctx->crc = 0;
> +
> + return 0;
> +}
> +
> +static int chksum_update(struct shash_desc *desc, const u8 *data,
> + unsigned int length)
> +{
> + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> + ctx->crc = crc_t10dif_neon(ctx->crc, data, length);

You need kernel_neon_begin/kernel_neon_end here

> + return 0;
> +}
> +
> +static int chksum_final(struct shash_desc *desc, u8 *out)
> +{
> + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> + *(__u16 *)out = ctx->crc;
> + return 0;
> +}
> +
> +static int __chksum_finup(__u16 *crcp, const u8 *data, unsigned int len,
> + u8 *out)
> +{
> + *(__u16 *)out = crc_t10dif_neon(*crcp, data, len);

... and here

> + return 0;
> +}
> +
> +static int chksum_finup(struct shash_desc *desc, const u8 *data,
> + unsigned int len, u8 *out)
> +{
> + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> + return __chksum_finup(&ctx->crc, data, len, out);
> +}
> +
> +static int chksum_digest(struct shash_desc *desc, const u8 *data,
> + unsigned int length, u8 *out)
> +{
> + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> + return __chksum_finup(&ctx->crc, data, length, out);
> +}
> +
> +static struct shash_alg alg = {
> + .digestsize = CRC_T10DIF_DIGEST_SIZE,
> + .init = chksum_init,
> + .update = chksum_update,
> + .final = chksum_final,
> + .finup = chksum_finup,
> + .digest = chksum_digest,
> + .descsize = sizeof(struct chksum_desc_ctx),
> + .base = {
> + .cra_name = "crct10dif",
> + .cra_driver_name = "crct10dif-neon",
> + .cra_priority = 200,
> + .cra_blocksize = CRC_T10DIF_BLOCK_SIZE,
> + .cra_module = THIS_MODULE,

Please align the = characters here, and add only a single space after

Note that you can do

.base.cra_name = xxx,
.base.xxx

as well.

> + }
> +};
> +
> +static int __init crct10dif_arm64_mod_init(void)
> +{
> + return crypto_register_shash(&alg);

You need to check here if your CPU has support for the 64x64->128
PMULL instruction.

> +}
> +
> +static void __exit crct10dif_arm64_mod_fini(void)
> +{
> + crypto_unregister_shash(&alg);
> +}
> +
> +module_init(crct10dif_arm64_mod_init);
> +module_exit(crct10dif_arm64_mod_fini);
> +
> +MODULE_AUTHOR("YueHaibing <yuehaibing@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("T10 DIF CRC calculation accelerated with ARM64 NEON instruction.");
> +MODULE_LICENSE("GPL");
> +
> +MODULE_ALIAS_CRYPTO("crct10dif");
> +MODULE_ALIAS_CRYPTO("crct10dif-neon");
> --
> 2.7.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Next message: Lee Jones: "Re: [PATCH v2 1/3] mfd: arizona: Correctly clean up after IRQs"
Previous message: Andrew Jones: "Re: [kvm-unit-tests PATCH v10 3/3] arm: pmu: Add CPI checking"
In reply to: YueHaibing: "[PATCH v3] arm64/crypto: Accelerated CRC T10 DIF computation"
Next in thread: Ard Biesheuvel: "Re: [PATCH v3] arm64/crypto: Accelerated CRC T10 DIF computation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]