Re: [PATCH v3 4/9] lib: introduce ipsec library

["Doherty, Declan" <declan.doherty@intel.com>]

On 06/12/2018 3:38 PM, Konstantin Ananyev wrote:
> Introduce librte_ipsec library.
> The library is supposed to utilize existing DPDK crypto-dev and
> security API to provide application with transparent IPsec processing API.
> That initial commit provides some base API to manage
> IPsec Security Association (SA) object.
> 

So cosmetics change suggested, otherwise looks fine to me.

> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>   MAINTAINERS                            |   5 +
...

> +
> +#ifndef _IPSEC_SQN_H_
> +#define _IPSEC_SQN_H_
> +
> +#define WINDOW_BUCKET_BITS		6 /* uint64_t */ > +#define WINDOW_BUCKET_SIZE		(1 << WINDOW_BUCKET_BITS)

1 << 6 is a really confusing way of defining a 64 bit bucket size, is it 
necessary to define this way?

> +#define WINDOW_BIT_LOC_MASK		(WINDOW_BUCKET_SIZE - 1)
> +
> +/* minimum number of bucket, power of 2*/
> +#define WINDOW_BUCKET_MIN		2
> +#define WINDOW_BUCKET_MAX		(INT16_MAX + 1)
> +
> +#define IS_ESN(sa)	((sa)->sqn_mask == UINT64_MAX)
> +
> +/*
> + * for given size, calculate required number of buckets.
> + */
> +static uint32_t
> +replay_num_bucket(uint32_t wsz)
> +{
> +	uint32_t nb;
> +
> +	nb = rte_align32pow2(RTE_ALIGN_MUL_CEIL(wsz, WINDOW_BUCKET_SIZE) /
> +		WINDOW_BUCKET_SIZE);
> +	nb = RTE_MAX(nb, (uint32_t)WINDOW_BUCKET_MIN);
> +
> +	return nb;
> +}
> +
> +/**
> + * Based on number of buckets calculated required size for the
> + * structure that holds replay window and sequnce number (RSN) information.

                                              ^^ typo

> + */
> +static size_t
> +rsn_size(uint32_t nb_bucket)
> +{
> +	size_t sz;
> +	struct replay_sqn *rsn;
> +
> +	sz = sizeof(*rsn) + nb_bucket * sizeof(rsn->window[0]);
> +	sz = RTE_ALIGN_CEIL(sz, RTE_CACHE_LINE_SIZE);
> +	return sz;
> +}
...

> +/**
> + * SA type is an 64-bit value that contain the following information:
> + * - IP version (IPv4/IPv6)
> + * - IPsec proto (ESP/AH)
> + * - inbound/outbound
> + * - mode (TRANSPORT/TUNNEL)
> + * - for TUNNEL outer IP version (IPv4/IPv6)
> + * ...
> + */
> +
> +enum {
> +	RTE_SATP_LOG_IPV,
> +	RTE_SATP_LOG_PROTO,
> +	RTE_SATP_LOG_DIR,
> +	RTE_SATP_LOG_MODE,
> +	RTE_SATP_LOG_NUM
> +};
> +
> +#define RTE_IPSEC_SATP_IPV_MASK		(1ULL << RTE_SATP_LOG_IPV)
> +#define RTE_IPSEC_SATP_IPV4		(0ULL << RTE_SATP_LOG_IPV)
> +#define RTE_IPSEC_SATP_IPV6		(1ULL << RTE_SATP_LOG_IPV)
> +
> +#define RTE_IPSEC_SATP_PROTO_MASK	(1ULL << RTE_SATP_LOG_PROTO)
> +#define RTE_IPSEC_SATP_PROTO_AH		(0ULL << RTE_SATP_LOG_PROTO)
> +#define RTE_IPSEC_SATP_PROTO_ESP	(1ULL << RTE_SATP_LOG_PROTO)
> +
> +#define RTE_IPSEC_SATP_DIR_MASK		(1ULL << RTE_SATP_LOG_DIR)
> +#define RTE_IPSEC_SATP_DIR_IB		(0ULL << RTE_SATP_LOG_DIR)
> +#define RTE_IPSEC_SATP_DIR_OB		(1ULL << RTE_SATP_LOG_DIR)
> +
> +#define RTE_IPSEC_SATP_MODE_MASK	(3ULL << RTE_SATP_LOG_MODE)
> +#define RTE_IPSEC_SATP_MODE_TRANS	(0ULL << RTE_SATP_LOG_MODE)
> +#define RTE_IPSEC_SATP_MODE_TUNLV4	(1ULL << RTE_SATP_LOG_MODE)
> +#define RTE_IPSEC_SATP_MODE_TUNLV6	(2ULL << RTE_SATP_LOG_MODE)
> +

for readability in the rest of the code I would suggest that using 
either use RTE_IPSEC_SA_TYPE_ or  just RTE_IPSEC_SA_ in the definitions 
above. Also in the enumeration it's not clear to me what the the _LOG_ 
means, it's being used as the offset, so maybe _OFFSET_ would be a 
better name but I I think it might clearer if absolute bit offsets were 
used.

> +/**
> + * get type of given SA
> + * @return
> + *   SA type value.
> + */
> +uint64_t __rte_experimental
> +rte_ipsec_sa_type(const struct rte_ipsec_sa *sa);
> +
> +/**
> + * Calculate requied SA size based on provided input parameters.
> + * @param prm
> + *   Parameters that wil be used to initialise SA object.
                         ^^ typo
> + * @return
> + *   - Actual size required for SA with given parameters.
> + *   - -EINVAL if the parameters are invalid.
> + */
> +int __rte_experimental
> +rte_ipsec_sa_size(const struct rte_ipsec_sa_prm *prm);
> +
> +/**

...

>   _LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
> 


Acked-by: Declan Doherty <declan.doherty@intel.com>