Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Wed, Aug 09, 2017 at 11:40:12AM +0200, Corentin Labbe wrote:
>
> I really didnt see how to do that since cra_type is const.
> Anyway, I think it cannot be possible since we could have two different 
> engine with two different prepare_request().
> 
> I will really appreciate any advice on what you want exactly.

The issue is that we have these handlers such as prepare_request
that gives us a level of indirection into the driver.  However,
on top of those we're adding another level of indirection based
on the type of request.

I'd like to see these two combined into just a single function
pointer.

My idea of using cra_type is indeed broken as these functions
ultimately live in the driver and not crypto API.

So how about something like this:

struct crypto_engine_op {
int (*prepare_request)(struct crypto_engine *engine,
   struct crypto_async_request *req);
int (*unprepare_request)(struct crypto_engine *engine,
 struct crypto_async_request *req);
int (*do_one_request)(struct crypto_engine *engine,
  struct crypto_async_request *req);
};

struct crypto_engine_reqctx {
struct crypto_engine_op *op;
};  

struct crypto_engine {
struct crypto_engine_op aead;
struct crypto_engine_op hash;
struct crypto_engine_op cipher;

...
};

Then in the request_to_engine functions you would store the right
op pointer inside the request context area.  Obviously the users
of crypto_engine would need to allocate space for the struct
crypto_engine_reqctx in their reqctx structure.

The only wart with this scheme is that the drivers end up seeing
struct crypto_async_request and will need to convert that to the
respective request types but I couldn't really find a better way.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jul 28, 2017 at 05:01:19PM +0200, Corentin Labbe wrote:
> On Fri, Jul 28, 2017 at 09:52:57PM +0800, Herbert Xu wrote:
> > On Fri, Jul 14, 2017 at 01:15:36PM +0200, Corentin Labbe wrote:
> > > On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> > > > On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> > > > >
> > > > > Since there are two different user of "crypto engine + ablkcipher", 
> > > > > it will be not easy to convert them in one serie. (I could do it, but 
> > > > > I simply could not test it for OMAP (lack of hw))
> > > > > And any new user which want to use crypto engine+skcipher (like me 
> > > > > with the sun8i-ce driver) are simply stuck.
> > > > 
> > > > You're right.  We'll need to do this in a backwards-compatible way.  In 
> > > > fact
> > > > we already do something similar in skcipher.c itself.  Simply look at 
> > > > the
> > > > cra_type field and if it matches blkcipher/ablkcipher/givcipher then 
> > > > it's
> > > > legacy ablkcipher, otherwise it's skcipher.
> > > > 
> > > > Also the way crypto_engine looks at the request type in the data-path is
> > > > suboptimal.  This should really be built into the cra_type object.  For
> > > > example, we can have cra_type->engine->prepare_request which would just
> > > > do the right thing.
> > > > 
> > > 
> > > Not sure to have well understand what you want.
> > > You want that I switch on cra_type instead of crypto_tfm_alg_type() ?
> > 
> > No I mean that we should have an engine hooks object registered
> > under cra_type so that you simply call
> > 
> > cra_type->engine->prepare_request()
> > 
> > regardless of what type you're using.
> > 
> 
> I am sorry, I didnt see how to do that.

Hello

I really didnt see how to do that since cra_type is const.
Anyway, I think it cannot be possible since we could have two different engine 
with two different prepare_request().

I will really appreciate any advice on what you want exactly.

Regards

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jul 28, 2017 at 09:52:57PM +0800, Herbert Xu wrote:
> On Fri, Jul 14, 2017 at 01:15:36PM +0200, Corentin Labbe wrote:
> > On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> > > On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> > > >
> > > > Since there are two different user of "crypto engine + ablkcipher", it 
> > > > will be not easy to convert them in one serie. (I could do it, but I 
> > > > simply could not test it for OMAP (lack of hw))
> > > > And any new user which want to use crypto engine+skcipher (like me with 
> > > > the sun8i-ce driver) are simply stuck.
> > > 
> > > You're right.  We'll need to do this in a backwards-compatible way.  In 
> > > fact
> > > we already do something similar in skcipher.c itself.  Simply look at the
> > > cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> > > legacy ablkcipher, otherwise it's skcipher.
> > > 
> > > Also the way crypto_engine looks at the request type in the data-path is
> > > suboptimal.  This should really be built into the cra_type object.  For
> > > example, we can have cra_type->engine->prepare_request which would just
> > > do the right thing.
> > > 
> > 
> > Not sure to have well understand what you want.
> > You want that I switch on cra_type instead of crypto_tfm_alg_type() ?
> 
> No I mean that we should have an engine hooks object registered
> under cra_type so that you simply call
> 
>   cra_type->engine->prepare_request()
> 
> regardless of what type you're using.
> 

I am sorry, I didnt see how to do that.

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Fri, Jul 14, 2017 at 01:15:36PM +0200, Corentin Labbe wrote:
> On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> > On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> > >
> > > Since there are two different user of "crypto engine + ablkcipher", it 
> > > will be not easy to convert them in one serie. (I could do it, but I 
> > > simply could not test it for OMAP (lack of hw))
> > > And any new user which want to use crypto engine+skcipher (like me with 
> > > the sun8i-ce driver) are simply stuck.
> > 
> > You're right.  We'll need to do this in a backwards-compatible way.  In fact
> > we already do something similar in skcipher.c itself.  Simply look at the
> > cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> > legacy ablkcipher, otherwise it's skcipher.
> > 
> > Also the way crypto_engine looks at the request type in the data-path is
> > suboptimal.  This should really be built into the cra_type object.  For
> > example, we can have cra_type->engine->prepare_request which would just
> > do the right thing.
> > 
> 
> Not sure to have well understand what you want.
> You want that I switch on cra_type instead of crypto_tfm_alg_type() ?

No I mean that we should have an engine hooks object registered
under cra_type so that you simply call

cra_type->engine->prepare_request()

regardless of what type you're using.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> >
> > Since there are two different user of "crypto engine + ablkcipher", it will 
> > be not easy to convert them in one serie. (I could do it, but I simply 
> > could not test it for OMAP (lack of hw))
> > And any new user which want to use crypto engine+skcipher (like me with the 
> > sun8i-ce driver) are simply stuck.
> 
> You're right.  We'll need to do this in a backwards-compatible way.  In fact
> we already do something similar in skcipher.c itself.  Simply look at the
> cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> legacy ablkcipher, otherwise it's skcipher.
> 
> Also the way crypto_engine looks at the request type in the data-path is
> suboptimal.  This should really be built into the cra_type object.  For
> example, we can have cra_type->engine->prepare_request which would just
> do the right thing.
> 

Not sure to have well understand what you want.
You want that I switch on cra_type instead of crypto_tfm_alg_type() ?

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
>
> Since there are two different user of "crypto engine + ablkcipher", it will 
> be not easy to convert them in one serie. (I could do it, but I simply could 
> not test it for OMAP (lack of hw))
> And any new user which want to use crypto engine+skcipher (like me with the 
> sun8i-ce driver) are simply stuck.

You're right.  We'll need to do this in a backwards-compatible way.  In fact
we already do something similar in skcipher.c itself.  Simply look at the
cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
legacy ablkcipher, otherwise it's skcipher.

Also the way crypto_engine looks at the request type in the data-path is
suboptimal.  This should really be built into the cra_type object.  For
example, we can have cra_type->engine->prepare_request which would just
do the right thing.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Mon, Jun 19, 2017 at 01:27:08PM +0800, Herbert Xu wrote:
> On Tue, Jun 06, 2017 at 03:44:17PM +0200, Corentin Labbe wrote:
> > The crypto engine could actually only enqueue hash and ablkcipher request.
> > This patch permit it to enqueue skcipher requets by adding all necessary
> > functions.
> > The only problem is that ablkcipher and skcipher id are the same, so
> > only one cipher type is usable on the same crypto engine.
> > 
> > Signed-off-by: Corentin Labbe 
> 
> I think this should be done as part of the skcipher conversion rather
> than as a standalone patch.
> 

Since there are two different user of "crypto engine + ablkcipher", it will be 
not easy to convert them in one serie. (I could do it, but I simply could not 
test it for OMAP (lack of hw))
And any new user which want to use crypto engine+skcipher (like me with the 
sun8i-ce driver) are simply stuck.

Regards

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Tue, Jun 06, 2017 at 03:44:17PM +0200, Corentin Labbe wrote:
> The crypto engine could actually only enqueue hash and ablkcipher request.
> This patch permit it to enqueue skcipher requets by adding all necessary
> functions.
> The only problem is that ablkcipher and skcipher id are the same, so
> only one cipher type is usable on the same crypto engine.
> 
> Signed-off-by: Corentin Labbe 

I think this should be done as part of the skcipher conversion rather
than as a standalone patch.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Tue, Jun 06, 2017 at 04:07:25PM +0200, Stephan Müller wrote:
> Am Dienstag, 6. Juni 2017, 15:44:17 CEST schrieb Corentin Labbe:
> 
> Hi Corentin,
> 
> > The crypto engine could actually only enqueue hash and ablkcipher request.
> > This patch permit it to enqueue skcipher requets by adding all necessary
> > functions.
> > The only problem is that ablkcipher and skcipher id are the same, so
> > only one cipher type is usable on the same crypto engine.
> 
> Why do you think this is needed? I thought that skcipher is the successor to 
> ablkcipher AND blkcipher. I.e. ablkcipher and blkcipher should be phased out. 
> Thus, I would infer that any ablkcipher support should or could be dropped.
> 

There are two user of it, crypto/omap and crypto/virtio.
And for crypto/omap, I have no way of testing any convertion to skcipher (no 
hardware).

Regards
Corentin Labbe

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Stephan Müller]

Am Dienstag, 6. Juni 2017, 15:44:17 CEST schrieb Corentin Labbe:

Hi Corentin,

> The crypto engine could actually only enqueue hash and ablkcipher request.
> This patch permit it to enqueue skcipher requets by adding all necessary
> functions.
> The only problem is that ablkcipher and skcipher id are the same, so
> only one cipher type is usable on the same crypto engine.

Why do you think this is needed? I thought that skcipher is the successor to 
ablkcipher AND blkcipher. I.e. ablkcipher and blkcipher should be phased out. 
Thus, I would infer that any ablkcipher support should or could be dropped.

Ciao
Stephan

[PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

The crypto engine could actually only enqueue hash and ablkcipher request.
This patch permit it to enqueue skcipher requets by adding all necessary
functions.
The only problem is that ablkcipher and skcipher id are the same, so
only one cipher type is usable on the same crypto engine.

Signed-off-by: Corentin Labbe 
---
 crypto/crypto_engine.c  | 138 
 include/crypto/engine.h |  14 +
 2 files changed, 141 insertions(+), 11 deletions(-)

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index 61e7c4e02fd2..fc0f58b6299c 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -36,6 +36,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
struct crypto_async_request *async_req, *backlog;
struct ahash_request *hreq;
struct ablkcipher_request *breq;
+   struct skcipher_request *skreq;
unsigned long flags;
bool was_busy = false;
int ret, rtype;
@@ -123,17 +124,34 @@ static void crypto_pump_requests(struct crypto_engine 
*engine,
}
return;
case CRYPTO_ALG_TYPE_ABLKCIPHER:
-   breq = ablkcipher_request_cast(engine->cur_req);
-   if (engine->prepare_cipher_request) {
-   ret = engine->prepare_cipher_request(engine, breq);
-   if (ret) {
-   dev_err(engine->dev, "failed to prepare 
request: %d\n",
-   ret);
-   goto req_err;
+   /* since CRYPTO_ALG_TYPE_ABLKCIPHER == CRYPTO_ALG_TYPE_SKCIPHER
+* differenciate both with the presence of cipher_one_request
+*/
+   if (engine->cipher_one_request) {
+   breq = ablkcipher_request_cast(engine->cur_req);
+   if (engine->prepare_cipher_request) {
+   ret = engine->prepare_cipher_request(engine, 
breq);
+   if (ret) {
+   dev_err(engine->dev, "failed to prepare 
request: %d\n",
+   ret);
+   goto req_err;
+   }
+   engine->cur_req_prepared = true;
}
-   engine->cur_req_prepared = true;
+   ret = engine->cipher_one_request(engine, breq);
+   } else {
+   skreq = skcipher_request_cast(engine->cur_req);
+   if (engine->prepare_skcipher_request) {
+   ret = engine->prepare_skcipher_request(engine, 
skreq);
+   if (ret) {
+   dev_err(engine->dev, "failed to prepare 
request: %d\n",
+   ret);
+   goto req_err;
+   }
+   engine->cur_req_prepared = true;
+   }
+   ret = engine->skcipher_one_request(engine, skreq);
}
-   ret = engine->cipher_one_request(engine, breq);
if (ret) {
dev_err(engine->dev, "failed to cipher one request from 
queue\n");
goto req_err;
@@ -151,8 +169,13 @@ static void crypto_pump_requests(struct crypto_engine 
*engine,
crypto_finalize_hash_request(engine, hreq, ret);
break;
case CRYPTO_ALG_TYPE_ABLKCIPHER:
-   breq = ablkcipher_request_cast(engine->cur_req);
-   crypto_finalize_cipher_request(engine, breq, ret);
+   if (engine->cipher_one_request) {
+   breq = ablkcipher_request_cast(engine->cur_req);
+   crypto_finalize_cipher_request(engine, breq, ret);
+   } else {
+   skreq = skcipher_request_cast(engine->cur_req);
+   crypto_finalize_skcipher_request(engine, skreq, ret);
+   }
break;
}
return;
@@ -213,6 +236,49 @@ int crypto_transfer_cipher_request_to_engine(struct 
crypto_engine *engine,
 EXPORT_SYMBOL_GPL(crypto_transfer_cipher_request_to_engine);
 
 /**
+ * crypto_transfer_skcipher_request - transfer the new request into the
+ * enginequeue
+ * @engine: the hardware engine
+ * @req: the request need to be listed into the engine queue
+ */
+int crypto_transfer_skcipher_request(struct crypto_engine *engine,
+struct skcipher_request *req,
+bool need_pump)
+{
+   unsigned long flags;
+   int ret;
+
+   spin_lock_irqsave(>queue_lock, flags);
+
+   if (!engine->running) {
+   spin_unlock_irqrestore(>queue_lock, flags);
+   return