Re: [net-next PATCH v9 5/8] cn10k-ipsec: Add SA add/del support for outb ipsec crypto offload

[Paolo Abeni <pabeni@redhat.com>]

On 11/8/24 05:57, Bharat Bhushan wrote:
> This patch adds support to add and delete Security Association
> (SA) xfrm ops. Hardware maintains SA context in memory allocated
> by software. Each SA context is 128 byte aligned and size of
> each context is multiple of 128-byte. Add support for transport
> and tunnel ipsec mode, ESP protocol, aead aes-gcm-icv16, key size
> 128/192/256-bits with 32bit salt.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v8->v9:
>  - Previous versions were supporting only 64 SAs and a bitmap was
>    used for same. That limitation is removed from this version.
>  - Replaced netdev_err with NL_SET_ERR_MSG_MOD in state add flow
>    as per comment in previous version 
>  - Changes related to mutex lock removal 
> 
> v5->v6:
>  - In ethtool flow, so not cleanup cptlf if SA are installed and
>    call netdev_update_features() when all SA's are un-installed.
>  - Description and comment re-word to replace "inline ipsec"
>    with "ipsec crypto offload"
> 
> v3->v4:
>  - Added check for crypto offload (XFRM_DEV_OFFLOAD_CRYPTO)
>    Thanks "Leon Romanovsky" for pointing out
> 
> v2->v3:
>  - Removed memset to zero wherever possible
>   (comment from Kalesh Anakkur Purayil)
>  - Corrected error handling when setting SA for inbound
>    (comment from Kalesh Anakkur Purayil)
>  - Move "netdev->xfrmdev_ops = &cn10k_ipsec_xfrmdev_ops;" to this patch
>    This fix build error with W=1
> 
>  .../marvell/octeontx2/nic/cn10k_ipsec.c       | 415 ++++++++++++++++++
>  .../marvell/octeontx2/nic/cn10k_ipsec.h       | 113 +++++
>  2 files changed, 528 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> index e09ce42075c7..ccbcc5001431 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> @@ -375,6 +375,391 @@ static int cn10k_outb_cpt_clean(struct otx2_nic *pf)
>  	return ret;
>  }
>  
> +static void cn10k_cpt_inst_flush(struct otx2_nic *pf, struct cpt_inst_s *inst,
> +				 u64 size)
> +{
> +	struct otx2_lmt_info *lmt_info;
> +	u64 val = 0, tar_addr = 0;
> +
> +	lmt_info = per_cpu_ptr(pf->hw.lmt_info, smp_processor_id());
> +	/* FIXME: val[0:10] LMT_ID.
> +	 * [12:15] no of LMTST - 1 in the burst.
> +	 * [19:63] data size of each LMTST in the burst except first.
> +	 */
> +	val = (lmt_info->lmt_id & 0x7FF);
> +	/* Target address for LMTST flush tells HW how many 128bit
> +	 * words are present.
> +	 * tar_addr[6:4] size of first LMTST - 1 in units of 128b.
> +	 */
> +	tar_addr |= pf->ipsec.io_addr | (((size / 16) - 1) & 0x7) << 4;
> +	dma_wmb();
> +	memcpy((u64 *)lmt_info->lmt_addr, inst, size);
> +	cn10k_lmt_flush(val, tar_addr);
> +}
> +
> +static int cn10k_wait_for_cpt_respose(struct otx2_nic *pf,
> +				      struct cpt_res_s *res)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(10000);
> +
> +	do {
> +		if (time_after(jiffies, timeout)) {
> +			netdev_err(pf->netdev, "CPT response timeout\n");
> +			return -EBUSY;
> +		}
> +	} while (res->compcode == CN10K_CPT_COMP_E_NOTDONE);

Why a READ_ONCE() annotation is not needed around the 'res->compcode'
access?

Possibly more relevant: it looks like this code is busy polling the H/W
for at most 10s, is that correct? If so that timeout is way too high my
several order of magnitude. You should likely use usleep_range() or
sleep_interruptible()

[...]
> +static int cn10k_ipsec_validate_state(struct xfrm_state *x,
> +				      struct netlink_ext_ack *extack)
> +{
> +	if (x->props.aalgo != SADB_AALG_NONE) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Cannot offload authenticated xfrm states\n");

No '\n' at the end of extack messages.

(many cases below)

Thanks,

Paolo