From: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
To: Akhil Goyal <akhil.goyal@nxp.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: Mon, 18 Sep 2017 18:11:17 +0000 [thread overview]
Message-ID: <E115CCD9D858EF4F90C690B0DCB4D8976CC1EF10@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <20170824000117.32186-3-akhil.goyal@nxp.com>
> -----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 }
> + }, }
> + }, }
next prev parent reply other threads:[~2017-09-18 18:11 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 [this message]
2017-10-03 8:45 ` Akhil Goyal
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=E115CCD9D858EF4F90C690B0DCB4D8976CC1EF10@IRSMSX108.ger.corp.intel.com \
--to=pablo.de.lara.guarch@intel.com \
--cc=akhil.goyal@nxp.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=john.mcnamara@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).