[dpdk-dev] [PATCH v3 0/8] crypto/security session framework rework

[Zhang, Roy Fan]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil at marvell.com>
> Sent: Wednesday, October 20, 2021 7:05 PM
> To: Power, Ciara <ciara.power at intel.com>; dev at dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev at intel.com>; thomas at monjalon.net; Zhang,
> Roy Fan <roy.fan.zhang at intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch at intel.com>
> Cc: david.marchand at redhat.com; hemant.agrawal at nxp.com; Anoob Joseph
> <anoobj at marvell.com>; Trahe, Fiona <fiona.trahe at intel.com>; Doherty,
> Declan <declan.doherty at intel.com>; matan at nvidia.com; g.singh at nxp.com;
> jianjay.zhou at huawei.com; asomalap at amd.com; ruifeng.wang at arm.com;
> Nicolau, Radu <radu.nicolau at intel.com>; ajit.khaparde at broadcom.com;
> Nagadheeraj Rottela <rnagadheeraj at marvell.com>; Ankur Dwivedi
> <adwivedi at marvell.com>; Wang, Haiyue <haiyue.wang at intel.com>;
> jiawenwu at trustnetic.com; jianwang at trustnetic.com; Jerin Jacob
> Kollanukkaran <jerinj at marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram at marvell.com>
> Subject: RE: [PATCH v3 0/8] crypto/security session framework rework
> 
> > > > I am seeing test failures for cryptodev_scheduler_autotest:
> > > > + Tests Total :       638
> > > >  + Tests Skipped :     280
> > > >  + Tests Executed :    638
> > > >  + Tests Unsupported:   0
> > > >  + Tests Passed :      18
> > > >  + Tests Failed :      340
> > > >
> > > > The error showing for each testcase:
> > > > scheduler_pmd_sym_session_configure() line 487: unable to config
> sym
> > > > session
> > > > CRYPTODEV: rte_cryptodev_sym_session_init() line 1743: dev_id 2
> failed
> > to
> > > > configure session details
> > > >
> > > > I believe the problem happens in
> > scheduler_pmd_sym_session_configure.
> > > > The full sess object is no longer accessible in here, but it is required to
> be
> > > > passed to rte_cryptodev_sym_session_init.
> > > > The init function expects access to sess rather than the private data,
> and
> > > now
> > > > fails as a result.
> > > >
> > > > static int
> > > > scheduler_pmd_sym_session_configure(struct rte_cryptodev *dev,
> > > >         struct rte_crypto_sym_xform *xform, void *sess,
> > > >         rte_iova_t sess_iova __rte_unused)
> > > > {
> > > >         struct scheduler_ctx *sched_ctx = dev->data->dev_private;
> > > >         uint32_t i;
> > > >         int ret;
> > > >         for (i = 0; i < sched_ctx->nb_workers; i++) {
> > > >                 struct scheduler_worker *worker = &sched_ctx->workers[i];
> > > >                 ret = rte_cryptodev_sym_session_init(worker->dev_id, sess,
> > > >                                         xform);
> > > >                 if (ret < 0) {
> > > >                         CR_SCHED_LOG(ERR, "unable to config sym session");
> > > >                         return ret;
> > > >                 }
> > > >         }
> > > >         return 0;
> > > > }
> > > >
> > > It looks like scheduler PMD is managing the stuff on its own for other
> > PMDs.
> > > The APIs are designed such that the app can call session_init multiple
> times
> > > With different dev_id on same sess.
> > > But here scheduler PMD internally want to configure other PMDs
> sess_priv
> > > By calling session_init.
> > >
> > > I wonder, why we have this 2 step session_create and session_init?
> > > Why can't we have it similar to security session create and let the
> scheduler
> > > PMD have its big session private data which can hold priv_data of as many
> > > PMDs
> > > as it want to schedule.
> > >
> > > Konstantin/Fan/Pablo what are your thoughts on this issue?
> > > Can we resolve this issue at priority in RC1(or probably RC2) for this
> release
> > > or
> > > else we defer it for next ABI break release?
> > >
> > > Thomas,
> > > Can we defer this for RC2? It does not seem to be fixed in 1 day.
> >
> > On another thought, this can be fixed with current patch also by having a
> big
> > session
> > Private data for scheduler PMD which is big enough to hold all other PMDs
> > data which
> > it want to schedule and then call the sess_configure function pointer of dev
> > directly.
> > What say? And this PMD change can be done in RC2. And this patchset go
> as
> > is in RC1.
> Here is the diff in scheduler PMD which should fix this issue in current
> patchset.
> 
> diff --git a/drivers/crypto/scheduler/scheduler_pmd_ops.c
> b/drivers/crypto/scheduler/scheduler_pmd_ops.c
> index b92ffd6026..0611ea2c6a 100644
> --- a/drivers/crypto/scheduler/scheduler_pmd_ops.c
> +++ b/drivers/crypto/scheduler/scheduler_pmd_ops.c
> @@ -450,9 +450,8 @@ scheduler_pmd_qp_setup(struct rte_cryptodev *dev,
> uint16_t qp_id,
>  }
> 
>  static uint32_t
> -scheduler_pmd_sym_session_get_size(struct rte_cryptodev *dev
> __rte_unused)
> +get_max_session_priv_size(struct scheduler_ctx *sched_ctx)
>  {
> -       struct scheduler_ctx *sched_ctx = dev->data->dev_private;
>         uint8_t i = 0;
>         uint32_t max_priv_sess_size = 0;
> 
> @@ -469,20 +468,35 @@ scheduler_pmd_sym_session_get_size(struct
> rte_cryptodev *dev __rte_unused)
>         return max_priv_sess_size;
>  }
> 
> +static uint32_t
> +scheduler_pmd_sym_session_get_size(struct rte_cryptodev *dev)
> +{
> +       struct scheduler_ctx *sched_ctx = dev->data->dev_private;
> +
> +       return get_max_session_priv_size(sched_ctx) * sched_ctx-
> >nb_workers;
> +}
> +
>  static int
>  scheduler_pmd_sym_session_configure(struct rte_cryptodev *dev,
>         struct rte_crypto_sym_xform *xform, void *sess,
>         rte_iova_t sess_iova __rte_unused)
>  {
>         struct scheduler_ctx *sched_ctx = dev->data->dev_private;
> +       uint32_t worker_sess_priv_sz = get_max_session_priv_size(sched_ctx);
>         uint32_t i;
>         int ret;
> 
>         for (i = 0; i < sched_ctx->nb_workers; i++) {
>                 struct scheduler_worker *worker = &sched_ctx->workers[i];
> +               struct rte_cryptodev *worker_dev =
> +                               rte_cryptodev_pmd_get_dev(worker->dev_id);
> +               uint8_t index = worker_dev->driver_id;
> 
> -               ret = rte_cryptodev_sym_session_init(worker->dev_id, sess,
> -                                       xform);
> +               ret = worker_dev->dev_ops->sym_session_configure(
> +                               worker_dev,
> +                               xform,
> +                               (uint8_t *)sess + (index * worker_sess_priv_sz),
> +                               sess_iova + (index * worker_sess_priv_sz));

This won't work. This will make the session configuration finish successfully
 but  the private data the worker initialized is not the private data the worker
will use during enqueue/dequeue (workers only uses the session private
data based on its driver id).

>                 if (ret < 0) {
>                         CR_SCHED_LOG(ERR, "unable to config sym session");
>                         return ret;