[PATCH] crypto: remove unused digest-appended feature

[PATCH] crypto: remove unused digest-appended feature

[Fiona Trahe]

The cryptodev API had specified that if the digest address field was
left empty on an authentication operation, then the PMD would assume
the digest was appended to the source or destination data.
This case was not handled at all by most PMDs and incorrectly handled
by the QAT PMD.
As no bugs were raised, it is assumed to be not needed, so this patch
removes it, rather than add handling for the case on all PMDs.
The digest can still be appended to the data, but its 
address must now be provided in the op.

I've added this cover letter to pre-empt the question which will arise 
in chained-mbuf case.
(Chained-mbuf support will be added in next few weeks for 17.02)
In a chained-mbuf case, if the digest is appended to the data, it could
in theory end up split across the last 2 mbufs. Using the digest address 
field only works for the case where the digest is in a flat buffer. The 
digest-appended feature would have provided one way of handling this, 
pushing the responsibility down to the PMD to follow the chain to find
the digest address and handle it whether in 1 or 2 mbufs. 
However, this is adding extra cycles on the data path in all PMDs  
to handle a case which may not arise in many applications.
The alternative is to give the application the responsibility 
to make sure the digest is in a flat buffer. 
Specifically if an application knows cases of a digest spanning
buffers can arise, then it can check for this case and memcpy the partial
start of the digest from the second last buffer into the last buffer
so it's no longer split.
Does anyone see a problem with this?


Fiona Trahe (1):
  crypto: remove unused digest-appended feature

 drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
 drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
 lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
 3 files changed, 4 insertions(+), 26 deletions(-)

-- 
2.5.0

[PATCH] crypto: remove unused digest-appended feature

[Fiona Trahe]

The cryptodev API had specified that if the digest address field was
left empty on an authentication operation, then the PMD would assume
the digest was appended to the source or destination data.
This case was not handled at all by most PMDs and incorrectly handled
by the QAT PMD.
As no bugs were raised, it is assumed to be not needed, so this patch
removes it, rather than add handling for the case on all PMDs.
The digest can still be appended to the data, but its
address must now be provided in the op.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
 drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
 lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
 3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c b/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
index 8900668..f4e24b3 100644
--- a/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
+++ b/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
@@ -439,6 +439,8 @@ void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr *header,
 				proto);
 	ICP_QAT_FW_LA_UPDATE_STATE_SET(header->serv_specif_flags,
 					   ICP_QAT_FW_LA_NO_UPDATE_STATE);
+	ICP_QAT_FW_LA_DIGEST_IN_BUFFER_SET(header->serv_specif_flags,
+					ICP_QAT_FW_LA_NO_DIGEST_IN_BUFFER);
 }
 
 int qat_alg_aead_session_create_content_desc_cipher(struct qat_session *cdesc,
diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
index 798cd98..6a6bd2e 100644
--- a/drivers/crypto/qat/qat_crypto.c
+++ b/drivers/crypto/qat/qat_crypto.c
@@ -955,7 +955,6 @@ qat_write_hw_desc_entry(struct rte_crypto_op *op, uint8_t *out_msg)
 	uint32_t cipher_len = 0, cipher_ofs = 0;
 	uint32_t auth_len = 0, auth_ofs = 0;
 	uint32_t min_ofs = 0;
-	uint32_t digest_appended = 1;
 	uint64_t buf_start = 0;
 
 
@@ -1068,14 +1067,7 @@ qat_write_hw_desc_entry(struct rte_crypto_op *op, uint8_t *out_msg)
 		}
 		min_ofs = auth_ofs;
 
-		if (op->sym->auth.digest.phys_addr) {
-			ICP_QAT_FW_LA_DIGEST_IN_BUFFER_SET(
-					qat_req->comn_hdr.serv_specif_flags,
-					ICP_QAT_FW_LA_NO_DIGEST_IN_BUFFER);
-			auth_param->auth_res_addr =
-					op->sym->auth.digest.phys_addr;
-			digest_appended = 0;
-		}
+		auth_param->auth_res_addr = op->sym->auth.digest.phys_addr;
 
 		auth_param->u1.aad_adr = op->sym->auth.aad.phys_addr;
 
@@ -1126,14 +1118,6 @@ qat_write_hw_desc_entry(struct rte_crypto_op *op, uint8_t *out_msg)
 		(cipher_param->cipher_offset + cipher_param->cipher_length)
 		: (auth_param->auth_off + auth_param->auth_len);
 
-	if (do_auth && digest_appended) {
-		if (ctx->auth_op == ICP_QAT_HW_AUTH_GENERATE)
-			qat_req->comn_mid.dst_length
-					+= op->sym->auth.digest.length;
-		else
-			qat_req->comn_mid.src_length
-				+= op->sym->auth.digest.length;
-	}
 
 	/* out-of-place operation (OOP) */
 	if (unlikely(op->sym->m_dst != NULL)) {
diff --git a/lib/librte_cryptodev/rte_crypto_sym.h b/lib/librte_cryptodev/rte_crypto_sym.h
index d3d38e4..d694723 100644
--- a/lib/librte_cryptodev/rte_crypto_sym.h
+++ b/lib/librte_cryptodev/rte_crypto_sym.h
@@ -541,8 +541,7 @@ struct rte_crypto_sym_op {
 
 		struct {
 			uint8_t *data;
-			/**< If this member of this structure is set this is a
-			 * pointer to the location where the digest result
+			/**< This points to the location where the digest result
 			 * should be inserted (in the case of digest generation)
 			 * or where the purported digest exists (in the case of
 			 * digest verification).
@@ -560,13 +559,6 @@ struct rte_crypto_sym_op {
 			 * @note
 			 * For GCM (@ref RTE_CRYPTO_AUTH_AES_GCM), for
 			 * "digest result" read "authentication tag T".
-			 *
-			 * If this member is not set the digest result is
-			 * understood to be in the destination buffer for
-			 * digest generation, and in the source buffer for
-			 * digest verification. The location of the digest
-			 * result in this case is immediately following the
-			 * region over which the digest is computed.
 			 */
 			phys_addr_t phys_addr;
 			/**< Physical address of digest */
-- 
2.5.0

Re: [PATCH] crypto: remove unused digest-appended feature

[Trahe, Fiona]

Note, this is a cover letter - see important clarification below.
Sorry, I missed adding the [PATCH 0/1] and [PATCH 1/1] to the email subject.

> -----Original Message-----
> From: Trahe, Fiona
> Sent: Thursday, November 17, 2016 5:33 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Griffin, John <john.griffin@intel.com>; Jastrzebski,
> MichalX K <michalx.k.jastrzebski@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>
> Subject: [PATCH] crypto: remove unused digest-appended feature
> 
> The cryptodev API had specified that if the digest address field was
> left empty on an authentication operation, then the PMD would assume
> the digest was appended to the source or destination data.
> This case was not handled at all by most PMDs and incorrectly handled
> by the QAT PMD.
> As no bugs were raised, it is assumed to be not needed, so this patch
> removes it, rather than add handling for the case on all PMDs.
> The digest can still be appended to the data, but its
> address must now be provided in the op.
> 
> I've added this cover letter to pre-empt the question which will arise
> in chained-mbuf case.
> (Chained-mbuf support will be added in next few weeks for 17.02)
> In a chained-mbuf case, if the digest is appended to the data, it could
> in theory end up split across the last 2 mbufs. Using the digest address
> field only works for the case where the digest is in a flat buffer. The
> digest-appended feature would have provided one way of handling this,
> pushing the responsibility down to the PMD to follow the chain to find
> the digest address and handle it whether in 1 or 2 mbufs.
> However, this is adding extra cycles on the data path in all PMDs
> to handle a case which may not arise in many applications.
> The alternative is to give the application the responsibility
> to make sure the digest is in a flat buffer.
> Specifically if an application knows cases of a digest spanning
> buffers can arise, then it can check for this case and memcpy the partial
> start of the digest from the second last buffer into the last buffer
> so it's no longer split.
> Does anyone see a problem with this?
> 
> 
> Fiona Trahe (1):
>   crypto: remove unused digest-appended feature
> 
>  drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
>  drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
>  lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
>  3 files changed, 4 insertions(+), 26 deletions(-)
> 
> --
> 2.5.0

Re: [PATCH] crypto: remove unused digest-appended feature

[John Griffin]

On 17/11/16 17:33, Fiona Trahe wrote:
> The cryptodev API had specified that if the digest address field was
> left empty on an authentication operation, then the PMD would assume
> the digest was appended to the source or destination data.
> This case was not handled at all by most PMDs and incorrectly handled
> by the QAT PMD.
> As no bugs were raised, it is assumed to be not needed, so this patch
> removes it, rather than add handling for the case on all PMDs.
> The digest can still be appended to the data, but its
> address must now be provided in the op.
>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
>   drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
>   drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
>   lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
>   3 files changed, 4 insertions(+), 26 deletions(-)
>

Acked-by: John Griffin <john.griffin@intel.com>

Re: [PATCH] crypto: remove unused digest-appended feature

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Griffin, John
> Sent: Friday, November 18, 2016 6:16 PM
> To: Trahe, Fiona; dev@dpdk.org
> Cc: De Lara Guarch, Pablo; Jastrzebski, MichalX K; Kusztal, ArkadiuszX
> Subject: Re: [PATCH] crypto: remove unused digest-appended feature
> 
> On 17/11/16 17:33, Fiona Trahe wrote:
> > The cryptodev API had specified that if the digest address field was
> > left empty on an authentication operation, then the PMD would assume
> > the digest was appended to the source or destination data.
> > This case was not handled at all by most PMDs and incorrectly handled
> > by the QAT PMD.
> > As no bugs were raised, it is assumed to be not needed, so this patch
> > removes it, rather than add handling for the case on all PMDs.
> > The digest can still be appended to the data, but its
> > address must now be provided in the op.
> >
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> >   drivers/crypto/qat/qat_adf/qat_algs_build_desc.c |  2 ++
> >   drivers/crypto/qat/qat_crypto.c                  | 18 +-----------------
> >   lib/librte_cryptodev/rte_crypto_sym.h            | 10 +---------
> >   3 files changed, 4 insertions(+), 26 deletions(-)
> >
> 
> Acked-by: John Griffin <john.griffin@intel.com>

Applied to dpdk-next-crypto.
Thanks,

Pablo