Hi,
On 6 February 2015 at 05:02, gaurav.rana(a)freescale.com
<gaurav.rana(a)freescale.com> wrote:
> Thanks for your valuable comments.
>
> Thanks and Regards,
> Gaurav Kumar Rana
>
> -----Original Message-----
> From: sjg(a)google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
> Sent: Friday, February 06, 2015 11:15 AM
> To: Rana Gaurav-B46163
> Cc: U-Boot Mailing List; Sun York-R58495; Wood Scott-B07421; Gupta Ruchika-R66431; Bansal Aneesh-B39320
> Subject: Re: [PATCH][v2] crypto/fsl - Add progressive hashing support using hardware acceleration.
>
> Hi,
>
> On 5 February 2015 at 07:29, Gaurav Rana <gaurav.rana(a)freescale.com> wrote:
>> Currently only normal hashing is supported using hardware acceleration.
>> Added support for progressinve hashing using h/w.
>>
>> Signed-off-by: Ruchika Gupta <ruchika.gupta(a)freescale.com>
>> Signed-off-by: Gaurav Rana <gaurav.rana(a)freescale.com>
>> CC: Simon Glass <sjg(a)chromium.org>
>> ---
>> Changes in v2:
>> Merge to common functions for SHA1 and SHA256.
>> Incorporate comments.
>
> Normally it is a good idea to list the comments you addressed.
>
> This patch looks good to me - I just have some nits below.
>
>>
>> README | 4 ++
>> common/hash.c | 10 +++
>> drivers/crypto/fsl/fsl_hash.c | 137
>> +++++++++++++++++++++++++++++++++++++++++-
>> drivers/crypto/fsl/fsl_hash.h | 33 ++++++++++
>> include/fsl_sec.h | 26 ++++++++
>> include/hw_sha.h | 39 ++++++++++++
>> 6 files changed, 248 insertions(+), 1 deletion(-) create mode 100644
>> drivers/crypto/fsl/fsl_hash.h
>>
>> diff --git a/README b/README
>> index cac7978..4c72baa 100644
>> --- a/README
>> +++ b/README
>> @@ -3151,6 +3151,10 @@ CBFS (Coreboot Filesystem) support
>>
>> CONFIG_SHA1 - support SHA1 hashing
>> CONFIG_SHA256 - support SHA256 hashing
>> + CONFIG_SHA_HW_ACCEL - support SHA1 and SHA256 hashing using
>> + hardware acceleration
>> + CONFIG_SHA_PROG_HW_ACCEL - support SHA1 and SHA256 progressive
>> + hashing using hw acceleration
>
> Can these go in Kconfig?
> [GAURAV KUMAR RANA] As hash.o gets build in common/ dir so should we define these Kconfigs in common/Kconfig?
> Or can you suggest us some location and we can take up this thing in next patch.
In order to enable these options (without using board config files
which is deprecated) you should add them to Kconfig somewhere. I
suspect that common/Kconfig is the right place, yes.
Best to do it in this patch and make sure you add sufficient 'help' in
the option.
>
>
>>
>> Note: There is also a sha1sum command, which should perhaps
>> be deprecated in favour of 'hash sha1'.
>> diff --git a/common/hash.c b/common/hash.c index d154d02..9e9f84b
>> 100644
>> --- a/common/hash.c
>> +++ b/common/hash.c
>> @@ -127,11 +127,21 @@ static struct hash_algo hash_algo[] = {
>> SHA1_SUM_LEN,
>> hw_sha1,
>> CHUNKSZ_SHA1,
>> +#ifdef CONFIG_SHA_PROG_HW_ACCEL
>> + hw_sha_init,
>> + hw_sha_update,
>> + hw_sha_finish,
>> +#endif
>> }, {
>> "sha256",
>> SHA256_SUM_LEN,
>> hw_sha256,
>> CHUNKSZ_SHA256,
>> +#ifdef CONFIG_SHA_PROG_HW_ACCEL
>> + hw_sha_init,
>> + hw_sha_update,
>> + hw_sha_finish,
>> +#endif
>> },
>> #endif
>> #ifdef CONFIG_SHA1
>> diff --git a/drivers/crypto/fsl/fsl_hash.c
>> b/drivers/crypto/fsl/fsl_hash.c index d77f257..65c35d7 100644
>> --- a/drivers/crypto/fsl/fsl_hash.c
>> +++ b/drivers/crypto/fsl/fsl_hash.c
>> @@ -10,6 +10,8 @@
>> #include "jobdesc.h"
>> #include "desc.h"
>> #include "jr.h"
>> +#include "fsl_hash.h"
>> +#include <hw_sha.h>
>>
>> #define CRYPTO_MAX_ALG_NAME 80
>> #define SHA1_DIGEST_SIZE 20
>> @@ -39,6 +41,113 @@ static struct caam_hash_template driver_hash[] = {
>> },
>> };
>>
>> +/* Create the context for progressive hashing using h/w acceleration.
>> + *
>> + * @ctxp: Pointer to the pointer of the context for hashing
>> + * @caam_algo: Enum for SHA1 or SHA256
>> + * @return 0 if ok, -ENOMEM on error
>> + */
>> +static int caam_hash_init(void **ctxp, enum caam_hash_algos
>> +caam_algo) {
>> + *ctxp = calloc(1, sizeof(struct sha_ctx));
>> + if (*ctxp == NULL) {
>> + debug("Cannot allocate memory for context\n");
>> + return -ENOMEM;
>> + }
>> + return 0;
>> +}
>> +
>> +/*
>> + * Update sg table for progressive hashing using h/w acceleration
>> + *
>> + * The context is freed by this function if an error occurs.
>> + *
>> + * @hash_ctx: Pointer to the context for hashing
>> + * @buf: Pointer to the buffer being hashed
>> + * @size: Size of the buffer being hashed
>> + * @is_last: 1 if this is the last update; 0 otherwise
>> + * @caam_algo: Enum for SHA1 or SHA256
>> + * @return 0 if ok, -EINVAL on error
>> + */
>> +static int caam_hash_update(void *hash_ctx, const void *buf,
>> + unsigned int size, int is_last,
>> + enum caam_hash_algos caam_algo) {
>> + uint32_t final = 0;
>> + dma_addr_t addr = virt_to_phys((void *)buf);
>> + struct sha_ctx *ctx = hash_ctx;
>> +
>> + if (ctx->sg_num >= MAX_SG) {
>> + free(ctx);
>> + return -EINVAL;
>> + }
>> +
>> +#ifdef CONFIG_PHYS_64BIT
>> + ctx->sg_tbl[ctx->sg_num].addr_hi = addr >> 32; #else
>> + ctx->sg_tbl[ctx->sg_num].addr_hi = 0x0; #endif
>
> To clarify my comment on this. If we are on a 32-bit machine, will not addr >> 32 be zero anyway? If so, can we avoid this #ifdef and just have the first line? Or does the type change?
> [GAURAV KUMAR RANA] Yes with CONFIG_PHYS_64BIT defined type of dma_addr_t gets modified to unsigned long long.
OK I see, that's fine.
>
>> + ctx->sg_tbl[ctx->sg_num].addr_lo = addr;
>> +
>> + sec_out32(&ctx->sg_tbl[ctx->sg_num].len_flag,
>> + (size & SG_ENTRY_LENGTH_MASK));
>> +
>> + ctx->sg_num++;
>> +
>> + if (is_last) {
>> + final = sec_in32(&ctx->sg_tbl[ctx->sg_num - 1].len_flag) |
>> + SG_ENTRY_FINAL_BIT;
>> + sec_out32(&ctx->sg_tbl[ctx->sg_num - 1].len_flag, final);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Perform progressive hashing on the given buffer and copy hash at
>> + * destination buffer
>> + *
>> + * The context is freed after completion of hash operation.
>> + *
>> + * @hash_ctx: Pointer to the context for hashing
>> + * @dest_buf: Pointer to the destination buffer where hash is to be
>> +copied
>> + * @size: Size of the buffer being hashed
>> + * @caam_algo: Enum for SHA1 or SHA256
>> + * @return 0 if ok, -EINVAL on error
>> + */
>> +static int caam_hash_finish(void *hash_ctx, void *dest_buf,
>> + int size, enum caam_hash_algos caam_algo)
>> +{
>> + uint32_t len = 0;
>> + struct sha_ctx *ctx = hash_ctx;
>> + int i = 0, ret = 0;
>> +
>> + if (size < driver_hash[caam_algo].digestsize) {
>> + free(ctx);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < ctx->sg_num; i++)
>> + len += (sec_in32(&ctx->sg_tbl[i].len_flag) &
>> + SG_ENTRY_LENGTH_MASK);
>> +
>> + inline_cnstr_jobdesc_hash(ctx->sha_desc, (uint8_t *)ctx->sg_tbl, len,
>> + ctx->hash,
>> + driver_hash[caam_algo].alg_type,
>> + driver_hash[caam_algo].digestsize,
>> + 1);
>> +
>> + ret = run_descriptor_jr(ctx->sha_desc);
>> +
>> + if (ret)
>> + debug("Error %x\n", ret);
>> + else
>> + memcpy(dest_buf, ctx->hash, sizeof(ctx->hash));
>> +
>> + free(ctx);
>> + return ret;
>> +}
>> +
>> int caam_hash(const unsigned char *pbuf, unsigned int buf_len,
>> unsigned char *pout, enum caam_hash_algos algo) { @@
>> -48,7 +157,7 @@ int caam_hash(const unsigned char *pbuf, unsigned int buf_len,
>> desc = malloc(sizeof(int) * MAX_CAAM_DESCSIZE);
>> if (!desc) {
>> debug("Not enough memory for descriptor allocation\n");
>> - return -1;
>> + return -ENOMEM;
>> }
>>
>> inline_cnstr_jobdesc_hash(desc, pbuf, buf_len, pout, @@ -75,3
>> +184,29 @@ void hw_sha1(const unsigned char *pbuf, unsigned int buf_len,
>> if (caam_hash(pbuf, buf_len, pout, SHA1))
>> printf("CAAM was not setup properly or it is
>> faulty\n"); }
>> +
>> +int hw_sha_init(struct hash_algo *algo, void **ctxp) {
>> + if (!strcmp(algo->name, driver_hash[SHA1].name))
>> + return caam_hash_init(ctxp, SHA1);
>> + else
>> + return caam_hash_init(ctxp, SHA256);
>
> Ick, please try to avoid duplicating code. You could have
>
> static enum caam_hash_algos algo_to_hash(struct hash_algo *algo) {
> return ...
> }
>
> and then have in hw_sha_init():
>
> return caam_hash_init(ctxp, algo_to_hash(algo))
>
> Similarly with the below:
> [GAURAV KUMAR RANA] Done
>
>> +}
>> +
>> +int hw_sha_update(struct hash_algo *algo, void *ctx, const void *buf,
>> + unsigned int size, int is_last) {
>> + if (!strcmp(algo->name, driver_hash[SHA1].name))
>> + return caam_hash_update(ctx, buf, size, is_last, SHA1);
>> + else
>> + return caam_hash_update(ctx, buf, size, is_last,
>> +SHA256); }
>> +
>> +int hw_sha_finish(struct hash_algo *algo, void *ctx, void *dest_buf,
>> + int size)
>> +{
>> + if (!strcmp(algo->name, driver_hash[SHA1].name))
>> + return caam_hash_finish(ctx, dest_buf, size, SHA1);
>> + else
>> + return caam_hash_finish(ctx, dest_buf, size, SHA256);
>> +}
>> diff --git a/drivers/crypto/fsl/fsl_hash.h
>> b/drivers/crypto/fsl/fsl_hash.h new file mode 100644 index
>> 0000000..4e840e6
>> --- /dev/null
>> +++ b/drivers/crypto/fsl/fsl_hash.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright 2014 Freescale Semiconductor, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + *
>> + */
>> +
>> +#ifndef _SHA_H
>> +#define _SHA_H
>> +
>> +#include <fsl_sec.h>
>> +#include <hash.h>
>> +#include "jr.h"
>> +
>> +#define MAX_SG 16
>
> I think this is max scatter gather so you should either make this longer and self-documenting or add a comment.
> [GAURAV KUMAR RANA] Done
How about:
/* We support at most 32 Scatter/Gather Entries */
>
>> +
>> +/*
>> + * Hash context contains the following fields
>> + * @sha_desc: Sha Descriptor
>> + * @sg_num: number of entries in sg table
>> + * @len: total length of buffer
>> + * @sg_tbl: sg entry table
>> + * @hash: index to the hash calculated */ struct sha_ctx {
>> + uint32_t sha_desc[64];
>> + uint32_t sg_num;
>> + uint32_t len;
>> + struct sg_entry sg_tbl[MAX_SG];
>> + u8 hash[HASH_MAX_DIGEST_SIZE]; };
>> +
>> +#endif
>> diff --git a/include/fsl_sec.h b/include/fsl_sec.h index
>> aa850a3..b6e6f04 100644
>> --- a/include/fsl_sec.h
>> +++ b/include/fsl_sec.h
>> @@ -175,6 +175,32 @@ struct jr_regs {
>> u32 jrcr;
>> };
>>
>> +/*
>> + * Scatter Gather Entry - Specifies the the Scatter Gather Format
>> + * related information
>> + */
>> +struct sg_entry {
>> +#ifdef CONFIG_SYS_FSL_SEC_LE
>> + uint32_t addr_lo; /* Memory Address - lo */
>> + uint16_t addr_hi; /* Memory Address of start of buffer - hi */
>> + uint16_t reserved_zero;
>> +#else
>> + uint16_t reserved_zero;
>> + uint16_t addr_hi; /* Memory Address of start of buffer - hi */
>> + uint32_t addr_lo; /* Memory Address - lo */
>> +#endif
>> +
>> + uint32_t len_flag; /* Length of the data in the frame */
>> +#define SG_ENTRY_LENGTH_MASK 0x3FFFFFFF
>> +#define SG_ENTRY_EXTENSION_BIT 0x80000000
>> +#define SG_ENTRY_FINAL_BIT 0x40000000
>> + uint32_t bpid_offset;
>> +#define SG_ENTRY_BPID_MASK 0x00FF0000
>> +#define SG_ENTRY_BPID_SHIFT 16
>> +#define SG_ENTRY_OFFSET_MASK 0x00001FFF
>> +#define SG_ENTRY_OFFSET_SHIFT 0
>> +};
>> +
>> int sec_init(void);
>> #endif
>>
>> diff --git a/include/hw_sha.h b/include/hw_sha.h index
>> 783350d..3d8213e 100644
>> --- a/include/hw_sha.h
>> +++ b/include/hw_sha.h
>> @@ -34,4 +34,43 @@ void hw_sha256(const uchar * in_addr, uint buflen,
>> */
>> void hw_sha1(const uchar * in_addr, uint buflen,
>> uchar * out_addr, uint chunk_size);
>> +
>> +/*
>> + * Create the context for sha progressive hashing using h/w
>> +acceleration
>> + *
>> + * @algo: Pointer to the hash_algo struct
>> + * @ctxp: Pointer to the pointer of the context for hashing
>> + * @return 0 if ok, -ve on error
>> + */
>> +int hw_sha_init(struct hash_algo *algo, void **ctxp);
>> +
>> +/*
>> + * Update buffer for sha progressive hashing using h/w acceleration
>> + *
>> + * The context is freed by this function if an error occurs.
>> + *
>> + * @algo: Pointer to the hash_algo struct
>> + * @ctx: Pointer to the context for hashing
>> + * @buf: Pointer to the buffer being hashed
>> + * @size: Size of the buffer being hashed
>> + * @is_last: 1 if this is the last update; 0 otherwise
>> + * @return 0 if ok, -ve on error
>> + */
>> +int hw_sha_update(struct hash_algo *algo, void *ctx, const void *buf,
>> + unsigned int size, int is_last);
>> +
>> +/*
>> + * Copy sha hash result at destination location
>> + *
>> + * The context is freed after completion of hash operation or after an error.
>> + *
>> + * @algo: Pointer to the hash_algo struct
>> + * @ctx: Pointer to the context for hashing
>> + * @dest_buf: Pointer to the destination buffer where hash is to be
>> +copied
>> + * @size: Size of the buffer being hashed
>> + * @return 0 if ok, -ve on error
>> + */
>> +int hw_sha_finish(struct hash_algo *algo, void *ctx, void *dest_buf,
>> + int size);
>> +
>> #endif
>> --
>> 1.8.1.4
Regards,
Simon
