Re: [PATCH v1 11/16] ethdev: refine TPID handling in flow API

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

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.mazarguil@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