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.
