From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 54AC31D33E for ; Fri, 6 Apr 2018 19:11:56 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id C249FA8007B; Fri, 6 Apr 2018 17:11:53 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Fri, 6 Apr 2018 18:11:42 +0100 To: Adrien Mazarguil , Thomas Monjalon , Ferruh Yigit , CC: 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 References: <20180404150312.12304-1-adrien.mazarguil@6wind.com> <20180406131736.19145-1-adrien.mazarguil@6wind.com> <20180406131736.19145-11-adrien.mazarguil@6wind.com> From: Andrew Rybchenko Message-ID: <81ba063f-2c8d-9711-9cef-007cfec5638a@solarflare.com> Date: Fri, 6 Apr 2018 20:11:38 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180406131736.19145-11-adrien.mazarguil@6wind.com> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23766.003 X-TM-AS-Result: No--33.307700-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1523034715-rhOFJ62dzHfk Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Fri, 06 Apr 2018 17:11:56 -0000 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 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. > + 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. > + .inner_type = RTE_BE16(0x0000), > }; > #endif <...>