| Message ID | 20210225182716.1402449-7-thara.gopinath@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for AEAD algorithms in Qualcomm Crypto Engine driver | expand |
On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: > Add register programming sequence for enabling AEAD > algorithms on the Qualcomm crypto engine. > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > --- > drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- > 1 file changed, 153 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c > index 05a71c5ecf61..54d209cb0525 100644 > --- a/drivers/crypto/qce/common.c > +++ b/drivers/crypto/qce/common.c > @@ -15,6 +15,16 @@ > #include "core.h" > #include "regs-v5.h" > #include "sha.h" > +#include "aead.h" > + > +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { > + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 > +}; > + > +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { > + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, > + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 > +}; > > static inline u32 qce_read(struct qce_device *qce, u32 offset) > { > @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) > qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); > } > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > { > u32 cfg = 0; > @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > return cfg; > } > +#endif > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > { > struct ahash_request *req = ahash_request_cast(async_req); > @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > } > #endif > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > { > u32 cfg = 0; > @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > return cfg; > } > +#endif > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) > { > u8 swap[QCE_AES_IV_LENGTH]; > @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) > } > #endif > > +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD > +static int qce_setup_regs_aead(struct crypto_async_request *async_req) > +{ > + struct aead_request *req = aead_request_cast(async_req); > + struct qce_aead_reqctx *rctx = aead_request_ctx(req); > + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); > + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); > + struct qce_device *qce = tmpl->qce; > + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; > + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; > + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; > + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; > + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; > + unsigned int enc_keylen = ctx->enc_keylen; > + unsigned int auth_keylen = ctx->auth_keylen; > + unsigned int enc_ivsize = rctx->ivsize; > + unsigned int auth_ivsize; > + unsigned int enckey_words, enciv_words; > + unsigned int authkey_words, authiv_words, authnonce_words; > + unsigned long flags = rctx->flags; > + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; I don't see any reason to initialize encr_cfg or auth_cfg. > + u32 *iv_last_word; > + > + qce_setup_config(qce); > + > + /* Write encryption key */ > + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); > + enckey_words = enc_keylen / sizeof(u32); > + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); Afaict all "array registers" in this function are affected by the CRYPTO_SETUP little endian bit, but you set this bit before launching the operation dependent on IS_CCM(). So is this really working for the !IS_CCM() case? > + > + /* Write encryption iv */ > + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); > + enciv_words = enc_ivsize / sizeof(u32); > + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); It would be nice if this snippet was extracted to a helper function. > + > + if (IS_CCM(rctx->flags)) { > + iv_last_word = (u32 *)&enciv[enciv_words - 1]; > +// qce_write(qce, REG_CNTR3_IV3, enciv[enciv_words - 1] + 1); I believe this is a remnant of the two surrounding lines. > + qce_write(qce, REG_CNTR3_IV3, (*iv_last_word) + 1); enciv is an array of big endian 32-bit integers, which you tell the compiler to treat as cpu-native endian, and then you do math on it. Afaict from the documentation the value of REG_CNTR3_IVn should be set to rctx->iv + 1, but if the hardware expects these in big endian then I think you added 16777216. Perhaps I'm missing something here though? PS. Based on how the documentation is written, shouldn't you write out REG_CNTR_IV[012] as well? > + qce_write_array(qce, REG_ENCR_CCM_INT_CNTR0, (u32 *)enciv, enciv_words); > + qce_write(qce, REG_CNTR_MASK, ~0); > + qce_write(qce, REG_CNTR_MASK0, ~0); > + qce_write(qce, REG_CNTR_MASK1, ~0); > + qce_write(qce, REG_CNTR_MASK2, ~0); > + } > + > + /* Clear authentication IV and KEY registers of previous values */ > + qce_clear_array(qce, REG_AUTH_IV0, 16); > + qce_clear_array(qce, REG_AUTH_KEY0, 16); > + > + /* Clear byte count */ > + qce_clear_array(qce, REG_AUTH_BYTECNT0, 4); > + > + /* Write authentication key */ > + qce_cpu_to_be32p_array(authkey, ctx->auth_key, auth_keylen); > + authkey_words = DIV_ROUND_UP(auth_keylen, sizeof(u32)); > + qce_write_array(qce, REG_AUTH_KEY0, (u32 *)authkey, authkey_words); > + > + if (IS_SHA_HMAC(rctx->flags)) { > + /* Write default authentication iv */ > + if (IS_SHA1_HMAC(rctx->flags)) { > + auth_ivsize = SHA1_DIGEST_SIZE; > + memcpy(authiv, std_iv_sha1, auth_ivsize); > + } else if (IS_SHA256_HMAC(rctx->flags)) { > + auth_ivsize = SHA256_DIGEST_SIZE; > + memcpy(authiv, std_iv_sha256, auth_ivsize); > + } > + authiv_words = auth_ivsize / sizeof(u32); > + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); AUTH_IV0 is affected by the little endian configuration, does this imply that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so I think it would be nice if you grouped the conditionals in a way that made that obvious when reading the function. > + } > + > + if (IS_CCM(rctx->flags)) { > + qce_cpu_to_be32p_array(authnonce, rctx->ccm_nonce, QCE_MAX_NONCE); > + authnonce_words = QCE_MAX_NONCE / sizeof(u32); > + qce_write_array(qce, REG_AUTH_INFO_NONCE0, (u32 *)authnonce, authnonce_words); > + } > + > + /* Set up ENCR_SEG_CFG */ > + encr_cfg = qce_encr_cfg(flags, enc_keylen); > + if (IS_ENCRYPT(flags)) > + encr_cfg |= BIT(ENCODE_SHIFT); > + qce_write(qce, REG_ENCR_SEG_CFG, encr_cfg); > + > + /* Set up AUTH_SEG_CFG */ > + auth_cfg = qce_auth_cfg(rctx->flags, auth_keylen, ctx->authsize); > + auth_cfg |= BIT(AUTH_LAST_SHIFT); > + auth_cfg |= BIT(AUTH_FIRST_SHIFT); > + if (IS_ENCRYPT(flags)) { > + if (IS_CCM(rctx->flags)) > + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; > + else > + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; > + } else { > + if (IS_CCM(rctx->flags)) > + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; > + else > + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; > + } > + qce_write(qce, REG_AUTH_SEG_CFG, auth_cfg); > + > + totallen = rctx->cryptlen + rctx->assoclen; > + > + /* Set the encryption size and start offset */ > + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) > + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen + ctx->authsize); > + else > + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen); > + qce_write(qce, REG_ENCR_SEG_START, rctx->assoclen & 0xffff); > + > + /* Set the authentication size and start offset */ > + qce_write(qce, REG_AUTH_SEG_SIZE, totallen); > + qce_write(qce, REG_AUTH_SEG_START, 0); > + > + /* Write total length */ > + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) > + qce_write(qce, REG_SEG_SIZE, totallen + ctx->authsize); > + else > + qce_write(qce, REG_SEG_SIZE, totallen); > + > + /* get little endianness */ > + config = qce_config_reg(qce, 1); > + qce_write(qce, REG_CONFIG, config); > + > + /* Start the process */ > + if (IS_CCM(flags)) > + qce_crypto_go(qce, 0); Second parameter is defined as "bool", please use "false" here (and true below). Or qce_crypto_go(qce, !IS_CCM(flags)); Regards, Bjorn > + else > + qce_crypto_go(qce, 1); > + > + return 0; > +} > +#endif > + > int qce_start(struct crypto_async_request *async_req, u32 type) > { > switch (type) { > @@ -396,6 +543,10 @@ int qce_start(struct crypto_async_request *async_req, u32 type) > #ifdef CONFIG_CRYPTO_DEV_QCE_SHA > case CRYPTO_ALG_TYPE_AHASH: > return qce_setup_regs_ahash(async_req); > +#endif > +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD > + case CRYPTO_ALG_TYPE_AEAD: > + return qce_setup_regs_aead(async_req); > #endif > default: > return -EINVAL; > -- > 2.25.1 >
Hi Bjorn, On 4/5/21 6:18 PM, Bjorn Andersson wrote: > On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: > >> Add register programming sequence for enabling AEAD >> algorithms on the Qualcomm crypto engine. >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- >> 1 file changed, 153 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c >> index 05a71c5ecf61..54d209cb0525 100644 >> --- a/drivers/crypto/qce/common.c >> +++ b/drivers/crypto/qce/common.c >> @@ -15,6 +15,16 @@ >> #include "core.h" >> #include "regs-v5.h" >> #include "sha.h" >> +#include "aead.h" >> + >> +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { >> + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 >> +}; >> + >> +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { >> + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, >> + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 >> +}; >> >> static inline u32 qce_read(struct qce_device *qce, u32 offset) >> { >> @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) >> qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); >> } >> >> -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >> +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >> static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >> { >> u32 cfg = 0; >> @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >> >> return cfg; >> } >> +#endif >> >> +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >> static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >> { >> struct ahash_request *req = ahash_request_cast(async_req); >> @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >> } >> #endif >> >> -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >> +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >> static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >> { >> u32 cfg = 0; >> @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >> >> return cfg; >> } >> +#endif >> >> +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >> static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) >> { >> u8 swap[QCE_AES_IV_LENGTH]; >> @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) >> } >> #endif >> >> +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD >> +static int qce_setup_regs_aead(struct crypto_async_request *async_req) >> +{ >> + struct aead_request *req = aead_request_cast(async_req); >> + struct qce_aead_reqctx *rctx = aead_request_ctx(req); >> + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); >> + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); >> + struct qce_device *qce = tmpl->qce; >> + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; >> + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; >> + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; >> + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; >> + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; >> + unsigned int enc_keylen = ctx->enc_keylen; >> + unsigned int auth_keylen = ctx->auth_keylen; >> + unsigned int enc_ivsize = rctx->ivsize; >> + unsigned int auth_ivsize; >> + unsigned int enckey_words, enciv_words; >> + unsigned int authkey_words, authiv_words, authnonce_words; >> + unsigned long flags = rctx->flags; >> + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; > > I don't see any reason to initialize encr_cfg or auth_cfg. right.. I will remove it > >> + u32 *iv_last_word; >> + >> + qce_setup_config(qce); >> + >> + /* Write encryption key */ >> + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); >> + enckey_words = enc_keylen / sizeof(u32); >> + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); > > Afaict all "array registers" in this function are affected by the > CRYPTO_SETUP little endian bit, but you set this bit before launching > the operation dependent on IS_CCM(). So is this really working for the > !IS_CCM() case? I am not sure I understand you. Below , /* get little endianness */ config = qce_config_reg(qce, 1); qce_write(qce, REG_CONFIG, config); is outside of any checks.. > >> + >> + /* Write encryption iv */ >> + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); >> + enciv_words = enc_ivsize / sizeof(u32); >> + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > > It would be nice if this snippet was extracted to a helper function. > >> + >> + if (IS_CCM(rctx->flags)) { >> + iv_last_word = (u32 *)&enciv[enciv_words - 1]; >> +// qce_write(qce, REG_CNTR3_IV3, enciv[enciv_words - 1] + 1); > > I believe this is a remnant of the two surrounding lines. It indeed is.. I will remove it. > >> + qce_write(qce, REG_CNTR3_IV3, (*iv_last_word) + 1); > > enciv is an array of big endian 32-bit integers, which you tell the > compiler to treat as cpu-native endian, and then you do math on it. > Afaict from the documentation the value of REG_CNTR3_IVn should be set > to rctx->iv + 1, but if the hardware expects these in big endian then I > think you added 16777216. So, the crypto engine documentation talks of writing to these registers in little endian mode. The byte stream that you get for iv from the user is in big endian mode as in the MSB is byte 0. So we kind of invert this and write to these registers. This is what happens with declaring the __be32 array and copying words to it from the byte stream. So now byte 0 is the LSB and a +1 will just add a 1 to it. I suspect from what I read in the documentation we could get away by removing this and writing the big endian byte stream directly and never setting the little endian in config register. Though I am not sure if this has ever been tested out. If we change it, it will be across algorithms and as a separate effort. > > Perhaps I'm missing something here though? > > PS. Based on how the documentation is written, shouldn't you write out > REG_CNTR_IV[012] as well? It is done on top, right ? qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > >> + qce_write_array(qce, REG_ENCR_CCM_INT_CNTR0, (u32 *)enciv, enciv_words); >> + qce_write(qce, REG_CNTR_MASK, ~0); >> + qce_write(qce, REG_CNTR_MASK0, ~0); >> + qce_write(qce, REG_CNTR_MASK1, ~0); >> + qce_write(qce, REG_CNTR_MASK2, ~0); >> + } >> + >> + /* Clear authentication IV and KEY registers of previous values */ >> + qce_clear_array(qce, REG_AUTH_IV0, 16); >> + qce_clear_array(qce, REG_AUTH_KEY0, 16); >> + >> + /* Clear byte count */ >> + qce_clear_array(qce, REG_AUTH_BYTECNT0, 4); >> + >> + /* Write authentication key */ >> + qce_cpu_to_be32p_array(authkey, ctx->auth_key, auth_keylen); >> + authkey_words = DIV_ROUND_UP(auth_keylen, sizeof(u32)); >> + qce_write_array(qce, REG_AUTH_KEY0, (u32 *)authkey, authkey_words); >> + >> + if (IS_SHA_HMAC(rctx->flags)) { >> + /* Write default authentication iv */ >> + if (IS_SHA1_HMAC(rctx->flags)) { >> + auth_ivsize = SHA1_DIGEST_SIZE; >> + memcpy(authiv, std_iv_sha1, auth_ivsize); >> + } else if (IS_SHA256_HMAC(rctx->flags)) { >> + auth_ivsize = SHA256_DIGEST_SIZE; >> + memcpy(authiv, std_iv_sha256, auth_ivsize); >> + } >> + authiv_words = auth_ivsize / sizeof(u32); >> + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); > > AUTH_IV0 is affected by the little endian configuration, does this imply > that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so > I think it would be nice if you grouped the conditionals in a way that > made that obvious when reading the function. So yes IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags. AUTH_IVn is 0 for ccm and has initial value for HMAC algorithms. I don't understand the confusion here. > >> + } >> + >> + if (IS_CCM(rctx->flags)) { >> + qce_cpu_to_be32p_array(authnonce, rctx->ccm_nonce, QCE_MAX_NONCE); >> + authnonce_words = QCE_MAX_NONCE / sizeof(u32); >> + qce_write_array(qce, REG_AUTH_INFO_NONCE0, (u32 *)authnonce, authnonce_words); >> + } >> + >> + /* Set up ENCR_SEG_CFG */ >> + encr_cfg = qce_encr_cfg(flags, enc_keylen); >> + if (IS_ENCRYPT(flags)) >> + encr_cfg |= BIT(ENCODE_SHIFT); >> + qce_write(qce, REG_ENCR_SEG_CFG, encr_cfg); >> + >> + /* Set up AUTH_SEG_CFG */ >> + auth_cfg = qce_auth_cfg(rctx->flags, auth_keylen, ctx->authsize); >> + auth_cfg |= BIT(AUTH_LAST_SHIFT); >> + auth_cfg |= BIT(AUTH_FIRST_SHIFT); >> + if (IS_ENCRYPT(flags)) { >> + if (IS_CCM(rctx->flags)) >> + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; >> + else >> + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; >> + } else { >> + if (IS_CCM(rctx->flags)) >> + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; >> + else >> + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; >> + } >> + qce_write(qce, REG_AUTH_SEG_CFG, auth_cfg); >> + >> + totallen = rctx->cryptlen + rctx->assoclen; >> + >> + /* Set the encryption size and start offset */ >> + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) >> + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen + ctx->authsize); >> + else >> + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen); >> + qce_write(qce, REG_ENCR_SEG_START, rctx->assoclen & 0xffff); >> + >> + /* Set the authentication size and start offset */ >> + qce_write(qce, REG_AUTH_SEG_SIZE, totallen); >> + qce_write(qce, REG_AUTH_SEG_START, 0); >> + >> + /* Write total length */ >> + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) >> + qce_write(qce, REG_SEG_SIZE, totallen + ctx->authsize); >> + else >> + qce_write(qce, REG_SEG_SIZE, totallen); >> + >> + /* get little endianness */ >> + config = qce_config_reg(qce, 1); >> + qce_write(qce, REG_CONFIG, config); >> + >> + /* Start the process */ >> + if (IS_CCM(flags)) >> + qce_crypto_go(qce, 0); > > Second parameter is defined as "bool", please use "false" here (and true > below). Or > > qce_crypto_go(qce, !IS_CCM(flags)); will do... I like the one liner better. -- Warm Regards Thara
On Tue 13 Apr 16:31 CDT 2021, Thara Gopinath wrote: > > Hi Bjorn, > > On 4/5/21 6:18 PM, Bjorn Andersson wrote: > > On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: > > > > > Add register programming sequence for enabling AEAD > > > algorithms on the Qualcomm crypto engine. > > > > > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > > --- > > > drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- > > > 1 file changed, 153 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c > > > index 05a71c5ecf61..54d209cb0525 100644 > > > --- a/drivers/crypto/qce/common.c > > > +++ b/drivers/crypto/qce/common.c > > > @@ -15,6 +15,16 @@ > > > #include "core.h" > > > #include "regs-v5.h" > > > #include "sha.h" > > > +#include "aead.h" > > > + > > > +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { > > > + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 > > > +}; > > > + > > > +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { > > > + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, > > > + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 > > > +}; > > > static inline u32 qce_read(struct qce_device *qce, u32 offset) > > > { > > > @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) > > > qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); > > > } > > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > > > +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > > > static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > > { > > > u32 cfg = 0; > > > @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > > return cfg; > > > } > > > +#endif > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > > > static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > > > { > > > struct ahash_request *req = ahash_request_cast(async_req); > > > @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > > > } > > > #endif > > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > > > +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > > > static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > > { > > > u32 cfg = 0; > > > @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > > return cfg; > > > } > > > +#endif > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > > > static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) > > > { > > > u8 swap[QCE_AES_IV_LENGTH]; > > > @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) > > > } > > > #endif > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD > > > +static int qce_setup_regs_aead(struct crypto_async_request *async_req) > > > +{ > > > + struct aead_request *req = aead_request_cast(async_req); > > > + struct qce_aead_reqctx *rctx = aead_request_ctx(req); > > > + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); > > > + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); > > > + struct qce_device *qce = tmpl->qce; > > > + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; > > > + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; > > > + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; > > > + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; > > > + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; > > > + unsigned int enc_keylen = ctx->enc_keylen; > > > + unsigned int auth_keylen = ctx->auth_keylen; > > > + unsigned int enc_ivsize = rctx->ivsize; > > > + unsigned int auth_ivsize; > > > + unsigned int enckey_words, enciv_words; > > > + unsigned int authkey_words, authiv_words, authnonce_words; > > > + unsigned long flags = rctx->flags; > > > + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; > > > > I don't see any reason to initialize encr_cfg or auth_cfg. > > right.. I will remove it > > > > > > + u32 *iv_last_word; > > > + > > > + qce_setup_config(qce); > > > + > > > + /* Write encryption key */ > > > + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); > > > + enckey_words = enc_keylen / sizeof(u32); > > > + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); > > > > Afaict all "array registers" in this function are affected by the > > CRYPTO_SETUP little endian bit, but you set this bit before launching > > the operation dependent on IS_CCM(). So is this really working for the > > !IS_CCM() case? > > I am not sure I understand you. Below , > /* get little endianness */ > config = qce_config_reg(qce, 1); > qce_write(qce, REG_CONFIG, config); > > is outside of any checks.. > You're right, I misread that snippet as I was jumping through the function. So we're unconditionally running the hardware in little endian mode. > > > > > + > > > + /* Write encryption iv */ > > > + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); > > > + enciv_words = enc_ivsize / sizeof(u32); > > > + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > > > > It would be nice if this snippet was extracted to a helper function. > > > > > + > > > + if (IS_CCM(rctx->flags)) { > > > + iv_last_word = (u32 *)&enciv[enciv_words - 1]; > > > +// qce_write(qce, REG_CNTR3_IV3, enciv[enciv_words - 1] + 1); > > > > I believe this is a remnant of the two surrounding lines. > > It indeed is.. I will remove it. > > > > > > + qce_write(qce, REG_CNTR3_IV3, (*iv_last_word) + 1); > > > > enciv is an array of big endian 32-bit integers, which you tell the > > compiler to treat as cpu-native endian, and then you do math on it. > > Afaict from the documentation the value of REG_CNTR3_IVn should be set > > to rctx->iv + 1, but if the hardware expects these in big endian then I > > think you added 16777216. > > So, the crypto engine documentation talks of writing to these registers in > little endian mode. The byte stream that you get for iv from the user > is in big endian mode as in the MSB is byte 0. So we kind of invert this and > write to these registers. This is what happens with declaring the __be32 > array and copying words to it from the byte stream. So now byte 0 is the LSB > and a +1 will just add a 1 to it. > But if the data come in big endian and after qce_cpu_to_be32p_array() you're able to do math on them with expected result and you're finally passing the data to writel() then I think that qce_cpu_to_be32p_array() is actually be32_to_cpu() and after the conversion you should carry the results in CPU-native u32 arrays - and thereby skip the typecasting. > I suspect from what I read in the documentation we could get away by > removing this and writing the big endian byte stream directly and never > setting the little endian in config register. Though I am not sure if this > has ever been tested out. If we change it, it will be across algorithms and > as a separate effort. writel() will, at least on arm64, convert the CPU native value to little endian before writing it out, so I think the current setting make sense. > > > > > Perhaps I'm missing something here though? > > > > PS. Based on how the documentation is written, shouldn't you write out > > REG_CNTR_IV[012] as well? > > It is done on top, right ? > qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > You're right, depending on enciv_words you write the 4 registers, then increment the last word and write that out again. > > > > > + qce_write_array(qce, REG_ENCR_CCM_INT_CNTR0, (u32 *)enciv, enciv_words); > > > + qce_write(qce, REG_CNTR_MASK, ~0); > > > + qce_write(qce, REG_CNTR_MASK0, ~0); > > > + qce_write(qce, REG_CNTR_MASK1, ~0); > > > + qce_write(qce, REG_CNTR_MASK2, ~0); > > > + } > > > + > > > + /* Clear authentication IV and KEY registers of previous values */ > > > + qce_clear_array(qce, REG_AUTH_IV0, 16); > > > + qce_clear_array(qce, REG_AUTH_KEY0, 16); > > > + > > > + /* Clear byte count */ > > > + qce_clear_array(qce, REG_AUTH_BYTECNT0, 4); > > > + > > > + /* Write authentication key */ > > > + qce_cpu_to_be32p_array(authkey, ctx->auth_key, auth_keylen); > > > + authkey_words = DIV_ROUND_UP(auth_keylen, sizeof(u32)); > > > + qce_write_array(qce, REG_AUTH_KEY0, (u32 *)authkey, authkey_words); > > > + > > > + if (IS_SHA_HMAC(rctx->flags)) { > > > + /* Write default authentication iv */ > > > + if (IS_SHA1_HMAC(rctx->flags)) { > > > + auth_ivsize = SHA1_DIGEST_SIZE; > > > + memcpy(authiv, std_iv_sha1, auth_ivsize); > > > + } else if (IS_SHA256_HMAC(rctx->flags)) { > > > + auth_ivsize = SHA256_DIGEST_SIZE; > > > + memcpy(authiv, std_iv_sha256, auth_ivsize); > > > + } > > > + authiv_words = auth_ivsize / sizeof(u32); > > > + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); > > > > AUTH_IV0 is affected by the little endian configuration, does this imply > > that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so > > I think it would be nice if you grouped the conditionals in a way that > > made that obvious when reading the function. > > So yes IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags. > AUTH_IVn is 0 for ccm and has initial value for HMAC algorithms. I don't > understand the confusion here. > I'm just saying that writing is as below would have made it obvious to me that IS_SHA_HMAC() and IS_CCM() are exclusive: if (IS_SHA_HMAC(flags)) { ... } else if (IS_CCM(flags)) { .... } Regards, Bjorn > > > > > + } > > > + > > > + if (IS_CCM(rctx->flags)) { > > > + qce_cpu_to_be32p_array(authnonce, rctx->ccm_nonce, QCE_MAX_NONCE); > > > + authnonce_words = QCE_MAX_NONCE / sizeof(u32); > > > + qce_write_array(qce, REG_AUTH_INFO_NONCE0, (u32 *)authnonce, authnonce_words); > > > + } > > > + > > > + /* Set up ENCR_SEG_CFG */ > > > + encr_cfg = qce_encr_cfg(flags, enc_keylen); > > > + if (IS_ENCRYPT(flags)) > > > + encr_cfg |= BIT(ENCODE_SHIFT); > > > + qce_write(qce, REG_ENCR_SEG_CFG, encr_cfg); > > > + > > > + /* Set up AUTH_SEG_CFG */ > > > + auth_cfg = qce_auth_cfg(rctx->flags, auth_keylen, ctx->authsize); > > > + auth_cfg |= BIT(AUTH_LAST_SHIFT); > > > + auth_cfg |= BIT(AUTH_FIRST_SHIFT); > > > + if (IS_ENCRYPT(flags)) { > > > + if (IS_CCM(rctx->flags)) > > > + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; > > > + else > > > + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; > > > + } else { > > > + if (IS_CCM(rctx->flags)) > > > + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; > > > + else > > > + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; > > > + } > > > + qce_write(qce, REG_AUTH_SEG_CFG, auth_cfg); > > > + > > > + totallen = rctx->cryptlen + rctx->assoclen; > > > + > > > + /* Set the encryption size and start offset */ > > > + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) > > > + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen + ctx->authsize); > > > + else > > > + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen); > > > + qce_write(qce, REG_ENCR_SEG_START, rctx->assoclen & 0xffff); > > > + > > > + /* Set the authentication size and start offset */ > > > + qce_write(qce, REG_AUTH_SEG_SIZE, totallen); > > > + qce_write(qce, REG_AUTH_SEG_START, 0); > > > + > > > + /* Write total length */ > > > + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) > > > + qce_write(qce, REG_SEG_SIZE, totallen + ctx->authsize); > > > + else > > > + qce_write(qce, REG_SEG_SIZE, totallen); > > > + > > > + /* get little endianness */ > > > + config = qce_config_reg(qce, 1); > > > + qce_write(qce, REG_CONFIG, config); > > > + > > > + /* Start the process */ > > > + if (IS_CCM(flags)) > > > + qce_crypto_go(qce, 0); > > > > Second parameter is defined as "bool", please use "false" here (and true > > below). Or > > > > qce_crypto_go(qce, !IS_CCM(flags)); > > will do... I like the one liner better. > > > -- > Warm Regards > Thara
On 4/5/21 6:18 PM, Bjorn Andersson wrote: > On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: > >> Add register programming sequence for enabling AEAD >> algorithms on the Qualcomm crypto engine. >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- >> 1 file changed, 153 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c >> index 05a71c5ecf61..54d209cb0525 100644 >> --- a/drivers/crypto/qce/common.c >> +++ b/drivers/crypto/qce/common.c >> @@ -15,6 +15,16 @@ >> #include "core.h" >> #include "regs-v5.h" >> #include "sha.h" >> +#include "aead.h" >> + >> +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { >> + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 >> +}; >> + >> +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { >> + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, >> + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 >> +}; >> >> static inline u32 qce_read(struct qce_device *qce, u32 offset) >> { >> @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) >> qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); >> } >> >> -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >> +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >> static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >> { >> u32 cfg = 0; >> @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >> >> return cfg; >> } >> +#endif >> >> +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >> static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >> { >> struct ahash_request *req = ahash_request_cast(async_req); >> @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >> } >> #endif >> >> -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >> +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >> static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >> { >> u32 cfg = 0; >> @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >> >> return cfg; >> } >> +#endif >> >> +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >> static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) >> { >> u8 swap[QCE_AES_IV_LENGTH]; >> @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) >> } >> #endif >> >> +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD >> +static int qce_setup_regs_aead(struct crypto_async_request *async_req) >> +{ >> + struct aead_request *req = aead_request_cast(async_req); >> + struct qce_aead_reqctx *rctx = aead_request_ctx(req); >> + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); >> + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); >> + struct qce_device *qce = tmpl->qce; >> + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; >> + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; >> + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; >> + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; >> + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; >> + unsigned int enc_keylen = ctx->enc_keylen; >> + unsigned int auth_keylen = ctx->auth_keylen; >> + unsigned int enc_ivsize = rctx->ivsize; >> + unsigned int auth_ivsize; >> + unsigned int enckey_words, enciv_words; >> + unsigned int authkey_words, authiv_words, authnonce_words; >> + unsigned long flags = rctx->flags; >> + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; > > I don't see any reason to initialize encr_cfg or auth_cfg. > >> + u32 *iv_last_word; >> + >> + qce_setup_config(qce); >> + >> + /* Write encryption key */ >> + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); >> + enckey_words = enc_keylen / sizeof(u32); >> + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); > > Afaict all "array registers" in this function are affected by the > CRYPTO_SETUP little endian bit, but you set this bit before launching > the operation dependent on IS_CCM(). So is this really working for the > !IS_CCM() case? > >> + >> + /* Write encryption iv */ >> + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); >> + enciv_words = enc_ivsize / sizeof(u32); >> + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > > It would be nice if this snippet was extracted to a helper function. I kind of forgot to type this earlier. So yes I agree in principle. It is more elegant to have something like qce_convert_be32_and_write and in the function do the above three steps. This snippet is prevalent in this driver code across other alogs as well (skcipher and hash). Take it up as a separate clean up activity ? -- Warm Regards Thara
On Tue 13 Apr 17:27 CDT 2021, Thara Gopinath wrote: > > > On 4/5/21 6:18 PM, Bjorn Andersson wrote: > > On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: > > > > > Add register programming sequence for enabling AEAD > > > algorithms on the Qualcomm crypto engine. > > > > > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > > --- > > > drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- > > > 1 file changed, 153 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c > > > index 05a71c5ecf61..54d209cb0525 100644 > > > --- a/drivers/crypto/qce/common.c > > > +++ b/drivers/crypto/qce/common.c > > > @@ -15,6 +15,16 @@ > > > #include "core.h" > > > #include "regs-v5.h" > > > #include "sha.h" > > > +#include "aead.h" > > > + > > > +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { > > > + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 > > > +}; > > > + > > > +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { > > > + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, > > > + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 > > > +}; > > > static inline u32 qce_read(struct qce_device *qce, u32 offset) > > > { > > > @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) > > > qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); > > > } > > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > > > +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > > > static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > > { > > > u32 cfg = 0; > > > @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > > return cfg; > > > } > > > +#endif > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > > > static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > > > { > > > struct ahash_request *req = ahash_request_cast(async_req); > > > @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > > > } > > > #endif > > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > > > +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > > > static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > > { > > > u32 cfg = 0; > > > @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > > return cfg; > > > } > > > +#endif > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > > > static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) > > > { > > > u8 swap[QCE_AES_IV_LENGTH]; > > > @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) > > > } > > > #endif > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD > > > +static int qce_setup_regs_aead(struct crypto_async_request *async_req) > > > +{ > > > + struct aead_request *req = aead_request_cast(async_req); > > > + struct qce_aead_reqctx *rctx = aead_request_ctx(req); > > > + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); > > > + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); > > > + struct qce_device *qce = tmpl->qce; > > > + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; > > > + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; > > > + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; > > > + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; > > > + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; > > > + unsigned int enc_keylen = ctx->enc_keylen; > > > + unsigned int auth_keylen = ctx->auth_keylen; > > > + unsigned int enc_ivsize = rctx->ivsize; > > > + unsigned int auth_ivsize; > > > + unsigned int enckey_words, enciv_words; > > > + unsigned int authkey_words, authiv_words, authnonce_words; > > > + unsigned long flags = rctx->flags; > > > + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; > > > > I don't see any reason to initialize encr_cfg or auth_cfg. > > > > > + u32 *iv_last_word; > > > + > > > + qce_setup_config(qce); > > > + > > > + /* Write encryption key */ > > > + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); > > > + enckey_words = enc_keylen / sizeof(u32); > > > + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); > > > > Afaict all "array registers" in this function are affected by the > > CRYPTO_SETUP little endian bit, but you set this bit before launching > > the operation dependent on IS_CCM(). So is this really working for the > > !IS_CCM() case? > > > > > + > > > + /* Write encryption iv */ > > > + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); > > > + enciv_words = enc_ivsize / sizeof(u32); > > > + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > > > > It would be nice if this snippet was extracted to a helper function. > > I kind of forgot to type this earlier. So yes I agree in principle. > It is more elegant to have something like qce_convert_be32_and_write > and in the function do the above three steps. This snippet is prevalent in > this driver code across other alogs as well (skcipher and hash). Perhaps make qce_cpu_to_be32p_array() (or qce_be32_to_cpu() per the other reply), could return the number of words written - that way you remove the ugly middle line at least - and can still poke at the data when needed. > Take it up as a separate clean up activity ? > Yes, that seems like a good idea. Regards, Bjorn
On 4/13/21 6:20 PM, Bjorn Andersson wrote: > On Tue 13 Apr 16:31 CDT 2021, Thara Gopinath wrote: > >> >> Hi Bjorn, >> >> On 4/5/21 6:18 PM, Bjorn Andersson wrote: >>> On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: >>> >>>> Add register programming sequence for enabling AEAD >>>> algorithms on the Qualcomm crypto engine. >>>> >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >>>> --- >>>> drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 153 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c >>>> index 05a71c5ecf61..54d209cb0525 100644 >>>> --- a/drivers/crypto/qce/common.c >>>> +++ b/drivers/crypto/qce/common.c >>>> @@ -15,6 +15,16 @@ >>>> #include "core.h" >>>> #include "regs-v5.h" >>>> #include "sha.h" >>>> +#include "aead.h" >>>> + >>>> +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { >>>> + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 >>>> +}; >>>> + >>>> +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { >>>> + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, >>>> + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 >>>> +}; >>>> static inline u32 qce_read(struct qce_device *qce, u32 offset) >>>> { >>>> @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) >>>> qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); >>>> } >>>> -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >>>> +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >>>> static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >>>> { >>>> u32 cfg = 0; >>>> @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >>>> return cfg; >>>> } >>>> +#endif >>>> +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >>>> static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >>>> { >>>> struct ahash_request *req = ahash_request_cast(async_req); >>>> @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >>>> } >>>> #endif >>>> -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >>>> +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >>>> static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >>>> { >>>> u32 cfg = 0; >>>> @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >>>> return cfg; >>>> } >>>> +#endif >>>> +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >>>> static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) >>>> { >>>> u8 swap[QCE_AES_IV_LENGTH]; >>>> @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) >>>> } >>>> #endif >>>> +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD >>>> +static int qce_setup_regs_aead(struct crypto_async_request *async_req) >>>> +{ >>>> + struct aead_request *req = aead_request_cast(async_req); >>>> + struct qce_aead_reqctx *rctx = aead_request_ctx(req); >>>> + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); >>>> + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); >>>> + struct qce_device *qce = tmpl->qce; >>>> + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; >>>> + unsigned int enc_keylen = ctx->enc_keylen; >>>> + unsigned int auth_keylen = ctx->auth_keylen; >>>> + unsigned int enc_ivsize = rctx->ivsize; >>>> + unsigned int auth_ivsize; >>>> + unsigned int enckey_words, enciv_words; >>>> + unsigned int authkey_words, authiv_words, authnonce_words; >>>> + unsigned long flags = rctx->flags; >>>> + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; >>> >>> I don't see any reason to initialize encr_cfg or auth_cfg. >> >> right.. I will remove it >> >>> >>>> + u32 *iv_last_word; >>>> + >>>> + qce_setup_config(qce); >>>> + >>>> + /* Write encryption key */ >>>> + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); >>>> + enckey_words = enc_keylen / sizeof(u32); >>>> + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); >>> >>> Afaict all "array registers" in this function are affected by the >>> CRYPTO_SETUP little endian bit, but you set this bit before launching >>> the operation dependent on IS_CCM(). So is this really working for the >>> !IS_CCM() case? >> >> I am not sure I understand you. Below , >> /* get little endianness */ >> config = qce_config_reg(qce, 1); >> qce_write(qce, REG_CONFIG, config); >> >> is outside of any checks.. >> > > You're right, I misread that snippet as I was jumping through the > function. So we're unconditionally running the hardware in little endian > mode. > >>> >>>> + >>>> + /* Write encryption iv */ >>>> + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); >>>> + enciv_words = enc_ivsize / sizeof(u32); >>>> + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); >>> >>> It would be nice if this snippet was extracted to a helper function. >>> >>>> + >>>> + if (IS_CCM(rctx->flags)) { >>>> + iv_last_word = (u32 *)&enciv[enciv_words - 1]; >>>> +// qce_write(qce, REG_CNTR3_IV3, enciv[enciv_words - 1] + 1); >>> >>> I believe this is a remnant of the two surrounding lines. >> >> It indeed is.. I will remove it. >> >>> >>>> + qce_write(qce, REG_CNTR3_IV3, (*iv_last_word) + 1); >>> >>> enciv is an array of big endian 32-bit integers, which you tell the >>> compiler to treat as cpu-native endian, and then you do math on it. >>> Afaict from the documentation the value of REG_CNTR3_IVn should be set >>> to rctx->iv + 1, but if the hardware expects these in big endian then I >>> think you added 16777216. >> >> So, the crypto engine documentation talks of writing to these registers in >> little endian mode. The byte stream that you get for iv from the user >> is in big endian mode as in the MSB is byte 0. So we kind of invert this and >> write to these registers. This is what happens with declaring the __be32 >> array and copying words to it from the byte stream. So now byte 0 is the LSB >> and a +1 will just add a 1 to it. >> > > But if the data come in big endian and after qce_cpu_to_be32p_array() > you're able to do math on them with expected result and you're finally > passing the data to writel() then I think that qce_cpu_to_be32p_array() > is actually be32_to_cpu() and after the conversion you should carry the > results in CPU-native u32 arrays - and thereby skip the typecasting. you mean I can replace __be32 arrays with u32 arrays ?? yes I probably can. I will try this out and if it works do the change. > >> I suspect from what I read in the documentation we could get away by >> removing this and writing the big endian byte stream directly and never >> setting the little endian in config register. Though I am not sure if this >> has ever been tested out. If we change it, it will be across algorithms and >> as a separate effort. > > writel() will, at least on arm64, convert the CPU native value to little > endian before writing it out, so I think the current setting make sense. hmm.. ok. > >> >>> >>> Perhaps I'm missing something here though? >>> >>> PS. Based on how the documentation is written, shouldn't you write out >>> REG_CNTR_IV[012] as well? >> >> It is done on top, right ? >> qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); >> > > You're right, depending on enciv_words you write the 4 registers, then > increment the last word and write that out again. > >>> >>>> + qce_write_array(qce, REG_ENCR_CCM_INT_CNTR0, (u32 *)enciv, enciv_words); >>>> + qce_write(qce, REG_CNTR_MASK, ~0); >>>> + qce_write(qce, REG_CNTR_MASK0, ~0); >>>> + qce_write(qce, REG_CNTR_MASK1, ~0); >>>> + qce_write(qce, REG_CNTR_MASK2, ~0); >>>> + } >>>> + >>>> + /* Clear authentication IV and KEY registers of previous values */ >>>> + qce_clear_array(qce, REG_AUTH_IV0, 16); >>>> + qce_clear_array(qce, REG_AUTH_KEY0, 16); >>>> + >>>> + /* Clear byte count */ >>>> + qce_clear_array(qce, REG_AUTH_BYTECNT0, 4); >>>> + >>>> + /* Write authentication key */ >>>> + qce_cpu_to_be32p_array(authkey, ctx->auth_key, auth_keylen); >>>> + authkey_words = DIV_ROUND_UP(auth_keylen, sizeof(u32)); >>>> + qce_write_array(qce, REG_AUTH_KEY0, (u32 *)authkey, authkey_words); >>>> + >>>> + if (IS_SHA_HMAC(rctx->flags)) { >>>> + /* Write default authentication iv */ >>>> + if (IS_SHA1_HMAC(rctx->flags)) { >>>> + auth_ivsize = SHA1_DIGEST_SIZE; >>>> + memcpy(authiv, std_iv_sha1, auth_ivsize); >>>> + } else if (IS_SHA256_HMAC(rctx->flags)) { >>>> + auth_ivsize = SHA256_DIGEST_SIZE; >>>> + memcpy(authiv, std_iv_sha256, auth_ivsize); >>>> + } >>>> + authiv_words = auth_ivsize / sizeof(u32); >>>> + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); >>> >>> AUTH_IV0 is affected by the little endian configuration, does this imply >>> that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so >>> I think it would be nice if you grouped the conditionals in a way that >>> made that obvious when reading the function. >> >> So yes IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags. >> AUTH_IVn is 0 for ccm and has initial value for HMAC algorithms. I don't >> understand the confusion here. >> > > I'm just saying that writing is as below would have made it obvious to > me that IS_SHA_HMAC() and IS_CCM() are exclusive: So regardless of the mode, it is a good idea to clear the IV registers which happens above in qce_clear_array(qce, REG_AUTH_IV0, 16); This is important becasue the size of IV varies between HMAC(SHA1) and HMAC(SHA256) and we don't want any previous bits sticking around. For CCM after the above step we don't do anything with AUTH_IV where as for SHA_HMAC we have to go and program the registers. I can split it into if (IS_SHA_HMAC(flags)) { ... } else { ... } but both snippets will have the above line code clearing the IV register and the if part will have more stuff actually programming these registers.. Is that what you are looking for ? -- Warm Regards Thara
On 4/13/21 6:33 PM, Bjorn Andersson wrote: > On Tue 13 Apr 17:27 CDT 2021, Thara Gopinath wrote: > >> >> >> On 4/5/21 6:18 PM, Bjorn Andersson wrote: >>> On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: >>> >>>> Add register programming sequence for enabling AEAD >>>> algorithms on the Qualcomm crypto engine. >>>> >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >>>> --- >>>> drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 153 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c >>>> index 05a71c5ecf61..54d209cb0525 100644 >>>> --- a/drivers/crypto/qce/common.c >>>> +++ b/drivers/crypto/qce/common.c >>>> @@ -15,6 +15,16 @@ >>>> #include "core.h" >>>> #include "regs-v5.h" >>>> #include "sha.h" >>>> +#include "aead.h" >>>> + >>>> +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { >>>> + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 >>>> +}; >>>> + >>>> +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { >>>> + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, >>>> + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 >>>> +}; >>>> static inline u32 qce_read(struct qce_device *qce, u32 offset) >>>> { >>>> @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) >>>> qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); >>>> } >>>> -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >>>> +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >>>> static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >>>> { >>>> u32 cfg = 0; >>>> @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) >>>> return cfg; >>>> } >>>> +#endif >>>> +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA >>>> static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >>>> { >>>> struct ahash_request *req = ahash_request_cast(async_req); >>>> @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) >>>> } >>>> #endif >>>> -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >>>> +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) >>>> static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >>>> { >>>> u32 cfg = 0; >>>> @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) >>>> return cfg; >>>> } >>>> +#endif >>>> +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER >>>> static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) >>>> { >>>> u8 swap[QCE_AES_IV_LENGTH]; >>>> @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) >>>> } >>>> #endif >>>> +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD >>>> +static int qce_setup_regs_aead(struct crypto_async_request *async_req) >>>> +{ >>>> + struct aead_request *req = aead_request_cast(async_req); >>>> + struct qce_aead_reqctx *rctx = aead_request_ctx(req); >>>> + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); >>>> + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); >>>> + struct qce_device *qce = tmpl->qce; >>>> + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; >>>> + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; >>>> + unsigned int enc_keylen = ctx->enc_keylen; >>>> + unsigned int auth_keylen = ctx->auth_keylen; >>>> + unsigned int enc_ivsize = rctx->ivsize; >>>> + unsigned int auth_ivsize; >>>> + unsigned int enckey_words, enciv_words; >>>> + unsigned int authkey_words, authiv_words, authnonce_words; >>>> + unsigned long flags = rctx->flags; >>>> + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; >>> >>> I don't see any reason to initialize encr_cfg or auth_cfg. >>> >>>> + u32 *iv_last_word; >>>> + >>>> + qce_setup_config(qce); >>>> + >>>> + /* Write encryption key */ >>>> + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); >>>> + enckey_words = enc_keylen / sizeof(u32); >>>> + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); >>> >>> Afaict all "array registers" in this function are affected by the >>> CRYPTO_SETUP little endian bit, but you set this bit before launching >>> the operation dependent on IS_CCM(). So is this really working for the >>> !IS_CCM() case? >>> >>>> + >>>> + /* Write encryption iv */ >>>> + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); >>>> + enciv_words = enc_ivsize / sizeof(u32); >>>> + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); >>> >>> It would be nice if this snippet was extracted to a helper function. >> >> I kind of forgot to type this earlier. So yes I agree in principle. >> It is more elegant to have something like qce_convert_be32_and_write >> and in the function do the above three steps. This snippet is prevalent in >> this driver code across other alogs as well (skcipher and hash). > > Perhaps make qce_cpu_to_be32p_array() (or qce_be32_to_cpu() per the > other reply), could return the number of words written - that way you > remove the ugly middle line at least - and can still poke at the data > when needed. Right. I will note this down for clean up as it touches other stuff as well. > >> Take it up as a separate clean up activity ? >> > > Yes, that seems like a good idea. > > Regards, > Bjorn > -- Warm Regards Thara
On Tue 13 Apr 17:44 CDT 2021, Thara Gopinath wrote: > > > On 4/13/21 6:20 PM, Bjorn Andersson wrote: > > On Tue 13 Apr 16:31 CDT 2021, Thara Gopinath wrote: > > > > > > > > Hi Bjorn, > > > > > > On 4/5/21 6:18 PM, Bjorn Andersson wrote: > > > > On Thu 25 Feb 12:27 CST 2021, Thara Gopinath wrote: > > > > > > > > > Add register programming sequence for enabling AEAD > > > > > algorithms on the Qualcomm crypto engine. > > > > > > > > > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > > > > --- > > > > > drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 153 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c > > > > > index 05a71c5ecf61..54d209cb0525 100644 > > > > > --- a/drivers/crypto/qce/common.c > > > > > +++ b/drivers/crypto/qce/common.c > > > > > @@ -15,6 +15,16 @@ > > > > > #include "core.h" > > > > > #include "regs-v5.h" > > > > > #include "sha.h" > > > > > +#include "aead.h" > > > > > + > > > > > +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { > > > > > + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 > > > > > +}; > > > > > + > > > > > +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { > > > > > + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, > > > > > + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 > > > > > +}; > > > > > static inline u32 qce_read(struct qce_device *qce, u32 offset) > > > > > { > > > > > @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) > > > > > qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); > > > > > } > > > > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > > > > > +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > > > > > static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > > > > { > > > > > u32 cfg = 0; > > > > > @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) > > > > > return cfg; > > > > > } > > > > > +#endif > > > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA > > > > > static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > > > > > { > > > > > struct ahash_request *req = ahash_request_cast(async_req); > > > > > @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) > > > > > } > > > > > #endif > > > > > -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > > > > > +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) > > > > > static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > > > > { > > > > > u32 cfg = 0; > > > > > @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) > > > > > return cfg; > > > > > } > > > > > +#endif > > > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER > > > > > static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) > > > > > { > > > > > u8 swap[QCE_AES_IV_LENGTH]; > > > > > @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) > > > > > } > > > > > #endif > > > > > +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD > > > > > +static int qce_setup_regs_aead(struct crypto_async_request *async_req) > > > > > +{ > > > > > + struct aead_request *req = aead_request_cast(async_req); > > > > > + struct qce_aead_reqctx *rctx = aead_request_ctx(req); > > > > > + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); > > > > > + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); > > > > > + struct qce_device *qce = tmpl->qce; > > > > > + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; > > > > > + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; > > > > > + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; > > > > > + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; > > > > > + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; > > > > > + unsigned int enc_keylen = ctx->enc_keylen; > > > > > + unsigned int auth_keylen = ctx->auth_keylen; > > > > > + unsigned int enc_ivsize = rctx->ivsize; > > > > > + unsigned int auth_ivsize; > > > > > + unsigned int enckey_words, enciv_words; > > > > > + unsigned int authkey_words, authiv_words, authnonce_words; > > > > > + unsigned long flags = rctx->flags; > > > > > + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; > > > > > > > > I don't see any reason to initialize encr_cfg or auth_cfg. > > > > > > right.. I will remove it > > > > > > > > > > > > + u32 *iv_last_word; > > > > > + > > > > > + qce_setup_config(qce); > > > > > + > > > > > + /* Write encryption key */ > > > > > + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); > > > > > + enckey_words = enc_keylen / sizeof(u32); > > > > > + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); > > > > > > > > Afaict all "array registers" in this function are affected by the > > > > CRYPTO_SETUP little endian bit, but you set this bit before launching > > > > the operation dependent on IS_CCM(). So is this really working for the > > > > !IS_CCM() case? > > > > > > I am not sure I understand you. Below , > > > /* get little endianness */ > > > config = qce_config_reg(qce, 1); > > > qce_write(qce, REG_CONFIG, config); > > > > > > is outside of any checks.. > > > > > > > You're right, I misread that snippet as I was jumping through the > > function. So we're unconditionally running the hardware in little endian > > mode. > > > > > > > > > > > + > > > > > + /* Write encryption iv */ > > > > > + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); > > > > > + enciv_words = enc_ivsize / sizeof(u32); > > > > > + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); > > > > > > > > It would be nice if this snippet was extracted to a helper function. > > > > > > > > > + > > > > > + if (IS_CCM(rctx->flags)) { > > > > > + iv_last_word = (u32 *)&enciv[enciv_words - 1]; > > > > > +// qce_write(qce, REG_CNTR3_IV3, enciv[enciv_words - 1] + 1); > > > > > > > > I believe this is a remnant of the two surrounding lines. > > > > > > It indeed is.. I will remove it. > > > > > > > > > > > > + qce_write(qce, REG_CNTR3_IV3, (*iv_last_word) + 1); > > > > > > > > enciv is an array of big endian 32-bit integers, which you tell the > > > > compiler to treat as cpu-native endian, and then you do math on it. > > > > Afaict from the documentation the value of REG_CNTR3_IVn should be set > > > > to rctx->iv + 1, but if the hardware expects these in big endian then I > > > > think you added 16777216. > > > > > > So, the crypto engine documentation talks of writing to these registers in > > > little endian mode. The byte stream that you get for iv from the user > > > is in big endian mode as in the MSB is byte 0. So we kind of invert this and > > > write to these registers. This is what happens with declaring the __be32 > > > array and copying words to it from the byte stream. So now byte 0 is the LSB > > > and a +1 will just add a 1 to it. > > > > > > > But if the data come in big endian and after qce_cpu_to_be32p_array() > > you're able to do math on them with expected result and you're finally > > passing the data to writel() then I think that qce_cpu_to_be32p_array() > > is actually be32_to_cpu() and after the conversion you should carry the > > results in CPU-native u32 arrays - and thereby skip the typecasting. > > you mean I can replace __be32 arrays with u32 arrays ?? yes I probably can. > I will try this out and if it works do the change. > Yes, given that you just typecast things as you do it should just work to move the typecast to the qce_cpu_to_be32p_array(). But as I said, this would indicate that what is cpu_to_be32() should have been be32_to_cpu() (both performs the same swap, it's just a matter of what type goes in and what type goes out). Looking at the other uses of qce_cpu_to_be32p_array() I suspect that it's the same situation there, so perhaps introduce a new function qce_be32_to_cpu() in this patchset (that returns number of words converted) and then look into the existing users after that? [..] > > > > > + if (IS_SHA_HMAC(rctx->flags)) { > > > > > + /* Write default authentication iv */ > > > > > + if (IS_SHA1_HMAC(rctx->flags)) { > > > > > + auth_ivsize = SHA1_DIGEST_SIZE; > > > > > + memcpy(authiv, std_iv_sha1, auth_ivsize); > > > > > + } else if (IS_SHA256_HMAC(rctx->flags)) { > > > > > + auth_ivsize = SHA256_DIGEST_SIZE; > > > > > + memcpy(authiv, std_iv_sha256, auth_ivsize); > > > > > + } > > > > > + authiv_words = auth_ivsize / sizeof(u32); > > > > > + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); > > > > > > > > AUTH_IV0 is affected by the little endian configuration, does this imply > > > > that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so > > > > I think it would be nice if you grouped the conditionals in a way that > > > > made that obvious when reading the function. > > > > > > So yes IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags. > > > AUTH_IVn is 0 for ccm and has initial value for HMAC algorithms. I don't > > > understand the confusion here. > > > > > > > I'm just saying that writing is as below would have made it obvious to > > me that IS_SHA_HMAC() and IS_CCM() are exclusive: > > So regardless of the mode, it is a good idea to clear the IV registers > which happens above in > > qce_clear_array(qce, REG_AUTH_IV0, 16); > > > This is important becasue the size of IV varies between HMAC(SHA1) and > HMAC(SHA256) and we don't want any previous bits sticking around. > For CCM after the above step we don't do anything with AUTH_IV where as for > SHA_HMAC we have to go and program the registers. I can split it into > if (IS_SHA_HMAC(flags)) { > ... > } else { > ... > } > > but both snippets will have the above line code clearing the IV register and > the if part will have more stuff actually programming these registers.. Is > that what you are looking for ? I didn't find an answer quickly to the question if the two where mutually exclusive and couldn't determine from the code flow either. But my comment seems to stem from my misunderstanding that the little endian bit was dependent on IS_CCM(). That said, if the logic really is "do this for IS_SHA_HMAC(), otherwise do that", then if else makes sense. Regards, Bjorn
On 4/13/21 7:09 PM, Bjorn Andersson wrote: > On Tue 13 Apr 17:44 CDT 2021, Thara Gopinath wrote: [..] > > Yes, given that you just typecast things as you do it should just work > to move the typecast to the qce_cpu_to_be32p_array(). > > But as I said, this would indicate that what is cpu_to_be32() should > have been be32_to_cpu() (both performs the same swap, it's just a matter > of what type goes in and what type goes out). > > Looking at the other uses of qce_cpu_to_be32p_array() I suspect that > it's the same situation there, so perhaps introduce a new function > qce_be32_to_cpu() in this patchset (that returns number of words > converted) and then look into the existing users after that? Hi! I have sent out the v2 with the new function. To me, it does look cleaner. So thanks! > > [..] >>>>>> + if (IS_SHA_HMAC(rctx->flags)) { >>>>>> + /* Write default authentication iv */ >>>>>> + if (IS_SHA1_HMAC(rctx->flags)) { >>>>>> + auth_ivsize = SHA1_DIGEST_SIZE; >>>>>> + memcpy(authiv, std_iv_sha1, auth_ivsize); >>>>>> + } else if (IS_SHA256_HMAC(rctx->flags)) { >>>>>> + auth_ivsize = SHA256_DIGEST_SIZE; >>>>>> + memcpy(authiv, std_iv_sha256, auth_ivsize); >>>>>> + } >>>>>> + authiv_words = auth_ivsize / sizeof(u32); >>>>>> + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); >>>>> >>>>> AUTH_IV0 is affected by the little endian configuration, does this imply >>>>> that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so >>>>> I think it would be nice if you grouped the conditionals in a way that >>>>> made that obvious when reading the function. >>>> >>>> So yes IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags. >>>> AUTH_IVn is 0 for ccm and has initial value for HMAC algorithms. I don't >>>> understand the confusion here. >>>> >>> >>> I'm just saying that writing is as below would have made it obvious to >>> me that IS_SHA_HMAC() and IS_CCM() are exclusive: >> >> So regardless of the mode, it is a good idea to clear the IV registers >> which happens above in >> >> qce_clear_array(qce, REG_AUTH_IV0, 16); >> >> >> This is important becasue the size of IV varies between HMAC(SHA1) and >> HMAC(SHA256) and we don't want any previous bits sticking around. >> For CCM after the above step we don't do anything with AUTH_IV where as for >> SHA_HMAC we have to go and program the registers. I can split it into >> if (IS_SHA_HMAC(flags)) { >> ... >> } else { >> ... >> } >> >> but both snippets will have the above line code clearing the IV register and >> the if part will have more stuff actually programming these registers.. Is >> that what you are looking for ? > > I didn't find an answer quickly to the question if the two where > mutually exclusive and couldn't determine from the code flow either. But > my comment seems to stem from my misunderstanding that the little endian > bit was dependent on IS_CCM(). > > That said, if the logic really is "do this for IS_SHA_HMAC(), otherwise > do that", then if else makes sense. So, the logic is really. do "clearing of IV" in all cases. Do setting of initial IV only for HMAC. I tried the if..else like you said. It did not look correct to duplicate the code clearing the IV. So I have added comments around this section hopefully making it clearer. Do take a look and let me know. I can rework it further if you think so. > > Regards, > Bjorn > -- Warm Regards Thara
diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c index 05a71c5ecf61..54d209cb0525 100644 --- a/drivers/crypto/qce/common.c +++ b/drivers/crypto/qce/common.c @@ -15,6 +15,16 @@ #include "core.h" #include "regs-v5.h" #include "sha.h" +#include "aead.h" + +static const u32 std_iv_sha1[SHA256_DIGEST_SIZE / sizeof(u32)] = { + SHA1_H0, SHA1_H1, SHA1_H2, SHA1_H3, SHA1_H4, 0, 0, 0 +}; + +static const u32 std_iv_sha256[SHA256_DIGEST_SIZE / sizeof(u32)] = { + SHA256_H0, SHA256_H1, SHA256_H2, SHA256_H3, + SHA256_H4, SHA256_H5, SHA256_H6, SHA256_H7 +}; static inline u32 qce_read(struct qce_device *qce, u32 offset) { @@ -96,7 +106,7 @@ static inline void qce_crypto_go(struct qce_device *qce, bool result_dump) qce_write(qce, REG_GOPROC, BIT(GO_SHIFT)); } -#ifdef CONFIG_CRYPTO_DEV_QCE_SHA +#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) { u32 cfg = 0; @@ -139,7 +149,9 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size, u32 auth_size) return cfg; } +#endif +#ifdef CONFIG_CRYPTO_DEV_QCE_SHA static int qce_setup_regs_ahash(struct crypto_async_request *async_req) { struct ahash_request *req = ahash_request_cast(async_req); @@ -225,7 +237,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req) } #endif -#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER +#if defined(CONFIG_CRYPTO_DEV_QCE_SKCIPHER) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD) static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) { u32 cfg = 0; @@ -271,7 +283,9 @@ static u32 qce_encr_cfg(unsigned long flags, u32 aes_key_size) return cfg; } +#endif +#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER static void qce_xts_swapiv(__be32 *dst, const u8 *src, unsigned int ivsize) { u8 swap[QCE_AES_IV_LENGTH]; @@ -386,6 +400,139 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req) } #endif +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD +static int qce_setup_regs_aead(struct crypto_async_request *async_req) +{ + struct aead_request *req = aead_request_cast(async_req); + struct qce_aead_reqctx *rctx = aead_request_ctx(req); + struct qce_aead_ctx *ctx = crypto_tfm_ctx(async_req->tfm); + struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req)); + struct qce_device *qce = tmpl->qce; + __be32 enckey[QCE_MAX_CIPHER_KEY_SIZE / sizeof(__be32)] = {0}; + __be32 enciv[QCE_MAX_IV_SIZE / sizeof(__be32)] = {0}; + __be32 authkey[QCE_SHA_HMAC_KEY_SIZE / sizeof(__be32)] = {0}; + __be32 authiv[SHA256_DIGEST_SIZE / sizeof(__be32)] = {0}; + __be32 authnonce[QCE_MAX_NONCE / sizeof(__be32)] = {0}; + unsigned int enc_keylen = ctx->enc_keylen; + unsigned int auth_keylen = ctx->auth_keylen; + unsigned int enc_ivsize = rctx->ivsize; + unsigned int auth_ivsize; + unsigned int enckey_words, enciv_words; + unsigned int authkey_words, authiv_words, authnonce_words; + unsigned long flags = rctx->flags; + u32 encr_cfg = 0, auth_cfg = 0, config, totallen; + u32 *iv_last_word; + + qce_setup_config(qce); + + /* Write encryption key */ + qce_cpu_to_be32p_array(enckey, ctx->enc_key, enc_keylen); + enckey_words = enc_keylen / sizeof(u32); + qce_write_array(qce, REG_ENCR_KEY0, (u32 *)enckey, enckey_words); + + /* Write encryption iv */ + qce_cpu_to_be32p_array(enciv, rctx->iv, enc_ivsize); + enciv_words = enc_ivsize / sizeof(u32); + qce_write_array(qce, REG_CNTR0_IV0, (u32 *)enciv, enciv_words); + + if (IS_CCM(rctx->flags)) { + iv_last_word = (u32 *)&enciv[enciv_words - 1]; +// qce_write(qce, REG_CNTR3_IV3, enciv[enciv_words - 1] + 1); + qce_write(qce, REG_CNTR3_IV3, (*iv_last_word) + 1); + qce_write_array(qce, REG_ENCR_CCM_INT_CNTR0, (u32 *)enciv, enciv_words); + qce_write(qce, REG_CNTR_MASK, ~0); + qce_write(qce, REG_CNTR_MASK0, ~0); + qce_write(qce, REG_CNTR_MASK1, ~0); + qce_write(qce, REG_CNTR_MASK2, ~0); + } + + /* Clear authentication IV and KEY registers of previous values */ + qce_clear_array(qce, REG_AUTH_IV0, 16); + qce_clear_array(qce, REG_AUTH_KEY0, 16); + + /* Clear byte count */ + qce_clear_array(qce, REG_AUTH_BYTECNT0, 4); + + /* Write authentication key */ + qce_cpu_to_be32p_array(authkey, ctx->auth_key, auth_keylen); + authkey_words = DIV_ROUND_UP(auth_keylen, sizeof(u32)); + qce_write_array(qce, REG_AUTH_KEY0, (u32 *)authkey, authkey_words); + + if (IS_SHA_HMAC(rctx->flags)) { + /* Write default authentication iv */ + if (IS_SHA1_HMAC(rctx->flags)) { + auth_ivsize = SHA1_DIGEST_SIZE; + memcpy(authiv, std_iv_sha1, auth_ivsize); + } else if (IS_SHA256_HMAC(rctx->flags)) { + auth_ivsize = SHA256_DIGEST_SIZE; + memcpy(authiv, std_iv_sha256, auth_ivsize); + } + authiv_words = auth_ivsize / sizeof(u32); + qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words); + } + + if (IS_CCM(rctx->flags)) { + qce_cpu_to_be32p_array(authnonce, rctx->ccm_nonce, QCE_MAX_NONCE); + authnonce_words = QCE_MAX_NONCE / sizeof(u32); + qce_write_array(qce, REG_AUTH_INFO_NONCE0, (u32 *)authnonce, authnonce_words); + } + + /* Set up ENCR_SEG_CFG */ + encr_cfg = qce_encr_cfg(flags, enc_keylen); + if (IS_ENCRYPT(flags)) + encr_cfg |= BIT(ENCODE_SHIFT); + qce_write(qce, REG_ENCR_SEG_CFG, encr_cfg); + + /* Set up AUTH_SEG_CFG */ + auth_cfg = qce_auth_cfg(rctx->flags, auth_keylen, ctx->authsize); + auth_cfg |= BIT(AUTH_LAST_SHIFT); + auth_cfg |= BIT(AUTH_FIRST_SHIFT); + if (IS_ENCRYPT(flags)) { + if (IS_CCM(rctx->flags)) + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; + else + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; + } else { + if (IS_CCM(rctx->flags)) + auth_cfg |= AUTH_POS_AFTER << AUTH_POS_SHIFT; + else + auth_cfg |= AUTH_POS_BEFORE << AUTH_POS_SHIFT; + } + qce_write(qce, REG_AUTH_SEG_CFG, auth_cfg); + + totallen = rctx->cryptlen + rctx->assoclen; + + /* Set the encryption size and start offset */ + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen + ctx->authsize); + else + qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen); + qce_write(qce, REG_ENCR_SEG_START, rctx->assoclen & 0xffff); + + /* Set the authentication size and start offset */ + qce_write(qce, REG_AUTH_SEG_SIZE, totallen); + qce_write(qce, REG_AUTH_SEG_START, 0); + + /* Write total length */ + if (IS_CCM(rctx->flags) && IS_DECRYPT(rctx->flags)) + qce_write(qce, REG_SEG_SIZE, totallen + ctx->authsize); + else + qce_write(qce, REG_SEG_SIZE, totallen); + + /* get little endianness */ + config = qce_config_reg(qce, 1); + qce_write(qce, REG_CONFIG, config); + + /* Start the process */ + if (IS_CCM(flags)) + qce_crypto_go(qce, 0); + else + qce_crypto_go(qce, 1); + + return 0; +} +#endif + int qce_start(struct crypto_async_request *async_req, u32 type) { switch (type) { @@ -396,6 +543,10 @@ int qce_start(struct crypto_async_request *async_req, u32 type) #ifdef CONFIG_CRYPTO_DEV_QCE_SHA case CRYPTO_ALG_TYPE_AHASH: return qce_setup_regs_ahash(async_req); +#endif +#ifdef CONFIG_CRYPTO_DEV_QCE_AEAD + case CRYPTO_ALG_TYPE_AEAD: + return qce_setup_regs_aead(async_req); #endif default: return -EINVAL;
Add register programming sequence for enabling AEAD algorithms on the Qualcomm crypto engine. Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> --- drivers/crypto/qce/common.c | 155 +++++++++++++++++++++++++++++++++++- 1 file changed, 153 insertions(+), 2 deletions(-) -- 2.25.1