[PATCH net-next 0/5] sfc: support TC decap rules

[PATCH net-next 0/5] sfc: support TC decap rules

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

This series adds support for offloading tunnel decapsulation TC rules to
 ef100 NICs, allowing matching encapsulated packets to be decapsulated in
 hardware and redirected to VFs.

Edward Cree (5):
  sfc: add notion of match on enc keys to MAE machinery
  sfc: handle enc keys in efx_tc_flower_parse_match()
  sfc: add functions to insert encap matches into the MAE
  sfc: add code to register and unregister encap matches
  sfc: add offloading of 'foreign' TC (decap) rules

 drivers/net/ethernet/sfc/mae.c | 237 ++++++++++++-
 drivers/net/ethernet/sfc/mae.h |  11 +
 drivers/net/ethernet/sfc/tc.c  | 596 ++++++++++++++++++++++++++++++++-
 drivers/net/ethernet/sfc/tc.h  |  37 ++
 4 files changed, 864 insertions(+), 17 deletions(-)

[PATCH net-next 1/5] sfc: add notion of match on enc keys to MAE machinery

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Extend the MAE caps check to validate that the hardware supports used
 outer-header matches.
Extend efx_mae_populate_match_criteria() to fill in the outer rule ID
 and VNI match fields.
Nothing yet populates these match fields, nor creates outer rules.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/mae.c | 104 ++++++++++++++++++++++++++++++++-
 drivers/net/ethernet/sfc/mae.h |   3 +
 drivers/net/ethernet/sfc/tc.h  |  24 ++++++++
 3 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index c53d354c1fb2..1a285facda34 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -254,13 +254,23 @@ static int efx_mae_get_rule_fields(struct efx_nic *efx, u32 cmd,
 	size_t outlen;
 	int rc, i;
 
+	/* AR and OR caps MCDIs have identical layout, so we are using the
+	 * same code for both.
+	 */
+	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_LEN(MAE_NUM_FIELDS) <
+		     MC_CMD_MAE_GET_OR_CAPS_OUT_LEN(MAE_NUM_FIELDS));
 	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_IN_LEN);
+	BUILD_BUG_ON(MC_CMD_MAE_GET_OR_CAPS_IN_LEN);
 
 	rc = efx_mcdi_rpc(efx, cmd, NULL, 0, outbuf, sizeof(outbuf), &outlen);
 	if (rc)
 		return rc;
+	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_COUNT_OFST !=
+		     MC_CMD_MAE_GET_OR_CAPS_OUT_COUNT_OFST);
 	count = MCDI_DWORD(outbuf, MAE_GET_AR_CAPS_OUT_COUNT);
 	memset(field_support, MAE_FIELD_UNSUPPORTED, MAE_NUM_FIELDS);
+	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_FIELD_FLAGS_OFST !=
+		     MC_CMD_MAE_GET_OR_CAPS_OUT_FIELD_FLAGS_OFST);
 	caps = _MCDI_DWORD(outbuf, MAE_GET_AR_CAPS_OUT_FIELD_FLAGS);
 	/* We're only interested in the support status enum, not any other
 	 * flags, so just extract that from each entry.
@@ -278,8 +288,12 @@ int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps)
 	rc = efx_mae_get_basic_caps(efx, caps);
 	if (rc)
 		return rc;
-	return efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_AR_CAPS,
-				       caps->action_rule_fields);
+	rc = efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_AR_CAPS,
+				     caps->action_rule_fields);
+	if (rc)
+		return rc;
+	return efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_OR_CAPS,
+				       caps->outer_rule_fields);
 }
 
 /* Bit twiddling:
@@ -432,11 +446,73 @@ int efx_mae_match_check_caps(struct efx_nic *efx,
 	    CHECK_BIT(IP_FIRST_FRAG, ip_firstfrag) ||
 	    CHECK(RECIRC_ID, recirc_id))
 		return rc;
+	/* Matches on outer fields are done in a separate hardware table,
+	 * the Outer Rule table.  Thus the Action Rule merely does an
+	 * exact match on Outer Rule ID if any outer field matches are
+	 * present.  The exception is the VNI/VSID (enc_keyid), which is
+	 * available to the Action Rule match iff the Outer Rule matched
+	 * (and thus identified the encap protocol to use to extract it).
+	 */
+	if (efx_tc_match_is_encap(mask)) {
+		rc = efx_mae_match_check_cap_typ(
+				supported_fields[MAE_FIELD_OUTER_RULE_ID],
+				MASK_ONES);
+		if (rc) {
+			NL_SET_ERR_MSG_MOD(extack, "No support for encap rule ID matches");
+			return rc;
+		}
+		if (CHECK(ENC_VNET_ID, enc_keyid))
+			return rc;
+	} else if (mask->enc_keyid) {
+		NL_SET_ERR_MSG_MOD(extack, "Match on enc_keyid requires other encap fields");
+		return -EINVAL;
+	}
 	return 0;
 }
 #undef CHECK_BIT
 #undef CHECK
 
+#define CHECK(_mcdi)	({						       \
+	rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_ ## _mcdi],\
+					 MASK_ONES);			       \
+	if (rc)								       \
+		NL_SET_ERR_MSG_FMT_MOD(extack,				       \
+				       "No support for field %s", #_mcdi);     \
+	rc;								       \
+})
+/* Checks that the fields needed for encap-rule matches are supported by the
+ * MAE.  All the fields are exact-match.
+ */
+int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
+				   struct netlink_ext_ack *extack)
+{
+	u8 *supported_fields = efx->tc->caps->outer_rule_fields;
+	int rc;
+
+	if (CHECK(ENC_ETHER_TYPE))
+		return rc;
+	switch (ipv) {
+	case 4:
+		if (CHECK(ENC_SRC_IP4) ||
+		    CHECK(ENC_DST_IP4))
+			return rc;
+		break;
+	case 6:
+		if (CHECK(ENC_SRC_IP6) ||
+		    CHECK(ENC_DST_IP6))
+			return rc;
+		break;
+	default: /* shouldn't happen */
+		EFX_WARN_ON_PARANOID(1);
+		break;
+	}
+	if (CHECK(ENC_L4_DPORT) ||
+	    CHECK(ENC_IP_PROTO))
+		return rc;
+	return 0;
+}
+#undef CHECK
+
 int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
 {
 	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_COUNTER_ALLOC_OUT_LEN(1));
@@ -941,6 +1017,30 @@ static int efx_mae_populate_match_criteria(MCDI_DECLARE_STRUCT_PTR(match_crit),
 				match->value.tcp_flags);
 	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_TCP_FLAGS_BE_MASK,
 				match->mask.tcp_flags);
+	/* enc-keys are handled indirectly, through encap_match ID */
+	if (match->encap) {
+		MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID,
+				      match->encap->fw_id);
+		MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID_MASK,
+				      (u32)-1);
+		/* enc_keyid (VNI/VSID) is not part of the encap_match */
+		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_ENC_VNET_ID_BE,
+					 match->value.enc_keyid);
+		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_ENC_VNET_ID_BE_MASK,
+					 match->mask.enc_keyid);
+	} else {
+		/* No enc-keys should appear in a rule without an encap_match */
+		if (WARN_ON_ONCE(match->mask.enc_src_ip) ||
+		    WARN_ON_ONCE(match->mask.enc_dst_ip) ||
+		    WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_src_ip6)) ||
+		    WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_dst_ip6)) ||
+		    WARN_ON_ONCE(match->mask.enc_ip_tos) ||
+		    WARN_ON_ONCE(match->mask.enc_ip_ttl) ||
+		    WARN_ON_ONCE(match->mask.enc_sport) ||
+		    WARN_ON_ONCE(match->mask.enc_dport) ||
+		    WARN_ON_ONCE(match->mask.enc_keyid))
+			return -EOPNOTSUPP;
+	}
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
index bec293a06733..a45d1791517f 100644
--- a/drivers/net/ethernet/sfc/mae.h
+++ b/drivers/net/ethernet/sfc/mae.h
@@ -72,6 +72,7 @@ struct mae_caps {
 	u32 match_field_count;
 	u32 action_prios;
 	u8 action_rule_fields[MAE_NUM_FIELDS];
+	u8 outer_rule_fields[MAE_NUM_FIELDS];
 };
 
 int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps);
@@ -79,6 +80,8 @@ int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps);
 int efx_mae_match_check_caps(struct efx_nic *efx,
 			     const struct efx_tc_match_fields *mask,
 			     struct netlink_ext_ack *extack);
+int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
+				   struct netlink_ext_ack *extack);
 
 int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
 int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index 542853f60c2a..c1485679507c 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -48,11 +48,35 @@ struct efx_tc_match_fields {
 	/* L4 */
 	__be16 l4_sport, l4_dport; /* Ports (UDP, TCP) */
 	__be16 tcp_flags;
+	/* Encap.  The following are *outer* fields.  Note that there are no
+	 * outer eth (L2) fields; this is because TC doesn't have them.
+	 */
+	__be32 enc_src_ip, enc_dst_ip;
+	struct in6_addr enc_src_ip6, enc_dst_ip6;
+	u8 enc_ip_tos, enc_ip_ttl;
+	__be16 enc_sport, enc_dport;
+	__be32 enc_keyid; /* e.g. VNI, VSID */
+};
+
+static inline bool efx_tc_match_is_encap(const struct efx_tc_match_fields *mask)
+{
+	return mask->enc_src_ip || mask->enc_dst_ip ||
+	       !ipv6_addr_any(&mask->enc_src_ip6) ||
+	       !ipv6_addr_any(&mask->enc_dst_ip6) || mask->enc_ip_tos ||
+	       mask->enc_ip_ttl || mask->enc_sport || mask->enc_dport;
+}
+
+struct efx_tc_encap_match {
+	__be32 src_ip, dst_ip;
+	struct in6_addr src_ip6, dst_ip6;
+	__be16 udp_dport;
+	u32 fw_id; /* index of this entry in firmware encap match table */
 };
 
 struct efx_tc_match {
 	struct efx_tc_match_fields value;
 	struct efx_tc_match_fields mask;
+	struct efx_tc_encap_match *encap;
 };
 
 struct efx_tc_action_set_list {

Re: [PATCH net-next 1/5] sfc: add notion of match on enc keys to MAE machinery

[Michal Swiatkowski]

On Tue, Mar 14, 2023 at 05:35:21PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Extend the MAE caps check to validate that the hardware supports used
>  outer-header matches.
> Extend efx_mae_populate_match_criteria() to fill in the outer rule ID
>  and VNI match fields.
> Nothing yet populates these match fields, nor creates outer rules.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/mae.c | 104 ++++++++++++++++++++++++++++++++-
>  drivers/net/ethernet/sfc/mae.h |   3 +
>  drivers/net/ethernet/sfc/tc.h  |  24 ++++++++
>  3 files changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
> index c53d354c1fb2..1a285facda34 100644
> --- a/drivers/net/ethernet/sfc/mae.c
> +++ b/drivers/net/ethernet/sfc/mae.c
> @@ -254,13 +254,23 @@ static int efx_mae_get_rule_fields(struct efx_nic *efx, u32 cmd,
>  	size_t outlen;
>  	int rc, i;
>  
> +	/* AR and OR caps MCDIs have identical layout, so we are using the
> +	 * same code for both.
> +	 */
> +	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_LEN(MAE_NUM_FIELDS) <
> +		     MC_CMD_MAE_GET_OR_CAPS_OUT_LEN(MAE_NUM_FIELDS));
>  	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_IN_LEN);
> +	BUILD_BUG_ON(MC_CMD_MAE_GET_OR_CAPS_IN_LEN);
>  
>  	rc = efx_mcdi_rpc(efx, cmd, NULL, 0, outbuf, sizeof(outbuf), &outlen);
>  	if (rc)
>  		return rc;
> +	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_COUNT_OFST !=
> +		     MC_CMD_MAE_GET_OR_CAPS_OUT_COUNT_OFST);
>  	count = MCDI_DWORD(outbuf, MAE_GET_AR_CAPS_OUT_COUNT);
>  	memset(field_support, MAE_FIELD_UNSUPPORTED, MAE_NUM_FIELDS);
> +	BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_FIELD_FLAGS_OFST !=
> +		     MC_CMD_MAE_GET_OR_CAPS_OUT_FIELD_FLAGS_OFST);
>  	caps = _MCDI_DWORD(outbuf, MAE_GET_AR_CAPS_OUT_FIELD_FLAGS);
>  	/* We're only interested in the support status enum, not any other
>  	 * flags, so just extract that from each entry.
> @@ -278,8 +288,12 @@ int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps)
>  	rc = efx_mae_get_basic_caps(efx, caps);
>  	if (rc)
>  		return rc;
> -	return efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_AR_CAPS,
> -				       caps->action_rule_fields);
> +	rc = efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_AR_CAPS,
> +				     caps->action_rule_fields);
> +	if (rc)
> +		return rc;
> +	return efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_OR_CAPS,
> +				       caps->outer_rule_fields);
>  }
>  
>  /* Bit twiddling:
> @@ -432,11 +446,73 @@ int efx_mae_match_check_caps(struct efx_nic *efx,
>  	    CHECK_BIT(IP_FIRST_FRAG, ip_firstfrag) ||
>  	    CHECK(RECIRC_ID, recirc_id))
>  		return rc;
> +	/* Matches on outer fields are done in a separate hardware table,
> +	 * the Outer Rule table.  Thus the Action Rule merely does an
> +	 * exact match on Outer Rule ID if any outer field matches are
> +	 * present.  The exception is the VNI/VSID (enc_keyid), which is
> +	 * available to the Action Rule match iff the Outer Rule matched
if I think :)

> +	 * (and thus identified the encap protocol to use to extract it).
> +	 */
> +	if (efx_tc_match_is_encap(mask)) {
> +		rc = efx_mae_match_check_cap_typ(
> +				supported_fields[MAE_FIELD_OUTER_RULE_ID],
> +				MASK_ONES);
> +		if (rc) {
> +			NL_SET_ERR_MSG_MOD(extack, "No support for encap rule ID matches");
> +			return rc;
> +		}
> +		if (CHECK(ENC_VNET_ID, enc_keyid))
> +			return rc;
> +	} else if (mask->enc_keyid) {
> +		NL_SET_ERR_MSG_MOD(extack, "Match on enc_keyid requires other encap fields");
> +		return -EINVAL;
> +	}
>  	return 0;
>  }
>  #undef CHECK_BIT
>  #undef CHECK
>  
> +#define CHECK(_mcdi)	({						       \
> +	rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_ ## _mcdi],\
> +					 MASK_ONES);			       \
> +	if (rc)								       \
> +		NL_SET_ERR_MSG_FMT_MOD(extack,				       \
> +				       "No support for field %s", #_mcdi);     \
> +	rc;								       \
> +})
Is there any reasone why macro is used instead of function? It is a
little hard to read becasue it is modyfing rc value.

> +/* Checks that the fields needed for encap-rule matches are supported by the
> + * MAE.  All the fields are exact-match.
> + */
> +int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
> +				   struct netlink_ext_ack *extack)
> +{
> +	u8 *supported_fields = efx->tc->caps->outer_rule_fields;
> +	int rc;
> +
> +	if (CHECK(ENC_ETHER_TYPE))
> +		return rc;
> +	switch (ipv) {
> +	case 4:
> +		if (CHECK(ENC_SRC_IP4) ||
> +		    CHECK(ENC_DST_IP4))
> +			return rc;
> +		break;
> +	case 6:
> +		if (CHECK(ENC_SRC_IP6) ||
> +		    CHECK(ENC_DST_IP6))
> +			return rc;
> +		break;
> +	default: /* shouldn't happen */
> +		EFX_WARN_ON_PARANOID(1);
> +		break;
> +	}
> +	if (CHECK(ENC_L4_DPORT) ||
> +	    CHECK(ENC_IP_PROTO))
> +		return rc;
> +	return 0;
> +}
> +#undef CHECK
> +
>  int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
>  {
>  	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_COUNTER_ALLOC_OUT_LEN(1));
> @@ -941,6 +1017,30 @@ static int efx_mae_populate_match_criteria(MCDI_DECLARE_STRUCT_PTR(match_crit),
>  				match->value.tcp_flags);
>  	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_TCP_FLAGS_BE_MASK,
>  				match->mask.tcp_flags);
> +	/* enc-keys are handled indirectly, through encap_match ID */
> +	if (match->encap) {
> +		MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID,
> +				      match->encap->fw_id);
> +		MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID_MASK,
> +				      (u32)-1);
U32_MAX can't be used here?

> +		/* enc_keyid (VNI/VSID) is not part of the encap_match */
> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_ENC_VNET_ID_BE,
> +					 match->value.enc_keyid);
> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_ENC_VNET_ID_BE_MASK,
> +					 match->mask.enc_keyid);
> +	} else {
> +		/* No enc-keys should appear in a rule without an encap_match */
> +		if (WARN_ON_ONCE(match->mask.enc_src_ip) ||
> +		    WARN_ON_ONCE(match->mask.enc_dst_ip) ||
> +		    WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_src_ip6)) ||
> +		    WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_dst_ip6)) ||
> +		    WARN_ON_ONCE(match->mask.enc_ip_tos) ||
> +		    WARN_ON_ONCE(match->mask.enc_ip_ttl) ||
> +		    WARN_ON_ONCE(match->mask.enc_sport) ||
> +		    WARN_ON_ONCE(match->mask.enc_dport) ||
> +		    WARN_ON_ONCE(match->mask.enc_keyid))
> +			return -EOPNOTSUPP;
Can be written as else if {}

Also, You define a similar function: efx_tc_match_is_encap(), I think it
can be used here.

> +	}
>  	return 0;
>  }
>  
> diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
> index bec293a06733..a45d1791517f 100644
> --- a/drivers/net/ethernet/sfc/mae.h
> +++ b/drivers/net/ethernet/sfc/mae.h
> @@ -72,6 +72,7 @@ struct mae_caps {
>  	u32 match_field_count;
>  	u32 action_prios;
>  	u8 action_rule_fields[MAE_NUM_FIELDS];
> +	u8 outer_rule_fields[MAE_NUM_FIELDS];
>  };
>  
>  int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps);
> @@ -79,6 +80,8 @@ int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps);
>  int efx_mae_match_check_caps(struct efx_nic *efx,
>  			     const struct efx_tc_match_fields *mask,
>  			     struct netlink_ext_ack *extack);
> +int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
> +				   struct netlink_ext_ack *extack);
>  
>  int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
>  int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
> diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
> index 542853f60c2a..c1485679507c 100644
> --- a/drivers/net/ethernet/sfc/tc.h
> +++ b/drivers/net/ethernet/sfc/tc.h
> @@ -48,11 +48,35 @@ struct efx_tc_match_fields {
>  	/* L4 */
>  	__be16 l4_sport, l4_dport; /* Ports (UDP, TCP) */
>  	__be16 tcp_flags;
> +	/* Encap.  The following are *outer* fields.  Note that there are no
> +	 * outer eth (L2) fields; this is because TC doesn't have them.
> +	 */
> +	__be32 enc_src_ip, enc_dst_ip;
> +	struct in6_addr enc_src_ip6, enc_dst_ip6;
> +	u8 enc_ip_tos, enc_ip_ttl;
> +	__be16 enc_sport, enc_dport;
> +	__be32 enc_keyid; /* e.g. VNI, VSID */
> +};
> +
> +static inline bool efx_tc_match_is_encap(const struct efx_tc_match_fields *mask)
> +{
> +	return mask->enc_src_ip || mask->enc_dst_ip ||
> +	       !ipv6_addr_any(&mask->enc_src_ip6) ||
> +	       !ipv6_addr_any(&mask->enc_dst_ip6) || mask->enc_ip_tos ||
> +	       mask->enc_ip_ttl || mask->enc_sport || mask->enc_dport;
> +}
> +
> +struct efx_tc_encap_match {
> +	__be32 src_ip, dst_ip;
> +	struct in6_addr src_ip6, dst_ip6;
> +	__be16 udp_dport;
What about source port? It isn't supported?

> +	u32 fw_id; /* index of this entry in firmware encap match table */
>  };
>  
>  struct efx_tc_match {
>  	struct efx_tc_match_fields value;
>  	struct efx_tc_match_fields mask;
> +	struct efx_tc_encap_match *encap;
>  };
>  
>  struct efx_tc_action_set_list {

Re: [PATCH net-next 1/5] sfc: add notion of match on enc keys to MAE machinery

[Edward Cree]

On 15/03/2023 06:19, Michal Swiatkowski wrote:
> On Tue, Mar 14, 2023 at 05:35:21PM +0000, edward.cree@amd.com wrote:
>> +	/* Matches on outer fields are done in a separate hardware table,
>> +	 * the Outer Rule table.  Thus the Action Rule merely does an
>> +	 * exact match on Outer Rule ID if any outer field matches are
>> +	 * present.  The exception is the VNI/VSID (enc_keyid), which is
>> +	 * available to the Action Rule match iff the Outer Rule matched
> if I think :)

Nope, I did mean iff: https://en.wiktionary.org/wiki/iff
Just my reflexes as an ex-mathmo kicking in again.

>> +#define CHECK(_mcdi)	({						       \
>> +	rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_ ## _mcdi],\
>> +					 MASK_ONES);			       \
>> +	if (rc)								       \
>> +		NL_SET_ERR_MSG_FMT_MOD(extack,				       \
>> +				       "No support for field %s", #_mcdi);     \
>> +	rc;								       \
>> +})
> Is there any reasone why macro is used instead of function? It is a
> little hard to read becasue it is modyfing rc value.

It makes its use more compact, as we can chain several calls with ||,
 and the short-circuiting means the first one to fail will set rc.
Perhaps less valuable here than in the efx_mae_match_check_caps()
 version, which has much longer ||-chains, but I thought it better to
 be consistent.

>> +	/* enc-keys are handled indirectly, through encap_match ID */
>> +	if (match->encap) {
>> +		MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID,
>> +				      match->encap->fw_id);
>> +		MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID_MASK,
>> +				      (u32)-1);
> U32_MAX can't be used here?

Yeah, it can, will change.  Thanks.

>> +	} else {
>> +		/* No enc-keys should appear in a rule without an encap_match */
>> +		if (WARN_ON_ONCE(match->mask.enc_src_ip) ||
>> +		    WARN_ON_ONCE(match->mask.enc_dst_ip) ||
>> +		    WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_src_ip6)) ||
>> +		    WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_dst_ip6)) ||
>> +		    WARN_ON_ONCE(match->mask.enc_ip_tos) ||
>> +		    WARN_ON_ONCE(match->mask.enc_ip_ttl) ||
>> +		    WARN_ON_ONCE(match->mask.enc_sport) ||
>> +		    WARN_ON_ONCE(match->mask.enc_dport) ||
>> +		    WARN_ON_ONCE(match->mask.enc_keyid))
>> +			return -EOPNOTSUPP;
> Can be written as else if {}

So it can.  Will change.

> Also, You define a similar function: efx_tc_match_is_encap(), I think it
> can be used here.

This way we get individualised warnings indicating which field is
 erroneously used, WARN_ON_ONCE(efx_tc_match_is_encap()) wouldn't
 give us that.

>> +struct efx_tc_encap_match {
>> +	__be32 src_ip, dst_ip;
>> +	struct in6_addr src_ip6, dst_ip6;
>> +	__be16 udp_dport;
> What about source port? It isn't supported?

The hardware can support it, but for simplicity, the initial driver
 implementation only allows one mask (set of keys), to make it easy
 to prevent two rules overlapping.  If there are optional keys or
 custom masks, then it's possible for two rules to both match the
 same packet, which causes undefined behaviour in some versions of
 the hardware.  We picked this key set as it appears to be what's
 used by a typical OvS deployment.
We do have some unsubmitted code that relaxes the driver limitation
 to allow an optional masked match on enc_ip_tos as long as the
 driver can prove there's no overlap with other rules; it should be
 possible to extend this to also allow an optional source port
 match.  I hope to follow up with this in a future patch series.

[PATCH net-next 2/5] sfc: handle enc keys in efx_tc_flower_parse_match()

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Translate the fields from flow dissector into struct efx_tc_match.
In efx_tc_flower_replace(), reject filters that match on them, because
 only 'foreign' filters (i.e. those for which the ingress dev is not
 the sfc netdev or any of its representors, e.g. a tunnel netdev) can
 use them.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/tc.c | 65 +++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index 2b07bb2fd735..d683665a8d87 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -193,6 +193,11 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 	      BIT(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
 	      BIT(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
 	      BIT(FLOW_DISSECTOR_KEY_PORTS) |
+	      BIT(FLOW_DISSECTOR_KEY_ENC_KEYID) |
+	      BIT(FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) |
+	      BIT(FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) |
+	      BIT(FLOW_DISSECTOR_KEY_ENC_PORTS) |
+	      BIT(FLOW_DISSECTOR_KEY_ENC_CONTROL) |
 	      BIT(FLOW_DISSECTOR_KEY_TCP) |
 	      BIT(FLOW_DISSECTOR_KEY_IP))) {
 		NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported flower keys %#x",
@@ -280,6 +285,61 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 	MAP_KEY_AND_MASK(PORTS, ports, src, l4_sport);
 	MAP_KEY_AND_MASK(PORTS, ports, dst, l4_dport);
 	MAP_KEY_AND_MASK(TCP, tcp, flags, tcp_flags);
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_CONTROL)) {
+		struct flow_match_control fm;
+
+		flow_rule_match_enc_control(rule, &fm);
+		if (fm.mask->flags) {
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported match on enc_control.flags %#x",
+					       fm.mask->flags);
+			return -EOPNOTSUPP;
+		}
+		if (!IS_ALL_ONES(fm.mask->addr_type)) {
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported enc addr_type mask %u (key %u)",
+					       fm.mask->addr_type,
+					       fm.key->addr_type);
+			return -EOPNOTSUPP;
+		}
+		switch (fm.key->addr_type) {
+		case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
+			MAP_ENC_KEY_AND_MASK(IPV4_ADDRS, ipv4_addrs, enc_ipv4_addrs,
+					     src, enc_src_ip);
+			MAP_ENC_KEY_AND_MASK(IPV4_ADDRS, ipv4_addrs, enc_ipv4_addrs,
+					     dst, enc_dst_ip);
+			break;
+#ifdef CONFIG_IPV6
+		case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
+			MAP_ENC_KEY_AND_MASK(IPV6_ADDRS, ipv6_addrs, enc_ipv6_addrs,
+					     src, enc_src_ip6);
+			MAP_ENC_KEY_AND_MASK(IPV6_ADDRS, ipv6_addrs, enc_ipv6_addrs,
+					     dst, enc_dst_ip6);
+			break;
+#endif
+		default:
+			NL_SET_ERR_MSG_FMT_MOD(extack,
+					       "Unsupported enc addr_type %u (supported are IPv4, IPv6)",
+					       fm.key->addr_type);
+			return -EOPNOTSUPP;
+		}
+#if !defined(EFX_USE_KCOMPAT) || defined(EFX_HAVE_FLOW_DISSECTOR_KEY_ENC_IP)
+		MAP_ENC_KEY_AND_MASK(IP, ip, enc_ip, tos, enc_ip_tos);
+		MAP_ENC_KEY_AND_MASK(IP, ip, enc_ip, ttl, enc_ip_ttl);
+#endif
+		MAP_ENC_KEY_AND_MASK(PORTS, ports, enc_ports, src, enc_sport);
+		MAP_ENC_KEY_AND_MASK(PORTS, ports, enc_ports, dst, enc_dport);
+		MAP_ENC_KEY_AND_MASK(KEYID, enc_keyid, enc_keyid, keyid, enc_keyid);
+	} else if (dissector->used_keys &
+		   (BIT(FLOW_DISSECTOR_KEY_ENC_KEYID) |
+		    BIT(FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) |
+		    BIT(FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) |
+#if !defined(EFX_USE_KCOMPAT) || defined(EFX_HAVE_FLOW_DISSECTOR_KEY_ENC_IP)
+		    BIT(FLOW_DISSECTOR_KEY_ENC_IP) |
+#endif
+		    BIT(FLOW_DISSECTOR_KEY_ENC_PORTS))) {
+		NL_SET_ERR_MSG_FMT_MOD(extack, "Flower enc keys require enc_control (keys: %#x)",
+				       dissector->used_keys);
+		return -EOPNOTSUPP;
+	}
 
 	return 0;
 }
@@ -373,6 +433,11 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 	rc = efx_tc_flower_parse_match(efx, fr, &match, extack);
 	if (rc)
 		return rc;
+	if (efx_tc_match_is_encap(&match.mask)) {
+		NL_SET_ERR_MSG_MOD(extack, "Ingress enc_key matches not supported");
+		rc = -EOPNOTSUPP;
+		goto release;
+	}
 
 	if (tc->common.chain_index) {
 		NL_SET_ERR_MSG_MOD(extack, "No support for nonzero chain_index");

Re: [PATCH net-next 2/5] sfc: handle enc keys in efx_tc_flower_parse_match()

[Michal Swiatkowski]

On Tue, Mar 14, 2023 at 05:35:22PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Translate the fields from flow dissector into struct efx_tc_match.
> In efx_tc_flower_replace(), reject filters that match on them, because
>  only 'foreign' filters (i.e. those for which the ingress dev is not
>  the sfc netdev or any of its representors, e.g. a tunnel netdev) can
>  use them.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/tc.c | 65 +++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
> index 2b07bb2fd735..d683665a8d87 100644
> --- a/drivers/net/ethernet/sfc/tc.c
> +++ b/drivers/net/ethernet/sfc/tc.c
> @@ -193,6 +193,11 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
>  	      BIT(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
>  	      BIT(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
>  	      BIT(FLOW_DISSECTOR_KEY_PORTS) |
> +	      BIT(FLOW_DISSECTOR_KEY_ENC_KEYID) |
> +	      BIT(FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) |
> +	      BIT(FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) |
> +	      BIT(FLOW_DISSECTOR_KEY_ENC_PORTS) |
> +	      BIT(FLOW_DISSECTOR_KEY_ENC_CONTROL) |
>  	      BIT(FLOW_DISSECTOR_KEY_TCP) |
>  	      BIT(FLOW_DISSECTOR_KEY_IP))) {
>  		NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported flower keys %#x",
> @@ -280,6 +285,61 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
>  	MAP_KEY_AND_MASK(PORTS, ports, src, l4_sport);
>  	MAP_KEY_AND_MASK(PORTS, ports, dst, l4_dport);
>  	MAP_KEY_AND_MASK(TCP, tcp, flags, tcp_flags);
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_CONTROL)) {
> +		struct flow_match_control fm;
> +
> +		flow_rule_match_enc_control(rule, &fm);
> +		if (fm.mask->flags) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported match on enc_control.flags %#x",
> +					       fm.mask->flags);
> +			return -EOPNOTSUPP;
> +		}
> +		if (!IS_ALL_ONES(fm.mask->addr_type)) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported enc addr_type mask %u (key %u)",
> +					       fm.mask->addr_type,
> +					       fm.key->addr_type);
> +			return -EOPNOTSUPP;
> +		}
> +		switch (fm.key->addr_type) {
> +		case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
> +			MAP_ENC_KEY_AND_MASK(IPV4_ADDRS, ipv4_addrs, enc_ipv4_addrs,
> +					     src, enc_src_ip);
> +			MAP_ENC_KEY_AND_MASK(IPV4_ADDRS, ipv4_addrs, enc_ipv4_addrs,
> +					     dst, enc_dst_ip);
> +			break;
> +#ifdef CONFIG_IPV6
> +		case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
> +			MAP_ENC_KEY_AND_MASK(IPV6_ADDRS, ipv6_addrs, enc_ipv6_addrs,
> +					     src, enc_src_ip6);
> +			MAP_ENC_KEY_AND_MASK(IPV6_ADDRS, ipv6_addrs, enc_ipv6_addrs,
> +					     dst, enc_dst_ip6);
> +			break;
> +#endif
> +		default:
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "Unsupported enc addr_type %u (supported are IPv4, IPv6)",
> +					       fm.key->addr_type);
> +			return -EOPNOTSUPP;
> +		}
> +#if !defined(EFX_USE_KCOMPAT) || defined(EFX_HAVE_FLOW_DISSECTOR_KEY_ENC_IP)
Are these defines already in kernel, or You want to add it to kconfig?
I can't find it in tree, aren't they some kind of OOT driver defines?

> +		MAP_ENC_KEY_AND_MASK(IP, ip, enc_ip, tos, enc_ip_tos);
> +		MAP_ENC_KEY_AND_MASK(IP, ip, enc_ip, ttl, enc_ip_ttl);
> +#endif
> +		MAP_ENC_KEY_AND_MASK(PORTS, ports, enc_ports, src, enc_sport);
> +		MAP_ENC_KEY_AND_MASK(PORTS, ports, enc_ports, dst, enc_dport);
> +		MAP_ENC_KEY_AND_MASK(KEYID, enc_keyid, enc_keyid, keyid, enc_keyid);
> +	} else if (dissector->used_keys &
> +		   (BIT(FLOW_DISSECTOR_KEY_ENC_KEYID) |
> +		    BIT(FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) |
> +		    BIT(FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) |
> +#if !defined(EFX_USE_KCOMPAT) || defined(EFX_HAVE_FLOW_DISSECTOR_KEY_ENC_IP)
> +		    BIT(FLOW_DISSECTOR_KEY_ENC_IP) |
> +#endif
> +		    BIT(FLOW_DISSECTOR_KEY_ENC_PORTS))) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "Flower enc keys require enc_control (keys: %#x)",
> +				       dissector->used_keys);
> +		return -EOPNOTSUPP;
> +	}
>  
>  	return 0;
>  }
> @@ -373,6 +433,11 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
>  	rc = efx_tc_flower_parse_match(efx, fr, &match, extack);
>  	if (rc)
>  		return rc;
> +	if (efx_tc_match_is_encap(&match.mask)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Ingress enc_key matches not supported");
> +		rc = -EOPNOTSUPP;
> +		goto release;
> +	}
>  
>  	if (tc->common.chain_index) {
>  		NL_SET_ERR_MSG_MOD(extack, "No support for nonzero chain_index");

Re: [PATCH net-next 2/5] sfc: handle enc keys in efx_tc_flower_parse_match()

[Edward Cree]

On 15/03/2023 09:01, Michal Swiatkowski wrote:
> On Tue, Mar 14, 2023 at 05:35:22PM +0000, edward.cree@amd.com wrote:
>> +#if !defined(EFX_USE_KCOMPAT) || defined(EFX_HAVE_FLOW_DISSECTOR_KEY_ENC_IP)
> Are these defines already in kernel, or You want to add it to kconfig?
> I can't find it in tree, aren't they some kind of OOT driver defines?

Whoops, yes, that's from our OOT driver, it's part of the machinery we
 use to make it build on older kernels.
Embarrassing copy-paste error that slipped through internal review :(
Will remove all instances of this in v2.  Thanks for catching it!

[PATCH net-next 3/5] sfc: add functions to insert encap matches into the MAE

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

An encap match corresponds to an entry in the exact-match Outer Rule
 table; the lookup response includes the encap type (protocol) allowing
 the hardware to continue parsing into the inner headers.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/mae.c | 105 +++++++++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/mae.h |   5 ++
 drivers/net/ethernet/sfc/tc.h  |   1 +
 3 files changed, 111 insertions(+)

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 1a285facda34..754391eb575f 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -564,6 +564,20 @@ int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
 	return 0;
 }
 
+static int efx_mae_encap_type_to_mae_type(enum efx_encap_type type)
+{
+	switch (type & EFX_ENCAP_TYPES_MASK) {
+	case EFX_ENCAP_TYPE_NONE:
+		return MAE_MCDI_ENCAP_TYPE_NONE;
+	case EFX_ENCAP_TYPE_VXLAN:
+		return MAE_MCDI_ENCAP_TYPE_VXLAN;
+	case EFX_ENCAP_TYPE_GENEVE:
+		return MAE_MCDI_ENCAP_TYPE_GENEVE;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 int efx_mae_lookup_mport(struct efx_nic *efx, u32 vf_idx, u32 *id)
 {
 	struct ef100_nic_data *nic_data = efx->nic_data;
@@ -921,6 +935,97 @@ int efx_mae_free_action_set_list(struct efx_nic *efx,
 	return 0;
 }
 
+int efx_mae_register_encap_match(struct efx_nic *efx,
+				 struct efx_tc_encap_match *encap)
+{
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_OUTER_RULE_INSERT_IN_LEN(MAE_ENC_FIELD_PAIRS_LEN));
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_OUTER_RULE_INSERT_OUT_LEN);
+	MCDI_DECLARE_STRUCT_PTR(match_crit);
+	size_t outlen;
+	int rc;
+
+	rc = efx_mae_encap_type_to_mae_type(encap->tun_type);
+	if (rc < 0)
+		return rc;
+	match_crit = _MCDI_DWORD(inbuf, MAE_OUTER_RULE_INSERT_IN_FIELD_MATCH_CRITERIA);
+	/* The struct contains IP src and dst, and udp dport.
+	 * So we actually need to filter on IP src and dst, L4 dport, and
+	 * ipproto == udp.
+	 */
+	MCDI_SET_DWORD(inbuf, MAE_OUTER_RULE_INSERT_IN_ENCAP_TYPE, rc);
+#ifdef CONFIG_IPV6
+	if (encap->src_ip | encap->dst_ip) {
+#endif
+		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP4_BE,
+					 encap->src_ip);
+		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP4_BE_MASK,
+					 ~(__be32)0);
+		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP4_BE,
+					 encap->dst_ip);
+		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP4_BE_MASK,
+					 ~(__be32)0);
+		MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE,
+					htons(ETH_P_IP));
+#ifdef CONFIG_IPV6
+	} else {
+		memcpy(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP6_BE),
+		       &encap->src_ip6, sizeof(encap->src_ip6));
+		memset(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP6_BE_MASK),
+		       0xff, sizeof(encap->src_ip6));
+		memcpy(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP6_BE),
+		       &encap->dst_ip6, sizeof(encap->dst_ip6));
+		memset(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP6_BE_MASK),
+		       0xff, sizeof(encap->dst_ip6));
+		MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE,
+					htons(ETH_P_IPV6));
+	}
+#endif
+	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE_MASK,
+				~(__be16)0);
+	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_L4_DPORT_BE,
+				encap->udp_dport);
+	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_L4_DPORT_BE_MASK,
+				~(__be16)0);
+	MCDI_STRUCT_SET_BYTE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_IP_PROTO, IPPROTO_UDP);
+	MCDI_STRUCT_SET_BYTE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_IP_PROTO_MASK, ~0);
+	rc = efx_mcdi_rpc(efx, MC_CMD_MAE_OUTER_RULE_INSERT, inbuf,
+			  sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
+	if (rc)
+		return rc;
+	if (outlen < sizeof(outbuf))
+		return -EIO;
+	encap->fw_id = MCDI_DWORD(outbuf, MAE_OUTER_RULE_INSERT_OUT_OR_ID);
+	return 0;
+}
+
+int efx_mae_unregister_encap_match(struct efx_nic *efx,
+				   struct efx_tc_encap_match *encap)
+{
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_OUTER_RULE_REMOVE_OUT_LEN(1));
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_OUTER_RULE_REMOVE_IN_LEN(1));
+	size_t outlen;
+	int rc;
+
+	MCDI_SET_DWORD(inbuf, MAE_OUTER_RULE_REMOVE_IN_OR_ID, encap->fw_id);
+	rc = efx_mcdi_rpc(efx, MC_CMD_MAE_OUTER_RULE_REMOVE, inbuf,
+			  sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
+	if (rc)
+		return rc;
+	if (outlen < sizeof(outbuf))
+		return -EIO;
+	/* FW freed a different ID than we asked for, should also never happen.
+	 * Warn because it means we've now got a different idea to the FW of
+	 * what encap_mds exist, which could cause mayhem later.
+	 */
+	if (WARN_ON(MCDI_DWORD(outbuf, MAE_OUTER_RULE_REMOVE_OUT_REMOVED_OR_ID) != encap->fw_id))
+		return -EIO;
+	/* We're probably about to free @encap, but let's just make sure its
+	 * fw_id is blatted so that it won't look valid if it leaks out.
+	 */
+	encap->fw_id = MC_CMD_MAE_OUTER_RULE_INSERT_OUT_OUTER_RULE_ID_NULL;
+	return 0;
+}
+
 static int efx_mae_populate_match_criteria(MCDI_DECLARE_STRUCT_PTR(match_crit),
 					   const struct efx_tc_match *match)
 {
diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
index a45d1791517f..5b45138aaaf4 100644
--- a/drivers/net/ethernet/sfc/mae.h
+++ b/drivers/net/ethernet/sfc/mae.h
@@ -94,6 +94,11 @@ int efx_mae_alloc_action_set_list(struct efx_nic *efx,
 int efx_mae_free_action_set_list(struct efx_nic *efx,
 				 struct efx_tc_action_set_list *acts);
 
+int efx_mae_register_encap_match(struct efx_nic *efx,
+				 struct efx_tc_encap_match *encap);
+int efx_mae_unregister_encap_match(struct efx_nic *efx,
+				   struct efx_tc_encap_match *encap);
+
 int efx_mae_insert_rule(struct efx_nic *efx, const struct efx_tc_match *match,
 			u32 prio, u32 acts_id, u32 *id);
 int efx_mae_delete_rule(struct efx_nic *efx, u32 id);
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index c1485679507c..19782c9a4354 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -70,6 +70,7 @@ struct efx_tc_encap_match {
 	__be32 src_ip, dst_ip;
 	struct in6_addr src_ip6, dst_ip6;
 	__be16 udp_dport;
+	u16 tun_type; /* enum efx_encap_type */
 	u32 fw_id; /* index of this entry in firmware encap match table */
 };
 

Re: [PATCH net-next 3/5] sfc: add functions to insert encap matches into the MAE

[Michal Swiatkowski]

On Tue, Mar 14, 2023 at 05:35:23PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> An encap match corresponds to an entry in the exact-match Outer Rule
>  table; the lookup response includes the encap type (protocol) allowing
>  the hardware to continue parsing into the inner headers.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/mae.c | 105 +++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/sfc/mae.h |   5 ++
>  drivers/net/ethernet/sfc/tc.h  |   1 +
>  3 files changed, 111 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
> index 1a285facda34..754391eb575f 100644
> --- a/drivers/net/ethernet/sfc/mae.c
> +++ b/drivers/net/ethernet/sfc/mae.c
> @@ -564,6 +564,20 @@ int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
>  	return 0;
>  }
>  
> +static int efx_mae_encap_type_to_mae_type(enum efx_encap_type type)
> +{
> +	switch (type & EFX_ENCAP_TYPES_MASK) {
> +	case EFX_ENCAP_TYPE_NONE:
> +		return MAE_MCDI_ENCAP_TYPE_NONE;
> +	case EFX_ENCAP_TYPE_VXLAN:
> +		return MAE_MCDI_ENCAP_TYPE_VXLAN;
> +	case EFX_ENCAP_TYPE_GENEVE:
> +		return MAE_MCDI_ENCAP_TYPE_GENEVE;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  int efx_mae_lookup_mport(struct efx_nic *efx, u32 vf_idx, u32 *id)
>  {
>  	struct ef100_nic_data *nic_data = efx->nic_data;
> @@ -921,6 +935,97 @@ int efx_mae_free_action_set_list(struct efx_nic *efx,
>  	return 0;
>  }
>  
> +int efx_mae_register_encap_match(struct efx_nic *efx,
> +				 struct efx_tc_encap_match *encap)
> +{
> +	MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_OUTER_RULE_INSERT_IN_LEN(MAE_ENC_FIELD_PAIRS_LEN));
> +	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_OUTER_RULE_INSERT_OUT_LEN);
> +	MCDI_DECLARE_STRUCT_PTR(match_crit);
> +	size_t outlen;
> +	int rc;
> +
> +	rc = efx_mae_encap_type_to_mae_type(encap->tun_type);
> +	if (rc < 0)
> +		return rc;
> +	match_crit = _MCDI_DWORD(inbuf, MAE_OUTER_RULE_INSERT_IN_FIELD_MATCH_CRITERIA);
> +	/* The struct contains IP src and dst, and udp dport.
> +	 * So we actually need to filter on IP src and dst, L4 dport, and
> +	 * ipproto == udp.
> +	 */
> +	MCDI_SET_DWORD(inbuf, MAE_OUTER_RULE_INSERT_IN_ENCAP_TYPE, rc);
> +#ifdef CONFIG_IPV6
> +	if (encap->src_ip | encap->dst_ip) {
> +#endif
Looks strange, in case CONFIG_IPV6 isn't defined You can also check if
theres is no zero ip.

> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP4_BE,
> +					 encap->src_ip);
> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP4_BE_MASK,
> +					 ~(__be32)0);
> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP4_BE,
> +					 encap->dst_ip);
> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP4_BE_MASK,
> +					 ~(__be32)0);
> +		MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE,
> +					htons(ETH_P_IP));
> +#ifdef CONFIG_IPV6
> +	} else {
> +		memcpy(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP6_BE),
> +		       &encap->src_ip6, sizeof(encap->src_ip6));
> +		memset(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP6_BE_MASK),
> +		       0xff, sizeof(encap->src_ip6));
> +		memcpy(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP6_BE),
> +		       &encap->dst_ip6, sizeof(encap->dst_ip6));
> +		memset(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP6_BE_MASK),
> +		       0xff, sizeof(encap->dst_ip6));
> +		MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE,
> +					htons(ETH_P_IPV6));
> +	}
> +#endif
> +	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE_MASK,
> +				~(__be16)0);
> +	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_L4_DPORT_BE,
> +				encap->udp_dport);
> +	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_L4_DPORT_BE_MASK,
> +				~(__be16)0);
Question, from tc we can set masks for matching fields. You are setting
default one, because hardware doesn't support different masks?

> +	MCDI_STRUCT_SET_BYTE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_IP_PROTO, IPPROTO_UDP);
> +	MCDI_STRUCT_SET_BYTE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_IP_PROTO_MASK, ~0);
> +	rc = efx_mcdi_rpc(efx, MC_CMD_MAE_OUTER_RULE_INSERT, inbuf,
> +			  sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
> +	if (rc)
> +		return rc;
> +	if (outlen < sizeof(outbuf))
> +		return -EIO;
> +	encap->fw_id = MCDI_DWORD(outbuf, MAE_OUTER_RULE_INSERT_OUT_OR_ID);
> +	return 0;
> +}
> +
> +int efx_mae_unregister_encap_match(struct efx_nic *efx,
> +				   struct efx_tc_encap_match *encap)
> +{
> +	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_OUTER_RULE_REMOVE_OUT_LEN(1));
> +	MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_OUTER_RULE_REMOVE_IN_LEN(1));
> +	size_t outlen;
> +	int rc;
> +
> +	MCDI_SET_DWORD(inbuf, MAE_OUTER_RULE_REMOVE_IN_OR_ID, encap->fw_id);
> +	rc = efx_mcdi_rpc(efx, MC_CMD_MAE_OUTER_RULE_REMOVE, inbuf,
> +			  sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
> +	if (rc)
> +		return rc;
> +	if (outlen < sizeof(outbuf))
> +		return -EIO;
> +	/* FW freed a different ID than we asked for, should also never happen.
> +	 * Warn because it means we've now got a different idea to the FW of
> +	 * what encap_mds exist, which could cause mayhem later.
> +	 */
> +	if (WARN_ON(MCDI_DWORD(outbuf, MAE_OUTER_RULE_REMOVE_OUT_REMOVED_OR_ID) != encap->fw_id))
> +		return -EIO;
> +	/* We're probably about to free @encap, but let's just make sure its
> +	 * fw_id is blatted so that it won't look valid if it leaks out.
> +	 */
> +	encap->fw_id = MC_CMD_MAE_OUTER_RULE_INSERT_OUT_OUTER_RULE_ID_NULL;
> +	return 0;
> +}
> +
>  static int efx_mae_populate_match_criteria(MCDI_DECLARE_STRUCT_PTR(match_crit),
>  					   const struct efx_tc_match *match)
>  {
> diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
> index a45d1791517f..5b45138aaaf4 100644
> --- a/drivers/net/ethernet/sfc/mae.h
> +++ b/drivers/net/ethernet/sfc/mae.h
> @@ -94,6 +94,11 @@ int efx_mae_alloc_action_set_list(struct efx_nic *efx,
>  int efx_mae_free_action_set_list(struct efx_nic *efx,
>  				 struct efx_tc_action_set_list *acts);
>  
> +int efx_mae_register_encap_match(struct efx_nic *efx,
> +				 struct efx_tc_encap_match *encap);
> +int efx_mae_unregister_encap_match(struct efx_nic *efx,
> +				   struct efx_tc_encap_match *encap);
> +
>  int efx_mae_insert_rule(struct efx_nic *efx, const struct efx_tc_match *match,
>  			u32 prio, u32 acts_id, u32 *id);
>  int efx_mae_delete_rule(struct efx_nic *efx, u32 id);
> diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
> index c1485679507c..19782c9a4354 100644
> --- a/drivers/net/ethernet/sfc/tc.h
> +++ b/drivers/net/ethernet/sfc/tc.h
> @@ -70,6 +70,7 @@ struct efx_tc_encap_match {
>  	__be32 src_ip, dst_ip;
>  	struct in6_addr src_ip6, dst_ip6;
>  	__be16 udp_dport;
> +	u16 tun_type; /* enum efx_encap_type */
>  	u32 fw_id; /* index of this entry in firmware encap match table */
>  };
>  

Re: [PATCH net-next 3/5] sfc: add functions to insert encap matches into the MAE

[Edward Cree]

On 15/03/2023 09:23, Michal Swiatkowski wrote:
> On Tue, Mar 14, 2023 at 05:35:23PM +0000, edward.cree@amd.com wrote:
>> +#ifdef CONFIG_IPV6
>> +	if (encap->src_ip | encap->dst_ip) {
>> +#endif
> Looks strange, in case CONFIG_IPV6 isn't defined You can also check if
> theres is no zero ip.

The idea is that #ifdef CONFIG_IPV6 then we use this to decide whether
 this is an IPv4 or IPv6 filter, otherwise we don't need to check
 anything because there's only IPv4.  What would the alternative be,
 put a WARN_ON_ONCE() and return -EINVAL in the else clause #ifndef
 CONFIG_IPV6?  Is that better?
I agree this does look strange.

> 
>> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP4_BE,
>> +					 encap->src_ip);
>> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP4_BE_MASK,
>> +					 ~(__be32)0);
>> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP4_BE,
>> +					 encap->dst_ip);
>> +		MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP4_BE_MASK,
>> +					 ~(__be32)0);
>> +		MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE,
>> +					htons(ETH_P_IP));
>> +#ifdef CONFIG_IPV6
>> +	} else {
>> +		memcpy(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP6_BE),
>> +		       &encap->src_ip6, sizeof(encap->src_ip6));
>> +		memset(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_SRC_IP6_BE_MASK),
>> +		       0xff, sizeof(encap->src_ip6));
>> +		memcpy(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP6_BE),
>> +		       &encap->dst_ip6, sizeof(encap->dst_ip6));
>> +		memset(MCDI_STRUCT_PTR(match_crit, MAE_ENC_FIELD_PAIRS_ENC_DST_IP6_BE_MASK),
>> +		       0xff, sizeof(encap->dst_ip6));
>> +		MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE,
>> +					htons(ETH_P_IPV6));
>> +	}
>> +#endif
>> +	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_ETHER_TYPE_BE_MASK,
>> +				~(__be16)0);
>> +	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_L4_DPORT_BE,
>> +				encap->udp_dport);
>> +	MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_ENC_FIELD_PAIRS_ENC_L4_DPORT_BE_MASK,
>> +				~(__be16)0);
> Question, from tc we can set masks for matching fields. You are setting
> default one, because hardware doesn't support different masks?

See my reply on patch #1 about mask sets and overlap.
The hardware supports masks on some fields in the Outer Rule table,
 although not (in the current version) L4 ports.

[PATCH net-next 4/5] sfc: add code to register and unregister encap matches

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Add a hashtable to detect duplicate and conflicting matches.  If match
 is not a duplicate, call MAE functions to add/remove it from OR table.
Calling code not added yet, so mark the new functions as unused.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/tc.c | 176 ++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/tc.h |  11 +++
 2 files changed, 187 insertions(+)

diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index d683665a8d87..dc092403af12 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -57,6 +57,12 @@ static s64 efx_tc_flower_external_mport(struct efx_nic *efx, struct efx_rep *efv
 	return mport;
 }
 
+static const struct rhashtable_params efx_tc_encap_match_ht_params = {
+	.key_len	= offsetof(struct efx_tc_encap_match, linkage),
+	.key_offset	= 0,
+	.head_offset	= offsetof(struct efx_tc_encap_match, linkage),
+};
+
 static const struct rhashtable_params efx_tc_match_action_ht_params = {
 	.key_len	= sizeof(unsigned long),
 	.key_offset	= offsetof(struct efx_tc_flow_rule, cookie),
@@ -344,6 +350,157 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 	return 0;
 }
 
+__always_unused
+static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
+					    struct efx_tc_match *match,
+					    enum efx_encap_type type,
+					    struct netlink_ext_ack *extack)
+{
+	struct efx_tc_encap_match *encap, *old;
+	unsigned char ipv;
+	int rc;
+
+	/* We require that the socket-defining fields (IP addrs and UDP dest
+	 * port) are present and exact-match.  Other fields are currently not
+	 * allowed.  This meets what OVS will ask for, and means that we don't
+	 * need to handle difficult checks for overlapping matches as could
+	 * come up if we allowed masks or varying sets of match fields.
+	 */
+	if (match->mask.enc_dst_ip | match->mask.enc_src_ip) {
+		ipv = 4;
+		if (!IS_ALL_ONES(match->mask.enc_dst_ip)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Egress encap match is not exact on dst IP address");
+			return -EOPNOTSUPP;
+		}
+		if (!IS_ALL_ONES(match->mask.enc_src_ip)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Egress encap match is not exact on src IP address");
+			return -EOPNOTSUPP;
+		}
+#ifdef CONFIG_IPV6
+		if (!ipv6_addr_any(&match->mask.enc_dst_ip6) ||
+		    !ipv6_addr_any(&match->mask.enc_src_ip6)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Egress encap match on both IPv4 and IPv6, don't understand");
+			return -EOPNOTSUPP;
+		}
+	} else {
+		ipv = 6;
+		if (!efx_ipv6_addr_all_ones(&match->mask.enc_dst_ip6)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Egress encap match is not exact on dst IP address");
+			return -EOPNOTSUPP;
+		}
+		if (!efx_ipv6_addr_all_ones(&match->mask.enc_src_ip6)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Egress encap match is not exact on src IP address");
+			return -EOPNOTSUPP;
+		}
+#endif
+	}
+	if (!IS_ALL_ONES(match->mask.enc_dport)) {
+		NL_SET_ERR_MSG_MOD(extack, "Egress encap match is not exact on dst UDP port");
+		return -EOPNOTSUPP;
+	}
+	if (match->mask.enc_sport) {
+		NL_SET_ERR_MSG_MOD(extack, "Egress encap match on src UDP port not supported");
+		return -EOPNOTSUPP;
+	}
+	if (match->mask.enc_ip_tos) {
+		NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP ToS not supported");
+		return -EOPNOTSUPP;
+	}
+	if (match->mask.enc_ip_ttl) {
+		NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP TTL not supported");
+		return -EOPNOTSUPP;
+	}
+
+	rc = efx_mae_check_encap_match_caps(efx, ipv, extack);
+	if (rc) {
+		NL_SET_ERR_MSG_FMT_MOD(extack, "MAE hw reports no support for IPv%d encap matches",
+				       ipv);
+		return -EOPNOTSUPP;
+	}
+
+	encap = kzalloc(sizeof(*encap), GFP_USER);
+	if (!encap)
+		return -ENOMEM;
+	switch (ipv) {
+	case 4:
+		encap->src_ip = match->value.enc_src_ip;
+		encap->dst_ip = match->value.enc_dst_ip;
+		break;
+#ifdef CONFIG_IPV6
+	case 6:
+		encap->src_ip6 = match->value.enc_src_ip6;
+		encap->dst_ip6 = match->value.enc_dst_ip6;
+		break;
+#endif
+	default: /* can't happen */
+		NL_SET_ERR_MSG_FMT_MOD(extack, "Egress encap match on bad IP version %d",
+				       ipv);
+		rc = -EOPNOTSUPP;
+		goto fail_allocated;
+	}
+	encap->udp_dport = match->value.enc_dport;
+	encap->tun_type = type;
+	old = rhashtable_lookup_get_insert_fast(&efx->tc->encap_match_ht,
+						&encap->linkage,
+						efx_tc_encap_match_ht_params);
+	if (old) {
+		/* don't need our new entry */
+		kfree(encap);
+		if (old->tun_type != type) {
+			NL_SET_ERR_MSG_FMT_MOD(extack,
+					       "Egress encap match with conflicting tun_type %u != %u",
+					       old->tun_type, type);
+			return -EEXIST;
+		}
+		if (!refcount_inc_not_zero(&old->ref))
+			return -EAGAIN;
+		/* existing entry found */
+		encap = old;
+	} else {
+		rc = efx_mae_register_encap_match(efx, encap);
+		if (rc) {
+			NL_SET_ERR_MSG_MOD(extack, "Failed to record egress encap match in HW");
+			goto fail_inserted;
+		}
+		refcount_set(&encap->ref, 1);
+	}
+	match->encap = encap;
+	return 0;
+fail_inserted:
+	rhashtable_remove_fast(&efx->tc->encap_match_ht, &encap->linkage,
+			       efx_tc_encap_match_ht_params);
+fail_allocated:
+	kfree(encap);
+	return rc;
+}
+
+__always_unused
+static void efx_tc_flower_release_encap_match(struct efx_nic *efx,
+					      struct efx_tc_encap_match *encap)
+{
+	int rc;
+
+	if (!refcount_dec_and_test(&encap->ref))
+		return; /* still in use */
+
+	rc = efx_mae_unregister_encap_match(efx, encap);
+	if (rc)
+		/* Display message but carry on and remove entry from our
+		 * SW tables, because there's not much we can do about it.
+		 */
+		netif_err(efx, drv, efx->net_dev,
+			  "Failed to release encap match %#x, rc %d\n",
+			  encap->fw_id, rc);
+	rhashtable_remove_fast(&efx->tc->encap_match_ht, &encap->linkage,
+			       efx_tc_encap_match_ht_params);
+	kfree(encap);
+}
+
 /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
 enum efx_tc_action_order {
 	EFX_TC_AO_VLAN_POP,
@@ -954,6 +1111,18 @@ void efx_fini_tc(struct efx_nic *efx)
 	efx->tc->up = false;
 }
 
+/* At teardown time, all TC filter rules (and thus all resources they created)
+ * should already have been removed.  If we find any in our hashtables, make a
+ * cursory attempt to clean up the software side.
+ */
+static void efx_tc_encap_match_free(void *ptr, void *__unused)
+{
+	struct efx_tc_encap_match *encap = ptr;
+
+	WARN_ON(refcount_read(&encap->ref));
+	kfree(encap);
+}
+
 int efx_init_struct_tc(struct efx_nic *efx)
 {
 	int rc;
@@ -976,6 +1145,9 @@ int efx_init_struct_tc(struct efx_nic *efx)
 	rc = efx_tc_init_counters(efx);
 	if (rc < 0)
 		goto fail_counters;
+	rc = rhashtable_init(&efx->tc->encap_match_ht, &efx_tc_encap_match_ht_params);
+	if (rc < 0)
+		goto fail_encap_match_ht;
 	rc = rhashtable_init(&efx->tc->match_action_ht, &efx_tc_match_action_ht_params);
 	if (rc < 0)
 		goto fail_match_action_ht;
@@ -988,6 +1160,8 @@ int efx_init_struct_tc(struct efx_nic *efx)
 	efx->extra_channel_type[EFX_EXTRA_CHANNEL_TC] = &efx_tc_channel_type;
 	return 0;
 fail_match_action_ht:
+	rhashtable_destroy(&efx->tc->encap_match_ht);
+fail_encap_match_ht:
 	efx_tc_destroy_counters(efx);
 fail_counters:
 	mutex_destroy(&efx->tc->mutex);
@@ -1010,6 +1184,8 @@ void efx_fini_struct_tc(struct efx_nic *efx)
 			     MC_CMD_MAE_ACTION_RULE_INSERT_OUT_ACTION_RULE_ID_NULL);
 	rhashtable_free_and_destroy(&efx->tc->match_action_ht, efx_tc_flow_free,
 				    efx);
+	rhashtable_free_and_destroy(&efx->tc->encap_match_ht,
+				    efx_tc_encap_match_free, NULL);
 	efx_tc_fini_counters(efx);
 	mutex_unlock(&efx->tc->mutex);
 	mutex_destroy(&efx->tc->mutex);
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index 19782c9a4354..d70c0ba86669 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -18,6 +18,13 @@
 
 #define IS_ALL_ONES(v)	(!(typeof (v))~(v))
 
+#ifdef CONFIG_IPV6
+static inline bool efx_ipv6_addr_all_ones(struct in6_addr *addr)
+{
+	return !memchr_inv(addr, 0xff, sizeof(*addr));
+}
+#endif
+
 struct efx_tc_action_set {
 	u16 vlan_push:2;
 	u16 vlan_pop:2;
@@ -70,7 +77,9 @@ struct efx_tc_encap_match {
 	__be32 src_ip, dst_ip;
 	struct in6_addr src_ip6, dst_ip6;
 	__be16 udp_dport;
+	struct rhash_head linkage;
 	u16 tun_type; /* enum efx_encap_type */
+	refcount_t ref;
 	u32 fw_id; /* index of this entry in firmware encap match table */
 };
 
@@ -107,6 +116,7 @@ enum efx_tc_rule_prios {
  * @mutex: Used to serialise operations on TC hashtables
  * @counter_ht: Hashtable of TC counters (FW IDs and counter values)
  * @counter_id_ht: Hashtable mapping TC counter cookies to counters
+ * @encap_match_ht: Hashtable of TC encap matches
  * @match_action_ht: Hashtable of TC match-action rules
  * @reps_mport_id: MAE port allocated for representor RX
  * @reps_filter_uc: VNIC filter for representor unicast RX (promisc)
@@ -130,6 +140,7 @@ struct efx_tc_state {
 	struct mutex mutex;
 	struct rhashtable counter_ht;
 	struct rhashtable counter_id_ht;
+	struct rhashtable encap_match_ht;
 	struct rhashtable match_action_ht;
 	u32 reps_mport_id, reps_mport_vport_id;
 	s32 reps_filter_uc, reps_filter_mc;

Re: [PATCH net-next 4/5] sfc: add code to register and unregister encap matches

[Michal Swiatkowski]

On Tue, Mar 14, 2023 at 05:35:24PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Add a hashtable to detect duplicate and conflicting matches.  If match
>  is not a duplicate, call MAE functions to add/remove it from OR table.
> Calling code not added yet, so mark the new functions as unused.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/tc.c | 176 ++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/sfc/tc.h |  11 +++
>  2 files changed, 187 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
> index d683665a8d87..dc092403af12 100644
> --- a/drivers/net/ethernet/sfc/tc.c
> +++ b/drivers/net/ethernet/sfc/tc.c
> @@ -57,6 +57,12 @@ static s64 efx_tc_flower_external_mport(struct efx_nic *efx, struct efx_rep *efv
>  	return mport;
>  }
>  
> +static const struct rhashtable_params efx_tc_encap_match_ht_params = {
> +	.key_len	= offsetof(struct efx_tc_encap_match, linkage),
> +	.key_offset	= 0,
> +	.head_offset	= offsetof(struct efx_tc_encap_match, linkage),
> +};
> +
>  static const struct rhashtable_params efx_tc_match_action_ht_params = {
>  	.key_len	= sizeof(unsigned long),
>  	.key_offset	= offsetof(struct efx_tc_flow_rule, cookie),
> @@ -344,6 +350,157 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
>  	return 0;
>  }
>  
> +__always_unused
> +static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
> +					    struct efx_tc_match *match,
> +					    enum efx_encap_type type,
> +					    struct netlink_ext_ack *extack)
> +{
> +	struct efx_tc_encap_match *encap, *old;
> +	unsigned char ipv;
int? or even boolean is_ipv4

> +	int rc;
> +
> +	/* We require that the socket-defining fields (IP addrs and UDP dest
> +	 * port) are present and exact-match.  Other fields are currently not
> +	 * allowed.  This meets what OVS will ask for, and means that we don't
> +	 * need to handle difficult checks for overlapping matches as could
> +	 * come up if we allowed masks or varying sets of match fields.
> +	 */
> +	if (match->mask.enc_dst_ip | match->mask.enc_src_ip) {
> +		ipv = 4;
> +		if (!IS_ALL_ONES(match->mask.enc_dst_ip)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Egress encap match is not exact on dst IP address");
> +			return -EOPNOTSUPP;
> +		}
> +		if (!IS_ALL_ONES(match->mask.enc_src_ip)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Egress encap match is not exact on src IP address");
Do You mean that only exact match is supported?

> +			return -EOPNOTSUPP;
> +		}
> +#ifdef CONFIG_IPV6
> +		if (!ipv6_addr_any(&match->mask.enc_dst_ip6) ||
> +		    !ipv6_addr_any(&match->mask.enc_src_ip6)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Egress encap match on both IPv4 and IPv6, don't understand");
> +			return -EOPNOTSUPP;
> +		}
> +	} else {
> +		ipv = 6;
> +		if (!efx_ipv6_addr_all_ones(&match->mask.enc_dst_ip6)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Egress encap match is not exact on dst IP address");
> +			return -EOPNOTSUPP;
> +		}
> +		if (!efx_ipv6_addr_all_ones(&match->mask.enc_src_ip6)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Egress encap match is not exact on src IP address");
> +			return -EOPNOTSUPP;
> +		}
> +#endif
> +	}
> +	if (!IS_ALL_ONES(match->mask.enc_dport)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Egress encap match is not exact on dst UDP port");
> +		return -EOPNOTSUPP;
> +	}
> +	if (match->mask.enc_sport) {
> +		NL_SET_ERR_MSG_MOD(extack, "Egress encap match on src UDP port not supported");
> +		return -EOPNOTSUPP;
> +	}
> +	if (match->mask.enc_ip_tos) {
> +		NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP ToS not supported");
> +		return -EOPNOTSUPP;
> +	}
> +	if (match->mask.enc_ip_ttl) {
> +		NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP TTL not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	rc = efx_mae_check_encap_match_caps(efx, ipv, extack);
> +	if (rc) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "MAE hw reports no support for IPv%d encap matches",
> +				       ipv);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	encap = kzalloc(sizeof(*encap), GFP_USER);
> +	if (!encap)
> +		return -ENOMEM;
> +	switch (ipv) {
> +	case 4:
> +		encap->src_ip = match->value.enc_src_ip;
> +		encap->dst_ip = match->value.enc_dst_ip;
> +		break;
> +#ifdef CONFIG_IPV6
> +	case 6:
> +		encap->src_ip6 = match->value.enc_src_ip6;
> +		encap->dst_ip6 = match->value.enc_dst_ip6;
> +		break;
> +#endif
> +	default: /* can't happen */
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "Egress encap match on bad IP version %d",
> +				       ipv);
> +		rc = -EOPNOTSUPP;
> +		goto fail_allocated;
I will rewrite it to if. You will get rid of this unreachable code.

> +	}
> +	encap->udp_dport = match->value.enc_dport;
> +	encap->tun_type = type;
> +	old = rhashtable_lookup_get_insert_fast(&efx->tc->encap_match_ht,
> +						&encap->linkage,
> +						efx_tc_encap_match_ht_params);
> +	if (old) {
> +		/* don't need our new entry */
> +		kfree(encap);
> +		if (old->tun_type != type) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "Egress encap match with conflicting tun_type %u != %u",
> +					       old->tun_type, type);
> +			return -EEXIST;
> +		}
> +		if (!refcount_inc_not_zero(&old->ref))
> +			return -EAGAIN;
> +		/* existing entry found */
> +		encap = old;
> +	} else {
> +		rc = efx_mae_register_encap_match(efx, encap);
> +		if (rc) {
> +			NL_SET_ERR_MSG_MOD(extack, "Failed to record egress encap match in HW");
> +			goto fail_inserted;
> +		}
> +		refcount_set(&encap->ref, 1);
> +	}
> +	match->encap = encap;
> +	return 0;
> +fail_inserted:
> +	rhashtable_remove_fast(&efx->tc->encap_match_ht, &encap->linkage,
> +			       efx_tc_encap_match_ht_params);
> +fail_allocated:
> +	kfree(encap);
> +	return rc;
> +}
> +
[...]

Re: [PATCH net-next 4/5] sfc: add code to register and unregister encap matches

[Edward Cree]

On 15/03/2023 09:43, Michal Swiatkowski wrote:
> On Tue, Mar 14, 2023 at 05:35:24PM +0000, edward.cree@amd.com wrote:
>> +static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
>> +					    struct efx_tc_match *match,
>> +					    enum efx_encap_type type,
>> +					    struct netlink_ext_ack *extack)
>> +{
>> +	struct efx_tc_encap_match *encap, *old;
>> +	unsigned char ipv;
> int? or even boolean is_ipv4
...
>> +	default: /* can't happen */
>> +		NL_SET_ERR_MSG_FMT_MOD(extack, "Egress encap match on bad IP version %d",
>> +				       ipv);
>> +		rc = -EOPNOTSUPP;
>> +		goto fail_allocated;
> I will rewrite it to if. You will get rid of this unreachable code.

Yeah, that's probably better.  Will do.

[PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap) rules

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

A 'foreign' rule is one for which the net_dev is not the sfc netdevice
 or any of its representors.  The driver registers indirect flow blocks
 for tunnel netdevs so that it can offload decap rules.  For example:

    tc filter add dev vxlan0 parent ffff: protocol ipv4 flower \
        enc_src_ip 10.1.0.2 enc_dst_ip 10.1.0.1 \
        enc_key_id 1000 enc_dst_port 4789 \
        action tunnel_key unset \
        action mirred egress redirect dev $REPRESENTOR

When notified of a rule like this, register an encap match on the IP
 and dport tuple (creating an Outer Rule table entry) and insert an MAE
 action rule to perform the decapsulation and deliver to the representee.

Move efx_tc_delete_rule() below efx_tc_flower_release_encap_match() to
 avoid the need for a forward declaration.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/mae.c |  28 ++-
 drivers/net/ethernet/sfc/mae.h |   3 +
 drivers/net/ethernet/sfc/tc.c  | 359 +++++++++++++++++++++++++++++++--
 drivers/net/ethernet/sfc/tc.h  |   1 +
 4 files changed, 374 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 754391eb575f..e8139076fcb0 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -241,6 +241,7 @@ static int efx_mae_get_basic_caps(struct efx_nic *efx, struct mae_caps *caps)
 	if (outlen < sizeof(outbuf))
 		return -EIO;
 	caps->match_field_count = MCDI_DWORD(outbuf, MAE_GET_CAPS_OUT_MATCH_FIELD_COUNT);
+	caps->encap_types = MCDI_DWORD(outbuf, MAE_GET_CAPS_OUT_ENCAP_TYPES_SUPPORTED);
 	caps->action_prios = MCDI_DWORD(outbuf, MAE_GET_CAPS_OUT_ACTION_PRIOS);
 	return 0;
 }
@@ -513,6 +514,28 @@ int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
 }
 #undef CHECK
 
+int efx_mae_check_encap_type_supported(struct efx_nic *efx, enum efx_encap_type typ)
+{
+	unsigned int bit;
+
+	switch (typ & EFX_ENCAP_TYPES_MASK) {
+	case EFX_ENCAP_TYPE_VXLAN:
+		bit = MC_CMD_MAE_GET_CAPS_OUT_ENCAP_TYPE_VXLAN_LBN;
+		break;
+	case EFX_ENCAP_TYPE_NVGRE:
+		bit = MC_CMD_MAE_GET_CAPS_OUT_ENCAP_TYPE_NVGRE_LBN;
+		break;
+	case EFX_ENCAP_TYPE_GENEVE:
+		bit = MC_CMD_MAE_GET_CAPS_OUT_ENCAP_TYPE_GENEVE_LBN;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	if (efx->tc->caps->encap_types & BIT(bit))
+		return 0;
+	return -EOPNOTSUPP;
+}
+
 int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
 {
 	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_COUNTER_ALLOC_OUT_LEN(1));
@@ -772,9 +795,10 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
 	size_t outlen;
 	int rc;
 
-	MCDI_POPULATE_DWORD_2(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
+	MCDI_POPULATE_DWORD_3(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
 			      MAE_ACTION_SET_ALLOC_IN_VLAN_PUSH, act->vlan_push,
-			      MAE_ACTION_SET_ALLOC_IN_VLAN_POP, act->vlan_pop);
+			      MAE_ACTION_SET_ALLOC_IN_VLAN_POP, act->vlan_pop,
+			      MAE_ACTION_SET_ALLOC_IN_DECAP, act->decap);
 
 	MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_SRC_MAC_ID,
 		       MC_CMD_MAE_MAC_ADDR_ALLOC_OUT_MAC_ID_NULL);
diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
index 5b45138aaaf4..6cc96f8adfea 100644
--- a/drivers/net/ethernet/sfc/mae.h
+++ b/drivers/net/ethernet/sfc/mae.h
@@ -70,6 +70,7 @@ void efx_mae_counters_grant_credits(struct work_struct *work);
 
 struct mae_caps {
 	u32 match_field_count;
+	u32 encap_types;
 	u32 action_prios;
 	u8 action_rule_fields[MAE_NUM_FIELDS];
 	u8 outer_rule_fields[MAE_NUM_FIELDS];
@@ -82,6 +83,8 @@ int efx_mae_match_check_caps(struct efx_nic *efx,
 			     struct netlink_ext_ack *extack);
 int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
 				   struct netlink_ext_ack *extack);
+int efx_mae_check_encap_type_supported(struct efx_nic *efx,
+				       enum efx_encap_type typ);
 
 int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
 int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index dc092403af12..8ccf25260312 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -10,12 +10,24 @@
  */
 
 #include <net/pkt_cls.h>
+#include <net/vxlan.h>
+#include <net/geneve.h>
 #include "tc.h"
 #include "tc_bindings.h"
 #include "mae.h"
 #include "ef100_rep.h"
 #include "efx.h"
 
+enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
+{
+	if (netif_is_vxlan(net_dev))
+		return EFX_ENCAP_TYPE_VXLAN;
+	if (netif_is_geneve(net_dev))
+		return EFX_ENCAP_TYPE_GENEVE;
+
+	return EFX_ENCAP_TYPE_NONE;
+}
+
 #define EFX_EFV_PF	NULL
 /* Look up the representor information (efv) for a device.
  * May return NULL for the PF (us), or an error pointer for a device that
@@ -43,6 +55,20 @@ static struct efx_rep *efx_tc_flower_lookup_efv(struct efx_nic *efx,
 	return efv;
 }
 
+/* Convert a driver-internal vport ID into an internal device (PF or VF) */
+static s64 efx_tc_flower_internal_mport(struct efx_nic *efx, struct efx_rep *efv)
+{
+	u32 mport;
+
+	if (IS_ERR(efv))
+		return PTR_ERR(efv);
+	if (!efv) /* device is PF (us) */
+		efx_mae_mport_uplink(efx, &mport);
+	else /* device is repr */
+		efx_mae_mport_mport(efx, efv->mport, &mport);
+	return mport;
+}
+
 /* Convert a driver-internal vport ID into an external device (wire or VF) */
 static s64 efx_tc_flower_external_mport(struct efx_nic *efx, struct efx_rep *efv)
 {
@@ -106,15 +132,6 @@ static void efx_tc_free_action_set_list(struct efx_nic *efx,
 	/* Don't kfree, as acts is embedded inside a struct efx_tc_flow_rule */
 }
 
-static void efx_tc_delete_rule(struct efx_nic *efx, struct efx_tc_flow_rule *rule)
-{
-	efx_mae_delete_rule(efx, rule->fw_id);
-
-	/* Release entries in subsidiary tables */
-	efx_tc_free_action_set_list(efx, &rule->acts, true);
-	rule->fw_id = MC_CMD_MAE_ACTION_RULE_INSERT_OUT_ACTION_RULE_ID_NULL;
-}
-
 static void efx_tc_flow_free(void *ptr, void *arg)
 {
 	struct efx_tc_flow_rule *rule = ptr;
@@ -350,7 +367,6 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 	return 0;
 }
 
-__always_unused
 static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
 					    struct efx_tc_match *match,
 					    enum efx_encap_type type,
@@ -479,7 +495,6 @@ static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
 	return rc;
 }
 
-__always_unused
 static void efx_tc_flower_release_encap_match(struct efx_nic *efx,
 					      struct efx_tc_encap_match *encap)
 {
@@ -501,8 +516,38 @@ static void efx_tc_flower_release_encap_match(struct efx_nic *efx,
 	kfree(encap);
 }
 
+static void efx_tc_delete_rule(struct efx_nic *efx, struct efx_tc_flow_rule *rule)
+{
+	efx_mae_delete_rule(efx, rule->fw_id);
+
+	/* Release entries in subsidiary tables */
+	efx_tc_free_action_set_list(efx, &rule->acts, true);
+	if (rule->match.encap)
+		efx_tc_flower_release_encap_match(efx, rule->match.encap);
+	rule->fw_id = MC_CMD_MAE_ACTION_RULE_INSERT_OUT_ACTION_RULE_ID_NULL;
+}
+
+static const char *efx_tc_encap_type_name(enum efx_encap_type typ, char *buf,
+					  size_t n)
+{
+	switch (typ) {
+	case EFX_ENCAP_TYPE_NONE:
+		return "none";
+	case EFX_ENCAP_TYPE_VXLAN:
+		return "vxlan";
+	case EFX_ENCAP_TYPE_NVGRE:
+		return "nvgre";
+	case EFX_ENCAP_TYPE_GENEVE:
+		return "geneve";
+	default:
+		snprintf(buf, n, "type %u\n", typ);
+		return buf;
+	}
+}
+
 /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
 enum efx_tc_action_order {
+	EFX_TC_AO_DECAP,
 	EFX_TC_AO_VLAN_POP,
 	EFX_TC_AO_VLAN_PUSH,
 	EFX_TC_AO_COUNT,
@@ -513,6 +558,10 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
 					  enum efx_tc_action_order new)
 {
 	switch (new) {
+	case EFX_TC_AO_DECAP:
+		if (act->decap)
+			return false;
+		fallthrough;
 	case EFX_TC_AO_VLAN_POP:
 		if (act->vlan_pop >= 2)
 			return false;
@@ -540,6 +589,288 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
 	}
 }
 
+static int efx_tc_flower_replace_foreign(struct efx_nic *efx,
+					 struct net_device *net_dev,
+					 struct flow_cls_offload *tc)
+{
+	struct flow_rule *fr = flow_cls_offload_flow_rule(tc);
+	struct netlink_ext_ack *extack = tc->common.extack;
+	struct efx_tc_flow_rule *rule = NULL, *old = NULL;
+	struct efx_tc_action_set *act = NULL;
+	bool found = false, uplinked = false;
+	const struct flow_action_entry *fa;
+	struct efx_tc_match match;
+	struct efx_rep *to_efv;
+	s64 rc;
+	int i;
+
+	/* Parse match */
+	memset(&match, 0, sizeof(match));
+	rc = efx_tc_flower_parse_match(efx, fr, &match, NULL);
+	if (rc)
+		return rc;
+	/* The rule as given to us doesn't specify a source netdevice.
+	 * But, determining whether packets from a VF should match it is
+	 * complicated, so leave those to the software slowpath: qualify
+	 * the filter with source m-port == wire.
+	 */
+	rc = efx_tc_flower_external_mport(efx, EFX_EFV_PF);
+	if (rc < 0) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to identify ingress m-port for foreign filter");
+		return rc;
+	}
+	match.value.ingress_port = rc;
+	match.mask.ingress_port = ~0;
+
+	if (tc->common.chain_index) {
+		NL_SET_ERR_MSG_MOD(extack, "No support for nonzero chain_index");
+		return -EOPNOTSUPP;
+	}
+	match.mask.recirc_id = 0xff;
+
+	flow_action_for_each(i, fa, &fr->action) {
+		switch (fa->id) {
+		case FLOW_ACTION_REDIRECT:
+		case FLOW_ACTION_MIRRED: /* mirred means mirror here */
+			to_efv = efx_tc_flower_lookup_efv(efx, fa->dev);
+			if (IS_ERR(to_efv))
+				continue;
+			found = true;
+			break;
+		default:
+			break;
+		}
+	}
+	if (!found) { /* We don't care. */
+		netif_dbg(efx, drv, efx->net_dev,
+			  "Ignoring foreign filter that doesn't egdev us\n");
+		rc = -EOPNOTSUPP;
+		goto release;
+	}
+
+	rc = efx_mae_match_check_caps(efx, &match.mask, NULL);
+	if (rc)
+		goto release;
+
+	if (efx_tc_match_is_encap(&match.mask)) {
+		enum efx_encap_type type;
+
+		type = efx_tc_indr_netdev_type(net_dev);
+		if (type == EFX_ENCAP_TYPE_NONE) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Egress encap match on unsupported tunnel device");
+			rc = -EOPNOTSUPP;
+			goto release;
+		}
+
+		rc = efx_mae_check_encap_type_supported(efx, type);
+		if (rc) {
+			char errbuf[16];
+
+			NL_SET_ERR_MSG_FMT_MOD(extack,
+					       "Firmware reports no support for %s encap match",
+					       efx_tc_encap_type_name(type, errbuf,
+								      sizeof(errbuf)));
+			goto release;
+		}
+
+		rc = efx_tc_flower_record_encap_match(efx, &match, type,
+						      extack);
+		if (rc)
+			goto release;
+	} else {
+		/* This is not a tunnel decap rule, ignore it */
+		netif_dbg(efx, drv, efx->net_dev,
+			  "Ignoring foreign filter without encap match\n");
+		rc = -EOPNOTSUPP;
+		goto release;
+	}
+
+	rule = kzalloc(sizeof(*rule), GFP_USER);
+	if (!rule) {
+		rc = -ENOMEM;
+		goto release;
+	}
+	INIT_LIST_HEAD(&rule->acts.list);
+	rule->cookie = tc->cookie;
+	old = rhashtable_lookup_get_insert_fast(&efx->tc->match_action_ht,
+						&rule->linkage,
+						efx_tc_match_action_ht_params);
+	if (old) {
+		netif_dbg(efx, drv, efx->net_dev,
+			  "Ignoring already-offloaded rule (cookie %lx)\n",
+			  tc->cookie);
+		rc = -EEXIST;
+		goto release;
+	}
+
+	/* Parse actions */
+	act = kzalloc(sizeof(*act), GFP_USER);
+	if (!act) {
+		rc = -ENOMEM;
+		goto release;
+	}
+
+	/* Parse actions.  For foreign rules we only support decap & redirect */
+	flow_action_for_each(i, fa, &fr->action) {
+		struct efx_tc_action_set save;
+
+		switch (fa->id) {
+		case FLOW_ACTION_REDIRECT:
+		case FLOW_ACTION_MIRRED:
+			/* See corresponding code in efx_tc_flower_replace() for
+			 * long explanations of what's going on here.
+			 */
+			save = *act;
+			if (fa->hw_stats) {
+				struct efx_tc_counter_index *ctr;
+
+				if (!(fa->hw_stats & FLOW_ACTION_HW_STATS_DELAYED)) {
+					NL_SET_ERR_MSG_FMT_MOD(extack,
+							       "hw_stats_type %u not supported (only 'delayed')",
+							       fa->hw_stats);
+					rc = -EOPNOTSUPP;
+					goto release;
+				}
+				if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_COUNT)) {
+					rc = -EOPNOTSUPP;
+					goto release;
+				}
+
+				ctr = efx_tc_flower_get_counter_index(efx,
+								      tc->cookie,
+								      EFX_TC_COUNTER_TYPE_AR);
+				if (IS_ERR(ctr)) {
+					rc = PTR_ERR(ctr);
+					NL_SET_ERR_MSG_MOD(extack, "Failed to obtain a counter");
+					goto release;
+				}
+				act->count = ctr;
+			}
+
+			if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_DELIVER)) {
+				/* can't happen */
+				rc = -EOPNOTSUPP;
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Deliver action violates action order (can't happen)");
+				goto release;
+			}
+			to_efv = efx_tc_flower_lookup_efv(efx, fa->dev);
+			/* PF implies egdev is us, in which case we really
+			 * want to deliver to the uplink (because this is an
+			 * ingress filter).  If we don't recognise the egdev
+			 * at all, then we'd better trap so SW can handle it.
+			 */
+			if (IS_ERR(to_efv))
+				to_efv = EFX_EFV_PF;
+			if (to_efv == EFX_EFV_PF) {
+				if (uplinked)
+					break;
+				uplinked = true;
+			}
+			rc = efx_tc_flower_internal_mport(efx, to_efv);
+			if (rc < 0) {
+				NL_SET_ERR_MSG_MOD(extack, "Failed to identify egress m-port");
+				goto release;
+			}
+			act->dest_mport = rc;
+			act->deliver = 1;
+			rc = efx_mae_alloc_action_set(efx, act);
+			if (rc) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Failed to write action set to hw (mirred)");
+				goto release;
+			}
+			list_add_tail(&act->list, &rule->acts.list);
+			act = NULL;
+			if (fa->id == FLOW_ACTION_REDIRECT)
+				break; /* end of the line */
+			/* Mirror, so continue on with saved act */
+			save.count = NULL;
+			act = kzalloc(sizeof(*act), GFP_USER);
+			if (!act) {
+				rc = -ENOMEM;
+				goto release;
+			}
+			*act = save;
+			break;
+		case FLOW_ACTION_TUNNEL_DECAP:
+			if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_DECAP)) {
+				rc = -EINVAL;
+				NL_SET_ERR_MSG_MOD(extack, "Decap action violates action order");
+				goto release;
+			}
+			act->decap = 1;
+			/* If we previously delivered/trapped to uplink, now
+			 * that we've decapped we'll want another copy if we
+			 * try to deliver/trap to uplink again.
+			 */
+			uplinked = false;
+			break;
+		default:
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
+					       fa->id);
+			rc = -EOPNOTSUPP;
+			goto release;
+		}
+	}
+
+	if (act) {
+		if (!uplinked) {
+			/* Not shot/redirected, so deliver to default dest (which is
+			 * the uplink, as this is an ingress filter)
+			 */
+			efx_mae_mport_uplink(efx, &act->dest_mport);
+			act->deliver = 1;
+		}
+		rc = efx_mae_alloc_action_set(efx, act);
+		if (rc) {
+			NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (deliver)");
+			goto release;
+		}
+		list_add_tail(&act->list, &rule->acts.list);
+		act = NULL; /* Prevent double-free in error path */
+	}
+
+	rule->match = match;
+
+	netif_dbg(efx, drv, efx->net_dev,
+		  "Successfully parsed foreign filter (cookie %lx)\n",
+		  tc->cookie);
+
+	rc = efx_mae_alloc_action_set_list(efx, &rule->acts);
+	if (rc) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to write action set list to hw");
+		goto release;
+	}
+	rc = efx_mae_insert_rule(efx, &rule->match, EFX_TC_PRIO_TC,
+				 rule->acts.fw_id, &rule->fw_id);
+	if (rc) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to insert rule in hw");
+		goto release_act;
+	}
+	return 0;
+
+release_act:
+	efx_mae_free_action_set_list(efx, &rule->acts);
+release:
+	/* We failed to insert the rule, so free up any entries we created in
+	 * subsidiary tables.
+	 */
+	if (act)
+		efx_tc_free_action_set(efx, act, false);
+	if (rule) {
+		rhashtable_remove_fast(&efx->tc->match_action_ht,
+				       &rule->linkage,
+				       efx_tc_match_action_ht_params);
+		efx_tc_free_action_set_list(efx, &rule->acts, false);
+	}
+	kfree(rule);
+	if (match.encap)
+		efx_tc_flower_release_encap_match(efx, match.encap);
+	return rc;
+}
+
 static int efx_tc_flower_replace(struct efx_nic *efx,
 				 struct net_device *net_dev,
 				 struct flow_cls_offload *tc,
@@ -564,10 +895,8 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 	from_efv = efx_tc_flower_lookup_efv(efx, net_dev);
 	if (IS_ERR(from_efv)) {
-		/* Might be a tunnel decap rule from an indirect block.
-		 * Support for those not implemented yet.
-		 */
-		return -EOPNOTSUPP;
+		/* Not from our PF or representors, so probably a tunnel dev */
+		return efx_tc_flower_replace_foreign(efx, net_dev, tc);
 	}
 
 	if (efv != from_efv) {
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index d70c0ba86669..47b6e9e35808 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -28,6 +28,7 @@ static inline bool efx_ipv6_addr_all_ones(struct in6_addr *addr)
 struct efx_tc_action_set {
 	u16 vlan_push:2;
 	u16 vlan_pop:2;
+	u16 decap:1;
 	u16 deliver:1;
 	__be16 vlan_tci[2]; /* TCIs for vlan_push */
 	__be16 vlan_proto[2]; /* Ethertypes for vlan_push */

Re: [PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap) rules

[kernel test robot]

Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20230314]
[cannot apply to net-next/master horms-ipvs/master linus/master v6.3-rc2 v6.3-rc1 v6.2 v6.3-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/edward-cree-amd-com/sfc-add-notion-of-match-on-enc-keys-to-MAE-machinery/20230315-013855
patch link:    https://lore.kernel.org/r/a7aabdb45290f1cd50681eb9e1d610893fbce299.1678815095.git.ecree.xilinx%40gmail.com
patch subject: [PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap) rules
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230315/202303150436.cQ46tTwI-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/51a99241aafeb3bd67a12aae5e9089c7aff2f3cd
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review edward-cree-amd-com/sfc-add-notion-of-match-on-enc-keys-to-MAE-machinery/20230315-013855
        git checkout 51a99241aafeb3bd67a12aae5e9089c7aff2f3cd
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303150436.cQ46tTwI-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/sfc/tc.c:21:21: warning: no previous prototype for 'efx_tc_indr_netdev_type' [-Wmissing-prototypes]
      21 | enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
         |                     ^~~~~~~~~~~~~~~~~~~~~~~


vim +/efx_tc_indr_netdev_type +21 drivers/net/ethernet/sfc/tc.c

    20	
  > 21	enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
    22	{
    23		if (netif_is_vxlan(net_dev))
    24			return EFX_ENCAP_TYPE_VXLAN;
    25		if (netif_is_geneve(net_dev))
    26			return EFX_ENCAP_TYPE_GENEVE;
    27	
    28		return EFX_ENCAP_TYPE_NONE;
    29	}
    30	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap) rules

[Michal Swiatkowski]

On Tue, Mar 14, 2023 at 05:35:25PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> A 'foreign' rule is one for which the net_dev is not the sfc netdevice
>  or any of its representors.  The driver registers indirect flow blocks
>  for tunnel netdevs so that it can offload decap rules.  For example:
> 
>     tc filter add dev vxlan0 parent ffff: protocol ipv4 flower \
>         enc_src_ip 10.1.0.2 enc_dst_ip 10.1.0.1 \
>         enc_key_id 1000 enc_dst_port 4789 \
>         action tunnel_key unset \
>         action mirred egress redirect dev $REPRESENTOR
> 
> When notified of a rule like this, register an encap match on the IP
>  and dport tuple (creating an Outer Rule table entry) and insert an MAE
>  action rule to perform the decapsulation and deliver to the representee.
> 
> Move efx_tc_delete_rule() below efx_tc_flower_release_encap_match() to
>  avoid the need for a forward declaration.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/mae.c |  28 ++-
>  drivers/net/ethernet/sfc/mae.h |   3 +
>  drivers/net/ethernet/sfc/tc.c  | 359 +++++++++++++++++++++++++++++++--
>  drivers/net/ethernet/sfc/tc.h  |   1 +
>  4 files changed, 374 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
> index 754391eb575f..e8139076fcb0 100644
> --- a/drivers/net/ethernet/sfc/mae.c
> +++ b/drivers/net/ethernet/sfc/mae.c
> @@ -241,6 +241,7 @@ static int efx_mae_get_basic_caps(struct efx_nic *efx, struct mae_caps *caps)
>  	if (outlen < sizeof(outbuf))
>  		return -EIO;
>  	caps->match_field_count = MCDI_DWORD(outbuf, MAE_GET_CAPS_OUT_MATCH_FIELD_COUNT);
> +	caps->encap_types = MCDI_DWORD(outbuf, MAE_GET_CAPS_OUT_ENCAP_TYPES_SUPPORTED);
>  	caps->action_prios = MCDI_DWORD(outbuf, MAE_GET_CAPS_OUT_ACTION_PRIOS);
>  	return 0;
>  }
> @@ -513,6 +514,28 @@ int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
>  }
>  #undef CHECK
>  
> +int efx_mae_check_encap_type_supported(struct efx_nic *efx, enum efx_encap_type typ)
> +{
> +	unsigned int bit;
> +
> +	switch (typ & EFX_ENCAP_TYPES_MASK) {
In 3/5 You ommited EFX_ENCAP_TYPE_NVGRE, was it intetional?

> +	case EFX_ENCAP_TYPE_VXLAN:
> +		bit = MC_CMD_MAE_GET_CAPS_OUT_ENCAP_TYPE_VXLAN_LBN;
> +		break;
> +	case EFX_ENCAP_TYPE_NVGRE:
> +		bit = MC_CMD_MAE_GET_CAPS_OUT_ENCAP_TYPE_NVGRE_LBN;
> +		break;
> +	case EFX_ENCAP_TYPE_GENEVE:
> +		bit = MC_CMD_MAE_GET_CAPS_OUT_ENCAP_TYPE_GENEVE_LBN;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	if (efx->tc->caps->encap_types & BIT(bit))
> +		return 0;
> +	return -EOPNOTSUPP;
> +}
> +
>  int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
>  {
>  	MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_COUNTER_ALLOC_OUT_LEN(1));
> @@ -772,9 +795,10 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
>  	size_t outlen;
>  	int rc;
>  
> -	MCDI_POPULATE_DWORD_2(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
> +	MCDI_POPULATE_DWORD_3(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
>  			      MAE_ACTION_SET_ALLOC_IN_VLAN_PUSH, act->vlan_push,
> -			      MAE_ACTION_SET_ALLOC_IN_VLAN_POP, act->vlan_pop);
> +			      MAE_ACTION_SET_ALLOC_IN_VLAN_POP, act->vlan_pop,
> +			      MAE_ACTION_SET_ALLOC_IN_DECAP, act->decap);
>  
>  	MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_SRC_MAC_ID,
>  		       MC_CMD_MAE_MAC_ADDR_ALLOC_OUT_MAC_ID_NULL);
> diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
> index 5b45138aaaf4..6cc96f8adfea 100644
> --- a/drivers/net/ethernet/sfc/mae.h
> +++ b/drivers/net/ethernet/sfc/mae.h
> @@ -70,6 +70,7 @@ void efx_mae_counters_grant_credits(struct work_struct *work);
>  
>  struct mae_caps {
>  	u32 match_field_count;
> +	u32 encap_types;
>  	u32 action_prios;
>  	u8 action_rule_fields[MAE_NUM_FIELDS];
>  	u8 outer_rule_fields[MAE_NUM_FIELDS];
> @@ -82,6 +83,8 @@ int efx_mae_match_check_caps(struct efx_nic *efx,
>  			     struct netlink_ext_ack *extack);
>  int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
>  				   struct netlink_ext_ack *extack);
> +int efx_mae_check_encap_type_supported(struct efx_nic *efx,
> +				       enum efx_encap_type typ);
>  
>  int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
>  int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
> index dc092403af12..8ccf25260312 100644
> --- a/drivers/net/ethernet/sfc/tc.c
> +++ b/drivers/net/ethernet/sfc/tc.c
> @@ -10,12 +10,24 @@
>   */
>  
>  #include <net/pkt_cls.h>
> +#include <net/vxlan.h>
> +#include <net/geneve.h>
>  #include "tc.h"
>  #include "tc_bindings.h"
>  #include "mae.h"
>  #include "ef100_rep.h"
>  #include "efx.h"
>  
> +enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
> +{
> +	if (netif_is_vxlan(net_dev))
> +		return EFX_ENCAP_TYPE_VXLAN;
> +	if (netif_is_geneve(net_dev))
> +		return EFX_ENCAP_TYPE_GENEVE;
netif_is_gretap or NVGRE isn't supported?

> +
> +	return EFX_ENCAP_TYPE_NONE;
> +}
> +
>  #define EFX_EFV_PF	NULL
>  /* Look up the representor information (efv) for a device.
>   * May return NULL for the PF (us), or an error pointer for a device that
> @@ -43,6 +55,20 @@ static struct efx_rep *efx_tc_flower_lookup_efv(struct efx_nic *efx,
>  	return efv;
>  }
>  
> +/* Convert a driver-internal vport ID into an internal device (PF or VF) */
> +static s64 efx_tc_flower_internal_mport(struct efx_nic *efx, struct efx_rep *efv)
> +{
> +	u32 mport;
> +
> +	if (IS_ERR(efv))
> +		return PTR_ERR(efv);
> +	if (!efv) /* device is PF (us) */
> +		efx_mae_mport_uplink(efx, &mport);
> +	else /* device is repr */
> +		efx_mae_mport_mport(efx, efv->mport, &mport);
> +	return mport;
> +}
> +
>  /* Convert a driver-internal vport ID into an external device (wire or VF) */
>  static s64 efx_tc_flower_external_mport(struct efx_nic *efx, struct efx_rep *efv)
>  {
> @@ -106,15 +132,6 @@ static void efx_tc_free_action_set_list(struct efx_nic *efx,
>  	/* Don't kfree, as acts is embedded inside a struct efx_tc_flow_rule */
>  }
>  
> -static void efx_tc_delete_rule(struct efx_nic *efx, struct efx_tc_flow_rule *rule)
> -{
> -	efx_mae_delete_rule(efx, rule->fw_id);
> -
> -	/* Release entries in subsidiary tables */
> -	efx_tc_free_action_set_list(efx, &rule->acts, true);
> -	rule->fw_id = MC_CMD_MAE_ACTION_RULE_INSERT_OUT_ACTION_RULE_ID_NULL;
> -}
> -
>  static void efx_tc_flow_free(void *ptr, void *arg)
>  {
>  	struct efx_tc_flow_rule *rule = ptr;
> @@ -350,7 +367,6 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
>  	return 0;
>  }
>  
> -__always_unused
>  static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
>  					    struct efx_tc_match *match,
>  					    enum efx_encap_type type,
> @@ -479,7 +495,6 @@ static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
>  	return rc;
>  }
>  
> -__always_unused
>  static void efx_tc_flower_release_encap_match(struct efx_nic *efx,
>  					      struct efx_tc_encap_match *encap)
>  {
> @@ -501,8 +516,38 @@ static void efx_tc_flower_release_encap_match(struct efx_nic *efx,
>  	kfree(encap);
>  }
>  
> +static void efx_tc_delete_rule(struct efx_nic *efx, struct efx_tc_flow_rule *rule)
> +{
> +	efx_mae_delete_rule(efx, rule->fw_id);
> +
> +	/* Release entries in subsidiary tables */
> +	efx_tc_free_action_set_list(efx, &rule->acts, true);
> +	if (rule->match.encap)
> +		efx_tc_flower_release_encap_match(efx, rule->match.encap);
> +	rule->fw_id = MC_CMD_MAE_ACTION_RULE_INSERT_OUT_ACTION_RULE_ID_NULL;
> +}
> +
> +static const char *efx_tc_encap_type_name(enum efx_encap_type typ, char *buf,
> +					  size_t n)
> +{
> +	switch (typ) {
> +	case EFX_ENCAP_TYPE_NONE:
> +		return "none";
> +	case EFX_ENCAP_TYPE_VXLAN:
> +		return "vxlan";
> +	case EFX_ENCAP_TYPE_NVGRE:
> +		return "nvgre";
> +	case EFX_ENCAP_TYPE_GENEVE:
> +		return "geneve";
> +	default:
> +		snprintf(buf, n, "type %u\n", typ);
> +		return buf;
I will return unsupported here, instead of playing with buffer.

> +	}
> +}
> +
>  /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
Is it a device documentation? Where it can be find?

>  enum efx_tc_action_order {
> +	EFX_TC_AO_DECAP,
>  	EFX_TC_AO_VLAN_POP,
>  	EFX_TC_AO_VLAN_PUSH,
>  	EFX_TC_AO_COUNT,
> @@ -513,6 +558,10 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
>  					  enum efx_tc_action_order new)
>  {
>  	switch (new) {
> +	case EFX_TC_AO_DECAP:
> +		if (act->decap)
> +			return false;
> +		fallthrough;
>  	case EFX_TC_AO_VLAN_POP:
>  		if (act->vlan_pop >= 2)
>  			return false;
> @@ -540,6 +589,288 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
>  	}
>  }
>  
> +static int efx_tc_flower_replace_foreign(struct efx_nic *efx,
> +					 struct net_device *net_dev,
> +					 struct flow_cls_offload *tc)
> +{
> +	struct flow_rule *fr = flow_cls_offload_flow_rule(tc);
> +	struct netlink_ext_ack *extack = tc->common.extack;
> +	struct efx_tc_flow_rule *rule = NULL, *old = NULL;
> +	struct efx_tc_action_set *act = NULL;
> +	bool found = false, uplinked = false;
> +	const struct flow_action_entry *fa;
> +	struct efx_tc_match match;
> +	struct efx_rep *to_efv;
> +	s64 rc;
> +	int i;
> +
> +	/* Parse match */
> +	memset(&match, 0, sizeof(match));
> +	rc = efx_tc_flower_parse_match(efx, fr, &match, NULL);
> +	if (rc)
> +		return rc;
> +	/* The rule as given to us doesn't specify a source netdevice.
> +	 * But, determining whether packets from a VF should match it is
> +	 * complicated, so leave those to the software slowpath: qualify
> +	 * the filter with source m-port == wire.
> +	 */
> +	rc = efx_tc_flower_external_mport(efx, EFX_EFV_PF);
Let's define extern_port as s64, a rc as int, it will be more readable I
think.

> +	if (rc < 0) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to identify ingress m-port for foreign filter");
> +		return rc;
> +	}
> +	match.value.ingress_port = rc;
> +	match.mask.ingress_port = ~0;
> +
> +	if (tc->common.chain_index) {
> +		NL_SET_ERR_MSG_MOD(extack, "No support for nonzero chain_index");
> +		return -EOPNOTSUPP;
> +	}
> +	match.mask.recirc_id = 0xff;
> +
> +	flow_action_for_each(i, fa, &fr->action) {
> +		switch (fa->id) {
> +		case FLOW_ACTION_REDIRECT:
> +		case FLOW_ACTION_MIRRED: /* mirred means mirror here */
> +			to_efv = efx_tc_flower_lookup_efv(efx, fa->dev);
> +			if (IS_ERR(to_efv))
> +				continue;
> +			found = true;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +	if (!found) { /* We don't care. */
> +		netif_dbg(efx, drv, efx->net_dev,
> +			  "Ignoring foreign filter that doesn't egdev us\n");
> +		rc = -EOPNOTSUPP;
> +		goto release;
> +	}
> +
> +	rc = efx_mae_match_check_caps(efx, &match.mask, NULL);
> +	if (rc)
> +		goto release;
> +
> +	if (efx_tc_match_is_encap(&match.mask)) {
> +		enum efx_encap_type type;
> +
> +		type = efx_tc_indr_netdev_type(net_dev);
> +		if (type == EFX_ENCAP_TYPE_NONE) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Egress encap match on unsupported tunnel device");
> +			rc = -EOPNOTSUPP;
> +			goto release;
> +		}
> +
> +		rc = efx_mae_check_encap_type_supported(efx, type);
> +		if (rc) {
> +			char errbuf[16];
> +
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "Firmware reports no support for %s encap match",
> +					       efx_tc_encap_type_name(type, errbuf,
> +								      sizeof(errbuf)));
> +			goto release;
> +		}
> +
> +		rc = efx_tc_flower_record_encap_match(efx, &match, type,
> +						      extack);
> +		if (rc)
> +			goto release;
> +	} else {
> +		/* This is not a tunnel decap rule, ignore it */
> +		netif_dbg(efx, drv, efx->net_dev,
> +			  "Ignoring foreign filter without encap match\n");
> +		rc = -EOPNOTSUPP;
> +		goto release;
> +	}
> +
> +	rule = kzalloc(sizeof(*rule), GFP_USER);
> +	if (!rule) {
> +		rc = -ENOMEM;
> +		goto release;
> +	}
> +	INIT_LIST_HEAD(&rule->acts.list);
> +	rule->cookie = tc->cookie;
> +	old = rhashtable_lookup_get_insert_fast(&efx->tc->match_action_ht,
> +						&rule->linkage,
> +						efx_tc_match_action_ht_params);
> +	if (old) {
> +		netif_dbg(efx, drv, efx->net_dev,
> +			  "Ignoring already-offloaded rule (cookie %lx)\n",
> +			  tc->cookie);
> +		rc = -EEXIST;
> +		goto release;
> +	}
> +
> +	/* Parse actions */
> +	act = kzalloc(sizeof(*act), GFP_USER);
> +	if (!act) {
> +		rc = -ENOMEM;
> +		goto release;
> +	}
> +
> +	/* Parse actions.  For foreign rules we only support decap & redirect */
> +	flow_action_for_each(i, fa, &fr->action) {
> +		struct efx_tc_action_set save;
> +
> +		switch (fa->id) {
> +		case FLOW_ACTION_REDIRECT:
> +		case FLOW_ACTION_MIRRED:
> +			/* See corresponding code in efx_tc_flower_replace() for
> +			 * long explanations of what's going on here.
> +			 */
> +			save = *act;
Why save is needed here? In one bloick You are changing act, in other
save.

> +			if (fa->hw_stats) {
> +				struct efx_tc_counter_index *ctr;
> +
> +				if (!(fa->hw_stats & FLOW_ACTION_HW_STATS_DELAYED)) {
> +					NL_SET_ERR_MSG_FMT_MOD(extack,
> +							       "hw_stats_type %u not supported (only 'delayed')",
> +							       fa->hw_stats);
> +					rc = -EOPNOTSUPP;
> +					goto release;
> +				}
> +				if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_COUNT)) {
> +					rc = -EOPNOTSUPP;
> +					goto release;
> +				}
> +
> +				ctr = efx_tc_flower_get_counter_index(efx,
> +								      tc->cookie,
> +								      EFX_TC_COUNTER_TYPE_AR);
> +				if (IS_ERR(ctr)) {
> +					rc = PTR_ERR(ctr);
> +					NL_SET_ERR_MSG_MOD(extack, "Failed to obtain a counter");
> +					goto release;
> +				}
> +				act->count = ctr;
> +			}
> +
> +			if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_DELIVER)) {
> +				/* can't happen */
> +				rc = -EOPNOTSUPP;
> +				NL_SET_ERR_MSG_MOD(extack,
> +						   "Deliver action violates action order (can't happen)");
> +				goto release;
> +			}
> +			to_efv = efx_tc_flower_lookup_efv(efx, fa->dev);
> +			/* PF implies egdev is us, in which case we really
> +			 * want to deliver to the uplink (because this is an
> +			 * ingress filter).  If we don't recognise the egdev
> +			 * at all, then we'd better trap so SW can handle it.
> +			 */
> +			if (IS_ERR(to_efv))
> +				to_efv = EFX_EFV_PF;
> +			if (to_efv == EFX_EFV_PF) {
> +				if (uplinked)
> +					break;
> +				uplinked = true;
> +			}
> +			rc = efx_tc_flower_internal_mport(efx, to_efv);
> +			if (rc < 0) {
> +				NL_SET_ERR_MSG_MOD(extack, "Failed to identify egress m-port");
> +				goto release;
> +			}
> +			act->dest_mport = rc;
> +			act->deliver = 1;
> +			rc = efx_mae_alloc_action_set(efx, act);
> +			if (rc) {
> +				NL_SET_ERR_MSG_MOD(extack,
> +						   "Failed to write action set to hw (mirred)");
> +				goto release;
> +			}
> +			list_add_tail(&act->list, &rule->acts.list);
> +			act = NULL;
act was allocated, You need to free it, or maybe it is being cleared in
alloc_action_set()? However, this function is really hard to follow.
Please explain it more widely.

> +			if (fa->id == FLOW_ACTION_REDIRECT)
> +				break; /* end of the line */
> +			/* Mirror, so continue on with saved act */
> +			save.count = NULL;
> +			act = kzalloc(sizeof(*act), GFP_USER);
> +			if (!act) {
> +				rc = -ENOMEM;
> +				goto release;
> +			}
> +			*act = save;
> +			break;
> +		case FLOW_ACTION_TUNNEL_DECAP:
> +			if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_DECAP)) {
> +				rc = -EINVAL;
> +				NL_SET_ERR_MSG_MOD(extack, "Decap action violates action order");
> +				goto release;
> +			}
> +			act->decap = 1;
> +			/* If we previously delivered/trapped to uplink, now
> +			 * that we've decapped we'll want another copy if we
> +			 * try to deliver/trap to uplink again.
> +			 */
> +			uplinked = false;
> +			break;
> +		default:
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
> +					       fa->id);
> +			rc = -EOPNOTSUPP;
> +			goto release;
> +		}
> +	}
> +
> +	if (act) {
> +		if (!uplinked) {
> +			/* Not shot/redirected, so deliver to default dest (which is
> +			 * the uplink, as this is an ingress filter)
> +			 */
> +			efx_mae_mport_uplink(efx, &act->dest_mport);
> +			act->deliver = 1;
> +		}
> +		rc = efx_mae_alloc_action_set(efx, act);
> +		if (rc) {
> +			NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (deliver)");
> +			goto release;
> +		}
> +		list_add_tail(&act->list, &rule->acts.list);
> +		act = NULL; /* Prevent double-free in error path */
> +	}
> +
> +	rule->match = match;
> +
> +	netif_dbg(efx, drv, efx->net_dev,
> +		  "Successfully parsed foreign filter (cookie %lx)\n",
> +		  tc->cookie);
> +
> +	rc = efx_mae_alloc_action_set_list(efx, &rule->acts);
> +	if (rc) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to write action set list to hw");
> +		goto release;
> +	}
> +	rc = efx_mae_insert_rule(efx, &rule->match, EFX_TC_PRIO_TC,
> +				 rule->acts.fw_id, &rule->fw_id);
> +	if (rc) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to insert rule in hw");
> +		goto release_act;
> +	}
act is saved somewhere?

> +	return 0;
> +
> +release_act:
> +	efx_mae_free_action_set_list(efx, &rule->acts);
> +release:
> +	/* We failed to insert the rule, so free up any entries we created in
> +	 * subsidiary tables.
> +	 */
> +	if (act)
> +		efx_tc_free_action_set(efx, act, false);
> +	if (rule) {
> +		rhashtable_remove_fast(&efx->tc->match_action_ht,
> +				       &rule->linkage,
> +				       efx_tc_match_action_ht_params);
> +		efx_tc_free_action_set_list(efx, &rule->acts, false);
> +	}
> +	kfree(rule);
> +	if (match.encap)
> +		efx_tc_flower_release_encap_match(efx, match.encap);
> +	return rc;
> +}
> +
>  static int efx_tc_flower_replace(struct efx_nic *efx,
>  				 struct net_device *net_dev,
>  				 struct flow_cls_offload *tc,
> @@ -564,10 +895,8 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
>  
>  	from_efv = efx_tc_flower_lookup_efv(efx, net_dev);
>  	if (IS_ERR(from_efv)) {
> -		/* Might be a tunnel decap rule from an indirect block.
> -		 * Support for those not implemented yet.
> -		 */
> -		return -EOPNOTSUPP;
> +		/* Not from our PF or representors, so probably a tunnel dev */
> +		return efx_tc_flower_replace_foreign(efx, net_dev, tc);
>  	}
>  
>  	if (efv != from_efv) {
> diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
> index d70c0ba86669..47b6e9e35808 100644
> --- a/drivers/net/ethernet/sfc/tc.h
> +++ b/drivers/net/ethernet/sfc/tc.h
> @@ -28,6 +28,7 @@ static inline bool efx_ipv6_addr_all_ones(struct in6_addr *addr)
>  struct efx_tc_action_set {
>  	u16 vlan_push:2;
>  	u16 vlan_pop:2;
> +	u16 decap:1;
>  	u16 deliver:1;
>  	__be16 vlan_tci[2]; /* TCIs for vlan_push */
>  	__be16 vlan_proto[2]; /* Ethertypes for vlan_push */

Re: [PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap) rules

[Edward Cree]

On 15/03/2023 10:11, Michal Swiatkowski wrote:
> On Tue, Mar 14, 2023 at 05:35:25PM +0000, edward.cree@amd.com wrote:
>> +int efx_mae_check_encap_type_supported(struct efx_nic *efx, enum efx_encap_type typ)
>> +{
>> +	unsigned int bit;
>> +
>> +	switch (typ & EFX_ENCAP_TYPES_MASK) {
> In 3/5 You ommited EFX_ENCAP_TYPE_NVGRE, was it intetional?

No, I'll go back and add it.

>> +enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
>> +{
>> +	if (netif_is_vxlan(net_dev))
>> +		return EFX_ENCAP_TYPE_VXLAN;
>> +	if (netif_is_geneve(net_dev))
>> +		return EFX_ENCAP_TYPE_GENEVE;
> netif_is_gretap or NVGRE isn't supported?

It should be supported, the hardware can handle it.
I'll add it in v2, and test to make sure it actually works ;)

>> +static const char *efx_tc_encap_type_name(enum efx_encap_type typ, char *buf,
>> +					  size_t n)
>> +{
>> +	switch (typ) {
>> +	case EFX_ENCAP_TYPE_NONE:
>> +		return "none";
>> +	case EFX_ENCAP_TYPE_VXLAN:
>> +		return "vxlan";
>> +	case EFX_ENCAP_TYPE_NVGRE:
>> +		return "nvgre";
>> +	case EFX_ENCAP_TYPE_GENEVE:
>> +		return "geneve";
>> +	default:
>> +		snprintf(buf, n, "type %u\n", typ);
>> +		return buf;
> I will return unsupported here, instead of playing with buffer.

Hmm, maybe if I add a one-time netif_warn()?  I don't like the
 idea of not getting the bogus value out anywhere where it can
 be debugged.

>>  /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
> Is it a device documentation? Where it can be find?

Ahh, turns out this document isn't public :(  I'll see if we
 can get this section of it split out into something public.

> 
>>  enum efx_tc_action_order {
>> +	EFX_TC_AO_DECAP,
>>  	EFX_TC_AO_VLAN_POP,
>>  	EFX_TC_AO_VLAN_PUSH,
>>  	EFX_TC_AO_COUNT,
>> @@ -513,6 +558,10 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
>>  					  enum efx_tc_action_order new)
>>  {
>>  	switch (new) {
>> +	case EFX_TC_AO_DECAP:
>> +		if (act->decap)
>> +			return false;
>> +		fallthrough;
>>  	case EFX_TC_AO_VLAN_POP:
>>  		if (act->vlan_pop >= 2)
>>  			return false;
>> @@ -540,6 +589,288 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
>>  	}
>>  }
>>  
>> +static int efx_tc_flower_replace_foreign(struct efx_nic *efx,
>> +					 struct net_device *net_dev,
>> +					 struct flow_cls_offload *tc)
>> +{
>> +	struct flow_rule *fr = flow_cls_offload_flow_rule(tc);
>> +	struct netlink_ext_ack *extack = tc->common.extack;
>> +	struct efx_tc_flow_rule *rule = NULL, *old = NULL;
>> +	struct efx_tc_action_set *act = NULL;
>> +	bool found = false, uplinked = false;
>> +	const struct flow_action_entry *fa;
>> +	struct efx_tc_match match;
>> +	struct efx_rep *to_efv;
>> +	s64 rc;
>> +	int i;
>> +
>> +	/* Parse match */
>> +	memset(&match, 0, sizeof(match));
>> +	rc = efx_tc_flower_parse_match(efx, fr, &match, NULL);
>> +	if (rc)
>> +		return rc;
>> +	/* The rule as given to us doesn't specify a source netdevice.
>> +	 * But, determining whether packets from a VF should match it is
>> +	 * complicated, so leave those to the software slowpath: qualify
>> +	 * the filter with source m-port == wire.
>> +	 */
>> +	rc = efx_tc_flower_external_mport(efx, EFX_EFV_PF);
> Let's define extern_port as s64, a rc as int, it will be more readable I
> think.

Will it?  I feel it looks more natural this way — this is the way
 it would be written if mports always fitted into an int; the fact
 that they're actually u32 and thus the value needs to be wider to
 hold either an mport or an -errno is locally irrelevant to the
 reader.

>> +	/* Parse actions.  For foreign rules we only support decap & redirect */
>> +	flow_action_for_each(i, fa, &fr->action) {
>> +		struct efx_tc_action_set save;
>> +
>> +		switch (fa->id) {
>> +		case FLOW_ACTION_REDIRECT:
>> +		case FLOW_ACTION_MIRRED:
>> +			/* See corresponding code in efx_tc_flower_replace() for
>> +			 * long explanations of what's going on here.
>> +			 */
>> +			save = *act;
> Why save is needed here? In one bloick You are changing act, in other
> save.

Below, we set:

>> +			act->dest_mport = rc;
>> +			act->deliver = 1;

but then if this action is a mirred egress mirror, we don't want the
 next action-set in the action-set-list to deliver to the same place.
So we save the action state (collection of mutations applied to the
 packet) before the mirred, so that we can resume from where we left
 off:

>> +			/* Mirror, so continue on with saved act */
>> +			save.count = NULL;
>> +			act = kzalloc(sizeof(*act), GFP_USER);
>> +			if (!act) {
>> +				rc = -ENOMEM;
>> +				goto release;
>> +			}
>> +			*act = save;

The setting of save.count is a copy-paste error; that's needed in
 the non-tunnel case (efx_tc_flower_replace()) since that has a
 separate block for attaching counters (handling the case of
 FLOW_ACTION_DROP which we don't accept here in
 efx_tc_flower_replace_foreign()), but it's not needed here; maybe
 that was part of the confusion.

>> +			rc = efx_mae_alloc_action_set(efx, act);
>> +			if (rc) {
>> +				NL_SET_ERR_MSG_MOD(extack,
>> +						   "Failed to write action set to hw (mirred)");
>> +				goto release;
>> +			}
>> +			list_add_tail(&act->list, &rule->acts.list);
>> +			act = NULL;
> act was allocated, You need to free it, or maybe it is being cleared in
> alloc_action_set()? However, this function is really hard to follow.
> Please explain it more widely.

The list_add_tail attaches act (the action-set) to the list in
 rule->acts (the action-set-list), meaning it will be freed when the
 action-set-list is destroyed by efx_tc_free_action_set_list(),
 either in this function's failure ladder or in efx_tc_delete_rule().
I will try and write an extended comment under /* Parse actions */ to
 explain the use of this act 'cursor'.  Or rather, I'll add the
 comment to the efx_tc_flower_replace() equivalent, which works the
 same way, and reference it from here.

>> +	rc = efx_mae_insert_rule(efx, &rule->match, EFX_TC_PRIO_TC,
>> +				 rule->acts.fw_id, &rule->fw_id);
>> +	if (rc) {
>> +		NL_SET_ERR_MSG_MOD(extack, "Failed to insert rule in hw");
>> +		goto release_act;
>> +	}
> act is saved somewhere?

Sorry, this should be called `release_acts`, as its job is to
 remove the action-set-list (rule->acts) from hardware.  Will fix.

Thanks for the very thorough review of this series :)

Re: [PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap) rules

[Edward Cree]

On 15/03/2023 14:43, Edward Cree wrote:
> On 15/03/2023 10:11, Michal Swiatkowski wrote:
>> On Tue, Mar 14, 2023 at 05:35:25PM +0000, edward.cree@amd.com wrote:
>>> +enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
>>> +{
>>> +	if (netif_is_vxlan(net_dev))
>>> +		return EFX_ENCAP_TYPE_VXLAN;
>>> +	if (netif_is_geneve(net_dev))
>>> +		return EFX_ENCAP_TYPE_GENEVE;
>> netif_is_gretap or NVGRE isn't supported?
> 
> It should be supported, the hardware can handle it.
> I'll add it in v2, and test to make sure it actually works ;)

Fun discovery: while the hardware supports NVGRE, it can *only*
 match on the VSID field, not the whole GRE Key.
TC flower, meanwhile, neither knows nor cares about NVGRE; gretap
 decap rules expect to match on the full 32-bit Key field, and you
 can't even mask them (there's no TCA_FLOWER_KEY_ENC_KEY_ID_MASK
 in the uAPI), meaning the driver can't just require the FlowID is
 masked out and shift the rest.

So enabling this support is nontrivial; I've decided to leave it
 out of the series and just remove all mention of NVGRE for now.