From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f181.google.com (mail-wr0-f181.google.com [209.85.128.181]) by dpdk.org (Postfix) with ESMTP id 716A61B76E for ; Mon, 9 Apr 2018 16:42:21 +0200 (CEST) Received: by mail-wr0-f181.google.com with SMTP id u46so9835631wrc.11 for ; Mon, 09 Apr 2018 07:42:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=qsalefwPOAxxW03KHys6/ZPo6DpTVp6FU1krD8m/Oeo=; b=i8KWW6pQLzTMQoXMdQBYrrY3fnnT9BJVpMd4D0r3yHzXRGS8buP2VqPUWXnvosX30r 3N9w/rMv3WpSQGjV5Atso3T0ZGs87vci6/H9DeDMnuyQU0Au0dMWWXYUSSo7WFo4evi4 jeVOnjAoHjcjCQZRt1uklv18kTNG4kq516L+oWdctsrwC8FmjyS1vD/Ub7wmOcIGXS/a CoJLYMo3vp7HeaJ//GS+iky0QFikH6w0oK4NSFoYR2t1Wz5BOkLtW32zAr9vVO6NKwJZ ayt/cjcEgObbE70dt1YsVhGhlvn6Ps33Ak5PonJqVG49fcNN7y30fdB4DqiXJHS9jphR Er1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=qsalefwPOAxxW03KHys6/ZPo6DpTVp6FU1krD8m/Oeo=; b=Wb0ln8rMcXTwO0pAYnrMakubACrBQB5ae22z6QfWNGFsrDMv3gnoGugxyfHF6BabuW 4Hp5+beAff0plq/TJUS1otuW0O/AN8t3i+gZNrV7Ex/hyMWCpY83ts3BJIcbzExFW37Z IEmb5mRPlqCDmkFGty/XB7Cz/uquhWZMS1nW+uGSjLD4zydl82VtglGZYM7fAZmKmADj 2nYUn0RG5PkPGblcOPWadvUtrFxzeHUcdnlKmPO9z7dmQxoTRCCMJ5wTaHkopbbLF1E1 +SLnSUN4TLDht1Dd/LfI2XeVyMiWwv83ckzp5l+szLFkcasAJG4cULUd2JnrWaTLuGmh MMag== X-Gm-Message-State: AElRT7GK5lZn5owMQGNBGBnT3o3GyRkb5r46ji/6qsAwySNEe9IBnMUk y7TObN5Q+Lf0TyWxcPHOGtOEjg== X-Google-Smtp-Source: AIpwx4+vxF4Ac2LglwlhoqAEx9pH5SGl05kFgN8PHZZWaPDo1nWn0qIK8//hcUVOWHNHnD8HT4zyYw== X-Received: by 10.223.201.12 with SMTP id m12mr28281831wrh.158.1523284941154; Mon, 09 Apr 2018 07:42:21 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id f2sm647858wre.76.2018.04.09.07.42.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Apr 2018 07:42:20 -0700 (PDT) Date: Mon, 9 Apr 2018 16:42:07 +0200 From: Adrien Mazarguil To: Andrew Rybchenko Cc: Thomas Monjalon , Ferruh Yigit , dev@dpdk.org, Wenzhuo Lu , Jingjing Wu , Ajit Khaparde , Somnath Kotur , John Daley , Hyong Youb Kim , Beilei Xing , Qi Zhang , Konstantin Ananyev , Nelio Laranjeiro , Yongseok Koh , Tomasz Duszynski , Dmitri Epshtein , Natalie Samsonov , Jianbo Liu , Pascal Mazon Message-ID: <20180409144207.GY4957@6wind.com> References: <20180404150312.12304-1-adrien.mazarguil@6wind.com> <20180406131736.19145-1-adrien.mazarguil@6wind.com> <20180406131736.19145-11-adrien.mazarguil@6wind.com> <81ba063f-2c8d-9711-9cef-007cfec5638a@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <81ba063f-2c8d-9711-9cef-007cfec5638a@solarflare.com> Subject: Re: [dpdk-dev] [PATCH v2 10/15] ethdev: refine TPID handling in flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Apr 2018 14:42:21 -0000 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 > > Cc: Ferruh Yigit > > Cc: Thomas Monjalon > > Cc: Wenzhuo Lu > > Cc: Jingjing Wu > > Cc: Ajit Khaparde > > Cc: Somnath Kotur > > Cc: John Daley > > Cc: Hyong Youb Kim > > Cc: Beilei Xing > > Cc: Qi Zhang > > Cc: Konstantin Ananyev > > Cc: Nelio Laranjeiro > > Cc: Yongseok Koh > > Cc: Tomasz Duszynski > > Cc: Dmitri Epshtein > > Cc: Natalie Samsonov > > Cc: Jianbo Liu > > Cc: Andrew Rybchenko > > Cc: Pascal Mazon > > > > --- > > > > 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 > > #include > > #include > > #include > > @@ -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