Re: [dpdk-dev] [PATCH v2] crypto/aesni_mb: use architure independent marcos

[Thomas Monjalon <thomas@monjalon.net>]

11/12/2018 13:29, Fan Zhang:
> From: Lukasz Krakowiak <lukaszx.krakowiak@intel.com>
> 
> This patch updates the aesni_mb to use IMB_* arch independent
> macros to reduce the code size and future maintaining effort.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> Signed-off-by: Lukasz Krakowiak <lukaszx.krakowiak@intel.com>
> ---
> v2:
> - making the PMD compatible with both new intel-ipsec-mb version 0.52 and older
> - fixed a bug
> 
>  drivers/crypto/aesni_mb/Makefile                   |   24 +-
>  drivers/crypto/aesni_mb/meson.build                |   14 +-
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_next.c    | 1237 ++++++++++++++++++++
>  .../crypto/aesni_mb/rte_aesni_mb_pmd_ops_next.c    |  681 +++++++++++
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h |   52 +-
>  5 files changed, 1998 insertions(+), 10 deletions(-)
>  create mode 100755 drivers/crypto/aesni_mb/rte_aesni_mb_pmd_next.c
>  create mode 100755 drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops_next.c
> 
> diff --git a/drivers/crypto/aesni_mb/Makefile b/drivers/crypto/aesni_mb/Makefile
> index 806a95eb8..24630a6ca 100644
> --- a/drivers/crypto/aesni_mb/Makefile
> +++ b/drivers/crypto/aesni_mb/Makefile
> @@ -22,8 +22,26 @@ LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
>  LDLIBS += -lrte_cryptodev
>  LDLIBS += -lrte_bus_vdev
>  
> -# library source files
> -SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd.c
> -SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd_ops.c
> +IMB_HDR = /usr/include/intel-ipsec-mb.h
> +
> +# 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),)
> +	# files for older version of IMB
> +	SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd.c
> +	SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd_ops.c
> +else
> +	ifeq ($(shell expr $(IMB_VERSION_NUM) \>= 0x3400), 1)
> +		# files for a new version of IMB
> +		SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd_next.c
> +		SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd_ops_next.c
> +	else
> +		# files for older version of IMB
> +		SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd.c
> +		SRCS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += rte_aesni_mb_pmd_ops.c
> +	endif
> +endif
>  
>  include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/crypto/aesni_mb/meson.build b/drivers/crypto/aesni_mb/meson.build
> index aae0995e5..490f68eaf 100644
> --- a/drivers/crypto/aesni_mb/meson.build
> +++ b/drivers/crypto/aesni_mb/meson.build
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2018 Intel Corporation
> -
> +IPSec_MB_ver_0_52 = '0.52.0'
>  lib = cc.find_library('IPSec_MB', required: false)
>  if not lib.found()
>  	build = false
> @@ -8,5 +8,15 @@ else
>  	ext_deps += lib
>  endif
>  
> -sources = files('rte_aesni_mb_pmd.c', 'rte_aesni_mb_pmd_ops.c')
> +imb_version = cc.get_define('IMB_VERSION_STR',
> +        prefix : '#include<intel-ipsec-mb.h>')
> +
> +if imb_version.version_compare('>=' + IPSec_MB_ver_0_52)
> +	message('Build for a new version of library IPSec_MB[' + imb_version + ']')
> +	sources = files('rte_aesni_mb_pmd_next.c', 'rte_aesni_mb_pmd_ops_next.c')
> +else
> +	message('Build for older version of library IPSec_MB[' + imb_version + ']')
> +	sources = files('rte_aesni_mb_pmd.c', 'rte_aesni_mb_pmd_ops.c')
> +endif
> +
>  deps += ['bus_vdev']

I don't know what you are trying to do, but I know it is not explained.
Adding files "*_next.c" looks to be a bad idea.
And worst: it does not compile with meson:
	drivers/crypto/aesni_mb/meson.build:11:0: ERROR:  Could not get define 'IMB_VERSION_STR'

This patch is a total mess which must be explained, tested and split in several patches.
I drop it from the merge to master and update all related AES patches
to "Changes Requested" in patchwork.