[U-Boot] crypto/fsl - Add progressive hashing support using hardware acceleration.

Currently only normal hashing is supported using hardware acceleration.
Added support for progressinve hashing using h/w.

Signed-off-by: Ruchika Gupta <ruchika.gupta@freescale.com>
Signed-off-by: Gaurav Rana <gaurav.rana@freescale.com>
CC: Simon Glass <sjg@chromium.org>
---
This patch is dependent on following series of 10 patches.
https://patchwork.ozlabs.org/patch/432126/
.
.
.


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

Hi,

On 28 January 2015 at 03:51, Gaurav Rana <gaurav.rana@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@freescale.com>
> Signed-off-by: Gaurav Rana <gaurav.rana@freescale.com>
> CC: Simon Glass <sjg@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.

> +       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()?

> + * @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.

> +
> +       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?

> +#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?

> +
> +       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?

>  #endif
> --
> 1.8.1.4

Regards,
Simon

Hi Simon,

Thanks for the review comments.

> -----Original Message-----
> From: sjg@google.com [mailto:sjg@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@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@freescale.com>
> > Signed-off-by: Gaurav Rana <gaurav.rana@freescale.com>
> > CC: Simon Glass <sjg@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

Hi Ruchika,

On 5 February 2015 at 04:20, Ruchika Gupta <ruchika.gupta@freescale.com> wrote:
> Hi Simon,
>
[snip]

>> > 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.

OK I see. It seems wrong, but maybe I misunderstand it. Anyway as you
say it has nothing to do with your patch.

> /*
>          * 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 ?
>

No let's leave it.

[snip]

Regards,
Simon