RE: [PATCH 1/3] crypto/ipsec_mb: add GCM sgl support to aesni_mb

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi Ciara,


> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Thursday, April 7, 2022 11:31 AM
> To: dev@dpdk.org
> Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Ji, Kai <kai.ji@intel.com>;
> Power, Ciara <ciara.power@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: [PATCH 1/3] crypto/ipsec_mb: add GCM sgl support to aesni_mb
> 
> Add SGL support for GCM algorithm through JOB API.
> 
> This change supports IN-PLACE SGL, OOP SGL IN and LB OUT, and OOP SGL IN
> and SGL OUT.
> 
> Feature flags are not added, as the PMD does not yet support SGL for all other
> algorithms.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>  drivers/crypto/ipsec_mb/pmd_aesni_mb.c      | 144 +++++++++++++++++++-
>  drivers/crypto/ipsec_mb/pmd_aesni_mb_priv.h |   2 +
>  2 files changed, 142 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> index afa0b6e3a4..09a0cc5ace 100644
> --- a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> +++ b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c

...

> @@ -1410,6 +1509,43 @@ set_mb_job_params(IMB_JOB *job, struct
> ipsec_mb_qp *qp,
>  	/* Set user data to be crypto operation data struct */
>  	job->user_data = op;
> 
> +	if (sgl && aead) {

I'd say you don't need to check for aead here, right?
The only way to reach this point is if cipher.mode is GCM or CHACHA_POLY,
which guarantees that aead = 1 always. Is this correct?
> +		base_job = *job;

I don't see sgl_state = IMB_SGL_INIT being set.
I think the code here is relying that this will be set to IMB_SGL_INIT by default,
but it is risky to assume that, so better to set it.

> +		job = IMB_SUBMIT_JOB(mb_mgr);
> +		total_len = op->sym->aead.data.length;
> +
> +		src_sgl.m = m_src;
> +		src_sgl.offset = m_offset;
> +

...

>--- a/drivers/crypto/ipsec_mb/pmd_aesni_mb_priv.h
> +++ b/drivers/crypto/ipsec_mb/pmd_aesni_mb_priv.h
> @@ -946,6 +946,8 @@ struct aesni_mb_session {
>  	struct {
>  		/* * AAD data length */
>  		uint16_t aad_len;
> +
> +		struct gcm_context_data gcm_sgl_ctx;

I don't think it's necessary to have this context data in here,
you can declare it inside set_mb_job_params,
unless this causes a performance drop.

Thanks,
Pablo

>  	} aead;
>  } __rte_cache_aligned;
> 
> --
> 2.25.1