Hi David,
> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of De Lara Guarch, Pablo
> Sent: Tuesday, April 7, 2020 7:51 PM
> To: Coyle, David <david.co...@intel.com>; dev@dpdk.org
> Cc: Doherty, Declan <declan.dohe...@intel.com>; Trahe, Fiona
> <fiona.tr...@intel.com>; Ryan, Brendan <brendan.r...@intel.com>;
> shreyansh.j...@nxp.com; hemant.agra...@nxp.com; O'loingsigh, Mairtin
> <mairtin.oloings...@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.co...@intel.com>
> > Sent: Friday, April 3, 2020 5:37 PM
> > To: dev@dpdk.org
> > Cc: Doherty, Declan <declan.dohe...@intel.com>; Trahe, Fiona
> > <fiona.tr...@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.gua...@intel.com>; Ryan, Brendan
> > <brendan.r...@intel.com>; shreyansh.j...@nxp.com;
> > hemant.agra...@nxp.com; Coyle, David <david.co...@intel.com>;
> > O'loingsigh, Mairtin <mairtin.oloings...@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.co...@intel.com>
> > Signed-off-by: Mairtin o Loingsigh <mairtin.oloings...@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
