Re: [dpdk-dev] [PATCH v2 10/15] ethdev: refine TPID handling in flow API

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Fri, Apr 06, 2018 at 08:11:38PM +0300, Andrew Rybchenko wrote:
> On 04/06/2018 04:25 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, and vlan->inner_type overrides
> >    eth->type if the latter has standard TPID value 0x8100, otherwise an
> >    error is triggered.
> > 
> > - 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 a configurable TPID value for the FDIR filter,
> >    with existing 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.
> > 
> > - mrvl: EtherType matching is supported but eth->type cannot be specified
> >    when a VLAN item is present. However vlan->inner_type is used if
> >    specified.
> > 
> > - sfc: same as bnxt with QinQ TPID value 0x88a8 additionally supported.
> > 
> > - tap: same as bnxt.
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.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>
> > 
> > ---
> > 
> > Hi PMD maintainers, while I'm pretty confident in these changes, I could
> > not validate them with all devices.
> > 
> > It would be great if you could apply this patch, run testpmd, create VLAN
> > flow rules with/without inner EtherType as described and send matching
> > traffic while making sure nothing was broken in the process.
> > 
> > Thanks!
> > ---
> >   app/test-pmd/cmdline_flow.c                 | 17 +++---
> >   doc/guides/nics/tap.rst                     |  2 +-
> >   doc/guides/prog_guide/rte_flow.rst          | 21 ++++++--
> >   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +-
> >   drivers/net/bnxt/bnxt_filter.c              | 39 +++++++++++---
> >   drivers/net/enic/enic_flow.c                | 22 +++++---
> >   drivers/net/i40e/i40e_flow.c                | 69 +++++++++++++++++++-----
> >   drivers/net/ixgbe/ixgbe_ethdev.c            |  3 +-
> >   drivers/net/mlx5/mlx5_flow.c                | 16 +++++-
> >   drivers/net/mvpp2/mrvl_flow.c               | 27 +++++++---
> >   drivers/net/sfc/sfc_flow.c                  | 28 ++++++++++
> >   drivers/net/tap/tap_flow.c                  | 16 ++++--
> >   lib/librte_ether/rte_flow.h                 | 24 ++++++---
> >   lib/librte_net/rte_ether.h                  |  1 +
> >   14 files changed, 229 insertions(+), 60 deletions(-)
> 
> <...>
> 
> > diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c
> > index bc4974edf..f61d4ec92 100644
> > --- a/drivers/net/sfc/sfc_flow.c
> > +++ b/drivers/net/sfc/sfc_flow.c
> > @@ -7,6 +7,7 @@
> >    * for Solarflare) and Solarflare Communications, Inc.
> >    */
> > +#include <rte_byteorder.h>
> >   #include <rte_tailq.h>
> >   #include <rte_common.h>
> >   #include <rte_ethdev_driver.h>
> > @@ -351,6 +352,7 @@ sfc_flow_parse_vlan(const struct rte_flow_item *item,
> >   	const struct rte_flow_item_vlan *mask = NULL;
> >   	const struct rte_flow_item_vlan supp_mask = {
> >   		.tci = rte_cpu_to_be_16(ETH_VLAN_ID_MAX),
> > +		.inner_type = RTE_BE16(0xffff),
> >   	};
> >   	rc = sfc_flow_parse_init(item,
> > @@ -393,6 +395,32 @@ sfc_flow_parse_vlan(const struct rte_flow_item *item,
> >   		return -rte_errno;
> >   	}
> > +	/*
> > +	 * If an EtherType was already specified, make sure it is a valid
> > +	 * TPID for the current VLAN layer before overwriting it with the
> > +	 * specified inner type.
> > +	 */
> > +	if (efx_spec->efs_match_flags & EFX_FILTER_MATCH_ETHER_TYPE &&
> > +	    efx_spec->efs_ether_type != RTE_BE16(ETHER_TYPE_VLAN) &&
> > +	    efx_spec->efs_ether_type != RTE_BE16(ETHER_TYPE_QINQ)) {
> 
> 1. efs_ether_type is host-endian

Whoops, looks like I only half-fixed that endian issue in v2.

> 2. HW recognizes more TPIDs (0x9100, 0x9200, 0x9300) as VLAN
> 3. However, if some TPID is specified, user may expect that only VLAN
> packets
>     with specified TPID match. It is false expectation since the information
> is not
>     passed to HW to match (and there is no way to match).
>     So, it is safer to deny TPID specification (i.e. keep the first
> condition only).
>     From the flexibility point of view it is possible to allow any, but it
> should be
>     documented that exact match is not checked in fact.

Thanks for pointing this out. I've decided to update all PMDs to disallow
TPID matching because many devices support multiple concurrent TPIDs and
there's no way to match a given one explicitly. This should make the patch
simpler as well.

> > +		rte_flow_error_set(error, EINVAL,
> > +				   RTE_FLOW_ERROR_TYPE_ITEM, item,
> > +				   "Unsupported outer TPID");
> > +		return -rte_errno;
> > +	}
> > +	if (!mask->inner_type) {
> > +		efx_spec->efs_match_flags &= ~EFX_FILTER_MATCH_ETHER_TYPE;
> > +		efx_spec->efs_ether_type = RTE_BE16(0x0000);
> 
> Nothing should be done here if above is done.
> 
> > +	} else if (mask->inner_type == supp_mask.inner_type) {
> > +		efx_spec->efs_match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
> > +		efx_spec->efs_ether_type = rte_bswap16(spec->inner_type);
> > +	} else {
> > +		rte_flow_error_set(error, EINVAL,
> > +				   RTE_FLOW_ERROR_TYPE_ITEM, item,
> > +				   "Bad mask for VLAN inner_type");
> > +		return -rte_errno;
> > +	}
> > +
> >   	return 0;
> >   }
> 
> <...>
> 
> > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> > index fc7e6705d..b13b0e2e6 100644
> > --- a/lib/librte_ether/rte_flow.h
> > +++ b/lib/librte_ether/rte_flow.h
> > @@ -475,19 +481,20 @@ static const struct rte_flow_item_eth rte_flow_item_eth_mask = {
> >    *
> >    * Matches an 802.1Q/ad VLAN tag.
> >    *
> > - * This type normally follows either RTE_FLOW_ITEM_TYPE_ETH or
> > - * RTE_FLOW_ITEM_TYPE_VLAN.
> > + * The corresponding standard outer EtherType (TPID) values are
> > + * ETHER_TYPE_VLAN or ETHER_TYPE_QINQ. It can be overridden by the preceding
> > + * pattern item.
> >    */
> >   struct rte_flow_item_vlan {
> > -	rte_be16_t tpid; /**< Tag protocol identifier. */
> >   	rte_be16_t tci; /**< Tag control information. */
> > +	rte_be16_t inner_type; /**< Inner EtherType or TPID. */
> >   };
> >   /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
> >   #ifndef __cplusplus
> >   static const struct rte_flow_item_vlan rte_flow_item_vlan_mask = {
> > -	.tpid = RTE_BE16(0x0000),
> > -	.tci = RTE_BE16(0xffff),
> > +	.tci = RTE_BE16(0x0fff),
> 
> It looks like unrelated change.

Yep, it should have been in a separate patch. I'll split it in my next
update. Thanks for reviewing.

> 
> > +	.inner_type = RTE_BE16(0x0000),
> >   };
> >   #endif
> 
> <...>
> 

-- 
Adrien Mazarguil
6WIND