Re: [dpdk-dev] [PATCH v2 2/4] raw/aesni_mb: add aesni_mb raw device

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi David,

> -----Original Message-----
> From: Coyle, David <david.coyle@intel.com>
> Sent: Friday, April 3, 2020 5:37 PM
> To: dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Ryan, Brendan <brendan.ryan@intel.com>;
> shreyansh.jain@nxp.com; hemant.agrawal@nxp.com; Coyle, David
> <david.coyle@intel.com>; O'loingsigh, Mairtin <mairtin.oloingsigh@intel.com>
> Subject: [PATCH v2 2/4] raw/aesni_mb: add aesni_mb raw device
> 
> Adding an AESNI-MB raw device, thereby exposing AESNI-MB to the
> rawdev API. The AESNI-MB raw device will use the multi-function
> interface to allow combined operations be sent to the AESNI-MB
> software library.
> 
> Signed-off-by: David Coyle <david.coyle@intel.com>
> Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>
> ---
>  config/common_base                            |    6 +
>  drivers/raw/Makefile                          |    2 +
>  drivers/raw/aesni_mb/Makefile                 |   47 +
>  drivers/raw/aesni_mb/aesni_mb_rawdev.c        | 1536 +++++++++++++++++
>  drivers/raw/aesni_mb/aesni_mb_rawdev.h        |  112 ++
>  drivers/raw/aesni_mb/aesni_mb_rawdev_test.c   | 1102 ++++++++++++
>  .../aesni_mb/aesni_mb_rawdev_test_vectors.h   | 1183 +++++++++++++
>  drivers/raw/aesni_mb/meson.build              |   26 +
>  .../aesni_mb/rte_rawdev_aesni_mb_version.map  |    3 +
>  drivers/raw/meson.build                       |    3 +-
>  mk/rte.app.mk                                 |    2 +

You missed adding the PMD to the MAINTAINERS file.

>  11 files changed, 4021 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/raw/aesni_mb/Makefile
>  create mode 100644 drivers/raw/aesni_mb/aesni_mb_rawdev.c
>  create mode 100644 drivers/raw/aesni_mb/aesni_mb_rawdev.h
>  create mode 100644 drivers/raw/aesni_mb/aesni_mb_rawdev_test.c
>  create mode 100644 drivers/raw/aesni_mb/aesni_mb_rawdev_test_vectors.h
>  create mode 100644 drivers/raw/aesni_mb/meson.build
>  create mode 100644
> drivers/raw/aesni_mb/rte_rawdev_aesni_mb_version.map
> 
> diff --git a/config/common_base b/config/common_base
> index 4f004968b..7ac6a3428 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -818,6 +818,12 @@
> CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV=y
>  #
>  CONFIG_RTE_LIBRTE_PMD_NTB_RAWDEV=y
> 
> +#
> +# Compile PMD for AESNI raw device
> +#
> +CONFIG_RTE_LIBRTE_PMD_AESNI_MB_RAWDEV=n
> +CONFIG_RTE_LIBRTE_PMD_AESNI_MB_RAWDEV_DEBUG=n
> +
>  #
>  # Compile multi-fn raw device interface
>  #
> diff --git a/drivers/raw/Makefile b/drivers/raw/Makefile
> index e16da8d95..5aa608e1e 100644
> --- a/drivers/raw/Makefile
> +++ b/drivers/raw/Makefile
> @@ -15,5 +15,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_PMD_NTB_RAWDEV) += ntb
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_DMA_RAWDEV) +=
> octeontx2_dma
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV) +=
> octeontx2_ep
>  DIRS-y += common
> +DIRS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB_RAWDEV) += aesni_mb
> +DEPDIRS-aesni_mb := common
> 
>  include $(RTE_SDK)/mk/rte.subdir.mk
> diff --git a/drivers/raw/aesni_mb/Makefile b/drivers/raw/aesni_mb/Makefile
> new file mode 100644
> index 000000000..0a40b75b4
> --- /dev/null
> +++ b/drivers/raw/aesni_mb/Makefile
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2020 Intel Corporation.
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +# library name
> +LIB = librte_pmd_aesni_mb_rawdev.a
> +
> +# build flags
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
> +
> +# versioning export map
> +EXPORT_MAP := rte_rawdev_aesni_mb_version.map
> +
> +# external library dependencies
> +LDLIBS += -lIPSec_MB
> +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> +LDLIBS += -lrte_rawdev
> +LDLIBS += -lrte_bus_vdev
> +LDLIBS += -lrte_multi_fn
> +
> +ifneq ($(CONFIG_RTE_LIBRTE_MULTI_FN_COMMON),y)
> +$(error "RTE_LIBRTE_MULTI_FN_COMMON is required to build aesni_mb raw
> device")
> +endif
> +
> +IMB_HDR = $(shell echo '\#include <intel-ipsec-mb.h>' | \
> +	$(CC) -E $(EXTRA_CFLAGS) - | grep 'intel-ipsec-mb.h' | \
> +	head -n1 | cut -d'"' -f2)
> +
> +# Detect library version
> +IMB_VERSION = $(shell grep -e "IMB_VERSION_STR" $(IMB_HDR) | cut -d'"' -
> f2)
> +IMB_VERSION_NUM = $(shell grep -e "IMB_VERSION_NUM" $(IMB_HDR) | cut
> -d' ' -f3)
> +
> +ifeq ($(IMB_VERSION),)
> +$(error "IPSec_MB version >= 0.53.3 is required to build aesni_mb raw device")
> +endif
> +
> +ifeq ($(shell expr $(IMB_VERSION_NUM) \< 0x3503), 1)
> +$(error "IPSec_MB version >= 0.53.3 is required to build aesni_mb raw device")
> +endif
> +
> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB_RAWDEV) +=
> aesni_mb_rawdev.c
> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB_RAWDEV) +=
> aesni_mb_rawdev_test.c
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/raw/aesni_mb/aesni_mb_rawdev.c
> b/drivers/raw/aesni_mb/aesni_mb_rawdev.c
> new file mode 100644
> index 000000000..946bdd871
> --- /dev/null
> +++ b/drivers/raw/aesni_mb/aesni_mb_rawdev.c
> @@ -0,0 +1,1536 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#include <stdbool.h>
> +
> +#include <intel-ipsec-mb.h>
> +
> +#include <rte_common.h>
> +#include <rte_hexdump.h>
> +#include <rte_cryptodev.h>
> +#include <rte_dev.h>
> +#include <rte_eal.h>
> +#include <rte_bus_vdev.h>
> +#include <rte_malloc.h>
> +#include <rte_cpuflags.h>
> +#include <rte_rawdev.h>
> +#include <rte_rawdev_pmd.h>
> +#include <rte_string_fns.h>
> +#include <rte_multi_fn.h>
> +#include <rte_ether.h>

No need for <rte_hexdump.h>, <rte_cryptodev.h>, <rte_dev.h>, <rte_cpuflags.h> and <rte_multi_fn.h>.
I think <rte_crypto.h> is missing, though, for "rte_crypto_sym_xform".

...

> +static bool
> +docsis_crc_crypto_encrypt_check(struct rte_multi_fn_xform *xform)
> +{
> +	struct rte_crypto_sym_xform *crypto_sym;
> +	struct rte_multi_fn_err_detect_xform *err_detect;
> +	struct rte_multi_fn_xform *next;
> +
> +	if (xform->type == RTE_MULTI_FN_XFORM_TYPE_ERR_DETECT) {
> +
> +		err_detect = &xform->err_detect;
> +		next = xform->next;
> +
> +		if (err_detect->algo ==
> +				RTE_MULTI_FN_ERR_DETECT_CRC32_ETH &&
> +		    err_detect->op ==
> +				RTE_MULTI_FN_ERR_DETECT_OP_GENERATE
> &&

I don't think leading spaces are allowed. Generally, double tab is used
in multi-line if's. Same applies in other parts of the code.

> +		    next != NULL &&
> +		    next->type == RTE_MULTI_FN_XFORM_TYPE_CRYPTO_SYM) {
> +

...

> +static bool
> +docsis_crypto_decrypt_crc_check(struct rte_multi_fn_xform *xform)
> +{
> +	struct rte_crypto_sym_xform *crypto_sym;
> +	struct rte_multi_fn_err_detect_xform *err_detect;
> +	struct rte_multi_fn_xform *next;
> +
> +	if (xform->type == RTE_MULTI_FN_XFORM_TYPE_CRYPTO_SYM) {

I think in order to reduce this many indentation levels, you can check for the opposite here and return false.

If (xform->type != RTE...)
	return false
...

> +
> +static bool
> +pon_crc_crypto_encrypt_bip_check(struct rte_multi_fn_xform *xform)
> +{
> +	struct rte_crypto_sym_xform *crypto_sym;
> +	struct rte_multi_fn_err_detect_xform *err_detect;
> +	struct rte_multi_fn_xform *next;
> +
> +	if (xform->type == RTE_MULTI_FN_XFORM_TYPE_ERR_DETECT) {
> +
> +		err_detect = &xform->err_detect;
> +		next = xform->next;

Same as above here.

> +
> +		if (err_detect->algo ==
> +				RTE_MULTI_FN_ERR_DETECT_CRC32_ETH &&
> +		    err_detect->op ==

> +static bool
> +pon_bip_crypto_decrypt_crc_check(struct rte_multi_fn_xform *xform)
> +{
> +	struct rte_crypto_sym_xform *crypto_sym;
> +	struct rte_multi_fn_err_detect_xform *err_detect;
> +	struct rte_multi_fn_xform *next;
> +
> +	if (xform->type == RTE_MULTI_FN_XFORM_TYPE_ERR_DETECT) {
> +
> +		err_detect = &xform->err_detect;
> +		next = xform->next;

Same as above.

> +}
> +
> +static enum aesni_mb_rawdev_op
> +session_support_check(struct rte_multi_fn_xform *xform)
> +{
> +	enum aesni_mb_rawdev_op op =
> AESNI_MB_RAWDEV_OP_NOT_SUPPORTED;
> +

...

> +static inline int
> +docsis_crypto_crc_check(struct rte_multi_fn_op *first_op,
> +			struct rte_multi_fn_op *cipher_op,
> +			struct rte_multi_fn_op *crc_op)
> +{
> +	struct rte_multi_fn_op *err_op = NULL;
> +	uint8_t err_op_status;
> +	const uint32_t offset_diff = DOCSIS_CIPHER_CRC_OFFSET_DIFF;
> +
> +	if (cipher_op->crypto_sym.cipher.data.length &&
> +	    crc_op->err_detect.data.length) {
> +		/* Cipher offset must be at least 12 greater than CRC offset */
> +		if (cipher_op->crypto_sym.cipher.data.offset <
> +		    ((uint32_t)crc_op->err_detect.data.offset + offset_diff)) {
> +			err_op = crc_op;
> +			err_op_status =
> RTE_MULTI_FN_ERR_DETECT_OP_STATUS_ERROR;
> +		/*
> +		 * Cipher length must be at least 8 less than CRC length, taking
> +		 * known differences of what is ciphered and what is crc'ed into
> +		 * account
> +		 */
> +		} else if ((cipher_op->crypto_sym.cipher.data.length +
> +				DOCSIS_CIPHER_CRC_LENGTH_DIFF) >

For consistency, you can use offset_diff here too, instead of the macro.

> +			    crc_op->err_detect.data.length) {
> +			err_op = crc_op;
> +			err_op_status =
> RTE_MULTI_FN_ERR_DETECT_OP_STATUS_ERROR;
> +		}
> +	}

...

> +static inline int
> +mb_job_params_set(JOB_AES_HMAC *job,
> +		  struct aesni_mb_rawdev_qp *qp,
> +		  struct rte_multi_fn_op *op,
> +		  uint8_t *output_idx)
> +{
> +	struct rte_mbuf *m_src, *m_dst;
> +	struct rte_multi_fn_op *cipher_op;
> +	struct rte_multi_fn_op *crc_op;
> +	struct rte_multi_fn_op *bip_op;
> +	uint32_t cipher_offset;
> +	struct aesni_mb_rawdev_session *session;
> +

...

> +				cipher_op->crypto_sym.cipher.data.length;

I would declare a variable like sym_op to reduce the length of these.
Maybe also for err_dectect.

> +
> +	switch (session->op) {
> +	case AESNI_MB_RAWDEV_OP_DOCSIS_CRC_CRYPTO:
> +	case AESNI_MB_RAWDEV_OP_DOCSIS_CRYPTO_CRC:
> +		job->hash_start_src_offset_in_bytes =
> +						crc_op-

...


> +++ b/drivers/raw/aesni_mb/aesni_mb_rawdev_test.c
> @@ -0,0 +1,1102 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation.

Could this be moved under the test app? Looks odd here.

Thanks,
Pablo