Re: [PATCH net-next 04/15] virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support

This adds VIRTCHNL_VLAN_ETHERTYPE_88E7 = BIT(3) to enum
virtchnl_vlan_support so that the 0x88E7 VLAN ethertype can be negotiated
over VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS.  The change touches only the
shared virtchnl header.

> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
> index 11bdab5522fd5..511c8827c640f 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -636,6 +636,7 @@ enum virtchnl_vlan_support {
>  	VIRTCHNL_VLAN_ETHERTYPE_8100 =		BIT(0),
>  	VIRTCHNL_VLAN_ETHERTYPE_88A8 =		BIT(1),
>  	VIRTCHNL_VLAN_ETHERTYPE_9100 =		BIT(2),
> +	VIRTCHNL_VLAN_ETHERTYPE_88E7 =		BIT(3),

[Severity: Medium]
Is there anything in the tree that can actually negotiate this bit?

The commit message says the bit is added "so that VF drivers can advertise
and negotiate 0x88E7 VLAN ethertype support through the existing
VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS handshake", but on the PF side
ice_vc_set_dvm_caps() and ice_vc_set_svm_caps() in
drivers/net/ethernet/intel/ice/virt/virtchnl.c only ever fill in:

	supported_caps->outer = VIRTCHNL_VLAN_ETHERTYPE_8100 |
				VIRTCHNL_VLAN_ETHERTYPE_88A8 |
				VIRTCHNL_VLAN_ETHERTYPE_9100 |
				VIRTCHNL_VLAN_ETHERTYPE_AND;
	caps->filtering.ethertype_init = VIRTCHNL_VLAN_ETHERTYPE_8100 |
					 VIRTCHNL_VLAN_ETHERTYPE_88A8 |
					 VIRTCHNL_VLAN_ETHERTYPE_9100;

so BIT(3) never appears in the caps message stored in vf->vlan_v2_caps.

On the VF side, iavf_tpid_to_vc_ethertype() in
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c maps only ETH_P_8021Q and
ETH_P_8021AD, and iavf_get_netdev_vlan_hw_features() /
iavf_get_netdev_vlan_features() in iavf_main.c test only the _8100 and
_88A8 bits, so a PF that did advertise BIT(3) would be silently ignored.

As a consequence, the ETH_P_8021AH / VIRTCHNL_VLAN_ETHERTYPE_88E7 cases
added by the follow-up patch in this series ("ice: add 0x88E7 handling to
SW validation paths") look unreachable.  ice_vc_validate_vlan_tpid() ends
with:

	if (!(filtering_caps & vlan_ethertype))
		return false;

and filtering_caps never contains BIT(3), so 0x88E7 filters are still
rejected.  Likewise ice_vc_get_tpid() is only reached after
ice_vc_valid_vlan_setting() has already done:

	if (ethertype_setting && !(negotiated_settings & ethertype_setting))
		return false;

Should the advertisement side land in the same series, or is the intent
that only an out-of-tree/firmware PF sets this bit?  If the latter, would
it be worth dropping the claim in the follow-up commit that "the PF
correctly accepts and translates 0x88E7 (ETH_P_8021AH) VLAN filters
requested by VFs", since that is not the case at the end of the series?

[Severity: Medium]
Is the terminology in the commit message right?  It says:

	IEEE 802.1ah (Provider Backbone Bridging) uses ethertype 0x88E7 as the
	Backbone Service Tag (B-TAG).

802.1ah uses 0x88A8 for the Backbone VLAN Tag (B-TAG), which is already
covered by VIRTCHNL_VLAN_ETHERTYPE_88A8, and 0x88E7 for the Backbone
Service Instance Tag (I-TAG).  The ice parser documentation in
drivers/net/ethernet/intel/ice/ice_protocol_type.h agrees:

 * K = STAG or BVLAN - Outer L2 header has STAG (ethernet type 0x88a8) or
 *		       BVLAN (ethernet type 0x88a8).
 * L = ITAG - Outer L2 header has ITAG *ethernet type 0x88e7)

Related to that, how is a 0x88E7 tag meant to be expressed through this
ABI?  struct virtchnl_vlan in this same header carries only:

	u16 tci;	/* tci[15:13] = PCP and tci[11:0] = VID */
	u16 tci_mask;
	u16 tpid;	/* 0x8100, 0x88a8, etc. ... */

which has no room for an 802.1ah 24-bit I-SID.

And on the hardware side, ICE_AQ_VSI_OUTER_TAG_TYPE_M in
drivers/net/ethernet/intel/ice/ice_adminq_cmd.h is a two-bit field with
only NONE/STAG/VLAN_8100/VLAN_9100 encodings, and
tpid_to_vsi_outer_vlan_type() in ice_vsi_vlan_lib.c ends with:

	default:
		*tag_type = 0;
		return -EINVAL;

So a peer that did negotiate BIT(3) and then sent
VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2 would reach
ice_vc_ena_vlan_offload() -> ice_vsi_ena_outer_stripping(vsi,
ETH_P_8021AH) and get -EINVAL, while its own feature flags claim support.
Could the commit message state which 802.1ah tag is meant and how it maps
onto struct virtchnl_vlan and the hardware outer tag types?

[Severity: Low]
Should the documentation block above this enum gain a matching line?  It
currently describes each ethertype bit:

 * VIRTCHNL_VLAN_ETHERTYPE_8100 - This field supports 0x8100 ethertype.
 * VIRTCHNL_VLAN_ETHERTYPE_88A8 - This field supports 0x88A8 ethertype.
 * VIRTCHNL_VLAN_ETHERTYPE_9100 - This field supports 0x9100 ethertype.

but there is no entry for the new bit, and the AND/XOR wording in the same
block is written in terms of the enumerated ethertypes.  Since 0x88E7 is
described as ETH_P_8021AH "802.1ah Backbone Service Tag" in
include/uapi/linux/if_ether.h but as ITAG in ice_protocol_type.h, a
third-party implementer reading this header cannot tell whether the bit
means B-TAG or I-TAG handling.  Adding the documentation line, and
referencing ETH_P_8021AH, would pin that down.

>  	VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1 =	BIT(8),
>  	VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2 =	BIT(9),
>  	VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 =	BIT(10),