[dpdk-dev] [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Thursday, August 24, 2017 1:01 AM
> To: dev at dpdk.org; De Lara Guarch, Pablo
> <pablo.de.lara.guarch at intel.com>
> Cc: Doherty, Declan <declan.doherty at intel.com>; Mcnamara, John
> <john.mcnamara at intel.com>; hemant.agrawal at nxp.com; Akhil Goyal
> <akhil.goyal at nxp.com>
> Subject: [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA
> platform
> 
> Signed-off-by: Forrest Shi <xuelin.shi at nxp.com>
> Signed-off-by: Akhil Goyal <akhil.goyal at nxp.com>
> Signed-off-by: Hemant Agrawal <hemant.agrawal at 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 }
> +				}, }
> +			}, }