On 4/25/2018 5:10 PM, Adrien Mazarguil wrote:
> 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 :)
OK, will update while applying.
>
> Thanks.
>
