[RFC,v2,5/9] ipsec: add SA data-path API

  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
  

-----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
>
  

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.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.

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
  

-----Original Message-----
> Date: Sun, 21 Oct 2018 22:01:48 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Awal, Mohammad Abdul"
>  <mohammad.abdul.awal@intel.com>, "Joseph, Anoob"
>  <Anoob.Joseph@cavium.com>, "Athreya, Narayana Prasad"
>  <NarayanaPrasad.Athreya@cavium.com>
> Subject: RE: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API
> 
> 
> Hi Jerin,

Hi Konstantin,

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

OK

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

OK

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

How about best of both worlds, i.e save space and enable compile check
by anonymous union of both functions

RTE_STD_C11
union {
	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);
};

> 
> >
> > 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.

OK

> 
> >
> > 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.

OK

> 
> >
> >
> > > +       };
> > > +
> > > +       if (ss->type >= RTE_DIM(tfunc))
> > > +               return NULL;
> > > +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.

OK

> 
> Konstantin
>
  

Hi Jerin,

> > > > +
> > > > +/**
> > > > + * 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);
> > > > +}
> > > > +
> > > 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.
> 
> How about best of both worlds, i.e save space and enable compile check
> by anonymous union of both functions
> 
> RTE_STD_C11
> union {
> 	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);
> };
> 

Yes, it is definitely possible, but then we still need 2 API functions,
Depending on input type, i.e:

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);
}

static inline uint16_t __rte_experimental
rte_ipsec_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[], uint16_t num)
 {
        return ss->func.pkt_process(ss, mb, num);
}

While if we'll have void *[], we can have just one function for both cases:

static inline uint16_t __rte_experimental
rte_ipsec_process(const struct rte_ipsec_session *ss, void *in[], uint16_t num)
 {
        return ss->func.process(ss, in, num);
}

Konstantin
  

-----Original Message-----
> Date: Sun, 28 Oct 2018 20:37:23 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Awal, Mohammad Abdul"
>  <mohammad.abdul.awal@intel.com>, "Joseph, Anoob"
>  <Anoob.Joseph@cavium.com>, "Athreya, Narayana Prasad"
>  <NarayanaPrasad.Athreya@cavium.com>
> Subject: RE: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API
> 

> 
> Hi Jerin,

Hi Konstantin,

> 
> > > > > +
> > > > > +/**
> > > > > + * 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);
> > > > > +}
> > > > > +
> > > > 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.
> >
> > How about best of both worlds, i.e save space and enable compile check
> > by anonymous union of both functions
> >
> > RTE_STD_C11
> > union {
> >       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);
> > };
> >
> 
> Yes, it is definitely possible, but then we still need 2 API functions,
> Depending on input type, i.e:
> 
> 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);
> }
> 
> static inline uint16_t __rte_experimental
> rte_ipsec_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[], uint16_t num)
>  {
>         return ss->func.pkt_process(ss, mb, num);
> }
> 
> While if we'll have void *[], we can have just one function for both cases:
> 
> static inline uint16_t __rte_experimental
> rte_ipsec_process(const struct rte_ipsec_session *ss, void *in[], uint16_t num)
>  {
>         return ss->func.process(ss, in, num);
> }

Since it will be called from different application code path. I would
prefer to have separate functions to allow strict compiler check.



> 
> Konstantin
  

Hi Jerin,
 

> > > > > > +
> > > > > > +/**
> > > > > > + * 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);
> > > > > > +}
> > > > > > +
> > > > > 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.
> > >
> > > How about best of both worlds, i.e save space and enable compile check
> > > by anonymous union of both functions
> > >
> > > RTE_STD_C11
> > > union {
> > >       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);
> > > };
> > >
> >
> > Yes, it is definitely possible, but then we still need 2 API functions,
> > Depending on input type, i.e:
> >
> > 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);
> > }
> >
> > static inline uint16_t __rte_experimental
> > rte_ipsec_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[], uint16_t num)
> >  {
> >         return ss->func.pkt_process(ss, mb, num);
> > }
> >
> > While if we'll have void *[], we can have just one function for both cases:
> >
> > static inline uint16_t __rte_experimental
> > rte_ipsec_process(const struct rte_ipsec_session *ss, void *in[], uint16_t num)
> >  {
> >         return ss->func.process(ss, in, num);
> > }
> 
> Since it will be called from different application code path. I would
> prefer to have separate functions to allow strict compiler check.
> 

Ok, let's keep them separate, NP with that.
I'll rename ipsec_(prepare|process) to ipsec_pkt_(prepare_process),
so you guys can add '_event_' functions later.
Konstantin
  

-----Original Message-----
> Date: Tue, 30 Oct 2018 13:53:30 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Awal, Mohammad Abdul"
>  <mohammad.abdul.awal@intel.com>, "Joseph, Anoob"
>  <Anoob.Joseph@cavium.com>, "Athreya, Narayana Prasad"
>  <NarayanaPrasad.Athreya@cavium.com>
> Subject: RE: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API
> 
> 
> Hi Jerin,
> 
> 
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * 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);
> > > > > > > +}
> > > > > > > +
> > > > > > 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.
> > > >
> > > > How about best of both worlds, i.e save space and enable compile check
> > > > by anonymous union of both functions
> > > >
> > > > RTE_STD_C11
> > > > union {
> > > >       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);
> > > > };
> > > >
> > >
> > > Yes, it is definitely possible, but then we still need 2 API functions,
> > > Depending on input type, i.e:
> > >
> > > 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);
> > > }
> > >
> > > static inline uint16_t __rte_experimental
> > > rte_ipsec_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[], uint16_t num)
> > >  {
> > >         return ss->func.pkt_process(ss, mb, num);
> > > }
> > >
> > > While if we'll have void *[], we can have just one function for both cases:
> > >
> > > static inline uint16_t __rte_experimental
> > > rte_ipsec_process(const struct rte_ipsec_session *ss, void *in[], uint16_t num)
> > >  {
> > >         return ss->func.process(ss, in, num);
> > > }
> >
> > Since it will be called from different application code path. I would
> > prefer to have separate functions to allow strict compiler check.
> >
> 
> Ok, let's keep them separate, NP with that.
> I'll rename ipsec_(prepare|process) to ipsec_pkt_(prepare_process),
> so you guys can add '_event_' functions later.

OK

> Konstantin
>