diff mbox

[RFC,v4,2/4] crypto: Introduce CRYPTO_ALG_BULK flag

Message ID 238ce3d506051c863300b90720c3e103175747cc.1465301616.git.baolin.wang@linaro.org
State New, archived
Headers show

Commit Message

(Exiting) Baolin Wang June 7, 2016, 12:17 p.m. UTC
Now some cipher hardware engines prefer to handle bulk block rather than one
sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
the intermediate values (IV) by themselves in one bulk block. This means we
can increase the size of the request by merging request rather than always 512
bytes and thus increase the hardware engine processing speed.

So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
mode.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/crypto/skcipher.h |    7 +++++++
 include/linux/crypto.h    |    6 ++++++
 2 files changed, 13 insertions(+)

Comments

Herbert Xu June 7, 2016, 2:16 p.m. UTC | #1
On Tue, Jun 07, 2016 at 08:17:05PM +0800, Baolin Wang wrote:
> Now some cipher hardware engines prefer to handle bulk block rather than one
> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
> the intermediate values (IV) by themselves in one bulk block. This means we
> can increase the size of the request by merging request rather than always 512
> bytes and thus increase the hardware engine processing speed.
> 
> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
> mode.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Nack.  As I said before, please do it using explicit IV generators
like we do for IPsec.
(Exiting) Baolin Wang June 8, 2016, 2 a.m. UTC | #2
Hi Herbert,

On 7 June 2016 at 22:16, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jun 07, 2016 at 08:17:05PM +0800, Baolin Wang wrote:
>> Now some cipher hardware engines prefer to handle bulk block rather than one
>> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
>> the intermediate values (IV) by themselves in one bulk block. This means we
>> can increase the size of the request by merging request rather than always 512
>> bytes and thus increase the hardware engine processing speed.
>>
>> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
>> mode.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>
> Nack.  As I said before, please do it using explicit IV generators
> like we do for IPsec.

OK. I would like to try your suggestion. Thanks.

> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
(Exiting) Baolin Wang June 15, 2016, 6:27 a.m. UTC | #3
Hi Herbert,

On 8 June 2016 at 10:00, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Herbert,
>
> On 7 June 2016 at 22:16, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>> On Tue, Jun 07, 2016 at 08:17:05PM +0800, Baolin Wang wrote:
>>> Now some cipher hardware engines prefer to handle bulk block rather than one
>>> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
>>> the intermediate values (IV) by themselves in one bulk block. This means we
>>> can increase the size of the request by merging request rather than always 512
>>> bytes and thus increase the hardware engine processing speed.
>>>
>>> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
>>> mode.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>
>> Nack.  As I said before, please do it using explicit IV generators
>> like we do for IPsec.
>
> OK. I would like to try your suggestion. Thanks.

After some investigation, I still think we should divide the bulk
request from dm-crypt into small request (each one is 512bytes) if
this algorithm is not support bulk mode (like CBC). We have talked
with dm-crypt
maintainers why dm-crypt always use 512 bytes as one request size in
below thread, could you please check it?
http://www.kernelhub.org/?p=2&msg=907022

That means if we move the IV handling into crypto API, we still can
not use bulk interface for all algorithm, for example we still need to
read/write with 512 bytes for CBC, you can't use 4k or more block on
CBC (and most other encryption modes). If only a part of 4k block is
written (and then system crash happens), CBC would corrupt the block
completely. It means if we map one whole bio with bulk interface in
dm-crypt, we need to divide into every 512 bytes requests in crypto
layer. So I don't think we can handle every algorithm with bulk
interface just moving the IV handling into crypto API. Thanks.

>
>> --
>> Email: Herbert Xu <herbert@gondor.apana.org.au>
>> Home Page: http://gondor.apana.org.au/~herbert/
>> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
>
>
> --
> Baolin.wang
> Best Regards
Herbert Xu June 15, 2016, 6:49 a.m. UTC | #4
On Wed, Jun 15, 2016 at 02:27:04PM +0800, Baolin Wang wrote:
> 
> After some investigation, I still think we should divide the bulk
> request from dm-crypt into small request (each one is 512bytes) if
> this algorithm is not support bulk mode (like CBC). We have talked
> with dm-crypt
> maintainers why dm-crypt always use 512 bytes as one request size in
> below thread, could you please check it?
> http://www.kernelhub.org/?p=2&msg=907022

That link only points to an email about an oops.

Diggin through that thread, the only objection I have seen is about
the fact that you have to generate a fresh IV for each sector, which
is precisely what I'm suggesting that you do.

IOW, implement the IV generators in the crypto API, and then you can
easily generate a new IV (if necessary) for each sector.

> That means if we move the IV handling into crypto API, we still can
> not use bulk interface for all algorithm, for example we still need to
> read/write with 512 bytes for CBC, you can't use 4k or more block on
> CBC (and most other encryption modes). If only a part of 4k block is
> written (and then system crash happens), CBC would corrupt the block
> completely. It means if we map one whole bio with bulk interface in
> dm-crypt, we need to divide into every 512 bytes requests in crypto
> layer. So I don't think we can handle every algorithm with bulk
> interface just moving the IV handling into crypto API. Thanks.

Of course you would do CBC in 512-byte blocks, but my point is that
you should do this in a crypto API algorithm, rather than dm-crypt
as we do now.  Once you implement that then dm-crypt can treat
every algorithm as if they supported bulk processing.

Cheers,
(Exiting) Baolin Wang June 15, 2016, 7:38 a.m. UTC | #5
On 15 June 2016 at 14:49, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Jun 15, 2016 at 02:27:04PM +0800, Baolin Wang wrote:
>>
>> After some investigation, I still think we should divide the bulk
>> request from dm-crypt into small request (each one is 512bytes) if
>> this algorithm is not support bulk mode (like CBC). We have talked
>> with dm-crypt
>> maintainers why dm-crypt always use 512 bytes as one request size in
>> below thread, could you please check it?
>> http://www.kernelhub.org/?p=2&msg=907022
>
> That link only points to an email about an oops.

Ah, sorry. Would you check this thread?
http://lkml.iu.edu/hypermail/linux/kernel/1601.1/03829.html

>
> Diggin through that thread, the only objection I have seen is about
> the fact that you have to generate a fresh IV for each sector, which
> is precisely what I'm suggesting that you do.
>
> IOW, implement the IV generators in the crypto API, and then you can
> easily generate a new IV (if necessary) for each sector.
>
>> That means if we move the IV handling into crypto API, we still can
>> not use bulk interface for all algorithm, for example we still need to
>> read/write with 512 bytes for CBC, you can't use 4k or more block on
>> CBC (and most other encryption modes). If only a part of 4k block is
>> written (and then system crash happens), CBC would corrupt the block
>> completely. It means if we map one whole bio with bulk interface in
>> dm-crypt, we need to divide into every 512 bytes requests in crypto
>> layer. So I don't think we can handle every algorithm with bulk
>> interface just moving the IV handling into crypto API. Thanks.
>
> Of course you would do CBC in 512-byte blocks, but my point is that
> you should do this in a crypto API algorithm, rather than dm-crypt
> as we do now.  Once you implement that then dm-crypt can treat
> every algorithm as if they supported bulk processing.

But that means we should divide the bulk request into 512-byte size
requests and break up the mapped sg table for each request. Another
hand we should allocate memory for each request in crypto layer, which
dm-crypt have supplied one high efficiency way. I think these are
really top level how to use the crypro APIs, does that need to move
into crypto laryer? Thanks.

>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu June 15, 2016, 7:39 a.m. UTC | #6
On Wed, Jun 15, 2016 at 03:38:02PM +0800, Baolin Wang wrote:
>
> But that means we should divide the bulk request into 512-byte size
> requests and break up the mapped sg table for each request. Another
> hand we should allocate memory for each request in crypto layer, which
> dm-crypt have supplied one high efficiency way. I think these are
> really top level how to use the crypro APIs, does that need to move
> into crypto laryer? Thanks.

I have already explained to you how you can piggy-back off dm-crypt's
allocation, so what's the problem?
(Exiting) Baolin Wang June 15, 2016, 8:48 a.m. UTC | #7
On 15 June 2016 at 15:39, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Jun 15, 2016 at 03:38:02PM +0800, Baolin Wang wrote:
>>
>> But that means we should divide the bulk request into 512-byte size
>> requests and break up the mapped sg table for each request. Another
>> hand we should allocate memory for each request in crypto layer, which
>> dm-crypt have supplied one high efficiency way. I think these are
>> really top level how to use the crypro APIs, does that need to move
>> into crypto laryer? Thanks.
>
> I have already explained to you how you can piggy-back off dm-crypt's
> allocation, so what's the problem?

Because the request created in dm-crypt is connecting with dm-crypt
closely, I am worried if it can work or introduce other issues if we
move these top level things into crypto layer. Anyway I will try to do
that. Thanks.

> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
diff mbox

Patch

diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 0f987f5..d89d29a 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -519,5 +519,12 @@  static inline void skcipher_request_set_crypt(
 	req->iv = iv;
 }
 
+static inline unsigned int skcipher_is_bulk_mode(struct crypto_skcipher *sk_tfm)
+{
+	struct crypto_tfm *tfm = crypto_skcipher_tfm(sk_tfm);
+
+	return crypto_tfm_alg_bulk(tfm);
+}
+
 #endif	/* _CRYPTO_SKCIPHER_H */
 
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 6e28c89..a315487 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -63,6 +63,7 @@ 
 #define CRYPTO_ALG_DEAD			0x00000020
 #define CRYPTO_ALG_DYING		0x00000040
 #define CRYPTO_ALG_ASYNC		0x00000080
+#define CRYPTO_ALG_BULK			0x00000100
 
 /*
  * Set this bit if and only if the algorithm requires another algorithm of
@@ -623,6 +624,11 @@  static inline u32 crypto_tfm_alg_type(struct crypto_tfm *tfm)
 	return tfm->__crt_alg->cra_flags & CRYPTO_ALG_TYPE_MASK;
 }
 
+static inline unsigned int crypto_tfm_alg_bulk(struct crypto_tfm *tfm)
+{
+	return tfm->__crt_alg->cra_flags & CRYPTO_ALG_BULK;
+}
+
 static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
 {
 	return tfm->__crt_alg->cra_blocksize;