[5/6] net/mlx5: add VLAN item and actions to switch flow rules
Checks
Commit Message
This enables flow rules to explicitly match VLAN traffic (VLAN pattern
item) and perform various operations on VLAN headers at the switch level
(OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID and OF_SET_VLAN_PCP actions).
Testpmd examples:
- Directing all VLAN traffic received on port ID 1 to port ID 0:
flow create 1 ingress transfer pattern eth / vlan / end actions
port_id id 0 / end
- Adding a VLAN header to IPv6 traffic received on port ID 1 and directing
it to port ID 0:
flow create 1 ingress transfer pattern eth / ipv6 / end actions
of_push_vlan ethertype 0x8100 / of_set_vlan_vid / port_id id 0 / end
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
drivers/net/mlx5/mlx5_nl_flow.c | 177 ++++++++++++++++++++++++++++++++++-
1 file changed, 173 insertions(+), 4 deletions(-)
Comments
On Wed, Jun 27, 2018 at 08:08:18PM +0200, Adrien Mazarguil wrote:
> This enables flow rules to explicitly match VLAN traffic (VLAN pattern
> item) and perform various operations on VLAN headers at the switch level
> (OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID and OF_SET_VLAN_PCP actions).
>
> Testpmd examples:
>
> - Directing all VLAN traffic received on port ID 1 to port ID 0:
>
> flow create 1 ingress transfer pattern eth / vlan / end actions
> port_id id 0 / end
>
> - Adding a VLAN header to IPv6 traffic received on port ID 1 and directing
> it to port ID 0:
>
> flow create 1 ingress transfer pattern eth / ipv6 / end actions
> of_push_vlan ethertype 0x8100 / of_set_vlan_vid / port_id id 0 / end
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
> drivers/net/mlx5/mlx5_nl_flow.c | 177 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 173 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_nl_flow.c b/drivers/net/mlx5/mlx5_nl_flow.c
> index ad1e001c6..a45d94fae 100644
> --- a/drivers/net/mlx5/mlx5_nl_flow.c
> +++ b/drivers/net/mlx5/mlx5_nl_flow.c
> @@ -13,6 +13,7 @@
> #include <linux/rtnetlink.h>
> #include <linux/tc_act/tc_gact.h>
> #include <linux/tc_act/tc_mirred.h>
> +#include <linux/tc_act/tc_vlan.h>
> #include <netinet/in.h>
> #include <stdalign.h>
> #include <stdbool.h>
> @@ -36,6 +37,7 @@ enum mlx5_nl_flow_trans {
> PATTERN,
> ITEM_VOID,
> ITEM_ETH,
> + ITEM_VLAN,
> ITEM_IPV4,
> ITEM_IPV6,
> ITEM_TCP,
> @@ -44,6 +46,10 @@ enum mlx5_nl_flow_trans {
> ACTION_VOID,
> ACTION_PORT_ID,
> ACTION_DROP,
> + ACTION_OF_POP_VLAN,
> + ACTION_OF_PUSH_VLAN,
> + ACTION_OF_SET_VLAN_VID,
> + ACTION_OF_SET_VLAN_PCP,
> END,
> };
>
> @@ -52,7 +58,8 @@ enum mlx5_nl_flow_trans {
> #define PATTERN_COMMON \
> ITEM_VOID, ACTIONS
> #define ACTIONS_COMMON \
> - ACTION_VOID
> + ACTION_VOID, ACTION_OF_POP_VLAN, ACTION_OF_PUSH_VLAN, \
> + ACTION_OF_SET_VLAN_VID, ACTION_OF_SET_VLAN_PCP
> #define ACTIONS_FATE \
> ACTION_PORT_ID, ACTION_DROP
>
> @@ -63,7 +70,8 @@ static const enum mlx5_nl_flow_trans *const mlx5_nl_flow_trans[] = {
> [ATTR] = TRANS(PATTERN),
> [PATTERN] = TRANS(ITEM_ETH, PATTERN_COMMON),
> [ITEM_VOID] = TRANS(BACK),
> - [ITEM_ETH] = TRANS(ITEM_IPV4, ITEM_IPV6, PATTERN_COMMON),
> + [ITEM_ETH] = TRANS(ITEM_IPV4, ITEM_IPV6, ITEM_VLAN, PATTERN_COMMON),
> + [ITEM_VLAN] = TRANS(ITEM_IPV4, ITEM_IPV6, PATTERN_COMMON),
> [ITEM_IPV4] = TRANS(ITEM_TCP, ITEM_UDP, PATTERN_COMMON),
> [ITEM_IPV6] = TRANS(ITEM_TCP, ITEM_UDP, PATTERN_COMMON),
> [ITEM_TCP] = TRANS(PATTERN_COMMON),
> @@ -72,12 +80,17 @@ static const enum mlx5_nl_flow_trans *const mlx5_nl_flow_trans[] = {
> [ACTION_VOID] = TRANS(BACK),
> [ACTION_PORT_ID] = TRANS(ACTION_VOID, END),
> [ACTION_DROP] = TRANS(ACTION_VOID, END),
> + [ACTION_OF_POP_VLAN] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
> + [ACTION_OF_PUSH_VLAN] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
> + [ACTION_OF_SET_VLAN_VID] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
> + [ACTION_OF_SET_VLAN_PCP] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
> [END] = NULL,
> };
>
> /** Empty masks for known item types. */
> static const union {
> struct rte_flow_item_eth eth;
> + struct rte_flow_item_vlan vlan;
> struct rte_flow_item_ipv4 ipv4;
> struct rte_flow_item_ipv6 ipv6;
> struct rte_flow_item_tcp tcp;
> @@ -87,6 +100,7 @@ static const union {
> /** Supported masks for known item types. */
> static const struct {
> struct rte_flow_item_eth eth;
> + struct rte_flow_item_vlan vlan;
> struct rte_flow_item_ipv4 ipv4;
> struct rte_flow_item_ipv6 ipv6;
> struct rte_flow_item_tcp tcp;
> @@ -97,6 +111,11 @@ static const struct {
> .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> .src.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> },
> + .vlan = {
> + /* PCP and VID only, no DEI. */
> + .tci = RTE_BE16(0xefff),
> + .inner_type = RTE_BE16(0xffff),
> + },
> .ipv4.hdr = {
> .next_proto_id = 0xff,
> .src_addr = RTE_BE32(0xffffffff),
> @@ -242,9 +261,13 @@ mlx5_nl_flow_transpose(void *buf,
> unsigned int n;
> uint32_t act_index_cur;
> bool eth_type_set;
> + bool vlan_present;
> + bool vlan_eth_type_set;
> bool ip_proto_set;
> struct nlattr *na_flower;
> struct nlattr *na_flower_act;
> + struct nlattr *na_vlan_id;
> + struct nlattr *na_vlan_priority;
> const enum mlx5_nl_flow_trans *trans;
> const enum mlx5_nl_flow_trans *back;
>
> @@ -256,15 +279,20 @@ mlx5_nl_flow_transpose(void *buf,
> n = 0;
> act_index_cur = 0;
> eth_type_set = false;
> + vlan_present = false;
> + vlan_eth_type_set = false;
> ip_proto_set = false;
> na_flower = NULL;
> na_flower_act = NULL;
> + na_vlan_id = NULL;
> + na_vlan_priority = NULL;
> trans = TRANS(ATTR);
> back = trans;
> trans:
> switch (trans[n++]) {
> union {
> const struct rte_flow_item_eth *eth;
> + const struct rte_flow_item_vlan *vlan;
> const struct rte_flow_item_ipv4 *ipv4;
> const struct rte_flow_item_ipv6 *ipv6;
> const struct rte_flow_item_tcp *tcp;
> @@ -272,6 +300,11 @@ mlx5_nl_flow_transpose(void *buf,
> } spec, mask;
> union {
> const struct rte_flow_action_port_id *port_id;
> + const struct rte_flow_action_of_push_vlan *of_push_vlan;
> + const struct rte_flow_action_of_set_vlan_vid *
> + of_set_vlan_vid;
> + const struct rte_flow_action_of_set_vlan_pcp *
> + of_set_vlan_pcp;
> } conf;
> struct nlmsghdr *nlh;
> struct tcmsg *tcm;
> @@ -408,6 +441,58 @@ mlx5_nl_flow_transpose(void *buf,
> goto error_nobufs;
> ++item;
> break;
> + case ITEM_VLAN:
> + if (item->type != RTE_FLOW_ITEM_TYPE_VLAN)
> + goto trans;
> + mask.vlan = mlx5_nl_flow_item_mask
> + (item, &rte_flow_item_vlan_mask,
> + &mlx5_nl_flow_mask_supported.vlan,
> + &mlx5_nl_flow_mask_empty.vlan,
> + sizeof(mlx5_nl_flow_mask_supported.vlan), error);
> + if (!mask.vlan)
> + return -rte_errno;
> + if (!eth_type_set &&
> + !mnl_attr_put_u16_check(buf, size,
> + TCA_FLOWER_KEY_ETH_TYPE,
> + RTE_BE16(ETH_P_8021Q)))
> + goto error_nobufs;
> + eth_type_set = 1;
> + vlan_present = 1;
> + if (mask.vlan == &mlx5_nl_flow_mask_empty.vlan) {
> + ++item;
> + break;
> + }
> + spec.vlan = item->spec;
> + if ((mask.vlan->tci & RTE_BE16(0xe000) &&
> + (mask.vlan->tci & RTE_BE16(0xe000)) != RTE_BE16(0xe000)) ||
> + (mask.vlan->tci & RTE_BE16(0x0fff) &&
> + (mask.vlan->tci & RTE_BE16(0x0fff)) != RTE_BE16(0x0fff)) ||
> + (mask.vlan->inner_type &&
> + mask.vlan->inner_type != RTE_BE16(0xffff)))
> + return rte_flow_error_set
> + (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM_MASK,
> + mask.vlan,
> + "no support for partial masks on"
> + " \"tci\" (PCP and VID parts) and"
> + " \"inner_type\" fields");
> + if (mask.vlan->inner_type) {
> + if (!mnl_attr_put_u16_check
> + (buf, size, TCA_FLOWER_KEY_VLAN_ETH_TYPE,
> + spec.vlan->inner_type))
> + goto error_nobufs;
> + vlan_eth_type_set = 1;
> + }
> + if ((mask.vlan->tci & RTE_BE16(0xe000) &&
> + !mnl_attr_put_u8_check
> + (buf, size, TCA_FLOWER_KEY_VLAN_PRIO,
> + (rte_be_to_cpu_16(spec.vlan->tci) >> 13) & 0x7)) ||
> + (mask.vlan->tci & RTE_BE16(0x0fff) &&
> + !mnl_attr_put_u16_check
> + (buf, size, TCA_FLOWER_KEY_VLAN_ID,
> + spec.vlan->tci & RTE_BE16(0x0fff))))
> + goto error_nobufs;
> + ++item;
> + break;
> case ITEM_IPV4:
> if (item->type != RTE_FLOW_ITEM_TYPE_IPV4)
> goto trans;
> @@ -418,12 +503,15 @@ mlx5_nl_flow_transpose(void *buf,
> sizeof(mlx5_nl_flow_mask_supported.ipv4), error);
> if (!mask.ipv4)
> return -rte_errno;
> - if (!eth_type_set &&
> + if ((!eth_type_set || !vlan_eth_type_set) &&
> !mnl_attr_put_u16_check(buf, size,
> + vlan_present ?
> + TCA_FLOWER_KEY_VLAN_ETH_TYPE :
> TCA_FLOWER_KEY_ETH_TYPE,
> RTE_BE16(ETH_P_IP)))
> goto error_nobufs;
> eth_type_set = 1;
> + vlan_eth_type_set = 1;
> if (mask.ipv4 == &mlx5_nl_flow_mask_empty.ipv4) {
> ++item;
> break;
> @@ -470,12 +558,15 @@ mlx5_nl_flow_transpose(void *buf,
> sizeof(mlx5_nl_flow_mask_supported.ipv6), error);
> if (!mask.ipv6)
> return -rte_errno;
> - if (!eth_type_set &&
> + if ((!eth_type_set || !vlan_eth_type_set) &&
> !mnl_attr_put_u16_check(buf, size,
> + vlan_present ?
> + TCA_FLOWER_KEY_VLAN_ETH_TYPE :
> TCA_FLOWER_KEY_ETH_TYPE,
> RTE_BE16(ETH_P_IPV6)))
> goto error_nobufs;
> eth_type_set = 1;
> + vlan_eth_type_set = 1;
> if (mask.ipv6 == &mlx5_nl_flow_mask_empty.ipv6) {
> ++item;
> break;
> @@ -681,6 +772,84 @@ mlx5_nl_flow_transpose(void *buf,
> mnl_attr_nest_end(buf, act_index);
> ++action;
> break;
> + case ACTION_OF_POP_VLAN:
> + if (action->type != RTE_FLOW_ACTION_TYPE_OF_POP_VLAN)
> + goto trans;
> + conf.of_push_vlan = NULL;
> + i = TCA_VLAN_ACT_POP;
> + goto action_of_vlan;
> + case ACTION_OF_PUSH_VLAN:
> + if (action->type != RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN)
> + goto trans;
> + conf.of_push_vlan = action->conf;
> + i = TCA_VLAN_ACT_PUSH;
> + goto action_of_vlan;
> + case ACTION_OF_SET_VLAN_VID:
> + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID)
> + goto trans;
> + conf.of_set_vlan_vid = action->conf;
> + if (na_vlan_id)
> + goto override_na_vlan_id;
> + i = TCA_VLAN_ACT_MODIFY;
> + goto action_of_vlan;
> + case ACTION_OF_SET_VLAN_PCP:
> + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP)
> + goto trans;
> + conf.of_set_vlan_pcp = action->conf;
> + if (na_vlan_priority)
> + goto override_na_vlan_priority;
> + i = TCA_VLAN_ACT_MODIFY;
> + goto action_of_vlan;
> +action_of_vlan:
> + act_index =
> + mnl_attr_nest_start_check(buf, size, act_index_cur++);
> + if (!act_index ||
> + !mnl_attr_put_strz_check(buf, size, TCA_ACT_KIND, "vlan"))
> + goto error_nobufs;
> + act = mnl_attr_nest_start_check(buf, size, TCA_ACT_OPTIONS);
> + if (!act)
> + goto error_nobufs;
> + if (!mnl_attr_put_check(buf, size, TCA_VLAN_PARMS,
> + sizeof(struct tc_vlan),
> + &(struct tc_vlan){
> + .action = TC_ACT_PIPE,
> + .v_action = i,
> + }))
> + goto error_nobufs;
> + if (i == TCA_VLAN_ACT_POP) {
> + mnl_attr_nest_end(buf, act);
> + ++action;
> + break;
> + }
> + if (i == TCA_VLAN_ACT_PUSH &&
> + !mnl_attr_put_u16_check(buf, size,
> + TCA_VLAN_PUSH_VLAN_PROTOCOL,
> + conf.of_push_vlan->ethertype))
> + goto error_nobufs;
> + na_vlan_id = mnl_nlmsg_get_payload_tail(buf);
> + if (!mnl_attr_put_u16_check(buf, size, TCA_VLAN_PAD, 0))
> + goto error_nobufs;
> + na_vlan_priority = mnl_nlmsg_get_payload_tail(buf);
> + if (!mnl_attr_put_u8_check(buf, size, TCA_VLAN_PAD, 0))
> + goto error_nobufs;
> + mnl_attr_nest_end(buf, act);
> + mnl_attr_nest_end(buf, act_index);
> + if (action->type == RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID) {
> +override_na_vlan_id:
> + na_vlan_id->nla_type = TCA_VLAN_PUSH_VLAN_ID;
> + *(uint16_t *)mnl_attr_get_payload(na_vlan_id) =
> + rte_be_to_cpu_16
> + (conf.of_set_vlan_vid->vlan_vid);
> + } else if (action->type ==
> + RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP) {
> +override_na_vlan_priority:
> + na_vlan_priority->nla_type =
> + TCA_VLAN_PUSH_VLAN_PRIORITY;
> + *(uint8_t *)mnl_attr_get_payload(na_vlan_priority) =
> + conf.of_set_vlan_pcp->vlan_pcp;
> + }
> + ++action;
> + break;
I'm wondering if there's no need to check the existence of VLAN in pattern when
having VLAN modification actions. For example, if flow has POP_VLAN action, its
pattern has to have VLAN item, doesn't it? Even though kernel driver has such
validation checks, mlx5_flow_validate() can't validate it.
In the PRM,
8.18.2.7 Packet Classification Ambiguities
...
In addition, a flow should not match or attempt to modify (Modify Header
action, Pop VLAN action) non-existing fields of a packet, as defined by
the packet classification process.
...
Thanks,
Yongseok
> case END:
> if (item->type != RTE_FLOW_ITEM_TYPE_END ||
> action->type != RTE_FLOW_ACTION_TYPE_END)
> --
> 2.11.0
On Wed, Jul 11, 2018 at 06:10:25PM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 08:08:18PM +0200, Adrien Mazarguil wrote:
> > This enables flow rules to explicitly match VLAN traffic (VLAN pattern
> > item) and perform various operations on VLAN headers at the switch level
> > (OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID and OF_SET_VLAN_PCP actions).
> >
> > Testpmd examples:
> >
> > - Directing all VLAN traffic received on port ID 1 to port ID 0:
> >
> > flow create 1 ingress transfer pattern eth / vlan / end actions
> > port_id id 0 / end
> >
> > - Adding a VLAN header to IPv6 traffic received on port ID 1 and directing
> > it to port ID 0:
> >
> > flow create 1 ingress transfer pattern eth / ipv6 / end actions
> > of_push_vlan ethertype 0x8100 / of_set_vlan_vid / port_id id 0 / end
> >
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
<snip>
> > @@ -681,6 +772,84 @@ mlx5_nl_flow_transpose(void *buf,
> > mnl_attr_nest_end(buf, act_index);
> > ++action;
> > break;
> > + case ACTION_OF_POP_VLAN:
> > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_POP_VLAN)
> > + goto trans;
> > + conf.of_push_vlan = NULL;
> > + i = TCA_VLAN_ACT_POP;
> > + goto action_of_vlan;
> > + case ACTION_OF_PUSH_VLAN:
> > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN)
> > + goto trans;
> > + conf.of_push_vlan = action->conf;
> > + i = TCA_VLAN_ACT_PUSH;
> > + goto action_of_vlan;
> > + case ACTION_OF_SET_VLAN_VID:
> > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID)
> > + goto trans;
> > + conf.of_set_vlan_vid = action->conf;
> > + if (na_vlan_id)
> > + goto override_na_vlan_id;
> > + i = TCA_VLAN_ACT_MODIFY;
> > + goto action_of_vlan;
> > + case ACTION_OF_SET_VLAN_PCP:
> > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP)
> > + goto trans;
> > + conf.of_set_vlan_pcp = action->conf;
> > + if (na_vlan_priority)
> > + goto override_na_vlan_priority;
> > + i = TCA_VLAN_ACT_MODIFY;
> > + goto action_of_vlan;
> > +action_of_vlan:
> > + act_index =
> > + mnl_attr_nest_start_check(buf, size, act_index_cur++);
> > + if (!act_index ||
> > + !mnl_attr_put_strz_check(buf, size, TCA_ACT_KIND, "vlan"))
> > + goto error_nobufs;
> > + act = mnl_attr_nest_start_check(buf, size, TCA_ACT_OPTIONS);
> > + if (!act)
> > + goto error_nobufs;
> > + if (!mnl_attr_put_check(buf, size, TCA_VLAN_PARMS,
> > + sizeof(struct tc_vlan),
> > + &(struct tc_vlan){
> > + .action = TC_ACT_PIPE,
> > + .v_action = i,
> > + }))
> > + goto error_nobufs;
> > + if (i == TCA_VLAN_ACT_POP) {
> > + mnl_attr_nest_end(buf, act);
> > + ++action;
> > + break;
> > + }
> > + if (i == TCA_VLAN_ACT_PUSH &&
> > + !mnl_attr_put_u16_check(buf, size,
> > + TCA_VLAN_PUSH_VLAN_PROTOCOL,
> > + conf.of_push_vlan->ethertype))
> > + goto error_nobufs;
> > + na_vlan_id = mnl_nlmsg_get_payload_tail(buf);
> > + if (!mnl_attr_put_u16_check(buf, size, TCA_VLAN_PAD, 0))
> > + goto error_nobufs;
> > + na_vlan_priority = mnl_nlmsg_get_payload_tail(buf);
> > + if (!mnl_attr_put_u8_check(buf, size, TCA_VLAN_PAD, 0))
> > + goto error_nobufs;
> > + mnl_attr_nest_end(buf, act);
> > + mnl_attr_nest_end(buf, act_index);
> > + if (action->type == RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID) {
> > +override_na_vlan_id:
> > + na_vlan_id->nla_type = TCA_VLAN_PUSH_VLAN_ID;
> > + *(uint16_t *)mnl_attr_get_payload(na_vlan_id) =
> > + rte_be_to_cpu_16
> > + (conf.of_set_vlan_vid->vlan_vid);
> > + } else if (action->type ==
> > + RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP) {
> > +override_na_vlan_priority:
> > + na_vlan_priority->nla_type =
> > + TCA_VLAN_PUSH_VLAN_PRIORITY;
> > + *(uint8_t *)mnl_attr_get_payload(na_vlan_priority) =
> > + conf.of_set_vlan_pcp->vlan_pcp;
> > + }
> > + ++action;
> > + break;
>
> I'm wondering if there's no need to check the existence of VLAN in pattern when
> having VLAN modification actions. For example, if flow has POP_VLAN action, its
> pattern has to have VLAN item, doesn't it?
Not necessarily. For instance there is no need to explicitly match VLAN
traffic if you somehow guarantee that only VLAN traffic goes through,
e.g. in case peer is configured to always push a VLAN header regardless,
requesting explicit match in this sense can be thought as an unnecessary
limitation.
I agree this check would have been mandatory if this check wasn't performed
elsewhere, as discussed below:
> Even though kernel driver has such
> validation checks, mlx5_flow_validate() can't validate it.
Yes, note this is consistent with the rest of this particular implementation
(VLAN POP is not an exception). This entire code is a somewhat generic
rte_flow-to-TC converter which doesn't check HW capabilities at all, it
doesn't check the private structure, type of device and so on. This role is
left to the kernel implementation and (optionally) the caller function.
The only explicit checks are performed at conversion stage; if something
cannot be converted from rte_flow to TC, as is the case for VLAN DEI (hence
the 0xefff mask). The rest is implicit, for instance I didn't bother to
implement all pattern items and fields, only the bare minimum.
Further, ConnectX-4 and ConnectX-5 have different capabilities. The former
only supports offloading destination MAC matching and the drop action at the
switch level. Depending on driver/firmware combinations, such and such
feature may or may not be present.
Checking everything in order to print nice error messages would have been
nice, but would have required a lot of effort. Hence the decision to
restrict the scope of this function.
> In the PRM,
> 8.18.2.7 Packet Classification Ambiguities
> ...
> In addition, a flow should not match or attempt to modify (Modify Header
> action, Pop VLAN action) non-existing fields of a packet, as defined by
> the packet classification process.
> ...
Fortunately this code is not running on top of PRM :)
This is my opinion anyway. If you think we need extra safety for (and only
for) VLAN POP, I'll add it, please confirm.
On Thu, Jul 12, 2018 at 12:47:09PM +0200, Adrien Mazarguil wrote:
> On Wed, Jul 11, 2018 at 06:10:25PM -0700, Yongseok Koh wrote:
> > On Wed, Jun 27, 2018 at 08:08:18PM +0200, Adrien Mazarguil wrote:
> > > This enables flow rules to explicitly match VLAN traffic (VLAN pattern
> > > item) and perform various operations on VLAN headers at the switch level
> > > (OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID and OF_SET_VLAN_PCP actions).
> > >
> > > Testpmd examples:
> > >
> > > - Directing all VLAN traffic received on port ID 1 to port ID 0:
> > >
> > > flow create 1 ingress transfer pattern eth / vlan / end actions
> > > port_id id 0 / end
> > >
> > > - Adding a VLAN header to IPv6 traffic received on port ID 1 and directing
> > > it to port ID 0:
> > >
> > > flow create 1 ingress transfer pattern eth / ipv6 / end actions
> > > of_push_vlan ethertype 0x8100 / of_set_vlan_vid / port_id id 0 / end
> > >
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
> > > @@ -681,6 +772,84 @@ mlx5_nl_flow_transpose(void *buf,
> > > mnl_attr_nest_end(buf, act_index);
> > > ++action;
> > > break;
> > > + case ACTION_OF_POP_VLAN:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_POP_VLAN)
> > > + goto trans;
> > > + conf.of_push_vlan = NULL;
> > > + i = TCA_VLAN_ACT_POP;
> > > + goto action_of_vlan;
> > > + case ACTION_OF_PUSH_VLAN:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN)
> > > + goto trans;
> > > + conf.of_push_vlan = action->conf;
> > > + i = TCA_VLAN_ACT_PUSH;
> > > + goto action_of_vlan;
> > > + case ACTION_OF_SET_VLAN_VID:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID)
> > > + goto trans;
> > > + conf.of_set_vlan_vid = action->conf;
> > > + if (na_vlan_id)
> > > + goto override_na_vlan_id;
> > > + i = TCA_VLAN_ACT_MODIFY;
> > > + goto action_of_vlan;
> > > + case ACTION_OF_SET_VLAN_PCP:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP)
> > > + goto trans;
> > > + conf.of_set_vlan_pcp = action->conf;
> > > + if (na_vlan_priority)
> > > + goto override_na_vlan_priority;
> > > + i = TCA_VLAN_ACT_MODIFY;
> > > + goto action_of_vlan;
> > > +action_of_vlan:
> > > + act_index =
> > > + mnl_attr_nest_start_check(buf, size, act_index_cur++);
> > > + if (!act_index ||
> > > + !mnl_attr_put_strz_check(buf, size, TCA_ACT_KIND, "vlan"))
> > > + goto error_nobufs;
> > > + act = mnl_attr_nest_start_check(buf, size, TCA_ACT_OPTIONS);
> > > + if (!act)
> > > + goto error_nobufs;
> > > + if (!mnl_attr_put_check(buf, size, TCA_VLAN_PARMS,
> > > + sizeof(struct tc_vlan),
> > > + &(struct tc_vlan){
> > > + .action = TC_ACT_PIPE,
> > > + .v_action = i,
> > > + }))
> > > + goto error_nobufs;
> > > + if (i == TCA_VLAN_ACT_POP) {
> > > + mnl_attr_nest_end(buf, act);
> > > + ++action;
> > > + break;
> > > + }
> > > + if (i == TCA_VLAN_ACT_PUSH &&
> > > + !mnl_attr_put_u16_check(buf, size,
> > > + TCA_VLAN_PUSH_VLAN_PROTOCOL,
> > > + conf.of_push_vlan->ethertype))
> > > + goto error_nobufs;
> > > + na_vlan_id = mnl_nlmsg_get_payload_tail(buf);
> > > + if (!mnl_attr_put_u16_check(buf, size, TCA_VLAN_PAD, 0))
> > > + goto error_nobufs;
> > > + na_vlan_priority = mnl_nlmsg_get_payload_tail(buf);
> > > + if (!mnl_attr_put_u8_check(buf, size, TCA_VLAN_PAD, 0))
> > > + goto error_nobufs;
> > > + mnl_attr_nest_end(buf, act);
> > > + mnl_attr_nest_end(buf, act_index);
> > > + if (action->type == RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID) {
> > > +override_na_vlan_id:
> > > + na_vlan_id->nla_type = TCA_VLAN_PUSH_VLAN_ID;
> > > + *(uint16_t *)mnl_attr_get_payload(na_vlan_id) =
> > > + rte_be_to_cpu_16
> > > + (conf.of_set_vlan_vid->vlan_vid);
> > > + } else if (action->type ==
> > > + RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP) {
> > > +override_na_vlan_priority:
> > > + na_vlan_priority->nla_type =
> > > + TCA_VLAN_PUSH_VLAN_PRIORITY;
> > > + *(uint8_t *)mnl_attr_get_payload(na_vlan_priority) =
> > > + conf.of_set_vlan_pcp->vlan_pcp;
> > > + }
> > > + ++action;
> > > + break;
> >
> > I'm wondering if there's no need to check the existence of VLAN in pattern when
> > having VLAN modification actions. For example, if flow has POP_VLAN action, its
> > pattern has to have VLAN item, doesn't it?
>
> Not necessarily. For instance there is no need to explicitly match VLAN
> traffic if you somehow guarantee that only VLAN traffic goes through,
> e.g. in case peer is configured to always push a VLAN header regardless,
> requesting explicit match in this sense can be thought as an unnecessary
> limitation.
>
> I agree this check would have been mandatory if this check wasn't performed
> elsewhere, as discussed below:
From user's perspective, it may not be necessary to specify VLAN in the pattern
as specifying POP_VLAN action implies that. But from device/PMD perspective,
there could be two options, a) complain about the violation or b) silently add
VLAN pattern to not cause unexpected behavior in the device.
> > Even though kernel driver has such
> > validation checks, mlx5_flow_validate() can't validate it.
>
> Yes, note this is consistent with the rest of this particular implementation
> (VLAN POP is not an exception). This entire code is a somewhat generic
> rte_flow-to-TC converter which doesn't check HW capabilities at all, it
> doesn't check the private structure, type of device and so on. This role is
> left to the kernel implementation and (optionally) the caller function.
>
> The only explicit checks are performed at conversion stage; if something
> cannot be converted from rte_flow to TC, as is the case for VLAN DEI (hence
> the 0xefff mask). The rest is implicit, for instance I didn't bother to
> implement all pattern items and fields, only the bare minimum.
>
> Further, ConnectX-4 and ConnectX-5 have different capabilities. The former
> only supports offloading destination MAC matching and the drop action at the
> switch level. Depending on driver/firmware combinations, such and such
> feature may or may not be present.
>
> Checking everything in order to print nice error messages would have been
> nice, but would have required a lot of effort. Hence the decision to
> restrict the scope of this function.
I worried about a case where a flow gets success from validation call but
creation call returns an error (error from kernel). It would be a violation of
requirement of rte_flow library.
However, I agree that this implementation should have limited scope for now as
the current lib/ker implementation is quite divergent. We have two separate
paths to configure the flow and this should be unified. Good news is we'll get
to have the unified path eventually.
> > In the PRM,
> > 8.18.2.7 Packet Classification Ambiguities
> > ...
> > In addition, a flow should not match or attempt to modify (Modify Header
> > action, Pop VLAN action) non-existing fields of a packet, as defined by
> > the packet classification process.
> > ...
>
> Fortunately this code is not running on top of PRM :)
>
> This is my opinion anyway. If you think we need extra safety for (and only
> for) VLAN POP, I'll add it, please confirm.
Well, I'll leave the decision to you.
Acked-by: Yongseok Koh <yskoh@mellanox.com>
Thanks
@@ -13,6 +13,7 @@
#include <linux/rtnetlink.h>
#include <linux/tc_act/tc_gact.h>
#include <linux/tc_act/tc_mirred.h>
+#include <linux/tc_act/tc_vlan.h>
#include <netinet/in.h>
#include <stdalign.h>
#include <stdbool.h>
@@ -36,6 +37,7 @@ enum mlx5_nl_flow_trans {
PATTERN,
ITEM_VOID,
ITEM_ETH,
+ ITEM_VLAN,
ITEM_IPV4,
ITEM_IPV6,
ITEM_TCP,
@@ -44,6 +46,10 @@ enum mlx5_nl_flow_trans {
ACTION_VOID,
ACTION_PORT_ID,
ACTION_DROP,
+ ACTION_OF_POP_VLAN,
+ ACTION_OF_PUSH_VLAN,
+ ACTION_OF_SET_VLAN_VID,
+ ACTION_OF_SET_VLAN_PCP,
END,
};
@@ -52,7 +58,8 @@ enum mlx5_nl_flow_trans {
#define PATTERN_COMMON \
ITEM_VOID, ACTIONS
#define ACTIONS_COMMON \
- ACTION_VOID
+ ACTION_VOID, ACTION_OF_POP_VLAN, ACTION_OF_PUSH_VLAN, \
+ ACTION_OF_SET_VLAN_VID, ACTION_OF_SET_VLAN_PCP
#define ACTIONS_FATE \
ACTION_PORT_ID, ACTION_DROP
@@ -63,7 +70,8 @@ static const enum mlx5_nl_flow_trans *const mlx5_nl_flow_trans[] = {
[ATTR] = TRANS(PATTERN),
[PATTERN] = TRANS(ITEM_ETH, PATTERN_COMMON),
[ITEM_VOID] = TRANS(BACK),
- [ITEM_ETH] = TRANS(ITEM_IPV4, ITEM_IPV6, PATTERN_COMMON),
+ [ITEM_ETH] = TRANS(ITEM_IPV4, ITEM_IPV6, ITEM_VLAN, PATTERN_COMMON),
+ [ITEM_VLAN] = TRANS(ITEM_IPV4, ITEM_IPV6, PATTERN_COMMON),
[ITEM_IPV4] = TRANS(ITEM_TCP, ITEM_UDP, PATTERN_COMMON),
[ITEM_IPV6] = TRANS(ITEM_TCP, ITEM_UDP, PATTERN_COMMON),
[ITEM_TCP] = TRANS(PATTERN_COMMON),
@@ -72,12 +80,17 @@ static const enum mlx5_nl_flow_trans *const mlx5_nl_flow_trans[] = {
[ACTION_VOID] = TRANS(BACK),
[ACTION_PORT_ID] = TRANS(ACTION_VOID, END),
[ACTION_DROP] = TRANS(ACTION_VOID, END),
+ [ACTION_OF_POP_VLAN] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
+ [ACTION_OF_PUSH_VLAN] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
+ [ACTION_OF_SET_VLAN_VID] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
+ [ACTION_OF_SET_VLAN_PCP] = TRANS(ACTIONS_FATE, ACTIONS_COMMON),
[END] = NULL,
};
/** Empty masks for known item types. */
static const union {
struct rte_flow_item_eth eth;
+ struct rte_flow_item_vlan vlan;
struct rte_flow_item_ipv4 ipv4;
struct rte_flow_item_ipv6 ipv6;
struct rte_flow_item_tcp tcp;
@@ -87,6 +100,7 @@ static const union {
/** Supported masks for known item types. */
static const struct {
struct rte_flow_item_eth eth;
+ struct rte_flow_item_vlan vlan;
struct rte_flow_item_ipv4 ipv4;
struct rte_flow_item_ipv6 ipv6;
struct rte_flow_item_tcp tcp;
@@ -97,6 +111,11 @@ static const struct {
.dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
.src.addr_bytes = "\xff\xff\xff\xff\xff\xff",
},
+ .vlan = {
+ /* PCP and VID only, no DEI. */
+ .tci = RTE_BE16(0xefff),
+ .inner_type = RTE_BE16(0xffff),
+ },
.ipv4.hdr = {
.next_proto_id = 0xff,
.src_addr = RTE_BE32(0xffffffff),
@@ -242,9 +261,13 @@ mlx5_nl_flow_transpose(void *buf,
unsigned int n;
uint32_t act_index_cur;
bool eth_type_set;
+ bool vlan_present;
+ bool vlan_eth_type_set;
bool ip_proto_set;
struct nlattr *na_flower;
struct nlattr *na_flower_act;
+ struct nlattr *na_vlan_id;
+ struct nlattr *na_vlan_priority;
const enum mlx5_nl_flow_trans *trans;
const enum mlx5_nl_flow_trans *back;
@@ -256,15 +279,20 @@ mlx5_nl_flow_transpose(void *buf,
n = 0;
act_index_cur = 0;
eth_type_set = false;
+ vlan_present = false;
+ vlan_eth_type_set = false;
ip_proto_set = false;
na_flower = NULL;
na_flower_act = NULL;
+ na_vlan_id = NULL;
+ na_vlan_priority = NULL;
trans = TRANS(ATTR);
back = trans;
trans:
switch (trans[n++]) {
union {
const struct rte_flow_item_eth *eth;
+ const struct rte_flow_item_vlan *vlan;
const struct rte_flow_item_ipv4 *ipv4;
const struct rte_flow_item_ipv6 *ipv6;
const struct rte_flow_item_tcp *tcp;
@@ -272,6 +300,11 @@ mlx5_nl_flow_transpose(void *buf,
} spec, mask;
union {
const struct rte_flow_action_port_id *port_id;
+ const struct rte_flow_action_of_push_vlan *of_push_vlan;
+ const struct rte_flow_action_of_set_vlan_vid *
+ of_set_vlan_vid;
+ const struct rte_flow_action_of_set_vlan_pcp *
+ of_set_vlan_pcp;
} conf;
struct nlmsghdr *nlh;
struct tcmsg *tcm;
@@ -408,6 +441,58 @@ mlx5_nl_flow_transpose(void *buf,
goto error_nobufs;
++item;
break;
+ case ITEM_VLAN:
+ if (item->type != RTE_FLOW_ITEM_TYPE_VLAN)
+ goto trans;
+ mask.vlan = mlx5_nl_flow_item_mask
+ (item, &rte_flow_item_vlan_mask,
+ &mlx5_nl_flow_mask_supported.vlan,
+ &mlx5_nl_flow_mask_empty.vlan,
+ sizeof(mlx5_nl_flow_mask_supported.vlan), error);
+ if (!mask.vlan)
+ return -rte_errno;
+ if (!eth_type_set &&
+ !mnl_attr_put_u16_check(buf, size,
+ TCA_FLOWER_KEY_ETH_TYPE,
+ RTE_BE16(ETH_P_8021Q)))
+ goto error_nobufs;
+ eth_type_set = 1;
+ vlan_present = 1;
+ if (mask.vlan == &mlx5_nl_flow_mask_empty.vlan) {
+ ++item;
+ break;
+ }
+ spec.vlan = item->spec;
+ if ((mask.vlan->tci & RTE_BE16(0xe000) &&
+ (mask.vlan->tci & RTE_BE16(0xe000)) != RTE_BE16(0xe000)) ||
+ (mask.vlan->tci & RTE_BE16(0x0fff) &&
+ (mask.vlan->tci & RTE_BE16(0x0fff)) != RTE_BE16(0x0fff)) ||
+ (mask.vlan->inner_type &&
+ mask.vlan->inner_type != RTE_BE16(0xffff)))
+ return rte_flow_error_set
+ (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM_MASK,
+ mask.vlan,
+ "no support for partial masks on"
+ " \"tci\" (PCP and VID parts) and"
+ " \"inner_type\" fields");
+ if (mask.vlan->inner_type) {
+ if (!mnl_attr_put_u16_check
+ (buf, size, TCA_FLOWER_KEY_VLAN_ETH_TYPE,
+ spec.vlan->inner_type))
+ goto error_nobufs;
+ vlan_eth_type_set = 1;
+ }
+ if ((mask.vlan->tci & RTE_BE16(0xe000) &&
+ !mnl_attr_put_u8_check
+ (buf, size, TCA_FLOWER_KEY_VLAN_PRIO,
+ (rte_be_to_cpu_16(spec.vlan->tci) >> 13) & 0x7)) ||
+ (mask.vlan->tci & RTE_BE16(0x0fff) &&
+ !mnl_attr_put_u16_check
+ (buf, size, TCA_FLOWER_KEY_VLAN_ID,
+ spec.vlan->tci & RTE_BE16(0x0fff))))
+ goto error_nobufs;
+ ++item;
+ break;
case ITEM_IPV4:
if (item->type != RTE_FLOW_ITEM_TYPE_IPV4)
goto trans;
@@ -418,12 +503,15 @@ mlx5_nl_flow_transpose(void *buf,
sizeof(mlx5_nl_flow_mask_supported.ipv4), error);
if (!mask.ipv4)
return -rte_errno;
- if (!eth_type_set &&
+ if ((!eth_type_set || !vlan_eth_type_set) &&
!mnl_attr_put_u16_check(buf, size,
+ vlan_present ?
+ TCA_FLOWER_KEY_VLAN_ETH_TYPE :
TCA_FLOWER_KEY_ETH_TYPE,
RTE_BE16(ETH_P_IP)))
goto error_nobufs;
eth_type_set = 1;
+ vlan_eth_type_set = 1;
if (mask.ipv4 == &mlx5_nl_flow_mask_empty.ipv4) {
++item;
break;
@@ -470,12 +558,15 @@ mlx5_nl_flow_transpose(void *buf,
sizeof(mlx5_nl_flow_mask_supported.ipv6), error);
if (!mask.ipv6)
return -rte_errno;
- if (!eth_type_set &&
+ if ((!eth_type_set || !vlan_eth_type_set) &&
!mnl_attr_put_u16_check(buf, size,
+ vlan_present ?
+ TCA_FLOWER_KEY_VLAN_ETH_TYPE :
TCA_FLOWER_KEY_ETH_TYPE,
RTE_BE16(ETH_P_IPV6)))
goto error_nobufs;
eth_type_set = 1;
+ vlan_eth_type_set = 1;
if (mask.ipv6 == &mlx5_nl_flow_mask_empty.ipv6) {
++item;
break;
@@ -681,6 +772,84 @@ mlx5_nl_flow_transpose(void *buf,
mnl_attr_nest_end(buf, act_index);
++action;
break;
+ case ACTION_OF_POP_VLAN:
+ if (action->type != RTE_FLOW_ACTION_TYPE_OF_POP_VLAN)
+ goto trans;
+ conf.of_push_vlan = NULL;
+ i = TCA_VLAN_ACT_POP;
+ goto action_of_vlan;
+ case ACTION_OF_PUSH_VLAN:
+ if (action->type != RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN)
+ goto trans;
+ conf.of_push_vlan = action->conf;
+ i = TCA_VLAN_ACT_PUSH;
+ goto action_of_vlan;
+ case ACTION_OF_SET_VLAN_VID:
+ if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID)
+ goto trans;
+ conf.of_set_vlan_vid = action->conf;
+ if (na_vlan_id)
+ goto override_na_vlan_id;
+ i = TCA_VLAN_ACT_MODIFY;
+ goto action_of_vlan;
+ case ACTION_OF_SET_VLAN_PCP:
+ if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP)
+ goto trans;
+ conf.of_set_vlan_pcp = action->conf;
+ if (na_vlan_priority)
+ goto override_na_vlan_priority;
+ i = TCA_VLAN_ACT_MODIFY;
+ goto action_of_vlan;
+action_of_vlan:
+ act_index =
+ mnl_attr_nest_start_check(buf, size, act_index_cur++);
+ if (!act_index ||
+ !mnl_attr_put_strz_check(buf, size, TCA_ACT_KIND, "vlan"))
+ goto error_nobufs;
+ act = mnl_attr_nest_start_check(buf, size, TCA_ACT_OPTIONS);
+ if (!act)
+ goto error_nobufs;
+ if (!mnl_attr_put_check(buf, size, TCA_VLAN_PARMS,
+ sizeof(struct tc_vlan),
+ &(struct tc_vlan){
+ .action = TC_ACT_PIPE,
+ .v_action = i,
+ }))
+ goto error_nobufs;
+ if (i == TCA_VLAN_ACT_POP) {
+ mnl_attr_nest_end(buf, act);
+ ++action;
+ break;
+ }
+ if (i == TCA_VLAN_ACT_PUSH &&
+ !mnl_attr_put_u16_check(buf, size,
+ TCA_VLAN_PUSH_VLAN_PROTOCOL,
+ conf.of_push_vlan->ethertype))
+ goto error_nobufs;
+ na_vlan_id = mnl_nlmsg_get_payload_tail(buf);
+ if (!mnl_attr_put_u16_check(buf, size, TCA_VLAN_PAD, 0))
+ goto error_nobufs;
+ na_vlan_priority = mnl_nlmsg_get_payload_tail(buf);
+ if (!mnl_attr_put_u8_check(buf, size, TCA_VLAN_PAD, 0))
+ goto error_nobufs;
+ mnl_attr_nest_end(buf, act);
+ mnl_attr_nest_end(buf, act_index);
+ if (action->type == RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID) {
+override_na_vlan_id:
+ na_vlan_id->nla_type = TCA_VLAN_PUSH_VLAN_ID;
+ *(uint16_t *)mnl_attr_get_payload(na_vlan_id) =
+ rte_be_to_cpu_16
+ (conf.of_set_vlan_vid->vlan_vid);
+ } else if (action->type ==
+ RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP) {
+override_na_vlan_priority:
+ na_vlan_priority->nla_type =
+ TCA_VLAN_PUSH_VLAN_PRIORITY;
+ *(uint8_t *)mnl_attr_get_payload(na_vlan_priority) =
+ conf.of_set_vlan_pcp->vlan_pcp;
+ }
+ ++action;
+ break;
case END:
if (item->type != RTE_FLOW_ITEM_TYPE_END ||
action->type != RTE_FLOW_ACTION_TYPE_END)