On 10/11/2018 7:50 PM, Konstantin Ananyev wrote: > Below are details and reasoning for proposed changes. > > 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear() > operate based on cytpodev device id, though inside > rte_cryptodev_sym_session device specific data is addressed > by driver id (not device id). > That creates a problem with current implementation when we have > two or more devices with the same driver used by the same session. > Consider the following example: > > struct rte_cryptodev_sym_session *sess; > rte_cryptodev_sym_session_init(dev_id=X, sess, ...); > rte_cryptodev_sym_session_init(dev_id=Y, sess, ...); > rte_cryptodev_sym_session_clear(dev_id=X, sess); > > After that point if X and Y uses the same driver, > then sess can't be used by device Y any more. > The reason for that - driver specific (not device specific) > data per session, plus there is no information > how many device instances use that data. > Probably the simplest way to deal with that issue - > add a reference counter per each driver data. > > 2.rte_cryptodev_sym_session_set_user_data() and > rte_cryptodev_sym_session_get_user_data() - > with current implementation there is no defined way for the user to > determine what is the max allowed size of the private data. > rte_cryptodev_sym_session_set_user_data() just blindly copies > user provided data without checking memory boundaries violation. > To overcome that issue propose to add 'uint16_t priv_size' into > rte_cryptodev_sym_session structure. > > 3.rte_cryptodev_sym_session contains an array of variable size for > driver specific data. > Though number of elements in that array is determined by static > variable nb_drivers, that could be modified by > rte_cryptodev_allocate_driver(). > That construction seems to work ok so far, as right now users register > all their PMDs at startup, though it doesn't mean that it would always > remain like that. > To make it less error prone propose to add 'uint16_t nb_drivers' > into the rte_cryptodev_sym_session structure. > At least that allows related functions to check that provided > driver id wouldn't overrun variable array boundaries, > again it allows to determine size of already allocated session > without accessing global variable. > > 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session > would have sort of readonly type data (init once at allocation time, > keep unmodified through session life-time). > That requires more changes in current cryptodev implementation: > Right now inside cryptodev framework both rte_cryptodev_sym_session > and driver specific session data are two completely different sctrucures > (e.g. struct cryptodev_sym_session and struct null_crypto_session). > Though current cryptodev implementation implicitly assumes that driver > will allocate both of them from within the same mempool. > Plus this is done in a manner that they override each other fields > (reuse the same space - sort of implicit C union). > That's probably not the best programming practice, > plus make impossible to have readonly fields inside both of them. > To overcome that situation propose to changed an API a bit, to allow > to use two different mempools for these two distinct data structures. > > 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session. > I suppose that self-explanatory, and might be used in a lot of places > (would be quite useful for ipsec library we develop). > > The new proposed layout for rte_cryptodev_sym_session: > struct rte_cryptodev_sym_session { > uint64_t userdata; > /**< Can be used for external metadata */ > uint16_t nb_drivers; > /**< number of elements in sess_data array */ > uint16_t priv_size; > /**< session private data will be placed after sess_data */ > __extension__ struct { > void *data; > uint16_t refcnt; > } sess_data[0]; > /**< Driver specific session material, variable size */ > }; > > Signed-off-by: Konstantin Ananyev <konstantin.anan...@intel.com>
Adding maintainers to ack this deprecation notice. These changes will impact all the PMDs and everyone should agree to these changes. from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs: Acked-by: Akhil Goyal <akhil.go...@nxp.com> > --- > doc/guides/rel_notes/deprecation.rst | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/doc/guides/rel_notes/deprecation.rst > b/doc/guides/rel_notes/deprecation.rst > index d2aec64d1..998a0d92c 100644 > --- a/doc/guides/rel_notes/deprecation.rst > +++ b/doc/guides/rel_notes/deprecation.rst > @@ -74,3 +74,12 @@ Deprecation Notices > > This is due to a lack of flexibility and reliance on a type unusable with > C++ programs (struct rte_flow_desc). > + > +* cryptodev: several API and ABI changes are planned for rte_cryptodev > + in v19.02: > + > + - The size and layout of ``rte_cryptodev_sym_session`` will change > + to fix existing issues. > + - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of > + ``rte_cryptodev_queue_pair_setup`` will change to to allow to use > + two different mempools for crypto and device private sessions.