From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akhil Goyal Subject: Re: [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform Date: Tue, 3 Oct 2017 14:15:14 +0530 Message-ID: <9c208292-0ddc-79f5-fbfb-4d003ad07224@nxp.com> References: <20170824000117.32186-1-akhil.goyal@nxp.com> <20170824000117.32186-3-akhil.goyal@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: "Doherty, Declan" , "Mcnamara, John" , "hemant.agrawal@nxp.com" To: "De Lara Guarch, Pablo" , "dev@dpdk.org" Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0049.outbound.protection.outlook.com [104.47.41.49]) by dpdk.org (Postfix) with ESMTP id 248251B2E9 for ; Tue, 3 Oct 2017 10:45:22 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Pablo, On 9/18/2017 11:41 PM, De Lara Guarch, Pablo wrote: > > >> -----Original Message----- >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com] >> Sent: Thursday, August 24, 2017 1:01 AM >> To: dev@dpdk.org; De Lara Guarch, Pablo >> >> Cc: Doherty, Declan ; Mcnamara, John >> ; hemant.agrawal@nxp.com; Akhil Goyal >> >> Subject: [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA >> platform >> >> Signed-off-by: Forrest Shi >> Signed-off-by: Akhil Goyal >> Signed-off-by: Hemant Agrawal >> --- >> MAINTAINERS | 5 + >> config/common_base | 8 + >> config/defconfig_arm64-dpaa-linuxapp-gcc | 14 + >> drivers/Makefile | 2 +- >> drivers/crypto/Makefile | 2 + >> drivers/crypto/dpaa_sec/Makefile | 71 + >> drivers/crypto/dpaa_sec/dpaa_sec.c | 1552 >> ++++++++++++++++++++ >> drivers/crypto/dpaa_sec/dpaa_sec.h | 403 +++++ >> drivers/crypto/dpaa_sec/dpaa_sec_log.h | 70 + >> .../crypto/dpaa_sec/rte_pmd_dpaa_sec_version.map | 4 + >> mk/rte.app.mk | 6 + >> 11 files changed, 2136 insertions(+), 1 deletion(-) >> create mode 100644 drivers/crypto/dpaa_sec/Makefile >> create mode 100644 drivers/crypto/dpaa_sec/dpaa_sec.c >> create mode 100644 drivers/crypto/dpaa_sec/dpaa_sec.h >> create mode 100644 drivers/crypto/dpaa_sec/dpaa_sec_log.h >> create mode 100644 >> drivers/crypto/dpaa_sec/rte_pmd_dpaa_sec_version.map >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 48afbfc..24b3b41 100644 > > ... > >> + >> +include $(RTE_SDK)/mk/rte.lib.mk >> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c >> b/drivers/crypto/dpaa_sec/dpaa_sec.c >> new file mode 100644 >> index 0000000..c8f8be9 >> --- /dev/null >> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c >> @@ -0,0 +1,1552 @@ >> +/*- >> + * BSD LICENSE >> + * > > ... > >> + >> +static inline struct dpaa_sec_op_ctx * >> +dpaa_sec_alloc_ctx(dpaa_sec_session *ses) >> +{ >> + struct dpaa_sec_op_ctx *ctx; >> + int retval; >> + >> + retval = rte_mempool_get(ses->ctx_pool, (void **)(&ctx)); >> + if (!ctx || retval) { >> + PMD_TX_LOG(ERR, "Alloc sec descriptor failed!"); >> + return NULL; >> + } >> + dcbz_64(&ctx->job.sg[0]); >> + dcbz_64(&ctx->job.sg[5]); >> + dcbz_64(&ctx->job.sg[9]); >> + dcbz_64(&ctx->job.sg[13]); > > Are these numbers ok? First, you should define macros for them, but it looks strange > that there is a gap of 5 between the first and the second, and the rest has a gap of 4. > >> + >> + ctx->ctx_pool = ses->ctx_pool; >> + >> + return ctx; >> +} >> + > > ... > >> +/* prepare command block of the session */ >> +static int >> +dpaa_sec_prep_cdb(dpaa_sec_session *ses) >> +{ >> + struct alginfo alginfo_c = {0}, alginfo_a = {0}, alginfo = {0}; >> + uint32_t shared_desc_len = 0; >> + struct sec_cdb *cdb = &ses->qp->cdb; >> + int err; >> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN >> + int swap = false; >> +#else >> + int swap = true; >> +#endif >> + >> + memset(cdb, 0, sizeof(struct sec_cdb)); >> + >> + if (is_cipher_only(ses)) { >> + caam_cipher_alg(ses, &alginfo_c); >> + if (alginfo_c.algtype == (unsigned >> int)DPAA_SEC_ALG_UNSUPPORT) { >> + PMD_TX_LOG(ERR, "not supported cipher alg\n"); >> + return -1; > > You could use -ENOTSUP, instead of -1. > I also checked that this function is called, but the return value is not verified, > so either check it when calling it, or change the return type to "void". > >> + } >> + >> + alginfo_c.key = (uint64_t)ses->cipher_key.data; >> + alginfo_c.keylen = ses->cipher_key.length; >> + alginfo_c.key_enc_flags = 0; >> + alginfo_c.key_type = RTA_DATA_IMM; >> + >> + shared_desc_len = cnstr_shdsc_blkcipher( >> + cdb->sh_desc, true, >> + swap, &alginfo_c, >> + NULL, >> + ses->iv.length, >> + ses->dir); > > ... > >> +static uint16_t >> +dpaa_sec_enqueue_burst(void *qp, struct rte_crypto_op **ops, >> + uint16_t nb_ops) >> +{ >> + /* Function to transmit the frames to given device and queuepair */ >> + uint32_t loop; >> + int32_t ret; >> + struct dpaa_sec_qp *dpaa_qp = (struct dpaa_sec_qp *)qp; >> + uint16_t num_tx = 0; >> + >> + if (unlikely(nb_ops == 0)) >> + return 0; >> + >> + if (ops[0]->sess_type != RTE_CRYPTO_OP_WITH_SESSION) { >> + PMD_TX_LOG(ERR, "sessionless crypto op not supported"); >> + return 0; >> + } > > Each operation is independent from the other ones, so that means that some operations > could have a session, while others not. Shouldn't you check each operation? >> + >> + /*Prepare each packet which is to be sent*/ >> + for (loop = 0; loop < nb_ops; loop++) { >> + ret = dpaa_sec_enqueue_op(ops[loop], dpaa_qp); >> + if (!ret) >> + num_tx++; >> + } >> + dpaa_qp->tx_pkts += num_tx; >> + dpaa_qp->tx_errs += nb_ops - num_tx; >> + >> + return num_tx; >> +} >> + > > ... > >> +/** Release queue pair */ >> +static int >> +dpaa_sec_queue_pair_release(struct rte_cryptodev *dev, >> + uint16_t qp_id) >> +{ >> + struct dpaa_sec_dev_private *internals; >> + struct dpaa_sec_qp *qp = NULL; >> + >> + PMD_INIT_FUNC_TRACE(); >> + >> + PMD_INIT_LOG(DEBUG, "dev =%p, queue =%d", dev, qp_id); >> + >> + internals = dev->data->dev_private; >> + if (qp_id >= internals->max_nb_queue_pairs) { >> + PMD_INIT_LOG(ERR, "Max supported qpid %d", >> + internals->max_nb_queue_pairs); >> + return -1; >> + } > > Better to return -EINVAL. > >> + >> + qp = &internals->qps[qp_id]; >> + qp->internals = NULL; >> + dev->data->queue_pairs[qp_id] = NULL; >> + >> + return 0; >> +} >> + >> +/** Setup a queue pair */ >> +static int >> +dpaa_sec_queue_pair_setup(struct rte_cryptodev *dev, uint16_t qp_id, >> + __rte_unused const struct rte_cryptodev_qp_conf >> *qp_conf, >> + __rte_unused int socket_id, >> + __rte_unused struct rte_mempool *session_pool) >> +{ >> + struct dpaa_sec_dev_private *internals; >> + struct dpaa_sec_qp *qp = NULL; >> + >> + PMD_INIT_LOG(DEBUG, "dev =%p, queue =%d, conf =%p", >> + dev, qp_id, qp_conf); >> + >> + internals = dev->data->dev_private; >> + if (qp_id >= internals->max_nb_queue_pairs) { >> + PMD_INIT_LOG(ERR, "Max supported qpid %d", >> + internals->max_nb_queue_pairs); >> + return -1; >> + } > > Better to return -EINVAL. > >> + >> + qp = &internals->qps[qp_id]; >> + qp->internals = internals; >> + dev->data->queue_pairs[qp_id] = qp; >> + >> + return 0; >> +} >> + > > ... > > >> +static >> +void dpaa_sec_stats_get(struct rte_cryptodev *dev __rte_unused, >> + struct rte_cryptodev_stats *stats __rte_unused) > > "static void" should be in the same line. Anyway, if this function is not going to be implemented, > then it is probably better to just remove it, so when rte_cryptodev_stats_get() gets called, > it will return -ENOTSUP, as the PMD does not actually support this. Same for the next function. > >> +{ >> + PMD_INIT_FUNC_TRACE(); >> + /* -ENOTSUP; */ >> +} >> + >> +static >> +void dpaa_sec_stats_reset(struct rte_cryptodev *dev __rte_unused) >> +{ >> + PMD_INIT_FUNC_TRACE(); >> + /* -ENOTSUP; */ >> +} >> + > > ... > >> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.h >> b/drivers/crypto/dpaa_sec/dpaa_sec.h >> new file mode 100644 >> index 0000000..2677f8b >> --- /dev/null >> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.h >> @@ -0,0 +1,403 @@ > > ... > >> +static const struct rte_cryptodev_capabilities dpaa_sec_capabilities[] = { >> + { /* MD5 HMAC */ >> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC, >> + {.sym = { >> + .xform_type = RTE_CRYPTO_SYM_XFORM_AUTH, >> + {.auth = { >> + .algo = RTE_CRYPTO_AUTH_MD5_HMAC, >> + .block_size = 64, >> + .key_size = { >> + .min = 1, >> + .max = 64, >> + .increment = 1 >> + }, >> + .digest_size = { >> + .min = 16, >> + .max = 16, >> + .increment = 0 >> + }, >> + .aad_size = { 0 } > > No need to include aad_size here, as it is only applicable to AEAD algorithms. > I just realized that this was left after the rework done in 17.08. > Unfortunately, removing it will be an API change, so it will not be removed at least until 18.02. > As it is not used, we should remove it from here, to avoid confusion. > >> + }, } >> + }, } >> + }, > > ... > >> + { /* SHA256 HMAC */ >> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC, >> + {.sym = { >> + .xform_type = RTE_CRYPTO_SYM_XFORM_AUTH, >> + {.auth = { >> + .algo = RTE_CRYPTO_AUTH_SHA256_HMAC, >> + .block_size = 64, >> + .key_size = { >> + .min = 1, >> + .max = 64, >> + .increment = 1 >> + }, >> + .digest_size = { >> + .min = 32, >> + .max = 32, >> + .increment = 0 >> + }, > > Unnecessary extra tab. > >> + .aad_size = { 0 } >> + }, } >> + }, } > > Thanks for your comments. I would incorporate all the comments in my v2. -Akhil