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

[Ananyev, Konstantin]

Hi Akhil,

> > > 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.
     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).