Hi Simon, Thanks for the review comments.
> -----Original Message----- > From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass > Sent: Thursday, February 05, 2015 8:55 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] crypto/fsl - Add progressive hashing support using > hardware acceleration. > > Hi, > > On 28 January 2015 at 03:51, Gaurav Rana <gaurav.r...@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.gu...@freescale.com> > > Signed-off-by: Gaurav Rana <gaurav.r...@freescale.com> > > CC: Simon Glass <s...@chromium.org> > > --- > > This patch is dependent on following series of 10 patches. > > https://patchwork.ozlabs.org/patch/432126/ > > . > > . > > Now applied to mainline. > > > . > > > > > > https://patchwork.ozlabs.org/patch/432135/ > > README | 4 ++ > > common/hash.c | 10 +++ > > drivers/crypto/fsl/fsl_hash.c | 141 > > ++++++++++++++++++++++++++++++++++++++++++ > > drivers/crypto/fsl/fsl_hash.h | 32 ++++++++++ > > include/fsl_sec.h | 30 +++++++++ > > include/hw_sha.h | 77 +++++++++++++++++++++++ > > 6 files changed, 294 insertions(+) > > create mode 100644 drivers/crypto/fsl/fsl_hash.h > > > > diff --git a/README b/README > > index cac7978..98aa31f 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 hw > > s/hw/hardware/ > > > + acceleration > > + CONFIG_SHA_PROG_HW_ACCEL - support SHA1 and SHA256 > progressive > > + hashing using hw acceleration > > > > 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..d4becd3 > > 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_sha1_init, > > + hw_sha1_update, > > + hw_sha1_finish, > > +#endif > > }, { > > "sha256", > > SHA256_SUM_LEN, > > hw_sha256, > > CHUNKSZ_SHA256, > > +#ifdef CONFIG_SHA_PROG_HW_ACCEL > > + hw_sha256_init, > > + hw_sha256_update, > > + hw_sha256_finish, > > +#endif > > }, > > #endif > > #ifdef CONFIG_SHA1 > > diff --git a/drivers/crypto/fsl/fsl_hash.c > > b/drivers/crypto/fsl/fsl_hash.c index d77f257..1681705 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,111 @@ 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, -1 on error > > + */ > > +static int caam_hash_init(void **ctxp, enum caam_hash_algos > > +caam_algo) { > > + struct sha_ctx *ctx = malloc(sizeof(struct sha_ctx)); > > Please check return value and return -ENOMEM. Also you can use > calloc() to zero it. Ok > > > + memset(ctx, 0, sizeof(struct sha_ctx)); > > + *ctxp = ctx; > > + 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 > > Shouldn't this be handled in finish()? The interface as defined in hash.h has is_last in the hash_update function. We have defined this function on similar line. Already existing function pointer as available in include/hash.h is pasted below for reference. /* * hash_update: Perform hashing on the given buffer * * 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, -1 on error */ int (*hash_update)(struct hash_algo *algo, void *ctx, const void *buf, unsigned int size, int is_last); Are you suggesting that we change the above function pointer in include/hash.h also ? > > > + * @caam_algo: Enum for SHA1 or SHA256 > > + * @return 0 if ok, -1 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 = (struct sha_ctx *)hash_ctx; > > I don't think you need the cast. Ok > > > + > > + if (ctx->sg_num >= MAX_SG) { > > + free(ctx); > > + return -1; > > + } > > + > > +#ifdef CONFIG_PHYS_64BIT > > + ctx->sg_tbl[ctx->sg_num].addr_hi = addr >> 32; > > Is this needed, or will addr >> 32 be 0 in this case anyway? Our platforms support > 32 bit physical address, There this will be required. > > > +#else > > + ctx->sg_tbl[ctx->sg_num].addr_hi = 0x0; #endif > > + 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); > > + } > > Can you move this block to the finish() method? To move this , we have made it compatible to function pointers already defined in include/hash.h as explained above. Should we change those function pointer definition too. > > > + > > + 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, -1 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 = (struct sha_ctx *)hash_ctx; > > + int i = 0, ret = 0; > > + > > + if (size < driver_hash[caam_algo].digestsize) { > > + free(ctx); > > + return -1; > > -ENOSPC > > > + } > > + > > + for (i = 0; i < ctx->sg_num; i++) > > + len += (sec_in32(&ctx->sg_tbl[i].len_flag) & > > Double space > > {} around this I think > > > + 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) { @@ > > -69,9 +176,43 @@ void hw_sha256(const unsigned char *pbuf, unsigned int > buf_len, > > printf("CAAM was not setup properly or it is > > faulty\n"); } > > > > +int hw_sha256_init(struct hash_algo *algo, void **ctxp) { > > + return caam_hash_init(ctxp, SHA256); } > > + > > +int hw_sha256_update(struct hash_algo *algo, void *ctx, const void *buf, > > + unsigned int size, int is_last) { > > + return caam_hash_update(ctx, buf, size, is_last, SHA256); } > > + > > +int hw_sha256_finish(struct hash_algo *algo, void *ctx, void *dest_buf, > > + int size) > > +{ > > + return caam_hash_finish(ctx, dest_buf, size, SHA256); } > > + > > void hw_sha1(const unsigned char *pbuf, unsigned int buf_len, > > unsigned char *pout, unsigned int chunk_size) > > { > > if (caam_hash(pbuf, buf_len, pout, SHA1)) > > printf("CAAM was not setup properly or it is > > faulty\n"); } > > + > > +int hw_sha1_init(struct hash_algo *algo, void **ctxp) { > > + return caam_hash_init(ctxp, SHA1); } > > + > > +int hw_sha1_update(struct hash_algo *algo, void *ctx, const void *buf, > > + unsigned int size, int is_last) { > > + return caam_hash_update(ctx, buf, size, is_last, SHA1); } > > + > > +int hw_sha1_finish(struct hash_algo *algo, void *ctx, void *dest_buf, > > + int size) > > +{ > > + return caam_hash_finish(ctx, dest_buf, size, SHA1); } > > diff --git a/drivers/crypto/fsl/fsl_hash.h > > b/drivers/crypto/fsl/fsl_hash.h new file mode 100644 index > > 0000000..7f07ea4 > > --- /dev/null > > +++ b/drivers/crypto/fsl/fsl_hash.h > > @@ -0,0 +1,32 @@ > > +/* > > + * Copyright 2014 Freescale Semiconductor, Inc. > > + * > > + * SPDX-License-Identifier: GPL-2.0+ > > + * > > + */ > > + > > +#ifndef _SHA_H > > +#define _SHA_H > > + > > +#include <fsl_sec.h> > > +#include "jr.h" > > + > > +#define MAX_SG 16 > > + > > +/* > > + * Hash context contain the following fields > > contains > > > + * Sha Descriptor > > Please can you use the standard format? > > @sha_desc: SHA descriptor > @sg_num: number of entries in sg table > etc. > > > + * number of entries in sg table > > + * total length of buffer > > + * sg entry table > > + * 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]; > > Do you need to include <hash.h> in this header to get this symbol? > > > +}; > > + > > +#endif > > diff --git a/include/fsl_sec.h b/include/fsl_sec.h index > > aa850a3..cece820 100644 > > --- a/include/fsl_sec.h > > +++ b/include/fsl_sec.h > > @@ -175,6 +175,36 @@ 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 the start of the > > + * buffer - hi > > + */ > > Please put this comment on one line before the addr_hi parameter. Or use the > @addr_hi structure-commenting approach before the structure. > But don't split comments like this. > > > + uint16_t reserved_zero; > > +#else > > + uint16_t reserved_zero; > > + uint16_t addr_hi; /* Memory Address of the start of the > > + * buffer - hi > > + */ > > here too. > > > + 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..6b040d2 100644 > > --- a/include/hw_sha.h > > +++ b/include/hw_sha.h > > @@ -22,6 +22,44 @@ > > void hw_sha256(const uchar * in_addr, uint buflen, > > uchar * out_addr, uint chunk_size); > > > > +/* > > + * Create the context for sha256 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, -1 on error > > + */ > > +int hw_sha256_init(struct hash_algo *algo, void **ctxp); > > + > > +/* > > + * Update buffer for sha256 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, -1 on error > > -ve on error, please fix globally. > > > > + */ > > +int hw_sha256_update(struct hash_algo *algo, void *ctx, const void *buf, > > + unsigned int size, int is_last); > > + > > +/* > > + * Copy sha256 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, -1 on error > > + */ > > +int hw_sha256_finish(struct hash_algo *algo, void *ctx, void *dest_buf, > > + int size); > > + > > /** > > * Computes hash value of input pbuf using h/w acceleration > > * > > @@ -34,4 +72,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 sha1 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, -1 on error > > + */ > > +int hw_sha1_init(struct hash_algo *algo, void **ctxp); > > + > > +/* > > + * Update buffer for sha1 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, -1 on error > > + */ > > +int hw_sha1_update(struct hash_algo *algo, void *ctx, const void *buf, > > + unsigned int size, int is_last); > > + > > +/* > > + * Copy sha1 hash result at destination location > > + * > > + * The context is freed after completion of hash operation. > > + * > > + * @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, -1 on error > > + */ > > +int hw_sha1_finish(struct hash_algo *algo, void *ctx, void *dest_buf, > > + int size); > > + > > The duplication is grim, but maybe we can move this to driver model later. > > But still, since you are passed in the algo, why don't you have common > functions for both SHA1 and SHA256? Ok...we will add common functions > > > #endif > > -- > > 1.8.1.4 > > Regards, > Simon Regards, Ruchika _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot