[dpdk-dev] [PATCH] cryptodev: add API note

[dpdk-dev] [PATCH] cryptodev: add API note

[Fiona Trahe]

Add note to cryptodev API that chained mbufs
are not supported in DOCSISBPI mode.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 lib/librte_cryptodev/rte_crypto_sym.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/librte_cryptodev/rte_crypto_sym.h b/lib/librte_cryptodev/rte_crypto_sym.h
index 4d5459f..e066b0a 100644
--- a/lib/librte_cryptodev/rte_crypto_sym.h
+++ b/lib/librte_cryptodev/rte_crypto_sym.h
@@ -417,8 +417,20 @@ struct rte_cryptodev_sym_session;
  * to the data in the source buffer.
  */
 struct rte_crypto_sym_op {
-	struct rte_mbuf *m_src;	/**< source mbuf */
-	struct rte_mbuf *m_dst;	/**< destination mbuf */
+	struct rte_mbuf *m_src;
+	 /**< source mbuf
+	 *
+	 * @note
+	 * For DOCSISBPI mode rte_mbuf.next must be NULL, i.e.
+	 * chained mbufs are not supported in this mode.
+	 */
+	struct rte_mbuf *m_dst;
+	/**< destination mbuf
+	 *
+	 * @note
+	 * For DOCSISBPI mode rte_mbuf.next must be NULL, i.e.
+	 * chained mbufs are not supported in this mode.
+	 */
 
 	enum rte_crypto_sym_op_sess_type sess_type;
 
-- 
2.5.0

Re: [dpdk-dev] [PATCH] cryptodev: add API note

[Declan Doherty]

On 23/03/2017 5:36 PM, Fiona Trahe wrote:
> Add note to cryptodev API that chained mbufs
> are not supported in DOCSISBPI mode.
>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
...
>


Hey Fiona,

Is this really a limitation of DOCSISBPI mode or just the PMDs which 
currently support these operations. I don't see any reason why DOCSISBPI 
mode cipher operation precludes scatter gather operations on the source 
payload.

If there is some fundamental reason why scatter gather operations can't 
be supported I think documenting this in the rte_crypto_cipher_algorithm 
enumeration comments make more sense than in the rte_crypto_sym_op 
structure, as we already specify extra requirements 
RTE_CRYPTO_CIPHER_AES_GCM and RTE_CRYPTO_CIPHER_AES_CCM there.

Re: [dpdk-dev] [PATCH] cryptodev: add API note

[Trahe, Fiona]

Hi Declan,

> -----Original Message-----
> From: Doherty, Declan
> Sent: Friday, March 24, 2017 10:53 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] cryptodev: add API note
> 
> On 23/03/2017 5:36 PM, Fiona Trahe wrote:
> > Add note to cryptodev API that chained mbufs
> > are not supported in DOCSISBPI mode.
> >
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> ...
> >
> 
> 
> Hey Fiona,
> 
> Is this really a limitation of DOCSISBPI mode or just the PMDs which
> currently support these operations. I don't see any reason why DOCSISBPI
> mode cipher operation precludes scatter gather operations on the source
> payload.
> 

The DOCSISBPI spec in section I.12 Fragmented Packet Encryption
https://apps.cablelabs.com/specification/CM-SP-SECv3.1
says "When a packet is fragmented, each fragment is independently encrypted using CBC mode with residual block processing"
Of course that doesn't guarantee that an application won't take one of those fragments and split it across multiple mbufs. But I checked with some subject matter experts who didn't see a use-case for it.
Due to the nature of DOCSIS there would be a performance impact in dereferencing through chained mbufs to get the address of the residual block, on that basis it seems like a better idea to preclude this on the API rather than having every PMD separately document a limitation or implement a less-performant path that's unlikely to be used.

> If there is some fundamental reason why scatter gather operations can't
> be supported I think documenting this in the rte_crypto_cipher_algorithm
> enumeration comments make more sense than in the rte_crypto_sym_op
> structure, as we already specify extra requirements
> RTE_CRYPTO_CIPHER_AES_GCM and RTE_CRYPTO_CIPHER_AES_CCM there.

It seems to me that this constraint is similar to the notes on many of the other fields 
in rte_crypto_sym_op. 
The CCM/GCM comments on the other hand would not fit so well on the xform struct 
as they're linking fields on both auth and cipher xforms so the comment would have to 
be duplicated in 2 places, so works better on the enum.
But I don't feel strongly about where it goes, if you still prefer (and agree it's ok to add this constraint on the API) I can move it to the enum.

Re: [dpdk-dev] [PATCH] cryptodev: add API note

[Declan Doherty]

On 24/03/17 12:52, Trahe, Fiona wrote:
> Hi Declan,
>
>> -----Original Message-----
>> From: Doherty, Declan
>> Sent: Friday, March 24, 2017 10:53 AM
>> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; De Lara Guarch,
>> Pablo <pablo.de.lara.guarch@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] cryptodev: add API note
>>
>> On 23/03/2017 5:36 PM, Fiona Trahe wrote:
>>> Add note to cryptodev API that chained mbufs
>>> are not supported in DOCSISBPI mode.
>>>
>>> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
>>> ---
>> ...
>>>
>>
>>
>> Hey Fiona,
>>
>> Is this really a limitation of DOCSISBPI mode or just the PMDs which
>> currently support these operations. I don't see any reason why DOCSISBPI
>> mode cipher operation precludes scatter gather operations on the source
>> payload.
>>
>
> The DOCSISBPI spec in section I.12 Fragmented Packet Encryption
> https://apps.cablelabs.com/specification/CM-SP-SECv3.1
> says "When a packet is fragmented, each fragment is independently encrypted using CBC mode with residual block processing"
> Of course that doesn't guarantee that an application won't take one of those fragments and split it across multiple mbufs. But I checked with some subject matter experts who didn't see a use-case for it.
> Due to the nature of DOCSIS there would be a performance impact in dereferencing through chained mbufs to get the address of the residual block, on that basis it seems like a better idea to preclude this on the API rather than having every PMD separately document a limitation or implement a less-performant path that's unlikely to be used.
>

Ok, thanks for clarifying that. So the only time this could possibly 
happen is if the mbufs themselves where not big enough to fit the packet 
MTU and the NIC port had the scatter rx path enabled.


>> If there is some fundamental reason why scatter gather operations can't
>> be supported I think documenting this in the rte_crypto_cipher_algorithm
>> enumeration comments make more sense than in the rte_crypto_sym_op
>> structure, as we already specify extra requirements
>> RTE_CRYPTO_CIPHER_AES_GCM and RTE_CRYPTO_CIPHER_AES_CCM there.
>
> It seems to me that this constraint is similar to the notes on many of the other fields
> in rte_crypto_sym_op.
> The CCM/GCM comments on the other hand would not fit so well on the xform struct
> as they're linking fields on both auth and cipher xforms so the comment would have to
> be duplicated in 2 places, so works better on the enum.
> But I don't feel strongly about where it goes, if you still prefer (and agree it's ok to add this constraint on the API) I can move it to the enum.
>

I you want to go ahead and place this restriction on these algorithms 
then to my mind it probably makes sense to document it with the 
algorithm definition in the enumeration definition.

[dpdk-dev] [PATCH v2] cryptodev: add API note

[Fiona Trahe]

Add note to cryptodev API that chained mbufs
are not supported in DOCSISBPI mode.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
v2 changes:
 - moved the comment from the rte_crypto_sym_op to the algorithm enum

 lib/librte_cryptodev/rte_crypto_sym.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_cryptodev/rte_crypto_sym.h b/lib/librte_cryptodev/rte_crypto_sym.h
index 4d5459f..508f4ee 100644
--- a/lib/librte_cryptodev/rte_crypto_sym.h
+++ b/lib/librte_cryptodev/rte_crypto_sym.h
@@ -111,11 +111,15 @@ enum rte_crypto_cipher_algorithm {
 	RTE_CRYPTO_CIPHER_AES_DOCSISBPI,
 	/**< AES algorithm using modes required by
 	 * DOCSIS Baseline Privacy Plus Spec.
+	 * Chained mbufs are not supported in this mode, i.e. rte_mbuf.next
+	 * for m_src and m_dst in the rte_crypto_sym_op must be NULL.
 	 */
 
 	RTE_CRYPTO_CIPHER_DES_DOCSISBPI,
 	/**< DES algorithm using modes required by
 	 * DOCSIS Baseline Privacy Plus Spec.
+	 * Chained mbufs are not supported in this mode, i.e. rte_mbuf.next
+	 * for m_src and m_dst in the rte_crypto_sym_op must be NULL.
 	 */
 
 	RTE_CRYPTO_CIPHER_LIST_END
-- 
2.5.0

Re: [dpdk-dev] [PATCH v2] cryptodev: add API note

[Declan Doherty]

On 03/04/2017 4:51 PM, Fiona Trahe wrote:
> Add note to cryptodev API that chained mbufs
> are not supported in DOCSISBPI mode.
>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
...
>

Acked-by: Declan Doherty <declan.doherty@intel.com>

Re: [dpdk-dev] [PATCH v2] cryptodev: add API note

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Declan Doherty
> Sent: Tuesday, April 04, 2017 10:08 AM
> To: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] cryptodev: add API note
> 
> On 03/04/2017 4:51 PM, Fiona Trahe wrote:
> > Add note to cryptodev API that chained mbufs
> > are not supported in DOCSISBPI mode.
> >
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> ...
> >
> 
> Acked-by: Declan Doherty <declan.doherty@intel.com>

Applied to dpdk-next-crypto.
Thanks,

Pablo