On Thu, Apr 05, 2018 at 02:20:34PM +0000, Zhang, Qi Z wrote:
> Hi Adrien:
>
> > 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!
> > ---
> > diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
> > index
> > 1b336df74..c6dd889ad 100644
> > --- a/drivers/net/i40e/i40e_flow.c
> > +++ b/drivers/net/i40e/i40e_flow.c
> > @@ -2490,24 +2490,36 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev
> > *dev,
> > "Invalid MAC_addr mask.");
> > return -rte_errno;
> > }
> > + }
> > + if (eth_spec && eth_mask && eth_mask->type) {
> > + enum rte_flow_item_type next = (item + 1)->type;
> >
> > - if ((eth_mask->type & UINT16_MAX) ==
> > - UINT16_MAX) {
> > - input_set |= I40E_INSET_LAST_ETHER_TYPE;
> > - filter->input.flow.l2_flow.ether_type =
> > - eth_spec->type;
> > + if (eth_mask->type != RTE_BE16(0xffff)) {
> > + rte_flow_error_set(error, EINVAL,
> > + RTE_FLOW_ERROR_TYPE_ITEM,
> > + item,
> > + "Invalid type mask.");
> > + return -rte_errno;
> > }
> >
> > ether_type = rte_be_to_cpu_16(eth_spec->type);
> > - if (ether_type == ETHER_TYPE_IPv4 ||
> > - ether_type == ETHER_TYPE_IPv6 ||
> > - ether_type == ETHER_TYPE_ARP ||
> > - ether_type == outer_tpid) {
> > +
> > + if ((next == RTE_FLOW_ITEM_TYPE_VLAN &&
> > + ether_type != outer_tpid) ||
> > + (next != RTE_FLOW_ITEM_TYPE_VLAN &&
> > + (ether_type == ETHER_TYPE_IPv4 ||
> > + ether_type == ETHER_TYPE_IPv6 ||
> > + ether_type == ETHER_TYPE_ARP ||
> > + ether_type == outer_tpid))) {
> > rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_ITEM,
> > item,
> > "Unsupported ether_type.");
> > return -rte_errno;
> > + } else if (next != RTE_FLOW_ITEM_TYPE_VLAN) {
> > + input_set |= I40E_INSET_LAST_ETHER_TYPE;
> > + filter->input.flow.l2_flow.ether_type =
> > + eth_spec->type;
> > }
> > }
> >
> > @@ -2518,6 +2530,14 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev
> > *dev,
> > case RTE_FLOW_ITEM_TYPE_VLAN:
> > vlan_spec = item->spec;
> > vlan_mask = item->mask;
> > +
> > + if (input_set & I40E_INSET_LAST_ETHER_TYPE) {
> > + rte_flow_error_set(error, EINVAL,
> > + RTE_FLOW_ERROR_TYPE_ITEM,
> > + item,
> > + "Unsupported outer TPID.");
> > + return -rte_errno;
> > + }
>
> Please correct me I'm wrong, now I40E_INSET_LAST_ETHER_TYPE will only be set
> when next != RTE_FLOW_ITEM_TYPE_VLAN
> while, RTE_FLOW_ITEM_TYPE_VLAN will only be the next to
> RTE_FLOW_ITEM_TYPE_ETH for fdir, so above check seems unnecessary ?
You're right, it's unnecessary. I can remove this change or leave it as a
safety measure for future contributions, since when parsing a VLAN item
one is not necessarily aware of what happened in previous iterations.
How about an assertion check for debugging purposes instead?
RTE_ASSERT(!(input_set & I40E_INSET_LAST_ETHER_TYPE));
--
Adrien Mazarguil
6WIND
