Hi,
On 6 February 2015 at 05:02, [email protected]
<[email protected]> wrote:
> Thanks for your valuable comments.
>
> Thanks and Regards,
> Gaurav Kumar Rana
>
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] 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 <[email protected]> wrote:
>> Currently only normal hashing is supported using hardware acceleration.
>> Added support for progressinve hashing using h/w.
>>
>> Signed-off-by: Ruchika Gupta <[email protected]>
>> Signed-off-by: Gaurav Rana <[email protected]>
>> CC: Simon Glass <[email protected]>
>> ---
>> 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
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
