Realized that I missed to respond to one of your comments regarding the use of
union.
Please find the comment below.
> > > > Introduce stateless packet preparation API for IPsec processing.
> > > > The new API would allow preparation of IPsec packets without
> > > > altering the internal state of an IPsec session.
> > > >
> > > > For outbound IPsec processing, the change enables user to provide
> > > > sequence number to be used for the IPsec operation.
> > >
> > > Few questions/nits below.
> > > As a generic one - we need to add a use-case/test-case for it.
> > > Without it I think the patch is incomplete.
> >
> > Ack. Will add test-case with v3.
> >
> > >
> > > >
> > > > Signed-off-by: Aakash Sasidharan <asasidha...@marvell.com>
> > > > ---
> > > > lib/ipsec/esp_outb.c | 85
> > > > +++++++++++++++++++++++++++++++------------
> > > > lib/ipsec/rte_ipsec.h | 68 ++++++++++++++++++++++++++++++++++
> > > > lib/ipsec/sa.c | 4 +-
> > > > lib/ipsec/sa.h | 8 ++++
> > > > 4 files changed, 140 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/lib/ipsec/esp_outb.c b/lib/ipsec/esp_outb.c index
> > > > ec87b1dce2..de28cb166d 100644
> > > > --- a/lib/ipsec/esp_outb.c
> > > > +++ b/lib/ipsec/esp_outb.c
> > > > @@ -288,13 +288,12 @@ outb_pkt_xprepare(const struct rte_ipsec_sa
> > > > *sa, rte_be64_t sqc,
> > > > /*
> > > > * setup/update packets and crypto ops for ESP outbound tunnel case.
> > > > */
> > > > -uint16_t
> > > > -esp_outb_tun_prepare(const struct rte_ipsec_session *ss, struct
> > > > rte_mbuf
> > > *mb[],
> > > > - struct rte_crypto_op *cop[], uint16_t num)
> > > > +static inline uint16_t
> > > > +esp_outb_tun_prepare_helper(const struct rte_ipsec_session *ss,
> > > > +struct
> > > rte_mbuf *mb[],
> > > > + struct rte_crypto_op *cop[], uint16_t num, uint64_t sqn)
> > > > {
> > > > int32_t rc;
> > > > - uint32_t i, k, n;
> > > > - uint64_t sqn;
> > > > + uint32_t i, k, n = num;
> > >
> > > You can just do s/num/n/ in function parameter list, then you don't
> > > need to keep 'num' at all.
> >
> > This function will be called for normal IPsec processing. The function
> > esn_outb_update_sqn() updates the local variable n passed as parameter
> > in OVERFLOW case. If we remove the local variable n, this error path
> > would be lost and it is not our intention I believe.
>
> Apologies. Replied too soon. I understand your suggestion and will update
> that in v3.
>
> >
> > >
> > > > rte_be64_t sqc;
> > > > struct rte_ipsec_sa *sa;
> > > > struct rte_cryptodev_sym_session *cs; @@ -305,11 +304,6 @@
> > > > esp_outb_tun_prepare(const struct rte_ipsec_session *ss, struct
> > > > rte_mbuf
> > > *mb[],
> > > > sa = ss->sa;
> > > > cs = ss->crypto.ses;
> > > >
> > > > - n = num;
> > > > - sqn = esn_outb_update_sqn(sa, &n);
> > > > - if (n != num)
> > > > - rte_errno = EOVERFLOW;
> > > > -
> > > > k = 0;
> > > > for (i = 0; i != n; i++) {
> > > >
> > > > @@ -339,6 +333,30 @@ esp_outb_tun_prepare(const struct
> > > rte_ipsec_session *ss, struct rte_mbuf *mb[],
> > > > return k;
> > > > }
> > > >
> > > > +uint16_t
> > > > +esp_outb_tun_prepare(const struct rte_ipsec_session *ss, struct
> > > > +rte_mbuf
> > > *mb[],
> > > > + struct rte_crypto_op *cop[], uint16_t num) {
> > > > + uint64_t sqn;
> > > > + uint32_t n;
> > > > +
> > > > + n = num;
> > > > + sqn = esn_outb_update_sqn(ss->sa, &n);
> > > > + if (n != num)
> > > > + rte_errno = EOVERFLOW;
> > > > +
> > > > + return esp_outb_tun_prepare_helper(ss, mb, cop, n, sqn); }
> > > > +
> > > > +uint16_t
> > > > +esp_outb_tun_prepare_stateless(const struct rte_ipsec_session
> > > > +*ss, struct
> > > rte_mbuf *mb[],
> > > > + struct rte_crypto_op *cop[], uint16_t num, struct
> > > > +rte_ipsec_state
> > > > +*state) {
> > > > + uint64_t sqn = state->sqn;
> > > > +
> > > > + return esp_outb_tun_prepare_helper(ss, mb, cop, num, sqn); }
> > > > +
> > > > /*
> > > > * setup/update packet data and metadata for ESP outbound
> > > > transport
> > case.
> > > > */
> > > > @@ -529,16 +547,15 @@ outb_cpu_crypto_prepare(const struct
> > > rte_ipsec_sa *sa, uint32_t *pofs,
> > > > return clen;
> > > > }
> > > >
> > > > -static uint16_t
> > > > -cpu_outb_pkt_prepare(const struct rte_ipsec_session *ss,
> > > > - struct rte_mbuf *mb[], uint16_t num,
> > > > - esp_outb_prepare_t prepare, uint32_t cofs_mask)
> > > > +static inline uint16_t
> > > > +cpu_outb_pkt_prepare_helper(const struct rte_ipsec_session *ss,
> > > > + struct rte_mbuf *mb[], uint16_t num, esp_outb_prepare_t
> > > prepare,
> > > > + uint32_t cofs_mask, uint64_t sqn)
> > > > {
> > > > int32_t rc;
> > > > - uint64_t sqn;
> > > > rte_be64_t sqc;
> > > > struct rte_ipsec_sa *sa;
> > > > - uint32_t i, k, n;
> > > > + uint32_t i, k, n = num;
> > >
> > > Same here, you can just use 'n' instead of 'num'.
> >
> > Same comment as above.
> >
> > >
> > > > uint32_t l2, l3;
> > > > union sym_op_data icv;
> > > > struct rte_crypto_va_iova_ptr iv[num]; @@ -551,11 +568,6 @@
> > > > cpu_outb_pkt_prepare(const struct rte_ipsec_session *ss,
> > > >
> > > > sa = ss->sa;
> > > >
> > > > - n = num;
> > > > - sqn = esn_outb_update_sqn(sa, &n);
> > > > - if (n != num)
> > > > - rte_errno = EOVERFLOW;
> > > > -
> > > > for (i = 0, k = 0; i != n; i++) {
> > > >
> > > > l2 = mb[i]->l2_len;
> > > > @@ -604,15 +616,40 @@ uint16_t
> > > > cpu_outb_tun_pkt_prepare(const struct rte_ipsec_session *ss,
> > > > struct rte_mbuf *mb[], uint16_t num) {
> > > > - return cpu_outb_pkt_prepare(ss, mb, num, outb_tun_pkt_prepare,
> > > 0);
> > > > + uint64_t sqn;
> > > > + uint32_t n;
> > > > +
> > > > + n = num;
> > > > + sqn = esn_outb_update_sqn(ss->sa, &n);
> > > > + if (n != num)
> > > > + rte_errno = EOVERFLOW;
> > > > +
> > > > + return cpu_outb_pkt_prepare_helper(ss, mb, n,
> > > outb_tun_pkt_prepare,
> > > > +0, sqn); }
> > > > +
> > > > +uint16_t
> > > > +cpu_outb_tun_pkt_prepare_stateless(const struct rte_ipsec_session
> *ss,
> > > > + struct rte_mbuf *mb[], uint16_t num, struct
> > > > rte_ipsec_state
> > > *state)
> > > > +{
> > > > + uint64_t sqn = state->sqn;
> > > > +
> > > > + return cpu_outb_pkt_prepare_helper(ss, mb, num,
> > > > +outb_tun_pkt_prepare, 0, sqn);
> > > > }
> > > >
> > > > uint16_t
> > > > cpu_outb_trs_pkt_prepare(const struct rte_ipsec_session *ss,
> > > > struct rte_mbuf *mb[], uint16_t num) {
> > > > - return cpu_outb_pkt_prepare(ss, mb, num, outb_trs_pkt_prepare,
> > > > - UINT32_MAX);
> > > > + uint64_t sqn;
> > > > + uint32_t n;
> > > > +
> > > > + n = num;
> > > > + sqn = esn_outb_update_sqn(ss->sa, &n);
> > > > + if (n != num)
> > > > + rte_errno = EOVERFLOW;
> > > > +
> > > > + return cpu_outb_pkt_prepare_helper(ss, mb, n,
> > > outb_trs_pkt_prepare,
> > > > + UINT32_MAX, sqn);
> > > > }
> > > >
> > > > /*
> > > > diff --git a/lib/ipsec/rte_ipsec.h b/lib/ipsec/rte_ipsec.h index
> > > > f15f6f2966..b462068203 100644
> > > > --- a/lib/ipsec/rte_ipsec.h
> > > > +++ b/lib/ipsec/rte_ipsec.h
> > > > @@ -23,11 +23,26 @@ extern "C" {
> > > >
> > > > struct rte_ipsec_session;
> > > >
> > > > +/**
> > > > + * IPsec state for stateless processing of a batch of IPsec packets.
> > > > + */
> > > > +struct rte_ipsec_state {
> > > > + union {
> > >
> > > Curious what is the purpose of 'union' here?
> > > What other mutually exclusive fields you plan to add here?
Our intention was to use a union to facilitate future support for the inbound
path.
Would it be more appropriate to define the rte_ipsec_state itself as a union?
For example:
union rte_ipsec_state {
struct {
uint64_t sqn;
} outbound;
};
What do you think would be the best approach to future-proof the API?
> > >
> > > > + /**
> > > > + * 64 bit sequence number to be used for the first
> > > > packet of
> > > the
> > > > + * batch of packets.
> > > > + */
> > > > + uint64_t sqn;
> > > > + };
> > > > +};
> > > > +
> > > > /**
> > > > * 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_pkt_crypto_prepare* below for more details).
> > > > + * - prepare_stateless - similar to prepare, but further processing is
> > > > done
> > > > + * based on IPsec state provided by the 'state' parameter.
> > > > * - process - finalize processing of packets after crypto-dev finished
> > > > * with them or process packets that are subjects to inline IPsec
> > > > offload
> > > > * (see rte_ipsec_pkt_process for more details).
> > > > @@ -42,6 +57,17 @@ struct rte_ipsec_sa_pkt_func {
> > > > struct rte_mbuf *mb[],
> > > > uint16_t num);
> > > > } prepare;
> > > > + union {
> > > > + uint16_t (*async)(const struct rte_ipsec_session *ss,
> > > > + struct rte_mbuf *mb[],
> > > > + struct rte_crypto_op *cop[],
> > > > + uint16_t num,
> > > > + struct rte_ipsec_state *state);
> > > > + uint16_t (*sync)(const struct rte_ipsec_session *ss,
> > > > + struct rte_mbuf *mb[],
> > > > + uint16_t num,
> > > > + struct rte_ipsec_state *state);
> > > > + } prepare_stateless;
> > > > uint16_t (*process)(const struct rte_ipsec_session *ss,
> > > > struct rte_mbuf *mb[],
> > > > uint16_t num);
> > > > @@ -128,6 +154,48 @@ rte_ipsec_pkt_cpu_prepare(const struct
> > > rte_ipsec_session *ss,
> > > > return ss->pkt_func.prepare.sync(ss, mb, num); }
> > > >
> > > > +/**
> > > > + * Same as rte_ipsec_pkt_crypto_prepare, but processing is done
> > > > +based on
> > > > + * IPsec state provided by the 'state' parameter. Internal IPsec
> > > > +state won't
> > > > + * be updated when this API is called.
> > > > + *
> > > > + * 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.
> > > > + * @param state
> > > > + * The IPsec state to be used for processing current batch of
> > > > packets.
> > > > + * @return
> > > > + * Number of successfully processed packets, with error code set in
> > > rte_errno.
> > > > + */
> > >
> > > We probably need to mark new API as 'rte_experimental'.
> >
> > Ack. Will update in v3.
> >
> > >
> > > > +static inline uint16_t
> > > > +rte_ipsec_pkt_crypto_prepare_stateless(const struct
> > > > +rte_ipsec_session
> > *ss,
> > > > + struct rte_mbuf *mb[], struct rte_crypto_op *cop[], uint16_t
> > > > num,
> > > > + struct rte_ipsec_state *state)
> > > > +{
> > > > + return ss->pkt_func.prepare_stateless.async(ss, mb, cop, num,
> > > > +state); }
> > > > +
> > > > +static inline uint16_t
> > > > +rte_ipsec_pkt_cpu_prepare_stateless(const struct rte_ipsec_session *ss,
> > > > + struct rte_mbuf *mb[], uint16_t num, struct rte_ipsec_state
> > > > +*state) {
> > > > + return ss->pkt_func.prepare_stateless.sync(ss, mb, num, state);
> > > > +}
> > > > +
> > > > /**
> > > > * Finalise processing of packets after crypto-dev finished with them
> > > > or
> > > > * process packets that are subjects to inline IPsec offload.
> > > > diff --git a/lib/ipsec/sa.c b/lib/ipsec/sa.c index
> > > > 2297bd6d72..741e079831 100644
> > > > --- a/lib/ipsec/sa.c
> > > > +++ b/lib/ipsec/sa.c
> > > > @@ -710,6 +710,7 @@ lksd_none_pkt_func_select(const struct
> > > rte_ipsec_sa *sa,
> > > > case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV4):
> > > > case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV6):
> > > > pf->prepare.async = esp_outb_tun_prepare;
> > > > + pf->prepare_stateless.async =
> > > esp_outb_tun_prepare_stateless;
> > > > pf->process = (sa->sqh_len != 0) ?
> > > > esp_outb_sqh_process : pkt_flag_process;
> > > > break;
> > > > @@ -748,6 +749,7 @@ cpu_crypto_pkt_func_select(const struct
> > > rte_ipsec_sa *sa,
> > > > case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV4):
> > > > case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV6):
> > > > pf->prepare.sync = cpu_outb_tun_pkt_prepare;
> > > > + pf->prepare_stateless.sync =
> > > cpu_outb_tun_pkt_prepare_stateless;
> > > > pf->process = (sa->sqh_len != 0) ?
> > > > esp_outb_sqh_process : pkt_flag_process;
> > > > break;
> > > > @@ -810,7 +812,7 @@ ipsec_sa_pkt_func_select(const struct
> > > rte_ipsec_session *ss,
> > > > int32_t rc;
> > > >
> > > > rc = 0;
> > > > - pf[0] = (struct rte_ipsec_sa_pkt_func) { {NULL}, NULL };
> > > > + pf[0] = (struct rte_ipsec_sa_pkt_func) { {NULL}, {NULL}, NULL };
> > > >
> > > > switch (ss->type) {
> > > > case RTE_SECURITY_ACTION_TYPE_NONE:
> > > > diff --git a/lib/ipsec/sa.h b/lib/ipsec/sa.h index
> > > > 719b5c735c..9b53586b2d 100644
> > > > --- a/lib/ipsec/sa.h
> > > > +++ b/lib/ipsec/sa.h
> > > > @@ -179,6 +179,10 @@ uint16_t
> > > > esp_outb_tun_prepare(const struct rte_ipsec_session *ss, struct
> > > > rte_mbuf
> > > *mb[],
> > > > struct rte_crypto_op *cop[], uint16_t num);
> > > >
> > > > +uint16_t
> > > > +esp_outb_tun_prepare_stateless(const struct rte_ipsec_session
> > > > +*ss, struct
> > > rte_mbuf *mb[],
> > > > + struct rte_crypto_op *cop[], uint16_t num, struct
> > > > +rte_ipsec_state *state);
> > > > +
> > > > uint16_t
> > > > esp_outb_trs_prepare(const struct rte_ipsec_session *ss, struct
> > > > rte_mbuf
> > > *mb[],
> > > > struct rte_crypto_op *cop[], uint16_t num); @@ -207,6 +211,10 @@
> > > > uint16_t cpu_outb_tun_pkt_prepare(const struct rte_ipsec_session *ss,
> > > > struct rte_mbuf *mb[], uint16_t num); uint16_t
> > > > +cpu_outb_tun_pkt_prepare_stateless(const struct rte_ipsec_session
> *ss,
> > > > + struct rte_mbuf *mb[], uint16_t num, struct
> > > > rte_ipsec_state
> > > > +*state);
> > > > +
> > > > +uint16_t
> > > > cpu_outb_trs_pkt_prepare(const struct rte_ipsec_session *ss,
> > > > struct rte_mbuf *mb[], uint16_t num);
> > > >
> > > > --
> > > > 2.25.1
