Re: [dpdk-dev] [PATCH v2 0/7] crypto/security session framework rework

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Akhil,

I tried to fix the problems of seg faults.
The seg-faults are gone now but all asym tests are failing too.
The reason is the rte_cryptodev_queue_pair_setup() checks the session mempool same for sym and asym.
Since we don't have a rte_cryptodev_asym_session_pool_create() the session mempool created by 
test_cryptodev_asym.c  with rte_mempool_create() will fail the mempool check when setting up the queue pair.

If you think my fix may be useful (although not resolving asym issue) I can send it.

Regards,
Fan



> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, October 14, 2021 7:57 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> matan@nvidia.com; g.singh@nxp.com; jianjay.zhou@huawei.com;
> asomalap@amd.com; ruifeng.wang@arm.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> ajit.khaparde@broadcom.com; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> Power, Ciara <ciara.power@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; jiawenwu@trustnetic.com;
> jianwang@trustnetic.com
> Subject: RE: [PATCH v2 0/7] crypto/security session framework rework
> 
> Hi Fan,
> Check for below QAT fix also
> > >
> > > Unfortunately the patches still cause seg-fault at QAT and SW PMDs.
> > >
> > > - for qat it fails at rte_security_ops->session_size_get not implemented.
> And for this one
> diff --git a/drivers/crypto/qat/qat_sym_pmd.c
> b/drivers/crypto/qat/qat_sym_pmd.c
> index efda921c05..96cd9d2eee 100644
> --- a/drivers/crypto/qat/qat_sym_pmd.c
> +++ b/drivers/crypto/qat/qat_sym_pmd.c
> @@ -306,6 +306,7 @@ static struct rte_security_ops security_qat_ops = {
> 
>                 .session_create = qat_security_session_create,
>                 .session_update = NULL,
> +               .session_get_size = qat_security_session_get_size,
>                 .session_stats_get = NULL,
>                 .session_destroy = qat_security_session_destroy,
>                 .set_pkt_metadata = NULL,
> diff --git a/drivers/crypto/qat/qat_sym_session.c
> b/drivers/crypto/qat/qat_sym_session.c
> index ef92f22c1a..41b5542343 100644
> --- a/drivers/crypto/qat/qat_sym_session.c
> +++ b/drivers/crypto/qat/qat_sym_session.c
> @@ -2297,4 +2297,10 @@ qat_security_session_destroy(void *dev
> __rte_unused, void *sess_priv)
>         }
>         return 0;
>  }
> +
> +static unsigned int
> +qat_security_session_get_size(void *device __rte_unused)
> +{
> +       return sizeof(struct qat_sym_session);
> +}
>  #endif
> 
> > > - for sw pmds the queue pair's session private mempools are not set.
> > >
> > Can you check if below change works for Kasumi. I will replicate for others.
> >
> > diff --git a/drivers/crypto/kasumi/kasumi_pmd_private.h
> > b/drivers/crypto/kasumi/kasumi_pmd_private.h
> > index abedcd616d..fe0e78e516 100644
> > --- a/drivers/crypto/kasumi/kasumi_pmd_private.h
> > +++ b/drivers/crypto/kasumi/kasumi_pmd_private.h
> > @@ -38,8 +38,6 @@ struct kasumi_qp {
> >         /**< Ring for placing processed ops */
> >         struct rte_mempool *sess_mp;
> >         /**< Session Mempool */
> > -       struct rte_mempool *sess_mp_priv;
> > -       /**< Session Private Data Mempool */
> >         struct rte_cryptodev_stats qp_stats;
> >         /**< Queue pair statistics */
> >         uint8_t temp_digest[KASUMI_DIGEST_LENGTH];
> > diff --git a/drivers/crypto/kasumi/rte_kasumi_pmd.c
> > b/drivers/crypto/kasumi/rte_kasumi_pmd.c
> > index d6f927417a..1fc59c8b8a 100644
> > --- a/drivers/crypto/kasumi/rte_kasumi_pmd.c
> > +++ b/drivers/crypto/kasumi/rte_kasumi_pmd.c
> > @@ -139,27 +139,24 @@ kasumi_get_session(struct kasumi_qp *qp, struct
> > rte_crypto_op *op)
> >                                         op->sym->session,
> >                                         cryptodev_driver_id);
> >         } else {
> > -               void *_sess = NULL;
> > -               void *_sess_private_data = NULL;
> > +               struct rte_cryptodev_sym_session *_sess = NULL;
> >
> > -               if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
> > +               /* Create temporary session */
> > +               _sess = rte_cryptodev_sym_session_create(qp->sess_mp);
> > +               if (_sess == NULL)
> >                         return NULL;
> >
> > -               if (rte_mempool_get(qp->sess_mp_priv,
> > -                               (void **)&_sess_private_data))
> > -                       return NULL;
> > -
> > -               sess = (struct kasumi_session *)_sess_private_data;
> > -
> > +               _sess->sess_data[cryptodev_driver_id].data =
> > +                               (void *)((uint8_t *)_sess +
> > +                               rte_cryptodev_sym_get_header_session_size() +
> > +                               (cryptodev_driver_id * _sess->priv_sz));
> > +               sess = _sess->sess_data[cryptodev_driver_id].data;
> >                 if (unlikely(kasumi_set_session_parameters(qp->mgr, sess,
> >                                 op->sym->xform) != 0)) {
> >                         rte_mempool_put(qp->sess_mp, _sess);
> > -                       rte_mempool_put(qp->sess_mp_priv, _sess_private_data);
> >                         sess = NULL;
> >                 }
> >                 op->sym->session = (struct rte_cryptodev_sym_session *)_sess;
> > -               set_sym_session_private_data(op->sym->session,
> > -                               cryptodev_driver_id, _sess_private_data);
> >         }
> >
> >         if (unlikely(sess == NULL))
> > @@ -327,7 +324,6 @@ process_ops(struct rte_crypto_op **ops, struct
> > kasumi_session *session,
> >                         memset(ops[i]->sym->session, 0,
> >                         rte_cryptodev_sym_get_existing_header_session_size(
> >                                         ops[i]->sym->session));
> > -                       rte_mempool_put(qp->sess_mp_priv, session);
> >                         rte_mempool_put(qp->sess_mp, ops[i]->sym->session);
> >                         ops[i]->sym->session = NULL;
> >                 }