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

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

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 {