From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 9EC5169D4 for ; Tue, 11 Dec 2018 18:25:38 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2018 09:25:37 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,343,1539673200"; d="scan'208";a="97926091" Received: from dwdohert-mobl.ger.corp.intel.com (HELO [163.33.176.66]) ([163.33.176.66]) by orsmga007.jf.intel.com with ESMTP; 11 Dec 2018 09:25:36 -0800 To: Konstantin Ananyev , dev@dpdk.org Cc: Mohammad Abdul Awal References: <1543596366-22617-2-git-send-email-konstantin.ananyev@intel.com> <1544110714-4514-5-git-send-email-konstantin.ananyev@intel.com> From: "Doherty, Declan" Message-ID: Date: Tue, 11 Dec 2018 17:25:36 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.3.3 MIME-Version: 1.0 In-Reply-To: <1544110714-4514-5-git-send-email-konstantin.ananyev@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 4/9] lib: introduce ipsec library X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Dec 2018 17:25:39 -0000 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 > Signed-off-by: Konstantin Ananyev > --- > 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