On Tue, Jul 03, 2018 at 04:56:21PM -0700, Yongseok Koh wrote: > On Wed, Jun 27, 2018 at 05:07:39PM +0200, Nelio Laranjeiro wrote: > > Signed-off-by: Nelio Laranjeiro <nelio.laranje...@6wind.com> > > --- > > drivers/net/mlx5/mlx5_flow.c | 123 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 123 insertions(+) > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > > index 6593eafa0..6a576ddd9 100644 > > --- a/drivers/net/mlx5/mlx5_flow.c > > +++ b/drivers/net/mlx5/mlx5_flow.c > > @@ -420,6 +420,126 @@ mlx5_flow_item_eth(const struct rte_flow_item *item, > > struct rte_flow *flow, > > return size; > > } > > > > +/** > > + * Update the VLAN tag in the Ethernet spec. > > + * > > + * @param attr[in, out] > > + * Pointer to Verbs attributes structure. > > + * @param eth[in] > > + * Verbs structure containing the VLAN information to copy. > > + */ > > +static void > > +mlx5_flow_item_vlan_update(struct ibv_flow_attr *attr, > > + struct ibv_flow_spec_eth *eth) > > +{ > > + unsigned int i; > > + enum ibv_flow_spec_type search = IBV_FLOW_SPEC_ETH; > > + struct ibv_spec_header *hdr = (struct ibv_spec_header *) > > + ((uint8_t *)attr + sizeof(struct ibv_flow_attr)); > > + > > + for (i = 0; i != attr->num_of_specs; ++i) { > > + if (hdr->type == search) { > > + struct ibv_flow_spec_eth *e = > > + (struct ibv_flow_spec_eth *)hdr; > > + > > + e->val.vlan_tag = eth->val.vlan_tag; > > + e->mask.vlan_tag = eth->mask.vlan_tag; > > You'll have to overwrite ether_type as well, otherwise the e->ether_type would > have TPID.
Right, > > + break; > > + } > > + hdr = (struct ibv_spec_header *)((uint8_t *)hdr + hdr->size); > > + } > > +} > > + > > +/** > > + * Validate VLAN layer and possibly create/modify the Verbs specification. > > + * > > + * @param item[in] > > + * Item specification. > > + * @param flow[in, out] > > + * Pointer to flow structure. > > + * @param flow_size[in] > > + * Size of the buffer to store the specification. > > + * @param error > > + * Pointer to error structure. > > + * > > + * @return > > + * size in bytes necessary for the conversion, a negative errno value > > + * otherwise and rte_errno is set. > > + */ > > +static int > > +mlx5_flow_item_vlan(const struct rte_flow_item *item, struct rte_flow > > *flow, > > + const size_t flow_size, struct rte_flow_error *error) > > +{ > > + const struct rte_flow_item_vlan *spec = item->spec; > > + const struct rte_flow_item_vlan *mask = item->mask; > > + const struct rte_flow_item_vlan nic_mask = { > > + .tci = RTE_BE16(0x0fff), > > + }; > > + unsigned int size = sizeof(struct ibv_flow_spec_eth); > > + struct ibv_flow_spec_eth eth = { > > + .type = IBV_FLOW_SPEC_ETH, > > + .size = size, > > + }; > > + int ret; > > + const uint32_t lm = MLX5_FLOW_LAYER_OUTER_L3 | > > + MLX5_FLOW_LAYER_OUTER_L4; > > What does 'lm' stand for? Better to use l34m? l34m seems better, > > + const uint32_t vlanm = MLX5_FLOW_LAYER_OUTER_VLAN; > > + const uint32_t l2m = MLX5_FLOW_LAYER_OUTER_L2; > > + > > + if (flow->layers & vlanm) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "L2 layers already configured"); > > L2? Not VLAN? Indeed it is VLAN. > > + else if ((flow->layers & lm) != 0) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "L2 layer cannot follow L3/L4 layer"); > > + if (!mask) > > + mask = &rte_flow_item_vlan_mask; > > + ret = mlx5_flow_item_validate(item, (const uint8_t *)mask, > > + (const uint8_t *)&nic_mask, > > + sizeof(struct rte_flow_item_vlan), error); > > + if (ret) > > + return ret; > > + if (spec) { > > + eth.val.vlan_tag = spec->tci; > > + eth.mask.vlan_tag = mask->tci; > > + eth.val.vlan_tag &= eth.mask.vlan_tag; > > + eth.val.ether_type = spec->inner_type; > > + eth.mask.ether_type = mask->inner_type; > > + eth.val.ether_type &= eth.mask.ether_type; > > + } > > + /* > > + * From verbs perspective an empty VLAN is equivalent > > + * to a packet without VLAN layer. > > + */ > > + if (!eth.mask.vlan_tag) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ITEM_SPEC, > > + item->spec, > > + "VLAN cannot be empty"); > > + /* Outer TPID cannot be matched. */ > > + if (eth.mask.ether_type) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ITEM_SPEC, > > + item->spec, > > + "VLAN TPID matching is not" > > + " supported"); > > Not sure 100% but I don't think ether_type means TPID but the inner packet > type > coming after the VLAN ID. E.g. > > /dmac/smac/0x8100/TCI/0x0800/ipv4... > ^ > | I remember to have faced such issue several months ago, from what I understood it is configurable but there is no way in Verbs to make it. Currently (after testing) it matches the above logic, I will remove this check. > > + if (!(flow->layers & l2m)) { > > + if (size <= flow_size) > > + mlx5_flow_spec_verbs_add(flow, ð, size); > > + } else { > > + if (flow->verbs.attr) > > + mlx5_flow_item_vlan_update(flow->verbs.attr, ð); > > + size = 0; /**< Only an update is done in eth specification. */ > > Any specific reason to use doxygen style comment only here? No reason, I'll remove it. Thanks, -- Nélio Laranjeiro 6WIND