[PATCH] x86/crypto: ghash: use C implementation for setkey()

[PATCH] x86/crypto: ghash: use C implementation for setkey()

[Ard Biesheuvel]

The GHASH setkey() function uses SSE registers but fails to call
kernel_fpu_begin()/kernel_fpu_end(). Instead of adding these calls, and
then having to deal with the restriction that they cannot be called from
interrupt context, move the setkey() implementation to the C domain.

Note that setkey() does not use any particular SSE features and is not
expected to become a performance bottleneck.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

I suppose this should be marked for stable as well?


 arch/x86/crypto/ghash-clmulni-intel_asm.S  | 29 -----------------------------
 arch/x86/crypto/ghash-clmulni-intel_glue.c | 12 +++++++++++-
 2 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/arch/x86/crypto/ghash-clmulni-intel_asm.S b/arch/x86/crypto/ghash-clmulni-intel_asm.S
index 586f41aac361..185fad49d86f 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_asm.S
+++ b/arch/x86/crypto/ghash-clmulni-intel_asm.S
@@ -24,10 +24,6 @@
 .align 16
 .Lbswap_mask:
 	.octa 0x000102030405060708090a0b0c0d0e0f
-.Lpoly:
-	.octa 0xc2000000000000000000000000000001
-.Ltwo_one:
-	.octa 0x00000001000000000000000000000001
 
 #define DATA	%xmm0
 #define SHASH	%xmm1
@@ -134,28 +130,3 @@ ENTRY(clmul_ghash_update)
 .Lupdate_just_ret:
 	ret
 ENDPROC(clmul_ghash_update)
-
-/*
- * void clmul_ghash_setkey(be128 *shash, const u8 *key);
- *
- * Calculate hash_key << 1 mod poly
- */
-ENTRY(clmul_ghash_setkey)
-	movaps .Lbswap_mask, BSWAP
-	movups (%rsi), %xmm0
-	PSHUFB_XMM BSWAP %xmm0
-	movaps %xmm0, %xmm1
-	psllq $1, %xmm0
-	psrlq $63, %xmm1
-	movaps %xmm1, %xmm2
-	pslldq $8, %xmm1
-	psrldq $8, %xmm2
-	por %xmm1, %xmm0
-	# reduction
-	pshufd $0b00100100, %xmm2, %xmm1
-	pcmpeqd .Ltwo_one, %xmm1
-	pand .Lpoly, %xmm1
-	pxor %xmm1, %xmm0
-	movups %xmm0, (%rdi)
-	ret
-ENDPROC(clmul_ghash_setkey)
diff --git a/arch/x86/crypto/ghash-clmulni-intel_glue.c b/arch/x86/crypto/ghash-clmulni-intel_glue.c
index 6759dd1135be..3945d8095e80 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_glue.c
+++ b/arch/x86/crypto/ghash-clmulni-intel_glue.c
@@ -58,13 +58,23 @@ static int ghash_setkey(struct crypto_shash *tfm,
 			const u8 *key, unsigned int keylen)
 {
 	struct ghash_ctx *ctx = crypto_shash_ctx(tfm);
+	be128 *x = (be128 *)key;
+	u64 a, b;
 
 	if (keylen != GHASH_BLOCK_SIZE) {
 		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
 		return -EINVAL;
 	}
 
-	clmul_ghash_setkey(&ctx->shash, key);
+	/* perform multiplication by 'x' in GF(2^128) */
+	a = be64_to_cpu(x->a);
+	b = be64_to_cpu(x->b);
+
+	ctx->shash.a = (__be64)((b << 1) ^ (a >> 63));
+	ctx->shash.b = (__be64)((a << 1) | (b >> 63));
+
+	if (a >> 63)
+		ctx->shash.b ^= cpu_to_be64(0xc2);
 
 	return 0;
 }
-- 
1.8.3.2

Re: [PATCH] x86/crypto: ghash: use C implementation for setkey()

[Herbert Xu]

On Thu, Mar 27, 2014 at 12:29:00PM +0100, Ard Biesheuvel wrote:
> The GHASH setkey() function uses SSE registers but fails to call
> kernel_fpu_begin()/kernel_fpu_end(). Instead of adding these calls, and
> then having to deal with the restriction that they cannot be called from
> interrupt context, move the setkey() implementation to the C domain.

Note that setkey cannot be called from interrupt context since
allocation/setkey is supposed to be slow-path material.

But your approach is fine by me.

> Note that setkey() does not use any particular SSE features and is not
> expected to become a performance bottleneck.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> I suppose this should be marked for stable as well?

Sure I'll add the cc.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] x86/crypto: ghash: use C implementation for setkey()

[Ard Biesheuvel]

On 27 March 2014 12:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Mar 27, 2014 at 12:29:00PM +0100, Ard Biesheuvel wrote:
>> The GHASH setkey() function uses SSE registers but fails to call
>> kernel_fpu_begin()/kernel_fpu_end(). Instead of adding these calls, and
>> then having to deal with the restriction that they cannot be called from
>> interrupt context, move the setkey() implementation to the C domain.
>
> Note that setkey cannot be called from interrupt context since
> allocation/setkey is supposed to be slow-path material.
>
> But your approach is fine by me.
>

I agree that it makes little sense to call this from atomic context,
but that still means (I think, but the x86 guys should confirm) that
you are supposed to call kernel_fpu_begin() and kernel_fpu_end().

>> Note that setkey() does not use any particular SSE features and is not
>> expected to become a performance bottleneck.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> I suppose this should be marked for stable as well?
>
> Sure I'll add the cc.
>

Perhaps wait for an ack from team x86?

-- 
Ard.

Re: [PATCH] x86/crypto: ghash: use C implementation for setkey()

[Ard Biesheuvel]

On 27 March 2014 12:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 27 March 2014 12:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>> On Thu, Mar 27, 2014 at 12:29:00PM +0100, Ard Biesheuvel wrote:
>>> The GHASH setkey() function uses SSE registers but fails to call
>>> kernel_fpu_begin()/kernel_fpu_end(). Instead of adding these calls, and
>>> then having to deal with the restriction that they cannot be called from
>>> interrupt context, move the setkey() implementation to the C domain.
>>
>> Note that setkey cannot be called from interrupt context since
>> allocation/setkey is supposed to be slow-path material.
>>
>> But your approach is fine by me.
>>
>
> I agree that it makes little sense to call this from atomic context,
> but that still means (I think, but the x86 guys should confirm) that
> you are supposed to call kernel_fpu_begin() and kernel_fpu_end().
>
>>> Note that setkey() does not use any particular SSE features and is not
>>> expected to become a performance bottleneck.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>
>>> I suppose this should be marked for stable as well?
>>
>> Sure I'll add the cc.
>>
>
> Perhaps wait for an ack from team x86?
>

... oh, and I noticed that I forgot to remove the forward declaration
of clmul_ghash_setkey() as well.

Sorry.

-- 
Ard.

Re: [PATCH] x86/crypto: ghash: use C implementation for setkey()

[H. Peter Anvin]

On 03/27/2014 04:46 AM, Ard Biesheuvel wrote:
> On 27 March 2014 12:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>> On Thu, Mar 27, 2014 at 12:29:00PM +0100, Ard Biesheuvel wrote:
>>> The GHASH setkey() function uses SSE registers but fails to call
>>> kernel_fpu_begin()/kernel_fpu_end(). Instead of adding these calls, and
>>> then having to deal with the restriction that they cannot be called from
>>> interrupt context, move the setkey() implementation to the C domain.
>>
>> Note that setkey cannot be called from interrupt context since
>> allocation/setkey is supposed to be slow-path material.
>>
>> But your approach is fine by me.
> 
> I agree that it makes little sense to call this from atomic context,
> but that still means (I think, but the x86 guys should confirm) that
> you are supposed to call kernel_fpu_begin() and kernel_fpu_end().
> 

Yes.  I'm suspecting calling kernel_fpu_begin() for a single GF
operation is probably not worth it, so I'm fine with reimplementing it
in integer logic.

Acked-by: H. Peter Anvin <hpa@linux.intel.com>

	-hpa