[U-Boot] [PATCH] crypto/fsl - Add progressive hashing support using hardware acceleration.
Ruchika Gupta
ruchika.gupta at freescale.com
Thu Feb 5 12:20:12 CET 2015
Hi Simon,
Thanks for the review comments.
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at 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.rana at 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 at freescale.com>
> > Signed-off-by: Gaurav Rana <gaurav.rana at freescale.com>
> > CC: Simon Glass <sjg at 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
More information about the U-Boot
mailing list