[2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))
diff mbox series

Message ID 1562309364-942-3-git-send-email-pvanleeuwen@verimatrix.com
State Accepted
Delegated to: Herbert Xu
Headers show
Series
  • crypto: inside-secure - add more AEAD ciphersuites
Related show

Commit Message

Pascal van Leeuwen July 5, 2019, 6:49 a.m. UTC
Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c        |  15 +--
 drivers/crypto/inside-secure/safexcel.h        |  32 +----
 drivers/crypto/inside-secure/safexcel_cipher.c | 155 +++++++++++++++++++++----
 3 files changed, 138 insertions(+), 64 deletions(-)

Comments

Antoine Tenart July 26, 2019, 12:33 p.m. UTC | #1
Hi Pascal,

On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

Could you add a commit message?

> -		/* H/W capabilities selection */
> -		val = EIP197_FUNCTION_RSVD;
> -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> +		/* H/W capabilities selection: just enable everything */
> +		writel(EIP197_FUNCTION_ALL,
> +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));

This should be in a separate patch. I'm also not sure about it, as
controlling exactly what algs are enabled in the h/w could prevent
misconfiguration issues in the control descriptors.

> @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
>  				    u32 length)
> -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {

I think it's better for maintenance and readability to have something
like:

  if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
      ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)

> +struct safexcel_alg_template safexcel_alg_ctr_aes = {
> +	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,

Same comment as in patch 1 about the .engines member of the struct.

> +	.alg.skcipher = {
> +		.setkey = safexcel_skcipher_aesctr_setkey,
> +		.encrypt = safexcel_ctr_aes_encrypt,
> +		.decrypt = safexcel_ctr_aes_decrypt,
> +		/* Add 4 to include the 4 byte nonce! */
> +		.min_keysize = AES_MIN_KEY_SIZE + 4,
> +		.max_keysize = AES_MAX_KEY_SIZE + 4,

You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the
patch as well).

> +		.ivsize = 8,

And CTR_RFC3686_IV_SIZE here.

Thanks!
Antoine
Pascal Van Leeuwen July 26, 2019, 1:28 p.m. UTC | #2
Antoine,

> Hi Pascal,
> 
> On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> 
> Could you add a commit message?
> 
I can do that. Just didn't know it was really needed.

> > -		/* H/W capabilities selection */
> > -		val = EIP197_FUNCTION_RSVD;
> > -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> > -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> > -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> > -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> > -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> > -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> > -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> > -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> > -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > +		/* H/W capabilities selection: just enable everything */
> > +		writel(EIP197_FUNCTION_ALL,
> > +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> 
> This should be in a separate patch.
>
It was added specifically to  get this functionality working as CTR mode was not
enabled.  So I don't see why it should be a seperate patch, really?
Instead of adding CTR mode to the list (which would have been perfectly valid
in this context anyway), I just chose to enable everything instead.

> I'm also not sure about it, as
> controlling exactly what algs are enabled in the h/w could prevent
> misconfiguration issues in the control descriptors.
> 
That's not really what it's supposed to be used for. It's supposed to be used for
disabling algorithms the application is not ALLOWED to use e.g. because they are
not deemed secure enough (in case you have to comply, with,  say, FIPS or so).

As for misconfiguration: you may just as well hit another enabled algorithm, the
odds of which will increase as I add more. And I'm very busy adding ALL of them,
by which time you wouldn't want to disable any anyway.

> > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> >  				    u32 length)
> > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> 
> I think it's better for maintenance and readability to have something
> like:
> 
>   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
>       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> 
Not really. I *really* want to execute that for any mode other than ECB,
ECB being the *only* mode that does not require an IV (which I know
for a fact, being the architect and all :-).
And I don't believe a long list of modes that *do* require an IV would 
be  more readable or easy to maintain than this single compare ...

> > +struct safexcel_alg_template safexcel_alg_ctr_aes = {
> > +	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
> 
> Same comment as in patch 1 about the .engines member of the struct.
> 
Same answer: depends on the disappearing patch I will resend shortly.

> > +	.alg.skcipher = {
> > +		.setkey = safexcel_skcipher_aesctr_setkey,
> > +		.encrypt = safexcel_ctr_aes_encrypt,
> > +		.decrypt = safexcel_ctr_aes_decrypt,
> > +		/* Add 4 to include the 4 byte nonce! */
> > +		.min_keysize = AES_MIN_KEY_SIZE + 4,
> > +		.max_keysize = AES_MAX_KEY_SIZE + 4,
> 
> You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the
> patch as well).
> 
Makes sense. I did not know such a constant existed already.

> > +		.ivsize = 8,
> 
> And CTR_RFC3686_IV_SIZE here.
> 
Idem

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
Antoine Tenart July 26, 2019, 1:46 p.m. UTC | #3
Hi Pascal,

On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> 
> > > -		/* H/W capabilities selection */
> > > -		val = EIP197_FUNCTION_RSVD;
> > > -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> > > -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> > > -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> > > -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> > > -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> > > -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> > > -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> > > -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> > > -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > > +		/* H/W capabilities selection: just enable everything */
> > > +		writel(EIP197_FUNCTION_ALL,
> > > +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > 
> > This should be in a separate patch.
> >
> It was added specifically to  get this functionality working as CTR mode was not
> enabled.  So I don't see why it should be a seperate patch, really?
> Instead of adding CTR mode to the list (which would have been perfectly valid
> in this context anyway), I just chose to enable everything instead.

Because it's not the same thing to enable everything and to add one extra
alg. This makes bissecting the driver harder because one would not except
this change in this kind of patch.

> > I'm also not sure about it, as
> > controlling exactly what algs are enabled in the h/w could prevent
> > misconfiguration issues in the control descriptors.
> > 
> That's not really what it's supposed to be used for. It's supposed to be used for
> disabling algorithms the application is not ALLOWED to use e.g. because they are
> not deemed secure enough (in case you have to comply, with,  say, FIPS or so).

OK, that answer my question and this makes sense.

> > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > >  				    u32 length)
> > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > 
> > I think it's better for maintenance and readability to have something
> > like:
> > 
> >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > 
> Not really. I *really* want to execute that for any mode other than ECB,
> ECB being the *only* mode that does not require an IV (which I know
> for a fact, being the architect and all :-).
> And I don't believe a long list of modes that *do* require an IV would 
> be  more readable or easy to maintain than this single compare ...

That's where I disagree as you need extra knowledge to be aware of this.
Being explicit removes any possible question one may ask. But that's a
small point really :)

Thanks!
Antoine
Pascal Van Leeuwen July 26, 2019, 2:29 p.m. UTC | #4
Antoine,

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> Sent: Friday, July 26, 2019 3:47 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))
> 
> Hi Pascal,
> 
> On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> >
> > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > > >  				    u32 length)
> > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > >
> > > I think it's better for maintenance and readability to have something
> > > like:
> > >
> > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > >
> > Not really. I *really* want to execute that for any mode other than ECB,
> > ECB being the *only* mode that does not require an IV (which I know
> > for a fact, being the architect and all :-).
> > And I don't believe a long list of modes that *do* require an IV would
> > be  more readable or easy to maintain than this single compare ...
> 
> That's where I disagree as you need extra knowledge to be aware of this.
> Being explicit removes any possible question one may ask. But that's a
> small point really :)
> 
Well, while we're disagreeing ... I disagree with your assertion that you
would need more knowledge to know which modes do NOT need an IV
than to know which modes DO need an IV. There's really no fundamental
difference, it's two sides of the exact same coin ... you need that
knowledge either way. 

That being said:

1) This code is executed for each individual cipher call, i.e. it's in the
critical path. Having just 1 compare-and-branch there is better for
performance than having many.

2) Generally, all else being equal, having less code is easier to maintain
than having more code. 

3) Stuffing an IV in the token while you don't need it is harmless (apart
from wasting some cycles). NOT stuffing an IV in the token while you DO
need it is fatal. i.e. the single compare always errs on the safe side ...

4) If there is anything unclear about an otherwise fine code construct,
then you clarify it by adding a comment, not by rewriting it to be inefficient
and redundant ;-)

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
Antoine Tenart July 30, 2019, 8:24 a.m. UTC | #5
On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote:
> > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > >
> > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > > > >  				    u32 length)
> > > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > > >
> > > > I think it's better for maintenance and readability to have something
> > > > like:
> > > >
> > > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > > >
> > > Not really. I *really* want to execute that for any mode other than ECB,
> > > ECB being the *only* mode that does not require an IV (which I know
> > > for a fact, being the architect and all :-).
> > > And I don't believe a long list of modes that *do* require an IV would
> > > be  more readable or easy to maintain than this single compare ...
> > 
> > That's where I disagree as you need extra knowledge to be aware of this.
> > Being explicit removes any possible question one may ask. But that's a
> > small point really :)
> > 
> Well, while we're disagreeing ... I disagree with your assertion that you
> would need more knowledge to know which modes do NOT need an IV
> than to know which modes DO need an IV. There's really no fundamental
> difference, it's two sides of the exact same coin ... you need that
> knowledge either way. 

The point is if you look for occurrences of, let's say
CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way
easier if you have direct comparisons.

> 1) This code is executed for each individual cipher call, i.e. it's in the
> critical path. Having just 1 compare-and-branch there is better for
> performance than having many.

Not sure about what the impact really is.

> 2) Generally, all else being equal, having less code is easier to maintain
> than having more code. 

That really depends, having readable code is easier to maintain :)

> 4) If there is anything unclear about an otherwise fine code construct,
> then you clarify it by adding a comment, not by rewriting it to be inefficient
> and redundant ;-)

Fair point.

Thanks,
Antoine
Pascal Van Leeuwen July 30, 2019, 10:54 a.m. UTC | #6
> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Tuesday, July 30, 2019 10:24 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> davem@davemloft.net
> Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))
> 
> On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote:
> > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On
> Behalf Of Antoine Tenart
> > > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > > >
> > > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx
> *ctx, u8 *iv,
> > > > > >  				    u32 length)
> > > > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > > > >
> > > > > I think it's better for maintenance and readability to have something
> > > > > like:
> > > > >
> > > > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > > > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > > > >
> > > > Not really. I *really* want to execute that for any mode other than ECB,
> > > > ECB being the *only* mode that does not require an IV (which I know
> > > > for a fact, being the architect and all :-).
> > > > And I don't believe a long list of modes that *do* require an IV would
> > > > be  more readable or easy to maintain than this single compare ...
> > >
> > > That's where I disagree as you need extra knowledge to be aware of this.
> > > Being explicit removes any possible question one may ask. But that's a
> > > small point really :)
> > >
> > Well, while we're disagreeing ... I disagree with your assertion that you
> > would need more knowledge to know which modes do NOT need an IV
> > than to know which modes DO need an IV. There's really no fundamental
> > difference, it's two sides of the exact same coin ... you need that
> > knowledge either way.
> 
> The point is if you look for occurrences of, let's say
> CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way
> easier if you have direct comparisons.
> 
Not a good enough reason considering the downsides of having less
performant code that is more difficult to maintain, IMHO (you will
have to keep adding modes as we have plenty more and may add even
more to the hardware later - and make sure the list is and stays
complete going forward). 
So how often do you really need to do that search?
Speaking as someone who has been adding new modes fairly recently
but never had any need for that ...

> > 1) This code is executed for each individual cipher call, i.e. it's in the
> > critical path. Having just 1 compare-and-branch there is better for
> > performance than having many.
> 
> Not sure about what the impact really is.
> 
Well, the driver is not exactly extracting maximum performance from 
our engine for small blocks at the moment, so I would say any cycle 
gained matters. Software overhead is the Achilles heel of hardware
acceleration. And if you don't accelerate, your hardware is defacto
useless. So anything in the critical path should be carefully
considered. (which is also why I object so strongly to all these API
corner cases we're forced to support, but now I digress ...)

You can debate about significance, but at some point it all adds up.
Better to not add the cycles - if it's no effort at all! - then to
have to optimize them away later. Speaking as someone who has spent
a major part of his professional life squeezing out those cycles to
meet very real and hard performance requirements.

> > 2) Generally, all else being equal, having less code is easier to maintain
> > than having more code.
> 
> That really depends, having readable code is easier to maintain :)
> 
But how would it be more readable? "Do this for any mode other than
ECB" seems pretty clear and readable to me? And is exactly what should
happen here. Also, less code of the same complexity level (i.e. the
same simple compares!) is still more readable than more code.

> > 4) If there is anything unclear about an otherwise fine code construct,
> > then you clarify it by adding a comment, not by rewriting it to be inefficient
> > and redundant ;-)
> 
> Fair point.
> 
> Thanks,
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Patch
diff mbox series

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index c3bb177..26f086b 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -506,17 +506,9 @@  static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 		      EIP197_PE_EIP96_TOKEN_CTRL_POST_REUSE_CTX;
 		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_TOKEN_CTRL(pe));
 
-		/* H/W capabilities selection */
-		val = EIP197_FUNCTION_RSVD;
-		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
-		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
-		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
-		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
-		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
-		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
-		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
-		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
-		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
+		/* H/W capabilities selection: just enable everything */
+		writel(EIP197_FUNCTION_ALL,
+		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
 	}
 
 	/* Command Descriptor Rings prepare */
@@ -987,6 +979,7 @@  static int safexcel_request_ring_irq(void *pdev, int irqid,
 	&safexcel_alg_cbc_des3_ede,
 	&safexcel_alg_ecb_aes,
 	&safexcel_alg_cbc_aes,
+	&safexcel_alg_ctr_aes,
 	&safexcel_alg_md5,
 	&safexcel_alg_sha1,
 	&safexcel_alg_sha224,
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index 765f481..af71120 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -279,35 +279,7 @@ 
 #define EIP197_PE_EIP96_TOKEN_CTRL_POST_REUSE_CTX	BIT(20)
 
 /* EIP197_PE_EIP96_FUNCTION_EN */
-#define EIP197_FUNCTION_RSVD			(BIT(6) | BIT(15) | BIT(20) | BIT(23))
-#define EIP197_PROTOCOL_HASH_ONLY		BIT(0)
-#define EIP197_PROTOCOL_ENCRYPT_ONLY		BIT(1)
-#define EIP197_PROTOCOL_HASH_ENCRYPT		BIT(2)
-#define EIP197_PROTOCOL_HASH_DECRYPT		BIT(3)
-#define EIP197_PROTOCOL_ENCRYPT_HASH		BIT(4)
-#define EIP197_PROTOCOL_DECRYPT_HASH		BIT(5)
-#define EIP197_ALG_ARC4				BIT(7)
-#define EIP197_ALG_AES_ECB			BIT(8)
-#define EIP197_ALG_AES_CBC			BIT(9)
-#define EIP197_ALG_AES_CTR_ICM			BIT(10)
-#define EIP197_ALG_AES_OFB			BIT(11)
-#define EIP197_ALG_AES_CFB			BIT(12)
-#define EIP197_ALG_DES_ECB			BIT(13)
-#define EIP197_ALG_DES_CBC			BIT(14)
-#define EIP197_ALG_DES_OFB			BIT(16)
-#define EIP197_ALG_DES_CFB			BIT(17)
-#define EIP197_ALG_3DES_ECB			BIT(18)
-#define EIP197_ALG_3DES_CBC			BIT(19)
-#define EIP197_ALG_3DES_OFB			BIT(21)
-#define EIP197_ALG_3DES_CFB			BIT(22)
-#define EIP197_ALG_MD5				BIT(24)
-#define EIP197_ALG_HMAC_MD5			BIT(25)
-#define EIP197_ALG_SHA1				BIT(26)
-#define EIP197_ALG_HMAC_SHA1			BIT(27)
-#define EIP197_ALG_SHA2				BIT(28)
-#define EIP197_ALG_HMAC_SHA2			BIT(29)
-#define EIP197_ALG_AES_XCBC_MAC			BIT(30)
-#define EIP197_ALG_GCM_HASH			BIT(31)
+#define EIP197_FUNCTION_ALL			0xffffffff
 
 /* EIP197_PE_EIP96_CONTEXT_CTRL */
 #define EIP197_CONTEXT_SIZE(n)			(n)
@@ -356,6 +328,7 @@  struct safexcel_context_record {
 /* control1 */
 #define CONTEXT_CONTROL_CRYPTO_MODE_ECB		(0 << 0)
 #define CONTEXT_CONTROL_CRYPTO_MODE_CBC		(1 << 0)
+#define CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD	(6 << 0)
 #define CONTEXT_CONTROL_IV0			BIT(5)
 #define CONTEXT_CONTROL_IV1			BIT(6)
 #define CONTEXT_CONTROL_IV2			BIT(7)
@@ -748,6 +721,7 @@  int safexcel_hmac_setkey(const char *alg, const u8 *key, unsigned int keylen,
 extern struct safexcel_alg_template safexcel_alg_cbc_des3_ede;
 extern struct safexcel_alg_template safexcel_alg_ecb_aes;
 extern struct safexcel_alg_template safexcel_alg_cbc_aes;
+extern struct safexcel_alg_template safexcel_alg_ctr_aes;
 extern struct safexcel_alg_template safexcel_alg_md5;
 extern struct safexcel_alg_template safexcel_alg_sha1;
 extern struct safexcel_alg_template safexcel_alg_sha224;
diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index 5eed890..91945b1 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -40,6 +40,7 @@  struct safexcel_cipher_ctx {
 	bool aead;
 
 	__le32 key[8];
+	u32 nonce;
 	unsigned int key_len;
 
 	/* All the below is AEAD specific */
@@ -62,9 +63,9 @@  static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				    u32 length)
 {
 	struct safexcel_token *token;
-	u32 offset = 0, block_sz = 0;
+	u32 block_sz = 0;
 
-	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
+	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
 		switch (ctx->alg) {
 		case SAFEXCEL_DES:
 			block_sz = DES_BLOCK_SIZE;
@@ -80,11 +81,20 @@  static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 			break;
 		}
 
-		offset = block_sz / sizeof(u32);
-		memcpy(cdesc->control_data.token, iv, block_sz);
+		if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
+			/* 32 bit nonce */
+			cdesc->control_data.token[0] = ctx->nonce;
+			/* 64 bit IV part */
+			memcpy(&cdesc->control_data.token[1], iv, 8);
+			/* 32 bit counter, start at 1 (big endian!) */
+			cdesc->control_data.token[3] = cpu_to_be32(1);
+		} else {
+			memcpy(cdesc->control_data.token, iv, block_sz);
+		}
 	}
 
-	token = (struct safexcel_token *)(cdesc->control_data.token + offset);
+	/* skip over worst case IV of 4 dwords, no need to be exact */
+	token = (struct safexcel_token *)(cdesc->control_data.token + 4);
 
 	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
 	token[0].packet_length = length;
@@ -101,33 +111,35 @@  static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				u32 cryptlen, u32 assoclen, u32 digestsize)
 {
 	struct safexcel_token *token;
-	unsigned offset = 0;
+	u32 block_sz = 0;
 
-	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
-		offset = AES_BLOCK_SIZE / sizeof(u32);
-		memcpy(cdesc->control_data.token, iv, AES_BLOCK_SIZE);
+	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
+		switch (ctx->alg) {
+		case SAFEXCEL_DES:
+			block_sz = DES_BLOCK_SIZE;
+			cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD;
+			break;
+		case SAFEXCEL_3DES:
+			block_sz = DES3_EDE_BLOCK_SIZE;
+			cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD;
+			break;
+		case SAFEXCEL_AES:
+			block_sz = AES_BLOCK_SIZE;
+			cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD;
+			break;
+		}
 
-		cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD;
+		memcpy(cdesc->control_data.token, iv, block_sz);
 	}
 
-	token = (struct safexcel_token *)(cdesc->control_data.token + offset);
-
 	if (direction == SAFEXCEL_DECRYPT)
 		cryptlen -= digestsize;
 
-	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
-	token[0].packet_length = assoclen;
-	token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH;
-
-	token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
-	token[1].packet_length = cryptlen;
-	token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
-	token[1].instructions = EIP197_TOKEN_INS_LAST |
-				EIP197_TOKEN_INS_TYPE_CRYPTO |
-				EIP197_TOKEN_INS_TYPE_HASH |
-				EIP197_TOKEN_INS_TYPE_OUTPUT;
-
 	if (direction == SAFEXCEL_ENCRYPT) {
+		/* align end of instruction sequence to end of token */
+		token = (struct safexcel_token *)(cdesc->control_data.token +
+			 EIP197_MAX_TOKENS - 3);
+
 		token[2].opcode = EIP197_TOKEN_OPCODE_INSERT;
 		token[2].packet_length = digestsize;
 		token[2].stat = EIP197_TOKEN_STAT_LAST_HASH |
@@ -135,6 +147,10 @@  static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 		token[2].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT |
 					EIP197_TOKEN_INS_INSERT_HASH_DIGEST;
 	} else {
+		/* align end of instruction sequence to end of token */
+		token = (struct safexcel_token *)(cdesc->control_data.token +
+			 EIP197_MAX_TOKENS - 4);
+
 		token[2].opcode = EIP197_TOKEN_OPCODE_RETRIEVE;
 		token[2].packet_length = digestsize;
 		token[2].stat = EIP197_TOKEN_STAT_LAST_HASH |
@@ -148,6 +164,19 @@  static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				EIP197_TOKEN_STAT_LAST_PACKET;
 		token[3].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT;
 	}
+
+	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+	token[0].packet_length = assoclen;
+	token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH;
+
+	token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+	token[1].packet_length = cryptlen;
+	token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
+	token[1].instructions = EIP197_TOKEN_INS_LAST |
+				EIP197_TOKEN_INS_TYPE_CRYPTO |
+				EIP197_TOKEN_INS_TYPE_HASH |
+				EIP197_TOKEN_INS_TYPE_OUTPUT;
+
 }
 
 static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm,
@@ -1044,6 +1073,84 @@  struct safexcel_alg_template safexcel_alg_cbc_aes = {
 	},
 };
 
+static int safexcel_ctr_aes_encrypt(struct skcipher_request *req)
+{
+	return safexcel_queue_req(&req->base, skcipher_request_ctx(req),
+			SAFEXCEL_ENCRYPT, CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD,
+			SAFEXCEL_AES);
+}
+
+static int safexcel_ctr_aes_decrypt(struct skcipher_request *req)
+{
+	return safexcel_queue_req(&req->base, skcipher_request_ctx(req),
+			SAFEXCEL_DECRYPT, CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD,
+			SAFEXCEL_AES);
+}
+
+static int safexcel_skcipher_aesctr_setkey(struct crypto_skcipher *ctfm,
+					   const u8 *key, unsigned int len)
+{
+	struct crypto_tfm *tfm = crypto_skcipher_tfm(ctfm);
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct safexcel_crypto_priv *priv = ctx->priv;
+	struct crypto_aes_ctx aes;
+	int ret, i;
+	unsigned int keylen;
+
+	/* last 4 bytes of key are the nonce! */
+	ctx->nonce = *(u32 *)(key + len - 4);
+	/* exclude the nonce here */
+	keylen = len - 4;
+	ret = crypto_aes_expand_key(&aes, key, keylen);
+	if (ret) {
+		crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+		return ret;
+	}
+
+	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) {
+		for (i = 0; i < keylen / sizeof(u32); i++) {
+			if (ctx->key[i] != cpu_to_le32(aes.key_enc[i])) {
+				ctx->base.needs_inv = true;
+				break;
+			}
+		}
+	}
+
+	for (i = 0; i < keylen / sizeof(u32); i++)
+		ctx->key[i] = cpu_to_le32(aes.key_enc[i]);
+
+	ctx->key_len = keylen;
+
+	memzero_explicit(&aes, sizeof(aes));
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
+	.alg.skcipher = {
+		.setkey = safexcel_skcipher_aesctr_setkey,
+		.encrypt = safexcel_ctr_aes_encrypt,
+		.decrypt = safexcel_ctr_aes_decrypt,
+		/* Add 4 to include the 4 byte nonce! */
+		.min_keysize = AES_MIN_KEY_SIZE + 4,
+		.max_keysize = AES_MAX_KEY_SIZE + 4,
+		.ivsize = 8,
+		.base = {
+			.cra_name = "rfc3686(ctr(aes))",
+			.cra_driver_name = "safexcel-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_skcipher_cra_init,
+			.cra_exit = safexcel_skcipher_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
 static int safexcel_cbc_des_encrypt(struct skcipher_request *req)
 {
 	return safexcel_queue_req(&req->base, skcipher_request_ctx(req),