Hi Jerin,
>
> Hi Konstantin,
>
> Overall it looks good, but some comments on event mode integration in
> performance effective way.
>
> >
> > 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 <mohammad.abdul.a...@intel.com>
> > Signed-off-by: Konstantin Ananyev <konstantin.anan...@intel.com>
> > ---
> > 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 := 1
> >
> > # all source are stored in SRCS-y
> > SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += sa.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += ses.c
> >
> > # install header files
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include += rte_ipsec.h
> > SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include += rte_ipsec_sa.h
> >
> > include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/lib/librte_ipsec/meson.build b/lib/librte_ipsec/meson.build
> > index 52c78eaeb..6e8c6fabe 100644
> > --- a/lib/librte_ipsec/meson.build
> > +++ b/lib/librte_ipsec/meson.build
> > @@ -3,8 +3,8 @@
> >
> > allow_experimental_apis = true
> >
> > -sources=files('sa.c')
> > +sources=files('sa.c', 'ses.c')
> >
> > -install_headers = files('rte_ipsec_sa.h')
> > +install_headers = files('rte_ipsec.h', 'rte_ipsec_sa.h')
> >
> > deps += ['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 <rte_ipsec_sa.h>
> > +#include <rte_mbuf.h>
> > +
> > +#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 session
> > + * (see *rte_ipsec_crypto_prepare* below for more details).
> > + * - process - finalize processing of packets after crypto-dev finished
> > + * with them or process packets that are subjects to inline IPsec offload
> > + * (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);
>
> 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, etc.
Let say inline_proto_process() could be split into:
inline_proto_outb_process() and inline_proto_inb_process() and
rte_ipsec_sa_func.process will point to appropriate one.
I probably will change things that way for next version.
>
> > +};
> > +
> > +/**
> > + * rte_ipsec_session is an aggregate structure that defines particular
> > + * IPsec Security Association IPsec (SA) on given security/crypto device:
> > + * - 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 packets.
> > + */
> > +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;
> > +};
>
> IMO, it can be cache aligned as it is used in fast path.
Good point, will add.
>
> > +
> > +/**
> > + * Checks that inside given rte_ipsec_session crypto/security fields
> > + * are filled correctly and setups function pointers based on these values.
> > + * @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* structures
> > + * which contain the input packets.
> > + * @param cop
> > + * The address of an array of *num* pointers to the output
> > *rte_crypto_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 num)
> > +{
> > + 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 removed,
> > + * *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 added,
> > + * 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* structures
> > + * 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);
> > +}
>
> Since we have separate functions and different application path for
> different mode
> and the arguments also differ. I think, It is better to integrate
> event mode like following
>
> static inline uint16_t __rte_experimental
> rte_ipsec_event_process(const struct rte_ipsec_session *ss, struct rte_event
> *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_mbuf
*mb[],uint16_t num);
uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte_event
*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.
>
> 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 events,
> having one function pointer for event would be enough.
>
> 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.
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.
>
> See below some implementation thoughts on this.
>
> > +
> > +#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[] = {
> > + [RTE_SECURITY_ACTION_TYPE_NONE] = {
> > + .prepare = lksd_none_prepare,
> > + .process = lksd_none_process,
> > + },
> > + [RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO] = {
> > + .prepare = NULL,
> > + .process = inline_crypto_process,
> > + },
> > + [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL] = {
> > + .prepare = NULL,
> > + .process = inline_proto_process,
> > + },
> > + [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL] = {
> > + .prepare = lksd_proto_prepare,
> > + .process = lksd_proto_process,
> > + },
>
> [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL][EVENT] = {
> .prepare = NULL,
> .process = NULL,
> .process_evt = lksd_event_process,
> },
> [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL][EVENT] = {
> .prepare = NULL,
> .process = NULL,
> .process_evt = inline_event_process,
> },
>
> 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.
>
>
> > + };
> > +
> > + if (ss->type >= 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 <rte_ipsec.h>
> > +#include "sa.h"
> > +
> > +static int
> > +session_check(struct rte_ipsec_session *ss)
> > +{
> > + if (ss == NULL)
> > + return -EINVAL;
> > +
> > + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) {
> > + if (ss->crypto.ses == NULL)
> > + return -EINVAL;
> > + } else if (ss->security.ses == NULL || ss->security.ctx == NULL)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +int __rte_experimental
> > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss)
> > +{
>
> Probably add one more argument to choose event vs poll so that
> above function pointers can be selected.
>
> 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.
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.
Konstantin
