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: dev <dev-bounces@dpdk.org> On Behalf Of De Lara Guarch, Pablo
> Sent: Tuesday, April 7, 2020 7:51 PM
> To: Coyle, David <david.coyle@intel.com>; dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Ryan, Brendan <brendan.ryan@intel.com>;
> shreyansh.jain@nxp.com; hemant.agrawal@nxp.com; O'loingsigh, Mairtin
> <mairtin.oloingsigh@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 2/4] raw/aesni_mb: add aesni_mb raw device
> 
> 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.

Just realized that other drivers are also including tests in their folders,
so I guess this is consistent too (sorry, I wasn't used to this new method).

Thanks,
Pablo
> 
> Thanks,
> Pablo