[PATCH mlx5-next 00/15] Mellanox, mlx5 vport metadata matching

[PATCH mlx5-next 00/15] Mellanox, mlx5 vport metadata matching

[Saeed Mahameed]

Hi, 

This series includes mlx5 updates for both rdma and net-next trees.
In case of no objection it will be applied to mlx5-next branch and later
on will be sent as pull request to rdma and net-next.

1) From Bodong, E-Switch vport cleanups patches.

2) From Jianbo, Vport meta data matching:

Hardware steering has no notion of vport number, and vport is an
abstract concept, so firmware need to translate the source vport
matching to match on the VHCA ID (Virtual HCA ID).

In dual-port RoCE, the dual-port VHCA is able to send also on the
second port on behalf of the affiliated vport, so now we can’t assume
anymore that vport is represented by single VHCA only.

To resolve this issue, we use metadata register as source port
indicator instead.

When a packet enters the eswitch, eswitch ingress traffic passes the
ingress ACL flow tables, where we tag the packets (via the metadata
value, in this case REG_C at index 0) with a unique value which will
act as an alias of the vport. In order to guarantee uniqueness, we use
the eswitch owner vhca id and the vport number as that value.

Usually, the vports are numbered in each eswitch as followed:
    - Physical Function (PF) vport, the number is 0.
    - Virtual Function (VF) vport, starting from 1.
    - Uplink vport, the reserved vport number for it is 0xFFFF.

With the metadata in each packet, we can then do matching on it, in
both fast path and slow path.

For slow path, there is a representor for each vport. Packet that
misses all offloaded rules in FDB, will be forwarded to the eswitch
manager vport. In its NIC RX, it then will be steered to the right
representor. The rules, which decide the destination representor,
previously were matching on source port, will now match metadata
instead.

---

Bodong Wang (3):
  net/mlx5: E-Switch, Use vport index when init rep
  {IB, net}/mlx5: E-Switch, Use index of rep for vport to IB port
    mapping
  RDMA/mlx5: Cleanup rep when doing unload

Jianbo Liu (12):
  net/mlx5: Introduce vport metadata matching bits and enum constants
  net/mlx5: Get vport ACL namespace by vport index
  net/mlx5: Support allocating modify header context from ingress ACL
  net/mlx5: Add flow context for flow tag
  net/mlx5: E-Switch, Tag packet with vport number in VF vports and
    uplink ingress ACLs
  net/mlx5e: Specifying known origin of packets matching the flow
  net/mlx5: E-Switch, Add match on vport metadata for rule in fast path
  net/mlx5: E-Switch, Add query and modify esw vport context functions
  net/mlx5: E-Switch, Pass metadata from FDB to eswitch manager
  net/mlx5: E-Switch, Add match on vport metadata for rule in slow path
  RDMA/mlx5: Add vport metadata matching for IB representors
  net/mlx5: E-Switch, Enable vport metadata matching if firmware
    supports it

 drivers/infiniband/hw/mlx5/flow.c             |  13 +-
 drivers/infiniband/hw/mlx5/ib_rep.c           |  33 +-
 drivers/infiniband/hw/mlx5/ib_rep.h           |  16 +
 drivers/infiniband/hw/mlx5/main.c             |  75 ++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |   2 +-
 .../mellanox/mlx5/core/diag/fs_tracepoint.h   |   4 +-
 .../mellanox/mlx5/core/en_fs_ethtool.c        |   2 +-
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |   7 +-
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |  30 +-
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  14 +
 .../mellanox/mlx5/core/eswitch_offloads.c     | 494 ++++++++++++++----
 .../ethernet/mellanox/mlx5/core/fpga/ipsec.c  |   8 +-
 .../net/ethernet/mellanox/mlx5/core/fs_cmd.c  |  10 +-
 .../net/ethernet/mellanox/mlx5/core/fs_core.c |  34 +-
 .../net/ethernet/mellanox/mlx5/core/fs_core.h |   1 +
 include/linux/mlx5/eswitch.h                  |   5 +
 include/linux/mlx5/fs.h                       |  16 +-
 include/linux/mlx5/mlx5_ifc.h                 |  56 +-
 18 files changed, 639 insertions(+), 181 deletions(-)

-- 
2.21.0

[PATCH mlx5-next 01/15] net/mlx5: Introduce vport metadata matching bits and enum constants

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

When a dual-port VHCA sends a RoCE packet on its non-native port, and
the packet arrives to its affiliated vport FDB, a mismatch might occur
on the rules that match the packet source vport. So we replace the
match on source port with the match on metadata that was configured in
ingress ACL, and that metadata will be passed further also to the NIC
RX table of the eswitch manager.

Introduce vport metadata matching bits and enum constants as a pre-step
towards metadata matching.
    o metadata type C registers in the misc parameters 2 fields.
    o esw_uplink_ingress_acl bit in esw cap. If it set, the device supports
      ingress ACL for the uplink vport.
    o fdb_to_vport_reg_* bits in flow table cap and esw vport context, to
      support propagating the metadata to the nic rx through the loopback
      path.
    o flow_source in flow context, to indicate the known origin of packets.
    o enum constants, to support the above bits.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 include/linux/mlx5/mlx5_ifc.h | 56 ++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 7 deletions(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index e3c154b573a2..d4409654f760 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -528,7 +528,21 @@ struct mlx5_ifc_fte_match_set_misc2_bits {
 
 	struct mlx5_ifc_fte_match_mpls_bits outer_first_mpls_over_udp;
 
-	u8         reserved_at_80[0x100];
+	u8         metadata_reg_c_7[0x20];
+
+	u8         metadata_reg_c_6[0x20];
+
+	u8         metadata_reg_c_5[0x20];
+
+	u8         metadata_reg_c_4[0x20];
+
+	u8         metadata_reg_c_3[0x20];
+
+	u8         metadata_reg_c_2[0x20];
+
+	u8         metadata_reg_c_1[0x20];
+
+	u8         metadata_reg_c_0[0x20];
 
 	u8         metadata_reg_a[0x20];
 
@@ -636,8 +650,22 @@ struct mlx5_ifc_flow_table_nic_cap_bits {
 	u8         reserved_at_e00[0x7200];
 };
 
+enum {
+	MLX5_FDB_TO_VPORT_REG_C_0 = 0x01,
+	MLX5_FDB_TO_VPORT_REG_C_1 = 0x02,
+	MLX5_FDB_TO_VPORT_REG_C_2 = 0x04,
+	MLX5_FDB_TO_VPORT_REG_C_3 = 0x08,
+	MLX5_FDB_TO_VPORT_REG_C_4 = 0x10,
+	MLX5_FDB_TO_VPORT_REG_C_5 = 0x20,
+	MLX5_FDB_TO_VPORT_REG_C_6 = 0x40,
+	MLX5_FDB_TO_VPORT_REG_C_7 = 0x80,
+};
+
 struct mlx5_ifc_flow_table_eswitch_cap_bits {
-	u8      reserved_at_0[0x1a];
+	u8      fdb_to_vport_reg_c_id[0x8];
+	u8      reserved_at_8[0xf];
+	u8      flow_source[0x1];
+	u8      reserved_at_18[0x2];
 	u8      multi_fdb_encap[0x1];
 	u8      reserved_at_1b[0x1];
 	u8      fdb_multi_path_to_table[0x1];
@@ -665,7 +693,9 @@ struct mlx5_ifc_e_switch_cap_bits {
 	u8         vport_svlan_insert[0x1];
 	u8         vport_cvlan_insert_if_not_exist[0x1];
 	u8         vport_cvlan_insert_overwrite[0x1];
-	u8         reserved_at_5[0x14];
+	u8         reserved_at_5[0x3];
+	u8         esw_uplink_ingress_acl[0x1];
+	u8         reserved_at_9[0x10];
 	u8         esw_functions_changed[0x1];
 	u8         reserved_at_1a[0x1];
 	u8         ecpf_vport_exists[0x1];
@@ -2555,6 +2585,12 @@ enum {
 	MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2 = 0x800,
 };
 
+enum {
+	MLX5_FLOW_CONTEXT_FLOW_SOURCE_ANY_VPORT         = 0x0,
+	MLX5_FLOW_CONTEXT_FLOW_SOURCE_UPLINK            = 0x1,
+	MLX5_FLOW_CONTEXT_FLOW_SOURCE_LOCAL_VPORT       = 0x2,
+};
+
 struct mlx5_ifc_vlan_bits {
 	u8         ethtype[0x10];
 	u8         prio[0x3];
@@ -2574,7 +2610,9 @@ struct mlx5_ifc_flow_context_bits {
 	u8         action[0x10];
 
 	u8         extended_destination[0x1];
-	u8         reserved_at_80[0x7];
+	u8         reserved_at_81[0x1];
+	u8         flow_source[0x2];
+	u8         reserved_at_84[0x4];
 	u8         destination_list_size[0x18];
 
 	u8         reserved_at_a0[0x8];
@@ -3099,12 +3137,14 @@ struct mlx5_ifc_hca_vport_context_bits {
 };
 
 struct mlx5_ifc_esw_vport_context_bits {
-	u8         reserved_at_0[0x3];
+	u8         fdb_to_vport_reg_c[0x1];
+	u8         reserved_at_1[0x2];
 	u8         vport_svlan_strip[0x1];
 	u8         vport_cvlan_strip[0x1];
 	u8         vport_svlan_insert[0x1];
 	u8         vport_cvlan_insert[0x2];
-	u8         reserved_at_8[0x18];
+	u8         fdb_to_vport_reg_c_id[0x8];
+	u8         reserved_at_10[0x10];
 
 	u8         reserved_at_20[0x20];
 
@@ -4985,7 +5025,8 @@ struct mlx5_ifc_modify_esw_vport_context_out_bits {
 };
 
 struct mlx5_ifc_esw_vport_context_fields_select_bits {
-	u8         reserved_at_0[0x1c];
+	u8         reserved_at_0[0x1b];
+	u8         fdb_to_vport_reg_c_id[0x1];
 	u8         vport_cvlan_insert[0x1];
 	u8         vport_svlan_insert[0x1];
 	u8         vport_cvlan_strip[0x1];
@@ -5182,6 +5223,7 @@ enum {
 	MLX5_ACTION_IN_FIELD_OUT_DIPV4         = 0x16,
 	MLX5_ACTION_IN_FIELD_OUT_FIRST_VID     = 0x17,
 	MLX5_ACTION_IN_FIELD_OUT_IPV6_HOPLIMIT = 0x47,
+	MLX5_ACTION_IN_FIELD_METADATA_REG_C_0  = 0x51,
 };
 
 struct mlx5_ifc_alloc_modify_header_context_out_bits {
-- 
2.21.0

[PATCH mlx5-next 02/15] net/mlx5: Get vport ACL namespace by vport index

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

The ingress and egress ACL root namespaces are created per vport and
stored into arrays. However, the vport number is not the same as the
index. Passing the array index, instead of vport number, to get the
correct ingress and egress acl namespace.

Fixes: 9b93ab981e3b ("net/mlx5: Separate ingress/egress namespaces for each vport")
Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 12010f85fa35..a42a23e505df 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -939,7 +939,7 @@ int esw_vport_enable_egress_acl(struct mlx5_eswitch *esw,
 		  vport->vport, MLX5_CAP_ESW_EGRESS_ACL(dev, log_max_ft_size));
 
 	root_ns = mlx5_get_flow_vport_acl_namespace(dev, MLX5_FLOW_NAMESPACE_ESW_EGRESS,
-						    vport->vport);
+			mlx5_eswitch_vport_num_to_index(esw, vport->vport));
 	if (!root_ns) {
 		esw_warn(dev, "Failed to get E-Switch egress flow namespace for vport (%d)\n", vport->vport);
 		return -EOPNOTSUPP;
@@ -1057,7 +1057,7 @@ int esw_vport_enable_ingress_acl(struct mlx5_eswitch *esw,
 		  vport->vport, MLX5_CAP_ESW_INGRESS_ACL(dev, log_max_ft_size));
 
 	root_ns = mlx5_get_flow_vport_acl_namespace(dev, MLX5_FLOW_NAMESPACE_ESW_INGRESS,
-						    vport->vport);
+			mlx5_eswitch_vport_num_to_index(esw, vport->vport));
 	if (!root_ns) {
 		esw_warn(dev, "Failed to get E-Switch ingress flow namespace for vport (%d)\n", vport->vport);
 		return -EOPNOTSUPP;
-- 
2.21.0

[PATCH mlx5-next 03/15] net/mlx5: Support allocating modify header context from ingress ACL

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

That modify header action can be then attached to a steering rule in
the ingress ACL.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index bb24c3797218..4f1d402926f1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -771,6 +771,10 @@ int mlx5_modify_header_alloc(struct mlx5_core_dev *dev,
 		max_actions = MLX5_CAP_FLOWTABLE_NIC_TX(dev, max_modify_header_actions);
 		table_type = FS_FT_NIC_TX;
 		break;
+	case MLX5_FLOW_NAMESPACE_ESW_INGRESS:
+		max_actions = MLX5_CAP_ESW_INGRESS_ACL(dev, max_modify_header_actions);
+		table_type = FS_FT_ESW_INGRESS_ACL;
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.21.0

[PATCH mlx5-next 04/15] net/mlx5: Add flow context for flow tag

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

Refactor the flow data structures, add new flow_context and move
flow_tag into it, as flow_tag doesn't belong to the rule action.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/infiniband/hw/mlx5/flow.c             | 13 ++++---
 drivers/infiniband/hw/mlx5/main.c             | 30 ++++++++++------
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |  1 +
 .../mellanox/mlx5/core/diag/fs_tracepoint.h   |  2 +-
 .../mellanox/mlx5/core/en_fs_ethtool.c        |  2 +-
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  7 ++--
 .../ethernet/mellanox/mlx5/core/fpga/ipsec.c  |  8 +++--
 .../net/ethernet/mellanox/mlx5/core/fs_cmd.c  |  3 +-
 .../net/ethernet/mellanox/mlx5/core/fs_core.c | 34 +++++++++----------
 .../net/ethernet/mellanox/mlx5/core/fs_core.h |  1 +
 include/linux/mlx5/fs.h                       | 15 +++++---
 11 files changed, 71 insertions(+), 45 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/flow.c b/drivers/infiniband/hw/mlx5/flow.c
index 1fc302d41a53..b8841355fcd5 100644
--- a/drivers/infiniband/hw/mlx5/flow.c
+++ b/drivers/infiniband/hw/mlx5/flow.c
@@ -65,11 +65,12 @@ static const struct uverbs_attr_spec mlx5_ib_flow_type[] = {
 static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
 	struct uverbs_attr_bundle *attrs)
 {
-	struct mlx5_flow_act flow_act = {.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG};
+	struct mlx5_flow_context flow_context = {.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG};
 	struct mlx5_ib_flow_handler *flow_handler;
 	struct mlx5_ib_flow_matcher *fs_matcher;
 	struct ib_uobject **arr_flow_actions;
 	struct ib_uflow_resources *uflow_res;
+	struct mlx5_flow_act flow_act = {};
 	void *devx_obj;
 	int dest_id, dest_type;
 	void *cmd_in;
@@ -172,17 +173,19 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
 				   arr_flow_actions[i]->object);
 	}
 
-	ret = uverbs_copy_from(&flow_act.flow_tag, attrs,
+	ret = uverbs_copy_from(&flow_context.flow_tag, attrs,
 			       MLX5_IB_ATTR_CREATE_FLOW_TAG);
 	if (!ret) {
-		if (flow_act.flow_tag >= BIT(24)) {
+		if (flow_context.flow_tag >= BIT(24)) {
 			ret = -EINVAL;
 			goto err_out;
 		}
-		flow_act.flags |= FLOW_ACT_HAS_TAG;
+		flow_context.flags |= FLOW_CONTEXT_HAS_TAG;
 	}
 
-	flow_handler = mlx5_ib_raw_fs_rule_add(dev, fs_matcher, &flow_act,
+	flow_handler = mlx5_ib_raw_fs_rule_add(dev, fs_matcher,
+					       &flow_context,
+					       &flow_act,
 					       counter_id,
 					       cmd_in, inlen,
 					       dest_id, dest_type);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index abac70ad5c7c..be4c9a687df7 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2666,11 +2666,15 @@ int parse_flow_flow_action(struct mlx5_ib_flow_action *maction,
 	}
 }
 
-static int parse_flow_attr(struct mlx5_core_dev *mdev, u32 *match_c,
-			   u32 *match_v, const union ib_flow_spec *ib_spec,
+static int parse_flow_attr(struct mlx5_core_dev *mdev,
+			   struct mlx5_flow_spec *spec,
+			   const union ib_flow_spec *ib_spec,
 			   const struct ib_flow_attr *flow_attr,
 			   struct mlx5_flow_act *action, u32 prev_type)
 {
+	struct mlx5_flow_context *flow_context = &spec->flow_context;
+	u32 *match_c = spec->match_criteria;
+	u32 *match_v = spec->match_value;
 	void *misc_params_c = MLX5_ADDR_OF(fte_match_param, match_c,
 					   misc_parameters);
 	void *misc_params_v = MLX5_ADDR_OF(fte_match_param, match_v,
@@ -2989,8 +2993,8 @@ static int parse_flow_attr(struct mlx5_core_dev *mdev, u32 *match_c,
 		if (ib_spec->flow_tag.tag_id >= BIT(24))
 			return -EINVAL;
 
-		action->flow_tag = ib_spec->flow_tag.tag_id;
-		action->flags |= FLOW_ACT_HAS_TAG;
+		flow_context->flow_tag = ib_spec->flow_tag.tag_id;
+		flow_context->flags |= FLOW_CONTEXT_HAS_TAG;
 		break;
 	case IB_FLOW_SPEC_ACTION_DROP:
 		if (FIELDS_NOT_SUPPORTED(ib_spec->drop,
@@ -3084,7 +3088,8 @@ is_valid_esp_aes_gcm(struct mlx5_core_dev *mdev,
 		return VALID_SPEC_NA;
 
 	return is_crypto && is_ipsec &&
-		(!egress || (!is_drop && !(flow_act->flags & FLOW_ACT_HAS_TAG))) ?
+		(!egress || (!is_drop &&
+			     !(spec->flow_context.flags & FLOW_CONTEXT_HAS_TAG))) ?
 		VALID_SPEC_VALID : VALID_SPEC_INVALID;
 }
 
@@ -3473,7 +3478,7 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 {
 	struct mlx5_flow_table	*ft = ft_prio->flow_table;
 	struct mlx5_ib_flow_handler *handler;
-	struct mlx5_flow_act flow_act = {.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG};
+	struct mlx5_flow_act flow_act = {};
 	struct mlx5_flow_spec *spec;
 	struct mlx5_flow_destination dest_arr[2] = {};
 	struct mlx5_flow_destination *rule_dst = dest_arr;
@@ -3504,8 +3509,7 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 	}
 
 	for (spec_index = 0; spec_index < flow_attr->num_of_specs; spec_index++) {
-		err = parse_flow_attr(dev->mdev, spec->match_criteria,
-				      spec->match_value,
+		err = parse_flow_attr(dev->mdev, spec,
 				      ib_flow, flow_attr, &flow_act,
 				      prev_type);
 		if (err < 0)
@@ -3572,11 +3576,11 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 					MLX5_FLOW_CONTEXT_ACTION_FWD_NEXT_PRIO;
 	}
 
-	if ((flow_act.flags & FLOW_ACT_HAS_TAG)  &&
+	if ((spec->flow_context.flags & FLOW_CONTEXT_HAS_TAG)  &&
 	    (flow_attr->type == IB_FLOW_ATTR_ALL_DEFAULT ||
 	     flow_attr->type == IB_FLOW_ATTR_MC_DEFAULT)) {
 		mlx5_ib_warn(dev, "Flow tag %u and attribute type %x isn't allowed in leftovers\n",
-			     flow_act.flow_tag, flow_attr->type);
+			     spec->flow_context.flow_tag, flow_attr->type);
 		err = -EINVAL;
 		goto free;
 	}
@@ -3947,6 +3951,7 @@ _create_raw_flow_rule(struct mlx5_ib_dev *dev,
 		      struct mlx5_ib_flow_prio *ft_prio,
 		      struct mlx5_flow_destination *dst,
 		      struct mlx5_ib_flow_matcher  *fs_matcher,
+		      struct mlx5_flow_context *flow_context,
 		      struct mlx5_flow_act *flow_act,
 		      void *cmd_in, int inlen,
 		      int dst_num)
@@ -3969,6 +3974,7 @@ _create_raw_flow_rule(struct mlx5_ib_dev *dev,
 	memcpy(spec->match_criteria, fs_matcher->matcher_mask.match_params,
 	       fs_matcher->mask_len);
 	spec->match_criteria_enable = fs_matcher->match_criteria_enable;
+	spec->flow_context = *flow_context;
 
 	handler->rule = mlx5_add_flow_rules(ft, spec,
 					    flow_act, dst, dst_num);
@@ -4033,6 +4039,7 @@ static bool raw_fs_is_multicast(struct mlx5_ib_flow_matcher *fs_matcher,
 struct mlx5_ib_flow_handler *
 mlx5_ib_raw_fs_rule_add(struct mlx5_ib_dev *dev,
 			struct mlx5_ib_flow_matcher *fs_matcher,
+			struct mlx5_flow_context *flow_context,
 			struct mlx5_flow_act *flow_act,
 			u32 counter_id,
 			void *cmd_in, int inlen, int dest_id,
@@ -4085,7 +4092,8 @@ mlx5_ib_raw_fs_rule_add(struct mlx5_ib_dev *dev,
 		dst_num++;
 	}
 
-	handler = _create_raw_flow_rule(dev, ft_prio, dst, fs_matcher, flow_act,
+	handler = _create_raw_flow_rule(dev, ft_prio, dst, fs_matcher,
+					flow_context, flow_act,
 					cmd_in, inlen, dst_num);
 
 	if (IS_ERR(handler)) {
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index a043af7ee366..1c205c2bd486 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1317,6 +1317,7 @@ extern const struct uapi_definition mlx5_ib_devx_defs[];
 extern const struct uapi_definition mlx5_ib_flow_defs[];
 struct mlx5_ib_flow_handler *mlx5_ib_raw_fs_rule_add(
 	struct mlx5_ib_dev *dev, struct mlx5_ib_flow_matcher *fs_matcher,
+	struct mlx5_flow_context *flow_context,
 	struct mlx5_flow_act *flow_act, u32 counter_id,
 	void *cmd_in, int inlen, int dest_id, int dest_type);
 bool mlx5_ib_devx_is_flow_dest(void *obj, int *dest_id, int *dest_type);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
index a4cf123e3f17..9ec46edf22a6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
@@ -204,7 +204,7 @@ TRACE_EVENT(mlx5_fs_set_fte,
 			   __entry->index = fte->index;
 			   __entry->action = fte->action.action;
 			   __entry->mask_enable = __entry->fg->mask.match_criteria_enable;
-			   __entry->flow_tag = fte->action.flow_tag;
+			   __entry->flow_tag = fte->flow_context.flow_tag;
 			   memcpy(__entry->mask_outer,
 				  MLX5_ADDR_OF(fte_match_param,
 					       &__entry->fg->mask.match_criteria,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
index 4421c10f58ae..839662644ed3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
@@ -426,7 +426,7 @@ add_ethtool_flow_rule(struct mlx5e_priv *priv,
 	}
 
 	spec->match_criteria_enable = (!outer_header_zero(spec->match_criteria));
-	flow_act.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
+	spec->flow_context.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 	rule = mlx5_add_flow_rules(ft, spec, &flow_act, dst, dst ? 1 : 0);
 	if (IS_ERR(rule)) {
 		err = PTR_ERR(rule);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 122f457091a2..8ff1ca46d8d3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -716,19 +716,22 @@ mlx5e_tc_add_nic_flow(struct mlx5e_priv *priv,
 		      struct mlx5e_tc_flow *flow,
 		      struct netlink_ext_ack *extack)
 {
+	struct mlx5_flow_context *flow_context = &parse_attr->spec.flow_context;
 	struct mlx5_nic_flow_attr *attr = flow->nic_attr;
 	struct mlx5_core_dev *dev = priv->mdev;
 	struct mlx5_flow_destination dest[2] = {};
 	struct mlx5_flow_act flow_act = {
 		.action = attr->action,
-		.flow_tag = attr->flow_tag,
 		.reformat_id = 0,
-		.flags    = FLOW_ACT_HAS_TAG | FLOW_ACT_NO_APPEND,
+		.flags    = FLOW_ACT_NO_APPEND,
 	};
 	struct mlx5_fc *counter = NULL;
 	bool table_created = false;
 	int err, dest_ix = 0;
 
+	flow_context->flags |= FLOW_CONTEXT_HAS_TAG;
+	flow_context->flow_tag = attr->flow_tag;
+
 	if (flow->flags & MLX5E_TC_FLOW_HAIRPIN) {
 		err = mlx5e_hairpin_flow_add(priv, flow, parse_attr, extack);
 		if (err) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
index 52c47d3dd5a5..c76da309506b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
@@ -636,7 +636,8 @@ static bool mlx5_is_fpga_egress_ipsec_rule(struct mlx5_core_dev *dev,
 					   u8 match_criteria_enable,
 					   const u32 *match_c,
 					   const u32 *match_v,
-					   struct mlx5_flow_act *flow_act)
+					   struct mlx5_flow_act *flow_act,
+					   struct mlx5_flow_context *flow_context)
 {
 	const void *outer_c = MLX5_ADDR_OF(fte_match_param, match_c,
 					   outer_headers);
@@ -655,7 +656,7 @@ static bool mlx5_is_fpga_egress_ipsec_rule(struct mlx5_core_dev *dev,
 	    (match_criteria_enable &
 	     ~(MLX5_MATCH_OUTER_HEADERS | MLX5_MATCH_MISC_PARAMETERS)) ||
 	    (flow_act->action & ~(MLX5_FLOW_CONTEXT_ACTION_ENCRYPT | MLX5_FLOW_CONTEXT_ACTION_ALLOW)) ||
-	     (flow_act->flags & FLOW_ACT_HAS_TAG))
+	     (flow_context->flags & FLOW_CONTEXT_HAS_TAG))
 		return false;
 
 	return true;
@@ -767,7 +768,8 @@ mlx5_fpga_ipsec_fs_create_sa_ctx(struct mlx5_core_dev *mdev,
 					    fg->mask.match_criteria_enable,
 					    fg->mask.match_criteria,
 					    fte->val,
-					    &fte->action))
+					    &fte->action,
+					    &fte->flow_context))
 		return ERR_PTR(-EINVAL);
 	else if (!mlx5_is_fpga_ipsec_rule(mdev,
 					  fg->mask.match_criteria_enable,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index 4f1d402926f1..fb1335a433ae 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -396,7 +396,8 @@ static int mlx5_cmd_set_fte(struct mlx5_core_dev *dev,
 	in_flow_context = MLX5_ADDR_OF(set_fte_in, in, flow_context);
 	MLX5_SET(flow_context, in_flow_context, group_id, group_id);
 
-	MLX5_SET(flow_context, in_flow_context, flow_tag, fte->action.flow_tag);
+	MLX5_SET(flow_context, in_flow_context, flow_tag,
+		 fte->flow_context.flow_tag);
 	MLX5_SET(flow_context, in_flow_context, extended_destination,
 		 extended_dest);
 	if (extended_dest) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
index fb5b61727ee7..9f5544ac6b8a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
@@ -584,7 +584,7 @@ static int insert_fte(struct mlx5_flow_group *fg, struct fs_fte *fte)
 }
 
 static struct fs_fte *alloc_fte(struct mlx5_flow_table *ft,
-				u32 *match_value,
+				struct mlx5_flow_spec *spec,
 				struct mlx5_flow_act *flow_act)
 {
 	struct mlx5_flow_steering *steering = get_steering(&ft->node);
@@ -594,9 +594,10 @@ static struct fs_fte *alloc_fte(struct mlx5_flow_table *ft,
 	if (!fte)
 		return ERR_PTR(-ENOMEM);
 
-	memcpy(fte->val, match_value, sizeof(fte->val));
+	memcpy(fte->val, &spec->match_value, sizeof(fte->val));
 	fte->node.type =  FS_TYPE_FLOW_ENTRY;
 	fte->action = *flow_act;
+	fte->flow_context = spec->flow_context;
 
 	tree_init_node(&fte->node, NULL, del_sw_fte);
 
@@ -1428,7 +1429,9 @@ static bool check_conflicting_actions(u32 action1, u32 action2)
 	return false;
 }
 
-static int check_conflicting_ftes(struct fs_fte *fte, const struct mlx5_flow_act *flow_act)
+static int check_conflicting_ftes(struct fs_fte *fte,
+				  const struct mlx5_flow_context *flow_context,
+				  const struct mlx5_flow_act *flow_act)
 {
 	if (check_conflicting_actions(flow_act->action, fte->action.action)) {
 		mlx5_core_warn(get_dev(&fte->node),
@@ -1436,12 +1439,12 @@ static int check_conflicting_ftes(struct fs_fte *fte, const struct mlx5_flow_act
 		return -EEXIST;
 	}
 
-	if ((flow_act->flags & FLOW_ACT_HAS_TAG) &&
-	    fte->action.flow_tag != flow_act->flow_tag) {
+	if ((flow_context->flags & FLOW_CONTEXT_HAS_TAG) &&
+	    fte->flow_context.flow_tag != flow_context->flow_tag) {
 		mlx5_core_warn(get_dev(&fte->node),
 			       "FTE flow tag %u already exists with different flow tag %u\n",
-			       fte->action.flow_tag,
-			       flow_act->flow_tag);
+			       fte->flow_context.flow_tag,
+			       flow_context->flow_tag);
 		return -EEXIST;
 	}
 
@@ -1449,7 +1452,7 @@ static int check_conflicting_ftes(struct fs_fte *fte, const struct mlx5_flow_act
 }
 
 static struct mlx5_flow_handle *add_rule_fg(struct mlx5_flow_group *fg,
-					    u32 *match_value,
+					    struct mlx5_flow_spec *spec,
 					    struct mlx5_flow_act *flow_act,
 					    struct mlx5_flow_destination *dest,
 					    int dest_num,
@@ -1460,7 +1463,7 @@ static struct mlx5_flow_handle *add_rule_fg(struct mlx5_flow_group *fg,
 	int i;
 	int ret;
 
-	ret = check_conflicting_ftes(fte, flow_act);
+	ret = check_conflicting_ftes(fte, &spec->flow_context, flow_act);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -1635,7 +1638,7 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 	u64  version;
 	int err;
 
-	fte = alloc_fte(ft, spec->match_value, flow_act);
+	fte = alloc_fte(ft, spec, flow_act);
 	if (IS_ERR(fte))
 		return  ERR_PTR(-ENOMEM);
 
@@ -1651,8 +1654,7 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 		fte_tmp = lookup_fte_locked(g, spec->match_value, take_write);
 		if (!fte_tmp)
 			continue;
-		rule = add_rule_fg(g, spec->match_value,
-				   flow_act, dest, dest_num, fte_tmp);
+		rule = add_rule_fg(g, spec, flow_act, dest, dest_num, fte_tmp);
 		up_write_ref_node(&fte_tmp->node, false);
 		tree_put_node(&fte_tmp->node, false);
 		kmem_cache_free(steering->ftes_cache, fte);
@@ -1699,8 +1701,7 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 
 		nested_down_write_ref_node(&fte->node, FS_LOCK_CHILD);
 		up_write_ref_node(&g->node, false);
-		rule = add_rule_fg(g, spec->match_value,
-				   flow_act, dest, dest_num, fte);
+		rule = add_rule_fg(g, spec, flow_act, dest, dest_num, fte);
 		up_write_ref_node(&fte->node, false);
 		tree_put_node(&fte->node, false);
 		return rule;
@@ -1786,7 +1787,7 @@ _mlx5_add_flow_rules(struct mlx5_flow_table *ft,
 	if (err)
 		goto err_release_fg;
 
-	fte = alloc_fte(ft, spec->match_value, flow_act);
+	fte = alloc_fte(ft, spec, flow_act);
 	if (IS_ERR(fte)) {
 		err = PTR_ERR(fte);
 		goto err_release_fg;
@@ -1800,8 +1801,7 @@ _mlx5_add_flow_rules(struct mlx5_flow_table *ft,
 
 	nested_down_write_ref_node(&fte->node, FS_LOCK_CHILD);
 	up_write_ref_node(&g->node, false);
-	rule = add_rule_fg(g, spec->match_value, flow_act, dest,
-			   dest_num, fte);
+	rule = add_rule_fg(g, spec, flow_act, dest, dest_num, fte);
 	up_write_ref_node(&fte->node, false);
 	tree_put_node(&fte->node, false);
 	tree_put_node(&g->node, false);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h
index a08c3d09a50f..c48c382f926f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h
@@ -170,6 +170,7 @@ struct fs_fte {
 	u32				val[MLX5_ST_SZ_DW_MATCH_PARAM];
 	u32				dests_size;
 	u32				index;
+	struct mlx5_flow_context	flow_context;
 	struct mlx5_flow_act		action;
 	enum fs_fte_status		status;
 	struct mlx5_fc			*counter;
diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
index 2ddaa97f2179..9bf49ce218fa 100644
--- a/include/linux/mlx5/fs.h
+++ b/include/linux/mlx5/fs.h
@@ -88,10 +88,20 @@ struct mlx5_flow_group;
 struct mlx5_flow_namespace;
 struct mlx5_flow_handle;
 
+enum {
+	FLOW_CONTEXT_HAS_TAG = BIT(0),
+};
+
+struct mlx5_flow_context {
+	u32 flags;
+	u32 flow_tag;
+};
+
 struct mlx5_flow_spec {
 	u8   match_criteria_enable;
 	u32  match_criteria[MLX5_ST_SZ_DW(fte_match_param)];
 	u32  match_value[MLX5_ST_SZ_DW(fte_match_param)];
+	struct mlx5_flow_context flow_context;
 };
 
 enum {
@@ -173,13 +183,11 @@ struct mlx5_fs_vlan {
 #define MLX5_FS_VLAN_DEPTH	2
 
 enum {
-	FLOW_ACT_HAS_TAG   = BIT(0),
-	FLOW_ACT_NO_APPEND = BIT(1),
+	FLOW_ACT_NO_APPEND = BIT(0),
 };
 
 struct mlx5_flow_act {
 	u32 action;
-	u32 flow_tag;
 	u32 reformat_id;
 	u32 modify_id;
 	uintptr_t esp_id;
@@ -190,7 +198,6 @@ struct mlx5_flow_act {
 
 #define MLX5_DECLARE_FLOW_ACT(name) \
 	struct mlx5_flow_act name = { .action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST,\
-				      .flow_tag = MLX5_FS_DEFAULT_FLOW_TAG, \
 				      .reformat_id = 0, \
 				      .modify_id = 0, \
 				      .flags =  0, }
-- 
2.21.0

[PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

When a dual-port VHCA sends a RoCE packet on its non-native port, and the
packet arrives to its affiliated vport FDB, a mismatch might occur on the
rules that match the packet source vport as it is not represented by single
VHCA only in this case. So we change to match on metadata instead of source
vport.
To do that, a rule is created in all vports and uplink ingress ACLs, to
save the source vport number and vhca id in the packet's metadata in order
to match on it later.
The metadata register used is the first of the 32-bit type C registers. It
can be used for matching and header modify operations. The higher 16 bits
of this register are for vhca id, and the lower 16 ones is for vport
number.
This change is not for dual-port RoCE only. If HW and FW allow, the vport
metadata matching is enabled by default.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
 .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
 include/linux/mlx5/eswitch.h                  |   3 +
 4 files changed, 161 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index a42a23e505df..1235fd84ae3a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct mlx5_eswitch *esw,
 
 	vport->ingress.drop_rule = NULL;
 	vport->ingress.allow_rule = NULL;
+
+	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
 }
 
 void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 8b9f2cf58e91..4417a195832e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -68,6 +68,8 @@ struct vport_ingress {
 	struct mlx5_flow_group *allow_spoofchk_only_grp;
 	struct mlx5_flow_group *allow_untagged_only_grp;
 	struct mlx5_flow_group *drop_grp;
+	int                      modify_metadata_id;
+	struct mlx5_flow_handle  *modify_metadata_rule;
 	struct mlx5_flow_handle  *allow_rule;
 	struct mlx5_flow_handle  *drop_rule;
 	struct mlx5_fc           *drop_counter;
@@ -196,6 +198,10 @@ struct mlx5_esw_functions {
 	u16			num_vfs;
 };
 
+enum {
+	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0),
+};
+
 struct mlx5_eswitch {
 	struct mlx5_core_dev    *dev;
 	struct mlx5_nb          nb;
@@ -203,6 +209,7 @@ struct mlx5_eswitch {
 	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
 	struct workqueue_struct *work_queue;
 	struct mlx5_vport       *vports;
+	u32                     flags;
 	int                     total_vports;
 	int                     enabled_vports;
 	/* Synchronize between vport change events
@@ -240,6 +247,8 @@ void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
 				  struct mlx5_vport *vport);
 void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
 				   struct mlx5_vport *vport);
+void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
+					       struct mlx5_vport *vport);
 
 /* E-Switch API */
 int mlx5_eswitch_init(struct mlx5_core_dev *dev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 17abb98b48af..871ae44dc132 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1555,32 +1555,16 @@ static void esw_offloads_devcom_cleanup(struct mlx5_eswitch *esw)
 static int esw_vport_ingress_prio_tag_config(struct mlx5_eswitch *esw,
 					     struct mlx5_vport *vport)
 {
-	struct mlx5_core_dev *dev = esw->dev;
 	struct mlx5_flow_act flow_act = {0};
 	struct mlx5_flow_spec *spec;
 	int err = 0;
 
 	/* For prio tag mode, there is only 1 FTEs:
-	 * 1) Untagged packets - push prio tag VLAN, allow
+	 * 1) Untagged packets - push prio tag VLAN and modify metadata if
+	 * required, allow
 	 * Unmatched traffic is allowed by default
 	 */
 
-	if (!MLX5_CAP_ESW_INGRESS_ACL(dev, ft_support))
-		return -EOPNOTSUPP;
-
-	esw_vport_cleanup_ingress_rules(esw, vport);
-
-	err = esw_vport_enable_ingress_acl(esw, vport);
-	if (err) {
-		mlx5_core_warn(esw->dev,
-			       "failed to enable prio tag ingress acl (%d) on vport[%d]\n",
-			       err, vport->vport);
-		return err;
-	}
-
-	esw_debug(esw->dev,
-		  "vport[%d] configure ingress rules\n", vport->vport);
-
 	spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
 	if (!spec) {
 		err = -ENOMEM;
@@ -1596,6 +1580,12 @@ static int esw_vport_ingress_prio_tag_config(struct mlx5_eswitch *esw,
 	flow_act.vlan[0].ethtype = ETH_P_8021Q;
 	flow_act.vlan[0].vid = 0;
 	flow_act.vlan[0].prio = 0;
+
+	if (vport->ingress.modify_metadata_rule) {
+		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_MOD_HDR;
+		flow_act.modify_id = vport->ingress.modify_metadata_id;
+	}
+
 	vport->ingress.allow_rule =
 		mlx5_add_flow_rules(vport->ingress.acl, spec,
 				    &flow_act, NULL, 0);
@@ -1616,6 +1606,59 @@ static int esw_vport_ingress_prio_tag_config(struct mlx5_eswitch *esw,
 	return err;
 }
 
+static int esw_vport_add_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
+						     struct mlx5_vport *vport)
+{
+	u8 action[MLX5_UN_SZ_BYTES(set_action_in_add_action_in_auto)] = {};
+	struct mlx5_flow_act flow_act = {};
+	struct mlx5_flow_spec spec = {};
+	int err = 0;
+
+	MLX5_SET(set_action_in, action, action_type, MLX5_ACTION_TYPE_SET);
+	MLX5_SET(set_action_in, action, field, MLX5_ACTION_IN_FIELD_METADATA_REG_C_0);
+	MLX5_SET(set_action_in, action, data,
+		 mlx5_eswitch_get_vport_metadata_for_match(esw, vport->vport));
+
+	err = mlx5_modify_header_alloc(esw->dev, MLX5_FLOW_NAMESPACE_ESW_INGRESS,
+				       1, action, &vport->ingress.modify_metadata_id);
+
+	if (err) {
+		esw_warn(esw->dev,
+			 "failed to alloc modify header for vport %d ingress acl (%d)\n",
+			 vport->vport, err);
+		return err;
+	}
+
+	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_MOD_HDR | MLX5_FLOW_CONTEXT_ACTION_ALLOW;
+	flow_act.modify_id = vport->ingress.modify_metadata_id;
+	vport->ingress.modify_metadata_rule = mlx5_add_flow_rules(vport->ingress.acl,
+								  &spec, &flow_act, NULL, 0);
+	if (IS_ERR(vport->ingress.modify_metadata_rule)) {
+		err = PTR_ERR(vport->ingress.modify_metadata_rule);
+		esw_warn(esw->dev,
+			 "failed to add setting metadata rule for vport %d ingress acl, err(%d)\n",
+			 vport->vport, err);
+		vport->ingress.modify_metadata_rule = NULL;
+		goto out;
+	}
+
+out:
+	if (err)
+		mlx5_modify_header_dealloc(esw->dev, vport->ingress.modify_metadata_id);
+	return err;
+}
+
+void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
+					       struct mlx5_vport *vport)
+{
+	if (vport->ingress.modify_metadata_rule) {
+		mlx5_del_flow_rules(vport->ingress.modify_metadata_rule);
+		mlx5_modify_header_dealloc(esw->dev, vport->ingress.modify_metadata_id);
+
+		vport->ingress.modify_metadata_rule = NULL;
+	}
+}
+
 static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
 					    struct mlx5_vport *vport)
 {
@@ -1623,6 +1666,9 @@ static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
 	struct mlx5_flow_spec *spec;
 	int err = 0;
 
+	if (!MLX5_CAP_GEN(esw->dev, prio_tag_required))
+		return 0;
+
 	/* For prio tag mode, there is only 1 FTEs:
 	 * 1) prio tag packets - pop the prio tag VLAN, allow
 	 * Unmatched traffic is allowed by default
@@ -1676,27 +1722,77 @@ static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
 	return err;
 }
 
-static int esw_prio_tag_acls_config(struct mlx5_eswitch *esw, int nvports)
+static int esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
+					   struct mlx5_vport *vport)
 {
-	struct mlx5_vport *vport = NULL;
-	int i, j;
 	int err;
 
-	mlx5_esw_for_each_vf_vport(esw, i, vport, nvports) {
+	if (!mlx5_eswitch_vport_match_metadata_enabled(esw) &&
+	    !MLX5_CAP_GEN(esw->dev, prio_tag_required))
+		return 0;
+
+	esw_vport_cleanup_ingress_rules(esw, vport);
+
+	err = esw_vport_enable_ingress_acl(esw, vport);
+	if (err) {
+		esw_warn(esw->dev,
+			 "failed to enable ingress acl (%d) on vport[%d]\n",
+			 err, vport->vport);
+		return err;
+	}
+
+	esw_debug(esw->dev,
+		  "vport[%d] configure ingress rules\n", vport->vport);
+
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		err = esw_vport_add_ingress_acl_modify_metadata(esw, vport);
+		if (err)
+			goto out;
+	}
+
+	if (MLX5_CAP_GEN(esw->dev, prio_tag_required) &&
+	    (vport->vport >= MLX5_VPORT_FIRST_VF &&
+	     vport->vport <= esw->dev->priv.sriov.num_vfs)) {
 		err = esw_vport_ingress_prio_tag_config(esw, vport);
 		if (err)
-			goto err_ingress;
-		err = esw_vport_egress_prio_tag_config(esw, vport);
+			goto out;
+	}
+
+out:
+	if (err)
+		esw_vport_disable_ingress_acl(esw, vport);
+	return err;
+}
+
+static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw)
+{
+	struct mlx5_vport *vport;
+	int i, j;
+	int err;
+
+	mlx5_esw_for_all_vports(esw, i, vport) {
+		err = esw_vport_ingress_common_config(esw, vport);
 		if (err)
-			goto err_egress;
+			goto err_ingress;
+
+		if (vport->vport >= MLX5_VPORT_FIRST_VF &&
+		    vport->vport <= esw->dev->priv.sriov.num_vfs) {
+			err = esw_vport_egress_prio_tag_config(esw, vport);
+			if (err)
+				goto err_egress;
+		}
 	}
 
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
+		esw_info(esw->dev, "Use metadata reg_c as source vport to match\n");
+
 	return 0;
 
 err_egress:
 	esw_vport_disable_ingress_acl(esw, vport);
 err_ingress:
-	mlx5_esw_for_each_vf_vport_reverse(esw, j, vport, i - 1) {
+	for (j = MLX5_VPORT_PF; j < i; j++) {
+		vport = &esw->vports[j];
 		esw_vport_disable_egress_acl(esw, vport);
 		esw_vport_disable_ingress_acl(esw, vport);
 	}
@@ -1704,15 +1800,17 @@ static int esw_prio_tag_acls_config(struct mlx5_eswitch *esw, int nvports)
 	return err;
 }
 
-static void esw_prio_tag_acls_cleanup(struct mlx5_eswitch *esw)
+static void esw_destroy_offloads_acl_tables(struct mlx5_eswitch *esw)
 {
 	struct mlx5_vport *vport;
 	int i;
 
-	mlx5_esw_for_each_vf_vport(esw, i, vport, esw->nvports) {
+	mlx5_esw_for_all_vports(esw, i, vport) {
 		esw_vport_disable_egress_acl(esw, vport);
 		esw_vport_disable_ingress_acl(esw, vport);
 	}
+
+	esw->flags &= ~MLX5_ESWITCH_VPORT_MATCH_METADATA;
 }
 
 static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
@@ -1722,15 +1820,13 @@ static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
 	memset(&esw->fdb_table.offloads, 0, sizeof(struct offloads_fdb));
 	mutex_init(&esw->fdb_table.offloads.fdb_prio_lock);
 
-	if (MLX5_CAP_GEN(esw->dev, prio_tag_required)) {
-		err = esw_prio_tag_acls_config(esw, nvports);
-		if (err)
-			return err;
-	}
+	err = esw_create_offloads_acl_tables(esw);
+	if (err)
+		return err;
 
 	err = esw_create_offloads_fdb_tables(esw, nvports);
 	if (err)
-		return err;
+		goto create_fdb_err;
 
 	err = esw_create_offloads_table(esw, nvports);
 	if (err)
@@ -1748,6 +1844,9 @@ static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
 create_ft_err:
 	esw_destroy_offloads_fdb_tables(esw);
 
+create_fdb_err:
+	esw_destroy_offloads_acl_tables(esw);
+
 	return err;
 }
 
@@ -1756,8 +1855,7 @@ static void esw_offloads_steering_cleanup(struct mlx5_eswitch *esw)
 	esw_destroy_vport_rx_group(esw);
 	esw_destroy_offloads_table(esw);
 	esw_destroy_offloads_fdb_tables(esw);
-	if (MLX5_CAP_GEN(esw->dev, prio_tag_required))
-		esw_prio_tag_acls_cleanup(esw);
+	esw_destroy_offloads_acl_tables(esw);
 }
 
 static void esw_functions_changed_event_handler(struct work_struct *work)
@@ -2290,3 +2388,16 @@ struct mlx5_eswitch_rep *mlx5_eswitch_vport_rep(struct mlx5_eswitch *esw,
 	return mlx5_eswitch_get_rep(esw, vport);
 }
 EXPORT_SYMBOL(mlx5_eswitch_vport_rep);
+
+u32 mlx5_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
+{
+	return esw->flags & MLX5_ESWITCH_VPORT_MATCH_METADATA;
+}
+EXPORT_SYMBOL(mlx5_eswitch_vport_match_metadata_enabled);
+
+u32 mlx5_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
+					      u16 vport)
+{
+	return ((MLX5_CAP_GEN(esw->dev, vhca_id) & 0xffff) << 16) | vport;
+}
+EXPORT_SYMBOL(mlx5_eswitch_get_vport_metadata_for_match);
diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h
index 174eec0871d9..d729f5e4d70a 100644
--- a/include/linux/mlx5/eswitch.h
+++ b/include/linux/mlx5/eswitch.h
@@ -64,6 +64,9 @@ struct mlx5_flow_handle *
 mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw,
 				    int vport, u32 sqn);
 
+u32 mlx5_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw);
+u32 mlx5_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, u16 vport);
+
 #ifdef CONFIG_MLX5_ESWITCH
 enum devlink_eswitch_encap_mode
 mlx5_eswitch_get_encap_mode(const struct mlx5_core_dev *dev);
-- 
2.21.0

[PATCH mlx5-next 06/15] net/mlx5e: Specifying known origin of packets matching the flow

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

In vport metadata matching, source port number is replaced by metadata.
While FW has no idea about what it is in the metadata, a syndrome will
happen. Specify a known origin to avoid the syndrome.
However, there is no functional change because ANY_VPORT (0) is filled
in flow_source, the same default value as before, as a pre-step towards
metadata matching for fast path.
There are two other values can be filled in flow_source. When setting
0x1, packet matching this rule is from uplink, while 0x2 is for packet
from other local vports.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h | 2 ++
 drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c             | 3 +++
 include/linux/mlx5/fs.h                                      | 1 +
 3 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
index 9ec46edf22a6..ddf1b87f1bc0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
@@ -187,6 +187,7 @@ TRACE_EVENT(mlx5_fs_set_fte,
 		__field(u32, index)
 		__field(u32, action)
 		__field(u32, flow_tag)
+		__field(u32, flow_source)
 		__field(u8,  mask_enable)
 		__field(int, new_fte)
 		__array(u32, mask_outer, MLX5_ST_SZ_DW(fte_match_set_lyr_2_4))
@@ -205,6 +206,7 @@ TRACE_EVENT(mlx5_fs_set_fte,
 			   __entry->action = fte->action.action;
 			   __entry->mask_enable = __entry->fg->mask.match_criteria_enable;
 			   __entry->flow_tag = fte->flow_context.flow_tag;
+			   __entry->flow_source = fte->flow_context.flow_source;
 			   memcpy(__entry->mask_outer,
 				  MLX5_ADDR_OF(fte_match_param,
 					       &__entry->fg->mask.match_criteria,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index fb1335a433ae..7ac1249eadc3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -398,6 +398,9 @@ static int mlx5_cmd_set_fte(struct mlx5_core_dev *dev,
 
 	MLX5_SET(flow_context, in_flow_context, flow_tag,
 		 fte->flow_context.flow_tag);
+	MLX5_SET(flow_context, in_flow_context, flow_source,
+		 fte->flow_context.flow_source);
+
 	MLX5_SET(flow_context, in_flow_context, extended_destination,
 		 extended_dest);
 	if (extended_dest) {
diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
index 9bf49ce218fa..dc7e7aa53a13 100644
--- a/include/linux/mlx5/fs.h
+++ b/include/linux/mlx5/fs.h
@@ -95,6 +95,7 @@ enum {
 struct mlx5_flow_context {
 	u32 flags;
 	u32 flow_tag;
+	u32 flow_source;
 };
 
 struct mlx5_flow_spec {
-- 
2.21.0

[PATCH mlx5-next 07/15] net/mlx5: E-Switch, Add match on vport metadata for rule in fast path

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

If FW's capabilities and configurations meet the requirement of vport
metadata matching, this feature will be used. As the information
about vport number and vhca_id related to packet is already stored to
its metadata register, which is used as an indicator for perticular
vport, now we can change to match on this metadata for all the
offloading rules in fast path.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../mellanox/mlx5/core/eswitch_offloads.c     | 85 +++++++++++--------
 1 file changed, 51 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 871ae44dc132..ee18896dfd46 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -88,6 +88,53 @@ u16 mlx5_eswitch_get_prio_range(struct mlx5_eswitch *esw)
 	return 1;
 }
 
+static void
+mlx5_eswitch_set_rule_source_port(struct mlx5_eswitch *esw,
+				  struct mlx5_flow_spec *spec,
+				  struct mlx5_esw_flow_attr *attr)
+{
+	void *misc2;
+	void *misc;
+
+	/* Use metadata matching because vport is not represented by single
+	 * VHCA in dual-port RoCE mode, and matching on source vport may fail.
+	 */
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		misc2 = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_2);
+		MLX5_SET(fte_match_set_misc2, misc2, metadata_reg_c_0,
+			 mlx5_eswitch_get_vport_metadata_for_match(attr->in_mdev->priv.eswitch,
+								   attr->in_rep->vport));
+
+		misc2 = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_2);
+		MLX5_SET_TO_ONES(fte_match_set_misc2, misc2, metadata_reg_c_0);
+
+		spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_2;
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
+		if (memchr_inv(misc, 0, MLX5_ST_SZ_BYTES(fte_match_set_misc)))
+			spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+	} else {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
+		MLX5_SET(fte_match_set_misc, misc, source_port, attr->in_rep->vport);
+
+		if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
+			MLX5_SET(fte_match_set_misc, misc,
+				 source_eswitch_owner_vhca_id,
+				 MLX5_CAP_GEN(attr->in_mdev, vhca_id));
+
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
+		MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+		if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
+			MLX5_SET_TO_ONES(fte_match_set_misc, misc,
+					 source_eswitch_owner_vhca_id);
+
+		spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+	}
+
+	if (MLX5_CAP_ESW_FLOWTABLE(esw->dev, flow_source) &&
+	    attr->in_rep->vport == MLX5_VPORT_UPLINK)
+		spec->flow_context.flow_source = MLX5_FLOW_CONTEXT_FLOW_SOURCE_UPLINK;
+}
+
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_spec *spec,
@@ -99,7 +146,6 @@ mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 	struct mlx5_flow_handle *rule;
 	struct mlx5_flow_table *fdb;
 	int j, i = 0;
-	void *misc;
 
 	if (esw->mode != SRIOV_OFFLOADS)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -159,21 +205,8 @@ mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 		i++;
 	}
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
-	MLX5_SET(fte_match_set_misc, misc, source_port, attr->in_rep->vport);
-
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET(fte_match_set_misc, misc,
-			 source_eswitch_owner_vhca_id,
-			 MLX5_CAP_GEN(attr->in_mdev, vhca_id));
+	mlx5_eswitch_set_rule_source_port(esw, spec, attr);
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET_TO_ONES(fte_match_set_misc, misc,
-				 source_eswitch_owner_vhca_id);
-
-	spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
 	if (flow_act.action & MLX5_FLOW_CONTEXT_ACTION_DECAP) {
 		if (attr->tunnel_match_level != MLX5_MATCH_NONE)
 			spec->match_criteria_enable |= MLX5_MATCH_OUTER_HEADERS;
@@ -219,7 +252,6 @@ mlx5_eswitch_add_fwd_rule(struct mlx5_eswitch *esw,
 	struct mlx5_flow_table *fast_fdb;
 	struct mlx5_flow_table *fwd_fdb;
 	struct mlx5_flow_handle *rule;
-	void *misc;
 	int i;
 
 	fast_fdb = esw_get_prio_table(esw, attr->chain, attr->prio, 0);
@@ -251,25 +283,10 @@ mlx5_eswitch_add_fwd_rule(struct mlx5_eswitch *esw,
 	dest[i].ft = fwd_fdb,
 	i++;
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
-	MLX5_SET(fte_match_set_misc, misc, source_port, attr->in_rep->vport);
-
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET(fte_match_set_misc, misc,
-			 source_eswitch_owner_vhca_id,
-			 MLX5_CAP_GEN(attr->in_mdev, vhca_id));
-
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET_TO_ONES(fte_match_set_misc, misc,
-				 source_eswitch_owner_vhca_id);
+	mlx5_eswitch_set_rule_source_port(esw, spec, attr);
 
-	if (attr->match_level == MLX5_MATCH_NONE)
-		spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
-	else
-		spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS |
-					      MLX5_MATCH_MISC_PARAMETERS;
+	if (attr->match_level != MLX5_MATCH_NONE)
+		spec->match_criteria_enable |= MLX5_MATCH_OUTER_HEADERS;
 
 	rule = mlx5_add_flow_rules(fast_fdb, spec, &flow_act, dest, i);
 
-- 
2.21.0

[PATCH mlx5-next 08/15] net/mlx5: E-Switch, Add query and modify esw vport context functions

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

Add esw vport query and modify functions, and exposing them is needed for
enabling or disabling registers passed as metatdata to vport NIC_RX table
in slow path.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/eswitch.c | 24 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  5 ++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 1235fd84ae3a..67598272d4a9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -134,6 +134,30 @@ static int modify_esw_vport_context_cmd(struct mlx5_core_dev *dev, u16 vport,
 	return mlx5_cmd_exec(dev, in, inlen, out, sizeof(out));
 }
 
+int mlx5_eswitch_modify_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					  void *in, int inlen)
+{
+	return modify_esw_vport_context_cmd(esw->dev, vport, in, inlen);
+}
+
+static int query_esw_vport_context_cmd(struct mlx5_core_dev *dev, u16 vport,
+				       void *out, int outlen)
+{
+	u32 in[MLX5_ST_SZ_DW(query_esw_vport_context_in)] = {};
+
+	MLX5_SET(query_esw_vport_context_in, in, opcode,
+		 MLX5_CMD_OP_QUERY_ESW_VPORT_CONTEXT);
+	MLX5_SET(modify_esw_vport_context_in, in, vport_number, vport);
+	MLX5_SET(modify_esw_vport_context_in, in, other_vport, 1);
+	return mlx5_cmd_exec(dev, in, sizeof(in), out, outlen);
+}
+
+int mlx5_eswitch_query_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					 void *out, int outlen)
+{
+	return query_esw_vport_context_cmd(esw->dev, vport, out, outlen);
+}
+
 static int modify_esw_vport_cvlan(struct mlx5_core_dev *dev, u16 vport,
 				  u16 vlan, u8 qos, u8 set_flags)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 4417a195832e..aebb1a4b9070 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -276,6 +276,11 @@ int mlx5_eswitch_get_vport_stats(struct mlx5_eswitch *esw,
 				 struct ifla_vf_stats *vf_stats);
 void mlx5_eswitch_del_send_to_vport_rule(struct mlx5_flow_handle *rule);
 
+int mlx5_eswitch_modify_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					  void *in, int inlen);
+int mlx5_eswitch_query_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					 void *out, int outlen);
+
 struct mlx5_flow_spec;
 struct mlx5_esw_flow_attr;
 
-- 
2.21.0

[PATCH mlx5-next 09/15] net/mlx5: E-Switch, Pass metadata from FDB to eswitch manager

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

In order to do matching on metadata in slow path when demuxing traffic
to representors, explicitly enable the feature that allows HW to pass
metadata REG_C_0 from FDB to eswitch manager NIC_RX table.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../mellanox/mlx5/core/eswitch_offloads.c     | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index ee18896dfd46..f7fd2afac461 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -574,6 +574,59 @@ void mlx5_eswitch_del_send_to_vport_rule(struct mlx5_flow_handle *rule)
 	mlx5_del_flow_rules(rule);
 }
 
+static int mlx5_eswitch_enable_passing_vport_metadata(struct mlx5_eswitch *esw)
+{
+	u32 out[MLX5_ST_SZ_DW(query_esw_vport_context_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(modify_esw_vport_context_in)] = {};
+	u8 fdb_to_vport_reg_c_id;
+	int err;
+
+	err = mlx5_eswitch_query_esw_vport_context(esw, esw->manager_vport,
+						   out, sizeof(out));
+	if (err)
+		return err;
+
+	fdb_to_vport_reg_c_id = MLX5_GET(query_esw_vport_context_out, out,
+					 esw_vport_context.fdb_to_vport_reg_c_id);
+
+	fdb_to_vport_reg_c_id |= MLX5_FDB_TO_VPORT_REG_C_0;
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 esw_vport_context.fdb_to_vport_reg_c_id, fdb_to_vport_reg_c_id);
+
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 field_select.fdb_to_vport_reg_c_id, 1);
+
+	return mlx5_eswitch_modify_esw_vport_context(esw, esw->manager_vport,
+						     in, sizeof(in));
+}
+
+static int mlx5_eswitch_disable_passing_vport_metadata(struct mlx5_eswitch *esw)
+{
+	u32 out[MLX5_ST_SZ_DW(query_esw_vport_context_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(modify_esw_vport_context_in)] = {};
+	u8 fdb_to_vport_reg_c_id;
+	int err;
+
+	err = mlx5_eswitch_query_esw_vport_context(esw, esw->manager_vport,
+						   out, sizeof(out));
+	if (err)
+		return err;
+
+	fdb_to_vport_reg_c_id = MLX5_GET(query_esw_vport_context_out, out,
+					 esw_vport_context.fdb_to_vport_reg_c_id);
+
+	fdb_to_vport_reg_c_id &= ~MLX5_FDB_TO_VPORT_REG_C_0;
+
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 esw_vport_context.fdb_to_vport_reg_c_id, fdb_to_vport_reg_c_id);
+
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 field_select.fdb_to_vport_reg_c_id, 1);
+
+	return mlx5_eswitch_modify_esw_vport_context(esw, esw->manager_vport,
+						     in, sizeof(in));
+}
+
 static void peer_miss_rules_setup(struct mlx5_core_dev *peer_dev,
 				  struct mlx5_flow_spec *spec,
 				  struct mlx5_flow_destination *dest)
@@ -1980,6 +2033,12 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int vf_nvports,
 	if (err)
 		return err;
 
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		err = mlx5_eswitch_enable_passing_vport_metadata(esw);
+		if (err)
+			goto err_vport_metadata;
+	}
+
 	/* Only load special vports reps. VF reps will be loaded in
 	 * context of functions_changed event handler through real
 	 * or emulated event.
@@ -2007,6 +2066,9 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int vf_nvports,
 	return 0;
 
 err_reps:
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
+		mlx5_eswitch_disable_passing_vport_metadata(esw);
+err_vport_metadata:
 	esw_offloads_steering_cleanup(esw);
 	return err;
 }
@@ -2036,6 +2098,8 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw)
 	mlx5_rdma_disable_roce(esw->dev);
 	esw_offloads_devcom_cleanup(esw);
 	esw_offloads_unload_all_reps(esw, esw->esw_funcs.num_vfs);
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
+		mlx5_eswitch_disable_passing_vport_metadata(esw);
 	esw_offloads_steering_cleanup(esw);
 }
 
-- 
2.21.0

[PATCH mlx5-next 10/15] net/mlx5: E-Switch, Add match on vport metadata for rule in slow path

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

In slow path, packet that not matched by any offloaded rule is
forwarded to eswitch vport manager for further processing.
Add matching on metadata for peer miss rules in FDB, and rules which
forward packet to correct representor in esw manager NIC_RX table.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../mellanox/mlx5/core/eswitch_offloads.c     | 142 +++++++++++++-----
 1 file changed, 107 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index f7fd2afac461..363517e29d4c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -627,23 +627,34 @@ static int mlx5_eswitch_disable_passing_vport_metadata(struct mlx5_eswitch *esw)
 						     in, sizeof(in));
 }
 
-static void peer_miss_rules_setup(struct mlx5_core_dev *peer_dev,
+static void peer_miss_rules_setup(struct mlx5_eswitch *esw,
+				  struct mlx5_core_dev *peer_dev,
 				  struct mlx5_flow_spec *spec,
 				  struct mlx5_flow_destination *dest)
 {
-	void *misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
-				  misc_parameters);
+	void *misc;
 
-	MLX5_SET(fte_match_set_misc, misc, source_eswitch_owner_vhca_id,
-		 MLX5_CAP_GEN(peer_dev, vhca_id));
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+				    misc_parameters_2);
+		MLX5_SET_TO_ONES(fte_match_set_misc2, misc, metadata_reg_c_0);
 
-	spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
+		spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS_2;
+	} else {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+				    misc_parameters);
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
-			    misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc,
-			 source_eswitch_owner_vhca_id);
+		MLX5_SET(fte_match_set_misc, misc, source_eswitch_owner_vhca_id,
+			 MLX5_CAP_GEN(peer_dev, vhca_id));
+
+		spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
+
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+				    misc_parameters);
+		MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+		MLX5_SET_TO_ONES(fte_match_set_misc, misc,
+				 source_eswitch_owner_vhca_id);
+	}
 
 	dest->type = MLX5_FLOW_DESTINATION_TYPE_VPORT;
 	dest->vport.num = peer_dev->priv.eswitch->manager_vport;
@@ -651,6 +662,26 @@ static void peer_miss_rules_setup(struct mlx5_core_dev *peer_dev,
 	dest->vport.flags |= MLX5_FLOW_DEST_VPORT_VHCA_ID;
 }
 
+static void esw_set_peer_miss_rule_source_port(struct mlx5_eswitch *esw,
+					       struct mlx5_eswitch *peer_esw,
+					       struct mlx5_flow_spec *spec,
+					       u16 vport)
+{
+	void *misc;
+
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+				    misc_parameters_2);
+		MLX5_SET(fte_match_set_misc2, misc, metadata_reg_c_0,
+			 mlx5_eswitch_get_vport_metadata_for_match(peer_esw,
+								   vport));
+	} else {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+				    misc_parameters);
+		MLX5_SET(fte_match_set_misc, misc, source_port, vport);
+	}
+}
+
 static int esw_add_fdb_peer_miss_rules(struct mlx5_eswitch *esw,
 				       struct mlx5_core_dev *peer_dev)
 {
@@ -668,7 +699,7 @@ static int esw_add_fdb_peer_miss_rules(struct mlx5_eswitch *esw,
 	if (!spec)
 		return -ENOMEM;
 
-	peer_miss_rules_setup(peer_dev, spec, &dest);
+	peer_miss_rules_setup(esw, peer_dev, spec, &dest);
 
 	flows = kvzalloc(nvports * sizeof(*flows), GFP_KERNEL);
 	if (!flows) {
@@ -681,7 +712,9 @@ static int esw_add_fdb_peer_miss_rules(struct mlx5_eswitch *esw,
 			    misc_parameters);
 
 	if (mlx5_core_is_ecpf_esw_manager(esw->dev)) {
-		MLX5_SET(fte_match_set_misc, misc, source_port, MLX5_VPORT_PF);
+		esw_set_peer_miss_rule_source_port(esw, peer_dev->priv.eswitch,
+						   spec, MLX5_VPORT_PF);
+
 		flow = mlx5_add_flow_rules(esw->fdb_table.offloads.slow_fdb,
 					   spec, &flow_act, &dest, 1);
 		if (IS_ERR(flow)) {
@@ -703,7 +736,10 @@ static int esw_add_fdb_peer_miss_rules(struct mlx5_eswitch *esw,
 	}
 
 	mlx5_esw_for_each_vf_vport_num(esw, i, mlx5_core_max_vfs(esw->dev)) {
-		MLX5_SET(fte_match_set_misc, misc, source_port, i);
+		esw_set_peer_miss_rule_source_port(esw,
+						   peer_dev->priv.eswitch,
+						   spec, i);
+
 		flow = mlx5_add_flow_rules(esw->fdb_table.offloads.slow_fdb,
 					   spec, &flow_act, &dest, 1);
 		if (IS_ERR(flow)) {
@@ -987,6 +1023,30 @@ static void esw_destroy_offloads_fast_fdb_tables(struct mlx5_eswitch *esw)
 #define MAX_PF_SQ 256
 #define MAX_SQ_NVPORTS 32
 
+static void esw_set_flow_group_source_port(struct mlx5_eswitch *esw,
+					   u32 *flow_group_in)
+{
+	void *match_criteria = MLX5_ADDR_OF(create_flow_group_in,
+					    flow_group_in,
+					    match_criteria);
+
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		MLX5_SET(create_flow_group_in, flow_group_in,
+			 match_criteria_enable,
+			 MLX5_MATCH_MISC_PARAMETERS_2);
+
+		MLX5_SET_TO_ONES(fte_match_param, match_criteria,
+				 misc_parameters_2.metadata_reg_c_0);
+	} else {
+		MLX5_SET(create_flow_group_in, flow_group_in,
+			 match_criteria_enable,
+			 MLX5_MATCH_MISC_PARAMETERS);
+
+		MLX5_SET_TO_ONES(fte_match_param, match_criteria,
+				 misc_parameters.source_port);
+	}
+}
+
 static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int nvports)
 {
 	int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in);
@@ -1084,19 +1144,21 @@ static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int nvports)
 
 	/* create peer esw miss group */
 	memset(flow_group_in, 0, inlen);
-	MLX5_SET(create_flow_group_in, flow_group_in, match_criteria_enable,
-		 MLX5_MATCH_MISC_PARAMETERS);
 
-	match_criteria = MLX5_ADDR_OF(create_flow_group_in, flow_group_in,
-				      match_criteria);
+	esw_set_flow_group_source_port(esw, flow_group_in);
+
+	if (!mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		match_criteria = MLX5_ADDR_OF(create_flow_group_in,
+					      flow_group_in,
+					      match_criteria);
 
-	MLX5_SET_TO_ONES(fte_match_param, match_criteria,
-			 misc_parameters.source_port);
-	MLX5_SET_TO_ONES(fte_match_param, match_criteria,
-			 misc_parameters.source_eswitch_owner_vhca_id);
+		MLX5_SET_TO_ONES(fte_match_param, match_criteria,
+				 misc_parameters.source_eswitch_owner_vhca_id);
+
+		MLX5_SET(create_flow_group_in, flow_group_in,
+			 source_eswitch_owner_vhca_id_valid, 1);
+	}
 
-	MLX5_SET(create_flow_group_in, flow_group_in,
-		 source_eswitch_owner_vhca_id_valid, 1);
 	MLX5_SET(create_flow_group_in, flow_group_in, start_flow_index, ix);
 	MLX5_SET(create_flow_group_in, flow_group_in, end_flow_index,
 		 ix + esw->total_vports - 1);
@@ -1210,7 +1272,6 @@ static int esw_create_vport_rx_group(struct mlx5_eswitch *esw, int nvports)
 	int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in);
 	struct mlx5_flow_group *g;
 	u32 *flow_group_in;
-	void *match_criteria, *misc;
 	int err = 0;
 
 	nvports = nvports + MLX5_ESW_MISS_FLOWS;
@@ -1220,12 +1281,8 @@ static int esw_create_vport_rx_group(struct mlx5_eswitch *esw, int nvports)
 
 	/* create vport rx group */
 	memset(flow_group_in, 0, inlen);
-	MLX5_SET(create_flow_group_in, flow_group_in, match_criteria_enable,
-		 MLX5_MATCH_MISC_PARAMETERS);
 
-	match_criteria = MLX5_ADDR_OF(create_flow_group_in, flow_group_in, match_criteria);
-	misc = MLX5_ADDR_OF(fte_match_param, match_criteria, misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+	esw_set_flow_group_source_port(esw, flow_group_in);
 
 	MLX5_SET(create_flow_group_in, flow_group_in, start_flow_index, 0);
 	MLX5_SET(create_flow_group_in, flow_group_in, end_flow_index, nvports - 1);
@@ -1264,13 +1321,24 @@ mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport,
 		goto out;
 	}
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
-	MLX5_SET(fte_match_set_misc, misc, source_port, vport);
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_2);
+		MLX5_SET(fte_match_set_misc2, misc, metadata_reg_c_0,
+			 mlx5_eswitch_get_vport_metadata_for_match(esw, vport));
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_2);
+		MLX5_SET_TO_ONES(fte_match_set_misc2, misc, metadata_reg_c_0);
 
-	spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
+		spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS_2;
+	} else {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
+		MLX5_SET(fte_match_set_misc, misc, source_port, vport);
+
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
+		MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+
+		spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
+	}
 
 	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
 	flow_rule = mlx5_add_flow_rules(esw->offloads.ft_offloads, spec,
@@ -1557,6 +1625,10 @@ static int mlx5_esw_offloads_devcom_event(int event,
 
 	switch (event) {
 	case ESW_OFFLOADS_DEVCOM_PAIR:
+		if (mlx5_eswitch_vport_match_metadata_enabled(esw) !=
+		    mlx5_eswitch_vport_match_metadata_enabled(peer_esw))
+			break;
+
 		err = mlx5_esw_offloads_pair(esw, peer_esw);
 		if (err)
 			goto err_out;
-- 
2.21.0

[PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

If vport metadata matching is enabled in eswitch, the rule created
must be changed to match on the metadata, instead of source port.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
 drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
 drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
 3 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
index 22e651cb5534..d4ed611de35d 100644
--- a/drivers/infiniband/hw/mlx5/ib_rep.c
+++ b/drivers/infiniband/hw/mlx5/ib_rep.c
@@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
 	return mlx5_eswitch_vport_rep(esw, vport);
 }
 
+u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
+{
+	return mlx5_eswitch_vport_match_metadata_enabled(esw);
+}
+
+u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
+						 u16 vport)
+{
+	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
+}
+
 struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev,
 						   struct mlx5_ib_sq *sq,
 						   u16 port)
diff --git a/drivers/infiniband/hw/mlx5/ib_rep.h b/drivers/infiniband/hw/mlx5/ib_rep.h
index 22adce2d6795..65a04b6b0df3 100644
--- a/drivers/infiniband/hw/mlx5/ib_rep.h
+++ b/drivers/infiniband/hw/mlx5/ib_rep.h
@@ -25,6 +25,9 @@ struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev,
 						   u16 port);
 struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw,
 					  int vport_index);
+u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw);
+u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
+						 u16 vport);
 #else /* CONFIG_MLX5_ESWITCH */
 static inline u8 mlx5_ib_eswitch_mode(struct mlx5_eswitch *esw)
 {
@@ -67,6 +70,19 @@ struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw,
 {
 	return NULL;
 }
+
+static inline
+u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
+{
+	return 0;
+};
+
+static inline
+u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
+						 u16 vport)
+{
+	return 0;
+};
 #endif
 
 static inline
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index be4c9a687df7..f97519c0c4df 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3469,6 +3469,37 @@ static int flow_counters_set_data(struct ib_counters *ibcounters,
 	return ret;
 }
 
+static void mlx5_ib_set_rule_source_port(struct mlx5_ib_dev *dev,
+					 struct mlx5_flow_spec *spec,
+					 struct mlx5_eswitch_rep *rep)
+{
+	struct mlx5_eswitch *esw = dev->mdev->priv.eswitch;
+	void *misc;
+
+	if (mlx5_ib_eswitch_vport_match_metadata_enabled(esw)) {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+				    misc_parameters_2);
+
+		MLX5_SET(fte_match_set_misc2, misc, metadata_reg_c_0,
+			 mlx5_ib_eswitch_get_vport_metadata_for_match(esw,
+								      rep->vport));
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+				    misc_parameters_2);
+
+		MLX5_SET_TO_ONES(fte_match_set_misc2, misc, metadata_reg_c_0);
+	} else {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
+				    misc_parameters);
+
+		MLX5_SET(fte_match_set_misc, misc, source_port, rep->vport);
+
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
+				    misc_parameters);
+
+		MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+	}
+}
+
 static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 						      struct mlx5_ib_flow_prio *ft_prio,
 						      const struct ib_flow_attr *flow_attr,
@@ -3523,19 +3554,15 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 		set_underlay_qp(dev, spec, underlay_qpn);
 
 	if (dev->is_rep) {
-		void *misc;
+		struct mlx5_eswitch_rep *rep;
 
-		if (!dev->port[flow_attr->port - 1].rep) {
+		rep = dev->port[flow_attr->port - 1].rep;
+		if (!rep) {
 			err = -EINVAL;
 			goto free;
 		}
-		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
-				    misc_parameters);
-		MLX5_SET(fte_match_set_misc, misc, source_port,
-			 dev->port[flow_attr->port - 1].rep->vport);
-		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
-				    misc_parameters);
-		MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+
+		mlx5_ib_set_rule_source_port(dev, spec, rep);
 	}
 
 	spec->match_criteria_enable = get_match_criteria_enable(spec->match_criteria);
-- 
2.21.0

[PATCH mlx5-next 12/15] net/mlx5: E-Switch, Enable vport metadata matching if firmware supports it

[Saeed Mahameed]

From: Jianbo Liu <jianbol@mellanox.com>

As the ingress ACL rules save vhca id and vport number to packet's
metadata REG_C_0, and the metadata matching for the rules in both fast
path and slow path are all added, enable this feature if supported.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c  | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 363517e29d4c..5124219a31de 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1906,12 +1906,25 @@ static int esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
 	return err;
 }
 
+static int esw_check_vport_match_metadata_supported(struct mlx5_eswitch *esw)
+{
+	return (MLX5_CAP_ESW_FLOWTABLE(esw->dev, fdb_to_vport_reg_c_id) &
+		MLX5_FDB_TO_VPORT_REG_C_0) &&
+	       MLX5_CAP_ESW_FLOWTABLE(esw->dev, flow_source) &&
+	       MLX5_CAP_ESW(esw->dev, esw_uplink_ingress_acl) &&
+	       !mlx5_core_is_ecpf_esw_manager(esw->dev) &&
+	       !mlx5_ecpf_vport_exists(esw->dev);
+}
+
 static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw)
 {
 	struct mlx5_vport *vport;
 	int i, j;
 	int err;
 
+	if (esw_check_vport_match_metadata_supported(esw))
+		esw->flags |= MLX5_ESWITCH_VPORT_MATCH_METADATA;
+
 	mlx5_esw_for_all_vports(esw, i, vport) {
 		err = esw_vport_ingress_common_config(esw, vport);
 		if (err)
-- 
2.21.0

[PATCH mlx5-next 13/15] net/mlx5: E-Switch, Use vport index when init rep

[Saeed Mahameed]

From: Bodong Wang <bodong@mellanox.com>

Driver is referring to the array index when doing rep initialization,
using vport is confusing as it's normally interpreted as vport number.

This patch doesn't change any functionality.

Signed-off-by: Bodong Wang <bodong@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 5124219a31de..f29b9e1f49ae 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1399,7 +1399,7 @@ int esw_offloads_init_reps(struct mlx5_eswitch *esw)
 	struct mlx5_core_dev *dev = esw->dev;
 	struct mlx5_eswitch_rep *rep;
 	u8 hw_id[ETH_ALEN], rep_type;
-	int vport;
+	int vport_index;
 
 	esw->offloads.vport_reps = kcalloc(total_vports,
 					   sizeof(struct mlx5_eswitch_rep),
@@ -1409,8 +1409,8 @@ int esw_offloads_init_reps(struct mlx5_eswitch *esw)
 
 	mlx5_query_nic_vport_mac_address(dev, 0, hw_id);
 
-	mlx5_esw_for_all_reps(esw, vport, rep) {
-		rep->vport = mlx5_eswitch_index_to_vport_num(esw, vport);
+	mlx5_esw_for_all_reps(esw, vport_index, rep) {
+		rep->vport = mlx5_eswitch_index_to_vport_num(esw, vport_index);
 		ether_addr_copy(rep->hw_id, hw_id);
 
 		for (rep_type = 0; rep_type < NUM_REP_TYPES; rep_type++)
-- 
2.21.0

[PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use index of rep for vport to IB port mapping

[Saeed Mahameed]

From: Bodong Wang <bodong@mellanox.com>

In the single IB device mode, the mapping between vport number and
rep relies on a counter. However for dynamic vport allocation, it is
desired to keep consistent map of eswitch vport and IB port.

Hence, simplify code to remove the free running counter and instead
use the available vport index during load/unload sequence from the
eswitch.

Signed-off-by: Bodong Wang <bodong@mellanox.com>
Suggested-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/infiniband/hw/mlx5/ib_rep.c                        | 4 ++--
 drivers/infiniband/hw/mlx5/mlx5_ib.h                       | 1 -
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 1 +
 include/linux/mlx5/eswitch.h                               | 2 ++
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
index d4ed611de35d..da4b936b3219 100644
--- a/drivers/infiniband/hw/mlx5/ib_rep.c
+++ b/drivers/infiniband/hw/mlx5/ib_rep.c
@@ -14,7 +14,7 @@ mlx5_ib_set_vport_rep(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
 	int vport_index;
 
 	ibdev = mlx5_ib_get_uplink_ibdev(dev->priv.eswitch);
-	vport_index = ibdev->free_port++;
+	vport_index = rep->vport_index;
 
 	ibdev->port[vport_index].rep = rep;
 	write_lock(&ibdev->port[vport_index].roce.netdev_lock);
@@ -50,7 +50,7 @@ mlx5_ib_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
 	}
 
 	ibdev->is_rep = true;
-	vport_index = ibdev->free_port++;
+	vport_index = rep->vport_index;
 	ibdev->port[vport_index].rep = rep;
 	ibdev->port[vport_index].roce.netdev =
 		mlx5_ib_get_rep_netdev(dev->priv.eswitch, rep->vport);
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 1c205c2bd486..ee73dc122d28 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -978,7 +978,6 @@ struct mlx5_ib_dev {
 	u16			devx_whitelist_uid;
 	struct mlx5_srq_table   srq_table;
 	struct mlx5_async_ctx   async_ctx;
-	int			free_port;
 };
 
 static inline struct mlx5_ib_cq *to_mibcq(struct mlx5_core_cq *mcq)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index f29b9e1f49ae..7fdea5600383 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1411,6 +1411,7 @@ int esw_offloads_init_reps(struct mlx5_eswitch *esw)
 
 	mlx5_esw_for_all_reps(esw, vport_index, rep) {
 		rep->vport = mlx5_eswitch_index_to_vport_num(esw, vport_index);
+		rep->vport_index = vport_index;
 		ether_addr_copy(rep->hw_id, hw_id);
 
 		for (rep_type = 0; rep_type < NUM_REP_TYPES; rep_type++)
diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h
index d729f5e4d70a..d29abefa03c6 100644
--- a/include/linux/mlx5/eswitch.h
+++ b/include/linux/mlx5/eswitch.h
@@ -46,6 +46,8 @@ struct mlx5_eswitch_rep {
 	u16		       vport;
 	u8		       hw_id[ETH_ALEN];
 	u16		       vlan;
+	/* Only IB rep is using vport_index */
+	u16		       vport_index;
 	u32		       vlan_refcount;
 };
 
-- 
2.21.0

[PATCH mlx5-next 15/15] RDMA/mlx5: Cleanup rep when doing unload

[Saeed Mahameed]

From: Bodong Wang <bodong@mellanox.com>

When an IB rep is loaded, netdev for the same vport is saved for later
reference. However, it's not cleaned up when doing unload. For ECPF,
kernel crashes when driver is referring to the already removed netdev.

Following steps lead to a shown call trace:
1. Create n VFs from host PF
2. Distroy the VFs
3. Run "rdma link" from ARM

Call trace:
  mlx5_ib_get_netdev+0x9c/0xe8 [mlx5_ib]
  mlx5_query_port_roce+0x268/0x558 [mlx5_ib]
  mlx5_ib_rep_query_port+0x14/0x34 [mlx5_ib]
  ib_query_port+0x9c/0xfc [ib_core]
  fill_port_info+0x74/0x28c [ib_core]
  nldev_port_get_doit+0x1a8/0x1e8 [ib_core]
  rdma_nl_rcv_msg+0x16c/0x1c0 [ib_core]
  rdma_nl_rcv+0xe8/0x144 [ib_core]
  netlink_unicast+0x184/0x214
  netlink_sendmsg+0x288/0x354
  sock_sendmsg+0x18/0x2c
  __sys_sendto+0xbc/0x138
  __arm64_sys_sendto+0x28/0x34
  el0_svc_common+0xb0/0x100
  el0_svc_handler+0x6c/0x84
  el0_svc+0x8/0xc

Cleanup the rep and netdev reference when unloading IB rep.

Fixes: 26628e2d58c9 ("RDMA/mlx5: Move to single device multiport ports in switchdev mode")
Signed-off-by: Bodong Wang <bodong@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/infiniband/hw/mlx5/ib_rep.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
index da4b936b3219..a4a54ddebb71 100644
--- a/drivers/infiniband/hw/mlx5/ib_rep.c
+++ b/drivers/infiniband/hw/mlx5/ib_rep.c
@@ -17,6 +17,7 @@ mlx5_ib_set_vport_rep(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
 	vport_index = rep->vport_index;
 
 	ibdev->port[vport_index].rep = rep;
+	rep->rep_data[REP_IB].priv = ibdev;
 	write_lock(&ibdev->port[vport_index].roce.netdev_lock);
 	ibdev->port[vport_index].roce.netdev =
 		mlx5_ib_get_rep_netdev(dev->priv.eswitch, rep->vport);
@@ -68,15 +69,18 @@ mlx5_ib_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
 static void
 mlx5_ib_vport_rep_unload(struct mlx5_eswitch_rep *rep)
 {
-	struct mlx5_ib_dev *dev;
-
-	if (!rep->rep_data[REP_IB].priv ||
-	    rep->vport != MLX5_VPORT_UPLINK)
-		return;
+	struct mlx5_ib_dev *dev = mlx5_ib_rep_to_dev(rep);
+	struct mlx5_ib_port *port;
 
-	dev = mlx5_ib_rep_to_dev(rep);
-	__mlx5_ib_remove(dev, dev->profile, MLX5_IB_STAGE_MAX);
+	port = &dev->port[rep->vport_index];
+	write_lock(&port->roce.netdev_lock);
+	port->roce.netdev = NULL;
+	write_unlock(&port->roce.netdev_lock);
 	rep->rep_data[REP_IB].priv = NULL;
+	port->rep = NULL;
+
+	if (rep->vport == MLX5_VPORT_UPLINK)
+		__mlx5_ib_remove(dev, dev->profile, MLX5_IB_STAGE_MAX);
 }
 
 static void *mlx5_ib_vport_get_proto_dev(struct mlx5_eswitch_rep *rep)
-- 
2.21.0

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Leon Romanovsky]

On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> From: Jianbo Liu <jianbol@mellanox.com>
>
> If vport metadata matching is enabled in eswitch, the rule created
> must be changed to match on the metadata, instead of source port.
>
> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Mark Bloch <markb@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
>  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
>  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
>  3 files changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> index 22e651cb5534..d4ed611de35d 100644
> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
>  	return mlx5_eswitch_vport_rep(esw, vport);
>  }
>
> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> +{
> +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> +}
> +
> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> +						 u16 vport)
> +{
> +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> +}

1. There is no need to introduce one line functions, call to that code directly.
2. It should be bool and not u32.

Thanks

RE: [PATCH mlx5-next 12/15] net/mlx5: E-Switch, Enable vport metadata matching if firmware supports it

[Parav Pandit]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Saeed Mahameed
> Sent: Tuesday, June 18, 2019 12:54 AM
> To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> <leonro@mellanox.com>
> Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> <jianbol@mellanox.com>; Roi Dayan <roid@mellanox.com>; Mark Bloch
> <markb@mellanox.com>
> Subject: [PATCH mlx5-next 12/15] net/mlx5: E-Switch, Enable vport metadata
> matching if firmware supports it
> 
> From: Jianbo Liu <jianbol@mellanox.com>
> 
> As the ingress ACL rules save vhca id and vport number to packet's metadata
> REG_C_0, and the metadata matching for the rules in both fast path and slow
> path are all added, enable this feature if supported.
> 
> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Mark Bloch <markb@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  .../ethernet/mellanox/mlx5/core/eswitch_offloads.c  | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 363517e29d4c..5124219a31de 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -1906,12 +1906,25 @@ static int
> esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
>  	return err;
>  }
> 
> +static int esw_check_vport_match_metadata_supported(struct mlx5_eswitch
> +*esw) {
> +	return (MLX5_CAP_ESW_FLOWTABLE(esw->dev,
> fdb_to_vport_reg_c_id) &
> +		MLX5_FDB_TO_VPORT_REG_C_0) &&
> +	       MLX5_CAP_ESW_FLOWTABLE(esw->dev, flow_source) &&
> +	       MLX5_CAP_ESW(esw->dev, esw_uplink_ingress_acl) &&
> +	       !mlx5_core_is_ecpf_esw_manager(esw->dev) &&
> +	       !mlx5_ecpf_vport_exists(esw->dev);
> +}
> +
struct mlx5_eswitch* should be const.
return type should be bool.

RE: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Parav Pandit]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Saeed Mahameed
> Sent: Tuesday, June 18, 2019 12:53 AM
> To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> <leonro@mellanox.com>
> Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport
> number in VF vports and uplink ingress ACLs
> 
> From: Jianbo Liu <jianbol@mellanox.com>
> 
> When a dual-port VHCA sends a RoCE packet on its non-native port, and the
> packet arrives to its affiliated vport FDB, a mismatch might occur on the rules
> that match the packet source vport as it is not represented by single VHCA only
> in this case. So we change to match on metadata instead of source vport.
> To do that, a rule is created in all vports and uplink ingress ACLs, to save the
> source vport number and vhca id in the packet's metadata in order to match on
> it later.
> The metadata register used is the first of the 32-bit type C registers. It can be
> used for matching and header modify operations. The higher 16 bits of this
> register are for vhca id, and the lower 16 ones is for vport number.
> This change is not for dual-port RoCE only. If HW and FW allow, the vport
> metadata matching is enabled by default.
> 
> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> Reviewed-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Mark Bloch <markb@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
>  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
>  .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
>  include/linux/mlx5/eswitch.h                  |   3 +
>  4 files changed, 161 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index a42a23e505df..1235fd84ae3a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct
> mlx5_eswitch *esw,
> 
>  	vport->ingress.drop_rule = NULL;
>  	vport->ingress.allow_rule = NULL;
> +
> +	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
>  }
> 
>  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw, diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> index 8b9f2cf58e91..4417a195832e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> @@ -68,6 +68,8 @@ struct vport_ingress {
>  	struct mlx5_flow_group *allow_spoofchk_only_grp;
>  	struct mlx5_flow_group *allow_untagged_only_grp;
>  	struct mlx5_flow_group *drop_grp;
> +	int                      modify_metadata_id;
No need for random alignment. Just have one white space after int.

> +	struct mlx5_flow_handle  *modify_metadata_rule;
>  	struct mlx5_flow_handle  *allow_rule;
>  	struct mlx5_flow_handle  *drop_rule;
>  	struct mlx5_fc           *drop_counter;
> @@ -196,6 +198,10 @@ struct mlx5_esw_functions {
>  	u16			num_vfs;
>  };
> 
> +enum {
> +	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0), };
> +
>  struct mlx5_eswitch {
>  	struct mlx5_core_dev    *dev;
>  	struct mlx5_nb          nb;
> @@ -203,6 +209,7 @@ struct mlx5_eswitch {
>  	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
>  	struct workqueue_struct *work_queue;
>  	struct mlx5_vport       *vports;
> +	u32                     flags;
Same as above, no need for extra aligment.

>  	int                     total_vports;
>  	int                     enabled_vports;
>  	/* Synchronize between vport change events @@ -240,6 +247,8 @@
> void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
>  				  struct mlx5_vport *vport);
>  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
>  				   struct mlx5_vport *vport);
> +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
> +					       struct mlx5_vport *vport);
> 
>  /* E-Switch API */
>  int mlx5_eswitch_init(struct mlx5_core_dev *dev); diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 17abb98b48af..871ae44dc132 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -1555,32 +1555,16 @@ static void esw_offloads_devcom_cleanup(struct
> mlx5_eswitch *esw)  static int esw_vport_ingress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  					     struct mlx5_vport *vport)
>  {
> -	struct mlx5_core_dev *dev = esw->dev;
>  	struct mlx5_flow_act flow_act = {0};
>  	struct mlx5_flow_spec *spec;
>  	int err = 0;
> 
>  	/* For prio tag mode, there is only 1 FTEs:
> -	 * 1) Untagged packets - push prio tag VLAN, allow
> +	 * 1) Untagged packets - push prio tag VLAN and modify metadata if
> +	 * required, allow
>  	 * Unmatched traffic is allowed by default
>  	 */
> 
> -	if (!MLX5_CAP_ESW_INGRESS_ACL(dev, ft_support))
> -		return -EOPNOTSUPP;
> -
> -	esw_vport_cleanup_ingress_rules(esw, vport);
> -
> -	err = esw_vport_enable_ingress_acl(esw, vport);
> -	if (err) {
> -		mlx5_core_warn(esw->dev,
> -			       "failed to enable prio tag ingress acl (%d) on
> vport[%d]\n",
> -			       err, vport->vport);
> -		return err;
> -	}
> -
> -	esw_debug(esw->dev,
> -		  "vport[%d] configure ingress rules\n", vport->vport);
> -
>  	spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
>  	if (!spec) {
>  		err = -ENOMEM;
> @@ -1596,6 +1580,12 @@ static int esw_vport_ingress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  	flow_act.vlan[0].ethtype = ETH_P_8021Q;
>  	flow_act.vlan[0].vid = 0;
>  	flow_act.vlan[0].prio = 0;
> +
> +	if (vport->ingress.modify_metadata_rule) {
> +		flow_act.action |=
> MLX5_FLOW_CONTEXT_ACTION_MOD_HDR;
> +		flow_act.modify_id = vport->ingress.modify_metadata_id;
> +	}
> +
>  	vport->ingress.allow_rule =
>  		mlx5_add_flow_rules(vport->ingress.acl, spec,
>  				    &flow_act, NULL, 0);
> @@ -1616,6 +1606,59 @@ static int esw_vport_ingress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  	return err;
>  }
> 
> +static int esw_vport_add_ingress_acl_modify_metadata(struct mlx5_eswitch
> *esw,
> +						     struct mlx5_vport *vport)
> +{
> +	u8 action[MLX5_UN_SZ_BYTES(set_action_in_add_action_in_auto)] =
> {};
> +	struct mlx5_flow_act flow_act = {};
> +	struct mlx5_flow_spec spec = {};
> +	int err = 0;
> +
> +	MLX5_SET(set_action_in, action, action_type,
> MLX5_ACTION_TYPE_SET);
> +	MLX5_SET(set_action_in, action, field,
> MLX5_ACTION_IN_FIELD_METADATA_REG_C_0);
> +	MLX5_SET(set_action_in, action, data,
> +		 mlx5_eswitch_get_vport_metadata_for_match(esw, vport-
> >vport));
> +
> +	err = mlx5_modify_header_alloc(esw->dev,
> MLX5_FLOW_NAMESPACE_ESW_INGRESS,
> +				       1, action, &vport-
> >ingress.modify_metadata_id);
> +
> +	if (err) {
> +		esw_warn(esw->dev,
> +			 "failed to alloc modify header for vport %d ingress acl
> (%d)\n",
> +			 vport->vport, err);
> +		return err;
> +	}
> +
> +	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_MOD_HDR |
> MLX5_FLOW_CONTEXT_ACTION_ALLOW;
> +	flow_act.modify_id = vport->ingress.modify_metadata_id;
> +	vport->ingress.modify_metadata_rule = mlx5_add_flow_rules(vport-
> >ingress.acl,
> +								  &spec,
> &flow_act, NULL, 0);
> +	if (IS_ERR(vport->ingress.modify_metadata_rule)) {
> +		err = PTR_ERR(vport->ingress.modify_metadata_rule);
> +		esw_warn(esw->dev,
> +			 "failed to add setting metadata rule for vport %d
> ingress acl, err(%d)\n",
> +			 vport->vport, err);
> +		vport->ingress.modify_metadata_rule = NULL;
> +		goto out;
> +	}
> +
> +out:
> +	if (err)
> +		mlx5_modify_header_dealloc(esw->dev, vport-
> >ingress.modify_metadata_id);
> +	return err;
> +}
> +
> +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
> +					       struct mlx5_vport *vport)
> +{
> +	if (vport->ingress.modify_metadata_rule) {
> +		mlx5_del_flow_rules(vport->ingress.modify_metadata_rule);
> +		mlx5_modify_header_dealloc(esw->dev,
> +vport->ingress.modify_metadata_id);
> +
> +		vport->ingress.modify_metadata_rule = NULL;
> +	}
> +}
> +
>  static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
>  					    struct mlx5_vport *vport)
>  {
> @@ -1623,6 +1666,9 @@ static int esw_vport_egress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  	struct mlx5_flow_spec *spec;
>  	int err = 0;
> 
> +	if (!MLX5_CAP_GEN(esw->dev, prio_tag_required))
> +		return 0;
> +
>  	/* For prio tag mode, there is only 1 FTEs:
>  	 * 1) prio tag packets - pop the prio tag VLAN, allow
>  	 * Unmatched traffic is allowed by default @@ -1676,27 +1722,77 @@
> static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
>  	return err;
>  }
> 
> -static int esw_prio_tag_acls_config(struct mlx5_eswitch *esw, int nvports)
> +static int esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
> +					   struct mlx5_vport *vport)
>  {
> -	struct mlx5_vport *vport = NULL;
> -	int i, j;
>  	int err;
> 
> -	mlx5_esw_for_each_vf_vport(esw, i, vport, nvports) {
> +	if (!mlx5_eswitch_vport_match_metadata_enabled(esw) &&
> +	    !MLX5_CAP_GEN(esw->dev, prio_tag_required))
> +		return 0;
> +
> +	esw_vport_cleanup_ingress_rules(esw, vport);
> +
> +	err = esw_vport_enable_ingress_acl(esw, vport);
> +	if (err) {
> +		esw_warn(esw->dev,
> +			 "failed to enable ingress acl (%d) on vport[%d]\n",
> +			 err, vport->vport);
> +		return err;
> +	}
> +
> +	esw_debug(esw->dev,
> +		  "vport[%d] configure ingress rules\n", vport->vport);
> +
> +	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
> +		err = esw_vport_add_ingress_acl_modify_metadata(esw,
> vport);
> +		if (err)
> +			goto out;
> +	}
> +
> +	if (MLX5_CAP_GEN(esw->dev, prio_tag_required) &&
> +	    (vport->vport >= MLX5_VPORT_FIRST_VF &&
> +	     vport->vport <= esw->dev->priv.sriov.num_vfs)) {
>  		err = esw_vport_ingress_prio_tag_config(esw, vport);
>  		if (err)
> -			goto err_ingress;
> -		err = esw_vport_egress_prio_tag_config(esw, vport);
> +			goto out;
> +	}
> +
> +out:
> +	if (err)
> +		esw_vport_disable_ingress_acl(esw, vport);
> +	return err;
> +}
> +
> +static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw) {
> +	struct mlx5_vport *vport;
> +	int i, j;
> +	int err;
> +
> +	mlx5_esw_for_all_vports(esw, i, vport) {
> +		err = esw_vport_ingress_common_config(esw, vport);
>  		if (err)
> -			goto err_egress;
> +			goto err_ingress;
> +
> +		if (vport->vport >= MLX5_VPORT_FIRST_VF &&
> +		    vport->vport <= esw->dev->priv.sriov.num_vfs) {
Add an helper API mlx5_esw_is_vport(const struct mlx5_esw *esw, const struct mlx5_vport *vport) 
and use at two places in ingress and egress config.

> +			err = esw_vport_egress_prio_tag_config(esw, vport);
> +			if (err)
> +				goto err_egress;
> +		}
>  	}
> 
> +	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
> +		esw_info(esw->dev, "Use metadata reg_c as source vport to
> match\n");
> +
>  	return 0;
> 
>  err_egress:
>  	esw_vport_disable_ingress_acl(esw, vport);
>  err_ingress:
> -	mlx5_esw_for_each_vf_vport_reverse(esw, j, vport, i - 1) {
> +	for (j = MLX5_VPORT_PF; j < i; j++) {
Keep the reverse order as before.

> +		vport = &esw->vports[j];
>  		esw_vport_disable_egress_acl(esw, vport);
>  		esw_vport_disable_ingress_acl(esw, vport);
>  	}
> @@ -1704,15 +1800,17 @@ static int esw_prio_tag_acls_config(struct
> mlx5_eswitch *esw, int nvports)
>  	return err;
>  }
> 
> -static void esw_prio_tag_acls_cleanup(struct mlx5_eswitch *esw)
> +static void esw_destroy_offloads_acl_tables(struct mlx5_eswitch *esw)
>  {
>  	struct mlx5_vport *vport;
>  	int i;
> 
> -	mlx5_esw_for_each_vf_vport(esw, i, vport, esw->nvports) {
> +	mlx5_esw_for_all_vports(esw, i, vport) {
If you are changing this, please do in reverse order to keep it exact mirror of create/enable sequence.

>  		esw_vport_disable_egress_acl(esw, vport);
>  		esw_vport_disable_ingress_acl(esw, vport);
>  	}
> +
> +	esw->flags &= ~MLX5_ESWITCH_VPORT_MATCH_METADATA;
>  }
> 
>  static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
> @@ -1722,15 +1820,13 @@ static int esw_offloads_steering_init(struct
> mlx5_eswitch *esw, int nvports)
>  	memset(&esw->fdb_table.offloads, 0, sizeof(struct offloads_fdb));
>  	mutex_init(&esw->fdb_table.offloads.fdb_prio_lock);
> 
> -	if (MLX5_CAP_GEN(esw->dev, prio_tag_required)) {
> -		err = esw_prio_tag_acls_config(esw, nvports);
> -		if (err)
> -			return err;
> -	}
> +	err = esw_create_offloads_acl_tables(esw);
> +	if (err)
> +		return err;
> 
>  	err = esw_create_offloads_fdb_tables(esw, nvports);
>  	if (err)
> -		return err;
> +		goto create_fdb_err;
> 
>  	err = esw_create_offloads_table(esw, nvports);
>  	if (err)
> @@ -1748,6 +1844,9 @@ static int esw_offloads_steering_init(struct
> mlx5_eswitch *esw, int nvports)
>  create_ft_err:
>  	esw_destroy_offloads_fdb_tables(esw);
> 
> +create_fdb_err:
> +	esw_destroy_offloads_acl_tables(esw);
> +
>  	return err;
>  }
> 
> @@ -1756,8 +1855,7 @@ static void esw_offloads_steering_cleanup(struct
> mlx5_eswitch *esw)
>  	esw_destroy_vport_rx_group(esw);
>  	esw_destroy_offloads_table(esw);
>  	esw_destroy_offloads_fdb_tables(esw);
> -	if (MLX5_CAP_GEN(esw->dev, prio_tag_required))
> -		esw_prio_tag_acls_cleanup(esw);
> +	esw_destroy_offloads_acl_tables(esw);
>  }
> 
>  static void esw_functions_changed_event_handler(struct work_struct *work)
> @@ -2290,3 +2388,16 @@ struct mlx5_eswitch_rep
> *mlx5_eswitch_vport_rep(struct mlx5_eswitch *esw,
>  	return mlx5_eswitch_get_rep(esw, vport);  }
> EXPORT_SYMBOL(mlx5_eswitch_vport_rep);
> +
> +u32 mlx5_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch
> *esw)
> +{
> +	return esw->flags & MLX5_ESWITCH_VPORT_MATCH_METADATA;
> +}
> +EXPORT_SYMBOL(mlx5_eswitch_vport_match_metadata_enabled);
> +
Return type should book and const *esw.

> +u32 mlx5_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> +					      u16 vport)
> +{
> +	return ((MLX5_CAP_GEN(esw->dev, vhca_id) & 0xffff) << 16) | vport; }
> +EXPORT_SYMBOL(mlx5_eswitch_get_vport_metadata_for_match);
This one too.

> diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h index
> 174eec0871d9..d729f5e4d70a 100644
> --- a/include/linux/mlx5/eswitch.h
> +++ b/include/linux/mlx5/eswitch.h
> @@ -64,6 +64,9 @@ struct mlx5_flow_handle *
> mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw,
>  				    int vport, u32 sqn);
> 
> +u32 mlx5_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch
> +*esw);
> +u32 mlx5_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> +u16 vport);
> +
>  #ifdef CONFIG_MLX5_ESWITCH
>  enum devlink_eswitch_encap_mode
>  mlx5_eswitch_get_encap_mode(const struct mlx5_core_dev *dev);
> --
> 2.21.0

Re: [PATCH mlx5-next 12/15] net/mlx5: E-Switch, Enable vport metadata matching if firmware supports it

[Leon Romanovsky]

On Tue, Jun 18, 2019 at 10:24:49AM +0000, Parav Pandit wrote:
>
>
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Saeed Mahameed
> > Sent: Tuesday, June 18, 2019 12:54 AM
> > To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > <leonro@mellanox.com>
> > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> > <jianbol@mellanox.com>; Roi Dayan <roid@mellanox.com>; Mark Bloch
> > <markb@mellanox.com>
> > Subject: [PATCH mlx5-next 12/15] net/mlx5: E-Switch, Enable vport metadata
> > matching if firmware supports it
> >
> > From: Jianbo Liu <jianbol@mellanox.com>
> >
> > As the ingress ACL rules save vhca id and vport number to packet's metadata
> > REG_C_0, and the metadata matching for the rules in both fast path and slow
> > path are all added, enable this feature if supported.
> >
> > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  .../ethernet/mellanox/mlx5/core/eswitch_offloads.c  | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > index 363517e29d4c..5124219a31de 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > @@ -1906,12 +1906,25 @@ static int
> > esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
> >  	return err;
> >  }
> >
> > +static int esw_check_vport_match_metadata_supported(struct mlx5_eswitch
> > +*esw) {
> > +	return (MLX5_CAP_ESW_FLOWTABLE(esw->dev,
> > fdb_to_vport_reg_c_id) &
> > +		MLX5_FDB_TO_VPORT_REG_C_0) &&
> > +	       MLX5_CAP_ESW_FLOWTABLE(esw->dev, flow_source) &&
> > +	       MLX5_CAP_ESW(esw->dev, esw_uplink_ingress_acl) &&
> > +	       !mlx5_core_is_ecpf_esw_manager(esw->dev) &&
> > +	       !mlx5_ecpf_vport_exists(esw->dev);
> > +}
> > +
> struct mlx5_eswitch* should be const.
> return type should be bool.

It is also completely indigestible and should be break into peaces to
make it readable.

Thanks

Re: [PATCH mlx5-next 15/15] RDMA/mlx5: Cleanup rep when doing unload

[Leon Romanovsky]

On Mon, Jun 17, 2019 at 07:23:39PM +0000, Saeed Mahameed wrote:
> From: Bodong Wang <bodong@mellanox.com>
>
> When an IB rep is loaded, netdev for the same vport is saved for later
> reference. However, it's not cleaned up when doing unload. For ECPF,
> kernel crashes when driver is referring to the already removed netdev.
>
> Following steps lead to a shown call trace:
> 1. Create n VFs from host PF
> 2. Distroy the VFs
> 3. Run "rdma link" from ARM
>
> Call trace:
>   mlx5_ib_get_netdev+0x9c/0xe8 [mlx5_ib]
>   mlx5_query_port_roce+0x268/0x558 [mlx5_ib]
>   mlx5_ib_rep_query_port+0x14/0x34 [mlx5_ib]
>   ib_query_port+0x9c/0xfc [ib_core]
>   fill_port_info+0x74/0x28c [ib_core]
>   nldev_port_get_doit+0x1a8/0x1e8 [ib_core]
>   rdma_nl_rcv_msg+0x16c/0x1c0 [ib_core]
>   rdma_nl_rcv+0xe8/0x144 [ib_core]
>   netlink_unicast+0x184/0x214
>   netlink_sendmsg+0x288/0x354
>   sock_sendmsg+0x18/0x2c
>   __sys_sendto+0xbc/0x138
>   __arm64_sys_sendto+0x28/0x34
>   el0_svc_common+0xb0/0x100
>   el0_svc_handler+0x6c/0x84
>   el0_svc+0x8/0xc
>
> Cleanup the rep and netdev reference when unloading IB rep.
>
> Fixes: 26628e2d58c9 ("RDMA/mlx5: Move to single device multiport ports in switchdev mode")
> Signed-off-by: Bodong Wang <bodong@mellanox.com>
> Reviewed-by: Mark Bloch <markb@mellanox.com>
> Reviewed-by: Parav Pandit <parav@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  drivers/infiniband/hw/mlx5/ib_rep.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

Re: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use index of rep for vport to IB port mapping

[Leon Romanovsky]

On Mon, Jun 17, 2019 at 07:23:37PM +0000, Saeed Mahameed wrote:
> From: Bodong Wang <bodong@mellanox.com>
>
> In the single IB device mode, the mapping between vport number and
> rep relies on a counter. However for dynamic vport allocation, it is
> desired to keep consistent map of eswitch vport and IB port.
>
> Hence, simplify code to remove the free running counter and instead
> use the available vport index during load/unload sequence from the
> eswitch.
>
> Signed-off-by: Bodong Wang <bodong@mellanox.com>
> Suggested-by: Parav Pandit <parav@mellanox.com>
> Reviewed-by: Parav Pandit <parav@mellanox.com>

We are not adding multiple "*-by" for same user, please choose one.

Thanks

RE: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use index of rep for vport to IB port mapping

[Parav Pandit]

Hi Leon,

> -----Original Message-----
> From: Leon Romanovsky
> Sent: Tuesday, June 18, 2019 4:12 PM
> To: Saeed Mahameed <saeedm@mellanox.com>
> Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Bodong Wang
> <bodong@mellanox.com>; Parav Pandit <parav@mellanox.com>; Mark Bloch
> <markb@mellanox.com>
> Subject: Re: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use index of rep
> for vport to IB port mapping
> 
> On Mon, Jun 17, 2019 at 07:23:37PM +0000, Saeed Mahameed wrote:
> > From: Bodong Wang <bodong@mellanox.com>
> >
> > In the single IB device mode, the mapping between vport number and rep
> > relies on a counter. However for dynamic vport allocation, it is
> > desired to keep consistent map of eswitch vport and IB port.
> >
> > Hence, simplify code to remove the free running counter and instead
> > use the available vport index during load/unload sequence from the
> > eswitch.
> >
> > Signed-off-by: Bodong Wang <bodong@mellanox.com>
> > Suggested-by: Parav Pandit <parav@mellanox.com>
> > Reviewed-by: Parav Pandit <parav@mellanox.com>
> 
> We are not adding multiple "*-by" for same user, please choose one.
> 
Suggested-by was added by Bodong during our discussion. Later on when I did gerrit +1, RB tag got added.

> Thanks

RE: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Parav Pandit]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Saeed Mahameed
> Sent: Tuesday, June 18, 2019 12:53 AM
> To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> <leonro@mellanox.com>
> Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport
> number in VF vports and uplink ingress ACLs
> 
> From: Jianbo Liu <jianbol@mellanox.com>
> 
> When a dual-port VHCA sends a RoCE packet on its non-native port, and the
> packet arrives to its affiliated vport FDB, a mismatch might occur on the rules
> that match the packet source vport as it is not represented by single VHCA only
> in this case. So we change to match on metadata instead of source vport.
> To do that, a rule is created in all vports and uplink ingress ACLs, to save the
> source vport number and vhca id in the packet's metadata in order to match on
> it later.
> The metadata register used is the first of the 32-bit type C registers. It can be
> used for matching and header modify operations. The higher 16 bits of this
> register are for vhca id, and the lower 16 ones is for vport number.
> This change is not for dual-port RoCE only. If HW and FW allow, the vport
> metadata matching is enabled by default.
> 
> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> Reviewed-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Mark Bloch <markb@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
>  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
>  .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
>  include/linux/mlx5/eswitch.h                  |   3 +
>  4 files changed, 161 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index a42a23e505df..1235fd84ae3a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct
> mlx5_eswitch *esw,
> 
>  	vport->ingress.drop_rule = NULL;
>  	vport->ingress.allow_rule = NULL;
> +
> +	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
>  }
> 
>  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw, diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> index 8b9f2cf58e91..4417a195832e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> @@ -68,6 +68,8 @@ struct vport_ingress {
>  	struct mlx5_flow_group *allow_spoofchk_only_grp;
>  	struct mlx5_flow_group *allow_untagged_only_grp;
>  	struct mlx5_flow_group *drop_grp;
> +	int                      modify_metadata_id;
> +	struct mlx5_flow_handle  *modify_metadata_rule;
>  	struct mlx5_flow_handle  *allow_rule;
>  	struct mlx5_flow_handle  *drop_rule;
>  	struct mlx5_fc           *drop_counter;
> @@ -196,6 +198,10 @@ struct mlx5_esw_functions {
>  	u16			num_vfs;
>  };
> 
> +enum {
> +	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0), };
> +
>  struct mlx5_eswitch {
>  	struct mlx5_core_dev    *dev;
>  	struct mlx5_nb          nb;
> @@ -203,6 +209,7 @@ struct mlx5_eswitch {
>  	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
>  	struct workqueue_struct *work_queue;
>  	struct mlx5_vport       *vports;
> +	u32                     flags;
>  	int                     total_vports;
>  	int                     enabled_vports;
>  	/* Synchronize between vport change events @@ -240,6 +247,8 @@
> void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
>  				  struct mlx5_vport *vport);
>  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
>  				   struct mlx5_vport *vport);
> +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
> +					       struct mlx5_vport *vport);
> 
>  /* E-Switch API */
>  int mlx5_eswitch_init(struct mlx5_core_dev *dev); diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 17abb98b48af..871ae44dc132 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -1555,32 +1555,16 @@ static void esw_offloads_devcom_cleanup(struct
> mlx5_eswitch *esw)  static int esw_vport_ingress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  					     struct mlx5_vport *vport)
>  {
> -	struct mlx5_core_dev *dev = esw->dev;
>  	struct mlx5_flow_act flow_act = {0};
>  	struct mlx5_flow_spec *spec;
>  	int err = 0;
> 
>  	/* For prio tag mode, there is only 1 FTEs:
> -	 * 1) Untagged packets - push prio tag VLAN, allow
> +	 * 1) Untagged packets - push prio tag VLAN and modify metadata if
> +	 * required, allow
>  	 * Unmatched traffic is allowed by default
>  	 */
> 
> -	if (!MLX5_CAP_ESW_INGRESS_ACL(dev, ft_support))
> -		return -EOPNOTSUPP;
> -
> -	esw_vport_cleanup_ingress_rules(esw, vport);
> -
> -	err = esw_vport_enable_ingress_acl(esw, vport);
> -	if (err) {
> -		mlx5_core_warn(esw->dev,
> -			       "failed to enable prio tag ingress acl (%d) on
> vport[%d]\n",
> -			       err, vport->vport);
> -		return err;
> -	}
> -
> -	esw_debug(esw->dev,
> -		  "vport[%d] configure ingress rules\n", vport->vport);
> -
>  	spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
>  	if (!spec) {
>  		err = -ENOMEM;
> @@ -1596,6 +1580,12 @@ static int esw_vport_ingress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  	flow_act.vlan[0].ethtype = ETH_P_8021Q;
>  	flow_act.vlan[0].vid = 0;
>  	flow_act.vlan[0].prio = 0;
> +
> +	if (vport->ingress.modify_metadata_rule) {
> +		flow_act.action |=
> MLX5_FLOW_CONTEXT_ACTION_MOD_HDR;
> +		flow_act.modify_id = vport->ingress.modify_metadata_id;
> +	}
> +
>  	vport->ingress.allow_rule =
>  		mlx5_add_flow_rules(vport->ingress.acl, spec,
>  				    &flow_act, NULL, 0);
> @@ -1616,6 +1606,59 @@ static int esw_vport_ingress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  	return err;
>  }
> 
> +static int esw_vport_add_ingress_acl_modify_metadata(struct mlx5_eswitch
> *esw,
> +						     struct mlx5_vport *vport)
> +{
> +	u8 action[MLX5_UN_SZ_BYTES(set_action_in_add_action_in_auto)] =
> {};
> +	struct mlx5_flow_act flow_act = {};
> +	struct mlx5_flow_spec spec = {};
> +	int err = 0;
> +
> +	MLX5_SET(set_action_in, action, action_type,
> MLX5_ACTION_TYPE_SET);
> +	MLX5_SET(set_action_in, action, field,
> MLX5_ACTION_IN_FIELD_METADATA_REG_C_0);
> +	MLX5_SET(set_action_in, action, data,
> +		 mlx5_eswitch_get_vport_metadata_for_match(esw, vport-
> >vport));
> +
> +	err = mlx5_modify_header_alloc(esw->dev,
> MLX5_FLOW_NAMESPACE_ESW_INGRESS,
> +				       1, action, &vport-
> >ingress.modify_metadata_id);
> +
Please remove the empty line.
> +	if (err) {
> +		esw_warn(esw->dev,
> +			 "failed to alloc modify header for vport %d ingress acl
> (%d)\n",
> +			 vport->vport, err);
> +		return err;
> +	}
> +
> +	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_MOD_HDR |
> MLX5_FLOW_CONTEXT_ACTION_ALLOW;
> +	flow_act.modify_id = vport->ingress.modify_metadata_id;
> +	vport->ingress.modify_metadata_rule = mlx5_add_flow_rules(vport-
> >ingress.acl,
> +								  &spec,
> &flow_act, NULL, 0);
> +	if (IS_ERR(vport->ingress.modify_metadata_rule)) {
> +		err = PTR_ERR(vport->ingress.modify_metadata_rule);
> +		esw_warn(esw->dev,
> +			 "failed to add setting metadata rule for vport %d
> ingress acl, err(%d)\n",
> +			 vport->vport, err);
> +		vport->ingress.modify_metadata_rule = NULL;
> +		goto out;
> +	}
> +
> +out:
> +	if (err)
> +		mlx5_modify_header_dealloc(esw->dev, vport-
> >ingress.modify_metadata_id);
> +	return err;
> +}
> +
> +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
> +					       struct mlx5_vport *vport)
> +{
> +	if (vport->ingress.modify_metadata_rule) {
> +		mlx5_del_flow_rules(vport->ingress.modify_metadata_rule);
> +		mlx5_modify_header_dealloc(esw->dev,
> +vport->ingress.modify_metadata_id);
> +
> +		vport->ingress.modify_metadata_rule = NULL;
> +	}
> +}
> +
>  static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
>  					    struct mlx5_vport *vport)
>  {
> @@ -1623,6 +1666,9 @@ static int esw_vport_egress_prio_tag_config(struct
> mlx5_eswitch *esw,
>  	struct mlx5_flow_spec *spec;
>  	int err = 0;
> 
> +	if (!MLX5_CAP_GEN(esw->dev, prio_tag_required))
> +		return 0;
> +
>  	/* For prio tag mode, there is only 1 FTEs:
>  	 * 1) prio tag packets - pop the prio tag VLAN, allow
>  	 * Unmatched traffic is allowed by default @@ -1676,27 +1722,77 @@
> static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
>  	return err;
>  }
> 
> -static int esw_prio_tag_acls_config(struct mlx5_eswitch *esw, int nvports)
> +static int esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
> +					   struct mlx5_vport *vport)
>  {
> -	struct mlx5_vport *vport = NULL;
> -	int i, j;
>  	int err;
> 
> -	mlx5_esw_for_each_vf_vport(esw, i, vport, nvports) {
> +	if (!mlx5_eswitch_vport_match_metadata_enabled(esw) &&
> +	    !MLX5_CAP_GEN(esw->dev, prio_tag_required))
> +		return 0;
> +
> +	esw_vport_cleanup_ingress_rules(esw, vport);
> +
> +	err = esw_vport_enable_ingress_acl(esw, vport);
> +	if (err) {
> +		esw_warn(esw->dev,
> +			 "failed to enable ingress acl (%d) on vport[%d]\n",
> +			 err, vport->vport);
> +		return err;
> +	}
> +
> +	esw_debug(esw->dev,
> +		  "vport[%d] configure ingress rules\n", vport->vport);
> +
> +	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
> +		err = esw_vport_add_ingress_acl_modify_metadata(esw,
> vport);
> +		if (err)
> +			goto out;
> +	}
> +
> +	if (MLX5_CAP_GEN(esw->dev, prio_tag_required) &&
> +	    (vport->vport >= MLX5_VPORT_FIRST_VF &&
> +	     vport->vport <= esw->dev->priv.sriov.num_vfs)) {
>  		err = esw_vport_ingress_prio_tag_config(esw, vport);
>  		if (err)
> -			goto err_ingress;
> -		err = esw_vport_egress_prio_tag_config(esw, vport);
> +			goto out;
> +	}
> +
> +out:
> +	if (err)
> +		esw_vport_disable_ingress_acl(esw, vport);
> +	return err;
> +}
> +
> +static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw) {
> +	struct mlx5_vport *vport;
> +	int i, j;
> +	int err;
> +
> +	mlx5_esw_for_all_vports(esw, i, vport) {
> +		err = esw_vport_ingress_common_config(esw, vport);
>  		if (err)
> -			goto err_egress;
> +			goto err_ingress;
> +
> +		if (vport->vport >= MLX5_VPORT_FIRST_VF &&
> +		    vport->vport <= esw->dev->priv.sriov.num_vfs) {
> +			err = esw_vport_egress_prio_tag_config(esw, vport);
> +			if (err)
> +				goto err_egress;
> +		}
>  	}
> 
> +	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
> +		esw_info(esw->dev, "Use metadata reg_c as source vport to
> match\n");
> +
>  	return 0;
> 
>  err_egress:
>  	esw_vport_disable_ingress_acl(esw, vport);
>  err_ingress:
> -	mlx5_esw_for_each_vf_vport_reverse(esw, j, vport, i - 1) {
> +	for (j = MLX5_VPORT_PF; j < i; j++) {
> +		vport = &esw->vports[j];
>  		esw_vport_disable_egress_acl(esw, vport);
>  		esw_vport_disable_ingress_acl(esw, vport);
>  	}
> @@ -1704,15 +1800,17 @@ static int esw_prio_tag_acls_config(struct
> mlx5_eswitch *esw, int nvports)
>  	return err;
>  }
> 
> -static void esw_prio_tag_acls_cleanup(struct mlx5_eswitch *esw)
> +static void esw_destroy_offloads_acl_tables(struct mlx5_eswitch *esw)
>  {
>  	struct mlx5_vport *vport;
>  	int i;
> 
> -	mlx5_esw_for_each_vf_vport(esw, i, vport, esw->nvports) {
> +	mlx5_esw_for_all_vports(esw, i, vport) {
>  		esw_vport_disable_egress_acl(esw, vport);
>  		esw_vport_disable_ingress_acl(esw, vport);
>  	}
> +
> +	esw->flags &= ~MLX5_ESWITCH_VPORT_MATCH_METADATA;
>  }
> 
>  static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
> @@ -1722,15 +1820,13 @@ static int esw_offloads_steering_init(struct
> mlx5_eswitch *esw, int nvports)
>  	memset(&esw->fdb_table.offloads, 0, sizeof(struct offloads_fdb));
>  	mutex_init(&esw->fdb_table.offloads.fdb_prio_lock);
> 
> -	if (MLX5_CAP_GEN(esw->dev, prio_tag_required)) {
> -		err = esw_prio_tag_acls_config(esw, nvports);
> -		if (err)
> -			return err;
> -	}
> +	err = esw_create_offloads_acl_tables(esw);
> +	if (err)
> +		return err;
> 
>  	err = esw_create_offloads_fdb_tables(esw, nvports);
>  	if (err)
> -		return err;
> +		goto create_fdb_err;
> 
>  	err = esw_create_offloads_table(esw, nvports);
>  	if (err)
> @@ -1748,6 +1844,9 @@ static int esw_offloads_steering_init(struct
> mlx5_eswitch *esw, int nvports)
>  create_ft_err:
>  	esw_destroy_offloads_fdb_tables(esw);
> 
> +create_fdb_err:
> +	esw_destroy_offloads_acl_tables(esw);
> +
>  	return err;
>  }
> 
> @@ -1756,8 +1855,7 @@ static void esw_offloads_steering_cleanup(struct
> mlx5_eswitch *esw)
>  	esw_destroy_vport_rx_group(esw);
>  	esw_destroy_offloads_table(esw);
>  	esw_destroy_offloads_fdb_tables(esw);
> -	if (MLX5_CAP_GEN(esw->dev, prio_tag_required))
> -		esw_prio_tag_acls_cleanup(esw);
> +	esw_destroy_offloads_acl_tables(esw);
>  }
> 
>  static void esw_functions_changed_event_handler(struct work_struct *work)
> @@ -2290,3 +2388,16 @@ struct mlx5_eswitch_rep
> *mlx5_eswitch_vport_rep(struct mlx5_eswitch *esw,
>  	return mlx5_eswitch_get_rep(esw, vport);  }
> EXPORT_SYMBOL(mlx5_eswitch_vport_rep);
> +
> +u32 mlx5_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch
> *esw)
> +{
> +	return esw->flags & MLX5_ESWITCH_VPORT_MATCH_METADATA;
> +}
> +EXPORT_SYMBOL(mlx5_eswitch_vport_match_metadata_enabled);
> +
> +u32 mlx5_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> +					      u16 vport)
> +{
> +	return ((MLX5_CAP_GEN(esw->dev, vhca_id) & 0xffff) << 16) | vport; }
> +EXPORT_SYMBOL(mlx5_eswitch_get_vport_metadata_for_match);
> diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h index
> 174eec0871d9..d729f5e4d70a 100644
> --- a/include/linux/mlx5/eswitch.h
> +++ b/include/linux/mlx5/eswitch.h
> @@ -64,6 +64,9 @@ struct mlx5_flow_handle *
> mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw,
>  				    int vport, u32 sqn);
> 
> +u32 mlx5_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch
> +*esw);
> +u32 mlx5_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> +u16 vport);
> +
As you might be aware that vport_num and vport_index are not one to one map, we have few bugs in that area.
To avoid confusion to developers and users of the API, please name any new API as either vport_num or vport_index.
This makes it clear and explicit, specially global APIs like this in include directory.

>  #ifdef CONFIG_MLX5_ESWITCH
>  enum devlink_eswitch_encap_mode
>  mlx5_eswitch_get_encap_mode(const struct mlx5_core_dev *dev);
> --
> 2.21.0

Re: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use index of rep for vport to IB port mapping

[Saeed Mahameed]

On Tue, 2019-06-18 at 10:47 +0000, Parav Pandit wrote:
> Hi Leon,
> 
> > -----Original Message-----
> > From: Leon Romanovsky
> > Sent: Tuesday, June 18, 2019 4:12 PM
> > To: Saeed Mahameed <saeedm@mellanox.com>
> > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Bodong Wang
> > <bodong@mellanox.com>; Parav Pandit <parav@mellanox.com>; Mark
> > Bloch
> > <markb@mellanox.com>
> > Subject: Re: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use
> > index of rep
> > for vport to IB port mapping
> > 
> > On Mon, Jun 17, 2019 at 07:23:37PM +0000, Saeed Mahameed wrote:
> > > From: Bodong Wang <bodong@mellanox.com>
> > > 
> > > In the single IB device mode, the mapping between vport number
> > > and rep
> > > relies on a counter. However for dynamic vport allocation, it is
> > > desired to keep consistent map of eswitch vport and IB port.
> > > 
> > > Hence, simplify code to remove the free running counter and
> > > instead
> > > use the available vport index during load/unload sequence from
> > > the
> > > eswitch.
> > > 
> > > Signed-off-by: Bodong Wang <bodong@mellanox.com>
> > > Suggested-by: Parav Pandit <parav@mellanox.com>
> > > Reviewed-by: Parav Pandit <parav@mellanox.com>
> > 
> > We are not adding multiple "*-by" for same user, please choose one.
> > 
> Suggested-by was added by Bodong during our discussion. Later on when
> I did gerrit +1, RB tag got added.
> 

Is there a rule against having multiple "*-by" ? i don't think so  and
there shouldn't be, users need to get the exact amount of recognition
as the amount of work they put into this patch, if they reviewed and
tested a patch they deserve two tags .. 

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Jianbo Liu]

The 06/18/2019 18:19, Leon Romanovsky wrote:
> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> > From: Jianbo Liu <jianbol@mellanox.com>
> >
> > If vport metadata matching is enabled in eswitch, the rule created
> > must be changed to match on the metadata, instead of source port.
> >
> > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> >  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> >  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
> >  3 files changed, 63 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> > index 22e651cb5534..d4ed611de35d 100644
> > --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> >  	return mlx5_eswitch_vport_rep(esw, vport);
> >  }
> >
> > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> > +{
> > +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> > +}
> > +
> > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> > +						 u16 vport)
> > +{
> > +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> > +}
> 
> 1. There is no need to introduce one line functions, call to that code directly.

No. They are in IB, and we don't want them be mixed up by the original
functions in eswitch. Please ask Mark more about it.

> 2. It should be bool and not u32.
> 
> Thanks

-- 

Re: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use index of rep for vport to IB port mapping

[Leon Romanovsky]

On Tue, Jun 18, 2019 at 06:25:46PM +0000, Saeed Mahameed wrote:
> On Tue, 2019-06-18 at 10:47 +0000, Parav Pandit wrote:
> > Hi Leon,
> >
> > > -----Original Message-----
> > > From: Leon Romanovsky
> > > Sent: Tuesday, June 18, 2019 4:12 PM
> > > To: Saeed Mahameed <saeedm@mellanox.com>
> > > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Bodong Wang
> > > <bodong@mellanox.com>; Parav Pandit <parav@mellanox.com>; Mark
> > > Bloch
> > > <markb@mellanox.com>
> > > Subject: Re: [PATCH mlx5-next 14/15] {IB, net}/mlx5: E-Switch, Use
> > > index of rep
> > > for vport to IB port mapping
> > >
> > > On Mon, Jun 17, 2019 at 07:23:37PM +0000, Saeed Mahameed wrote:
> > > > From: Bodong Wang <bodong@mellanox.com>
> > > >
> > > > In the single IB device mode, the mapping between vport number
> > > > and rep
> > > > relies on a counter. However for dynamic vport allocation, it is
> > > > desired to keep consistent map of eswitch vport and IB port.
> > > >
> > > > Hence, simplify code to remove the free running counter and
> > > > instead
> > > > use the available vport index during load/unload sequence from
> > > > the
> > > > eswitch.
> > > >
> > > > Signed-off-by: Bodong Wang <bodong@mellanox.com>
> > > > Suggested-by: Parav Pandit <parav@mellanox.com>
> > > > Reviewed-by: Parav Pandit <parav@mellanox.com>
> > >
> > > We are not adding multiple "*-by" for same user, please choose one.
> > >
> > Suggested-by was added by Bodong during our discussion. Later on when
> > I did gerrit +1, RB tag got added.
> >
>
> Is there a rule against having multiple "*-by" ? i don't think so  and
> there shouldn't be, users need to get the exact amount of recognition
> as the amount of work they put into this patch, if they reviewed and
> tested a patch they deserve two tags ..

Not everything in the world has and needs rules, sometimes common sense
is enough. It goes without saying that during internal review process,
developer suggested something. Recognition comes in many ways in the
kernel but definitely not by number of tags with specific developer
name on it, especially if this developer comes from same company
as patch author.

If we extend your claim, both you and me should add this type of
signature block for almost every patch which we submit:

Reviewed-by: ....
Tested-by:  ....
Suggested-by: ...
Signed-by: ...

Thanks

>
>

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Leon Romanovsky]

On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
> The 06/18/2019 18:19, Leon Romanovsky wrote:
> > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> > > From: Jianbo Liu <jianbol@mellanox.com>
> > >
> > > If vport metadata matching is enabled in eswitch, the rule created
> > > must be changed to match on the metadata, instead of source port.
> > >
> > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > > ---
> > >  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> > >  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> > >  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
> > >  3 files changed, 63 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> > > index 22e651cb5534..d4ed611de35d 100644
> > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> > >  	return mlx5_eswitch_vport_rep(esw, vport);
> > >  }
> > >
> > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> > > +{
> > > +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> > > +}
> > > +
> > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> > > +						 u16 vport)
> > > +{
> > > +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> > > +}
> >
> > 1. There is no need to introduce one line functions, call to that code directly.
>
> No. They are in IB, and we don't want them be mixed up by the original
> functions in eswitch. Please ask Mark more about it.

Please enlighten me.

>
> > 2. It should be bool and not u32.
> >
> > Thanks
>
> --

Re: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Jianbo Liu]

The 06/18/2019 18:31, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > Behalf Of Saeed Mahameed
> > Sent: Tuesday, June 18, 2019 12:53 AM
> > To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > <leonro@mellanox.com>
> > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> > <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> > <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> > Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport
> > number in VF vports and uplink ingress ACLs
> > 
> > From: Jianbo Liu <jianbol@mellanox.com>
> > 
> > When a dual-port VHCA sends a RoCE packet on its non-native port, and the
> > packet arrives to its affiliated vport FDB, a mismatch might occur on the rules
> > that match the packet source vport as it is not represented by single VHCA only
> > in this case. So we change to match on metadata instead of source vport.
> > To do that, a rule is created in all vports and uplink ingress ACLs, to save the
> > source vport number and vhca id in the packet's metadata in order to match on
> > it later.
> > The metadata register used is the first of the 32-bit type C registers. It can be
> > used for matching and header modify operations. The higher 16 bits of this
> > register are for vhca id, and the lower 16 ones is for vport number.
> > This change is not for dual-port RoCE only. If HW and FW allow, the vport
> > metadata matching is enabled by default.
> > 
> > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > Reviewed-by: Eli Britstein <elibr@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
> >  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
> >  .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
> >  include/linux/mlx5/eswitch.h                  |   3 +
> >  4 files changed, 161 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > index a42a23e505df..1235fd84ae3a 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > @@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct
> > mlx5_eswitch *esw,
> > 
> >  	vport->ingress.drop_rule = NULL;
> >  	vport->ingress.allow_rule = NULL;
> > +
> > +	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
> >  }
> > 
> >  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw, diff --git
> > a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > index 8b9f2cf58e91..4417a195832e 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > @@ -68,6 +68,8 @@ struct vport_ingress {
> >  	struct mlx5_flow_group *allow_spoofchk_only_grp;
> >  	struct mlx5_flow_group *allow_untagged_only_grp;
> >  	struct mlx5_flow_group *drop_grp;
> > +	int                      modify_metadata_id;
> No need for random alignment. Just have one white space after int.

Not random. It's to align with other lines in the this structure.
There are also other fileds with more than one spaces after type.
It looks ugly if there are different styles in the same structure.

> 
> > +	struct mlx5_flow_handle  *modify_metadata_rule;
> >  	struct mlx5_flow_handle  *allow_rule;
> >  	struct mlx5_flow_handle  *drop_rule;
> >  	struct mlx5_fc           *drop_counter;
> > @@ -196,6 +198,10 @@ struct mlx5_esw_functions {
> >  	u16			num_vfs;
> >  };
> > 
> > +enum {
> > +	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0), };
> > +
> >  struct mlx5_eswitch {
> >  	struct mlx5_core_dev    *dev;
> >  	struct mlx5_nb          nb;
> > @@ -203,6 +209,7 @@ struct mlx5_eswitch {
> >  	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
> >  	struct workqueue_struct *work_queue;
> >  	struct mlx5_vport       *vports;
> > +	u32                     flags;
> Same as above, no need for extra aligment.

Same reason.

> 
> >  	int                     total_vports;
> >  	int                     enabled_vports;
> >  	/* Synchronize between vport change events @@ -240,6 +247,8 @@
> > void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
> >  				  struct mlx5_vport *vport);
> >  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
> >  				   struct mlx5_vport *vport);
> > +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
> > +					       struct mlx5_vport *vport);
> > 
> >  /* E-Switch API */
> >  int mlx5_eswitch_init(struct mlx5_core_dev *dev); diff --git
> > a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > index 17abb98b48af..871ae44dc132 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c

...

> > +static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw) {
> > +	struct mlx5_vport *vport;
> > +	int i, j;
> > +	int err;
> > +
> > +	mlx5_esw_for_all_vports(esw, i, vport) {
> > +		err = esw_vport_ingress_common_config(esw, vport);
> >  		if (err)
> > -			goto err_egress;
> > +			goto err_ingress;
> > +
> > +		if (vport->vport >= MLX5_VPORT_FIRST_VF &&
> > +		    vport->vport <= esw->dev->priv.sriov.num_vfs) {
> Add an helper API mlx5_esw_is_vport(const struct mlx5_esw *esw, const struct mlx5_vport *vport) 
> and use at two places in ingress and egress config.

It's very simple logic, but new API make things complicated. If adding
mlx5_esw_is_vport() as you suggested, no one can know what's the meaning
of this function from name, and need to check the implementation again,
which will waste too much time.

> 

RE: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Parav Pandit]

> -----Original Message-----
> From: Jianbo Liu
> Sent: Wednesday, June 19, 2019 10:42 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> <leonro@mellanox.com>; netdev@vger.kernel.org; linux-
> rdma@vger.kernel.org; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> Subject: Re: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with
> vport number in VF vports and uplink ingress ACLs
> 
> The 06/18/2019 18:31, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > Behalf Of Saeed Mahameed
> > > Sent: Tuesday, June 18, 2019 12:53 AM
> > > To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > > <leonro@mellanox.com>
> > > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> > > <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi
> > > Dayan <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> > > Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with
> > > vport number in VF vports and uplink ingress ACLs
> > >
> > > From: Jianbo Liu <jianbol@mellanox.com>
> > >
> > > When a dual-port VHCA sends a RoCE packet on its non-native port,
> > > and the packet arrives to its affiliated vport FDB, a mismatch might
> > > occur on the rules that match the packet source vport as it is not
> > > represented by single VHCA only in this case. So we change to match on
> metadata instead of source vport.
> > > To do that, a rule is created in all vports and uplink ingress ACLs,
> > > to save the source vport number and vhca id in the packet's metadata
> > > in order to match on it later.
> > > The metadata register used is the first of the 32-bit type C
> > > registers. It can be used for matching and header modify operations.
> > > The higher 16 bits of this register are for vhca id, and the lower 16 ones is
> for vport number.
> > > This change is not for dual-port RoCE only. If HW and FW allow, the
> > > vport metadata matching is enabled by default.
> > >
> > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > > Reviewed-by: Eli Britstein <elibr@mellanox.com>
> > > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > > ---
> > >  .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
> > >  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
> > >  .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
> > >  include/linux/mlx5/eswitch.h                  |   3 +
> > >  4 files changed, 161 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > index a42a23e505df..1235fd84ae3a 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > @@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct
> > > mlx5_eswitch *esw,
> > >
> > >  	vport->ingress.drop_rule = NULL;
> > >  	vport->ingress.allow_rule = NULL;
> > > +
> > > +	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
> > >  }
> > >
> > >  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw, diff
> > > --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > index 8b9f2cf58e91..4417a195832e 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > @@ -68,6 +68,8 @@ struct vport_ingress {
> > >  	struct mlx5_flow_group *allow_spoofchk_only_grp;
> > >  	struct mlx5_flow_group *allow_untagged_only_grp;
> > >  	struct mlx5_flow_group *drop_grp;
> > > +	int                      modify_metadata_id;
> > No need for random alignment. Just have one white space after int.
> 
> Not random. It's to align with other lines in the this structure.
> There are also other fileds with more than one spaces after type.
> It looks ugly if there are different styles in the same structure.
> 
Whatever was done in past was done.
There will be mixed alignment anyway.

> >
> > > +	struct mlx5_flow_handle  *modify_metadata_rule;
> > >  	struct mlx5_flow_handle  *allow_rule;
> > >  	struct mlx5_flow_handle  *drop_rule;
> > >  	struct mlx5_fc           *drop_counter;
> > > @@ -196,6 +198,10 @@ struct mlx5_esw_functions {
> > >  	u16			num_vfs;
> > >  };
> > >
> > > +enum {
> > > +	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0), };
> > > +
> > >  struct mlx5_eswitch {
> > >  	struct mlx5_core_dev    *dev;
> > >  	struct mlx5_nb          nb;
> > > @@ -203,6 +209,7 @@ struct mlx5_eswitch {
> > >  	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
> > >  	struct workqueue_struct *work_queue;
> > >  	struct mlx5_vport       *vports;
> > > +	u32                     flags;
> > Same as above, no need for extra aligment.
> 
> Same reason.
> 
> >
> > >  	int                     total_vports;
> > >  	int                     enabled_vports;
> > >  	/* Synchronize between vport change events @@ -240,6 +247,8 @@
> > > void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
> > >  				  struct mlx5_vport *vport);
> > >  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
> > >  				   struct mlx5_vport *vport);
> > > +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch
> *esw,
> > > +					       struct mlx5_vport *vport);
> > >
> > >  /* E-Switch API */
> > >  int mlx5_eswitch_init(struct mlx5_core_dev *dev); diff --git
> > > a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > > index 17abb98b48af..871ae44dc132 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> 
> ...
> 
> > > +static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw) {
> > > +	struct mlx5_vport *vport;
> > > +	int i, j;
> > > +	int err;
> > > +
> > > +	mlx5_esw_for_all_vports(esw, i, vport) {
> > > +		err = esw_vport_ingress_common_config(esw, vport);
> > >  		if (err)
> > > -			goto err_egress;
> > > +			goto err_ingress;
> > > +
> > > +		if (vport->vport >= MLX5_VPORT_FIRST_VF &&
> > > +		    vport->vport <= esw->dev->priv.sriov.num_vfs) {
> > Add an helper API mlx5_esw_is_vport(const struct mlx5_esw *esw, const
> > struct mlx5_vport *vport) and use at two places in ingress and egress config.
> 
> It's very simple logic, but new API make things complicated.

No. it doesn't. Right API name is,
mlx5_esw_is_vf_vport().
mlx5_esw_is_vf_rep()...
etc.

> If adding mlx5_esw_is_vport() as you suggested, no one can know what's the meaning of
> this function from name, and need to check the implementation again, which
> will waste too much time.
> 
mlx5_esw_is_vf_vport() is self-explanatory name which won't waste time.

I am already having this API in my two series, but since yours is already out, it make sense to introduce in this patch.

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Jianbo Liu]

The 06/19/2019 13:04, Leon Romanovsky wrote:
> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
> > The 06/18/2019 18:19, Leon Romanovsky wrote:
> > > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> > > > From: Jianbo Liu <jianbol@mellanox.com>
> > > >
> > > > If vport metadata matching is enabled in eswitch, the rule created
> > > > must be changed to match on the metadata, instead of source port.
> > > >
> > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > > > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > > > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > > > ---
> > > >  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> > > >  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> > > >  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
> > > >  3 files changed, 63 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> > > > index 22e651cb5534..d4ed611de35d 100644
> > > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> > > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> > > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> > > >  	return mlx5_eswitch_vport_rep(esw, vport);
> > > >  }
> > > >
> > > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> > > > +{
> > > > +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> > > > +}
> > > > +
> > > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> > > > +						 u16 vport)
> > > > +{
> > > > +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> > > > +}
> > >
> > > 1. There is no need to introduce one line functions, call to that code directly.
> >
> > No. They are in IB, and we don't want them be mixed up by the original
> > functions in eswitch. Please ask Mark more about it.
> 
> Please enlighten me.

It was suggested by Mark in prevouis review.
I think it's because there are in different modules, and better to with
different names, so introduce there extra one line functions.
Please correct me if I'm wrong, Mark...

> 
> >
> > > 2. It should be bool and not u32.
> > >
> > > Thanks
> >
> > --

-- 

Re: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Jianbo Liu]

The 06/19/2019 13:42, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: Jianbo Liu
> > Sent: Wednesday, June 19, 2019 10:42 AM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > <leonro@mellanox.com>; netdev@vger.kernel.org; linux-
> > rdma@vger.kernel.org; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> > <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> > Subject: Re: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with
> > vport number in VF vports and uplink ingress ACLs
> > 
> > The 06/18/2019 18:31, Parav Pandit wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > > Behalf Of Saeed Mahameed
> > > > Sent: Tuesday, June 18, 2019 12:53 AM
> > > > To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > > > <leonro@mellanox.com>
> > > > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> > > > <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi
> > > > Dayan <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> > > > Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with
> > > > vport number in VF vports and uplink ingress ACLs
> > > >
> > > > From: Jianbo Liu <jianbol@mellanox.com>
> > > >
> > > > When a dual-port VHCA sends a RoCE packet on its non-native port,
> > > > and the packet arrives to its affiliated vport FDB, a mismatch might
> > > > occur on the rules that match the packet source vport as it is not
> > > > represented by single VHCA only in this case. So we change to match on
> > metadata instead of source vport.
> > > > To do that, a rule is created in all vports and uplink ingress ACLs,
> > > > to save the source vport number and vhca id in the packet's metadata
> > > > in order to match on it later.
> > > > The metadata register used is the first of the 32-bit type C
> > > > registers. It can be used for matching and header modify operations.
> > > > The higher 16 bits of this register are for vhca id, and the lower 16 ones is
> > for vport number.
> > > > This change is not for dual-port RoCE only. If HW and FW allow, the
> > > > vport metadata matching is enabled by default.
> > > >
> > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > > > Reviewed-by: Eli Britstein <elibr@mellanox.com>
> > > > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > > > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > > > ---
> > > >  .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
> > > >  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
> > > >  .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
> > > >  include/linux/mlx5/eswitch.h                  |   3 +
> > > >  4 files changed, 161 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > > index a42a23e505df..1235fd84ae3a 100644
> > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > > @@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct
> > > > mlx5_eswitch *esw,
> > > >
> > > >  	vport->ingress.drop_rule = NULL;
> > > >  	vport->ingress.allow_rule = NULL;
> > > > +
> > > > +	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
> > > >  }
> > > >
> > > >  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw, diff
> > > > --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > > index 8b9f2cf58e91..4417a195832e 100644
> > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> > > > @@ -68,6 +68,8 @@ struct vport_ingress {
> > > >  	struct mlx5_flow_group *allow_spoofchk_only_grp;
> > > >  	struct mlx5_flow_group *allow_untagged_only_grp;
> > > >  	struct mlx5_flow_group *drop_grp;
> > > > +	int                      modify_metadata_id;
> > > No need for random alignment. Just have one white space after int.
> > 
> > Not random. It's to align with other lines in the this structure.
> > There are also other fileds with more than one spaces after type.
> > It looks ugly if there are different styles in the same structure.
> > 
> Whatever was done in past was done.
> There will be mixed alignment anyway.
> 
> > >
> > > > +	struct mlx5_flow_handle  *modify_metadata_rule;
> > > >  	struct mlx5_flow_handle  *allow_rule;
> > > >  	struct mlx5_flow_handle  *drop_rule;
> > > >  	struct mlx5_fc           *drop_counter;
> > > > @@ -196,6 +198,10 @@ struct mlx5_esw_functions {
> > > >  	u16			num_vfs;
> > > >  };
> > > >
> > > > +enum {
> > > > +	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0), };
> > > > +
> > > >  struct mlx5_eswitch {
> > > >  	struct mlx5_core_dev    *dev;
> > > >  	struct mlx5_nb          nb;
> > > > @@ -203,6 +209,7 @@ struct mlx5_eswitch {
> > > >  	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
> > > >  	struct workqueue_struct *work_queue;
> > > >  	struct mlx5_vport       *vports;
> > > > +	u32                     flags;
> > > Same as above, no need for extra aligment.
> > 
> > Same reason.
> > 
> > >
> > > >  	int                     total_vports;
> > > >  	int                     enabled_vports;
> > > >  	/* Synchronize between vport change events @@ -240,6 +247,8 @@
> > > > void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
> > > >  				  struct mlx5_vport *vport);
> > > >  void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
> > > >  				   struct mlx5_vport *vport);
> > > > +void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch
> > *esw,
> > > > +					       struct mlx5_vport *vport);
> > > >
> > > >  /* E-Switch API */
> > > >  int mlx5_eswitch_init(struct mlx5_core_dev *dev); diff --git
> > > > a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > > > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > > > index 17abb98b48af..871ae44dc132 100644
> > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > 
> > ...
> > 
> > > > +static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw) {
> > > > +	struct mlx5_vport *vport;
> > > > +	int i, j;
> > > > +	int err;
> > > > +
> > > > +	mlx5_esw_for_all_vports(esw, i, vport) {
> > > > +		err = esw_vport_ingress_common_config(esw, vport);
> > > >  		if (err)
> > > > -			goto err_egress;
> > > > +			goto err_ingress;
> > > > +
> > > > +		if (vport->vport >= MLX5_VPORT_FIRST_VF &&
> > > > +		    vport->vport <= esw->dev->priv.sriov.num_vfs) {
> > > Add an helper API mlx5_esw_is_vport(const struct mlx5_esw *esw, const
> > > struct mlx5_vport *vport) and use at two places in ingress and egress config.
> > 
> > It's very simple logic, but new API make things complicated.
> 
> No. it doesn't. Right API name is,
> mlx5_esw_is_vf_vport().
> mlx5_esw_is_vf_rep()...
> etc.
> 
> > If adding mlx5_esw_is_vport() as you suggested, no one can know what's the meaning of
> > this function from name, and need to check the implementation again, which
> > will waste too much time.
> > 
> mlx5_esw_is_vf_vport() is self-explanatory name which won't waste time.
> 
> I am already having this API in my two series, but since yours is already out, it make sense to introduce in this patch.

Could you please send me? I will add to this series. Thanks!

-- 

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Leon Romanovsky]

On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
> The 06/19/2019 13:04, Leon Romanovsky wrote:
> > On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
> > > The 06/18/2019 18:19, Leon Romanovsky wrote:
> > > > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> > > > > From: Jianbo Liu <jianbol@mellanox.com>
> > > > >
> > > > > If vport metadata matching is enabled in eswitch, the rule created
> > > > > must be changed to match on the metadata, instead of source port.
> > > > >
> > > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > > > > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > > > > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > > > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > > > > ---
> > > > >  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> > > > >  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> > > > >  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
> > > > >  3 files changed, 63 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> > > > > index 22e651cb5534..d4ed611de35d 100644
> > > > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> > > > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> > > > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> > > > >  	return mlx5_eswitch_vport_rep(esw, vport);
> > > > >  }
> > > > >
> > > > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> > > > > +{
> > > > > +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> > > > > +}
> > > > > +
> > > > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> > > > > +						 u16 vport)
> > > > > +{
> > > > > +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> > > > > +}
> > > >
> > > > 1. There is no need to introduce one line functions, call to that code directly.
> > >
> > > No. They are in IB, and we don't want them be mixed up by the original
> > > functions in eswitch. Please ask Mark more about it.
> >
> > Please enlighten me.
>
> It was suggested by Mark in prevouis review.
> I think it's because there are in different modules, and better to with
> different names, so introduce there extra one line functions.
> Please correct me if I'm wrong, Mark...

mlx5_ib is full of direct function calls to mlx5_core and it is done on
purpose for at least two reasons. First is to control in one place
all compilation options and expose proper API interface with and without
specific kernel config is on. Second is to emphasize that this is core
function and save us time in refactoring and reviewing.

>
> >
> > >
> > > > 2. It should be bool and not u32.
> > > >
> > > > Thanks
> > >
> > > --
>
> --

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Mark Bloch]

On 6/18/2019 23:51, Leon Romanovsky wrote:
> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
>> The 06/19/2019 13:04, Leon Romanovsky wrote:
>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
>>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
>>>>>> From: Jianbo Liu <jianbol@mellanox.com>
>>>>>>
>>>>>> If vport metadata matching is enabled in eswitch, the rule created
>>>>>> must be changed to match on the metadata, instead of source port.
>>>>>>
>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>>>>>> ---
>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
>>>>>>  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
>>>>>>  3 files changed, 63 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>> index 22e651cb5534..d4ed611de35d 100644
>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
>>>>>>  	return mlx5_eswitch_vport_rep(esw, vport);
>>>>>>  }
>>>>>>
>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
>>>>>> +{
>>>>>> +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
>>>>>> +}
>>>>>> +
>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
>>>>>> +						 u16 vport)
>>>>>> +{
>>>>>> +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
>>>>>> +}
>>>>>
>>>>> 1. There is no need to introduce one line functions, call to that code directly.
>>>>
>>>> No. They are in IB, and we don't want them be mixed up by the original
>>>> functions in eswitch. Please ask Mark more about it.
>>>
>>> Please enlighten me.
>>
>> It was suggested by Mark in prevouis review.
>> I think it's because there are in different modules, and better to with
>> different names, so introduce there extra one line functions.
>> Please correct me if I'm wrong, Mark...
> 
> mlx5_ib is full of direct function calls to mlx5_core and it is done on
> purpose for at least two reasons. First is to control in one place
> all compilation options and expose proper API interface with and without
> specific kernel config is on. Second is to emphasize that this is core
> function and save us time in refactoring and reviewing.

This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
(Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
should have probably done the same.

As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
I want to group together stuff in separate files.

If you prefer direct calls that's okay as well.

Mark

> 
>>
>>>
>>>>
>>>>> 2. It should be bool and not u32.
>>>>>
>>>>> Thanks
>>>>
>>>> --
>>
>> --

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Leon Romanovsky]

On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote:
>
>
> On 6/18/2019 23:51, Leon Romanovsky wrote:
> > On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
> >> The 06/19/2019 13:04, Leon Romanovsky wrote:
> >>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
> >>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
> >>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> >>>>>> From: Jianbo Liu <jianbol@mellanox.com>
> >>>>>>
> >>>>>> If vport metadata matching is enabled in eswitch, the rule created
> >>>>>> must be changed to match on the metadata, instead of source port.
> >>>>>>
> >>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> >>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
> >>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
> >>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> >>>>>> ---
> >>>>>>  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> >>>>>>  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> >>>>>>  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
> >>>>>>  3 files changed, 63 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>> index 22e651cb5534..d4ed611de35d 100644
> >>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> >>>>>>  	return mlx5_eswitch_vport_rep(esw, vport);
> >>>>>>  }
> >>>>>>
> >>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> >>>>>> +{
> >>>>>> +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> >>>>>> +}
> >>>>>> +
> >>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> >>>>>> +						 u16 vport)
> >>>>>> +{
> >>>>>> +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> >>>>>> +}
> >>>>>
> >>>>> 1. There is no need to introduce one line functions, call to that code directly.
> >>>>
> >>>> No. They are in IB, and we don't want them be mixed up by the original
> >>>> functions in eswitch. Please ask Mark more about it.
> >>>
> >>> Please enlighten me.
> >>
> >> It was suggested by Mark in prevouis review.
> >> I think it's because there are in different modules, and better to with
> >> different names, so introduce there extra one line functions.
> >> Please correct me if I'm wrong, Mark...
> >
> > mlx5_ib is full of direct function calls to mlx5_core and it is done on
> > purpose for at least two reasons. First is to control in one place
> > all compilation options and expose proper API interface with and without
> > specific kernel config is on. Second is to emphasize that this is core
> > function and save us time in refactoring and reviewing.
>
> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
> should have probably done the same.

This is exactly the problem, eswitch.h should provide stubs for all
exported functions, so other clients of eswitch won't need to deal with
various unrelated config options.

>
> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
> I want to group together stuff in separate files.

Yes, it is right thing to do.

>
> If you prefer direct calls that's okay as well.

Yes, please.

>
> Mark
>
> >
> >>
> >>>
> >>>>
> >>>>> 2. It should be bool and not u32.
> >>>>>
> >>>>> Thanks
> >>>>
> >>>> --
> >>
> >> --

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Mark Bloch]

On 6/19/2019 00:43, Leon Romanovsky wrote:
> On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote:
>>
>>
>> On 6/18/2019 23:51, Leon Romanovsky wrote:
>>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
>>>> The 06/19/2019 13:04, Leon Romanovsky wrote:
>>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
>>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
>>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
>>>>>>>> From: Jianbo Liu <jianbol@mellanox.com>
>>>>>>>>
>>>>>>>> If vport metadata matching is enabled in eswitch, the rule created
>>>>>>>> must be changed to match on the metadata, instead of source port.
>>>>>>>>
>>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
>>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
>>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>>>>>>>> ---
>>>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
>>>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
>>>>>>>>  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
>>>>>>>>  3 files changed, 63 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>> index 22e651cb5534..d4ed611de35d 100644
>>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
>>>>>>>>  	return mlx5_eswitch_vport_rep(esw, vport);
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
>>>>>>>> +{
>>>>>>>> +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
>>>>>>>> +						 u16 vport)
>>>>>>>> +{
>>>>>>>> +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
>>>>>>>> +}
>>>>>>>
>>>>>>> 1. There is no need to introduce one line functions, call to that code directly.
>>>>>>
>>>>>> No. They are in IB, and we don't want them be mixed up by the original
>>>>>> functions in eswitch. Please ask Mark more about it.
>>>>>
>>>>> Please enlighten me.
>>>>
>>>> It was suggested by Mark in prevouis review.
>>>> I think it's because there are in different modules, and better to with
>>>> different names, so introduce there extra one line functions.
>>>> Please correct me if I'm wrong, Mark...
>>>
>>> mlx5_ib is full of direct function calls to mlx5_core and it is done on
>>> purpose for at least two reasons. First is to control in one place
>>> all compilation options and expose proper API interface with and without
>>> specific kernel config is on. Second is to emphasize that this is core
>>> function and save us time in refactoring and reviewing.
>>
>> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
>> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
>> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
>> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
>> should have probably done the same.
> 
> This is exactly the problem, eswitch.h should provide stubs for all
> exported functions, so other clients of eswitch won't need to deal with
> various unrelated config options.

The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly
but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling
the eswitch layer.

If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when
CONFIG_MLX5_ESWITCH id defined.

so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h).
For me it makes it easier to follow, but I can adept if you don't like it.

Mark

> 
>>
>> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
>> I want to group together stuff in separate files.
> 
> Yes, it is right thing to do.
> 
>>
>> If you prefer direct calls that's okay as well.
> 
> Yes, please.
> 
>>
>> Mark
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> 2. It should be bool and not u32.
>>>>>>>
>>>>>>> Thanks
>>>>>>
>>>>>> --
>>>>
>>>> --

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Leon Romanovsky]

On Wed, Jun 19, 2019 at 07:58:51AM +0000, Mark Bloch wrote:
>
>
> On 6/19/2019 00:43, Leon Romanovsky wrote:
> > On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote:
> >>
> >>
> >> On 6/18/2019 23:51, Leon Romanovsky wrote:
> >>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
> >>>> The 06/19/2019 13:04, Leon Romanovsky wrote:
> >>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
> >>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
> >>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> >>>>>>>> From: Jianbo Liu <jianbol@mellanox.com>
> >>>>>>>>
> >>>>>>>> If vport metadata matching is enabled in eswitch, the rule created
> >>>>>>>> must be changed to match on the metadata, instead of source port.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> >>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
> >>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
> >>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> >>>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> >>>>>>>>  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
> >>>>>>>>  3 files changed, 63 insertions(+), 9 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>>>> index 22e651cb5534..d4ed611de35d 100644
> >>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> >>>>>>>>  	return mlx5_eswitch_vport_rep(esw, vport);
> >>>>>>>>  }
> >>>>>>>>
> >>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> >>>>>>>> +{
> >>>>>>>> +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> >>>>>>>> +						 u16 vport)
> >>>>>>>> +{
> >>>>>>>> +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> >>>>>>>> +}
> >>>>>>>
> >>>>>>> 1. There is no need to introduce one line functions, call to that code directly.
> >>>>>>
> >>>>>> No. They are in IB, and we don't want them be mixed up by the original
> >>>>>> functions in eswitch. Please ask Mark more about it.
> >>>>>
> >>>>> Please enlighten me.
> >>>>
> >>>> It was suggested by Mark in prevouis review.
> >>>> I think it's because there are in different modules, and better to with
> >>>> different names, so introduce there extra one line functions.
> >>>> Please correct me if I'm wrong, Mark...
> >>>
> >>> mlx5_ib is full of direct function calls to mlx5_core and it is done on
> >>> purpose for at least two reasons. First is to control in one place
> >>> all compilation options and expose proper API interface with and without
> >>> specific kernel config is on. Second is to emphasize that this is core
> >>> function and save us time in refactoring and reviewing.
> >>
> >> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
> >> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
> >> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
> >> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
> >> should have probably done the same.
> >
> > This is exactly the problem, eswitch.h should provide stubs for all
> > exported functions, so other clients of eswitch won't need to deal with
> > various unrelated config options.
>
> The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly
> but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling
> the eswitch layer.
>
> If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when
> CONFIG_MLX5_ESWITCH id defined.

This simple patch + cleanup of ib_rep.h will do the trick.

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 67b9e7ac569a..b917ba28659e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -59,7 +59,9 @@
 #include <linux/in.h>
 #include <linux/etherdevice.h>
 #include "mlx5_ib.h"
+#if defined(CONFIG_MLX5_ESWITCH)
 #include "ib_rep.h"
+#endif
 #include "cmd.h"
 #include "srq.h"
 #include <linux/mlx5/fs_helpers.h>
@@ -6765,6 +6767,7 @@ static const struct mlx5_ib_profile  pf_profile = {
                        mlx5_ib_stage_delay_drop_cleanup),
	     };

+#if defined(CONFIG_MLX5_ESWITCH)
 const struct mlx5_ib_profile uplink_rep_profile = {
	STAGE_CREATE(MLX5_IB_STAGE_INIT,
		     mlx5_ib_stage_init_init,
@@ -6812,6 +6815,7 @@
 const struct mlx5_ib_profile uplink_rep_profile = {
		               mlx5_ib_stage_post_ib_reg_umr_init,
		               NULL),
		};

>
> so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h).
> For me it makes it easier to follow, but I can adept if you don't like it.
>
> Mark
>
> >
> >>
> >> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
> >> I want to group together stuff in separate files.
> >
> > Yes, it is right thing to do.
> >
> >>
> >> If you prefer direct calls that's okay as well.
> >
> > Yes, please.
> >
> >>
> >> Mark
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> 2. It should be bool and not u32.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>
> >>>>>> --
> >>>>
> >>>> --

Re: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs

[Jianbo Liu]

The 06/18/2019 18:31, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > Behalf Of Saeed Mahameed
> > Sent: Tuesday, June 18, 2019 12:53 AM
> > To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > <leonro@mellanox.com>
> > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> > <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> > <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> > Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport
> > number in VF vports and uplink ingress ACLs
> > 
> > From: Jianbo Liu <jianbol@mellanox.com>
> > 
> > When a dual-port VHCA sends a RoCE packet on its non-native port, and the
> > packet arrives to its affiliated vport FDB, a mismatch might occur on the rules
> > that match the packet source vport as it is not represented by single VHCA only
> > in this case. So we change to match on metadata instead of source vport.
> > To do that, a rule is created in all vports and uplink ingress ACLs, to save the
> > source vport number and vhca id in the packet's metadata in order to match on
> > it later.
> > The metadata register used is the first of the 32-bit type C registers. It can be
> > used for matching and header modify operations. The higher 16 bits of this
> > register are for vhca id, and the lower 16 ones is for vport number.
> > This change is not for dual-port RoCE only. If HW and FW allow, the vport
> > metadata matching is enabled by default.
> > 
> > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > Reviewed-by: Eli Britstein <elibr@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
> >  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
> >  .../mellanox/mlx5/core/eswitch_offloads.c     | 183 ++++++++++++++----
> >  include/linux/mlx5/eswitch.h                  |   3 +
> >  4 files changed, 161 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > index a42a23e505df..1235fd84ae3a 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c

...

> > +			err = esw_vport_egress_prio_tag_config(esw, vport);
> > +			if (err)
> > +				goto err_egress;
> > +		}
> >  	}
> > 
> > +	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
> > +		esw_info(esw->dev, "Use metadata reg_c as source vport to
> > match\n");
> > +
> >  	return 0;
> > 
> >  err_egress:
> >  	esw_vport_disable_ingress_acl(esw, vport);
> >  err_ingress:
> > -	mlx5_esw_for_each_vf_vport_reverse(esw, j, vport, i - 1) {
> > +	for (j = MLX5_VPORT_PF; j < i; j++) {
> Keep the reverse order as before.

The vports are independent from each other. It doesn't matter disabling
them in or out of order. I don't understand what's the benifit.

> 
> > +		vport = &esw->vports[j];
> >  		esw_vport_disable_egress_acl(esw, vport);
> >  		esw_vport_disable_ingress_acl(esw, vport);
> >  	}
> > @@ -1704,15 +1800,17 @@ static int esw_prio_tag_acls_config(struct
> > mlx5_eswitch *esw, int nvports)
> >  	return err;
> >  }
> > 
> > -static void esw_prio_tag_acls_cleanup(struct mlx5_eswitch *esw)
> > +static void esw_destroy_offloads_acl_tables(struct mlx5_eswitch *esw)
> >  {
> >  	struct mlx5_vport *vport;
> >  	int i;
> > 
> > -	mlx5_esw_for_each_vf_vport(esw, i, vport, esw->nvports) {
> > +	mlx5_esw_for_all_vports(esw, i, vport) {
> If you are changing this, please do in reverse order to keep it exact mirror of create/enable sequence.

Same...

> 
> >  		esw_vport_disable_egress_acl(esw, vport);
> >  		esw_vport_disable_ingress_acl(esw, vport);
> >  	}
> > +
> > +	esw->flags &= ~MLX5_ESWITCH_VPORT_MATCH_METADATA;
> >  }
> > 
> >  static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)

Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors

[Mark Bloch]

On 6/19/19 1:12 AM, Leon Romanovsky wrote:
> On Wed, Jun 19, 2019 at 07:58:51AM +0000, Mark Bloch wrote:
>>
>>
>> On 6/19/2019 00:43, Leon Romanovsky wrote:
>>> On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote:
>>>>
>>>>
>>>> On 6/18/2019 23:51, Leon Romanovsky wrote:
>>>>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
>>>>>> The 06/19/2019 13:04, Leon Romanovsky wrote:
>>>>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
>>>>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
>>>>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
>>>>>>>>>> From: Jianbo Liu <jianbol@mellanox.com>
>>>>>>>>>>
>>>>>>>>>> If vport metadata matching is enabled in eswitch, the rule created
>>>>>>>>>> must be changed to match on the metadata, instead of source port.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
>>>>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>>>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
>>>>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
>>>>>>>>>>  drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
>>>>>>>>>>  drivers/infiniband/hw/mlx5/main.c   | 45 +++++++++++++++++++++++------
>>>>>>>>>>  3 files changed, 63 insertions(+), 9 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>>>> index 22e651cb5534..d4ed611de35d 100644
>>>>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
>>>>>>>>>>  	return mlx5_eswitch_vport_rep(esw, vport);
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
>>>>>>>>>> +{
>>>>>>>>>> +	return mlx5_eswitch_vport_match_metadata_enabled(esw);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
>>>>>>>>>> +						 u16 vport)
>>>>>>>>>> +{
>>>>>>>>>> +	return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
>>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>> 1. There is no need to introduce one line functions, call to that code directly.
>>>>>>>>
>>>>>>>> No. They are in IB, and we don't want them be mixed up by the original
>>>>>>>> functions in eswitch. Please ask Mark more about it.
>>>>>>>
>>>>>>> Please enlighten me.
>>>>>>
>>>>>> It was suggested by Mark in prevouis review.
>>>>>> I think it's because there are in different modules, and better to with
>>>>>> different names, so introduce there extra one line functions.
>>>>>> Please correct me if I'm wrong, Mark...
>>>>>
>>>>> mlx5_ib is full of direct function calls to mlx5_core and it is done on
>>>>> purpose for at least two reasons. First is to control in one place
>>>>> all compilation options and expose proper API interface with and without
>>>>> specific kernel config is on. Second is to emphasize that this is core
>>>>> function and save us time in refactoring and reviewing.
>>>>
>>>> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
>>>> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
>>>> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
>>>> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
>>>> should have probably done the same.
>>>
>>> This is exactly the problem, eswitch.h should provide stubs for all
>>> exported functions, so other clients of eswitch won't need to deal with
>>> various unrelated config options.
>>
>> The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly
>> but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling
>> the eswitch layer.
>>
>> If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when
>> CONFIG_MLX5_ESWITCH id defined.
> 
> This simple patch + cleanup of ib_rep.h will do the trick.
> 
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 67b9e7ac569a..b917ba28659e 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -59,7 +59,9 @@
>  #include <linux/in.h>
>  #include <linux/etherdevice.h>
>  #include "mlx5_ib.h"
> +#if defined(CONFIG_MLX5_ESWITCH)
>  #include "ib_rep.h"
> +#endif
>  #include "cmd.h"
>  #include "srq.h"
>  #include <linux/mlx5/fs_helpers.h>
> @@ -6765,6 +6767,7 @@ static const struct mlx5_ib_profile  pf_profile = {
>                         mlx5_ib_stage_delay_drop_cleanup),
> 	     };
> 
> +#if defined(CONFIG_MLX5_ESWITCH)
>  const struct mlx5_ib_profile uplink_rep_profile = {
> 	STAGE_CREATE(MLX5_IB_STAGE_INIT,
> 		     mlx5_ib_stage_init_init,
> @@ -6812,6 +6815,7 @@
>  const struct mlx5_ib_profile uplink_rep_profile = {
> 		               mlx5_ib_stage_post_ib_reg_umr_init,
> 		               NULL),
> 		};
>

I really dislike seeing #if defined(CONFIG_MLX5_ESWITCH) inside .c files
and here it's easily avoided so I don't see a reason do it it.

Can this cleanup wait for after this series?

Mark
 
>>
>> so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h).
>> For me it makes it easier to follow, but I can adept if you don't like it.
>>
>> Mark
>>
>>>
>>>>
>>>> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
>>>> I want to group together stuff in separate files.
>>>
>>> Yes, it is right thing to do.
>>>
>>>>
>>>> If you prefer direct calls that's okay as well.
>>>
>>> Yes, please.
>>>
>>>>
>>>> Mark
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 2. It should be bool and not u32.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> --
>>>>>>
>>>>>> --