Re: [dpdk-dev] [PATCH v4 1/2] examples/ipsec-secgw: fix 1st packet dropped for inline crypto

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Bernard,


> -----Original Message-----
> From: Akhil Goyal
> Sent: Thursday, April 18, 2019 7:21 PM
> To: Bernard Iremonger <bernard.iremonger@intel.com>; dev@dpdk.org;
> konstantin.ananyev@intel.com
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v4 1/2] examples/ipsec-secgw: fix 1st packet dropped for
> inline crypto
> 
> Hi Bernard,
> 
> > -       RTE_LOG_DP(DEBUG, IPSEC, "Create session for SA spi %u on cryptodev "
> > -                       "%u qp %u\n", sa->spi,
> > -                       ipsec_ctx->tbl[cdev_id_qp].id,
> > -                       ipsec_ctx->tbl[cdev_id_qp].qp);
> > +       if ((sa == NULL) || (pool == NULL))
> > +               return -EINVAL;
> >
> > -       if (sa->type != RTE_SECURITY_ACTION_TYPE_NONE) {
> > -               struct rte_security_session_conf sess_conf = {
> > +       struct rte_security_session_conf sess_conf = {
> >                         .action_type = sa->type,
> >                         .protocol = RTE_SECURITY_PROTOCOL_IPSEC,
> >                         {.ipsec = {
> > @@ -90,247 +65,340 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct
> > ipsec_sa *sa)
> >                         } },
> >                         .crypto_xform = sa->xforms,
> >                         .userdata = NULL,
> > -
> >                 };
> >
> > -               if (sa->type ==
> RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL)
> > {
> > -                       struct rte_security_ctx *ctx = (struct rte_security_ctx *)
> > -                                                       rte_cryptodev_get_sec_ctx(
> > -                                                       ipsec_ctx->tbl[cdev_id_qp].id);
> > -
> > -                       /* Set IPsec parameters in conf */
> > -                       set_ipsec_conf(sa, &(sess_conf.ipsec));
> > -
> > -                       sa->sec_session = rte_security_session_create(ctx,
> > -                                       &sess_conf, ipsec_ctx->session_pool);
> > -                       if (sa->sec_session == NULL) {
> > -                               RTE_LOG(ERR, IPSEC,
> > -                               "SEC Session init failed: err: %d\n", ret);
> > -                               return -1;
> > -                       }
> > -               } else if (sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO)
> {
> > -                       struct rte_flow_error err;
> > -                       struct rte_security_ctx *ctx = (struct rte_security_ctx *)
> > -                                                       rte_eth_dev_get_sec_ctx(
> > -                                                       sa->portid);
> > -                       const struct rte_security_capability *sec_cap;
> > -                       int ret = 0;
> > -
> > -                       sa->sec_session = rte_security_session_create(ctx,
> > -                                       &sess_conf, ipsec_ctx->session_pool);
> > -                       if (sa->sec_session == NULL) {
> > -                               RTE_LOG(ERR, IPSEC,
> > -                               "SEC Session init failed: err: %d\n", ret);
> > -                               return -1;
> > -                       }
> > +       if (sa->type == RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL) {
> > +               ctx = (struct rte_security_ctx *)
> > +                               rte_eth_dev_get_sec_ctx(sa->portid);
> 
> This is breaking the lookaside mode. Ctx was retrieved using the ipsec_ctx->tbl
> struct rte_security_ctx *ctx = (struct rte_security_ctx *)
> 				rte_cryptodev_get_sec_ctx(
> 				ipsec_ctx->tbl[cdev_id_qp].id);
> 
> I am looking into it, but I don't have time left to get it integrated in RC2. So this
> has to be pushed to RC3

It looks like there are multiple issues in this patch wrt lookaside and none cases. Only the inline cases seem to be working.

1. the patch removes the cdev_mapping concept completely. Cdev_id_qp is not getting used.
    The port_id cannot be used in case of crypto, the mapping of cdev/qp/core is done differently for inbound and outbound ports which is missed in this patch.

2. crypto sessions are created using the session mempool and the private data is allocated using the session priv_mempool which is removed in this patch. This will break cases where the priv data is more than the size of sess_mp element size.
    Also the security sessions need to be allocated using the session_priv_mp instead of the session_mp.
Please check this one.
http://patches.dpdk.org/patch/52981/

Ideally this issue should be resolved by adding another parameter in rte_security_session_create which can take another mempool pointer for private data allocation. But this cannot be done in this release as it would need a deprecation notice.

With the above issues I don't see your patch going in 19.05 release cycle.

Regards,
Akhil

> 
> 
> 
> >
> > -                       sec_cap = rte_security_capabilities_get(ctx);
> > +               /* Set IPsec parameters in conf */
> > +               set_ipsec_conf(sa, &(sess_conf.ipsec));
> >
> > -                       /* iterate until ESP tunnel*/
> > -                       while (sec_cap->action !=
> > -                                       RTE_SECURITY_ACTION_TYPE_NONE) {
> > +               sa->sec_session = rte_security_session_create(ctx,
> > +                               &sess_conf, pool);
> > +               if (sa->sec_session == NULL) {
> > +                       RTE_LOG(ERR, IPSEC,
> > +                               "SEC Session init failed: err: %d\n",
> > +                               ret);
> > +                       return -1;
> > +               }