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.a...@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.anan...@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.dohe...@intel.com>

Reply via email to