mbox series

[v2,0/7] crypto/security session framework rework

Message ID 20211013192222.1582631-1-gakhil@marvell.com (mailing list archive)
Headers show
Series crypto/security session framework rework | expand

Message

Akhil Goyal Oct. 13, 2021, 7:22 p.m. UTC
As discussed in last release deprecation notice,
crypto and security session framework are reworked
to reduce the need of two mempool objects and
remove the requirement to expose the rte_security_session
and rte_cryptodev_sym_session structures.
Design methodology is explained in the patch description.

Similar work will need to be done for asymmetric sessions
as well. Asymmetric session need another rework and is
postponed to next release. Since it is still in experimental
stage, we can modify the APIs in next release as well.

The patches are compilable with all affected PMDs
and tested with dpdk-test and ipsec-secgw app on CN9k platform.

Changes in v2:
- Added new parameter iova in PMD session configure APIs for
  session priv pointer to be used in QAT/CNXK/etc PMDs.
- Hide rte_cryptodev_sym_session and rte_security_session structs.
- Added compilation workaround for net PMDs(ixgbe/txgbe)
  for inline ipsec.
  Patches with actual fix is beynd the scope of this patchset.
- Added inline APIs to access the opaque data and fast metadata.
- Remove commented code.
TODO
- Release notes/deprecation notice removal.
- Documentation updates.
- Asym APIs - postponed for next release.

Akhil Goyal (7):
  security: rework session framework
  security: hide security session struct
  net/cnxk: rework security session framework
  security: pass session iova in PMD sess create
  cryptodev: rework session framework
  cryptodev: hide sym session structure
  cryptodev: pass session iova in configure session

 app/test-crypto-perf/cperf.h                  |   1 -
 app/test-crypto-perf/cperf_ops.c              |  46 ++--
 app/test-crypto-perf/cperf_ops.h              |   6 +-
 app/test-crypto-perf/cperf_test_latency.c     |   5 +-
 app/test-crypto-perf/cperf_test_latency.h     |   1 -
 .../cperf_test_pmd_cyclecount.c               |   7 +-
 .../cperf_test_pmd_cyclecount.h               |   1 -
 app/test-crypto-perf/cperf_test_throughput.c  |   5 +-
 app/test-crypto-perf/cperf_test_throughput.h  |   1 -
 app/test-crypto-perf/cperf_test_verify.c      |   5 +-
 app/test-crypto-perf/cperf_test_verify.h      |   1 -
 app/test-crypto-perf/main.c                   |  29 +--
 app/test/test_cryptodev.c                     | 147 ++++---------
 app/test/test_cryptodev.h                     |   1 -
 app/test/test_cryptodev_asym.c                |   1 -
 app/test/test_cryptodev_blockcipher.c         |   6 +-
 app/test/test_event_crypto_adapter.c          |  28 +--
 app/test/test_ipsec.c                         |  34 +--
 app/test/test_ipsec_perf.c                    |   4 +-
 app/test/test_security.c                      | 196 ++++--------------
 drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c  |  33 +--
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c    |   5 +-
 .../crypto/aesni_mb/rte_aesni_mb_pmd_ops.c    |  65 ++----
 drivers/crypto/armv8/rte_armv8_pmd_ops.c      |  34 +--
 drivers/crypto/bcmfs/bcmfs_sym_session.c      |  36 +---
 drivers/crypto/bcmfs/bcmfs_sym_session.h      |   6 +-
 drivers/crypto/caam_jr/caam_jr.c              |  65 ++----
 drivers/crypto/ccp/ccp_pmd_ops.c              |  32 +--
 drivers/crypto/cnxk/cn10k_cryptodev_ops.c     |  24 +--
 drivers/crypto/cnxk/cn10k_ipsec.c             |  53 +----
 drivers/crypto/cnxk/cn9k_cryptodev_ops.c      |  20 +-
 drivers/crypto/cnxk/cn9k_ipsec.c              |  75 +++----
 drivers/crypto/cnxk/cnxk_cryptodev_ops.c      |  61 ++----
 drivers/crypto/cnxk/cnxk_cryptodev_ops.h      |  14 +-
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c   |  69 ++----
 drivers/crypto/dpaa_sec/dpaa_sec.c            |  67 ++----
 drivers/crypto/kasumi/rte_kasumi_pmd_ops.c    |  34 +--
 drivers/crypto/mlx5/mlx5_crypto.c             |  25 +--
 drivers/crypto/mvsam/rte_mrvl_pmd.c           |   3 +-
 drivers/crypto/mvsam/rte_mrvl_pmd_ops.c       |  48 +----
 drivers/crypto/nitrox/nitrox_sym.c            |  33 +--
 drivers/crypto/null/null_crypto_pmd_ops.c     |  34 +--
 .../crypto/octeontx/otx_cryptodev_hw_access.h |   1 -
 drivers/crypto/octeontx/otx_cryptodev_ops.c   |  68 +++---
 drivers/crypto/octeontx2/otx2_cryptodev_ops.c |  63 +++---
 .../octeontx2/otx2_cryptodev_ops_helper.h     |  16 +-
 drivers/crypto/octeontx2/otx2_cryptodev_sec.c |  77 +++----
 drivers/crypto/openssl/rte_openssl_pmd_ops.c  |  35 +---
 drivers/crypto/qat/qat_sym.c                  |   3 +-
 drivers/crypto/qat/qat_sym.h                  |   8 +-
 drivers/crypto/qat/qat_sym_session.c          |  66 ++----
 drivers/crypto/qat/qat_sym_session.h          |  14 +-
 drivers/crypto/scheduler/scheduler_pmd_ops.c  |  10 +-
 drivers/crypto/snow3g/rte_snow3g_pmd_ops.c    |  34 +--
 drivers/crypto/virtio/virtio_cryptodev.c      |  32 +--
 drivers/crypto/zuc/rte_zuc_pmd_ops.c          |  35 +---
 .../octeontx2/otx2_evdev_crypto_adptr_rx.h    |   3 +-
 drivers/net/cnxk/cn10k_ethdev_sec.c           |  64 +++---
 drivers/net/cnxk/cn9k_ethdev_sec.c            |  59 ++----
 drivers/net/cnxk/cnxk_ethdev.c                |   6 +-
 drivers/net/cnxk/cnxk_ethdev.h                |   6 -
 drivers/net/cnxk/cnxk_ethdev_sec.c            |  21 --
 drivers/net/ixgbe/ixgbe_ipsec.c               |  38 +---
 drivers/net/octeontx2/otx2_ethdev_sec.c       |  52 ++---
 drivers/net/octeontx2/otx2_ethdev_sec_tx.h    |   2 +-
 drivers/net/txgbe/txgbe_ipsec.c               |  38 +---
 examples/fips_validation/fips_dev_self_test.c |  32 +--
 examples/fips_validation/main.c               |  20 +-
 examples/ipsec-secgw/ipsec-secgw.c            |  40 ----
 examples/ipsec-secgw/ipsec.c                  |  12 +-
 examples/ipsec-secgw/ipsec.h                  |   1 -
 examples/ipsec-secgw/ipsec_worker.c           |   4 -
 examples/l2fwd-crypto/main.c                  |  41 +---
 examples/vhost_crypto/main.c                  |  16 +-
 lib/cryptodev/cryptodev_pmd.h                 |  33 ++-
 lib/cryptodev/rte_crypto.h                    |   2 +-
 lib/cryptodev/rte_crypto_sym.h                |   2 +-
 lib/cryptodev/rte_cryptodev.c                 |  88 +++++---
 lib/cryptodev/rte_cryptodev.h                 |  70 +++----
 lib/cryptodev/rte_cryptodev_trace.h           |  16 +-
 lib/ipsec/rte_ipsec.h                         |   4 +-
 lib/ipsec/rte_ipsec_group.h                   |  13 +-
 lib/ipsec/ses.c                               |   6 +-
 lib/pipeline/rte_table_action.c               |   8 +-
 lib/pipeline/rte_table_action.h               |   2 +-
 lib/security/rte_security.c                   |  32 +--
 lib/security/rte_security.h                   |  85 +++++---
 lib/security/rte_security_driver.h            |  31 ++-
 lib/vhost/rte_vhost_crypto.h                  |   3 -
 lib/vhost/vhost_crypto.c                      |   7 +-
 90 files changed, 816 insertions(+), 1864 deletions(-)

Comments

Akhil Goyal Oct. 14, 2021, 11:47 a.m. UTC | #1
> Subject: [PATCH v2 0/7] crypto/security session framework rework
> 
> As discussed in last release deprecation notice,
> crypto and security session framework are reworked
> to reduce the need of two mempool objects and
> remove the requirement to expose the rte_security_session
> and rte_cryptodev_sym_session structures.
> Design methodology is explained in the patch description.
> 
> Similar work will need to be done for asymmetric sessions
> as well. Asymmetric session need another rework and is
> postponed to next release. Since it is still in experimental
> stage, we can modify the APIs in next release as well.
> 
> The patches are compilable with all affected PMDs
> and tested with dpdk-test and ipsec-secgw app on CN9k platform.
> 
> Changes in v2:
> - Added new parameter iova in PMD session configure APIs for
>   session priv pointer to be used in QAT/CNXK/etc PMDs.
> - Hide rte_cryptodev_sym_session and rte_security_session structs.
> - Added compilation workaround for net PMDs(ixgbe/txgbe)
>   for inline ipsec.
>   Patches with actual fix is beynd the scope of this patchset.
> - Added inline APIs to access the opaque data and fast metadata.
> - Remove commented code.
> TODO
> - Release notes/deprecation notice removal.
> - Documentation updates.
> - Asym APIs - postponed for next release.
> 
> Akhil Goyal (7):
>   security: rework session framework
>   security: hide security session struct
>   net/cnxk: rework security session framework
>   security: pass session iova in PMD sess create
>   cryptodev: rework session framework
>   cryptodev: hide sym session structure
>   cryptodev: pass session iova in configure session
> 
The series is rebased over following to avoid merge conflicts
http://patches.dpdk.org/user/todo/dpdk/?series=19519
http://patches.dpdk.org/user/todo/dpdk/?series=19467
http://patches.dpdk.org/user/todo/dpdk/?series=19551

(HEAD -> next-crypto) cryptodev: pass session iova in configure session
cryptodev: hide sym session structure
cryptodev: rework session framework
security: pass session iova in PMD sess create
net/cnxk: rework security session framework
security: hide security session struct
security: rework session framework
cryptodev: move device specific structures
cryptodev: update fast path APIs to use new flat array
cryptodev: move inline APIs into separate structure
cryptodev: allocate max space for internal qp array
cryptodev: separate out internal structures
security: add reserved bitfields
security: hide internal API
cryptodev: remove LIST_END enumerators
test/crypto-perf: support lookaside IPsec
Zhang, Roy Fan Oct. 14, 2021, 12:30 p.m. UTC | #2
Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, October 14, 2021 12:48 PM
> To: Akhil Goyal <gakhil@marvell.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; Zhang, Roy Fan
> <roy.fan.zhang@intel.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
...
> The series is rebased over following to avoid merge conflicts
> http://patches.dpdk.org/user/todo/dpdk/?series=19519

The series 19519 will make multi-process case not working for all crypto PMDs. 
Please provide a solution first.
 
> http://patches.dpdk.org/user/todo/dpdk/?series=19467
> http://patches.dpdk.org/user/todo/dpdk/?series=19551
> 
> (HEAD -> next-crypto) cryptodev: pass session iova in configure session
> cryptodev: hide sym session structure
> cryptodev: rework session framework
> security: pass session iova in PMD sess create
> net/cnxk: rework security session framework
> security: hide security session struct
> security: rework session framework
> cryptodev: move device specific structures
> cryptodev: update fast path APIs to use new flat array
> cryptodev: move inline APIs into separate structure
> cryptodev: allocate max space for internal qp array
> cryptodev: separate out internal structures
> security: add reserved bitfields
> security: hide internal API
> cryptodev: remove LIST_END enumerators
> test/crypto-perf: support lookaside IPsec
Akhil Goyal Oct. 14, 2021, 12:34 p.m. UTC | #3
> Hi Akhil,
> 
> The series 19519 will make multi-process case not working for all crypto
> PMDs.
> Please provide a solution first.
> 
Yes I am working on it, will update the series asap.
Zhang, Roy Fan Oct. 14, 2021, 5:07 p.m. UTC | #4
Hi,

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.
- for sw pmds the queue pair's session private mempools are not set.

Regards,
Fan

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, October 14, 2021 12:48 PM
> To: Akhil Goyal <gakhil@marvell.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; Zhang, Roy Fan
> <roy.fan.zhang@intel.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
> 
> > Subject: [PATCH v2 0/7] crypto/security session framework rework
> >
> > As discussed in last release deprecation notice,
> > crypto and security session framework are reworked
> > to reduce the need of two mempool objects and
> > remove the requirement to expose the rte_security_session
> > and rte_cryptodev_sym_session structures.
> > Design methodology is explained in the patch description.
> >
> > Similar work will need to be done for asymmetric sessions
> > as well. Asymmetric session need another rework and is
> > postponed to next release. Since it is still in experimental
> > stage, we can modify the APIs in next release as well.
> >
> > The patches are compilable with all affected PMDs
> > and tested with dpdk-test and ipsec-secgw app on CN9k platform.
> >
> > Changes in v2:
> > - Added new parameter iova in PMD session configure APIs for
> >   session priv pointer to be used in QAT/CNXK/etc PMDs.
> > - Hide rte_cryptodev_sym_session and rte_security_session structs.
> > - Added compilation workaround for net PMDs(ixgbe/txgbe)
> >   for inline ipsec.
> >   Patches with actual fix is beynd the scope of this patchset.
> > - Added inline APIs to access the opaque data and fast metadata.
> > - Remove commented code.
> > TODO
> > - Release notes/deprecation notice removal.
> > - Documentation updates.
> > - Asym APIs - postponed for next release.
> >
> > Akhil Goyal (7):
> >   security: rework session framework
> >   security: hide security session struct
> >   net/cnxk: rework security session framework
> >   security: pass session iova in PMD sess create
> >   cryptodev: rework session framework
> >   cryptodev: hide sym session structure
> >   cryptodev: pass session iova in configure session
> >
> The series is rebased over following to avoid merge conflicts
> http://patches.dpdk.org/user/todo/dpdk/?series=19519
> http://patches.dpdk.org/user/todo/dpdk/?series=19467
> http://patches.dpdk.org/user/todo/dpdk/?series=19551
> 
> (HEAD -> next-crypto) cryptodev: pass session iova in configure session
> cryptodev: hide sym session structure
> cryptodev: rework session framework
> security: pass session iova in PMD sess create
> net/cnxk: rework security session framework
> security: hide security session struct
> security: rework session framework
> cryptodev: move device specific structures
> cryptodev: update fast path APIs to use new flat array
> cryptodev: move inline APIs into separate structure
> cryptodev: allocate max space for internal qp array
> cryptodev: separate out internal structures
> security: add reserved bitfields
> security: hide internal API
> cryptodev: remove LIST_END enumerators
> test/crypto-perf: support lookaside IPsec
Akhil Goyal Oct. 14, 2021, 6:23 p.m. UTC | #5
Hi Fan,
> 
> 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.
> - 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;
                }
Akhil Goyal Oct. 14, 2021, 6:57 p.m. UTC | #6
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;
>                 }
Zhang, Roy Fan Oct. 15, 2021, 8:12 a.m. UTC | #7
Hi Akhil,

It shall work but Kasumi tests are passing :-)
It is snow3g and aesni-mb/gcm that are failing.
Thanks

Regards,
Fan

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, October 14, 2021 7:24 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,
> >
> > 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.
> > - 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;
>                 }
Zhang, Roy Fan Oct. 15, 2021, 3:33 p.m. UTC | #8
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;
> >                 }
Akhil Goyal Oct. 15, 2021, 5:42 p.m. UTC | #9
> 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.
> 
Is it a different fix than what I proposed below? If yes, you can send the diff.
I already made the below changes for all the PMDs.
I will try to fix the asym issue, but I suppose it can be dealt in the app
Which can be fixed separately in RC2.

Also, found the root cause of multi process issue, working on making the patches.
Will send v3 soon with all 3 issues(docsis/mp/sessless) fixed atleast.
For Asym, may send a separate patch.

> > 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;
> > >                 }
Akhil Goyal Oct. 15, 2021, 6:47 p.m. UTC | #10
> > 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.
> >
> Is it a different fix than what I proposed below? If yes, you can send the diff.
> I already made the below changes for all the PMDs.
> I will try to fix the asym issue, but I suppose it can be dealt in the app
> Which can be fixed separately in RC2.
> 
> Also, found the root cause of multi process issue, working on making the
> patches.
> Will send v3 soon with all 3 issues(docsis/mp/sessless) fixed atleast.
> For Asym, may send a separate patch.
> 
For Asym issue, it looks like the APIs are not written properly and has many
Issues compared to sym.
Looking at the API rte_cryptodev_queue_pair_setup(), it only support
mp_session(or priv_sess_mp) for symmetric sessions even without my changes.

Hence, a qp does not have mempool for sessionless Asym processing and looking at current
Drivers, only QAT support asym session less and it does not use mempool stored in qp.

Hence IMO, it is safe to remove the check from rte_cryptodev_queue_pair_setup()
        if (!qp_conf->mp_session) {
                CDEV_LOG_ERR("Invalid mempools\n");
                return -EINVAL;
        }
Or we can have give a CDEV_LOG_INFO (to indicate session mempool not present, session less won't work) instead of CDEV_LOG_ERR and fall through.

For sym case, it is checking again in next line if session_mp is there or not.

I hope, the asym cases will work once we remove the above check and pass
Null in the asym app while setting up queue pairs. What say?
Zhang, Roy Fan Oct. 16, 2021, 1:21 p.m. UTC | #11
Hi Akhil,

I didn't work on the asym problem. As stated in the email I could think of the solution is to add new API to create asym session pool - or you may have better solution. 

BTW current test_cryptodev_asym.c the function testsuite_setup() creates the queue pair before creating the session pool, which will always made the queue pair creation fail at the library layer - as the session pool cannot be empty. I don't think the session pool is mandatory when creating the queue pair as it is only needed for session-less operation even for sym crypto - this change also doesn't make sense for the crypto PMDs who don't support session-less operation.

My sym fix is as same as your proposal. Here is my diff as ref for sym crypto seg fault fix.

diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
index 330aad8157..990fc99763 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
@@ -174,27 +174,25 @@ aesni_gcm_get_session(struct aesni_gcm_qp *qp, struct rte_crypto_op *op)
 					sym_op->session,
 					cryptodev_driver_id);
 	} else  {
-		void *_sess;
-		void *_sess_private_data = NULL;
+		struct rte_cryptodev_sym_session *_sess =
+			rte_cryptodev_sym_session_create(qp->sess_mp);
 
-		if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
+		if (_sess == NULL)
 			return NULL;
 
-		if (rte_mempool_get(qp->sess_mp_priv,
-				(void **)&_sess_private_data))
-			return NULL;
+		_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 = (struct aesni_gcm_session *)_sess_private_data;
+		sess = _sess->sess_data[cryptodev_driver_id].data;
 
 		if (unlikely(aesni_gcm_set_session_parameters(qp->ops,
 				sess, sym_op->xform) != 0)) {
 			rte_mempool_put(qp->sess_mp, _sess);
-			rte_mempool_put(qp->sess_mp_priv, _sess_private_data);
 			sess = NULL;
 		}
 		sym_op->session = (struct rte_cryptodev_sym_session *)_sess;
-		set_sym_session_private_data(sym_op->session,
-				cryptodev_driver_id, _sess_private_data);
 	}
 
 	if (unlikely(sess == NULL))
@@ -716,7 +714,6 @@ handle_completed_gcm_crypto_op(struct aesni_gcm_qp *qp,
 		memset(op->sym->session, 0,
 			rte_cryptodev_sym_get_existing_header_session_size(
 				op->sym->session));
-		rte_mempool_put(qp->sess_mp_priv, sess);
 		rte_mempool_put(qp->sess_mp, op->sym->session);
 		op->sym->session = NULL;
 	}
diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h b/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h
index 2763d1c492..cb37fd6b29 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h
@@ -52,8 +52,6 @@ struct aesni_gcm_qp {
 	/**< Queue pair statistics */
 	struct rte_mempool *sess_mp;
 	/**< Session Mempool */
-	struct rte_mempool *sess_mp_priv;
-	/**< Session Private Data Mempool */
 	uint16_t id;
 	/**< Queue Pair Identifier */
 	char name[RTE_CRYPTODEV_NAME_MAX_LEN];
diff --git a/drivers/crypto/aesni_mb/aesni_mb_pmd_private.h b/drivers/crypto/aesni_mb/aesni_mb_pmd_private.h
index 11e7bf5d18..2398fdf1b8 100644
--- a/drivers/crypto/aesni_mb/aesni_mb_pmd_private.h
+++ b/drivers/crypto/aesni_mb/aesni_mb_pmd_private.h
@@ -182,8 +182,6 @@ struct aesni_mb_qp {
 	/**< Ring for placing operations ready for processing */
 	struct rte_mempool *sess_mp;
 	/**< Session Mempool */
-	struct rte_mempool *sess_mp_priv;
-	/**< Session Private Data Mempool */
 	struct rte_cryptodev_stats stats;
 	/**< Queue pair statistics */
 	uint8_t digest_idx;
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
index e8da9ea9e1..d9e525c86f 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
@@ -1024,27 +1024,25 @@ get_session(struct aesni_mb_qp *qp, struct rte_crypto_op *op)
 					(op->sym->sec_session);
 #endif
 	} else {
-		void *_sess = rte_cryptodev_sym_session_create(qp->sess_mp);
-		void *_sess_private_data = NULL;
+		struct rte_cryptodev_sym_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->sess_data[cryptodev_driver_id].data =
+			(void *)((uint8_t *)_sess +
+				rte_cryptodev_sym_get_header_session_size() +
+				(cryptodev_driver_id * _sess->priv_sz));
 
-		sess = (struct aesni_mb_session *)_sess_private_data;
+		sess = _sess->sess_data[cryptodev_driver_id].data;
 
 		if (unlikely(aesni_mb_set_session_parameters(qp->mb_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);
+		op->sym->session = _sess;
 	}
 
 	if (unlikely(sess == NULL))
@@ -1688,7 +1686,6 @@ post_process_mb_job(struct aesni_mb_qp *qp, JOB_AES_HMAC *job)
 		memset(op->sym->session, 0,
 			rte_cryptodev_sym_get_existing_header_session_size(
 				op->sym->session));
-		rte_mempool_put(qp->sess_mp_priv, sess);
 		rte_mempool_put(qp->sess_mp, op->sym->session);
 		op->sym->session = NULL;
 	}
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
index b7a806d51c..b9c0f8b9ee 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
@@ -1072,8 +1072,16 @@ aesni_mb_pmd_sec_capa_get(void *device __rte_unused)
 	return aesni_mb_pmd_security_cap;
 }
 
+/** Returns the size of the aesni multi-buffer session structure */
+static unsigned
+aesni_mb_pmd_sec_session_get_size(void *dev __rte_unused)
+{
+	return sizeof(struct aesni_mb_session);
+}
+
 static struct rte_security_ops aesni_mb_pmd_sec_ops = {
 		.session_create = aesni_mb_pmd_sec_sess_create,
+		.session_get_size = aesni_mb_pmd_sec_session_get_size,
 		.session_update = NULL,
 		.session_stats_get = NULL,
 		.session_destroy = aesni_mb_pmd_sec_sess_destroy,
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..f130400152 100644
--- a/drivers/crypto/kasumi/rte_kasumi_pmd.c
+++ b/drivers/crypto/kasumi/rte_kasumi_pmd.c
@@ -139,27 +139,25 @@ 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 =
+			rte_cryptodev_sym_session_create(qp->sess_mp);
 
-		if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
+		if (_sess == NULL)
 			return NULL;
 
-		if (rte_mempool_get(qp->sess_mp_priv,
-				(void **)&_sess_private_data))
-			return NULL;
+		_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 = (struct kasumi_session *)_sess_private_data;
+		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 +325,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;
 		}
diff --git a/drivers/crypto/qat/qat_sym_pmd.c b/drivers/crypto/qat/qat_sym_pmd.c
index efda921c05..a55fb4f342 100644
--- a/drivers/crypto/qat/qat_sym_pmd.c
+++ b/drivers/crypto/qat/qat_sym_pmd.c
@@ -305,6 +305,7 @@ qat_security_cap_get(void *device __rte_unused)
 static struct rte_security_ops security_qat_ops = {
 
 		.session_create = qat_security_session_create,
+		.session_get_size = qat_security_session_get_private_size,
 		.session_update = NULL,
 		.session_stats_get = NULL,
 		.session_destroy = qat_security_session_destroy,
diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c
index ef92f22c1a..4066230155 100644
--- a/drivers/crypto/qat/qat_sym_session.c
+++ b/drivers/crypto/qat/qat_sym_session.c
@@ -2298,3 +2298,10 @@ qat_security_session_destroy(void *dev __rte_unused, void *sess_priv)
 	return 0;
 }
 #endif
+
+unsigned int
+qat_security_session_get_private_size(void *dev __rte_unused)
+{
+	return RTE_ALIGN_CEIL(sizeof(struct qat_sym_session), 8);
+}
+
diff --git a/drivers/crypto/qat/qat_sym_session.h b/drivers/crypto/qat/qat_sym_session.h
index 4b7de4c9e7..a02ba01adf 100644
--- a/drivers/crypto/qat/qat_sym_session.h
+++ b/drivers/crypto/qat/qat_sym_session.h
@@ -169,6 +169,9 @@ qat_security_session_create(void *dev, struct rte_security_session_conf *conf,
 		void *sess, rte_iova_t sess_iova);
 int
 qat_security_session_destroy(void *dev, void *sess);
+
+unsigned int
+qat_security_session_get_private_size(void *dev);
 #endif
 
 #endif /* _QAT_SYM_SESSION_H_ */
diff --git a/drivers/crypto/snow3g/rte_snow3g_pmd.c b/drivers/crypto/snow3g/rte_snow3g_pmd.c
index 8284ac0b66..02e65393e3 100644
--- a/drivers/crypto/snow3g/rte_snow3g_pmd.c
+++ b/drivers/crypto/snow3g/rte_snow3g_pmd.c
@@ -149,27 +149,26 @@ snow3g_get_session(struct snow3g_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 =
+			rte_cryptodev_sym_session_create(qp->sess_mp);
 
-		if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
+		if (_sess == NULL)
 			return NULL;
 
-		if (rte_mempool_get(qp->sess_mp_priv,
-				(void **)&_sess_private_data))
-			return NULL;
+		_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;
 
-		sess = (struct snow3g_session *)_sess_private_data;
 
 		if (unlikely(snow3g_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))
@@ -352,7 +351,6 @@ process_ops(struct rte_crypto_op **ops, struct snow3g_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;
 		}
diff --git a/drivers/crypto/snow3g/snow3g_pmd_private.h b/drivers/crypto/snow3g/snow3g_pmd_private.h
index 23cf078a9c..96897d4651 100644
--- a/drivers/crypto/snow3g/snow3g_pmd_private.h
+++ b/drivers/crypto/snow3g/snow3g_pmd_private.h
@@ -39,8 +39,6 @@ struct snow3g_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[SNOW3G_DIGEST_LENGTH];
diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index d4b343a7af..2ac333fc35 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -138,27 +138,25 @@ zuc_get_session(struct zuc_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 =
+			rte_cryptodev_sym_session_create(qp->sess_mp);
 
-		if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
+		if (_sess == NULL)
 			return NULL;
 
-		if (rte_mempool_get(qp->sess_mp_priv,
-				(void **)&_sess_private_data))
-			return NULL;
+		_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 = (struct zuc_session *)_sess_private_data;
+		sess = _sess->sess_data[cryptodev_driver_id].data;
 
 		if (unlikely(zuc_set_session_parameters(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))
@@ -341,7 +339,6 @@ process_ops(struct rte_crypto_op **ops, enum zuc_operation op_type,
 			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, sessions[i]);
 			rte_mempool_put(qp->sess_mp, ops[i]->sym->session);
 			ops[i]->sym->session = NULL;
 		}
diff --git a/drivers/crypto/zuc/zuc_pmd_private.h b/drivers/crypto/zuc/zuc_pmd_private.h
index d8684891ee..23cd9dc458 100644
--- a/drivers/crypto/zuc/zuc_pmd_private.h
+++ b/drivers/crypto/zuc/zuc_pmd_private.h
@@ -38,8 +38,6 @@ struct zuc_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[ZUC_DIGEST_LENGTH];


regards,
Fan



> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, October 15, 2021 6:43 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 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.
> >
> Is it a different fix than what I proposed below? If yes, you can send the diff.
> I already made the below changes for all the PMDs.
> I will try to fix the asym issue, but I suppose it can be dealt in the app
> Which can be fixed separately in RC2.
> 
> Also, found the root cause of multi process issue, working on making the
> patches.
> Will send v3 soon with all 3 issues(docsis/mp/sessless) fixed atleast.
> For Asym, may send a separate patch.
> 
> > > 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;
> > > >                 }
Zhang, Roy Fan Oct. 16, 2021, 1:31 p.m. UTC | #12
Hi,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, October 15, 2021 7:47 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 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.
> > >
> > Is it a different fix than what I proposed below? If yes, you can send the
> diff.
> > I already made the below changes for all the PMDs.
> > I will try to fix the asym issue, but I suppose it can be dealt in the app
> > Which can be fixed separately in RC2.
> >
> > Also, found the root cause of multi process issue, working on making the
> > patches.
> > Will send v3 soon with all 3 issues(docsis/mp/sessless) fixed atleast.
> > For Asym, may send a separate patch.
> >
> For Asym issue, it looks like the APIs are not written properly and has many
> Issues compared to sym.
> Looking at the API rte_cryptodev_queue_pair_setup(), it only support
> mp_session(or priv_sess_mp) for symmetric sessions even without my
> changes.
> 
> Hence, a qp does not have mempool for sessionless Asym processing and
> looking at current
> Drivers, only QAT support asym session less and it does not use mempool
> stored in qp.
> 
> Hence IMO, it is safe to remove the check from
> rte_cryptodev_queue_pair_setup()
>         if (!qp_conf->mp_session) {
>                 CDEV_LOG_ERR("Invalid mempools\n");
>                 return -EINVAL;
>         }
> Or we can have give a CDEV_LOG_INFO (to indicate session mempool not
> present, session less won't work) instead of CDEV_LOG_ERR and fall through.
> 

Yes this is a valid fix. It will make queue pair setup work as before.
The old code was like this:

	if ((qp_conf->mp_session && !qp_conf->mp_session_private) ||
			(!qp_conf->mp_session && qp_conf->mp_session_private)) {
		CDEV_LOG_ERR("Invalid mempools\n");
		return -EINVAL;
	}

The requirement was either you provide 2 mempools (one for session and one for
session private) or you don't provide session mempool when creating queue pair at
all. Only otherwise the error is returned.

> For sym case, it is checking again in next line if session_mp is there or not.
> 
> I hope, the asym cases will work once we remove the above check and pass
> Null in the asym app while setting up queue pairs. What say?

It shall be working.  Thanks.
> 
> 
>