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

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Akhil

<snip>

> > > > > > 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.
> > > >
> > > > Not exactly.
> > > > cdev_id_qp is still setup, and is still used to decide to which
> > > > crypto-dev to enqueuer the crypto-op:
> > > > ipsec_enqueue(...)
> > > > {
> > > >    ...
> > > >    enqueue_cop(&ipsec_ctx->tbl[sa->cdev_id_qp], &priv->cop);
> > >
> > > I don't see anybody filling "sa->cdev_id_qp". Please let me know if
> > > I have
> > missed it somewhere.
> > > It is memset to 0 I guess.
> >
> >
> > Yep, true we lost it somewhere during the rework.
> >
> > >
> > > >
> > > >
> > > > Same in ipsec_process().
> > > >
> > > > For initialization, yes cdev_id_qp is not used anymore.
> > > > As discussed here:
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> > s.dp
> > > > dk.org%2Farchives%2Fdev%2F2019-
> > > >
> >
> March%2F127725.html&amp;data=02%7C01%7Cakhil.goyal%40nxp.com%7C04
> > > >
> >
> 194193cfc04c0b629008d6c7eea247%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> > > >
> >
> 7C0%7C0%7C636916225072561313&amp;sdata=ga9IiqhYRWOz9QkRDIXNiigInk
> > > > soVGgu1E5EetqvE%2FA%3D&amp;reserved=0
> > > >
> > > > I think the problem you are hitting with lookaside-proto is that
> > > > for it we use 2 different values here:
> > > > a) In create_sec_session we use portid (it also should be
> > > > rte_cryptodev_get_sec_ctx() here)
> > > >     if (sa->type == RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL) {
> > > >                 ctx = (struct rte_security_ctx *)
> > > >
> > > > rte_eth_dev_get_sec_ctx(sa->portid);
> > > It should be rte_cryptodev_get_sec_ctx in the first place. And it
> > > needs a
> > cdev_id as input based on the cdev mapping done while initializing
> > > the cryptodev and neither the portid and nor cdev_id_qp.
> > > > b) in enqueue() we use cdev_id_qp
> > > >
> > > > Right now these values could be different.
> > > > As I understand we need to make sure that fro lookaside-proto
> > > > cdev_id_qp
> > ==
> > > > portid provided by user, correct?
> > > No it is not the case. Right now for lookaside there is no use of
> > > portid in case
> > of lookaside case.
> > > As the cdev/qp/core mappings are managed internally and the user
> > > cannot
> > tweak it from cfg file.
> > >
> >
> >
> > Hmm, then at least that line  is wrong here:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.
> > dpdk
> >
> .org%2Fguides%2Fsample_app_ug%2Fipsec_secgw.html&amp;data=02%7C01%
> >
> 7Cakhil.goyal%40nxp.com%7Cb1e931a9967943887a0c08d6c7f49d28%7C686ea
> >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636916250754452306&amp;sda
> >
> ta=tNjHCO0O2rfh8shhQXu93qrM0Hr0OZVXUVhMcsg53dw%3D&amp;reserved=
> > 0
> >
> > sa out 5 cipher_algo aes-128-cbc cipher_key
> > 0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0 \ auth_algo sha1-hmac auth_key
> > 0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0 \ mode ipv4-tunnel src
> > 172.16.1.5 dst 172.16.2.5 \ type lookaside-protocol-offload port_id 4
> >
> > And probably:
> > "Port/device ID of the ethernet/crypto accelerator for which the SA is
> > configured."
> > Need to be rephrased to remove crypto accelerator notice.
> Yes.
> 
> >
> > Another question - why you guys don't consider using portid for lookaside-
> proto?
> > As I can see add_mapping(function that  fills cdev_id_qp) doesn't
> > bother to check which rte_security protocols are supported (only
> > crypto capabilities are checked).
> > So not sure does current code will work ok with a mix of lookaside-
> > none/lookaside-proto devices.
> > Forcing user to specify crypto-dev id for lookaside-proto (via portid
> > or so) will simplify things significantly.
> I will think about it. Actually initially when portid was introduced, the intent was
> same but it did not work well.
> Because there may be a case of a cryptodev with multiple queues, so there will
> be a mapping done internally in the application and matching it with the user
> provided portid will be difficult.
> 
> I believe that this could be done but would need some rework.
> 
> >
> > Konstantin

Could we consider going back to the V2 version of this patch set which fixed the issue with the inline code and left the lookaside code unchanged?

We are not in a position test any changes to the lookaside code.

Regards,

Bernard.