Re: [dpdk-dev] [PATCH 2/3] cryptodev: add security operation to crypto operation

["Coyle, David" <david.coyle@intel.com>]

Hi Konstantin,

> > > > >
> > > > > >
> > > > > >  /** Status of crypto operation */ @@ -121,6 +123,13 @@ struct
> > > > > > rte_crypto_op {
> > > > > >  		struct rte_crypto_asym_op asym[0];
> > > > > >  		/**< Asymmetric operation parameters */
> > > > > >
> > > > > > +#ifdef RTE_LIBRTE_SECURITY
> > > > > > +		uint8_t security[0];
> > > > > > +		/**< Security operation parameters
> > > > > > +		 * - Must be accessed through a rte_security_op
> pointer
> > > > > > +		 */
> > > > > > +#endif
> > > > > > +
> > > > > >  	}; /**< operation specific parameters */  };
> > > > >
> > > > > Is there any point to have this extra level of indirection?
> > > > > Might be simply:
> > > > >
> > > > > enum rte_crypto_op_type {
> > > > > 	....
> > > > > +	RTE_CRYPTO_OP_TYPE_SEC_DOCSIS,
> > > > > };
> > > > > ...
> > > > > struct rte_crypto_op {
> > > > > 	....
> > > > > 	__extension__
> > > > >         union {
> > > > >                 struct rte_crypto_sym_op sym[0];
> > > > >                 /**< Symmetric operation parameters */
> > > > >
> > > > >                 struct rte_crypto_asym_op asym[0];
> > > > >                 /**< Asymmetric operation parameters */
> > > > >
> > > > > +	struct rte_security_docsis_op docsis[0];
> > > > >
> > > > >         }; /**< operation specific parameters */
> > > > >
> > > > > ?
> > > > [DC] This was to allow some form of extensibility and not to limit
> > > > this to just
> > > DOCSIS.
> > > > If it's felt that having the extra level of indirection is
> > > > overkill, it can be easily
> > > changed.
> > > >
> > > > However, we cannot include a struct of type 'struct
> > > > rte_security_docsis_op' (or 'struct rte_security_op') directly
> > > > here, without creating nasty circular dependency of includes
> > > > between
> > > rte_cryptodev and rte_security.
> > > >
> > > > I had tried defining an opaque version 'struct rte_security_op' (i.e.
> > > > no fields within the struct) here in rte_crypto.h, but the
> > > > compiler complained that it couldn't determine the size of the
> > > > struct, even though
> > > it's a zero length array.
> > > >
> > > > That is why I had to use the uint8_t in 'uint8_t security[0];' - I
> > > > don't like this, but I couldn't find another way that kept the
> > > > compiler happy
> > > and didn't create a circular dependency.
> > >
> > > I see... would it be an option to name this struct 'struct
> > > rte_sym_docsis_op and and move actual definition inside
> > > lib/librte_cryptodev/rte_crypto_sym.h?
> > >
> > [DC] It's certainly an option and would work but I don't think it's a
> > good idea to be putting protocol specific structs like this in rte_cryptodev -
> that's what rte_security is for.
> > Do you think it would be ok to do this?
> 
> I personally don't see a problem with this.
> In fact, as an extra  thought - why we can't have docsis xform defined in
> lib/librte_cryptodev/rte_crypto_sym.h too, and then just have it  as a
> member inside struct rte_crypto_sym_xform union?
> Then we can have rte_cryptodev_sym_session that supports docsis stuff.
> 
[DC] Because DOCSIS protocol and CRC are not specifically crypto related is why
we initially went down the rawdev/multi-fn route and now the rte_security route.
I think adding docsis xforms/ops and CRC related data to cryptodev would be adding
too much non-crypto algorithm related stuff to this library. There would then be some
protocols like IPSec and PDCP with their definitions in rte_security and others like
DOCSIS in rte_cryptodev - that doesn't seem good to me.

Yes, from a DOCSIS equipment vendors point-of-view, who already use cryptodev
for just encryption/decryption, adding DOCSIS to cryptodev would be best
for them in order to get better DOCSIS support in DPDK as it would mean less
churn for their applications. However, from a DPDK point-of-view, I don't think it
would be correct to do this.

That's just my opinion, and again I'd be interested to hear other people's thoughts.

> >
> > I'd be interested to hear what cryptodev/security maintainers and others
> think too.
> > Akhil/Declan - any thoughts on best approach here?