| Message ID | 238ce3d506051c863300b90720c3e103175747cc.1465301616.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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.
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
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
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,
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
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?
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 --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;
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(+)