Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> > > > User can use the same session, that is what I am also insisting, but it may have
> > > separate
> > > > Session private data. Cryptodev session create API provide that functionality
> > > and we can
> > > > Leverage that.
> > >
> > > rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which means
> > > we can't use
> > > the same rte_cryptodev_sym_session to hold sessions for both sync and async
> > > mode
> > > for the same device. Off course we can add a hard requirement that any driver
> > > that wants to
> > > support process() has to create sessions that can handle both  process and
> > > enqueue/dequeue,
> > > but then again  what for to create such overhead?
> > >
> > > BTW, to be honest, I don't consider current rte_cryptodev_sym_session
> > > construct for multiple device_ids:
> > > __extension__ struct {
> > >                 void *data;
> > >                 uint16_t refcnt;
> > >         } sess_data[0];
> > >         /**< Driver specific session material, variable size */
> > >
> > Yes I also feel the same. I was also not in favor of this when it was introduced.
> > Please go ahead and remove this. I have no issues with that.
> 
> If you are not happy with that structure, and admit there are issues with it,
> why do you push for reusing it for cpu-crypto API?
> Why  not to take step back, take into account current drawbacks
> and define something that (hopefully) would suite us better?
> Again new API will be experimental for some time, so we'll
> have some opportunity to see does it works and if not fix it.
> 
> About removing data[] from existing rte_cryptodev_sym_session -
> Personally would like to do that, but the change seems to be too massive.
> Definitely not ready for such effort right now.
> 
> >
> > > as an advantage.
> > > It looks too error prone for me:
> > > 1. Simultaneous session initialization/de-initialization for devices with the same
> > > driver_id is not possible.
> > > 2. It assumes that all device driver will be loaded before we start to create
> > > session pools.
> > >
> > > Right now it seems ok, as no-one requires such functionality, but I don't know
> > > how it will be in future.
> > > For me rte_security session model, where for each security context user have to
> > > create new session
> > > looks much more robust.
> > Agreed
> >
> > >
> > > >
> > > > BTW, I can see a v2 to this RFC which is still based on security library.
> > >
> > > Yes, v2 was concentrated on fixing found issues, some code restructuring,
> > > i.e. - changes that would be needed anyway whatever API aproach we'll choose.
> > >
> > > > When do you plan
> > > > To submit the patches for crypto based APIs. We have RC1 merge deadline for
> > > this
> > > > patchset on 21st Oct.
> > >
> > > We'd like to start working on it ASAP, but it seems we still have a major
> > > disagreement
> > > about how this crypto-dev API should look like.
> > > Which makes me think - should we return to our original proposal via
> > > rte_security?
> > > It still looks to me like clean and straightforward way to enable this new API,
> > > and probably wouldn't cause that much controversy.
> > > What do you think?
> >
> > I cannot spend more time discussing on this until RC1 date. I have some other stuff pending.
> > You can send the patches early next week with the approach that I mentioned or else we
> > can discuss this post RC1(which would mean deferring to 20.02).
> >
> > But moving back to security is not acceptable to me. The code should be put where it is
> > intended and not where it is easy to put. You are not doing any rte_security stuff.
> >
> 
> Ok, then my suggestion:
> Let's at least write down all points about crypto-dev approach where we
> disagree and then probably try to resolve them one by one....
> If we fail to make an agreement/progress in next week or so,
> (and no more reviews from the community)
> will have bring that subject to TB meeting to decide.
> Sounds fair to you?
> 
> List is below.
> Please add/correct me, if I missed something.
> 
> Konstantin
> 
> 1. extra input parameters to create/init rte_(cpu)_sym_session.
> 
> Will leverage existing 6B gap inside rte_crypto_*_xform between 'algo' and 'key' fields.
> New fields will be optional and would be used by PMD only when cpu-crypto session is requested.
> For lksd-crypto session PMD is free to ignore these fields.
> No ABI breakage is required.
> 
> Hopefully no controversy here with #1.
> 
> 2. cpu-crypto create/init.
>     a) Our suggestion - introduce new API for that:
>         - rte_crypto_cpu_sym_init() that would init completely opaque  rte_crypto_cpu_sym_session.
>         - struct rte_crypto_cpu_sym_session_ops {(*process)(...); (*clear); /*whatever else we'll need *'};
>         - rte_crypto_cpu_sym_get_ops(const struct rte_crypto_sym_xform *xforms)
>           that would return const struct rte_crypto_cpu_sym_session_ops *based on input xforms.
> 	Advantages:
> 	1)  totally opaque data structure (no ABI breakages in future), PMD writer is totally free
> 	     with it format and contents.
> 	2) each session entity is self-contained, user doesn't need to bring along dev_id etc.
> 	    dev_id is needed  only at init stage, after that user will use session ops to perform
> 	    all operations on that session (process(), clear(), etc.).
> 	3) User can decide does he wants to store ops[] pointer on a per session basis,
> 	    or on a per group of same sessions, or...
> 	4) No mandatory mempools for private sessions. User can allocate memory for cpu-crypto
> 	    session whenever he likes.
> 	Disadvantages:
> 	5) Extra changes in control path
> 	6) User has to store session_ops pointer explicitly.

After another thought if 2.a.6 is really that big deal we can have small shim layer on top:

rte_crypto_cpu_sym_session { void *ses; struct rte_crypto_cpu_sym_session_ops * const ops; }
OR even
rte_crypto_cpu_sym_session { void *ses; struct rte_crypto_cpu_sym_session_ops ops; }

And merge rte_crypto_cpu_sym_init() and rte_crypto_cpu_sym_get_ops() into one (init).

Then process() can become a wrapper:
rte_crypto_cpu_sym_process(ses, ...) {return ses->ops->process(ses->ses, ...);}
OR
rte_crypto_cpu_sym_process(ses, ...) {return ses->ops.process(ses->ses, ...);}

if that would help to reach consensus - works for me. 

>      b) Your suggestion - reuse existing rte_cryptodev_sym_session_init() and existing rte_cryptodev_sym_session
>       structure.
> 	Advantages:
> 	1) allows to reuse same struct and init/create/clear() functions.
> 	    Probably less changes in control path.
> 	Disadvantages:
> 	2) rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which means that
> 	    we can't use the same rte_cryptodev_sym_session to hold private sessions pointers
> 	    for both sync and async mode  for the same device.
>                    So wthe only option we have - make PMD devops->sym_session_configure()
> 	    always create a session that can work in both cpu and lksd modes.
> 	    For some implementations that would probably mean that under the hood  PMD would create
> 	    2 different session structs (sync/async) and then use one or another depending on from what API been called.
> 	    Seems doable, but ...:
>                    - will contradict with statement from 1:
> 	      " New fields will be optional and would be used by PMD only when cpu-crypto session is requested."
>                       Now it becomes mandatory for all apps to specify cpu-crypto related parameters too,
> 	       even if they don't plan to use that mode - i.e. behavior change, existing app change.
>                      - might cause extra space overhead.
> 	3) not possible to store device (not driver) specific data within the session, but I think it is not really needed right now.
> 	    So probably minor compared to 2.b.2.
> 
> Actually #3 follows from #2, but decided to have them separated.
> 
> 3. process() parameters/behavior
>     a) Our suggestion: user stores ptr to session ops (or to (*process) itself) and just does:
>         session_ops->process(sess, ...);
> 	Advantages:
> 	1) fastest possible execution path
> 	2) no need to carry on dev_id for data-path
> 	Disadvantages:
> 	3) user has to carry on session_ops pointer explicitly
>     b) Your suggestion: add  (*cpu_process) inside rte_cryptodev_ops and then:
>         rte_crypto_cpu_sym_process(uint8_t dev_id, rte_cryptodev_sym_session  *sess, /*data parameters*/) {...
>                      rte_cryptodevs[dev_id].dev_ops->cpu_process(ses, ...);
>                       /*and then inside PMD specifc process: */
>                      pmd_private_session = sess->sess_data[this_pmd_driver_id].data;
>                      /* and then most likely either */
>                      pmd_private_session->process(pmd_private_session, ...);
>                      /* or jump based on session/input data */
> 	Advantages:
> 	1) don't see any...
> 	Disadvantages:
> 	2) User has to carry on dev_id inside data-path
> 	3) Extra level of indirection (plus data dependency) - both for data and instructions.
> 	    Possible slowdown compared to a) (not measured).
>