From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id A0C84237 for ; Mon, 18 Sep 2017 20:11:21 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP; 18 Sep 2017 11:11:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,414,1500966000"; d="scan'208";a="136669515" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga002.jf.intel.com with ESMTP; 18 Sep 2017 11:11:20 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.167]) by IRSMSX101.ger.corp.intel.com ([169.254.1.22]) with mapi id 14.03.0319.002; Mon, 18 Sep 2017 19:11:18 +0100 From: "De Lara Guarch, Pablo" To: Akhil Goyal , "dev@dpdk.org" CC: "Doherty, Declan" , "Mcnamara, John" , "hemant.agrawal@nxp.com" Thread-Topic: [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform Thread-Index: AQHTHGxuXbbyXCpiKE6i8kxABL/KE6K63csA Date: Mon, 18 Sep 2017 18:11:17 +0000 Message-ID: References: <20170824000117.32186-1-akhil.goyal@nxp.com> <20170824000117.32186-3-akhil.goyal@nxp.com> In-Reply-To: <20170824000117.32186-3-akhil.goyal@nxp.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2ZlNGQ1MDEtMWFiMC00OTcxLWJmM2ItMWFkNmE4ZTQwNWE4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6InVnTXRwdFRTMjh5K21XWEc0Um85eUwzRVFhVlg4WFJHUjJTdmNpalVSNE09In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Sep 2017 18:11:22 -0000 > -----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 >=20 > 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 >=20 > 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 =3D 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 look= s 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 =3D 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 =3D {0}, alginfo_a =3D {0}, alginfo =3D {0}; > + uint32_t shared_desc_len =3D 0; > + struct sec_cdb *cdb =3D &ses->qp->cdb; > + int err; > +#if RTE_BYTE_ORDER =3D=3D RTE_BIG_ENDIAN > + int swap =3D false; > +#else > + int swap =3D true; > +#endif > + > + memset(cdb, 0, sizeof(struct sec_cdb)); > + > + if (is_cipher_only(ses)) { > + caam_cipher_alg(ses, &alginfo_c); > + if (alginfo_c.algtype =3D=3D (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 ve= rified, so either check it when calling it, or change the return type to "void". > + } > + > + alginfo_c.key =3D (uint64_t)ses->cipher_key.data; > + alginfo_c.keylen =3D ses->cipher_key.length; > + alginfo_c.key_enc_flags =3D 0; > + alginfo_c.key_type =3D RTA_DATA_IMM; > + > + shared_desc_len =3D 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 =3D (struct dpaa_sec_qp *)qp; > + uint16_t num_tx =3D 0; > + > + if (unlikely(nb_ops =3D=3D 0)) > + return 0; > + > + if (ops[0]->sess_type !=3D 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 =3D 0; loop < nb_ops; loop++) { > + ret =3D dpaa_sec_enqueue_op(ops[loop], dpaa_qp); > + if (!ret) > + num_tx++; > + } > + dpaa_qp->tx_pkts +=3D num_tx; > + dpaa_qp->tx_errs +=3D 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 =3D NULL; > + > + PMD_INIT_FUNC_TRACE(); > + > + PMD_INIT_LOG(DEBUG, "dev =3D%p, queue =3D%d", dev, qp_id); > + > + internals =3D dev->data->dev_private; > + if (qp_id >=3D 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 =3D &internals->qps[qp_id]; > + qp->internals =3D NULL; > + dev->data->queue_pairs[qp_id] =3D 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 =3D NULL; > + > + PMD_INIT_LOG(DEBUG, "dev =3D%p, queue =3D%d, conf =3D%p", > + dev, qp_id, qp_conf); > + > + internals =3D dev->data->dev_private; > + if (qp_id >=3D 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 =3D &internals->qps[qp_id]; > + qp->internals =3D internals; > + dev->data->queue_pairs[qp_id] =3D 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 g= oing to be implemented, then it is probably better to just remove it, so when rte_cryptodev_stats_g= et() gets called, it will return -ENOTSUP, as the PMD does not actually support this. Same fo= r 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[] = =3D { > + { /* MD5 HMAC */ > + .op =3D RTE_CRYPTO_OP_TYPE_SYMMETRIC, > + {.sym =3D { > + .xform_type =3D RTE_CRYPTO_SYM_XFORM_AUTH, > + {.auth =3D { > + .algo =3D RTE_CRYPTO_AUTH_MD5_HMAC, > + .block_size =3D 64, > + .key_size =3D { > + .min =3D 1, > + .max =3D 64, > + .increment =3D 1 > + }, > + .digest_size =3D { > + .min =3D 16, > + .max =3D 16, > + .increment =3D 0 > + }, > + .aad_size =3D { 0 } No need to include aad_size here, as it is only applicable to AEAD algorith= ms. 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 =3D RTE_CRYPTO_OP_TYPE_SYMMETRIC, > + {.sym =3D { > + .xform_type =3D RTE_CRYPTO_SYM_XFORM_AUTH, > + {.auth =3D { > + .algo =3D RTE_CRYPTO_AUTH_SHA256_HMAC, > + .block_size =3D 64, > + .key_size =3D { > + .min =3D 1, > + .max =3D 64, > + .increment =3D 1 > + }, > + .digest_size =3D { > + .min =3D 32, > + .max =3D 32, > + .increment =3D 0 > + }, Unnecessary extra tab. > + .aad_size =3D { 0 } > + }, } > + }, }