Re: [PATCH v6 10/16] ethdev: fix TPID handling in flow API

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.mazarguil@6wind.com>
>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> Cc: Jingjing Wu <jingjing.wu@intel.com>
>> Cc: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Cc: Somnath Kotur <somnath.kotur@broadcom.com>
>> Cc: John Daley <johndale@cisco.com>
>> Cc: Hyong Youb Kim <hyonkim@cisco.com>
>> Cc: Beilei Xing <beilei.xing@intel.com>
>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> Cc: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>> Cc: Yongseok Koh <yskoh@mellanox.com>
>> Cc: Tomasz Duszynski <tdu@semihalf.com>
>> Cc: Dmitri Epshtein <dima@marvell.com>
>> Cc: Natalie Samsonov <nsamsono@marvell.com>
>> Cc: Jianbo Liu <jianbo.liu@arm.com>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>> Cc: Pascal Mazon <pascal.mazon@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.
>