[2/3] cryptodev: add security operation to crypto operation

  Add a new security operation structure to the crypto operation to allow
protocol specific parameters defined in rte_security be defined for a
crypto operation.

Please note this is API changes only. Implementation will follow in
next version.

Signed-off-by: David Coyle <david.coyle@intel.com>
Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>
---
 lib/librte_cryptodev/rte_crypto.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
  

> 
> Add a new security operation structure to the crypto operation to allow
> protocol specific parameters defined in rte_security be defined for a
> crypto operation.
> 
> Please note this is API changes only. Implementation will follow in
> next version.
> 
> Signed-off-by: David Coyle <david.coyle@intel.com>
> Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>
> ---
>  lib/librte_cryptodev/rte_crypto.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cryptodev/rte_crypto.h b/lib/librte_cryptodev/rte_crypto.h
> index fd5ef3a87..fbc1df791 100644
> --- a/lib/librte_cryptodev/rte_crypto.h
> +++ b/lib/librte_cryptodev/rte_crypto.h
> @@ -31,8 +31,10 @@ enum rte_crypto_op_type {
>  	/**< Undefined operation type */
>  	RTE_CRYPTO_OP_TYPE_SYMMETRIC,
>  	/**< Symmetric operation */
> -	RTE_CRYPTO_OP_TYPE_ASYMMETRIC
> +	RTE_CRYPTO_OP_TYPE_ASYMMETRIC,
>  	/**< Asymmetric operation */
> +	RTE_CRYPTO_OP_TYPE_SECURITY
> +	/**< Security operation */
>  };
> 
>  /** 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 */
 
?
  

Hi Konstantin, see below

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, June 9, 2020 2:23 PM
> 
> 
> >
> >  /** 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.
  

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?
  

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'd be interested to hear what cryptodev/security maintainers and others think too.
Akhil/Declan - any thoughts on best approach here?
>
  

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.

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

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?
  

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?
  

Hi Akhil

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Tuesday, June 23, 2020 7:38 PM
> > > >
> > > [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.
> 
[DC] I take your point that introducing the rte_security_docsis_op (and the outer
rte_security_op) structure is just adding extra bytes and as Konstantin mentioned,
unnecessary levels of indirection. I am happy to go with the approach of using the
auth offset and length from the sym_op for the CRC values, if there are no major
objections from others on this. This simplifies things and also means we can now
remove the 'uint8_t security[0]' field from rte_crypto_op which was never a nice
thing.

Konstantin also suggested moving the docsis xform to cryptodev. However, I
feel this would be a step too far for cryptodev and propose we keep the docsis
xform in rte_security. It is then consistent with the other protocols like IPSec.