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.mazarg...@6wind.com>
> Acked-by: Andrew Rybchenko <arybche...@solarflare.com>
> Cc: Ferruh Yigit <ferruh.yi...@intel.com>
> Cc: Thomas Monjalon <tho...@monjalon.net>
> Cc: Wenzhuo Lu <wenzhuo...@intel.com>
> Cc: Jingjing Wu <jingjing...@intel.com>
> Cc: Ajit Khaparde <ajit.khapa...@broadcom.com>
> Cc: Somnath Kotur <somnath.ko...@broadcom.com>
> Cc: John Daley <johnd...@cisco.com>
> Cc: Hyong Youb Kim <hyon...@cisco.com>
> Cc: Beilei Xing <beilei.x...@intel.com>
> Cc: Qi Zhang <qi.z.zh...@intel.com>
> Cc: Konstantin Ananyev <konstantin.anan...@intel.com>
> Cc: Nelio Laranjeiro <nelio.laranje...@6wind.com>
> Cc: Yongseok Koh <ys...@mellanox.com>
> Cc: Tomasz Duszynski <t...@semihalf.com>
> Cc: Dmitri Epshtein <d...@marvell.com>
> Cc: Natalie Samsonov <nsams...@marvell.com>
> Cc: Jianbo Liu <jianbo....@arm.com>
> Cc: Andrew Rybchenko <arybche...@solarflare.com>
> Cc: Pascal Mazon <pascal.ma...@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
