[v2,02/22] lib: add pdcp protocol

  Add Packet Data Convergence Protocol (PDCP) processing library.

The library is similar to lib_ipsec which provides IPsec processing
capabilities in DPDK.

PDCP would involve roughly the following options,
1. Transfer of user plane data
2. Transfer of control plane data
3. Header compression
4. Uplink data compression
5. Ciphering and integrity protection

PDCP library provides following control path APIs that is used to
configure various PDCP entities,
1. rte_pdcp_entity_establish()
2. rte_pdcp_entity_suspend()
3. rte_pdcp_entity_release()

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 doc/api/doxy-api-index.md |   3 +-
 doc/api/doxy-api.conf.in  |   1 +
 lib/meson.build           |   1 +
 lib/pdcp/meson.build      |  17 +++++
 lib/pdcp/pdcp_crypto.c    |  21 +++++
 lib/pdcp/pdcp_crypto.h    |  15 ++++
 lib/pdcp/pdcp_entity.h    |  95 +++++++++++++++++++++++
 lib/pdcp/pdcp_process.c   | 138 +++++++++++++++++++++++++++++++++
 lib/pdcp/pdcp_process.h   |  13 ++++
 lib/pdcp/rte_pdcp.c       | 138 +++++++++++++++++++++++++++++++++
 lib/pdcp/rte_pdcp.h       | 157 ++++++++++++++++++++++++++++++++++++++
 lib/pdcp/version.map      |  10 +++
 12 files changed, 608 insertions(+), 1 deletion(-)
 create mode 100644 lib/pdcp/meson.build
 create mode 100644 lib/pdcp/pdcp_crypto.c
 create mode 100644 lib/pdcp/pdcp_crypto.h
 create mode 100644 lib/pdcp/pdcp_entity.h
 create mode 100644 lib/pdcp/pdcp_process.c
 create mode 100644 lib/pdcp/pdcp_process.h
 create mode 100644 lib/pdcp/rte_pdcp.c
 create mode 100644 lib/pdcp/rte_pdcp.h
 create mode 100644 lib/pdcp/version.map
  

Hi Anoob,

Fix check patch issues and please see some inline comments.

> Subject: [PATCH v2 02/22] lib: add pdcp protocol
> 
> Add Packet Data Convergence Protocol (PDCP) processing library.
> 
> The library is similar to lib_ipsec which provides IPsec processing
> capabilities in DPDK.
> 
> PDCP would involve roughly the following options,
> 1. Transfer of user plane data
> 2. Transfer of control plane data
> 3. Header compression
> 4. Uplink data compression
> 5. Ciphering and integrity protection
> 
> PDCP library provides following control path APIs that is used to
> configure various PDCP entities,
> 1. rte_pdcp_entity_establish()
> 2. rte_pdcp_entity_suspend()
> 3. rte_pdcp_entity_release()
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
>  doc/api/doxy-api-index.md |   3 +-
>  doc/api/doxy-api.conf.in  |   1 +
>  lib/meson.build           |   1 +
>  lib/pdcp/meson.build      |  17 +++++
>  lib/pdcp/pdcp_crypto.c    |  21 +++++
>  lib/pdcp/pdcp_crypto.h    |  15 ++++
>  lib/pdcp/pdcp_entity.h    |  95 +++++++++++++++++++++++
>  lib/pdcp/pdcp_process.c   | 138 +++++++++++++++++++++++++++++++++
>  lib/pdcp/pdcp_process.h   |  13 ++++
>  lib/pdcp/rte_pdcp.c       | 138 +++++++++++++++++++++++++++++++++
>  lib/pdcp/rte_pdcp.h       | 157 ++++++++++++++++++++++++++++++++++++++
>  lib/pdcp/version.map      |  10 +++
>  12 files changed, 608 insertions(+), 1 deletion(-)
>  create mode 100644 lib/pdcp/meson.build
>  create mode 100644 lib/pdcp/pdcp_crypto.c
>  create mode 100644 lib/pdcp/pdcp_crypto.h
>  create mode 100644 lib/pdcp/pdcp_entity.h
>  create mode 100644 lib/pdcp/pdcp_process.c
>  create mode 100644 lib/pdcp/pdcp_process.h
>  create mode 100644 lib/pdcp/rte_pdcp.c
>  create mode 100644 lib/pdcp/rte_pdcp.h
>  create mode 100644 lib/pdcp/version.map
> 
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index debbe4134f..cd7a6cae44 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -128,7 +128,8 @@ The public API headers are grouped by topics:
>    [eCPRI](@ref rte_ecpri.h),
>    [L2TPv2](@ref rte_l2tpv2.h),
>    [PPP](@ref rte_ppp.h),
> -  [PDCP hdr](@ref rte_pdcp_hdr.h)
> +  [PDCP hdr](@ref rte_pdcp_hdr.h),
> +  [PDCP](@ref rte_pdcp.h)
> 
>  - **QoS**:
>    [metering](@ref rte_meter.h),
> diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in
> index d230a19e1f..58789308a9 100644
> --- a/doc/api/doxy-api.conf.in
> +++ b/doc/api/doxy-api.conf.in
> @@ -62,6 +62,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-
> index.md \
>                            @TOPDIR@/lib/net \
>                            @TOPDIR@/lib/pcapng \
>                            @TOPDIR@/lib/pci \
> +                          @TOPDIR@/lib/pdcp \
>                            @TOPDIR@/lib/pdump \
>                            @TOPDIR@/lib/pipeline \
>                            @TOPDIR@/lib/port \
> diff --git a/lib/meson.build b/lib/meson.build
> index 0812ce6026..d217c04ea9 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -64,6 +64,7 @@ libraries = [
>          'flow_classify', # flow_classify lib depends on pkt framework table lib
>          'graph',
>          'node',
> +        'pdcp', # pdcp lib depends on crypto and security
>  ]
> 
>  optional_libs = [
> diff --git a/lib/pdcp/meson.build b/lib/pdcp/meson.build
> new file mode 100644
> index 0000000000..ccaf426240
> --- /dev/null
> +++ b/lib/pdcp/meson.build
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(C) 2023 Marvell.
> +
> +if is_windows
> +    build = false
> +    reason = 'not supported on Windows'
> +    subdir_done()
> +endif
> +
> +sources = files(
> +        'pdcp_crypto.c',
> +        'pdcp_process.c',
> +        'rte_pdcp.c',
> +        )
> +headers = files('rte_pdcp.h')
> +
> +deps += ['mbuf', 'net', 'cryptodev', 'security']
> diff --git a/lib/pdcp/pdcp_crypto.c b/lib/pdcp/pdcp_crypto.c
> new file mode 100644
> index 0000000000..755e27ec9e
> --- /dev/null
> +++ b/lib/pdcp/pdcp_crypto.c
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#include <rte_pdcp.h>
> +
> +#include "pdcp_crypto.h"
> +
> +int
> +pdcp_crypto_sess_create(struct rte_pdcp_entity *entity, const struct
> rte_pdcp_entity_conf *conf)
> +{
> +	RTE_SET_USED(entity);
> +	RTE_SET_USED(conf);
> +	return 0;
> +}
> +
> +void
> +pdcp_crypto_sess_destroy(struct rte_pdcp_entity *entity)
> +{
> +	RTE_SET_USED(entity);
> +}
> diff --git a/lib/pdcp/pdcp_crypto.h b/lib/pdcp/pdcp_crypto.h
> new file mode 100644
> index 0000000000..6563331d37
> --- /dev/null
> +++ b/lib/pdcp/pdcp_crypto.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#ifndef PDCP_CRYPTO_H
> +#define PDCP_CRYPTO_H
> +
> +#include <rte_pdcp.h>
> +
> +int pdcp_crypto_sess_create(struct rte_pdcp_entity *entity,
> +			    const struct rte_pdcp_entity_conf *conf);
> +
> +void pdcp_crypto_sess_destroy(struct rte_pdcp_entity *entity);
> +
> +#endif /* PDCP_CRYPTO_H */
> diff --git a/lib/pdcp/pdcp_entity.h b/lib/pdcp/pdcp_entity.h
> new file mode 100644
> index 0000000000..ca1d56b516
> --- /dev/null
> +++ b/lib/pdcp/pdcp_entity.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#ifndef PDCP_ENTITY_H
> +#define PDCP_ENTITY_H
> +
> +#include <rte_common.h>
> +#include <rte_crypto_sym.h>
> +#include <rte_mempool.h>
> +#include <rte_pdcp.h>
> +#include <rte_security.h>
> +
> +struct entity_priv;
> +
> +/* IV generation function based on the entity configuration */
> +typedef void (*iv_gen_t)(struct rte_crypto_op *cop, const struct entity_priv
> *en_priv,
> +			 uint32_t count);
> +
> +struct entity_state {
> +	uint32_t rx_next;
> +	uint32_t tx_next;
> +	uint32_t rx_deliv;
> +	uint32_t rx_reord;
> +};
> +
> +/*
> + * Layout of PDCP entity: [rte_pdcp_entity] [entity_priv] [entity_dl/ul]
> + */
> +
> +struct entity_priv {
> +	/** Crypto sym session. */
> +	struct rte_cryptodev_sym_session *crypto_sess;
> +	/** Entity specific IV generation function. */
> +	iv_gen_t iv_gen;
> +	/** Entity state variables. */
> +	struct entity_state state;
> +	/** Flags. */
> +	struct {
> +		/** PDCP PDU has 4 byte MAC-I. */
> +		uint64_t is_authenticated : 1;
> +		/** Cipher offset & length in bits. */
> +		uint64_t is_ciph_in_bits : 1;
> +		/** Auth offset & length in bits. */
> +		uint64_t is_auth_in_bits : 1;
> +		/** Is UL/transmitting PDCP entity. */
> +		uint64_t is_ul_entity : 1;
> +		/** Is NULL auth. */
> +		uint64_t is_null_auth : 1;
> +	} flags;
> +	/** Crypto op pool. */
> +	struct rte_mempool *cop_pool;
> +	/** PDCP header size. */
> +	uint8_t hdr_sz;
> +	/** PDCP AAD size. For AES-CMAC, additional message is prepended for
> the operation. */
> +	uint8_t aad_sz;
> +	/** Device ID of the device to be used for offload. */
> +	uint8_t dev_id;
> +};
> +
> +struct entity_priv_dl_part {
> +	/* NOTE: when in-order-delivery is supported, post PDCP packets would
> need to cached. */
> +	uint8_t dummy;
> +};
> +
> +struct entity_priv_ul_part {
> +	/*
> +	 * NOTE: when re-establish is supported, plain PDCP packets & COUNT
> values need to be
> +	 * cached.
> +	 */
> +	uint8_t dummy;
> +};
> +
> +static inline struct entity_priv *
> +entity_priv_get(const struct rte_pdcp_entity *entity) {
> +	return RTE_PTR_ADD(entity, sizeof(struct rte_pdcp_entity));
> +}
> +
> +static inline struct entity_priv_dl_part *
> +entity_dl_part_get(const struct rte_pdcp_entity *entity) {
> +	return RTE_PTR_ADD(entity, sizeof(struct rte_pdcp_entity) +
> sizeof(struct entity_priv));
> +}
> +
> +static inline struct entity_priv_ul_part *
> +entity_ul_part_get(const struct rte_pdcp_entity *entity) {
> +	return RTE_PTR_ADD(entity, sizeof(struct rte_pdcp_entity) +
> sizeof(struct entity_priv));
> +}
> +
> +static inline int
> +pdcp_hdr_size_get(enum rte_security_pdcp_sn_size sn_size)
> +{
> +	return RTE_ALIGN_MUL_CEIL(sn_size, 8) / 8;
> +}
> +
> +#endif /* PDCP_ENTITY_H */
> diff --git a/lib/pdcp/pdcp_process.c b/lib/pdcp/pdcp_process.c
> new file mode 100644
> index 0000000000..d4b158536d
> --- /dev/null
> +++ b/lib/pdcp/pdcp_process.c
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#include <rte_crypto.h>
> +#include <rte_crypto_sym.h>
> +#include <rte_cryptodev.h>
> +#include <rte_memcpy.h>
> +#include <rte_pdcp.h>
> +#include <rte_pdcp_hdr.h>
> +
> +#include "pdcp_crypto.h"
> +#include "pdcp_entity.h"
> +#include "pdcp_process.h"
> +
> +static int
> +pdcp_crypto_xfrm_get(const struct rte_pdcp_entity_conf *conf, struct
> rte_crypto_sym_xform **c_xfrm,
> +		     struct rte_crypto_sym_xform **a_xfrm)
> +{
> +	*c_xfrm = NULL;
> +	*a_xfrm = NULL;
> +
> +	if (conf->crypto_xfrm == NULL)
> +		return -EINVAL;
> +
> +	if (conf->crypto_xfrm->type == RTE_CRYPTO_SYM_XFORM_CIPHER) {
> +		*c_xfrm = conf->crypto_xfrm;
> +		*a_xfrm = conf->crypto_xfrm->next;
> +	} else if (conf->crypto_xfrm->type ==
> RTE_CRYPTO_SYM_XFORM_AUTH) {
> +		*a_xfrm = conf->crypto_xfrm;
> +		*c_xfrm = conf->crypto_xfrm->next;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +pdcp_entity_priv_populate(struct entity_priv *en_priv, const struct
> rte_pdcp_entity_conf *conf)
> +{
> +	struct rte_crypto_sym_xform *c_xfrm, *a_xfrm;
> +	int ret;
> +
> +	/**
> +	 * flags.is_authenticated
> +	 *
> +	 * MAC-I would be added in case of control plane packets and when
> authentication
> +	 * transform is not NULL.
> +	 */
> +
> +	if (conf->pdcp_xfrm.domain ==
> RTE_SECURITY_PDCP_MODE_CONTROL)
> +		en_priv->flags.is_authenticated = 1;

This check should be added after getting the xfrm.
If domain == control and a_xfrm is NULL, then it should be error, right?

> +
> +	ret = pdcp_crypto_xfrm_get(conf, &c_xfrm, &a_xfrm);
> +	if (ret)
> +		return ret;
> +
> +	if (a_xfrm != NULL)
> +		en_priv->flags.is_authenticated = 1;
> +
> +	/**
> +	 * flags.is_ciph_in_bits
> +	 *
> +	 * For ZUC & SNOW3G cipher algos, offset & length need to be provided
> in bits.
> +	 */
> +
> +	if ((c_xfrm->cipher.algo == RTE_CRYPTO_CIPHER_SNOW3G_UEA2) ||
> +	    (c_xfrm->cipher.algo == RTE_CRYPTO_CIPHER_ZUC_EEA3))
> +		en_priv->flags.is_ciph_in_bits = 1;
> +
> +	/**
> +	 * flags.is_auth_in_bits
> +	 *
> +	 * For ZUC & SNOW3G authentication algos, offset & length need to be
> provided in bits.
> +	 */
> +
> +	if (a_xfrm != NULL) {
> +		if ((a_xfrm->auth.algo == RTE_CRYPTO_AUTH_SNOW3G_UIA2)
> ||
> +		    (a_xfrm->auth.algo == RTE_CRYPTO_AUTH_ZUC_EIA3))
> +			en_priv->flags.is_auth_in_bits = 1;
> +	}
> +
> +	/**
> +	 * flags.is_ul_entity
> +	 *
> +	 * Indicate whether the entity is UL/transmitting PDCP entity.
> +	 */
> +	if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> +		en_priv->flags.is_ul_entity = 1;
> +
> +	/**
> +	 * flags.is_null_auth
> +	 *
> +	 * For NULL auth, 4B zeros need to be added by lib PDCP. Indicate that
> +	 * algo is NULL auth to perform the same.
> +	 */
> +	if (a_xfrm != NULL && a_xfrm->auth.algo ==
> RTE_CRYPTO_AUTH_NULL)
> +		en_priv->flags.is_null_auth = 1;
> +
> +	/**
> +	 * hdr_sz
> +	 *
> +	 * PDCP header size of the entity
> +	 */
> +	en_priv->hdr_sz = pdcp_hdr_size_get(conf->pdcp_xfrm.sn_size);
> +
> +	/**
> +	 * aad_sz
> +	 *
> +	 * For AES-CMAC, additional message is prepended for processing. Need
> to be trimmed after
> +	 * crypto processing is done.
> +	 */
> +	if (a_xfrm != NULL && a_xfrm->auth.algo ==
> RTE_CRYPTO_AUTH_AES_CMAC)
> +		en_priv->aad_sz = 8;
> +	else
> +		en_priv->aad_sz = 0;
> +
> +	return 0;
> +}
> +
> +int
> +pdcp_process_func_set(struct rte_pdcp_entity *entity, const struct
> rte_pdcp_entity_conf *conf)
> +{
> +	struct entity_priv *en_priv;
> +	int ret;
> +
> +	if (entity == NULL || conf == NULL)
> +		return -EINVAL;
> +
> +	en_priv = entity_priv_get(entity);
> +
> +	ret = pdcp_entity_priv_populate(en_priv, conf);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> diff --git a/lib/pdcp/pdcp_process.h b/lib/pdcp/pdcp_process.h
> new file mode 100644
> index 0000000000..fd53fff0aa
> --- /dev/null
> +++ b/lib/pdcp/pdcp_process.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#ifndef PDCP_PROCESS_H
> +#define PDCP_PROCESS_H
> +
> +#include <rte_pdcp.h>
> +
> +int
> +pdcp_process_func_set(struct rte_pdcp_entity *entity, const struct
> rte_pdcp_entity_conf *conf);
> +
> +#endif /* PDCP_PROCESS_H */
> diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c
> new file mode 100644
> index 0000000000..8914548dbd
> --- /dev/null
> +++ b/lib/pdcp/rte_pdcp.c
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#include <rte_errno.h>
> +#include <rte_pdcp.h>
> +#include <rte_malloc.h>
> +
> +#include "pdcp_crypto.h"
> +#include "pdcp_entity.h"
> +#include "pdcp_process.h"
> +
> +static int
> +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf)
> +{
> +	int size;
> +
> +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct entity_priv);
> +
> +	if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_DOWNLINK)
> +		size += sizeof(struct entity_priv_dl_part);
> +	else if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> +		size += sizeof(struct entity_priv_ul_part);
> +	else
> +		return -EINVAL;
> +
> +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE);
> +}
> +
> +struct rte_pdcp_entity *
> +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf)
> +{
> +	struct rte_pdcp_entity *entity = NULL;
> +	struct entity_priv *en_priv;
> +	int ret, entity_size;
> +
> +	if (conf == NULL || conf->cop_pool == NULL) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}

errnos are normally set as positive values.


> +
> +	if (conf->pdcp_xfrm.en_ordering || conf-
> >pdcp_xfrm.remove_duplicates || conf->is_slrb ||
> +	    conf->en_sec_offload) {
> +		rte_errno = -ENOTSUP;
> +		return NULL;
> +	}
> +
> +	/*
> +	 * 6.3.2 PDCP SN
> +	 * Length: 12 or 18 bits as indicated in table 6.3.2-1. The length of the
> PDCP SN is
> +	 * configured by upper layers (pdcp-SN-SizeUL, pdcp-SN-SizeDL, or sl-
> PDCP-SN-Size in
> +	 * TS 38.331 [3])
> +	 */
> +	if ((conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_12) &&
> +	    (conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_18)) {
> +		rte_errno = -ENOTSUP;
> +		return NULL;
> +	}

Check for PDCP crypto algos may also be added.
As only 4 cipher and 4 auth algos are supported in case of PDCP.

> +
> +	if (conf->pdcp_xfrm.hfn || conf->pdcp_xfrm.hfn_threshold) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}

What is the reason to set errno as EINVAL when HFN is set?

> +
> +	entity_size = pdcp_entity_size_get(conf);
> +	if (entity_size < 0) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +
> +	entity = rte_zmalloc_socket("pdcp_entity", entity_size,
> RTE_CACHE_LINE_SIZE, SOCKET_ID_ANY);
> +	if (entity == NULL) {
> +		rte_errno = -ENOMEM;
> +		return NULL;
> +	}
> +
> +	en_priv = entity_priv_get(entity);
> +
> +	en_priv->state.rx_deliv = conf->count;
> +	en_priv->state.tx_next = conf->count;
> +	en_priv->cop_pool = conf->cop_pool;
> +
> +	/* Setup crypto session */
> +	ret = pdcp_crypto_sess_create(entity, conf);
> +	if (ret)
> +		goto entity_free;
> +
> +	ret = pdcp_process_func_set(entity, conf);
> +	if (ret)
> +		goto crypto_sess_destroy;
> +
> +	return entity;
> +
> +crypto_sess_destroy:
> +	pdcp_crypto_sess_destroy(entity);
> +entity_free:
> +	rte_free(entity);
> +	rte_errno = ret;
> +	return NULL;
> +}
> +
> +int
> +rte_pdcp_entity_release(struct rte_pdcp_entity *pdcp_entity, struct rte_mbuf
> *out_mb[])
> +{
> +	if (pdcp_entity == NULL)
> +		return -EINVAL;
> +
> +	/* Teardown crypto sessions */
> +	pdcp_crypto_sess_destroy(pdcp_entity);
> +
> +	rte_free(pdcp_entity);
> +
> +	RTE_SET_USED(out_mb);
> +	return 0;
> +}
> +
> +int
> +rte_pdcp_entity_suspend(struct rte_pdcp_entity *pdcp_entity,
> +			struct rte_mbuf *out_mb[])
> +{
> +	struct entity_priv *en_priv;
> +
> +	if (pdcp_entity == NULL)
> +		return -EINVAL;
> +
> +	en_priv = entity_priv_get(pdcp_entity);
> +
> +	if (en_priv->flags.is_ul_entity) {
> +		en_priv->state.tx_next = 0;
> +	} else {
> +		en_priv->state.rx_next = 0;
> +		en_priv->state.rx_deliv = 0;
> +	}
> +
> +	RTE_SET_USED(out_mb);
> +
> +	return 0;
> +}
> diff --git a/lib/pdcp/rte_pdcp.h b/lib/pdcp/rte_pdcp.h
> new file mode 100644
> index 0000000000..33c355b05a
> --- /dev/null
> +++ b/lib/pdcp/rte_pdcp.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#ifndef RTE_PDCP_H
> +#define RTE_PDCP_H
> +
> +/**
> + * @file rte_pdcp.h
> + *
> + * RTE PDCP support.
> + *
> + * librte_pdcp provides a framework for PDCP protocol processing.

A framework for PDCP protocol processing.

> + */
> +
> +#include <rte_compat.h>
> +#include <rte_common.h>
> +#include <rte_mempool.h>
> +#include <rte_security.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * PDCP entity.

You can probably explain more on what a PDCP entity is.

> + */
> +struct rte_pdcp_entity {
> +	/**
> +	 * PDCP entities may hold packets for purposes of in-order delivery (in
> +	 * case of receiving PDCP entity) and re-transmission (in case of
> +	 * transmitting PDCP entity).
> +	 *
> +	 * For receiving PDCP entity, it may hold packets when in-order
> +	 * delivery is enabled. The packets would be cached until either a
> +	 * packet that completes the sequence arrives or when t-Reordering
> timer
> +	 * expires.
> +	 *
> +	 * When post-processing of PDCP packet which completes a sequence is
> +	 * done, the API may return more packets than enqueued. Application is
> +	 * expected to provide *rte_pdcp_pkt_post_process()* with *out_mb*
> +	 * which can hold maximum number of packets which may be returned.
> +	 */

The above comment explains the need for holding the packets.
But it does not talk about the parameter it is explaining.
This explanation should be part of programmer's guide and not the API guide.

> +	uint32_t max_pkt_cache;
> +	/** User area for saving application data. */
> +	uint64_t user_area[2];

Is it being used right now in the patches?
If not, can we add it later?
And if really needed now, can we rename to user_data

> +} __rte_cache_aligned;
> +
> +/**
> + * PDCP entity configuration to be used for establishing an entity.
> + */
> +/* Structure rte_pdcp_entity_conf 8< */
> +struct rte_pdcp_entity_conf {
> +	/** PDCP transform for the entity. */
> +	struct rte_security_pdcp_xform pdcp_xfrm;
> +	/** Crypto transform applicable for the entity. */
> +	struct rte_crypto_sym_xform *crypto_xfrm;
> +	/** Mempool for crypto symmetric session. */
> +	struct rte_mempool *sess_mpool;
> +	/** Crypto op pool.*/
> +	struct rte_mempool *cop_pool;
> +	/**
> +	 * 32 bit count value (HFN + SN) to be used for the first packet.
> +	 * pdcp_xfrm.hfn would be ignored as the HFN would be derived from
> this value.
> +	 */

If the HFN is to be ignored, then why to add a check in entity establish and return EINVAL?
It should be silently ignored in that case with a debug print at max.


> +	uint32_t count;
> +	/** Indicate whether the PDCP entity belongs to Side Link Radio Bearer.
> */
> +	bool is_slrb;
> +	/** Enable security offload on the device specified. */
> +	bool en_sec_offload;
> +	/** Device on which security/crypto session need to be created. */
> +	uint8_t dev_id;
> +	/** Reverse direction during IV generation. Can be used to simulate UE
> crypto processing.*/
> +	bool reverse_iv_direction;
> +};
> +/* >8 End of structure rte_pdcp_entity_conf. */
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * 5.1.1 PDCP entity establishment
> + *
> + * Establish PDCP entity based on provided input configuration.
> + *
> + * @param conf
> + *   Parameters to be used for initializing PDCP entity object.
> + * @return
> + *   - Valid handle if success
> + *   - NULL in case of failure. rte_errno will be set to error code
> + */
> +__rte_experimental
> +struct rte_pdcp_entity *
> +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * 5.1.3 PDCP entity release
> + *
> + * Release PDCP entity.
> + *
> + * For UL/transmitting PDCP entity, all stored PDCP SDUs would be dropped.
> + * For DL/receiving PDCP entity, the stored PDCP SDUs would be returned in
> + * *out_mb* buffer. The buffer should be large enough to hold all cached
> + * packets in the entity.
> + *
> + * @param pdcp_entity
> + *   Pointer to the PDCP entity to be released.
> + * @param[out] out_mb
> + *   The address of an array that can hold up to
> *rte_pdcp_entity.max_pkt_cache*
> + *   pointers to *rte_mbuf* structures.
> + * @return
> + *   -  0: Success and no cached packets to return
> + *   - >0: Success and the number of packets returned in out_mb
> + *   - <0: Error code in case of failures
> + */
> +__rte_experimental
> +int
> +rte_pdcp_entity_release(struct rte_pdcp_entity *pdcp_entity,
> +			struct rte_mbuf *out_mb[]);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * 5.1.4 PDCP entity suspend
> + *
> + * Suspend PDCP entity.
> + *
> + * For DL/receiving PDCP entity, the stored PDCP SDUs would be returned in
> + * *out_mb* buffer. The buffer should be large enough to hold all cached
> + * packets in the entity.
> + *
> + * For UL/transmitting PDCP entity, *out_mb* buffer would be unused.
> + *
> + * @param pdcp_entity
> + *   Pointer to the PDCP entity to be suspended.
> + * @param[out] out_mb
> + *   The address of an array that can hold up to
> *rte_pdcp_entity.max_pkt_cache*
> + *   pointers to *rte_mbuf* structures.
> + * @return
> + *   -  0: Success and no cached packets to return
> + *   - >0: Success and the number of packets returned in out_mb
> + *   - <0: Error code in case of failures
> + */
> +__rte_experimental
> +int
> +rte_pdcp_entity_suspend(struct rte_pdcp_entity *pdcp_entity,
> +			struct rte_mbuf *out_mb[]);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* RTE_PDCP_H */
> diff --git a/lib/pdcp/version.map b/lib/pdcp/version.map
> new file mode 100644
> index 0000000000..923e165f3f
> --- /dev/null
> +++ b/lib/pdcp/version.map
> @@ -0,0 +1,10 @@
> +EXPERIMENTAL {
> +	global:
> +
> +	# added in 23.07
> +	rte_pdcp_entity_establish;
> +	rte_pdcp_entity_release;
> +	rte_pdcp_entity_suspend;
> +
> +	local: *;
> +};
> --
> 2.25.1
  

Hi Akhil,

Thanks for the review. Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, May 16, 2023 9:01 PM
> To: Anoob Joseph <anoobj@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>; Bernard
> Iremonger <bernard.iremonger@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Mattias Rönnblom
> <mattias.ronnblom@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Volodymyr Fialko <vfialko@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>
> Subject: RE: [PATCH v2 02/22] lib: add pdcp protocol
> 
> Hi Anoob,
> 
> Fix check patch issues and please see some inline comments.

[Anoob] Checkpatch warnings are false positives. I'll try to work around couple of them. Rest we may need to ignore.

> 
> > Subject: [PATCH v2 02/22] lib: add pdcp protocol
> >
> > Add Packet Data Convergence Protocol (PDCP) processing library.
> >
> > The library is similar to lib_ipsec which provides IPsec processing
> > capabilities in DPDK.
> >
> > PDCP would involve roughly the following options, 1. Transfer of user
> > plane data 2. Transfer of control plane data 3. Header compression 4.
> > Uplink data compression 5. Ciphering and integrity protection
> >
> > PDCP library provides following control path APIs that is used to
> > configure various PDCP entities, 1. rte_pdcp_entity_establish() 2.
> > rte_pdcp_entity_suspend() 3. rte_pdcp_entity_release()
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> > Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> > ---
> >  doc/api/doxy-api-index.md |   3 +-
> >  doc/api/doxy-api.conf.in  |   1 +
> >  lib/meson.build           |   1 +
> >  lib/pdcp/meson.build      |  17 +++++
> >  lib/pdcp/pdcp_crypto.c    |  21 +++++
> >  lib/pdcp/pdcp_crypto.h    |  15 ++++
> >  lib/pdcp/pdcp_entity.h    |  95 +++++++++++++++++++++++
> >  lib/pdcp/pdcp_process.c   | 138
> +++++++++++++++++++++++++++++++++
> >  lib/pdcp/pdcp_process.h   |  13 ++++
> >  lib/pdcp/rte_pdcp.c       | 138 +++++++++++++++++++++++++++++++++
> >  lib/pdcp/rte_pdcp.h       | 157
> ++++++++++++++++++++++++++++++++++++++
> >  lib/pdcp/version.map      |  10 +++
> >  12 files changed, 608 insertions(+), 1 deletion(-)  create mode
> > 100644 lib/pdcp/meson.build  create mode 100644 lib/pdcp/pdcp_crypto.c
> > create mode 100644 lib/pdcp/pdcp_crypto.h  create mode 100644
> > lib/pdcp/pdcp_entity.h  create mode 100644 lib/pdcp/pdcp_process.c
> > create mode 100644 lib/pdcp/pdcp_process.h  create mode 100644
> > lib/pdcp/rte_pdcp.c  create mode 100644 lib/pdcp/rte_pdcp.h  create
> > mode 100644 lib/pdcp/version.map
> >
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index debbe4134f..cd7a6cae44 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -128,7 +128,8 @@ The public API headers are grouped by topics:
> >    [eCPRI](@ref rte_ecpri.h),
> >    [L2TPv2](@ref rte_l2tpv2.h),
> >    [PPP](@ref rte_ppp.h),
> > -  [PDCP hdr](@ref rte_pdcp_hdr.h)
> > +  [PDCP hdr](@ref rte_pdcp_hdr.h),
> > +  [PDCP](@ref rte_pdcp.h)
> >
> >  - **QoS**:
> >    [metering](@ref rte_meter.h),
> > diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in index
> > d230a19e1f..58789308a9 100644
> > --- a/doc/api/doxy-api.conf.in
> > +++ b/doc/api/doxy-api.conf.in
> > @@ -62,6 +62,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-
> > index.md \
> >                            @TOPDIR@/lib/net \
> >                            @TOPDIR@/lib/pcapng \
> >                            @TOPDIR@/lib/pci \
> > +                          @TOPDIR@/lib/pdcp \
> >                            @TOPDIR@/lib/pdump \
> >                            @TOPDIR@/lib/pipeline \
> >                            @TOPDIR@/lib/port \ diff --git
> > a/lib/meson.build b/lib/meson.build index 0812ce6026..d217c04ea9
> > 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -64,6 +64,7 @@ libraries = [
> >          'flow_classify', # flow_classify lib depends on pkt framework table lib
> >          'graph',
> >          'node',
> > +        'pdcp', # pdcp lib depends on crypto and security
> >  ]
> >
> >  optional_libs = [
> > diff --git a/lib/pdcp/meson.build b/lib/pdcp/meson.build new file mode
> > 100644 index 0000000000..ccaf426240
> > --- /dev/null
> > +++ b/lib/pdcp/meson.build
> > @@ -0,0 +1,17 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(C) 2023 Marvell.
> > +
> > +if is_windows
> > +    build = false
> > +    reason = 'not supported on Windows'
> > +    subdir_done()
> > +endif
> > +
> > +sources = files(
> > +        'pdcp_crypto.c',
> > +        'pdcp_process.c',
> > +        'rte_pdcp.c',
> > +        )
> > +headers = files('rte_pdcp.h')
> > +
> > +deps += ['mbuf', 'net', 'cryptodev', 'security']
> > diff --git a/lib/pdcp/pdcp_crypto.c b/lib/pdcp/pdcp_crypto.c new file
> > mode 100644 index 0000000000..755e27ec9e
> > --- /dev/null
> > +++ b/lib/pdcp/pdcp_crypto.c
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#include <rte_pdcp.h>
> > +
> > +#include "pdcp_crypto.h"
> > +
> > +int
> > +pdcp_crypto_sess_create(struct rte_pdcp_entity *entity, const struct
> > rte_pdcp_entity_conf *conf)
> > +{
> > +	RTE_SET_USED(entity);
> > +	RTE_SET_USED(conf);
> > +	return 0;
> > +}
> > +
> > +void
> > +pdcp_crypto_sess_destroy(struct rte_pdcp_entity *entity) {
> > +	RTE_SET_USED(entity);
> > +}
> > diff --git a/lib/pdcp/pdcp_crypto.h b/lib/pdcp/pdcp_crypto.h new file
> > mode 100644 index 0000000000..6563331d37
> > --- /dev/null
> > +++ b/lib/pdcp/pdcp_crypto.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#ifndef PDCP_CRYPTO_H
> > +#define PDCP_CRYPTO_H
> > +
> > +#include <rte_pdcp.h>
> > +
> > +int pdcp_crypto_sess_create(struct rte_pdcp_entity *entity,
> > +			    const struct rte_pdcp_entity_conf *conf);
> > +
> > +void pdcp_crypto_sess_destroy(struct rte_pdcp_entity *entity);
> > +
> > +#endif /* PDCP_CRYPTO_H */
> > diff --git a/lib/pdcp/pdcp_entity.h b/lib/pdcp/pdcp_entity.h new file
> > mode 100644 index 0000000000..ca1d56b516
> > --- /dev/null
> > +++ b/lib/pdcp/pdcp_entity.h
> > @@ -0,0 +1,95 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#ifndef PDCP_ENTITY_H
> > +#define PDCP_ENTITY_H
> > +
> > +#include <rte_common.h>
> > +#include <rte_crypto_sym.h>
> > +#include <rte_mempool.h>
> > +#include <rte_pdcp.h>
> > +#include <rte_security.h>
> > +
> > +struct entity_priv;
> > +
> > +/* IV generation function based on the entity configuration */
> > +typedef void (*iv_gen_t)(struct rte_crypto_op *cop, const struct
> > +entity_priv
> > *en_priv,
> > +			 uint32_t count);
> > +
> > +struct entity_state {
> > +	uint32_t rx_next;
> > +	uint32_t tx_next;
> > +	uint32_t rx_deliv;
> > +	uint32_t rx_reord;
> > +};
> > +
> > +/*
> > + * Layout of PDCP entity: [rte_pdcp_entity] [entity_priv]
> > +[entity_dl/ul]  */
> > +
> > +struct entity_priv {
> > +	/** Crypto sym session. */
> > +	struct rte_cryptodev_sym_session *crypto_sess;
> > +	/** Entity specific IV generation function. */
> > +	iv_gen_t iv_gen;
> > +	/** Entity state variables. */
> > +	struct entity_state state;
> > +	/** Flags. */
> > +	struct {
> > +		/** PDCP PDU has 4 byte MAC-I. */
> > +		uint64_t is_authenticated : 1;
> > +		/** Cipher offset & length in bits. */
> > +		uint64_t is_ciph_in_bits : 1;
> > +		/** Auth offset & length in bits. */
> > +		uint64_t is_auth_in_bits : 1;
> > +		/** Is UL/transmitting PDCP entity. */
> > +		uint64_t is_ul_entity : 1;
> > +		/** Is NULL auth. */
> > +		uint64_t is_null_auth : 1;
> > +	} flags;
> > +	/** Crypto op pool. */
> > +	struct rte_mempool *cop_pool;
> > +	/** PDCP header size. */
> > +	uint8_t hdr_sz;
> > +	/** PDCP AAD size. For AES-CMAC, additional message is prepended
> for
> > the operation. */
> > +	uint8_t aad_sz;
> > +	/** Device ID of the device to be used for offload. */
> > +	uint8_t dev_id;
> > +};
> > +
> > +struct entity_priv_dl_part {
> > +	/* NOTE: when in-order-delivery is supported, post PDCP packets
> > +would
> > need to cached. */
> > +	uint8_t dummy;
> > +};
> > +
> > +struct entity_priv_ul_part {
> > +	/*
> > +	 * NOTE: when re-establish is supported, plain PDCP packets &
> COUNT
> > values need to be
> > +	 * cached.
> > +	 */
> > +	uint8_t dummy;
> > +};
> > +
> > +static inline struct entity_priv *
> > +entity_priv_get(const struct rte_pdcp_entity *entity) {
> > +	return RTE_PTR_ADD(entity, sizeof(struct rte_pdcp_entity)); }
> > +
> > +static inline struct entity_priv_dl_part * entity_dl_part_get(const
> > +struct rte_pdcp_entity *entity) {
> > +	return RTE_PTR_ADD(entity, sizeof(struct rte_pdcp_entity) +
> > sizeof(struct entity_priv));
> > +}
> > +
> > +static inline struct entity_priv_ul_part * entity_ul_part_get(const
> > +struct rte_pdcp_entity *entity) {
> > +	return RTE_PTR_ADD(entity, sizeof(struct rte_pdcp_entity) +
> > sizeof(struct entity_priv));
> > +}
> > +
> > +static inline int
> > +pdcp_hdr_size_get(enum rte_security_pdcp_sn_size sn_size) {
> > +	return RTE_ALIGN_MUL_CEIL(sn_size, 8) / 8; }
> > +
> > +#endif /* PDCP_ENTITY_H */
> > diff --git a/lib/pdcp/pdcp_process.c b/lib/pdcp/pdcp_process.c new
> > file mode 100644 index 0000000000..d4b158536d
> > --- /dev/null
> > +++ b/lib/pdcp/pdcp_process.c
> > @@ -0,0 +1,138 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#include <rte_crypto.h>
> > +#include <rte_crypto_sym.h>
> > +#include <rte_cryptodev.h>
> > +#include <rte_memcpy.h>
> > +#include <rte_pdcp.h>
> > +#include <rte_pdcp_hdr.h>
> > +
> > +#include "pdcp_crypto.h"
> > +#include "pdcp_entity.h"
> > +#include "pdcp_process.h"
> > +
> > +static int
> > +pdcp_crypto_xfrm_get(const struct rte_pdcp_entity_conf *conf, struct
> > rte_crypto_sym_xform **c_xfrm,
> > +		     struct rte_crypto_sym_xform **a_xfrm) {
> > +	*c_xfrm = NULL;
> > +	*a_xfrm = NULL;
> > +
> > +	if (conf->crypto_xfrm == NULL)
> > +		return -EINVAL;
> > +
> > +	if (conf->crypto_xfrm->type == RTE_CRYPTO_SYM_XFORM_CIPHER)
> {
> > +		*c_xfrm = conf->crypto_xfrm;
> > +		*a_xfrm = conf->crypto_xfrm->next;
> > +	} else if (conf->crypto_xfrm->type ==
> > RTE_CRYPTO_SYM_XFORM_AUTH) {
> > +		*a_xfrm = conf->crypto_xfrm;
> > +		*c_xfrm = conf->crypto_xfrm->next;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +pdcp_entity_priv_populate(struct entity_priv *en_priv, const struct
> > rte_pdcp_entity_conf *conf)
> > +{
> > +	struct rte_crypto_sym_xform *c_xfrm, *a_xfrm;
> > +	int ret;
> > +
> > +	/**
> > +	 * flags.is_authenticated
> > +	 *
> > +	 * MAC-I would be added in case of control plane packets and when
> > authentication
> > +	 * transform is not NULL.
> > +	 */
> > +
> > +	if (conf->pdcp_xfrm.domain ==
> > RTE_SECURITY_PDCP_MODE_CONTROL)
> > +		en_priv->flags.is_authenticated = 1;
> 
> This check should be added after getting the xfrm.
> If domain == control and a_xfrm is NULL, then it should be error, right?

[Anoob] Lib PDCP would handle such cases. Even if a_xfrm is non NULL but is NULL auth, it is lib PDCP which would add zeroized digest. And a_xfrm == NULL is also treated as NULL auth generally. The comment above this explains the same. Idea is to have lib PDCP handle all possible cases rather than putting too much restrictions on both app & PMD.

> 
> > +
> > +	ret = pdcp_crypto_xfrm_get(conf, &c_xfrm, &a_xfrm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (a_xfrm != NULL)
> > +		en_priv->flags.is_authenticated = 1;
> > +
> > +	/**
> > +	 * flags.is_ciph_in_bits
> > +	 *
> > +	 * For ZUC & SNOW3G cipher algos, offset & length need to be
> > +provided
> > in bits.
> > +	 */
> > +
> > +	if ((c_xfrm->cipher.algo == RTE_CRYPTO_CIPHER_SNOW3G_UEA2)
> ||
> > +	    (c_xfrm->cipher.algo == RTE_CRYPTO_CIPHER_ZUC_EEA3))
> > +		en_priv->flags.is_ciph_in_bits = 1;
> > +
> > +	/**
> > +	 * flags.is_auth_in_bits
> > +	 *
> > +	 * For ZUC & SNOW3G authentication algos, offset & length need to
> be
> > provided in bits.
> > +	 */
> > +
> > +	if (a_xfrm != NULL) {
> > +		if ((a_xfrm->auth.algo ==
> RTE_CRYPTO_AUTH_SNOW3G_UIA2)
> > ||
> > +		    (a_xfrm->auth.algo == RTE_CRYPTO_AUTH_ZUC_EIA3))
> > +			en_priv->flags.is_auth_in_bits = 1;
> > +	}
> > +
> > +	/**
> > +	 * flags.is_ul_entity
> > +	 *
> > +	 * Indicate whether the entity is UL/transmitting PDCP entity.
> > +	 */
> > +	if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> > +		en_priv->flags.is_ul_entity = 1;
> > +
> > +	/**
> > +	 * flags.is_null_auth
> > +	 *
> > +	 * For NULL auth, 4B zeros need to be added by lib PDCP. Indicate
> that
> > +	 * algo is NULL auth to perform the same.
> > +	 */
> > +	if (a_xfrm != NULL && a_xfrm->auth.algo ==
> > RTE_CRYPTO_AUTH_NULL)
> > +		en_priv->flags.is_null_auth = 1;
> > +
> > +	/**
> > +	 * hdr_sz
> > +	 *
> > +	 * PDCP header size of the entity
> > +	 */
> > +	en_priv->hdr_sz = pdcp_hdr_size_get(conf->pdcp_xfrm.sn_size);
> > +
> > +	/**
> > +	 * aad_sz
> > +	 *
> > +	 * For AES-CMAC, additional message is prepended for processing.
> > +Need
> > to be trimmed after
> > +	 * crypto processing is done.
> > +	 */
> > +	if (a_xfrm != NULL && a_xfrm->auth.algo ==
> > RTE_CRYPTO_AUTH_AES_CMAC)
> > +		en_priv->aad_sz = 8;
> > +	else
> > +		en_priv->aad_sz = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +pdcp_process_func_set(struct rte_pdcp_entity *entity, const struct
> > rte_pdcp_entity_conf *conf)
> > +{
> > +	struct entity_priv *en_priv;
> > +	int ret;
> > +
> > +	if (entity == NULL || conf == NULL)
> > +		return -EINVAL;
> > +
> > +	en_priv = entity_priv_get(entity);
> > +
> > +	ret = pdcp_entity_priv_populate(en_priv, conf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > diff --git a/lib/pdcp/pdcp_process.h b/lib/pdcp/pdcp_process.h new
> > file mode 100644 index 0000000000..fd53fff0aa
> > --- /dev/null
> > +++ b/lib/pdcp/pdcp_process.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#ifndef PDCP_PROCESS_H
> > +#define PDCP_PROCESS_H
> > +
> > +#include <rte_pdcp.h>
> > +
> > +int
> > +pdcp_process_func_set(struct rte_pdcp_entity *entity, const struct
> > rte_pdcp_entity_conf *conf);
> > +
> > +#endif /* PDCP_PROCESS_H */
> > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new file mode
> > 100644 index 0000000000..8914548dbd
> > --- /dev/null
> > +++ b/lib/pdcp/rte_pdcp.c
> > @@ -0,0 +1,138 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#include <rte_errno.h>
> > +#include <rte_pdcp.h>
> > +#include <rte_malloc.h>
> > +
> > +#include "pdcp_crypto.h"
> > +#include "pdcp_entity.h"
> > +#include "pdcp_process.h"
> > +
> > +static int
> > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > +	int size;
> > +
> > +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct entity_priv);
> > +
> > +	if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_DOWNLINK)
> > +		size += sizeof(struct entity_priv_dl_part);
> > +	else if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> > +		size += sizeof(struct entity_priv_ul_part);
> > +	else
> > +		return -EINVAL;
> > +
> > +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > +
> > +struct rte_pdcp_entity *
> > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf) {
> > +	struct rte_pdcp_entity *entity = NULL;
> > +	struct entity_priv *en_priv;
> > +	int ret, entity_size;
> > +
> > +	if (conf == NULL || conf->cop_pool == NULL) {
> > +		rte_errno = -EINVAL;
> > +		return NULL;
> > +	}
> 
> errnos are normally set as positive values.

[Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc and all APIs are returning negative values in case of errors.

> 
> 
> > +
> > +	if (conf->pdcp_xfrm.en_ordering || conf-
> > >pdcp_xfrm.remove_duplicates || conf->is_slrb ||
> > +	    conf->en_sec_offload) {
> > +		rte_errno = -ENOTSUP;
> > +		return NULL;
> > +	}
> > +
> > +	/*
> > +	 * 6.3.2 PDCP SN
> > +	 * Length: 12 or 18 bits as indicated in table 6.3.2-1. The length
> > +of the
> > PDCP SN is
> > +	 * configured by upper layers (pdcp-SN-SizeUL, pdcp-SN-SizeDL, or
> > +sl-
> > PDCP-SN-Size in
> > +	 * TS 38.331 [3])
> > +	 */
> > +	if ((conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_12)
> &&
> > +	    (conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_18)) {
> > +		rte_errno = -ENOTSUP;
> > +		return NULL;
> > +	}
> 
> Check for PDCP crypto algos may also be added.
> As only 4 cipher and 4 auth algos are supported in case of PDCP.

[Anoob] Validation happens when we create session. Please check,
pdcp: add crypto session create and destroy

> 
> > +
> > +	if (conf->pdcp_xfrm.hfn || conf->pdcp_xfrm.hfn_threshold) {
> > +		rte_errno = -EINVAL;
> > +		return NULL;
> > +	}
> 
> What is the reason to set errno as EINVAL when HFN is set?

[Anoob] HFN is part of pdcp_xfrm which is defined in rte_security. Lib PDCP allows user to specify complete 32 bit count value using rte_pdcp_entity_conf.count. Since HFN is also used to construct 32 bit count value, having two ways to set count would be misleading. Hence lib PDCP would enforce that application does not set this value.

> 
> > +
> > +	entity_size = pdcp_entity_size_get(conf);
> > +	if (entity_size < 0) {
> > +		rte_errno = -EINVAL;
> > +		return NULL;
> > +	}
> > +
> > +	entity = rte_zmalloc_socket("pdcp_entity", entity_size,
> > RTE_CACHE_LINE_SIZE, SOCKET_ID_ANY);
> > +	if (entity == NULL) {
> > +		rte_errno = -ENOMEM;
> > +		return NULL;
> > +	}
> > +
> > +	en_priv = entity_priv_get(entity);
> > +
> > +	en_priv->state.rx_deliv = conf->count;
> > +	en_priv->state.tx_next = conf->count;
> > +	en_priv->cop_pool = conf->cop_pool;
> > +
> > +	/* Setup crypto session */
> > +	ret = pdcp_crypto_sess_create(entity, conf);
> > +	if (ret)
> > +		goto entity_free;
> > +
> > +	ret = pdcp_process_func_set(entity, conf);
> > +	if (ret)
> > +		goto crypto_sess_destroy;
> > +
> > +	return entity;
> > +
> > +crypto_sess_destroy:
> > +	pdcp_crypto_sess_destroy(entity);
> > +entity_free:
> > +	rte_free(entity);
> > +	rte_errno = ret;
> > +	return NULL;
> > +}
> > +
> > +int
> > +rte_pdcp_entity_release(struct rte_pdcp_entity *pdcp_entity, struct
> > +rte_mbuf
> > *out_mb[])
> > +{
> > +	if (pdcp_entity == NULL)
> > +		return -EINVAL;
> > +
> > +	/* Teardown crypto sessions */
> > +	pdcp_crypto_sess_destroy(pdcp_entity);
> > +
> > +	rte_free(pdcp_entity);
> > +
> > +	RTE_SET_USED(out_mb);
> > +	return 0;
> > +}
> > +
> > +int
> > +rte_pdcp_entity_suspend(struct rte_pdcp_entity *pdcp_entity,
> > +			struct rte_mbuf *out_mb[])
> > +{
> > +	struct entity_priv *en_priv;
> > +
> > +	if (pdcp_entity == NULL)
> > +		return -EINVAL;
> > +
> > +	en_priv = entity_priv_get(pdcp_entity);
> > +
> > +	if (en_priv->flags.is_ul_entity) {
> > +		en_priv->state.tx_next = 0;
> > +	} else {
> > +		en_priv->state.rx_next = 0;
> > +		en_priv->state.rx_deliv = 0;
> > +	}
> > +
> > +	RTE_SET_USED(out_mb);
> > +
> > +	return 0;
> > +}
> > diff --git a/lib/pdcp/rte_pdcp.h b/lib/pdcp/rte_pdcp.h new file mode
> > 100644 index 0000000000..33c355b05a
> > --- /dev/null
> > +++ b/lib/pdcp/rte_pdcp.h
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#ifndef RTE_PDCP_H
> > +#define RTE_PDCP_H
> > +
> > +/**
> > + * @file rte_pdcp.h
> > + *
> > + * RTE PDCP support.
> > + *
> > + * librte_pdcp provides a framework for PDCP protocol processing.
> 
> A framework for PDCP protocol processing.

[Anoob] Will do next version.

> 
> > + */
> > +
> > +#include <rte_compat.h>
> > +#include <rte_common.h>
> > +#include <rte_mempool.h>
> > +#include <rte_security.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * PDCP entity.
> 
> You can probably explain more on what a PDCP entity is.

[Anoob] Will do in next version.

> 
> > + */
> > +struct rte_pdcp_entity {
> > +	/**
> > +	 * PDCP entities may hold packets for purposes of in-order delivery
> (in
> > +	 * case of receiving PDCP entity) and re-transmission (in case of
> > +	 * transmitting PDCP entity).
> > +	 *
> > +	 * For receiving PDCP entity, it may hold packets when in-order
> > +	 * delivery is enabled. The packets would be cached until either a
> > +	 * packet that completes the sequence arrives or when t-Reordering
> > timer
> > +	 * expires.
> > +	 *
> > +	 * When post-processing of PDCP packet which completes a
> sequence is
> > +	 * done, the API may return more packets than enqueued.
> Application is
> > +	 * expected to provide *rte_pdcp_pkt_post_process()* with
> *out_mb*
> > +	 * which can hold maximum number of packets which may be
> returned.
> > +	 */
> 
> The above comment explains the need for holding the packets.
> But it does not talk about the parameter it is explaining.
> This explanation should be part of programmer's guide and not the API
> guide.

[Anoob] Agreed. Will make the required changes.

> 
> > +	uint32_t max_pkt_cache;
> > +	/** User area for saving application data. */
> > +	uint64_t user_area[2];
> 
> Is it being used right now in the patches?
> If not, can we add it later?
> And if really needed now, can we rename to user_data

[Anoob] Agreed. Will remove.

> 
> > +} __rte_cache_aligned;
> > +
> > +/**
> > + * PDCP entity configuration to be used for establishing an entity.
> > + */
> > +/* Structure rte_pdcp_entity_conf 8< */ struct rte_pdcp_entity_conf {
> > +	/** PDCP transform for the entity. */
> > +	struct rte_security_pdcp_xform pdcp_xfrm;
> > +	/** Crypto transform applicable for the entity. */
> > +	struct rte_crypto_sym_xform *crypto_xfrm;
> > +	/** Mempool for crypto symmetric session. */
> > +	struct rte_mempool *sess_mpool;
> > +	/** Crypto op pool.*/
> > +	struct rte_mempool *cop_pool;
> > +	/**
> > +	 * 32 bit count value (HFN + SN) to be used for the first packet.
> > +	 * pdcp_xfrm.hfn would be ignored as the HFN would be derived
> from
> > this value.
> > +	 */
> 
> If the HFN is to be ignored, then why to add a check in entity establish and
> return EINVAL?
> It should be silently ignored in that case with a debug print at max.

[Anoob] Explained above. Please check.

> 
> 
> > +	uint32_t count;
> > +	/** Indicate whether the PDCP entity belongs to Side Link Radio
> Bearer.
> > */
> > +	bool is_slrb;
> > +	/** Enable security offload on the device specified. */
> > +	bool en_sec_offload;
> > +	/** Device on which security/crypto session need to be created. */
> > +	uint8_t dev_id;
> > +	/** Reverse direction during IV generation. Can be used to simulate
> > +UE
> > crypto processing.*/
> > +	bool reverse_iv_direction;
> > +};
> > +/* >8 End of structure rte_pdcp_entity_conf. */
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * 5.1.1 PDCP entity establishment
> > + *
> > + * Establish PDCP entity based on provided input configuration.
> > + *
> > + * @param conf
> > + *   Parameters to be used for initializing PDCP entity object.
> > + * @return
> > + *   - Valid handle if success
> > + *   - NULL in case of failure. rte_errno will be set to error code
> > + */
> > +__rte_experimental
> > +struct rte_pdcp_entity *
> > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * 5.1.3 PDCP entity release
> > + *
> > + * Release PDCP entity.
> > + *
> > + * For UL/transmitting PDCP entity, all stored PDCP SDUs would be
> dropped.
> > + * For DL/receiving PDCP entity, the stored PDCP SDUs would be
> > +returned in
> > + * *out_mb* buffer. The buffer should be large enough to hold all
> > +cached
> > + * packets in the entity.
> > + *
> > + * @param pdcp_entity
> > + *   Pointer to the PDCP entity to be released.
> > + * @param[out] out_mb
> > + *   The address of an array that can hold up to
> > *rte_pdcp_entity.max_pkt_cache*
> > + *   pointers to *rte_mbuf* structures.
> > + * @return
> > + *   -  0: Success and no cached packets to return
> > + *   - >0: Success and the number of packets returned in out_mb
> > + *   - <0: Error code in case of failures
> > + */
> > +__rte_experimental
> > +int
> > +rte_pdcp_entity_release(struct rte_pdcp_entity *pdcp_entity,
> > +			struct rte_mbuf *out_mb[]);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * 5.1.4 PDCP entity suspend
> > + *
> > + * Suspend PDCP entity.
> > + *
> > + * For DL/receiving PDCP entity, the stored PDCP SDUs would be
> > +returned in
> > + * *out_mb* buffer. The buffer should be large enough to hold all
> > +cached
> > + * packets in the entity.
> > + *
> > + * For UL/transmitting PDCP entity, *out_mb* buffer would be unused.
> > + *
> > + * @param pdcp_entity
> > + *   Pointer to the PDCP entity to be suspended.
> > + * @param[out] out_mb
> > + *   The address of an array that can hold up to
> > *rte_pdcp_entity.max_pkt_cache*
> > + *   pointers to *rte_mbuf* structures.
> > + * @return
> > + *   -  0: Success and no cached packets to return
> > + *   - >0: Success and the number of packets returned in out_mb
> > + *   - <0: Error code in case of failures
> > + */
> > +__rte_experimental
> > +int
> > +rte_pdcp_entity_suspend(struct rte_pdcp_entity *pdcp_entity,
> > +			struct rte_mbuf *out_mb[]);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* RTE_PDCP_H */
> > diff --git a/lib/pdcp/version.map b/lib/pdcp/version.map new file mode
> > 100644 index 0000000000..923e165f3f
> > --- /dev/null
> > +++ b/lib/pdcp/version.map
> > @@ -0,0 +1,10 @@
> > +EXPERIMENTAL {
> > +	global:
> > +
> > +	# added in 23.07
> > +	rte_pdcp_entity_establish;
> > +	rte_pdcp_entity_release;
> > +	rte_pdcp_entity_suspend;
> > +
> > +	local: *;
> > +};
> > --
> > 2.25.1
  

Hi Anoob,
> > > +static int
> > > +pdcp_entity_priv_populate(struct entity_priv *en_priv, const struct
> > > rte_pdcp_entity_conf *conf)
> > > +{
> > > +	struct rte_crypto_sym_xform *c_xfrm, *a_xfrm;
> > > +	int ret;
> > > +
> > > +	/**
> > > +	 * flags.is_authenticated
> > > +	 *
> > > +	 * MAC-I would be added in case of control plane packets and when
> > > authentication
> > > +	 * transform is not NULL.
> > > +	 */
> > > +
> > > +	if (conf->pdcp_xfrm.domain ==
> > > RTE_SECURITY_PDCP_MODE_CONTROL)
> > > +		en_priv->flags.is_authenticated = 1;
> >
> > This check should be added after getting the xfrm.
> > If domain == control and a_xfrm is NULL, then it should be error, right?
> 
> [Anoob] Lib PDCP would handle such cases. Even if a_xfrm is non NULL but is
> NULL auth, it is lib PDCP which would add zeroized digest. And a_xfrm == NULL
> is also treated as NULL auth generally. The comment above this explains the
> same. Idea is to have lib PDCP handle all possible cases rather than putting too
> much restrictions on both app & PMD.

I believe there is a case in PDCP which allows no integrity as well.(LTE Uplane)
In that case, a_xfrm is NULL is not equivalent to NULL auth.
No integrity would mean no MAC-I.



> > > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new file mode
> > > 100644 index 0000000000..8914548dbd
> > > --- /dev/null
> > > +++ b/lib/pdcp/rte_pdcp.c
> > > @@ -0,0 +1,138 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright(C) 2023 Marvell.
> > > + */
> > > +
> > > +#include <rte_errno.h>
> > > +#include <rte_pdcp.h>
> > > +#include <rte_malloc.h>
> > > +
> > > +#include "pdcp_crypto.h"
> > > +#include "pdcp_entity.h"
> > > +#include "pdcp_process.h"
> > > +
> > > +static int
> > > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > > +	int size;
> > > +
> > > +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct entity_priv);
> > > +
> > > +	if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_DOWNLINK)
> > > +		size += sizeof(struct entity_priv_dl_part);
> > > +	else if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> > > +		size += sizeof(struct entity_priv_ul_part);
> > > +	else
> > > +		return -EINVAL;
> > > +
> > > +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > > +
> > > +struct rte_pdcp_entity *
> > > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf) {
> > > +	struct rte_pdcp_entity *entity = NULL;
> > > +	struct entity_priv *en_priv;
> > > +	int ret, entity_size;
> > > +
> > > +	if (conf == NULL || conf->cop_pool == NULL) {
> > > +		rte_errno = -EINVAL;
> > > +		return NULL;
> > > +	}
> >
> > errnos are normally set as positive values.
> 
> [Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc and all APIs are
> returning negative values in case of errors.

Check again lib/ethdev/rte_ethdev.c
rte_errno are set as positive values only and 
APIs return error numbers as negative values.


> 
> >
> >
> > > +
> > > +	if (conf->pdcp_xfrm.en_ordering || conf-
> > > >pdcp_xfrm.remove_duplicates || conf->is_slrb ||
> > > +	    conf->en_sec_offload) {
> > > +		rte_errno = -ENOTSUP;
> > > +		return NULL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * 6.3.2 PDCP SN
> > > +	 * Length: 12 or 18 bits as indicated in table 6.3.2-1. The length
> > > +of the
> > > PDCP SN is
> > > +	 * configured by upper layers (pdcp-SN-SizeUL, pdcp-SN-SizeDL, or
> > > +sl-
> > > PDCP-SN-Size in
> > > +	 * TS 38.331 [3])
> > > +	 */
> > > +	if ((conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_12)
> > &&
> > > +	    (conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_18)) {
> > > +		rte_errno = -ENOTSUP;
> > > +		return NULL;
> > > +	}
> >
> > Check for PDCP crypto algos may also be added.
> > As only 4 cipher and 4 auth algos are supported in case of PDCP.
> 
> [Anoob] Validation happens when we create session. Please check,
> pdcp: add crypto session create and destroy

Agreed, already acked that.

> 
> >
> > > +
> > > +	if (conf->pdcp_xfrm.hfn || conf->pdcp_xfrm.hfn_threshold) {
> > > +		rte_errno = -EINVAL;
> > > +		return NULL;
> > > +	}
> >
> > What is the reason to set errno as EINVAL when HFN is set?
> 
> [Anoob] HFN is part of pdcp_xfrm which is defined in rte_security. Lib PDCP
> allows user to specify complete 32 bit count value using
> rte_pdcp_entity_conf.count. Since HFN is also used to construct 32 bit count
> value, having two ways to set count would be misleading. Hence lib PDCP would
> enforce that application does not set this value.
> 
> >
> > > +/**
> > > + * PDCP entity configuration to be used for establishing an entity.
> > > + */
> > > +/* Structure rte_pdcp_entity_conf 8< */ struct rte_pdcp_entity_conf {
> > > +	/** PDCP transform for the entity. */
> > > +	struct rte_security_pdcp_xform pdcp_xfrm;
> > > +	/** Crypto transform applicable for the entity. */
> > > +	struct rte_crypto_sym_xform *crypto_xfrm;
> > > +	/** Mempool for crypto symmetric session. */
> > > +	struct rte_mempool *sess_mpool;
> > > +	/** Crypto op pool.*/
> > > +	struct rte_mempool *cop_pool;
> > > +	/**
> > > +	 * 32 bit count value (HFN + SN) to be used for the first packet.
> > > +	 * pdcp_xfrm.hfn would be ignored as the HFN would be derived
> > from
> > > this value.
> > > +	 */
> >
> > If the HFN is to be ignored, then why to add a check in entity establish and
> > return EINVAL?
> > It should be silently ignored in that case with a debug print at max.
> 
> [Anoob] Explained above. Please check.
> 
No it is not explained. Above comments says
" pdcp_xfrm.hfn would be ignored as the HFN would be derived from this value"

Here you are saying the field will be ignored.
But while creating entity, it is returning EINVAL if hfn is provided.
  

Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 18, 2023 1:10 PM
> To: Anoob Joseph <anoobj@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>; Bernard
> Iremonger <bernard.iremonger@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Mattias Rönnblom
> <mattias.ronnblom@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Volodymyr Fialko <vfialko@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>
> Subject: RE: [PATCH v2 02/22] lib: add pdcp protocol
> 
> Hi Anoob,
> > > > +static int
> > > > +pdcp_entity_priv_populate(struct entity_priv *en_priv, const
> > > > +struct
> > > > rte_pdcp_entity_conf *conf)
> > > > +{
> > > > +	struct rte_crypto_sym_xform *c_xfrm, *a_xfrm;
> > > > +	int ret;
> > > > +
> > > > +	/**
> > > > +	 * flags.is_authenticated
> > > > +	 *
> > > > +	 * MAC-I would be added in case of control plane packets and
> > > > +when
> > > > authentication
> > > > +	 * transform is not NULL.
> > > > +	 */
> > > > +
> > > > +	if (conf->pdcp_xfrm.domain ==
> > > > RTE_SECURITY_PDCP_MODE_CONTROL)
> > > > +		en_priv->flags.is_authenticated = 1;
> > >
> > > This check should be added after getting the xfrm.
> > > If domain == control and a_xfrm is NULL, then it should be error, right?
> >
> > [Anoob] Lib PDCP would handle such cases. Even if a_xfrm is non NULL
> > but is NULL auth, it is lib PDCP which would add zeroized digest. And
> > a_xfrm == NULL is also treated as NULL auth generally. The comment
> > above this explains the same. Idea is to have lib PDCP handle all
> > possible cases rather than putting too much restrictions on both app &
> PMD.
> 
> I believe there is a case in PDCP which allows no integrity as well.(LTE Uplane)
> In that case, a_xfrm is NULL is not equivalent to NULL auth.
> No integrity would mean no MAC-I.

[Anoob] The comment explains the logic. If it is control plane, then digest is must and that case lib PDCP is trying to handle. Anyway, adding an extra error check and returning error in that case is not a big deal. Will add that in the appropriate place.

> 
> 
> 
> > > > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new file
> > > > mode
> > > > 100644 index 0000000000..8914548dbd
> > > > --- /dev/null
> > > > +++ b/lib/pdcp/rte_pdcp.c
> > > > @@ -0,0 +1,138 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(C) 2023 Marvell.
> > > > + */
> > > > +
> > > > +#include <rte_errno.h>
> > > > +#include <rte_pdcp.h>
> > > > +#include <rte_malloc.h>
> > > > +
> > > > +#include "pdcp_crypto.h"
> > > > +#include "pdcp_entity.h"
> > > > +#include "pdcp_process.h"
> > > > +
> > > > +static int
> > > > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > > > +	int size;
> > > > +
> > > > +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct
> > > > +entity_priv);
> > > > +
> > > > +	if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_DOWNLINK)
> > > > +		size += sizeof(struct entity_priv_dl_part);
> > > > +	else if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> > > > +		size += sizeof(struct entity_priv_ul_part);
> > > > +	else
> > > > +		return -EINVAL;
> > > > +
> > > > +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > > > +
> > > > +struct rte_pdcp_entity *
> > > > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf) {
> > > > +	struct rte_pdcp_entity *entity = NULL;
> > > > +	struct entity_priv *en_priv;
> > > > +	int ret, entity_size;
> > > > +
> > > > +	if (conf == NULL || conf->cop_pool == NULL) {
> > > > +		rte_errno = -EINVAL;
> > > > +		return NULL;
> > > > +	}
> > >
> > > errnos are normally set as positive values.
> >
> > [Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc and
> > all APIs are returning negative values in case of errors.
> 
> Check again lib/ethdev/rte_ethdev.c
> rte_errno are set as positive values only and APIs return error numbers as
> negative values.

[Anoob] Okay. There are many APIs were this is not done correctly (check rte_flow.h) . For lib PDCP additions, I'll have this handled. Some of these conventions should be documented to avoid confusion.

> 
> 
> >
> > >
> > >
> > > > +
> > > > +	if (conf->pdcp_xfrm.en_ordering || conf-
> > > > >pdcp_xfrm.remove_duplicates || conf->is_slrb ||
> > > > +	    conf->en_sec_offload) {
> > > > +		rte_errno = -ENOTSUP;
> > > > +		return NULL;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * 6.3.2 PDCP SN
> > > > +	 * Length: 12 or 18 bits as indicated in table 6.3.2-1. The length
> > > > +of the
> > > > PDCP SN is
> > > > +	 * configured by upper layers (pdcp-SN-SizeUL, pdcp-SN-SizeDL, or
> > > > +sl-
> > > > PDCP-SN-Size in
> > > > +	 * TS 38.331 [3])
> > > > +	 */
> > > > +	if ((conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_12)
> > > &&
> > > > +	    (conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_18)) {
> > > > +		rte_errno = -ENOTSUP;
> > > > +		return NULL;
> > > > +	}
> > >
> > > Check for PDCP crypto algos may also be added.
> > > As only 4 cipher and 4 auth algos are supported in case of PDCP.
> >
> > [Anoob] Validation happens when we create session. Please check,
> > pdcp: add crypto session create and destroy
> 
> Agreed, already acked that.
> 
> >
> > >
> > > > +
> > > > +	if (conf->pdcp_xfrm.hfn || conf->pdcp_xfrm.hfn_threshold) {
> > > > +		rte_errno = -EINVAL;
> > > > +		return NULL;
> > > > +	}
> > >
> > > What is the reason to set errno as EINVAL when HFN is set?
> >
> > [Anoob] HFN is part of pdcp_xfrm which is defined in rte_security. Lib PDCP
> > allows user to specify complete 32 bit count value using
> > rte_pdcp_entity_conf.count. Since HFN is also used to construct 32 bit
> count
> > value, having two ways to set count would be misleading. Hence lib PDCP
> would
> > enforce that application does not set this value.
> >
> > >
> > > > +/**
> > > > + * PDCP entity configuration to be used for establishing an entity.
> > > > + */
> > > > +/* Structure rte_pdcp_entity_conf 8< */ struct rte_pdcp_entity_conf
> {
> > > > +	/** PDCP transform for the entity. */
> > > > +	struct rte_security_pdcp_xform pdcp_xfrm;
> > > > +	/** Crypto transform applicable for the entity. */
> > > > +	struct rte_crypto_sym_xform *crypto_xfrm;
> > > > +	/** Mempool for crypto symmetric session. */
> > > > +	struct rte_mempool *sess_mpool;
> > > > +	/** Crypto op pool.*/
> > > > +	struct rte_mempool *cop_pool;
> > > > +	/**
> > > > +	 * 32 bit count value (HFN + SN) to be used for the first packet.
> > > > +	 * pdcp_xfrm.hfn would be ignored as the HFN would be derived
> > > from
> > > > this value.
> > > > +	 */
> > >
> > > If the HFN is to be ignored, then why to add a check in entity establish
> and
> > > return EINVAL?
> > > It should be silently ignored in that case with a debug print at max.
> >
> > [Anoob] Explained above. Please check.
> >
> No it is not explained. Above comments says
> " pdcp_xfrm.hfn would be ignored as the HFN would be derived from this
> value"
> 
> Here you are saying the field will be ignored.
> But while creating entity, it is returning EINVAL if hfn is provided.

[Anoob] Agreed. Will address in next version.
  

> > > > > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new file
> > > > > mode
> > > > > 100644 index 0000000000..8914548dbd
> > > > > --- /dev/null
> > > > > +++ b/lib/pdcp/rte_pdcp.c
> > > > > @@ -0,0 +1,138 @@
> > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > + * Copyright(C) 2023 Marvell.
> > > > > + */
> > > > > +
> > > > > +#include <rte_errno.h>
> > > > > +#include <rte_pdcp.h>
> > > > > +#include <rte_malloc.h>
> > > > > +
> > > > > +#include "pdcp_crypto.h"
> > > > > +#include "pdcp_entity.h"
> > > > > +#include "pdcp_process.h"
> > > > > +
> > > > > +static int
> > > > > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > > > > +	int size;
> > > > > +
> > > > > +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct
> > > > > +entity_priv);
> > > > > +
> > > > > +	if (conf->pdcp_xfrm.pkt_dir ==
> RTE_SECURITY_PDCP_DOWNLINK)
> > > > > +		size += sizeof(struct entity_priv_dl_part);
> > > > > +	else if (conf->pdcp_xfrm.pkt_dir ==
> RTE_SECURITY_PDCP_UPLINK)
> > > > > +		size += sizeof(struct entity_priv_ul_part);
> > > > > +	else
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > > > > +
> > > > > +struct rte_pdcp_entity *
> > > > > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf) {
> > > > > +	struct rte_pdcp_entity *entity = NULL;
> > > > > +	struct entity_priv *en_priv;
> > > > > +	int ret, entity_size;
> > > > > +
> > > > > +	if (conf == NULL || conf->cop_pool == NULL) {
> > > > > +		rte_errno = -EINVAL;
> > > > > +		return NULL;
> > > > > +	}
> > > >
> > > > errnos are normally set as positive values.
> > >
> > > [Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc and
> > > all APIs are returning negative values in case of errors.
> >
> > Check again lib/ethdev/rte_ethdev.c
> > rte_errno are set as positive values only and APIs return error numbers as
> > negative values.
> 
> [Anoob] Okay. There are many APIs were this is not done correctly (check
> rte_flow.h) . For lib PDCP additions, I'll have this handled. Some of these
> conventions should be documented to avoid confusion.

I am not sure what you are referring to. 
I cannot find any discrepancy in rte_flow.c as well.

Can you give an example where it is wrong. We can ask specific people to fix that.

Regarding documentation.
It is mentioned in rte_errno.h
/**
 * Error number value, stored per-thread, which can be queried after
 * calls to certain functions to determine why those functions failed.
 *
 * Uses standard values from errno.h wherever possible, with a small number
 * of additional possible values for RTE-specific conditions.
 */
#define rte_errno RTE_PER_LCORE(_rte_errno)

And errno.h has all positive values defined.
  

Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 18, 2023 2:16 PM
> To: Anoob Joseph <anoobj@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>; Bernard
> Iremonger <bernard.iremonger@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Mattias Rönnblom
> <mattias.ronnblom@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Volodymyr Fialko <vfialko@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>
> Subject: RE: [PATCH v2 02/22] lib: add pdcp protocol
> 
> > > > > > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new
> > > > > > file mode
> > > > > > 100644 index 0000000000..8914548dbd
> > > > > > --- /dev/null
> > > > > > +++ b/lib/pdcp/rte_pdcp.c
> > > > > > @@ -0,0 +1,138 @@
> > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > + * Copyright(C) 2023 Marvell.
> > > > > > + */
> > > > > > +
> > > > > > +#include <rte_errno.h>
> > > > > > +#include <rte_pdcp.h>
> > > > > > +#include <rte_malloc.h>
> > > > > > +
> > > > > > +#include "pdcp_crypto.h"
> > > > > > +#include "pdcp_entity.h"
> > > > > > +#include "pdcp_process.h"
> > > > > > +
> > > > > > +static int
> > > > > > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > > > > > +	int size;
> > > > > > +
> > > > > > +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct
> > > > > > +entity_priv);
> > > > > > +
> > > > > > +	if (conf->pdcp_xfrm.pkt_dir ==
> > RTE_SECURITY_PDCP_DOWNLINK)
> > > > > > +		size += sizeof(struct entity_priv_dl_part);
> > > > > > +	else if (conf->pdcp_xfrm.pkt_dir ==
> > RTE_SECURITY_PDCP_UPLINK)
> > > > > > +		size += sizeof(struct entity_priv_ul_part);
> > > > > > +	else
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > > > > > +
> > > > > > +struct rte_pdcp_entity *
> > > > > > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf
> *conf) {
> > > > > > +	struct rte_pdcp_entity *entity = NULL;
> > > > > > +	struct entity_priv *en_priv;
> > > > > > +	int ret, entity_size;
> > > > > > +
> > > > > > +	if (conf == NULL || conf->cop_pool == NULL) {
> > > > > > +		rte_errno = -EINVAL;
> > > > > > +		return NULL;
> > > > > > +	}
> > > > >
> > > > > errnos are normally set as positive values.
> > > >
> > > > [Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc
> > > > and all APIs are returning negative values in case of errors.
> > >
> > > Check again lib/ethdev/rte_ethdev.c
> > > rte_errno are set as positive values only and APIs return error
> > > numbers as negative values.
> >
> > [Anoob] Okay. There are many APIs were this is not done correctly
> > (check
> > rte_flow.h) . For lib PDCP additions, I'll have this handled. Some of
> > these conventions should be documented to avoid confusion.
> 
> I am not sure what you are referring to.
> I cannot find any discrepancy in rte_flow.c as well.
> 
> Can you give an example where it is wrong. We can ask specific people to fix
> that.
> 
> Regarding documentation.
> It is mentioned in rte_errno.h
> /**
>  * Error number value, stored per-thread, which can be queried after
>  * calls to certain functions to determine why those functions failed.
>  *
>  * Uses standard values from errno.h wherever possible, with a small
> number
>  * of additional possible values for RTE-specific conditions.
>  */
> #define rte_errno RTE_PER_LCORE(_rte_errno)
> 
> And errno.h has all positive values defined.

[Anoob] Agreed. There are descriptions in rte_flow.h which got us confused. Thanks for the clarification. Changes in PDCP will come in next version.