[dpdk-dev] [PATCH 01/16] config: add Cavium CPT PMD skeleton

[De Lara Guarch, Pablo]

Hi Anoob,

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob at caviumnetworks.com]
> Sent: Thursday, June 14, 2018 3:57 AM
> To: Anoob Joseph <anoob.joseph at caviumnetworks.com>
> Cc: Akhil Goyal <akhil.goyal at nxp.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch at intel.com>; Thomas Monjalon
> <thomas at monjalon.net>; Nithin Dabilpuram <nithin.dabilpuram at cavium.com>;
> Ankur Dwivedi <ankur.dwivedi at cavium.com>; Murthy NSSR
> <Nidadavolu.Murthy at cavium.com>; Narayana Prasad
> <narayanaprasad.athreya at caviumnetworks.com>; Ragothaman Jayaraman
> <Ragothaman.Jayaraman at cavium.com>; Srisivasubramanian Srinivasan
> <Srisivasubramanian.Srinivasan at cavium.com>; dev at dpdk.org
> Subject: Re: [PATCH 01/16] config: add Cavium CPT PMD skeleton
> 
> -----Original Message-----
> > Date: Fri,  8 Jun 2018 22:15:10 +0530
> > From: Anoob Joseph <anoob.joseph at caviumnetworks.com>
> > To: Akhil Goyal <akhil.goyal at nxp.com>, Pablo de Lara
> > <pablo.de.lara.guarch at intel.com>, Thomas Monjalon
> > <thomas at monjalon.net>
> > Cc: Nithin Dabilpuram <nithin.dabilpuram at cavium.com>, Ankur Dwivedi
> > <ankur.dwivedi at cavium.com>, Jerin Jacob
> > <jerin.jacob at caviumnetworks.com>,  Murthy NSSR
> > <Nidadavolu.Murthy at cavium.com>, Narayana Prasad
> > <narayanaprasad.athreya at caviumnetworks.com>, Ragothaman Jayaraman
> > <Ragothaman.Jayaraman at cavium.com>, Srisivasubramanian Srinivasan
> > <Srisivasubramanian.Srinivasan at cavium.com>, dev at dpdk.org
> > Subject: [PATCH 01/16] config: add Cavium CPT PMD skeleton
> > X-Mailer: git-send-email 2.7.4
> >
> > From: Nithin Dabilpuram <nithin.dabilpuram at cavium.com>
> >
> > Add makefile and config file options.
> > Also add version map file and maintainers file to claim
> > responsibility.
> >
> > Signed-off-by: Ankur Dwivedi <ankur.dwivedi at cavium.com>
> > Signed-off-by: Murthy NSSR <Nidadavolu.Murthy at cavium.com>
> > Signed-off-by: Nithin Dabilpuram <nithin.dabilpuram at cavium.com>
> > Signed-off-by: Ragothaman Jayaraman
> <Ragothaman.Jayaraman at cavium.com>
> > Signed-off-by: Srisivasubramanian Srinivasan
> > <Srisivasubramanian.Srinivasan at cavium.com>
> > ---
> >  MAINTAINERS                                |  7 ++++++
> >  config/common_base                         |  8 ++++++
> >  drivers/crypto/Makefile                    |  1 +
> >  drivers/crypto/cpt/Makefile                | 40
> ++++++++++++++++++++++++++++++
> >  drivers/crypto/cpt/rte_pmd_cpt_version.map |  4 +++
> >  mk/rte.app.mk                              |  2 +-
> >  6 files changed, 61 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/crypto/cpt/Makefile  create mode 100644
> > drivers/crypto/cpt/rte_pmd_cpt_version.map
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 4667fa7..1b4a8eb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -504,6 +504,13 @@ F: drivers/net/octeontx/
> >  F: doc/guides/nics/octeontx.rst
> >  F: doc/guides/nics/features/octeontx.ini
> >
> > +Cavium CPT
> > +M: Ankur Dwivedi <ankur.dwivedi at cavium.com>
> > +M: Nithin Dabilpuram <nithin.dabilpuram at cavium.com>
> > +M: Murthy NSSR <Nidadavolu.Murthy at cavium.com>
> > +F: drivers/crypto/cpt/
> > +F: doc/guides/cryptodevs/cpt.rst

Add the documentation file line in the patch where you actually add it.

> 
> Move this under crypto drivers section if it is not already the case.
> 
> > +
> >  Chelsio cxgbe
> >  M: Rahul Lakkireddy <rahul.lakkireddy at chelsio.com>
> >  F: drivers/net/cxgbe/
> > diff --git a/config/common_base b/config/common_base index
> > 6b0d1cb..85e03a8 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -631,6 +631,14 @@ CONFIG_RTE_LIBRTE_PMD_DPAA_EVENTDEV=n
> >  CONFIG_RTE_LIBRTE_PMD_DPAA2_EVENTDEV=n
> >
> >  #
> > +# Compile PMD for Cavium CPT Crypto device #
> > +CONFIG_RTE_LIBRTE_PMD_CPT=n
> 
> Please enable this option  by default, If there are arm64 specific usage then
> please stub it out so that it can compile on all architectures. It will help to verify
> the API changes across the architecture by author.(i.e author should not depend
> arm64 box to verify the compilation changes) and enable distribution OS support
> like Ubuntu as they building with default arm64 config.

Agreed. Actually, I thought this was disabled because of the dependency with libcrypto
(based on the Makefile below), but actually it is not needed.
Also, move this where the other crypto drivers are.

> 
> 
> > +CONFIG_RTE_LIBRTE_PMD_CPT_DEBUG_INIT=n
> > +CONFIG_RTE_LIBRTE_PMD_CPT_DEBUG_RX=n
> > +CONFIG_RTE_LIBRTE_PMD_CPT_DEBUG_TX=n
> 
> This config option is not required when we are using dynamic debugging
> 
> > +
> > +#
> >  # Compile raw device support
> >  # EXPERIMENTAL: API may change without prior notice  # diff --git
> > a/drivers/crypto/Makefile b/drivers/crypto/Makefile index
> > 1d0c88e..a0515f3 100644
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> > @@ -22,5 +22,6 @@ ifeq ($(CONFIG_RTE_LIBRTE_DPAA_BUS),y)
> >  DIRS-$(CONFIG_RTE_LIBRTE_PMD_DPAA_SEC) += dpaa_sec  endif
> >  DIRS-$(CONFIG_RTE_LIBRTE_PMD_VIRTIO_CRYPTO) += virtio
> > +DIRS-$(CONFIG_RTE_LIBRTE_PMD_CPT) += cpt
> >
> >  include $(RTE_SDK)/mk/rte.subdir.mk
> > diff --git a/drivers/crypto/cpt/Makefile b/drivers/crypto/cpt/Makefile
> > new file mode 100644 index 0000000..b2d950d
> > --- /dev/null
> > +++ b/drivers/crypto/cpt/Makefile
> > @@ -0,0 +1,40 @@
> > +
> > +include $(RTE_SDK)/mk/rte.vars.mk
> > +
> > +# library name
> > +LIB = librte_pmd_cptvf.a
> > +
> > +# library version
> > +LIBABIVER := 1
> > +
> > +# build flags
> > +CFLAGS += $(WERROR_FLAGS)
> > +
> > +# external library include paths
> > +CFLAGS += -I$(LIBCRYPTO_THUNDERX_PATH)/include
> > +LDLIBS += -L$(LIBCRYPTO_THUNDERX_PATH) -lcrypto
> 
> What is the external library dependency here? Does look like it is documented in
> doc patch.
> 
> If there is something, I think, better to change LIBCRYPTO_THUNDERX_PATH to
> LIBCRYPTO_OCTEONTX_PATH

Is this needed? I don't see any dependency with libcrypto and I don't see this variable
documented in the driver guide.

> 
> > +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring LDLIBS +=
> > +-lrte_cryptodev LDLIBS += -lrte_pci -lrte_bus_pci
> > +
> > +VPATH += $(RTE_SDK)/drivers/crypto/cpt/base
> > +
> > +CFLAGS += -O3
> > +#CFLAGS += -DAUTH_SOFT_COMPUTE_IPAD_OPAD #CFLAGS += -
> DCPT_DEBUG
> 
> Remove commented out stuff.
> 
> > +
> > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_CPT) +=
> > +
> > +# export include files
> > +SYMLINK-y-include +=
> > +
> > +# versioning export map
> > +EXPORT_MAP := rte_pmd_cpt_version.map
> > +
> > +# library dependencies
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_CPT) += lib/librte_eal
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_CPT) += lib/librte_cryptodev
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_CPT) += lib/librte_mempool
> > +lib/librte_mbuf
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_CPT) += lib/librte_malloc
> > +
> > +include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/drivers/crypto/cpt/rte_pmd_cpt_version.map
> > b/drivers/crypto/cpt/rte_pmd_cpt_version.map
> > new file mode 100644
> > index 0000000..9b9ab1a
> > --- /dev/null
> > +++ b/drivers/crypto/cpt/rte_pmd_cpt_version.map
> > @@ -0,0 +1,4 @@
> > +DPDK_18.05 {
> > +

DPDK_18.08.

> > +	local: *;
> > +};
> > diff --git a/mk/rte.app.mk b/mk/rte.app.mk index 1e32c83..158066d
> > 100644
> > --- a/mk/rte.app.mk
> > +++ b/mk/rte.app.mk
> > @@ -209,7 +209,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_DPAA_SEC)   +=
> -lrte_pmd_dpaa_sec
> >  endif # CONFIG_RTE_LIBRTE_DPAA_BUS
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_VIRTIO_CRYPTO) +=
> > -lrte_pmd_virtio_crypto  endif # CONFIG_RTE_LIBRTE_CRYPTODEV
> > -
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_CPT)   += -lrte_pmd_cptvf

This should be inside ifeq ($(CONFIG_RTE_LIBRTE_CRYPTODEV),y).

> >  ifeq ($(CONFIG_RTE_LIBRTE_COMPRESSDEV),y)
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_ISAL) += -lrte_pmd_isal_comp
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_ISAL) += -lisal
> 
> 
> You can add meson build as well here on the same patch.
> 
> 
> > --
> > 1.9.3
> >