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.
Thanks,
Pablo
