Hi Konstantin/David, > > Hi David, > > > > > > > > > > > > > > > > > > /** 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.
Adding DOCSIS alone is not an issue in the cryptodev. The intent of this patchset and Previous RFCs was chaining of two - DOCSIS and CRC which are supposed to be separate Blocks and we need a way to combine the two and use it in the application. rte_security provides a way to handle such protocols for algo combinations. However, IMO we do not really need a separate rte_security_docsis_op structure, As it has parameters which are already there in the rte_crypto_sym_op. This new op Struct is just adding extra bytes which can be avoided if we use sym_op->auth.data.offset And sym_op->auth.data.length in place of crc offset and crc length. We may just need to add comment in the struct definition about its usage for CRC cases. > > > > > I'd be interested to hear what cryptodev/security maintainers and others > > think > too. > > Akhil/Declan - any thoughts on best approach here?