Re: [dpdk-dev] [EXT] [PATCH] crypto/openssl: support SG for inplace buffers

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Anoob,
> Hi Akhil,
> > As per current support, scatter Gather is only supported for out of place
> input
> 
> [Anoob] s/scatter/Scatter
Ok
> 
> > and output buffers.
> > This patch add support for scatter gather for inplace buffers.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >  doc/guides/cryptodevs/features/openssl.ini |  1 +
> >  drivers/crypto/openssl/rte_openssl_pmd.c   | 82 +++++++++++++++++---
> --
> >  2 files changed, 64 insertions(+), 19 deletions(-)
> >
> > diff --git a/doc/guides/cryptodevs/features/openssl.ini
> > b/doc/guides/cryptodevs/features/openssl.ini
> > index 6ddca39e7..30ffb111d 100644
> > --- a/doc/guides/cryptodevs/features/openssl.ini
> > +++ b/doc/guides/cryptodevs/features/openssl.ini
> > @@ -6,6 +6,7 @@
> >  [Features]
> >  Symmetric crypto       = Y
> >  Sym operation chaining = Y
> > +In Place SGL           = Y
> >  OOP SGL In LB  Out     = Y
> >  OOP LB  In LB  Out     = Y
> >  Asymmetric crypto      = Y
> > diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c
> > b/drivers/crypto/openssl/rte_openssl_pmd.c
> > index 2f5552840..304ebc6ff 100644
> > --- a/drivers/crypto/openssl/rte_openssl_pmd.c
> > +++ b/drivers/crypto/openssl/rte_openssl_pmd.c
> > @@ -798,12 +798,12 @@ get_session(struct openssl_qp *qp, struct
> > rte_crypto_op *op)
> >   */
> >  static inline int
> >  process_openssl_encryption_update(struct rte_mbuf *mbuf_src, int
> offset,
> > -		uint8_t **dst, int srclen, EVP_CIPHER_CTX *ctx)
> > +		uint8_t **dst, int srclen, EVP_CIPHER_CTX *ctx, uint8_t
> inplace)
> >  {
> >  	struct rte_mbuf *m;
> >  	int dstlen;
> >  	int l, n = srclen;
> > -	uint8_t *src;
> > +	uint8_t *src, temp[128];
> 
> [Anoob] What is 128? Wouldn't a macro be better here?
Will change it to EVP_CIPHER_CTX_block_size . 
> 
> >
> >  	for (m = mbuf_src; m != NULL && offset > rte_pktmbuf_data_len(m);
> >  			m = m->next)
> > @@ -813,6 +813,8 @@ process_openssl_encryption_update(struct
> rte_mbuf
> > *mbuf_src, int offset,
> >  		return -1;
> >
> >  	src = rte_pktmbuf_mtod_offset(m, uint8_t *, offset);
> > +	if (inplace)
> > +		*dst = src;
> >
> >  	l = rte_pktmbuf_data_len(m) - offset;
> >  	if (srclen <= l) {
> > @@ -829,8 +831,24 @@ process_openssl_encryption_update(struct
> rte_mbuf
> > *mbuf_src, int offset,
> >  	n -= l;
> >
> >  	for (m = m->next; (m != NULL) && (n > 0); m = m->next) {
> > +		uint8_t diff = l - dstlen, rem;
> > +
> >  		src = rte_pktmbuf_mtod(m, uint8_t *);
> >  		l = rte_pktmbuf_data_len(m) < n ?
> rte_pktmbuf_data_len(m) : n;
> 
> [Anoob] Can you explain the logic of below lines? I was trying to understand
> the two 'rte_memcpy's and how dst is being used there.

As per the openssl API manual
" EVP_EncryptUpdate() encrypts inl bytes from the buffer in and writes the encrypted version to out. This function can be called multiple times to encrypt successive blocks of data. The amount of data written depends on the block alignment of the encrypted data: as a result the amount of data written may be anything from zero bytes to (inl + cipher_block_size - 1) so out should contain sufficient room. The actual number of bytes written is placed in outl. It also checks if in and out are partially overlapping, and if they are 0 is returned to indicate failure. "

So the problem with SG inline bufs is that if a particular SG is not block aligned, then it will write less data for that SG. So we need to call EVP_EncryptUpdate for the diff bytes of previous SG and take the remaining of the block size from the next SG.
Now we cannot store that in dst as it is,  because it is not a contiguous buffer. So will get the output of EVP_EncryptUpdate to temp and copy diff bytes in previous SG and the remaining bytes to next SG and update pointer accordingly.

> 
> > +		if (diff && inplace) {
> > +			rem = l > (EVP_CIPHER_CTX_block_size(ctx) - diff) ?
> > +				(EVP_CIPHER_CTX_block_size(ctx) - diff) : l;
> 
> [Anoob] Can't we use RTE_MIN here?
Yes will change that.
> 
> > +			if (EVP_EncryptUpdate(ctx, temp,
> > +						&dstlen, src, rem) <= 0)
> > +				return -1;
> > +			n -= rem;
> > +			rte_memcpy(*dst, temp, diff);
> > +			rte_memcpy(src, temp + diff, rem);
> > +			src += rem;
> > +			l -= rem;
> > +		}
> > +		if (inplace)
> > +			*dst = src;
> >  		if (EVP_EncryptUpdate(ctx, *dst, &dstlen, src, l) <= 0)
> >  			return -1;
> >  		*dst += dstlen;
> > @@ -842,12 +860,12 @@ process_openssl_encryption_update(struct
> rte_mbuf
> > *mbuf_src, int offset,
> >
> >  static inline int
> >  process_openssl_decryption_update(struct rte_mbuf *mbuf_src, int
> offset,
> > -		uint8_t **dst, int srclen, EVP_CIPHER_CTX *ctx)
> > +		uint8_t **dst, int srclen, EVP_CIPHER_CTX *ctx, uint8_t
> inplace)
> >  {
> >  	struct rte_mbuf *m;
> >  	int dstlen;
> >  	int l, n = srclen;
> > -	uint8_t *src;
> > +	uint8_t *src, temp[128];
> >
> >  	for (m = mbuf_src; m != NULL && offset > rte_pktmbuf_data_len(m);
> >  			m = m->next)
> > @@ -857,6 +875,8 @@ process_openssl_decryption_update(struct
> rte_mbuf
> > *mbuf_src, int offset,
> >  		return -1;
> >
> >  	src = rte_pktmbuf_mtod_offset(m, uint8_t *, offset);
> > +	if (inplace)
> > +		*dst = src;
> >
> >  	l = rte_pktmbuf_data_len(m) - offset;
> >  	if (srclen <= l) {
> > @@ -873,8 +893,24 @@ process_openssl_decryption_update(struct
> rte_mbuf
> > *mbuf_src, int offset,
> >  	n -= l;
> >
> >  	for (m = m->next; (m != NULL) && (n > 0); m = m->next) {
> > +		uint8_t diff = l - dstlen, rem;
> > +
> >  		src = rte_pktmbuf_mtod(m, uint8_t *);
> >  		l = rte_pktmbuf_data_len(m) < n ?
> rte_pktmbuf_data_len(m) : n;
> > +		if (diff && inplace) {
> > +			rem = l > (EVP_CIPHER_CTX_block_size(ctx) - diff) ?
> > +				(EVP_CIPHER_CTX_block_size(ctx) - diff) : l;
> > +			if (EVP_EncryptUpdate(ctx, temp,
> > +						&dstlen, src, rem) <= 0)
> 
> [Anoob] Shouldn't this be EVP_DecryptUpdate()?
Yes, good catch.
> 
> > +				return -1;
> > +			n -= rem;
> > +			rte_memcpy(*dst, temp, diff);
> > +			rte_memcpy(src, temp + diff, rem);
> > +			src += rem;
> > +			l -= rem;
> > +		}
> > +		if (inplace)
> > +			*dst = src;
> >  		if (EVP_DecryptUpdate(ctx, *dst, &dstlen, src, l) <= 0)
> >  			return -1;
> >  		*dst += dstlen;
> > @@ -887,7 +923,8 @@ process_openssl_decryption_update(struct
> rte_mbuf
> > *mbuf_src, int offset,
> >  /** Process standard openssl cipher encryption */  static int
> > process_openssl_cipher_encrypt(struct rte_mbuf *mbuf_src, uint8_t *dst,
> > -		int offset, uint8_t *iv, int srclen, EVP_CIPHER_CTX *ctx)
> > +		int offset, uint8_t *iv, int srclen, EVP_CIPHER_CTX *ctx,
> > +		uint8_t inplace)
> >  {
> >  	int totlen;
> >
> > @@ -897,7 +934,7 @@ process_openssl_cipher_encrypt(struct rte_mbuf
> > *mbuf_src, uint8_t *dst,
> >  	EVP_CIPHER_CTX_set_padding(ctx, 0);
> >
> >  	if (process_openssl_encryption_update(mbuf_src, offset, &dst,
> > -			srclen, ctx))
> > +			srclen, ctx, inplace))
> >  		goto process_cipher_encrypt_err;
> >
> >  	if (EVP_EncryptFinal_ex(ctx, dst, &totlen) <= 0) @@ -936,7 +973,8
> @@
> > process_openssl_cipher_bpi_encrypt(uint8_t *src, uint8_t *dst,
> >  /** Process standard openssl cipher decryption */  static int
> > process_openssl_cipher_decrypt(struct rte_mbuf *mbuf_src, uint8_t *dst,
> > -		int offset, uint8_t *iv, int srclen, EVP_CIPHER_CTX *ctx)
> > +		int offset, uint8_t *iv, int srclen, EVP_CIPHER_CTX *ctx,
> > +		uint8_t inplace)
> >  {
> >  	int totlen;
> >
> > @@ -946,7 +984,7 @@ process_openssl_cipher_decrypt(struct rte_mbuf
> > *mbuf_src, uint8_t *dst,
> >  	EVP_CIPHER_CTX_set_padding(ctx, 0);
> >
> >  	if (process_openssl_decryption_update(mbuf_src, offset, &dst,
> > -			srclen, ctx))
> > +			srclen, ctx, inplace))
> >  		goto process_cipher_decrypt_err;
> >
> >  	if (EVP_DecryptFinal_ex(ctx, dst, &totlen) <= 0) @@ -1033,7 +1071,7
> > @@ process_openssl_auth_encryption_gcm(struct rte_mbuf *mbuf_src,
> int
> > offset,
> >
> >  	if (srclen > 0)
> >  		if (process_openssl_encryption_update(mbuf_src, offset,
> &dst,
> > -				srclen, ctx))
> > +				srclen, ctx, 0))
> >  			goto process_auth_encryption_gcm_err;
> >
> >  	/* Workaround open ssl bug in version less then 1.0.1f */ @@ -
> 1078,7
> > +1116,7 @@ process_openssl_auth_encryption_ccm(struct rte_mbuf
> > *mbuf_src, int offset,
> >
> >  	if (srclen > 0)
> >  		if (process_openssl_encryption_update(mbuf_src, offset,
> &dst,
> > -				srclen, ctx))
> > +				srclen, ctx, 0))
> >  			goto process_auth_encryption_ccm_err;
> >
> >  	if (EVP_EncryptFinal_ex(ctx, dst, &len) <= 0) @@ -1115,7 +1153,7
> @@
> > process_openssl_auth_decryption_gcm(struct rte_mbuf *mbuf_src, int
> offset,
> >
> >  	if (srclen > 0)
> >  		if (process_openssl_decryption_update(mbuf_src, offset,
> &dst,
> > -				srclen, ctx))
> > +				srclen, ctx, 0))
> >  			goto process_auth_decryption_gcm_err;
> >
> >  	/* Workaround open ssl bug in version less then 1.0.1f */ @@ -
> 1161,7
> > +1199,7 @@ process_openssl_auth_decryption_ccm(struct rte_mbuf
> > *mbuf_src, int offset,
> >
> >  	if (srclen > 0)
> >  		if (process_openssl_decryption_update(mbuf_src, offset,
> &dst,
> > -				srclen, ctx))
> > +				srclen, ctx, 0))
> >  			return -EFAULT;
> >
> >  	return 0;
> > @@ -1372,12 +1410,15 @@ process_openssl_cipher_op  {
> >  	uint8_t *dst, *iv;
> >  	int srclen, status;
> > +	uint8_t inplace = (mbuf_src == mbuf_dst) ? 1 : 0;
> >
> >  	/*
> > -	 * Segmented destination buffer is not supported for
> > -	 * encryption/decryption
> > +	 * Segmented OOP destination buffer is not supported for
> encryption/
> > +	 * decryption. In case of des3ctr, even inplace segmented buffers are
> > +	 * not supported.
> >  	 */
> > -	if (!rte_pktmbuf_is_contiguous(mbuf_dst)) {
> > +	if (!rte_pktmbuf_is_contiguous(mbuf_dst) &&
> > +			(!inplace || sess->cipher.mode !=
> > OPENSSL_CIPHER_LIB)) {
> >  		op->status = RTE_CRYPTO_OP_STATUS_ERROR;
> >  		return;
> >  	}
> > @@ -1393,11 +1434,13 @@ process_openssl_cipher_op
> >  		if (sess->cipher.direction ==
> > RTE_CRYPTO_CIPHER_OP_ENCRYPT)
> >  			status = process_openssl_cipher_encrypt(mbuf_src,
> dst,
> >  					op->sym->cipher.data.offset, iv,
> > -					srclen, sess->cipher.ctx);
> > +					srclen, sess->cipher.ctx,
> > +					inplace);
> >  		else
> >  			status = process_openssl_cipher_decrypt(mbuf_src,
> dst,
> >  					op->sym->cipher.data.offset, iv,
> > -					srclen, sess->cipher.ctx);
> > +					srclen, sess->cipher.ctx,
> > +					inplace);
> >  	else
> >  		status = process_openssl_cipher_des3ctr(mbuf_src, dst,
> >  				op->sym->cipher.data.offset, iv,
> > @@ -1441,7 +1484,7 @@ process_openssl_docsis_bpi_op(struct
> rte_crypto_op
> > *op,
> >  			/* Encrypt with the block aligned stream with CBC
> > mode */
> >  			status = process_openssl_cipher_encrypt(mbuf_src,
> dst,
> >  					op->sym->cipher.data.offset, iv,
> > -					srclen, sess->cipher.ctx);
> > +					srclen, sess->cipher.ctx, 0);
> >  			if (last_block_len) {
> >  				/* Point at last block */
> >  				dst += srclen;
> > @@ -1491,7 +1534,7 @@ process_openssl_docsis_bpi_op(struct
> rte_crypto_op
> > *op,
> >  			/* Decrypt with CBC mode */
> >  			status |= process_openssl_cipher_decrypt(mbuf_src,
> > dst,
> >  					op->sym->cipher.data.offset, iv,
> > -					srclen, sess->cipher.ctx);
> > +					srclen, sess->cipher.ctx, 0);
> >  		}
> >  	}
> >
> > @@ -2121,6 +2164,7 @@ cryptodev_openssl_create(const char *name,
> >  	dev->feature_flags = RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO |
> >  			RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING |
> >  			RTE_CRYPTODEV_FF_CPU_AESNI |
> > +			RTE_CRYPTODEV_FF_IN_PLACE_SGL |
> >  			RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT |
> >  			RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT |
> >  			RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO |
> > --
> > 2.17.1