Re: [PATCH net-next v3 3/3] nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer

[Leon Romanovsky <leon@kernel.org>]

On Tue, Nov 01, 2022 at 12:02:48PM +0100, Simon Horman wrote:
> From: Huanhuan Wang <huanhuan.wang@corigine.com>
> 
> Xfrm callbacks are implemented to offload SA info into firmware
> by mailbox. It supports 16K SA info in total.
> 
> Expose ipsec offload feature to upper layer, this feature will
> signal the availability of the offload.
> 
> Based on initial work of Norm Bagley <norman.bagley@netronome.com>.
> 
> Signed-off-by: Huanhuan Wang <huanhuan.wang@corigine.com>
> Reviewed-by: Louis Peens <louis.peens@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
>  .../net/ethernet/netronome/nfp/crypto/ipsec.c | 532 +++++++++++++++++-
>  .../ethernet/netronome/nfp/nfp_net_common.c   |   6 +
>  .../net/ethernet/netronome/nfp/nfp_net_ctrl.h |   4 +-
>  3 files changed, 538 insertions(+), 4 deletions(-)

<...>

>  static int nfp_net_xfrm_add_state(struct xfrm_state *x)
>  {
> -	return -EOPNOTSUPP;
> +	struct net_device *netdev = x->xso.dev;
> +	struct nfp_ipsec_cfg_mssg msg = {0};

I think that I already wrote it {0} -> {};

> +	int i, key_len, trunc_len, err = 0;
> +	struct nfp_ipsec_cfg_add_sa *cfg;
> +	struct nfp_net *nn;
> +	unsigned int saidx;
> +	__be32 *p;

<...>

> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_MD5_96;
> +		else if (trunc_len == 128)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_MD5_128;
> +		else
> +			trunc_len = 0;

IMHO, this is better to write as switch-case in separate function.

> +		break;
> +	case SADB_AALG_SHA1HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA1_96;
> +		else if (trunc_len == 80)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA1_80;
> +		else
> +			trunc_len = 0;
> +		break;

Ditto.

> +	case SADB_X_AALG_SHA2_256HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA256_96;
> +		else if (trunc_len == 128)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA256_128;
> +		else
> +			trunc_len = 0;
> +		break;
> +	case SADB_X_AALG_SHA2_384HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA384_96;
> +		else if (trunc_len == 192)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA384_192;
> +		else
> +			trunc_len = 0;
> +		break;
> +	case SADB_X_AALG_SHA2_512HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA512_96;
> +		else if (trunc_len == 256)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA512_256;
> +		else
> +			trunc_len = 0;
> +		break;
> +	default:
> +		nn_err(nn, "Unsupported authentication algorithm\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!trunc_len) {
> +		nn_err(nn, "Unsupported authentication algorithm trunc length\n");
> +		return -EINVAL;
> +	}
> +
> +	if (x->aalg) {
> +		p = (__be32 *)x->aalg->alg_key;
> +		key_len = DIV_ROUND_UP(x->aalg->alg_key_len, BITS_PER_BYTE);
> +		if (key_len > sizeof(cfg->auth_key)) {
> +			nn_err(nn, "Insufficient space for offloaded auth key\n");
> +			return -EINVAL;
> +		}
> +		for (i = 0; i < key_len / sizeof(cfg->auth_key[0]) ; i++)
> +			cfg->auth_key[i] = ntohl(*p++);

I wonder if you can't declare p as u32 and use memcpy here instead of
u32->__be32->u32 conversions.

Thanks