From: Akhil Goyal <akhil.goyal@nxp.com>
To: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "Doherty, Declan" <declan.doherty@intel.com>,
"Mcnamara, John" <john.mcnamara@intel.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>
Subject: Re: [dpdk-dev] [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform
Date: Tue, 3 Oct 2017 14:15:14 +0530 [thread overview]
Message-ID: <9c208292-0ddc-79f5-fbfb-4d003ad07224@nxp.com> (raw)
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8976CC1EF10@IRSMSX108.ger.corp.intel.com>
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
>> <pablo.de.lara.guarch@intel.com>
>> Cc: Doherty, Declan <declan.doherty@intel.com>; Mcnamara, John
>> <john.mcnamara@intel.com>; hemant.agrawal@nxp.com; Akhil Goyal
>> <akhil.goyal@nxp.com>
>> Subject: [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA
>> platform
>>
>> Signed-off-by: Forrest Shi <xuelin.shi@nxp.com>
>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>> 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
next prev parent reply other threads:[~2017-10-03 8:45 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 0:01 [dpdk-dev] [PATCH 0/4] Introducing NXP dpaa_sec based cryptodev pmd Akhil Goyal
2017-08-24 0:01 ` [dpdk-dev] [PATCH 1/4] bus/dpaa: scan for DPAA Crypto devices Akhil Goyal
2017-09-18 14:24 ` De Lara Guarch, Pablo
2017-08-24 0:01 ` [dpdk-dev] [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform Akhil Goyal
2017-09-18 18:11 ` De Lara Guarch, Pablo
2017-10-03 8:45 ` Akhil Goyal [this message]
2017-08-24 0:01 ` [dpdk-dev] [PATCH 3/4] test/crypto: add dpaa crypto test cases Akhil Goyal
2017-09-18 18:19 ` De Lara Guarch, Pablo
2017-10-03 8:45 ` Akhil Goyal
2017-08-24 0:01 ` [dpdk-dev] [PATCH 4/4] doc: add NXP DPAA SEC Akhil Goyal
2017-09-18 14:32 ` De Lara Guarch, Pablo
2017-09-19 14:44 ` Mcnamara, John
2017-08-24 0:08 ` [dpdk-dev] [PATCH 0/4] Introducing NXP dpaa_sec based cryptodev pmd Akhil Goyal
2017-10-03 8:50 ` [dpdk-dev] [PATCH v2 " Akhil Goyal
2017-10-03 8:50 ` [dpdk-dev] [PATCH v2 1/4] bus/dpaa: scan for DPAA Crypto devices Akhil Goyal
2017-10-03 8:50 ` [dpdk-dev] [PATCH v2 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform Akhil Goyal
2017-10-03 8:50 ` [dpdk-dev] [PATCH v2 3/4] test/crypto: add dpaa crypto test cases Akhil Goyal
2017-10-03 8:50 ` [dpdk-dev] [PATCH v2 4/4] doc: add NXP DPAA SEC Akhil Goyal
2017-10-03 9:40 ` [dpdk-dev] [PATCH v3 0/4] Introducing NXP dpaa_sec based cryptodev pmd akhil.goyal
2017-10-03 9:40 ` [dpdk-dev] [PATCH v3 1/4] bus/dpaa: scan for DPAA Crypto devices akhil.goyal
2017-10-03 9:40 ` [dpdk-dev] [PATCH v3 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform akhil.goyal
2017-10-03 9:40 ` [dpdk-dev] [PATCH v3 3/4] test/crypto: add dpaa crypto test cases akhil.goyal
2017-10-03 9:40 ` [dpdk-dev] [PATCH v3 4/4] doc: add NXP DPAA SEC akhil.goyal
2017-10-05 13:33 ` [dpdk-dev] [PATCH v4 0/4] Introducing NXP dpaa_sec based cryptodev pmd akhil.goyal
2017-10-05 13:33 ` [dpdk-dev] [PATCH v4 1/4] bus/dpaa: scan for DPAA Crypto devices akhil.goyal
2017-10-05 13:33 ` [dpdk-dev] [PATCH v4 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform akhil.goyal
2017-10-05 13:33 ` [dpdk-dev] [PATCH v4 3/4] test/crypto: add dpaa crypto test cases akhil.goyal
2017-10-05 13:33 ` [dpdk-dev] [PATCH v4 4/4] doc: add NXP DPAA SEC akhil.goyal
2017-10-09 14:21 ` [dpdk-dev] [PATCH v5 0/4] Introducing NXP dpaa_sec based cryptodev pmd akhil.goyal
2017-10-09 14:21 ` [dpdk-dev] [PATCH v5 1/4] bus/dpaa: scan for DPAA Crypto devices akhil.goyal
2017-10-09 14:21 ` [dpdk-dev] [PATCH v5 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform akhil.goyal
2017-10-09 14:21 ` [dpdk-dev] [PATCH v5 3/4] test/crypto: add dpaa crypto test cases akhil.goyal
2017-10-09 14:21 ` [dpdk-dev] [PATCH v5 4/4] doc: add NXP DPAA SEC akhil.goyal
2017-10-09 15:28 ` [dpdk-dev] [PATCH v5 0/4] Introducing NXP dpaa_sec based cryptodev pmd De Lara Guarch, Pablo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9c208292-0ddc-79f5-fbfb-4d003ad07224@nxp.com \
--to=akhil.goyal@nxp.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=john.mcnamara@intel.com \
--cc=pablo.de.lara.guarch@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).