Hi David, > -----Original Message----- > From: dev <[email protected]> On Behalf Of De Lara Guarch, Pablo > Sent: Tuesday, April 7, 2020 7:51 PM > To: Coyle, David <[email protected]>; [email protected] > Cc: Doherty, Declan <[email protected]>; Trahe, Fiona > <[email protected]>; Ryan, Brendan <[email protected]>; > [email protected]; [email protected]; O'loingsigh, Mairtin > <[email protected]> > Subject: Re: [dpdk-dev] [PATCH v2 2/4] raw/aesni_mb: add aesni_mb raw device > > Hi David, > > > -----Original Message----- > > From: Coyle, David <[email protected]> > > Sent: Friday, April 3, 2020 5:37 PM > > To: [email protected] > > Cc: Doherty, Declan <[email protected]>; Trahe, Fiona > > <[email protected]>; De Lara Guarch, Pablo > > <[email protected]>; Ryan, Brendan > > <[email protected]>; [email protected]; > > [email protected]; Coyle, David <[email protected]>; > > O'loingsigh, Mairtin <[email protected]> > > 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 <[email protected]> > > Signed-off-by: Mairtin o Loingsigh <[email protected]> > > --- > > 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
