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

[Edward Cree <ecree.xilinx@gmail.com>]

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.