From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 0C5C34C99 for ; Mon, 22 Oct 2018 00:01:51 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Oct 2018 15:01:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,409,1534834800"; d="scan'208";a="267597183" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga005.jf.intel.com with ESMTP; 21 Oct 2018 15:01:49 -0700 Received: from irsmsx106.ger.corp.intel.com ([169.254.8.45]) by IRSMSX152.ger.corp.intel.com ([169.254.6.110]) with mapi id 14.03.0319.002; Sun, 21 Oct 2018 23:01:48 +0100 From: "Ananyev, Konstantin" To: Jerin Jacob CC: "dev@dpdk.org" , "Awal, Mohammad Abdul" , "Joseph, Anoob" , "Athreya, Narayana Prasad" Thread-Topic: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API Thread-Index: AQHUX/1MO7G62AqbpUemv5mhomzRtqUlQeGAgAUEBGA= Date: Sun, 21 Oct 2018 22:01:48 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580102FEA53E@IRSMSX106.ger.corp.intel.com> References: <1535129598-27301-1-git-send-email-konstantin.ananyev@intel.com> <1539109420-13412-6-git-send-email-konstantin.ananyev@intel.com> <20181018173745.GA14157@jerin> In-Reply-To: <20181018173745.GA14157@jerin> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTAzYTU5MzEtZjgzZS00ODYyLTg1ODgtNjA0ZTNhODQ3MGYwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRjVSODNVNUNTNmdZejJJZkNPY3BLVmJrYjZLNmhWcVlXVnl5YnBuZTZOYVplSzB3b0pZVityK0MwVnptZCswOCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API 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: Sun, 21 Oct 2018 22:01:52 -0000 Hi Jerin, >=20 > Hi Konstantin, >=20 > Overall it looks good, but some comments on event mode integration in > performance effective way. >=20 > > > > Introduce Security Association (SA-level) data-path API > > Operates at SA level, provides functions to: > > - initialize/teardown SA object > > - process inbound/outbound ESP/AH packets associated with the given= SA > > (decrypt/encrypt, authenticate, check integrity, > > add/remove ESP/AH related headers and data, etc.). > > > > Signed-off-by: Mohammad Abdul Awal > > Signed-off-by: Konstantin Ananyev > > --- > > lib/librte_ipsec/Makefile | 2 + > > lib/librte_ipsec/meson.build | 4 +- > > lib/librte_ipsec/rte_ipsec.h | 154 +++++++++++++++++++++++++= ++++++++ > > lib/librte_ipsec/rte_ipsec_version.map | 3 + > > lib/librte_ipsec/sa.c | 98 ++++++++++++++++++++- > > lib/librte_ipsec/sa.h | 3 + > > lib/librte_ipsec/ses.c | 45 ++++++++++ > > 7 files changed, 306 insertions(+), 3 deletions(-) > > create mode 100644 lib/librte_ipsec/rte_ipsec.h > > create mode 100644 lib/librte_ipsec/ses.c > > > > diff --git a/lib/librte_ipsec/Makefile b/lib/librte_ipsec/Makefile > > index 7758dcc6d..79f187fae 100644 > > --- a/lib/librte_ipsec/Makefile > > +++ b/lib/librte_ipsec/Makefile > > @@ -17,8 +17,10 @@ LIBABIVER :=3D 1 > > > > # all source are stored in SRCS-y > > SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) +=3D sa.c > > +SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) +=3D ses.c > > > > # install header files > > +SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include +=3D rte_ipsec.h > > SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include +=3D rte_ipsec_sa.h > > > > include $(RTE_SDK)/mk/rte.lib.mk > > diff --git a/lib/librte_ipsec/meson.build b/lib/librte_ipsec/meson.buil= d > > index 52c78eaeb..6e8c6fabe 100644 > > --- a/lib/librte_ipsec/meson.build > > +++ b/lib/librte_ipsec/meson.build > > @@ -3,8 +3,8 @@ > > > > allow_experimental_apis =3D true > > > > -sources=3Dfiles('sa.c') > > +sources=3Dfiles('sa.c', 'ses.c') > > > > -install_headers =3D files('rte_ipsec_sa.h') > > +install_headers =3D files('rte_ipsec.h', 'rte_ipsec_sa.h') > > > > deps +=3D ['mbuf', 'net', 'cryptodev', 'security'] > > diff --git a/lib/librte_ipsec/rte_ipsec.h b/lib/librte_ipsec/rte_ipsec.= h > > new file mode 100644 > > index 000000000..5c9a1ed0b > > --- /dev/null > > +++ b/lib/librte_ipsec/rte_ipsec.h > > @@ -0,0 +1,154 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2018 Intel Corporation > > + */ > > + > > +#ifndef _RTE_IPSEC_H_ > > +#define _RTE_IPSEC_H_ > > + > > +/** > > + * @file rte_ipsec.h > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * RTE IPsec support. > > + * librte_ipsec provides a framework for data-path IPsec protocol > > + * processing (ESP/AH). > > + * IKEv2 protocol support right now is out of scope of that draft. > > + * Though it tries to define related API in such way, that it could be= adopted > > + * by IKEv2 implementation. > > + */ > > + > > +#include > > +#include > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +struct rte_ipsec_session; > > + > > +/** > > + * IPsec session specific functions that will be used to: > > + * - prepare - for input mbufs and given IPsec session prepare crypto = ops > > + * that can be enqueued into the cryptodev associated with given ses= sion > > + * (see *rte_ipsec_crypto_prepare* below for more details). > > + * - process - finalize processing of packets after crypto-dev finishe= d > > + * with them or process packets that are subjects to inline IPsec of= fload > > + * (see rte_ipsec_process for more details). > > + */ > > +struct rte_ipsec_sa_func { > > + uint16_t (*prepare)(const struct rte_ipsec_session *ss, > > + struct rte_mbuf *mb[], > > + struct rte_crypto_op *cop[], > > + uint16_t num); > > + uint16_t (*process)(const struct rte_ipsec_session *ss, > > + struct rte_mbuf *mb[], > > + uint16_t num); >=20 > IMO, It makes sense to have separate function pointers for inbound and > outbound so that, implementation would be clean and we can avoid a > "if" check. SA object by itself is unidirectional (either inbound or outbound), so I don't think we need 2 function pointers here. Yes, right now, inside ipsec lib we select functions by action_type only, but it doesn't have to stay that way. It could be action_type, direction, mode (tunnel/transport), event/poll, et= c. Let say inline_proto_process() could be split into: inline_proto_outb_process() and inline_proto_inb_process() and=20 rte_ipsec_sa_func.process will point to appropriate one. I probably will change things that way for next version. >=20 > > +}; > > + > > +/** > > + * rte_ipsec_session is an aggregate structure that defines particular > > + * IPsec Security Association IPsec (SA) on given security/crypto devi= ce: > > + * - pointer to the SA object > > + * - security session action type > > + * - pointer to security/crypto session, plus other related data > > + * - session/device specific functions to prepare/process IPsec packet= s. > > + */ > > +struct rte_ipsec_session { > > + > > + /** > > + * SA that session belongs to. > > + * Note that multiple sessions can belong to the same SA. > > + */ > > + struct rte_ipsec_sa *sa; > > + /** session action type */ > > + enum rte_security_session_action_type type; > > + /** session and related data */ > > + union { > > + struct { > > + struct rte_cryptodev_sym_session *ses; > > + } crypto; > > + struct { > > + struct rte_security_session *ses; > > + struct rte_security_ctx *ctx; > > + uint32_t ol_flags; > > + } security; > > + }; > > + /** functions to prepare/process IPsec packets */ > > + struct rte_ipsec_sa_func func; > > +}; >=20 > IMO, it can be cache aligned as it is used in fast path. Good point, will add. >=20 > > + > > +/** > > + * Checks that inside given rte_ipsec_session crypto/security fields > > + * are filled correctly and setups function pointers based on these va= lues. > > + * @param ss > > + * Pointer to the *rte_ipsec_session* object > > + * @return > > + * - Zero if operation completed successfully. > > + * - -EINVAL if the parameters are invalid. > > + */ > > +int __rte_experimental > > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss); > > + > > +/** > > + * For input mbufs and given IPsec session prepare crypto ops that can= be > > + * enqueued into the cryptodev associated with given session. > > + * expects that for each input packet: > > + * - l2_len, l3_len are setup correctly > > + * Note that erroneous mbufs are not freed by the function, > > + * but are placed beyond last valid mbuf in the *mb* array. > > + * It is a user responsibility to handle them further. > > + * @param ss > > + * Pointer to the *rte_ipsec_session* object the packets belong to. > > + * @param mb > > + * The address of an array of *num* pointers to *rte_mbuf* structure= s > > + * which contain the input packets. > > + * @param cop > > + * The address of an array of *num* pointers to the output *rte_cryp= to_op* > > + * structures. > > + * @param num > > + * The maximum number of packets to process. > > + * @return > > + * Number of successfully processed packets, with error code set in = rte_errno. > > + */ > > +static inline uint16_t __rte_experimental > > +rte_ipsec_crypto_prepare(const struct rte_ipsec_session *ss, > > + struct rte_mbuf *mb[], struct rte_crypto_op *cop[], uint16_t nu= m) > > +{ > > + return ss->func.prepare(ss, mb, cop, num); > > +} > > + > > +/** > > + * Finalise processing of packets after crypto-dev finished with them = or > > + * process packets that are subjects to inline IPsec offload. > > + * Expects that for each input packet: > > + * - l2_len, l3_len are setup correctly > > + * Output mbufs will be: > > + * inbound - decrypted & authenticated, ESP(AH) related headers remove= d, > > + * *l2_len* and *l3_len* fields are updated. > > + * outbound - appropriate mbuf fields (ol_flags, tx_offloads, etc.) > > + * properly setup, if necessary - IP headers updated, ESP(AH) fields a= dded, > > + * Note that erroneous mbufs are not freed by the function, > > + * but are placed beyond last valid mbuf in the *mb* array. > > + * It is a user responsibility to handle them further. > > + * @param ss > > + * Pointer to the *rte_ipsec_session* object the packets belong to. > > + * @param mb > > + * The address of an array of *num* pointers to *rte_mbuf* structure= s > > + * which contain the input packets. > > + * @param num > > + * The maximum number of packets to process. > > + * @return > > + * Number of successfully processed packets, with error code set in = rte_errno. > > + */ > > +static inline uint16_t __rte_experimental > > +rte_ipsec_process(const struct rte_ipsec_session *ss, struct rte_mbuf = *mb[], > > + uint16_t num) > > +{ > > + return ss->func.process(ss, mb, num); > > +} >=20 > Since we have separate functions and different application path for diff= erent mode > and the arguments also differ. I think, It is better to integrate > event mode like following >=20 > static inline uint16_t __rte_experimental > rte_ipsec_event_process(const struct rte_ipsec_session *ss, struct rte_ev= ent *ev[], uint16_t num) > { > return ss->func.event_process(ss, ev, num); > } To fulfill that, we can either have 2 separate function pointers: uint16_t (*pkt_process)( const struct rte_ipsec_session *ss, struct rte_mbu= f *mb[],uint16_t num); uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte_e= vent *ev[],uint16_t num); Or we can keep one function pointer, but change it to accept just array of = pointers: uint16_t (*process)( const struct rte_ipsec_session *ss, void *in[],uint16_= t num); and then make session_prepare() to choose a proper function based on input. I am ok with both schemes, but second one seems a bit nicer to me.=20 >=20 > This is to, > 1) Avoid Event mode application code duplication > 2) Better I$ utilization rather moving event specific and mbuff > specific at different code locations > 3) Better performance as inside one function pointer we can do things > in one shot rather splitting the work to application and library. > 4) Event mode has different modes like ATQ, non ATQ etc, These things > we can abstract through exiting function pointer scheme. > 5) atomicity & ordering problems can be sorted out internally with the ev= ents, > having one function pointer for event would be enough. >=20 > We will need some event related info (event dev, port, atomic queue to > use etc) which need to be added in rte_ipsec_session *ss as UNION so it > wont impact the normal mode. This way, all the required functionality of = this library > can be used with event-based model. =20 Yes, to support event model, I imagine ipsec_session might need to contain some event specific data. I am absolutely ok with that idea in general. Related fields can be added to the ipsec_session structure as needed, together with actual event mode implementation.=20 > > See below some implementation thoughts on this. >=20 > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif /* _RTE_IPSEC_H_ */ > > diff --git a/lib/librte_ipsec/rte_ipsec_version.map b/lib/librte_ipsec/= rte_ipsec_version.map > > +const struct rte_ipsec_sa_func * > > +ipsec_sa_func_select(const struct rte_ipsec_session *ss) > > +{ > > + static const struct rte_ipsec_sa_func tfunc[] =3D { > > + [RTE_SECURITY_ACTION_TYPE_NONE] =3D { > > + .prepare =3D lksd_none_prepare, > > + .process =3D lksd_none_process, > > + }, > > + [RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO] =3D { > > + .prepare =3D NULL, > > + .process =3D inline_crypto_process, > > + }, > > + [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL] =3D { > > + .prepare =3D NULL, > > + .process =3D inline_proto_process, > > + }, > > + [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL] =3D { > > + .prepare =3D lksd_proto_prepare, > > + .process =3D lksd_proto_process, > > + }, >=20 > [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL][EVENT] =3D { > .prepare =3D NULL, > .process =3D NULL, > .process_evt =3D lksd_event_process, > }, > [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL][EVENT] =3D { > .prepare =3D NULL, > .process =3D NULL, > .process_evt =3D inline_event_process, > }, >=20 > Probably add one more dimension in array to choose event/poll? That's a static function/array, surely we can have here as many dimensions = as we need to. As I said below, will probably need to select a function based on direction= , mode, etc. anyway. NP to have extra logic to select event/mbuf based functions. >=20 >=20 > > + }; > > + > > + if (ss->type >=3D RTE_DIM(tfunc)) > > + return NULL; > > + > > + return tfunc + ss->type; > > +} > > diff --git a/lib/librte_ipsec/sa.h b/lib/librte_ipsec/sa.h > > index ef030334c..13a5a68f3 100644 > > --- a/lib/librte_ipsec/sa.h > > +++ b/lib/librte_ipsec/sa.h > > @@ -72,4 +72,7 @@ struct rte_ipsec_sa { > > > > } __rte_cache_aligned; > > > > +const struct rte_ipsec_sa_func * > > +ipsec_sa_func_select(const struct rte_ipsec_session *ss); > > + > > #endif /* _SA_H_ */ > > diff --git a/lib/librte_ipsec/ses.c b/lib/librte_ipsec/ses.c > > new file mode 100644 > > index 000000000..afefda937 > > --- /dev/null > > +++ b/lib/librte_ipsec/ses.c > > @@ -0,0 +1,45 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2018 Intel Corporation > > + */ > > + > > +#include > > +#include "sa.h" > > + > > +static int > > +session_check(struct rte_ipsec_session *ss) > > +{ > > + if (ss =3D=3D NULL) > > + return -EINVAL; > > + > > + if (ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_NONE) { > > + if (ss->crypto.ses =3D=3D NULL) > > + return -EINVAL; > > + } else if (ss->security.ses =3D=3D NULL || ss->security.ctx =3D= =3D NULL) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +int __rte_experimental > > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > +{ >=20 > Probably add one more argument to choose event vs poll so that > above function pointers can be selected. >=20 > or have different API like rte_ipsec_use_mode(EVENT) or API > other slow path scheme to select the mode. Yes, we would need something here.=20 I think we can have some field inside ipsec_session that defines which input types (mbuf/event) session expects. I suppose we would need such field anyway - as you said above, ipsec_session most likely will contain a union for event/non-event related = data.=20 Konstantin