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

["Coyle, David" <david.coyle@intel.com>]

Hi Pablo
Thank you for reviewing and the comments - see below for resolutions.
The changes will be available in v3 shortly

David

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Tuesday, April 7, 2020 7:51 PM
> 
> Hi David,
> 
> > -----Original Message-----
> > From: Coyle, David <david.coyle@intel.com>
> > Sent: Friday, April 3, 2020 5:37 PM
> >
> > 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.

[DC] Added the new directories to 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/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".

[DC] Removed only hexdump and dev. All the others would seem to be needed.
rte_cpu_get_flag_enabled() is called from cpuflags
And lots of stuff from multi_fn is used throughout this file
> 
> ...
> 
> > +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.

[DC] Indentation of multi-line if statements have been fixed here and in other patches
> 
> > +		    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.

[DC] This was a good suggestion. Have use opposite logic throughout all these "check" functions which greatly reduces indentation

> 
> 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.

[DC] Same fix as above
> 
> > +
> > +		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.

[DC] And same here too
> 
> > +}
> > +
> > +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.

[DC] This check here is a LENGTH_DIFF so re-using offset_diff would work so well
I only added the offset_diff variable to get around a compiler error, so I would have used the macro there too if I could have.
Decided not to make any change here

> 
> > +			    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.

[DC] Have added variables like you suggested which does improve things alright in terms of line length
> 
> > +
> > +	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.

[DC] As you already spotted yourself, this is how rawdev define there tests.
When we add the QAT multi-function PMD, we will have to look at this though as both will need the same set of tests.
May have to add the tests directly under the test app then

> 
> Thanks,
> Pablo