Re: [PATCH net-next v1 13/17] net/mlx5: Simplify IPsec capabilities logic

[Saeed Mahameed <saeedm@nvidia.com>]

On 19 Apr 13:13, Leon Romanovsky wrote:
>From: Leon Romanovsky <leonro@nvidia.com>
>
>Reduce number of hard-coded IPsec capabilities by making sure
>that mlx5_ipsec_device_caps() sets only supported bits.
>
>As part of this change, remove _accel_ notations from the names
>and prepare the code to IPsec full offload mode.
>

Can you explain why remove __accel__ notation ?
__accel__ notation and decoupling from other common netdev features is done
for modularity purpose, en_accel directories are separated so we can
implement complex/stateful accelerations while avoid contaminating/affecting
common data-path performance sensitives flows.

I think keeping __accel__ notations is a must here for the above reasons,
unless you have a more strong reason to remove it.. 

>Reviewed-by: Raed Salem <raeds@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> .../mellanox/mlx5/core/en_accel/ipsec.c       | 16 ++------------
> .../mellanox/mlx5/core/en_accel/ipsec.h       |  9 +++-----
> .../mlx5/core/en_accel/ipsec_offload.c        | 22 +++++++++----------
> 3 files changed, 16 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>index Clean IPsec FS add/delete rules28729b1cc6e6..be7650d2cfd3 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>@@ -215,7 +215,7 @@ static inline int mlx5e_xfrm_validate_state(struct xfrm_state *x)
> 		return -EINVAL;
> 	}
> 	if (x->props.flags & XFRM_STATE_ESN &&
>-	    !(mlx5_ipsec_device_caps(priv->mdev) & MLX5_ACCEL_IPSEC_CAP_ESN)) {
>+	    !(mlx5_ipsec_device_caps(priv->mdev) & MLX5_IPSEC_CAP_ESN)) {
> 		netdev_info(netdev, "Cannot offload ESN xfrm states\n");
> 		return -EINVAL;
> 	}
>@@ -262,11 +262,6 @@ static inline int mlx5e_xfrm_validate_state(struct xfrm_state *x)
> 		netdev_info(netdev, "Cannot offload xfrm states with geniv other than seqiv\n");
> 		return -EINVAL;
> 	}
>-	if (x->props.family == AF_INET6 &&
>-	    !(mlx5_ipsec_device_caps(priv->mdev) & MLX5_ACCEL_IPSEC_CAP_IPV6)) {
>-		netdev_info(netdev, "IPv6 xfrm state offload is not supported by this device\n");
>-		return -EINVAL;
>-	}
> 	return 0;
> }
>
>@@ -457,12 +452,6 @@ void mlx5e_ipsec_build_netdev(struct mlx5e_priv *priv)
> 	if (!mlx5_ipsec_device_caps(mdev))
> 		return;
>
>-	if (!(mlx5_ipsec_device_caps(mdev) & MLX5_ACCEL_IPSEC_CAP_ESP) ||
>-	    !MLX5_CAP_ETH(mdev, swp)) {
>-		mlx5_core_dbg(mdev, "mlx5e: ESP and SWP offload not supported\n");
>-		return;
>-	}
>-
> 	mlx5_core_info(mdev, "mlx5e: IPSec ESP acceleration enabled\n");
> 	netdev->xfrmdev_ops = &mlx5e_ipsec_xfrmdev_ops;
> 	netdev->features |= NETIF_F_HW_ESP;
>@@ -476,8 +465,7 @@ void mlx5e_ipsec_build_netdev(struct mlx5e_priv *priv)
> 	netdev->features |= NETIF_F_HW_ESP_TX_CSUM;
> 	netdev->hw_enc_features |= NETIF_F_HW_ESP_TX_CSUM;
>
>-	if (!(mlx5_ipsec_device_caps(mdev) & MLX5_ACCEL_IPSEC_CAP_LSO) ||
>-	    !MLX5_CAP_ETH(mdev, swp_lso)) {
>+	if (!MLX5_CAP_ETH(mdev, swp_lso)) {
> 		mlx5_core_dbg(mdev, "mlx5e: ESP LSO not supported\n");
> 		return;
> 	}
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>index af1467cbb7c7..97c55620089d 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>@@ -102,12 +102,9 @@ struct mlx5_accel_esp_xfrm_attrs {
> 	u8 is_ipv6;
> };
>
>-enum mlx5_accel_ipsec_cap {
>-	MLX5_ACCEL_IPSEC_CAP_DEVICE		= 1 << 0,
>-	MLX5_ACCEL_IPSEC_CAP_ESP		= 1 << 1,
>-	MLX5_ACCEL_IPSEC_CAP_IPV6		= 1 << 2,
>-	MLX5_ACCEL_IPSEC_CAP_LSO		= 1 << 3,
>-	MLX5_ACCEL_IPSEC_CAP_ESN		= 1 << 4,
>+enum mlx5_ipsec_cap {
>+	MLX5_IPSEC_CAP_CRYPTO		= 1 << 0,
>+	MLX5_IPSEC_CAP_ESN		= 1 << 1,
> };
>
> struct mlx5e_priv;
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_offload.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_offload.c
>index 817747d5229e..b44bce3f4ef1 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_offload.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_offload.c
>@@ -7,7 +7,7 @@
>
> u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev)
> {
>-	u32 caps;
>+	u32 caps = 0;
>
> 	if (!MLX5_CAP_GEN(mdev, ipsec_offload))
> 		return 0;
>@@ -19,23 +19,23 @@ u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev)
> 	    MLX5_HCA_CAP_GENERAL_OBJECT_TYPES_IPSEC))
> 		return 0;
>
>-	if (!MLX5_CAP_IPSEC(mdev, ipsec_crypto_offload) ||
>-	    !MLX5_CAP_ETH(mdev, insert_trailer))
>-		return 0;
>-
> 	if (!MLX5_CAP_FLOWTABLE_NIC_TX(mdev, ipsec_encrypt) ||
> 	    !MLX5_CAP_FLOWTABLE_NIC_RX(mdev, ipsec_decrypt))
> 		return 0;
>
>-	caps = MLX5_ACCEL_IPSEC_CAP_DEVICE | MLX5_ACCEL_IPSEC_CAP_IPV6 |
>-	       MLX5_ACCEL_IPSEC_CAP_LSO;
>+	if (!MLX5_CAP_IPSEC(mdev, ipsec_crypto_esp_aes_gcm_128_encrypt) ||
>+	    !MLX5_CAP_IPSEC(mdev, ipsec_crypto_esp_aes_gcm_128_decrypt))
>+		return 0;
>
>-	if (MLX5_CAP_IPSEC(mdev, ipsec_crypto_esp_aes_gcm_128_encrypt) &&
>-	    MLX5_CAP_IPSEC(mdev, ipsec_crypto_esp_aes_gcm_128_decrypt))
>-		caps |= MLX5_ACCEL_IPSEC_CAP_ESP;
>+	if (MLX5_CAP_IPSEC(mdev, ipsec_crypto_offload) &&
>+	    MLX5_CAP_ETH(mdev, insert_trailer) && MLX5_CAP_ETH(mdev, swp))
>+		caps |= MLX5_IPSEC_CAP_CRYPTO;
>+
>+	if (!caps)
>+		return 0;
>
> 	if (MLX5_CAP_IPSEC(mdev, ipsec_esn))
>-		caps |= MLX5_ACCEL_IPSEC_CAP_ESN;
>+		caps |= MLX5_IPSEC_CAP_ESN;
>
> 	/* We can accommodate up to 2^24 different IPsec objects
> 	 * because we use up to 24 bit in flow table metadata
>-- 
>2.35.1
>