[dpdk-dev] [PATCH v6 10/16] ethdev: fix TPID handling in flow API

[Adrien Mazarguil]

On Wed, Apr 25, 2018 at 05:27:56PM +0200, Adrien Mazarguil wrote:
> TPID handling in rte_flow VLAN and E_TAG pattern item definitions is not
> consistent with the normal stacking order of pattern items, which is
> confusing to applications.
> 
> Problem is that when followed by one of these layers, the EtherType field
> of the preceding layer keeps its "inner" definition, and the "outer" TPID
> is provided by the subsequent layer, the reverse of how a packet looks like
> on the wire:
> 
>  Wire:     [ ETH TPID = A | VLAN EtherType = B | B DATA ]
>  rte_flow: [ ETH EtherType = B | VLAN TPID = A | B DATA ]
> 
> Worse, when QinQ is involved, the stacking order of VLAN layers is
> unspecified. It is unclear whether it should be reversed (innermost to
> outermost) as well given TPID applies to the previous layer:
> 
>  Wire:       [ ETH TPID = A | VLAN TPID = B | VLAN EtherType = C | C DATA ]
>  rte_flow 1: [ ETH EtherType = C | VLAN TPID = B | VLAN TPID = A | C DATA ]
>  rte_flow 2: [ ETH EtherType = C | VLAN TPID = A | VLAN TPID = B | C DATA ]
> 
> While specifying EtherType/TPID is hopefully rarely necessary, the stacking
> order in case of QinQ and the lack of documentation remain an issue.
> 
> This patch replaces TPID in the VLAN pattern item with an inner
> EtherType/TPID as is usually done everywhere else (e.g. struct vlan_hdr),
> clarifies documentation and updates all relevant code.
> 
> It breaks ABI compatibility for the following public functions:
> 
> - rte_flow_copy()
> - rte_flow_create()
> - rte_flow_query()
> - rte_flow_validate()
> 
> Summary of changes for PMDs that implement ETH, VLAN or E_TAG pattern
> items:
> 
> - bnxt: EtherType matching is supported with and without VLAN, but TPID
>   matching is not and triggers an error.
> 
> - e1000: EtherType matching is only supported with the ETHERTYPE filter,
>   which does not support VLAN matching, therefore no impact.
> 
> - enic: same as bnxt.
> 
> - i40e: same as bnxt with existing FDIR limitations on allowed EtherType
>   values. The remaining filter types (VXLAN, NVGRE, QINQ) do not support
>   EtherType matching.
> 
> - ixgbe: same as e1000, with additional minor change to rely on the new
>   E-Tag macro definition.
> 
> - mlx4: EtherType/TPID matching is not supported, no impact.
> 
> - mlx5: same as bnxt.
> 
> - mvpp2: same as bnxt.
> 
> - sfc: same as bnxt.
> 
> - tap: same as bnxt.
> 
> Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")
> Fixes: 99e7003831c3 ("net/ixgbe: parse L2 tunnel filter")
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil at 6wind.com>
> Acked-by: Andrew Rybchenko <arybchenko at solarflare.com>
> Cc: Ferruh Yigit <ferruh.yigit at intel.com>
> Cc: Thomas Monjalon <thomas at monjalon.net>
> Cc: Wenzhuo Lu <wenzhuo.lu at intel.com>
> Cc: Jingjing Wu <jingjing.wu at intel.com>
> Cc: Ajit Khaparde <ajit.khaparde at broadcom.com>
> Cc: Somnath Kotur <somnath.kotur at broadcom.com>
> Cc: John Daley <johndale at cisco.com>
> Cc: Hyong Youb Kim <hyonkim at cisco.com>
> Cc: Beilei Xing <beilei.xing at intel.com>
> Cc: Qi Zhang <qi.z.zhang at intel.com>
> Cc: Konstantin Ananyev <konstantin.ananyev at intel.com>
> Cc: Nelio Laranjeiro <nelio.laranjeiro at 6wind.com>
> Cc: Yongseok Koh <yskoh at mellanox.com>
> Cc: Tomasz Duszynski <tdu at semihalf.com>
> Cc: Dmitri Epshtein <dima at marvell.com>
> Cc: Natalie Samsonov <nsamsono at marvell.com>
> Cc: Jianbo Liu <jianbo.liu at arm.com>
> Cc: Andrew Rybchenko <arybchenko at solarflare.com>
> Cc: Pascal Mazon <pascal.mazon at 6wind.com>
> 
> ---
> 
> v6 changes:
> 
> - Reworded patch title as a "fix" (not candidate for backports) since it
>   addresses a flaw in the original API definition.
> - Updated API and ABI changes sections in release notes.
> 
> v3 changes:
> 
> Updated mrvl to mvpp2.
> 
> Moved unrelated default TCI mask update to separate patch.
> 
> Fixed sfc according to Andrew's comments [1], which made so much sense that
> I standardized on the same behavior for all other PMDs: matching outer TPID
> is never supported when a VLAN pattern item is present.
> 
> This is done because many devices accept several TPIDs but do not provide
> means to match a given one explicitly, it's all or nothing, and that makes
> the resulting flow rule inaccurate.
> 
> [1] http://dpdk.org/ml/archives/dev/2018-April/095870.html
<snip>
> diff --git a/drivers/net/mvpp2/mrvl_flow.c b/drivers/net/mvpp2/mrvl_flow.c
<snip>
> +	if (mask->inner_type) {
> +		struct rte_flow_item_eth spec_eth = {
> +			.type = spec->inner_type,
> +		};
> +		struct rte_flow_item_eth mask_eth = {
> +			.type = mask->inner_type,
> +		};
> +
> +		RTE_LOG(WARNING, PMD, "inner eth type mask is ignored\n");
> +		ret = mrvl_parse_type(spec_eth, mask_eth, flow);

Looks like there's a compilation issue left on the above line, which should
obviously read:

 ret = mrvl_parse_type(&spec_eth, &mask_eth, flow);

Ferruh, can you update it while applying?
I'd hate to spam the world with v7 :)

Thanks.

-- 
Adrien Mazarguil
6WIND