datapath: Simplify ODPAT_SET_DL_TCI action.

There's no need to have a mask in this action, because both parts of the
TCI are part of the flow structure.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>

diff --git a/datapath/actions.c b/datapath/actions.c
index 5365278..5904c83 100644 (file)
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -74,7 +74,6 @@ static struct sk_buff *modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
                                       const struct odp_flow_key *key,
                                       const union odp_action *a, int n_actions)
 {
-       __be16 mask = a->dl_tci.mask;
        __be16 tci = a->dl_tci.tci;
 
        skb = make_writable(skb, VLAN_HLEN);
@@ -92,7 +91,7 @@ static struct sk_buff *modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
                vh = vlan_eth_hdr(skb);
                old_tci = vh->h_vlan_TCI;
 
-               vh->h_vlan_TCI = (vh->h_vlan_TCI & ~mask) | tci;
+               vh->h_vlan_TCI = tci;
 
                if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE) {
                        __be16 diff[] = { ~old_tci, vh->h_vlan_TCI };

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9eeb28c..b41110d 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -904,7 +904,6 @@ static int validate_actions(const struct sw_flow_actions *actions)
 
        for (i = 0; i < actions->n_actions; i++) {
                const union odp_action *a = &actions->actions[i];
-               __be16 mask;
 
                switch (a->type) {
                case ODPAT_CONTROLLER:
@@ -928,12 +927,7 @@ static int validate_actions(const struct sw_flow_actions *actions)
                        break;
 
                case ODPAT_SET_DL_TCI:
-                       mask = a->dl_tci.mask;
-                       if (mask != htons(VLAN_VID_MASK) &&
-                           mask != htons(VLAN_PCP_MASK) &&
-                           mask != htons(VLAN_VID_MASK | VLAN_PCP_MASK))
-                               return -EINVAL;
-                       if (a->dl_tci.tci & ~mask)
+                       if (a->dl_tci.tci & htons(VLAN_CFI_MASK))
                                return -EINVAL;
                        break;
 

diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
index 1aa8066..b0e9dfb 100644 (file)
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -258,7 +258,7 @@ struct odp_flowvec {
 /* Action types. */
 #define ODPAT_OUTPUT            0    /* Output to switch port. */
 #define ODPAT_CONTROLLER        2    /* Send copy to controller. */
-#define ODPAT_SET_DL_TCI        3    /* Set the 802.1q VLAN VID and/or PCP. */
+#define ODPAT_SET_DL_TCI        3    /* Set the 802.1q TCI value. */
 #define ODPAT_STRIP_VLAN        5    /* Strip the 802.1q header. */
 #define ODPAT_SET_DL_SRC        6    /* Ethernet source address. */
 #define ODPAT_SET_DL_DST        7    /* Ethernet destination address. */
@@ -295,10 +295,8 @@ struct odp_action_tunnel {
 /* Action structure for ODPAT_SET_DL_TCI. */
 struct odp_action_dl_tci {
     uint16_t type;              /* ODPAT_SET_DL_TCI. */
-    ovs_be16 tci;               /* New TCI.  Bits not in mask must be zero. */
-    ovs_be16 mask;              /* 0x0fff to set VID, 0xe000 to set PCP,
-                                 * or 0xefff to set both. */
-    uint16_t reserved;
+    ovs_be16 tci;               /* New TCI.  CFI bit must be zero. */
+    uint32_t reserved;
 };
 
 /* Action structure for ODPAT_SET_DL_SRC/DST. */

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index aa4fdf8..3df6598 100644 (file)
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -667,12 +667,7 @@ dpif_netdev_validate_actions(const union odp_action *actions, int n_actions,
 
         case ODPAT_SET_DL_TCI:
             *mutates = true;
-            if (a->dl_tci.mask != htons(VLAN_VID_MASK)
-                && a->dl_tci.mask != htons(VLAN_PCP_MASK)
-                && a->dl_tci.mask != htons(VLAN_VID_MASK | VLAN_PCP_MASK)) {
-                return EINVAL;
-            }
-            if (a->dl_tci.tci & ~a->dl_tci.mask){
+            if (a->dl_tci.tci & htons(VLAN_CFI)) {
                 return EINVAL;
             }
             break;
@@ -1011,16 +1006,12 @@ dp_netdev_wait(void)
 }
 
 
-/* Modify the TCI field of 'packet'.  If a VLAN tag is not present, one is
- * added with the TCI field set to 'a->tci'.  If a VLAN tag is present, then
- * 'a->mask' bits are cleared before 'a->tci' is logically OR'd into the TCI
- * field.
- *
- * Note that the function does not ensure that 'a->tci' does not affect bits
- * outside of 'a->mask'.
+/* Modify the TCI field of 'packet'.  If a VLAN tag is present, its TCI field
+ * is replaced by 'tci'.  If a VLAN tag is not present, one is added with the
+ * TCI field set to 'tci'.
  */
 static void
-dp_netdev_set_dl_tci(struct ofpbuf *packet, const struct odp_action_dl_tci *a)
+dp_netdev_set_dl_tci(struct ofpbuf *packet, uint16_t tci)
 {
     struct vlan_eth_header *veh;
     struct eth_header *eh;
@@ -1028,16 +1019,15 @@ dp_netdev_set_dl_tci(struct ofpbuf *packet, const struct odp_action_dl_tci *a)
     eh = packet->l2;
     if (packet->size >= sizeof(struct vlan_eth_header)
         && eh->eth_type == htons(ETH_TYPE_VLAN)) {
-        /* Clear 'mask' bits, but maintain other TCI bits. */
         veh = packet->l2;
-        veh->veth_tci = (veh->veth_tci & ~a->mask) | a->tci;
+        veh->veth_tci = tci;
     } else {
         /* Insert new 802.1Q header. */
         struct vlan_eth_header tmp;
         memcpy(tmp.veth_dst, eh->eth_dst, ETH_ADDR_LEN);
         memcpy(tmp.veth_src, eh->eth_src, ETH_ADDR_LEN);
         tmp.veth_type = htons(ETH_TYPE_VLAN);
-        tmp.veth_tci = htons(a->tci);
+        tmp.veth_tci = tci;
         tmp.veth_next_type = eh->eth_type;
 
         veh = ofpbuf_push_uninit(packet, VLAN_HEADER_LEN);
@@ -1235,7 +1225,7 @@ dp_netdev_execute_actions(struct dp_netdev *dp,
             break;
 
         case ODPAT_SET_DL_TCI:
-            dp_netdev_set_dl_tci(packet, &a->dl_tci);
+            dp_netdev_set_dl_tci(packet, a->dl_tci.tci);
             break;
 
         case ODPAT_STRIP_VLAN:

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 130d55d..511ec3a 100644 (file)
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -71,26 +71,10 @@ format_odp_action(struct ds *ds, const union odp_action *a)
     case ODPAT_SET_TUNNEL:
         ds_put_format(ds, "set_tunnel(0x%08"PRIx32")", ntohl(a->tunnel.tun_id));
         break;
-    case ODPAT_SET_DL_TCI: {
-        int vid = vlan_tci_to_vid(a->dl_tci.tci);
-        int pcp = vlan_tci_to_pcp(a->dl_tci.tci);
-
-        switch (ntohs(a->dl_tci.mask)) {
-        case VLAN_VID_MASK:
-            ds_put_format(ds, "set_tci(vlan=%d)", vid);
-            break;
-        case VLAN_PCP_MASK:
-            ds_put_format(ds, "set_tci(pcp=%d)", pcp);
-            break;
-        case VLAN_VID_MASK | VLAN_PCP_MASK:
-            ds_put_format(ds, "set_tci(vlan=%d,pcp=%d)", vid, pcp);
-            break;
-        default:
-            ds_put_format(ds, "set_tci(tci=%04"PRIx16",mask=%04"PRIx16")",
-                          ntohs(a->dl_tci.tci), ntohs(a->dl_tci.mask));
-            break;
-        }
-    }
+    case ODPAT_SET_DL_TCI:
+        ds_put_format(ds, "set_tci(vid=%"PRIu16",pcp=%d)",
+                      vlan_tci_to_vid(a->dl_tci.tci),
+                      vlan_tci_to_pcp(a->dl_tci.tci));
         break;
     case ODPAT_STRIP_VLAN:
         ds_put_format(ds, "strip_vlan");

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index d8d3ced..99e5943 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -587,7 +587,25 @@ check_action(const union ofp_action *a, unsigned int len, int max_ports)
         return check_output_port(ntohs(a->output.port), max_ports);
 
     case OFPAT_SET_VLAN_VID:
+        error = check_action_exact_len(a, len, 8);
+        if (error) {
+            return error;
+        }
+        if (a->vlan_vid.vlan_vid & ~htons(0xfff)) {
+            return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT);
+        }
+        return 0;
+
     case OFPAT_SET_VLAN_PCP:
+        error = check_action_exact_len(a, len, 8);
+        if (error) {
+            return error;
+        }
+        if (a->vlan_vid.vlan_vid & ~7) {
+            return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT);
+        }
+        return 0;
+
     case OFPAT_STRIP_VLAN:
     case OFPAT_SET_NW_SRC:
     case OFPAT_SET_NW_DST:

diff --git a/ofproto/ofproto-sflow.c b/ofproto/ofproto-sflow.c
index de9bf89..784e552 100644 (file)
--- a/ofproto/ofproto-sflow.c
+++ b/ofproto/ofproto-sflow.c
@@ -567,6 +567,7 @@ ofproto_sflow_received(struct ofproto_sflow *os, struct odp_msg *msg)
     n_outputs = 0;
     for (i = 0; i < n_actions; i++) {
         const union odp_action *a = &actions[i];
+        uint16_t tci;
 
         switch (a->type) {
         case ODPAT_OUTPUT:
@@ -575,12 +576,9 @@ ofproto_sflow_received(struct ofproto_sflow *os, struct odp_msg *msg)
             break;
 
         case ODPAT_SET_DL_TCI:
-            if (a->dl_tci.mask & htons(VLAN_VID_MASK)) {
-                switchElem.flowType.sw.dst_vlan = vlan_tci_to_vid(a->dl_tci.tci);
-            }
-            if (a->dl_tci.mask & htons(VLAN_PCP_MASK)) {
-                switchElem.flowType.sw.dst_priority = vlan_tci_to_pcp(a->dl_tci.tci);
-            }
+            tci = a->dl_tci.tci;
+            switchElem.flowType.sw.dst_vlan = vlan_tci_to_vid(tci);
+            switchElem.flowType.sw.dst_priority = vlan_tci_to_pcp(tci);
             break;
 
         default:

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bb2bf5a..ab5d476 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2768,16 +2768,15 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
 
         case OFPAT_SET_VLAN_VID:
             oa = odp_actions_add(ctx->out, ODPAT_SET_DL_TCI);
-            oa->dl_tci.tci = ia->vlan_vid.vlan_vid & htons(VLAN_VID_MASK);
-            oa->dl_tci.mask = htons(VLAN_VID_MASK);
+            oa->dl_tci.tci = ia->vlan_vid.vlan_vid;
+            oa->dl_tci.tci |= htons(ctx->flow.dl_vlan_pcp << VLAN_PCP_SHIFT);
             ctx->flow.dl_vlan = ia->vlan_vid.vlan_vid;
             break;
 
         case OFPAT_SET_VLAN_PCP:
             oa = odp_actions_add(ctx->out, ODPAT_SET_DL_TCI);
-            oa->dl_tci.tci = htons((ia->vlan_pcp.vlan_pcp << VLAN_PCP_SHIFT)
-                                   & VLAN_PCP_MASK);
-            oa->dl_tci.mask = htons(VLAN_PCP_MASK);
+            oa->dl_tci.tci = htons(ia->vlan_pcp.vlan_pcp << VLAN_PCP_SHIFT);
+            oa->dl_tci.tci |= ctx->flow.dl_vlan;
             ctx->flow.dl_vlan_pcp = ia->vlan_pcp.vlan_pcp;
             break;
 

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 4a7f90b..75cce3c 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2330,7 +2330,7 @@ compose_actions(struct bridge *br, const struct flow *flow, uint16_t vlan,
             } else {
                 a = odp_actions_add(actions, ODPAT_SET_DL_TCI);
                 a->dl_tci.tci = htons(p->vlan & VLAN_VID_MASK);
-                a->dl_tci.mask = htons(VLAN_VID_MASK);
+                a->dl_tci.tci |= htons(flow->dl_vlan_pcp << VLAN_PCP_SHIFT);
             }
             cur_vlan = p->vlan;
         }