Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Vladimir Zapolskiy]

Hi Kamil,

I'll just answer to your question, all the comments from you are accepted,
please send a new version with the minor fixes, hopefully the change will
be included into v4.15-rc.

On 10/24/2017 02:27 PM, Kamil Konieczny wrote:
> Hi Vladimir,
> 
> Thank you for review, I will apply almost all of your remarks,
> see answers below.
> 
> On 22.10.2017 12:18, Vladimir Zapolskiy wrote:
>> Hi Kamil,
>>
>> thank you for updates, I have just a few more comments.
>>

[snip]

>>> +/**
>>> + * s5p_hash_import - import hash state
>>> + * @req:   AHASH request
>>> + * @in:buffer with state to be imported from
>>> + */
>>> +static int s5p_hash_import(struct ahash_request *req, const void *in)
>>> +{
>>> +   struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
>>> +   struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>>> +   struct s5p_hash_ctx *tctx = crypto_ahash_ctx(tfm);
>>> +   const struct s5p_hash_reqctx *ctx_in = in;
>>> +
>>> +   memcpy(ctx, in, sizeof(*ctx) + BUFLEN);
>>> +   if (ctx_in->bufcnt < 0 || ctx_in->bufcnt > BUFLEN) {
>>
>> Now "ctx_in->bufcnt < 0" condition can be removed, moreover it
>> will eliminate a warning reproted by the compiler:
>>
>> drivers/crypto/s5p-sss.c:1748:21: warning: comparison of unsigned expression 
>> < 0 is always false [-Wtype-limits]
>>   if (ctx_in->bufcnt < 0 || ctx_in->bufcnt > BUFLEN) {
>>  ^
> 
> You are right, I will drop first condition. BTW what compiler options are you 
> using ?
> This one was not reported by my compilation process.
> 

Regularly I specify 'make C=1 W=1' options to build a kernel with changes,
some of the new reported warnings are false positives, but in general it
makes sense to check the output to catch potential issues like this one.

--
With best wishes,
Vladimir

Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Kamil Konieczny]

Hi Vladimir,

Thank you for review, I will apply almost all of your remarks,
see answers below.

On 22.10.2017 12:18, Vladimir Zapolskiy wrote:
> Hi Kamil,
> 
> thank you for updates, I have just a few more comments.
> 
> On 10/17/2017 02:28 PM, Kamil Konieczny wrote:
>> [...]
>> - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
>>   as they are nedded for fallback.
> 
> Typo, s/nedded/needed/

ok, Thank you for spotting this.

> [snip]
> 
>>  
>>  #include 
>>  #include 
>> +#include 
> 
> I can not find which interfaces from linux/delay.h are needed.
> I suppose it should not be added.
> 
> [snip]

Yes, you are right, I will delete this 'include delay.h'

>> -static struct s5p_aes_dev *s5p_dev;
>> +/**
>> + * struct s5p_hash_reqctx - HASH request context
>> + * @dev:Associated device
> 
> dev or dd?

dd

>> + * @op_update:  Current request operation (OP_UPDATE or OP_FINAL)
>> + * @digcnt: Number of bytes processed by HW (without buffer[] ones)
>> + * @digest: Digest message or IV for partial result
>> + * @nregs:  Number of HW registers for digest or IV read/write
>> + * @engine: Bits for selecting type of HASH in SSS block
>> + * @sg: sg for DMA transfer
>> + * @sg_len: Length of sg for DMA transfer
>> + * @sgl[]:  sg for joining buffer and req->src scatterlist
>> + * @skip:   Skip offset in req->src for current op
>> + * @total:  Total number of bytes for current request
>> + * @finup:  Keep state for finup or final.
>> + * @error:  Keep track of error.
>> + * @bufcnt: Number of bytes holded in buffer[]
>> + * @buffer[]:   For byte(s) from end of req->src in UPDATE op
>> + */
>> +struct s5p_hash_reqctx {
>> +struct s5p_aes_dev  *dd;
>> +boolop_update;
>> +
> 
> [snip]
> 
>> +
>> +/**
>> + * s5p_hash_shash_digest() - calculate shash digest
>> + * @tfm:crypto transformation
>> + * @flags:  tfm flags
>> + * @data:   input data
>> + * @len:length of data
>> + * @out:output buffer
>> + */
>> +static int s5p_hash_shash_digest(struct crypto_shash *tfm, u32 flags,
>> + const u8 *data, unsigned int len, u8 *out)
>> +{
>> +SHASH_DESC_ON_STACK(shash, tfm);
> 
> Just for information, this line produces a spatch warning:
> 
>   drivers/crypto/s5p-sss.c:1534:9: warning: Variable length array is used.
> 
> I think it can be ignored.

I also think it can be ignored, it uses 104 bytes on stack in case of SHA256 ,
sizeof struct sha256_state for SW generic implementation, found in 
include/crypto/sha.h
 
>> +
>> +shash->tfm = tfm;
>> +shash->flags = flags & ~CRYPTO_TFM_REQ_MAY_SLEEP;
>> +
>> +return crypto_shash_digest(shash, data, len, out);
>> +}
>> +
> 
> [snip]
> 
>> +/**
>> + * s5p_hash_final() - close up hash and calculate digest
>> + * @req:AHASH request
>> + *
>> + * Note: in final req->src do not have any data, and req->nbytes can be
>> + * non-zero.
>> + *
>> + * If there were no input data processed yet and the buffered hash data is
>> + * less than BUFLEN (64) then calculate the final hash immediately by using
>> + * SW algorithm fallback.
>> + *
>> + * Otherwise enqueues the current AHASH request with OP_FINAL operation op
>> + * and finalize hash message in HW. Note that if digcnt!=0 then there were
>> + * previous update op, so there are always some buffered bytes in 
>> ctx->buffer,
>> + * which means that ctx->bufcnt!=0
>> + *
>> + * Returns:
>> + * 0 if the request has been processed immediately,
>> + * -EINPROGRESS if the operation has been queued for later execution or is 
>> set
>> + *  to processing by HW,
>> + * -EBUSY if queue is full and request should be resubmitted later, other
>> + *  negative values on error.
> 
> Do you want to add -EINVAL into the list also?

Here I made bad formatting, it should read:

* -EBUSY if queue is full and request should be resubmitted later,
* other negative values on error.

I do not want to specify explicitly all error negative codes here, as it also 
uses
s5p_hash_enqueue and these return codes are referenced from other places. I 
intended
to listing success values, 0, -EINPROGRESS, then explaining -EBUSY, then adding 
all
other negative as error. The other values can be -EINVAL, -ENOMEM or maybe 
other, when
this module gets extended to HMAC implementation.

>> + */
>> +static int s5p_hash_final(struct ahash_request *req)
>> +{
>> +struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
>> +
>> +ctx->finup = true;
>> +if (ctx->error)
>> +return -EINVAL; /* uncompleted hash is not needed */
>> +
>> +if (!ctx->digcnt && ctx->bufcnt < BUFLEN)
>> +return s5p_hash_final_shash(req);
>> +
>> +return s5p_hash_enqueue(req, false); /* HASH_OP_FINAL */
>> +}
>> +
> 
> [snip]
> 
>> +/**
>> + * s5p_hash_finup() - process last req->src and calculate digest
>> + * @req:AHASH request containing the last update data
>> + *
>> + * Return values: see s5p_hash_final above.
>> + */
>> +static 

Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Vladimir Zapolskiy]

Hi Kamil,

thank you for updates, I have just a few more comments.

On 10/17/2017 02:28 PM, Kamil Konieczny wrote:
> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
> It uses the crypto framework asynchronous hash api.
> It is based on omap-sham.c driver.
> S5P has some HW differencies and is not implemented.
> 
> Modifications in s5p-sss:
> 
> - Add hash supporting structures and functions.
> 
> - Modify irq handler to handle both aes and hash signals.
> 
> - Resize resource end in probe if EXYNOS_HASH is enabled in
>   Kconfig.
> 
> - Add new copyright line and new author.
> 
> - Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6
>   with crypto run-time self test testmgr
>   and with tcrypt module with: modprobe tcrypt sec=1 mode=N
>   where N=402, 403, 404 (MD5, SHA1, SHA256).
> 
> Modifications in drivers/crypto/Kconfig:
> 
> - Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG
>   and CRYPTO_DEV_S5P
> 
> - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
>   as they are nedded for fallback.

Typo, s/nedded/needed/

[snip]

>  
>  #include 
>  #include 
> +#include 

I can not find which interfaces from linux/delay.h are needed.
I suppose it should not be added.

[snip]

> -static struct s5p_aes_dev *s5p_dev;
> +/**
> + * struct s5p_hash_reqctx - HASH request context
> + * @dev: Associated device

dev or dd?

> + * @op_update:   Current request operation (OP_UPDATE or OP_FINAL)
> + * @digcnt:  Number of bytes processed by HW (without buffer[] ones)
> + * @digest:  Digest message or IV for partial result
> + * @nregs:   Number of HW registers for digest or IV read/write
> + * @engine:  Bits for selecting type of HASH in SSS block
> + * @sg:  sg for DMA transfer
> + * @sg_len:  Length of sg for DMA transfer
> + * @sgl[]:   sg for joining buffer and req->src scatterlist
> + * @skip:Skip offset in req->src for current op
> + * @total:   Total number of bytes for current request
> + * @finup:   Keep state for finup or final.
> + * @error:   Keep track of error.
> + * @bufcnt:  Number of bytes holded in buffer[]
> + * @buffer[]:For byte(s) from end of req->src in UPDATE op
> + */
> +struct s5p_hash_reqctx {
> + struct s5p_aes_dev  *dd;
> + boolop_update;
> +
> + u64 digcnt;
> + u8  digest[SHA256_DIGEST_SIZE];
> +
> + unsigned intnregs; /* digest_size / sizeof(reg) */
> + u32 engine;
> +
> + struct scatterlist  *sg;
> + unsigned intsg_len;
> + struct scatterlist  sgl[2];
> + unsigned intskip;
> + unsigned inttotal;
> + boolfinup;
> + boolerror;
> +
> + u32 bufcnt;
> + u8  buffer[0];
> +};

[snip]

> +
> +/**
> + * s5p_hash_shash_digest() - calculate shash digest
> + * @tfm: crypto transformation
> + * @flags:   tfm flags
> + * @data:input data
> + * @len: length of data
> + * @out: output buffer
> + */
> +static int s5p_hash_shash_digest(struct crypto_shash *tfm, u32 flags,
> +  const u8 *data, unsigned int len, u8 *out)
> +{
> + SHASH_DESC_ON_STACK(shash, tfm);

Just for information, this line produces a spatch warning:

  drivers/crypto/s5p-sss.c:1534:9: warning: Variable length array is used.

I think it can be ignored.

> +
> + shash->tfm = tfm;
> + shash->flags = flags & ~CRYPTO_TFM_REQ_MAY_SLEEP;
> +
> + return crypto_shash_digest(shash, data, len, out);
> +}
> +

[snip]

> +/**
> + * s5p_hash_final() - close up hash and calculate digest
> + * @req: AHASH request
> + *
> + * Note: in final req->src do not have any data, and req->nbytes can be
> + * non-zero.
> + *
> + * If there were no input data processed yet and the buffered hash data is
> + * less than BUFLEN (64) then calculate the final hash immediately by using
> + * SW algorithm fallback.
> + *
> + * Otherwise enqueues the current AHASH request with OP_FINAL operation op
> + * and finalize hash message in HW. Note that if digcnt!=0 then there were
> + * previous update op, so there are always some buffered bytes in 
> ctx->buffer,
> + * which means that ctx->bufcnt!=0
> + *
> + * Returns:
> + * 0 if the request has been processed immediately,
> + * -EINPROGRESS if the operation has been queued for later execution or is 
> set
> + *   to processing by HW,
> + * -EBUSY if queue is full and request should be resubmitted later, other
> + *   negative values on error.

Do you want to add -EINVAL into the list also?

> + */
> +static int s5p_hash_final(struct ahash_request *req)
> +{
> + struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
> +
> + ctx->finup = true;
> + if (ctx->error)
> + return -EINVAL; /* uncompleted hash is not needed */
> +
> + if (!ctx->digcnt && ctx->bufcnt < BUFLEN)
> + 

Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Krzysztof Kozlowski]

On Tue, Oct 17, 2017 at 1:28 PM, Kamil Konieczny
 wrote:
> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
> It uses the crypto framework asynchronous hash api.
> It is based on omap-sham.c driver.
> S5P has some HW differencies and is not implemented.
>
> Modifications in s5p-sss:
>
> - Add hash supporting structures and functions.
>
> - Modify irq handler to handle both aes and hash signals.
>
> - Resize resource end in probe if EXYNOS_HASH is enabled in
>   Kconfig.
>
> - Add new copyright line and new author.
>
> - Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6
>   with crypto run-time self test testmgr
>   and with tcrypt module with: modprobe tcrypt sec=1 mode=N
>   where N=402, 403, 404 (MD5, SHA1, SHA256).
>
> Modifications in drivers/crypto/Kconfig:
>
> - Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG
>   and CRYPTO_DEV_S5P
>
> - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
>   as they are nedded for fallback.
>
> Signed-off-by: Kamil Konieczny 
> ---
>  drivers/crypto/Kconfig   |   14 +
>  drivers/crypto/s5p-sss.c | 1407 
> +-
>  2 files changed, 1411 insertions(+), 10 deletions(-)


Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

[PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Kamil Konieczny]

Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
It uses the crypto framework asynchronous hash api.
It is based on omap-sham.c driver.
S5P has some HW differencies and is not implemented.

Modifications in s5p-sss:

- Add hash supporting structures and functions.

- Modify irq handler to handle both aes and hash signals.

- Resize resource end in probe if EXYNOS_HASH is enabled in
  Kconfig.

- Add new copyright line and new author.

- Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6
  with crypto run-time self test testmgr
  and with tcrypt module with: modprobe tcrypt sec=1 mode=N
  where N=402, 403, 404 (MD5, SHA1, SHA256).

Modifications in drivers/crypto/Kconfig:

- Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG
  and CRYPTO_DEV_S5P

- Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
  as they are nedded for fallback.

Signed-off-by: Kamil Konieczny 
---
 drivers/crypto/Kconfig   |   14 +
 drivers/crypto/s5p-sss.c | 1407 +-
 2 files changed, 1411 insertions(+), 10 deletions(-)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4b75084fabad..dea4d33d9c7f 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -439,6 +439,20 @@ config CRYPTO_DEV_S5P
  Select this to offload Samsung S5PV210 or S5PC110, Exynos from AES
  algorithms execution.
 
+config CRYPTO_DEV_EXYNOS_HASH
+   bool "Support for Samsung Exynos HASH accelerator"
+   depends on CRYPTO_DEV_S5P
+   depends on !CRYPTO_DEV_EXYNOS_RNG && CRYPTO_DEV_EXYNOS_RNG!=m
+   select CRYPTO_SHA1
+   select CRYPTO_MD5
+   select CRYPTO_SHA256
+   help
+ Select this to offload Exynos from HASH MD5/SHA1/SHA256.
+ This will select software SHA1, MD5 and SHA256 as they are
+ needed for small and zero-size messages.
+ HASH algorithms will be disabled if EXYNOS_RNG
+ is enabled due to hw conflict.
+
 config CRYPTO_DEV_NX
bool "Support for IBM PowerPC Nest (NX) cryptographic acceleration"
depends on PPC64
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index dfae1865c384..c9feccf21efe 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -1,18 +1,21 @@
 /*
  * Cryptographic API.
  *
- * Support for Samsung S5PV210 HW acceleration.
+ * Support for Samsung S5PV210 and Exynos HW acceleration.
  *
  * Copyright (C) 2011 NetUP Inc. All rights reserved.
+ * Copyright (c) 2017 Samsung Electronics Co., Ltd. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as published
  * by the Free Software Foundation.
  *
+ * Hash part based on omap-sham.c driver.
  */
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -30,28 +33,41 @@
 #include 
 #include 
 
+#include 
+#include 
+#include 
+#include 
+
 #define _SBF(s, v) ((v) << (s))
 
 /* Feed control registers */
 #define SSS_REG_FCINTSTAT  0x
+#define SSS_FCINTSTAT_HPARTINT BIT(7)
+#define SSS_FCINTSTAT_HDONEINT BIT(5)
 #define SSS_FCINTSTAT_BRDMAINT BIT(3)
 #define SSS_FCINTSTAT_BTDMAINT BIT(2)
 #define SSS_FCINTSTAT_HRDMAINT BIT(1)
 #define SSS_FCINTSTAT_PKDMAINT BIT(0)
 
 #define SSS_REG_FCINTENSET 0x0004
+#define SSS_FCINTENSET_HPARTINTENSET   BIT(7)
+#define SSS_FCINTENSET_HDONEINTENSET   BIT(5)
 #define SSS_FCINTENSET_BRDMAINTENSET   BIT(3)
 #define SSS_FCINTENSET_BTDMAINTENSET   BIT(2)
 #define SSS_FCINTENSET_HRDMAINTENSET   BIT(1)
 #define SSS_FCINTENSET_PKDMAINTENSET   BIT(0)
 
 #define SSS_REG_FCINTENCLR 0x0008
+#define SSS_FCINTENCLR_HPARTINTENCLR   BIT(7)
+#define SSS_FCINTENCLR_HDONEINTENCLR   BIT(5)
 #define SSS_FCINTENCLR_BRDMAINTENCLR   BIT(3)
 #define SSS_FCINTENCLR_BTDMAINTENCLR   BIT(2)
 #define SSS_FCINTENCLR_HRDMAINTENCLR   BIT(1)
 #define SSS_FCINTENCLR_PKDMAINTENCLR   BIT(0)
 
 #define SSS_REG_FCINTPEND  0x000C
+#define SSS_FCINTPEND_HPARTINTPBIT(7)
+#define SSS_FCINTPEND_HDONEINTPBIT(5)
 #define SSS_FCINTPEND_BRDMAINTPBIT(3)
 #define SSS_FCINTPEND_BTDMAINTPBIT(2)
 #define SSS_FCINTPEND_HRDMAINTPBIT(1)
@@ -72,6 +88,7 @@
 #define SSS_HASHIN_INDEPENDENT _SBF(0, 0x00)
 #define SSS_HASHIN_CIPHER_INPUT_SBF(0, 0x01)
 #define SSS_HASHIN_CIPHER_OUTPUT   _SBF(0, 0x02)
+#define SSS_HASHIN_MASK_SBF(0, 0x03)
 
 #define SSS_REG_FCBRDMAS   0x0020
 #define SSS_REG_FCBRDMAL   0x0024
@@ -146,9 +163,80 @@
 #define AES_KEY_LEN16
 #define CRYPTO_QUEUE_LEN   1
 
+/* HASH registers */
+#define SSS_REG_HASH_CTRL  0x00
+
+#define SSS_HASH_USER_IV_ENBIT(5)
+#define SSS_HASH_INIT_BIT