Re: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Tue, 9 Oct 2018 19:23:36 +0100
> From: Konstantin Ananyev <konstantin.ananyev@intel.com>
> To: dev@dpdk.org
> CC: Konstantin Ananyev <konstantin.ananyev@intel.com>, Mohammad Abdul Awal
>  <mohammad.abdul.awal@intel.com>
> Subject: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API
> X-Mailer: git-send-email 1.7.0.7
> 

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.awal@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@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.

> +};
> +
> +/**
> + * 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.

> +
> +/**
> + * 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);
}

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.

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? 


> +       };
> +
> +       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. 

> +       int32_t rc;
> +       const struct rte_ipsec_sa_func *fp;
> +
> +       rc = session_check(ss);
> +       if (rc != 0)
> +               return rc;
> +
> +       fp = ipsec_sa_func_select(ss);
> +       if (fp == NULL)
> +               return -ENOTSUP;
> +
> +       ss->func = fp[0];
> +
> +       if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE)
> +               ss->crypto.ses->userdata = (uintptr_t)ss;
> +       else
> +               ss->security.ses->userdata = (uintptr_t)ss;
> +
> +       return 0;
> +}
> --
> 2.13.6
>