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

[Andrew Rybchenko]

On 04/10/2018 07:36 PM, 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.
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil at 6wind.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>
>
> ---
>
> 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
> ---
>   app/test-pmd/cmdline_flow.c                 | 17 +++----
>   doc/guides/nics/tap.rst                     |  2 +-
>   doc/guides/prog_guide/rte_flow.rst          | 19 ++++++--
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +-
>   drivers/net/bnxt/bnxt_filter.c              | 35 +++++++++++---
>   drivers/net/enic/enic_flow.c                | 19 +++++---
>   drivers/net/i40e/i40e_flow.c                | 60 ++++++++++++++++++++----
>   drivers/net/ixgbe/ixgbe_ethdev.c            |  3 +-
>   drivers/net/mlx5/mlx5_flow.c                | 13 ++++-
>   drivers/net/mvpp2/mrvl_flow.c               | 26 +++++++---
>   drivers/net/sfc/sfc_flow.c                  | 18 +++++++
>   drivers/net/tap/tap_flow.c                  | 14 ++++--
>   lib/librte_ether/rte_flow.h                 | 22 ++++++---
>   lib/librte_net/rte_ether.h                  |  1 +
>   14 files changed, 198 insertions(+), 55 deletions(-)

Generic and net/sfc
Acked-by: Andrew Rybchenko <arybchenko at solarflare.com>