diff mbox series

[net-next,v1,09/17] net/mlx5: Simplify HW context interfaces by using SA entry

Message ID 3ad7b80c6f58d938550dd3259c5eaaecd8833af4.1650363043.git.leonro@nvidia.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Extra IPsec cleanup | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count fail Series longer than 15 patches (and no cover letter)
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers warning 2 maintainers not CCed: linux-rdma@vger.kernel.org borisp@nvidia.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 436 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Leon Romanovsky April 19, 2022, 10:13 a.m. UTC
From: Leon Romanovsky <leonro@nvidia.com>

SA context logic used multiple structures to store same data
over and over. By simplifying the SA context interfaces, we
can remove extra structs.

Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec.c       |  50 ++---
 .../mellanox/mlx5/core/en_accel/ipsec.h       |  27 ++-
 .../mlx5/core/en_accel/ipsec_offload.c        | 182 ++++--------------
 3 files changed, 62 insertions(+), 197 deletions(-)

Comments

Saeed Mahameed April 22, 2022, 10:19 p.m. UTC | #1
On 19 Apr 13:13, Leon Romanovsky wrote:
>From: Leon Romanovsky <leonro@nvidia.com>
>
>SA context logic used multiple structures to store same data
>over and over. By simplifying the SA context interfaces, we
>can remove extra structs.
>
>Reviewed-by: Raed Salem <raeds@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> .../mellanox/mlx5/core/en_accel/ipsec.c       |  50 ++---
> .../mellanox/mlx5/core/en_accel/ipsec.h       |  27 ++-
> .../mlx5/core/en_accel/ipsec_offload.c        | 182 ++++--------------
> 3 files changed, 62 insertions(+), 197 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 0daf9350471f..537311a74bfb 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>@@ -63,9 +63,9 @@ struct xfrm_state *mlx5e_ipsec_sadb_rx_lookup(struct mlx5e_ipsec *ipsec,
> 	return ret;
> }
>
>-static int  mlx5e_ipsec_sadb_rx_add(struct mlx5e_ipsec_sa_entry *sa_entry,
>-				    unsigned int handle)
>+static int mlx5e_ipsec_sadb_rx_add(struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>+	unsigned int handle = sa_entry->ipsec_obj_id;
> 	struct mlx5e_ipsec *ipsec = sa_entry->ipsec;
> 	struct mlx5e_ipsec_sa_entry *_sa_entry;
> 	unsigned long flags;
>@@ -277,16 +277,14 @@ static void _update_xfrm_state(struct work_struct *work)
> 	struct mlx5e_ipsec_sa_entry *sa_entry = container_of(
> 		modify_work, struct mlx5e_ipsec_sa_entry, modify_work);
>
>-	mlx5_accel_esp_modify_xfrm(sa_entry->xfrm, &modify_work->attrs);
>+	mlx5_accel_esp_modify_xfrm(sa_entry, &modify_work->attrs);
> }
>
> static int mlx5e_xfrm_add_state(struct xfrm_state *x)
> {
> 	struct mlx5e_ipsec_sa_entry *sa_entry = NULL;
> 	struct net_device *netdev = x->xso.real_dev;
>-	struct mlx5_accel_esp_xfrm_attrs attrs;
> 	struct mlx5e_priv *priv;
>-	unsigned int sa_handle;
> 	int err;
>
> 	priv = netdev_priv(netdev);
>@@ -309,33 +307,20 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x)
> 	/* check esn */
> 	mlx5e_ipsec_update_esn_state(sa_entry);
>
>-	/* create xfrm */
>-	mlx5e_ipsec_build_accel_xfrm_attrs(sa_entry, &attrs);
>-	sa_entry->xfrm = mlx5_accel_esp_create_xfrm(priv->mdev, &attrs);
>-	if (IS_ERR(sa_entry->xfrm)) {
>-		err = PTR_ERR(sa_entry->xfrm);
>-		goto err_sa_entry;
>-	}
>-
>+	mlx5e_ipsec_build_accel_xfrm_attrs(sa_entry, &sa_entry->attrs);
> 	/* create hw context */
>-	sa_entry->hw_context =
>-			mlx5_accel_esp_create_hw_context(priv->mdev,
>-							 sa_entry->xfrm,
>-							 &sa_handle);
>-	if (IS_ERR(sa_entry->hw_context)) {
>-		err = PTR_ERR(sa_entry->hw_context);
>+	err = mlx5_ipsec_create_sa_ctx(sa_entry);
>+	if (err)
> 		goto err_xfrm;
>-	}
>
>-	sa_entry->ipsec_obj_id = sa_handle;
>-	err = mlx5e_accel_ipsec_fs_add_rule(priv, &sa_entry->xfrm->attrs,
>+	err = mlx5e_accel_ipsec_fs_add_rule(priv, &sa_entry->attrs,
> 					    sa_entry->ipsec_obj_id,
> 					    &sa_entry->ipsec_rule);
> 	if (err)
> 		goto err_hw_ctx;
>
> 	if (x->xso.flags & XFRM_OFFLOAD_INBOUND) {
>-		err = mlx5e_ipsec_sadb_rx_add(sa_entry, sa_handle);
>+		err = mlx5e_ipsec_sadb_rx_add(sa_entry);
> 		if (err)
> 			goto err_add_rule;
> 	} else {
>@@ -348,15 +333,12 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x)
> 	goto out;
>
> err_add_rule:
>-	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->xfrm->attrs,
>+	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->attrs,
> 				      &sa_entry->ipsec_rule);
> err_hw_ctx:
>-	mlx5_accel_esp_free_hw_context(priv->mdev, sa_entry->hw_context);
>+	mlx5_ipsec_free_sa_ctx(sa_entry);
> err_xfrm:
>-	mlx5_accel_esp_destroy_xfrm(sa_entry->xfrm);
>-err_sa_entry:
> 	kfree(sa_entry);
>-
> out:
> 	return err;
> }
>@@ -374,14 +356,10 @@ static void mlx5e_xfrm_free_state(struct xfrm_state *x)
> 	struct mlx5e_ipsec_sa_entry *sa_entry = to_ipsec_sa_entry(x);
> 	struct mlx5e_priv *priv = netdev_priv(x->xso.dev);
>
>-	if (sa_entry->hw_context) {
>-		cancel_work_sync(&sa_entry->modify_work.work);
>-		mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->xfrm->attrs,
>-					      &sa_entry->ipsec_rule);
>-		mlx5_accel_esp_free_hw_context(sa_entry->xfrm->mdev, sa_entry->hw_context);
>-		mlx5_accel_esp_destroy_xfrm(sa_entry->xfrm);
>-	}
>-
>+	cancel_work_sync(&sa_entry->modify_work.work);
>+	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->attrs,
>+				      &sa_entry->ipsec_rule);
>+	mlx5_ipsec_free_sa_ctx(sa_entry);
> 	kfree(sa_entry);
> }
>
>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 b438b0358c36..cdcb95f90623 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>@@ -102,11 +102,6 @@ struct mlx5_accel_esp_xfrm_attrs {
> 	u8 is_ipv6;
> };
>
>-struct mlx5_accel_esp_xfrm {
>-	struct mlx5_core_dev  *mdev;
>-	struct mlx5_accel_esp_xfrm_attrs attrs;
>-};
>-
> enum mlx5_accel_ipsec_cap {
> 	MLX5_ACCEL_IPSEC_CAP_DEVICE		= 1 << 0,
> 	MLX5_ACCEL_IPSEC_CAP_ESP		= 1 << 1,
>@@ -162,11 +157,11 @@ struct mlx5e_ipsec_sa_entry {
> 	unsigned int handle; /* Handle in SADB_RX */
> 	struct xfrm_state *x;
> 	struct mlx5e_ipsec *ipsec;
>-	struct mlx5_accel_esp_xfrm *xfrm;
>-	void *hw_context;
>+	struct mlx5_accel_esp_xfrm_attrs attrs;
> 	void (*set_iv_op)(struct sk_buff *skb, struct xfrm_state *x,
> 			  struct xfrm_offload *xo);
> 	u32 ipsec_obj_id;
>+	u32 enc_key_id;
> 	struct mlx5e_ipsec_rule ipsec_rule;
> 	struct mlx5e_ipsec_modify_state_work modify_work;
> };
>@@ -188,19 +183,19 @@ void mlx5e_accel_ipsec_fs_del_rule(struct mlx5e_priv *priv,
> 				   struct mlx5_accel_esp_xfrm_attrs *attrs,
> 				   struct mlx5e_ipsec_rule *ipsec_rule);
>
>-void *mlx5_accel_esp_create_hw_context(struct mlx5_core_dev *mdev,
>-				       struct mlx5_accel_esp_xfrm *xfrm,
>-				       u32 *sa_handle);
>-void mlx5_accel_esp_free_hw_context(struct mlx5_core_dev *mdev, void *context);
>+int mlx5_ipsec_create_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry);
>+void mlx5_ipsec_free_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry);
>
> u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev);
>
>-struct mlx5_accel_esp_xfrm *
>-mlx5_accel_esp_create_xfrm(struct mlx5_core_dev *mdev,
>-			   const struct mlx5_accel_esp_xfrm_attrs *attrs);
>-void mlx5_accel_esp_destroy_xfrm(struct mlx5_accel_esp_xfrm *xfrm);
>-void mlx5_accel_esp_modify_xfrm(struct mlx5_accel_esp_xfrm *xfrm,
>+void mlx5_accel_esp_modify_xfrm(struct mlx5e_ipsec_sa_entry *sa_entry,
> 				const struct mlx5_accel_esp_xfrm_attrs *attrs);
>+
>+static inline struct mlx5_core_dev *
>+mlx5e_ipsec_sa2dev(struct mlx5e_ipsec_sa_entry *sa_entry)
>+{
>+	return sa_entry->ipsec->mdev;
>+}
> #else
> static inline int mlx5e_ipsec_init(struct mlx5e_priv *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 a7bd31d10bd4..817747d5229e 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
>@@ -5,21 +5,6 @@
> #include "ipsec.h"
> #include "lib/mlx5.h"
>
>-struct mlx5_ipsec_sa_ctx {
>-	struct rhash_head hash;
>-	u32 enc_key_id;
>-	u32 ipsec_obj_id;
>-	/* hw ctx */
>-	struct mlx5_core_dev *dev;
>-	struct mlx5_ipsec_esp_xfrm *mxfrm;
>-};
>-
>-struct mlx5_ipsec_esp_xfrm {
>-	/* reference counter of SA ctx */
>-	struct mlx5_ipsec_sa_ctx *sa_ctx;
>-	struct mlx5_accel_esp_xfrm accel_xfrm;
>-};
>-
> u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev)
> {
> 	u32 caps;
>@@ -61,43 +46,11 @@ u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev)
> }
> EXPORT_SYMBOL_GPL(mlx5_ipsec_device_caps);
>
>-struct mlx5_accel_esp_xfrm *
>-mlx5_accel_esp_create_xfrm(struct mlx5_core_dev *mdev,
>-			   const struct mlx5_accel_esp_xfrm_attrs *attrs)
>-{
>-	struct mlx5_ipsec_esp_xfrm *mxfrm;
>-
>-	mxfrm = kzalloc(sizeof(*mxfrm), GFP_KERNEL);
>-	if (!mxfrm)
>-		return ERR_PTR(-ENOMEM);
>-
>-	memcpy(&mxfrm->accel_xfrm.attrs, attrs,
>-	       sizeof(mxfrm->accel_xfrm.attrs));
>-
>-	mxfrm->accel_xfrm.mdev = mdev;
>-	return &mxfrm->accel_xfrm;
>-}
>-
>-void mlx5_accel_esp_destroy_xfrm(struct mlx5_accel_esp_xfrm *xfrm)
>+static int mlx5_create_ipsec_obj(struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>-	struct mlx5_ipsec_esp_xfrm *mxfrm = container_of(xfrm, struct mlx5_ipsec_esp_xfrm,
>-							 accel_xfrm);
>-
>-	kfree(mxfrm);
>-}
>-
>-struct mlx5_ipsec_obj_attrs {
>-	const struct aes_gcm_keymat *aes_gcm;
>-	u32 accel_flags;
>-	u32 esn_msb;
>-	u32 enc_key_id;
>-};
>-
>-static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
>-				 struct mlx5_ipsec_obj_attrs *attrs,
>-				 u32 *ipsec_id)

I don't see the point of this change, the function used to receive two
primitives, now it receives a god object, just to grab the two primitives,
this breaks the bottom up design, and contaminates the code with the
sa_entry container, that only should be visible by high-level ipsec module and
the SA DB, all service and low level functions should remain as
primitive and simple as possible to avoid future abuse and reduce the scope
and visibility of god objects. The effect of this change is more severe in
the next patch.

Even within the same file, i still recommend a monotonic bottom up
design and keep the complex objects usage to as few hight level functions
as possible.

>-{
>-	const struct aes_gcm_keymat *aes_gcm = attrs->aes_gcm;
>+	struct mlx5_accel_esp_xfrm_attrs *attrs = &sa_entry->attrs;
>+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
>+	struct aes_gcm_keymat *aes_gcm = &attrs->keymat.aes_gcm;
> 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)];
> 	u32 in[MLX5_ST_SZ_DW(create_ipsec_obj_in)] = {};
> 	void *obj, *salt_p, *salt_iv_p;
>@@ -128,14 +81,14 @@ static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
> 	salt_iv_p = MLX5_ADDR_OF(ipsec_obj, obj, implicit_iv);
> 	memcpy(salt_iv_p, &aes_gcm->seq_iv, sizeof(aes_gcm->seq_iv));
> 	/* esn */
>-	if (attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED) {
>+	if (attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED) {
> 		MLX5_SET(ipsec_obj, obj, esn_en, 1);
>-		MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn_msb);
>-		if (attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
>+		MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn);
>+		if (attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
> 			MLX5_SET(ipsec_obj, obj, esn_overlap, 1);
> 	}
>
>-	MLX5_SET(ipsec_obj, obj, dekn, attrs->enc_key_id);
>+	MLX5_SET(ipsec_obj, obj, dekn, sa_entry->enc_key_id);
>
> 	/* general object fields set */
> 	MLX5_SET(general_obj_in_cmd_hdr, in, opcode,
>@@ -145,13 +98,15 @@ static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
>
> 	err = mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
> 	if (!err)
>-		*ipsec_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
>+		sa_entry->ipsec_obj_id =
>+			MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
>
> 	return err;
> }
>
>-static void mlx5_destroy_ipsec_obj(struct mlx5_core_dev *mdev, u32 ipsec_id)
>+static void mlx5_destroy_ipsec_obj(struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
> 	u32 in[MLX5_ST_SZ_DW(general_obj_in_cmd_hdr)] = {};
> 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)];
>
>@@ -159,79 +114,52 @@ static void mlx5_destroy_ipsec_obj(struct mlx5_core_dev *mdev, u32 ipsec_id)
> 		 MLX5_CMD_OP_DESTROY_GENERAL_OBJECT);
> 	MLX5_SET(general_obj_in_cmd_hdr, in, obj_type,
> 		 MLX5_GENERAL_OBJECT_TYPES_IPSEC);
>-	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, ipsec_id);
>+	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, sa_entry->ipsec_obj_id);
>
> 	mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
> }
>
>-static void *mlx5_ipsec_offload_create_sa_ctx(struct mlx5_core_dev *mdev,
>-					      struct mlx5_accel_esp_xfrm *accel_xfrm,
>-					      const __be32 saddr[4], const __be32 daddr[4],
>-					      const __be32 spi, bool is_ipv6, u32 *hw_handle)
>+int mlx5_ipsec_create_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>-	struct mlx5_accel_esp_xfrm_attrs *xfrm_attrs = &accel_xfrm->attrs;
>-	struct aes_gcm_keymat *aes_gcm = &xfrm_attrs->keymat.aes_gcm;
>-	struct mlx5_ipsec_obj_attrs ipsec_attrs = {};
>-	struct mlx5_ipsec_esp_xfrm *mxfrm;
>-	struct mlx5_ipsec_sa_ctx *sa_ctx;
>+	struct aes_gcm_keymat *aes_gcm = &sa_entry->attrs.keymat.aes_gcm;
>+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
> 	int err;
>
>-	/* alloc SA context */
>-	sa_ctx = kzalloc(sizeof(*sa_ctx), GFP_KERNEL);
>-	if (!sa_ctx)
>-		return ERR_PTR(-ENOMEM);
>-
>-	sa_ctx->dev = mdev;
>-
>-	mxfrm = container_of(accel_xfrm, struct mlx5_ipsec_esp_xfrm, accel_xfrm);
>-	sa_ctx->mxfrm = mxfrm;
>-
> 	/* key */
> 	err = mlx5_create_encryption_key(mdev, aes_gcm->aes_key,
> 					 aes_gcm->key_len / BITS_PER_BYTE,
> 					 MLX5_ACCEL_OBJ_IPSEC_KEY,
>-					 &sa_ctx->enc_key_id);
>+					 &sa_entry->enc_key_id);
> 	if (err) {
> 		mlx5_core_dbg(mdev, "Failed to create encryption key (err = %d)\n", err);
>-		goto err_sa_ctx;
>+		return err;
> 	}
>
>-	ipsec_attrs.aes_gcm = aes_gcm;
>-	ipsec_attrs.accel_flags = accel_xfrm->attrs.flags;
>-	ipsec_attrs.esn_msb = accel_xfrm->attrs.esn;
>-	ipsec_attrs.enc_key_id = sa_ctx->enc_key_id;
>-	err = mlx5_create_ipsec_obj(mdev, &ipsec_attrs,
>-				    &sa_ctx->ipsec_obj_id);
>+	err = mlx5_create_ipsec_obj(sa_entry);
> 	if (err) {
> 		mlx5_core_dbg(mdev, "Failed to create IPsec object (err = %d)\n", err);
> 		goto err_enc_key;
> 	}
>
>-	*hw_handle = sa_ctx->ipsec_obj_id;
>-	mxfrm->sa_ctx = sa_ctx;
>-
>-	return sa_ctx;
>+	return 0;
>
> err_enc_key:
>-	mlx5_destroy_encryption_key(mdev, sa_ctx->enc_key_id);
>-err_sa_ctx:
>-	kfree(sa_ctx);
>-	return ERR_PTR(err);
>+	mlx5_destroy_encryption_key(mdev, sa_entry->enc_key_id);
>+	return err;
> }
>
>-static void mlx5_ipsec_offload_delete_sa_ctx(void *context)
>+void mlx5_ipsec_free_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>-	struct mlx5_ipsec_sa_ctx *sa_ctx = (struct mlx5_ipsec_sa_ctx *)context;
>+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
>
>-	mlx5_destroy_ipsec_obj(sa_ctx->dev, sa_ctx->ipsec_obj_id);
>-	mlx5_destroy_encryption_key(sa_ctx->dev, sa_ctx->enc_key_id);
>-	kfree(sa_ctx);
>+	mlx5_destroy_ipsec_obj(sa_entry);
>+	mlx5_destroy_encryption_key(mdev, sa_entry->enc_key_id);
> }
>
>-static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
>-				 struct mlx5_ipsec_obj_attrs *attrs,
>-				 u32 ipsec_id)
>+static int mlx5_modify_ipsec_obj(struct mlx5e_ipsec_sa_entry *sa_entry,
>+				 const struct mlx5_accel_esp_xfrm_attrs *attrs)
> {
>+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
> 	u32 in[MLX5_ST_SZ_DW(modify_ipsec_obj_in)] = {};
> 	u32 out[MLX5_ST_SZ_DW(query_ipsec_obj_out)];
> 	u64 modify_field_select = 0;
>@@ -239,7 +167,7 @@ static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
> 	void *obj;
> 	int err;
>
>-	if (!(attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED))
>+	if (!(attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED))
> 		return 0;
>
> 	general_obj_types = MLX5_CAP_GEN_64(mdev, general_obj_types);
>@@ -249,11 +177,11 @@ static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
> 	/* general object fields set */
> 	MLX5_SET(general_obj_in_cmd_hdr, in, opcode, MLX5_CMD_OP_QUERY_GENERAL_OBJECT);
> 	MLX5_SET(general_obj_in_cmd_hdr, in, obj_type, MLX5_GENERAL_OBJECT_TYPES_IPSEC);
>-	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, ipsec_id);
>+	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, sa_entry->ipsec_obj_id);
> 	err = mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
> 	if (err) {
> 		mlx5_core_err(mdev, "Query IPsec object failed (Object id %d), err = %d\n",
>-			      ipsec_id, err);
>+			      sa_entry->ipsec_obj_id, err);
> 		return err;
> 	}
>
>@@ -266,8 +194,8 @@ static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
> 		return -EOPNOTSUPP;
>
> 	obj = MLX5_ADDR_OF(modify_ipsec_obj_in, in, ipsec_object);
>-	MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn_msb);
>-	if (attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
>+	MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn);
>+	if (attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
> 		MLX5_SET(ipsec_obj, obj, esn_overlap, 1);
>
> 	/* general object fields set */
>@@ -276,50 +204,14 @@ static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
> 	return mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
> }
>
>-void mlx5_accel_esp_modify_xfrm(struct mlx5_accel_esp_xfrm *xfrm,
>+void mlx5_accel_esp_modify_xfrm(struct mlx5e_ipsec_sa_entry *sa_entry,
> 				const struct mlx5_accel_esp_xfrm_attrs *attrs)
> {
>-	struct mlx5_ipsec_obj_attrs ipsec_attrs = {};
>-	struct mlx5_core_dev *mdev = xfrm->mdev;
>-	struct mlx5_ipsec_esp_xfrm *mxfrm;
> 	int err;
>
>-	mxfrm = container_of(xfrm, struct mlx5_ipsec_esp_xfrm, accel_xfrm);
>-
>-	/* need to add find and replace in ipsec_rhash_sa the sa_ctx */
>-	/* modify device with new hw_sa */
>-	ipsec_attrs.accel_flags = attrs->flags;
>-	ipsec_attrs.esn_msb = attrs->esn;
>-	err = mlx5_modify_ipsec_obj(mdev,
>-				    &ipsec_attrs,
>-				    mxfrm->sa_ctx->ipsec_obj_id);
>-
>+	err = mlx5_modify_ipsec_obj(sa_entry, attrs);
> 	if (err)
> 		return;
>
>-	memcpy(&xfrm->attrs, attrs, sizeof(xfrm->attrs));
>-}
>-
>-void *mlx5_accel_esp_create_hw_context(struct mlx5_core_dev *mdev,
>-				       struct mlx5_accel_esp_xfrm *xfrm,
>-				       u32 *sa_handle)
>-{
>-	__be32 saddr[4] = {}, daddr[4] = {};
>-
>-	if (!xfrm->attrs.is_ipv6) {
>-		saddr[3] = xfrm->attrs.saddr.a4;
>-		daddr[3] = xfrm->attrs.daddr.a4;
>-	} else {
>-		memcpy(saddr, xfrm->attrs.saddr.a6, sizeof(saddr));
>-		memcpy(daddr, xfrm->attrs.daddr.a6, sizeof(daddr));
>-	}
>-
>-	return mlx5_ipsec_offload_create_sa_ctx(mdev, xfrm, saddr, daddr,
>-						xfrm->attrs.spi,
>-						xfrm->attrs.is_ipv6, sa_handle);
>-}
>-
>-void mlx5_accel_esp_free_hw_context(struct mlx5_core_dev *mdev, void *context)
>-{
>-	mlx5_ipsec_offload_delete_sa_ctx(context);
>+	memcpy(&sa_entry->attrs, attrs, sizeof(sa_entry->attrs));
> }
>-- 
>2.35.1
>
Leon Romanovsky May 1, 2022, 8:56 a.m. UTC | #2
On Fri, Apr 22, 2022 at 03:19:35PM -0700, Saeed Mahameed wrote:
> On 19 Apr 13:13, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > SA context logic used multiple structures to store same data
> > over and over. By simplifying the SA context interfaces, we
> > can remove extra structs.
> > 
> > Reviewed-by: Raed Salem <raeds@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > .../mellanox/mlx5/core/en_accel/ipsec.c       |  50 ++---
> > .../mellanox/mlx5/core/en_accel/ipsec.h       |  27 ++-
> > .../mlx5/core/en_accel/ipsec_offload.c        | 182 ++++--------------
> > 3 files changed, 62 insertions(+), 197 deletions(-)

<...>

> > -static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
> > -				 struct mlx5_ipsec_obj_attrs *attrs,
> > -				 u32 *ipsec_id)
> 
> I don't see the point of this change, the function used to receive two
> primitives, now it receives a god object, just to grab the two primitives,
> this breaks the bottom up design, and contaminates the code with the
> sa_entry container, that only should be visible by high-level ipsec module and
> the SA DB, all service and low level functions should remain as
> primitive and simple as possible to avoid future abuse and reduce the scope
> and visibility of god objects. The effect of this change is more severe in
> the next patch.
> 
> Even within the same file, i still recommend a monotonic bottom up
> design and keep the complex objects usage to as few hight level functions
> as possible.

Like you said: same file, same data copied in and out - it is not bottom
up design for me.

Thanks
diff mbox series

Patch

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 0daf9350471f..537311a74bfb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -63,9 +63,9 @@  struct xfrm_state *mlx5e_ipsec_sadb_rx_lookup(struct mlx5e_ipsec *ipsec,
 	return ret;
 }
 
-static int  mlx5e_ipsec_sadb_rx_add(struct mlx5e_ipsec_sa_entry *sa_entry,
-				    unsigned int handle)
+static int mlx5e_ipsec_sadb_rx_add(struct mlx5e_ipsec_sa_entry *sa_entry)
 {
+	unsigned int handle = sa_entry->ipsec_obj_id;
 	struct mlx5e_ipsec *ipsec = sa_entry->ipsec;
 	struct mlx5e_ipsec_sa_entry *_sa_entry;
 	unsigned long flags;
@@ -277,16 +277,14 @@  static void _update_xfrm_state(struct work_struct *work)
 	struct mlx5e_ipsec_sa_entry *sa_entry = container_of(
 		modify_work, struct mlx5e_ipsec_sa_entry, modify_work);
 
-	mlx5_accel_esp_modify_xfrm(sa_entry->xfrm, &modify_work->attrs);
+	mlx5_accel_esp_modify_xfrm(sa_entry, &modify_work->attrs);
 }
 
 static int mlx5e_xfrm_add_state(struct xfrm_state *x)
 {
 	struct mlx5e_ipsec_sa_entry *sa_entry = NULL;
 	struct net_device *netdev = x->xso.real_dev;
-	struct mlx5_accel_esp_xfrm_attrs attrs;
 	struct mlx5e_priv *priv;
-	unsigned int sa_handle;
 	int err;
 
 	priv = netdev_priv(netdev);
@@ -309,33 +307,20 @@  static int mlx5e_xfrm_add_state(struct xfrm_state *x)
 	/* check esn */
 	mlx5e_ipsec_update_esn_state(sa_entry);
 
-	/* create xfrm */
-	mlx5e_ipsec_build_accel_xfrm_attrs(sa_entry, &attrs);
-	sa_entry->xfrm = mlx5_accel_esp_create_xfrm(priv->mdev, &attrs);
-	if (IS_ERR(sa_entry->xfrm)) {
-		err = PTR_ERR(sa_entry->xfrm);
-		goto err_sa_entry;
-	}
-
+	mlx5e_ipsec_build_accel_xfrm_attrs(sa_entry, &sa_entry->attrs);
 	/* create hw context */
-	sa_entry->hw_context =
-			mlx5_accel_esp_create_hw_context(priv->mdev,
-							 sa_entry->xfrm,
-							 &sa_handle);
-	if (IS_ERR(sa_entry->hw_context)) {
-		err = PTR_ERR(sa_entry->hw_context);
+	err = mlx5_ipsec_create_sa_ctx(sa_entry);
+	if (err)
 		goto err_xfrm;
-	}
 
-	sa_entry->ipsec_obj_id = sa_handle;
-	err = mlx5e_accel_ipsec_fs_add_rule(priv, &sa_entry->xfrm->attrs,
+	err = mlx5e_accel_ipsec_fs_add_rule(priv, &sa_entry->attrs,
 					    sa_entry->ipsec_obj_id,
 					    &sa_entry->ipsec_rule);
 	if (err)
 		goto err_hw_ctx;
 
 	if (x->xso.flags & XFRM_OFFLOAD_INBOUND) {
-		err = mlx5e_ipsec_sadb_rx_add(sa_entry, sa_handle);
+		err = mlx5e_ipsec_sadb_rx_add(sa_entry);
 		if (err)
 			goto err_add_rule;
 	} else {
@@ -348,15 +333,12 @@  static int mlx5e_xfrm_add_state(struct xfrm_state *x)
 	goto out;
 
 err_add_rule:
-	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->xfrm->attrs,
+	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->attrs,
 				      &sa_entry->ipsec_rule);
 err_hw_ctx:
-	mlx5_accel_esp_free_hw_context(priv->mdev, sa_entry->hw_context);
+	mlx5_ipsec_free_sa_ctx(sa_entry);
 err_xfrm:
-	mlx5_accel_esp_destroy_xfrm(sa_entry->xfrm);
-err_sa_entry:
 	kfree(sa_entry);
-
 out:
 	return err;
 }
@@ -374,14 +356,10 @@  static void mlx5e_xfrm_free_state(struct xfrm_state *x)
 	struct mlx5e_ipsec_sa_entry *sa_entry = to_ipsec_sa_entry(x);
 	struct mlx5e_priv *priv = netdev_priv(x->xso.dev);
 
-	if (sa_entry->hw_context) {
-		cancel_work_sync(&sa_entry->modify_work.work);
-		mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->xfrm->attrs,
-					      &sa_entry->ipsec_rule);
-		mlx5_accel_esp_free_hw_context(sa_entry->xfrm->mdev, sa_entry->hw_context);
-		mlx5_accel_esp_destroy_xfrm(sa_entry->xfrm);
-	}
-
+	cancel_work_sync(&sa_entry->modify_work.work);
+	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->attrs,
+				      &sa_entry->ipsec_rule);
+	mlx5_ipsec_free_sa_ctx(sa_entry);
 	kfree(sa_entry);
 }
 
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 b438b0358c36..cdcb95f90623 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
@@ -102,11 +102,6 @@  struct mlx5_accel_esp_xfrm_attrs {
 	u8 is_ipv6;
 };
 
-struct mlx5_accel_esp_xfrm {
-	struct mlx5_core_dev  *mdev;
-	struct mlx5_accel_esp_xfrm_attrs attrs;
-};
-
 enum mlx5_accel_ipsec_cap {
 	MLX5_ACCEL_IPSEC_CAP_DEVICE		= 1 << 0,
 	MLX5_ACCEL_IPSEC_CAP_ESP		= 1 << 1,
@@ -162,11 +157,11 @@  struct mlx5e_ipsec_sa_entry {
 	unsigned int handle; /* Handle in SADB_RX */
 	struct xfrm_state *x;
 	struct mlx5e_ipsec *ipsec;
-	struct mlx5_accel_esp_xfrm *xfrm;
-	void *hw_context;
+	struct mlx5_accel_esp_xfrm_attrs attrs;
 	void (*set_iv_op)(struct sk_buff *skb, struct xfrm_state *x,
 			  struct xfrm_offload *xo);
 	u32 ipsec_obj_id;
+	u32 enc_key_id;
 	struct mlx5e_ipsec_rule ipsec_rule;
 	struct mlx5e_ipsec_modify_state_work modify_work;
 };
@@ -188,19 +183,19 @@  void mlx5e_accel_ipsec_fs_del_rule(struct mlx5e_priv *priv,
 				   struct mlx5_accel_esp_xfrm_attrs *attrs,
 				   struct mlx5e_ipsec_rule *ipsec_rule);
 
-void *mlx5_accel_esp_create_hw_context(struct mlx5_core_dev *mdev,
-				       struct mlx5_accel_esp_xfrm *xfrm,
-				       u32 *sa_handle);
-void mlx5_accel_esp_free_hw_context(struct mlx5_core_dev *mdev, void *context);
+int mlx5_ipsec_create_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry);
+void mlx5_ipsec_free_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry);
 
 u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev);
 
-struct mlx5_accel_esp_xfrm *
-mlx5_accel_esp_create_xfrm(struct mlx5_core_dev *mdev,
-			   const struct mlx5_accel_esp_xfrm_attrs *attrs);
-void mlx5_accel_esp_destroy_xfrm(struct mlx5_accel_esp_xfrm *xfrm);
-void mlx5_accel_esp_modify_xfrm(struct mlx5_accel_esp_xfrm *xfrm,
+void mlx5_accel_esp_modify_xfrm(struct mlx5e_ipsec_sa_entry *sa_entry,
 				const struct mlx5_accel_esp_xfrm_attrs *attrs);
+
+static inline struct mlx5_core_dev *
+mlx5e_ipsec_sa2dev(struct mlx5e_ipsec_sa_entry *sa_entry)
+{
+	return sa_entry->ipsec->mdev;
+}
 #else
 static inline int mlx5e_ipsec_init(struct mlx5e_priv *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 a7bd31d10bd4..817747d5229e 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
@@ -5,21 +5,6 @@ 
 #include "ipsec.h"
 #include "lib/mlx5.h"
 
-struct mlx5_ipsec_sa_ctx {
-	struct rhash_head hash;
-	u32 enc_key_id;
-	u32 ipsec_obj_id;
-	/* hw ctx */
-	struct mlx5_core_dev *dev;
-	struct mlx5_ipsec_esp_xfrm *mxfrm;
-};
-
-struct mlx5_ipsec_esp_xfrm {
-	/* reference counter of SA ctx */
-	struct mlx5_ipsec_sa_ctx *sa_ctx;
-	struct mlx5_accel_esp_xfrm accel_xfrm;
-};
-
 u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev)
 {
 	u32 caps;
@@ -61,43 +46,11 @@  u32 mlx5_ipsec_device_caps(struct mlx5_core_dev *mdev)
 }
 EXPORT_SYMBOL_GPL(mlx5_ipsec_device_caps);
 
-struct mlx5_accel_esp_xfrm *
-mlx5_accel_esp_create_xfrm(struct mlx5_core_dev *mdev,
-			   const struct mlx5_accel_esp_xfrm_attrs *attrs)
-{
-	struct mlx5_ipsec_esp_xfrm *mxfrm;
-
-	mxfrm = kzalloc(sizeof(*mxfrm), GFP_KERNEL);
-	if (!mxfrm)
-		return ERR_PTR(-ENOMEM);
-
-	memcpy(&mxfrm->accel_xfrm.attrs, attrs,
-	       sizeof(mxfrm->accel_xfrm.attrs));
-
-	mxfrm->accel_xfrm.mdev = mdev;
-	return &mxfrm->accel_xfrm;
-}
-
-void mlx5_accel_esp_destroy_xfrm(struct mlx5_accel_esp_xfrm *xfrm)
+static int mlx5_create_ipsec_obj(struct mlx5e_ipsec_sa_entry *sa_entry)
 {
-	struct mlx5_ipsec_esp_xfrm *mxfrm = container_of(xfrm, struct mlx5_ipsec_esp_xfrm,
-							 accel_xfrm);
-
-	kfree(mxfrm);
-}
-
-struct mlx5_ipsec_obj_attrs {
-	const struct aes_gcm_keymat *aes_gcm;
-	u32 accel_flags;
-	u32 esn_msb;
-	u32 enc_key_id;
-};
-
-static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
-				 struct mlx5_ipsec_obj_attrs *attrs,
-				 u32 *ipsec_id)
-{
-	const struct aes_gcm_keymat *aes_gcm = attrs->aes_gcm;
+	struct mlx5_accel_esp_xfrm_attrs *attrs = &sa_entry->attrs;
+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
+	struct aes_gcm_keymat *aes_gcm = &attrs->keymat.aes_gcm;
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)];
 	u32 in[MLX5_ST_SZ_DW(create_ipsec_obj_in)] = {};
 	void *obj, *salt_p, *salt_iv_p;
@@ -128,14 +81,14 @@  static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
 	salt_iv_p = MLX5_ADDR_OF(ipsec_obj, obj, implicit_iv);
 	memcpy(salt_iv_p, &aes_gcm->seq_iv, sizeof(aes_gcm->seq_iv));
 	/* esn */
-	if (attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED) {
+	if (attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED) {
 		MLX5_SET(ipsec_obj, obj, esn_en, 1);
-		MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn_msb);
-		if (attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
+		MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn);
+		if (attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
 			MLX5_SET(ipsec_obj, obj, esn_overlap, 1);
 	}
 
-	MLX5_SET(ipsec_obj, obj, dekn, attrs->enc_key_id);
+	MLX5_SET(ipsec_obj, obj, dekn, sa_entry->enc_key_id);
 
 	/* general object fields set */
 	MLX5_SET(general_obj_in_cmd_hdr, in, opcode,
@@ -145,13 +98,15 @@  static int mlx5_create_ipsec_obj(struct mlx5_core_dev *mdev,
 
 	err = mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
 	if (!err)
-		*ipsec_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
+		sa_entry->ipsec_obj_id =
+			MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
 
 	return err;
 }
 
-static void mlx5_destroy_ipsec_obj(struct mlx5_core_dev *mdev, u32 ipsec_id)
+static void mlx5_destroy_ipsec_obj(struct mlx5e_ipsec_sa_entry *sa_entry)
 {
+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
 	u32 in[MLX5_ST_SZ_DW(general_obj_in_cmd_hdr)] = {};
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)];
 
@@ -159,79 +114,52 @@  static void mlx5_destroy_ipsec_obj(struct mlx5_core_dev *mdev, u32 ipsec_id)
 		 MLX5_CMD_OP_DESTROY_GENERAL_OBJECT);
 	MLX5_SET(general_obj_in_cmd_hdr, in, obj_type,
 		 MLX5_GENERAL_OBJECT_TYPES_IPSEC);
-	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, ipsec_id);
+	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, sa_entry->ipsec_obj_id);
 
 	mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
 }
 
-static void *mlx5_ipsec_offload_create_sa_ctx(struct mlx5_core_dev *mdev,
-					      struct mlx5_accel_esp_xfrm *accel_xfrm,
-					      const __be32 saddr[4], const __be32 daddr[4],
-					      const __be32 spi, bool is_ipv6, u32 *hw_handle)
+int mlx5_ipsec_create_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry)
 {
-	struct mlx5_accel_esp_xfrm_attrs *xfrm_attrs = &accel_xfrm->attrs;
-	struct aes_gcm_keymat *aes_gcm = &xfrm_attrs->keymat.aes_gcm;
-	struct mlx5_ipsec_obj_attrs ipsec_attrs = {};
-	struct mlx5_ipsec_esp_xfrm *mxfrm;
-	struct mlx5_ipsec_sa_ctx *sa_ctx;
+	struct aes_gcm_keymat *aes_gcm = &sa_entry->attrs.keymat.aes_gcm;
+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
 	int err;
 
-	/* alloc SA context */
-	sa_ctx = kzalloc(sizeof(*sa_ctx), GFP_KERNEL);
-	if (!sa_ctx)
-		return ERR_PTR(-ENOMEM);
-
-	sa_ctx->dev = mdev;
-
-	mxfrm = container_of(accel_xfrm, struct mlx5_ipsec_esp_xfrm, accel_xfrm);
-	sa_ctx->mxfrm = mxfrm;
-
 	/* key */
 	err = mlx5_create_encryption_key(mdev, aes_gcm->aes_key,
 					 aes_gcm->key_len / BITS_PER_BYTE,
 					 MLX5_ACCEL_OBJ_IPSEC_KEY,
-					 &sa_ctx->enc_key_id);
+					 &sa_entry->enc_key_id);
 	if (err) {
 		mlx5_core_dbg(mdev, "Failed to create encryption key (err = %d)\n", err);
-		goto err_sa_ctx;
+		return err;
 	}
 
-	ipsec_attrs.aes_gcm = aes_gcm;
-	ipsec_attrs.accel_flags = accel_xfrm->attrs.flags;
-	ipsec_attrs.esn_msb = accel_xfrm->attrs.esn;
-	ipsec_attrs.enc_key_id = sa_ctx->enc_key_id;
-	err = mlx5_create_ipsec_obj(mdev, &ipsec_attrs,
-				    &sa_ctx->ipsec_obj_id);
+	err = mlx5_create_ipsec_obj(sa_entry);
 	if (err) {
 		mlx5_core_dbg(mdev, "Failed to create IPsec object (err = %d)\n", err);
 		goto err_enc_key;
 	}
 
-	*hw_handle = sa_ctx->ipsec_obj_id;
-	mxfrm->sa_ctx = sa_ctx;
-
-	return sa_ctx;
+	return 0;
 
 err_enc_key:
-	mlx5_destroy_encryption_key(mdev, sa_ctx->enc_key_id);
-err_sa_ctx:
-	kfree(sa_ctx);
-	return ERR_PTR(err);
+	mlx5_destroy_encryption_key(mdev, sa_entry->enc_key_id);
+	return err;
 }
 
-static void mlx5_ipsec_offload_delete_sa_ctx(void *context)
+void mlx5_ipsec_free_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry)
 {
-	struct mlx5_ipsec_sa_ctx *sa_ctx = (struct mlx5_ipsec_sa_ctx *)context;
+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
 
-	mlx5_destroy_ipsec_obj(sa_ctx->dev, sa_ctx->ipsec_obj_id);
-	mlx5_destroy_encryption_key(sa_ctx->dev, sa_ctx->enc_key_id);
-	kfree(sa_ctx);
+	mlx5_destroy_ipsec_obj(sa_entry);
+	mlx5_destroy_encryption_key(mdev, sa_entry->enc_key_id);
 }
 
-static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
-				 struct mlx5_ipsec_obj_attrs *attrs,
-				 u32 ipsec_id)
+static int mlx5_modify_ipsec_obj(struct mlx5e_ipsec_sa_entry *sa_entry,
+				 const struct mlx5_accel_esp_xfrm_attrs *attrs)
 {
+	struct mlx5_core_dev *mdev = mlx5e_ipsec_sa2dev(sa_entry);
 	u32 in[MLX5_ST_SZ_DW(modify_ipsec_obj_in)] = {};
 	u32 out[MLX5_ST_SZ_DW(query_ipsec_obj_out)];
 	u64 modify_field_select = 0;
@@ -239,7 +167,7 @@  static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
 	void *obj;
 	int err;
 
-	if (!(attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED))
+	if (!(attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_TRIGGERED))
 		return 0;
 
 	general_obj_types = MLX5_CAP_GEN_64(mdev, general_obj_types);
@@ -249,11 +177,11 @@  static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
 	/* general object fields set */
 	MLX5_SET(general_obj_in_cmd_hdr, in, opcode, MLX5_CMD_OP_QUERY_GENERAL_OBJECT);
 	MLX5_SET(general_obj_in_cmd_hdr, in, obj_type, MLX5_GENERAL_OBJECT_TYPES_IPSEC);
-	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, ipsec_id);
+	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, sa_entry->ipsec_obj_id);
 	err = mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
 	if (err) {
 		mlx5_core_err(mdev, "Query IPsec object failed (Object id %d), err = %d\n",
-			      ipsec_id, err);
+			      sa_entry->ipsec_obj_id, err);
 		return err;
 	}
 
@@ -266,8 +194,8 @@  static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
 		return -EOPNOTSUPP;
 
 	obj = MLX5_ADDR_OF(modify_ipsec_obj_in, in, ipsec_object);
-	MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn_msb);
-	if (attrs->accel_flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
+	MLX5_SET(ipsec_obj, obj, esn_msb, attrs->esn);
+	if (attrs->flags & MLX5_ACCEL_ESP_FLAGS_ESN_STATE_OVERLAP)
 		MLX5_SET(ipsec_obj, obj, esn_overlap, 1);
 
 	/* general object fields set */
@@ -276,50 +204,14 @@  static int mlx5_modify_ipsec_obj(struct mlx5_core_dev *mdev,
 	return mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
 }
 
-void mlx5_accel_esp_modify_xfrm(struct mlx5_accel_esp_xfrm *xfrm,
+void mlx5_accel_esp_modify_xfrm(struct mlx5e_ipsec_sa_entry *sa_entry,
 				const struct mlx5_accel_esp_xfrm_attrs *attrs)
 {
-	struct mlx5_ipsec_obj_attrs ipsec_attrs = {};
-	struct mlx5_core_dev *mdev = xfrm->mdev;
-	struct mlx5_ipsec_esp_xfrm *mxfrm;
 	int err;
 
-	mxfrm = container_of(xfrm, struct mlx5_ipsec_esp_xfrm, accel_xfrm);
-
-	/* need to add find and replace in ipsec_rhash_sa the sa_ctx */
-	/* modify device with new hw_sa */
-	ipsec_attrs.accel_flags = attrs->flags;
-	ipsec_attrs.esn_msb = attrs->esn;
-	err = mlx5_modify_ipsec_obj(mdev,
-				    &ipsec_attrs,
-				    mxfrm->sa_ctx->ipsec_obj_id);
-
+	err = mlx5_modify_ipsec_obj(sa_entry, attrs);
 	if (err)
 		return;
 
-	memcpy(&xfrm->attrs, attrs, sizeof(xfrm->attrs));
-}
-
-void *mlx5_accel_esp_create_hw_context(struct mlx5_core_dev *mdev,
-				       struct mlx5_accel_esp_xfrm *xfrm,
-				       u32 *sa_handle)
-{
-	__be32 saddr[4] = {}, daddr[4] = {};
-
-	if (!xfrm->attrs.is_ipv6) {
-		saddr[3] = xfrm->attrs.saddr.a4;
-		daddr[3] = xfrm->attrs.daddr.a4;
-	} else {
-		memcpy(saddr, xfrm->attrs.saddr.a6, sizeof(saddr));
-		memcpy(daddr, xfrm->attrs.daddr.a6, sizeof(daddr));
-	}
-
-	return mlx5_ipsec_offload_create_sa_ctx(mdev, xfrm, saddr, daddr,
-						xfrm->attrs.spi,
-						xfrm->attrs.is_ipv6, sa_handle);
-}
-
-void mlx5_accel_esp_free_hw_context(struct mlx5_core_dev *mdev, void *context)
-{
-	mlx5_ipsec_offload_delete_sa_ctx(context);
+	memcpy(&sa_entry->attrs, attrs, sizeof(sa_entry->attrs));
 }