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.mazarg...@6wind.com>
Cc: Ferruh Yigit <ferruh.yi...@intel.com>
Cc: Thomas Monjalon <tho...@monjalon.net>
Cc: Wenzhuo Lu <wenzhuo...@intel.com>
Cc: Jingjing Wu <jingjing...@intel.com>
Cc: Ajit Khaparde <ajit.khapa...@broadcom.com>
Cc: Somnath Kotur <somnath.ko...@broadcom.com>
Cc: John Daley <johnd...@cisco.com>
Cc: Hyong Youb Kim <hyon...@cisco.com>
Cc: Beilei Xing <beilei.x...@intel.com>
Cc: Qi Zhang <qi.z.zh...@intel.com>
Cc: Konstantin Ananyev <konstantin.anan...@intel.com>
Cc: Nelio Laranjeiro <nelio.laranje...@6wind.com>
Cc: Yongseok Koh <ys...@mellanox.com>
Cc: Tomasz Duszynski <t...@semihalf.com>
Cc: Dmitri Epshtein <d...@marvell.com>
Cc: Natalie Samsonov <nsams...@marvell.com>
Cc: Jianbo Liu <jianbo....@arm.com>
Cc: Andrew Rybchenko <arybche...@solarflare.com>
Cc: Pascal Mazon <pascal.ma...@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
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
<...>
