On Thu, Apr 05, 2018 at 11:55:10AM +0200, Nélio Laranjeiro wrote:
> On Wed, Apr 04, 2018 at 05:56:49PM +0200, 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.
> >
> > 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.
> >
> > - 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>
<snip>
> > @@ -1306,12 +1309,21 @@ mlx5_flow_create_vlan(const struct rte_flow_item
> > *item,
> > */
> > if (!eth->mask.vlan_tag)
> > goto error;
> > + /* Exactly one TPID value is allowed if specified. */
> > + if ((eth->val.ether_type & eth->mask.ether_type) !=
> > + (RTE_BE16(0x8100) & eth->mask.ether_type)) {
>
> You can use ETHER_TYPE_VLAN present in rte_ether.h instead of hard coded
> values.
Good catch, I forgot about those macros. I'll make the change in all
impacted code in the next revision.
--
Adrien Mazarguil
6WIND
