OF 1.1 set vlan vid/pcp compatibility.

OpenFlow 1.1 set vlan actions only modify existing vlan
headers, while OF 1.0 actions push a new vlan header if one
does not exist already.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index cccd6b1..ca26038 100644 (file)
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -501,6 +501,8 @@ ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
 {
     enum ofputil_action_code code;
     enum ofperr error;
+    struct ofpact_vlan_vid *vlan_vid;
+    struct ofpact_vlan_pcp *vlan_pcp;
 
     error = decode_openflow10_action(a, &code);
     if (error) {
@@ -520,14 +522,20 @@ ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
         if (a->vlan_vid.vlan_vid & ~htons(0xfff)) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_VID(out)->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid = ofpact_put_SET_VLAN_VID(out);
+        vlan_vid->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid->push_vlan_if_needed = true;
+        vlan_vid->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT10_SET_VLAN_PCP:
         if (a->vlan_pcp.vlan_pcp & ~7) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_PCP(out)->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp = ofpact_put_SET_VLAN_PCP(out);
+        vlan_pcp->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp->push_vlan_if_needed = true;
+        vlan_pcp->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT10_STRIP_VLAN:
@@ -774,6 +782,8 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
 {
     enum ofputil_action_code code;
     enum ofperr error;
+    struct ofpact_vlan_vid *vlan_vid;
+    struct ofpact_vlan_pcp *vlan_pcp;
 
     error = decode_openflow11_action(a, &code);
     if (error) {
@@ -793,14 +803,20 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
         if (a->vlan_vid.vlan_vid & ~htons(0xfff)) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_VID(out)->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid = ofpact_put_SET_VLAN_VID(out);
+        vlan_vid->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid->push_vlan_if_needed = false;
+        vlan_vid->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT11_SET_VLAN_PCP:
         if (a->vlan_pcp.vlan_pcp & ~7) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_PCP(out)->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp = ofpact_put_SET_VLAN_PCP(out);
+        vlan_pcp->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp->push_vlan_if_needed = false;
+        vlan_pcp->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT11_PUSH_VLAN:
@@ -1538,9 +1554,12 @@ exit:
     return error;
 }
 \f
-/* May modify flow->dl_type, caller must restore it. */
+/* May modify flow->dl_type and flow->vlan_tci, caller must restore them.
+ *
+ * Modifies some actions, filling in fields that could not be properly set
+ * without context. */
 static enum ofperr
-ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
+ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
                uint8_t table_id, bool enforce_consistency)
 {
     const struct ofpact_enqueue *enqueue;
@@ -1569,13 +1588,37 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
         return bundle_check(ofpact_get_BUNDLE(a), max_ports, flow);
 
     case OFPACT_SET_VLAN_VID:
+        /* Remember if we saw a vlan tag in the flow to aid translating to
+         * OpenFlow 1.1+ if need be. */
+        ofpact_get_SET_VLAN_VID(a)->flow_has_vlan =
+            (flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
+        if (!(flow->vlan_tci & htons(VLAN_CFI)) &&
+            !ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+            goto inconsistent;
+        }
+        /* Temporary mark that we have a vlan tag. */
+        flow->vlan_tci |= htons(VLAN_CFI);
+        return 0;
+
     case OFPACT_SET_VLAN_PCP:
+        /* Remember if we saw a vlan tag in the flow to aid translating to
+         * OpenFlow 1.1+ if need be. */
+        ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan =
+            (flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
+        if (!(flow->vlan_tci & htons(VLAN_CFI)) &&
+            !ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+            goto inconsistent;
+        }
+        /* Temporary mark that we have a vlan tag. */
+        flow->vlan_tci |= htons(VLAN_CFI);
         return 0;
 
     case OFPACT_STRIP_VLAN:
         if (!(flow->vlan_tci & htons(VLAN_CFI))) {
             goto inconsistent;
         }
+        /* Temporary mark that we have no vlan tag. */
+        flow->vlan_tci = htons(0);
         return 0;
 
     case OFPACT_PUSH_VLAN:
@@ -1583,6 +1626,8 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
             /* Multiple VLAN headers not supported. */
             return OFPERR_OFPBAC_BAD_TAG;
         }
+        /* Temporary mark that we have a vlan tag. */
+        flow->vlan_tci |= htons(VLAN_CFI);
         return 0;
 
     case OFPACT_SET_ETH_SRC:
@@ -1713,14 +1758,17 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
  * appropriate for a packet with the prerequisites satisfied by 'flow' in a
  * switch with no more than 'max_ports' ports.
  *
+ * May annotate ofpacts with information gathered from the 'flow'.
+ *
  * May temporarily modify 'flow', but restores the changes before returning. */
 enum ofperr
-ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
+ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
               struct flow *flow, ofp_port_t max_ports, uint8_t table_id,
               bool enforce_consistency)
 {
-    const struct ofpact *a;
+    struct ofpact *a;
     ovs_be16 dl_type = flow->dl_type;
+    ovs_be16 vlan_tci = flow->vlan_tci;
     enum ofperr error = 0;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
@@ -1730,7 +1778,9 @@ ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
             break;
         }
     }
-    flow->dl_type = dl_type; /* Restore. */
+    /* Restore fields that may have been modified. */
+    flow->dl_type = dl_type;
+    flow->vlan_tci = vlan_tci;
     return error;
 }
 
@@ -2114,6 +2164,10 @@ ofpact_to_openflow10(const struct ofpact *a, struct ofpbuf *out)
         break;
 
     case OFPACT_PUSH_VLAN:
+        /* PUSH is a side effect of a SET_VLAN_VID/PCP, which should
+         * follow this action. */
+        break;
+
     case OFPACT_CLEAR_ACTIONS:
     case OFPACT_WRITE_ACTIONS:
     case OFPACT_GOTO_TABLE:
@@ -2206,11 +2260,23 @@ ofpact_to_openflow11(const struct ofpact *a, struct ofpbuf *out)
         break;
 
     case OFPACT_SET_VLAN_VID:
+        /* Push a VLAN tag, if one was not seen at action validation time. */
+        if (!ofpact_get_SET_VLAN_VID(a)->flow_has_vlan
+            && ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+            ofputil_put_OFPAT11_PUSH_VLAN(out)->ethertype
+                = htons(ETH_TYPE_VLAN_8021Q);
+        }
         ofputil_put_OFPAT11_SET_VLAN_VID(out)->vlan_vid
             = htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid);
         break;
 
     case OFPACT_SET_VLAN_PCP:
+        /* Push a VLAN tag, if one was not seen at action validation time. */
+        if (!ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan
+            && ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+            ofputil_put_OFPAT11_PUSH_VLAN(out)->ethertype
+                = htons(ETH_TYPE_VLAN_8021Q);
+        }
         ofputil_put_OFPAT11_SET_VLAN_PCP(out)->vlan_pcp
             = ofpact_get_SET_VLAN_PCP(a)->vlan_pcp;
         break;
@@ -2690,12 +2756,18 @@ ofpact_format(const struct ofpact *a, struct ds *s)
         break;
 
     case OFPACT_SET_VLAN_VID:
-        ds_put_format(s, "mod_vlan_vid:%"PRIu16,
+        ds_put_format(s, "%s:%"PRIu16,
+                      (a->compat == OFPUTIL_OFPAT11_SET_VLAN_VID
+                       ? "set_vlan_vid"
+                       : "mod_vlan_vid"),
                       ofpact_get_SET_VLAN_VID(a)->vlan_vid);
         break;
 
     case OFPACT_SET_VLAN_PCP:
-        ds_put_format(s, "mod_vlan_pcp:%"PRIu8,
+        ds_put_format(s, "%s:%"PRIu8,
+                      (a->compat == OFPUTIL_OFPAT11_SET_VLAN_PCP
+                       ? "set_vlan_pcp"
+                       : "mod_vlan_pcp"),
                       ofpact_get_SET_VLAN_PCP(a)->vlan_pcp);
         break;
 

diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index e097468..7be4e92 100644 (file)
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -257,18 +257,32 @@ struct ofpact_bundle {
 
 /* OFPACT_SET_VLAN_VID.
  *
- * Used for OFPAT10_SET_VLAN_VID. */
+ * We keep track if vlan was present at action validation time to avoid a
+ * PUSH_VLAN when translating to OpenFlow 1.1+.
+ *
+ * We also keep the originating OFPUTIL action code in ofpact.compat.
+ *
+ * Used for OFPAT10_SET_VLAN_VID and OFPAT11_SET_VLAN_VID. */
 struct ofpact_vlan_vid {
     struct ofpact ofpact;
     uint16_t vlan_vid;          /* VLAN VID in low 12 bits, 0 in other bits. */
+    bool push_vlan_if_needed;   /* OF 1.0 semantics if true. */
+    bool flow_has_vlan;         /* VLAN present at action validation time? */
 };
 
 /* OFPACT_SET_VLAN_PCP.
  *
- * Used for OFPAT10_SET_VLAN_PCP. */
+ * We keep track if vlan was present at action validation time to avoid a
+ * PUSH_VLAN when translating to OpenFlow 1.1+.
+ *
+ * We also keep the originating OFPUTIL action code in ofpact.compat.
+ *
+ * Used for OFPAT10_SET_VLAN_PCP and OFPAT11_SET_VLAN_PCP. */
 struct ofpact_vlan_pcp {
     struct ofpact ofpact;
     uint8_t vlan_pcp;           /* VLAN PCP in low 3 bits, 0 in other bits. */
+    bool push_vlan_if_needed;   /* OF 1.0 semantics if true. */
+    bool flow_has_vlan;         /* VLAN present at action validation time? */
 };
 
 /* OFPACT_SET_ETH_SRC, OFPACT_SET_ETH_DST.
@@ -563,7 +577,7 @@ enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
                                                  enum ofp_version version,
                                                  unsigned int instructions_len,
                                                  struct ofpbuf *ofpacts);
-enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
+enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
                           struct flow *, ofp_port_t max_ports,
                           uint8_t table_id, bool enforce_consistency);
 enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 757b971..73a56b8 100644 (file)
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -599,6 +599,8 @@ parse_named_action(enum ofputil_action_code code,
 {
     size_t orig_size = ofpacts->size;
     struct ofpact_tunnel *tunnel;
+    struct ofpact_vlan_vid *vlan_vid;
+    struct ofpact_vlan_pcp *vlan_pcp;
     char *error = NULL;
     uint16_t ethertype = 0;
     uint16_t vid = 0;
@@ -624,7 +626,10 @@ parse_named_action(enum ofputil_action_code code,
         if (vid & ~VLAN_VID_MASK) {
             return xasprintf("%s: not a valid VLAN VID", arg);
         }
-        ofpact_put_SET_VLAN_VID(ofpacts)->vlan_vid = vid;
+        vlan_vid = ofpact_put_SET_VLAN_VID(ofpacts);
+        vlan_vid->vlan_vid = vid;
+        vlan_vid->ofpact.compat = code;
+        vlan_vid->push_vlan_if_needed = code == OFPUTIL_OFPAT10_SET_VLAN_VID;
         break;
 
     case OFPUTIL_OFPAT10_SET_VLAN_PCP:
@@ -637,7 +642,10 @@ parse_named_action(enum ofputil_action_code code,
         if (pcp & ~7) {
             return xasprintf("%s: not a valid VLAN PCP", arg);
         }
-        ofpact_put_SET_VLAN_PCP(ofpacts)->vlan_pcp = pcp;
+        vlan_pcp = ofpact_put_SET_VLAN_PCP(ofpacts);
+        vlan_pcp->vlan_pcp = pcp;
+        vlan_pcp->ofpact.compat = code;
+        vlan_pcp->push_vlan_if_needed = code == OFPUTIL_OFPAT10_SET_VLAN_PCP;
         break;
 
     case OFPUTIL_OFPAT12_SET_FIELD:

diff --git a/lib/ofp-util.def b/lib/ofp-util.def
index 752fd06..631a6b1 100644 (file)
--- a/lib/ofp-util.def
+++ b/lib/ofp-util.def
@@ -20,8 +20,8 @@ OFPAT10_ACTION(OFPAT10_ENQUEUE,      ofp10_action_enqueue,  "enqueue")
 #define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME)
 #endif
 OFPAT11_ACTION(OFPAT11_OUTPUT,       ofp11_action_output, 0, "output")
-OFPAT11_ACTION(OFPAT11_SET_VLAN_VID, ofp_action_vlan_vid, 0, "mod_vlan_vid")
-OFPAT11_ACTION(OFPAT11_SET_VLAN_PCP, ofp_action_vlan_pcp, 0, "mod_vlan_pcp")
+OFPAT11_ACTION(OFPAT11_SET_VLAN_VID, ofp_action_vlan_vid, 0, "set_vlan_vid")
+OFPAT11_ACTION(OFPAT11_SET_VLAN_PCP, ofp_action_vlan_pcp, 0, "set_vlan_pcp")
 OFPAT11_ACTION(OFPAT11_SET_DL_SRC,   ofp_action_dl_addr,  0, "mod_dl_src")
 OFPAT11_ACTION(OFPAT11_SET_DL_DST,   ofp_action_dl_addr,  0, "mod_dl_dst")
 OFPAT11_ACTION(OFPAT11_SET_NW_SRC,   ofp_action_nw_addr,  0, "mod_nw_src")

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 48d078d..f19dbf1 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2320,17 +2320,22 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_VLAN_VID:
             wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
-            flow->vlan_tci &= ~htons(VLAN_VID_MASK);
-            flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
-                               | htons(VLAN_CFI));
+            if (flow->vlan_tci & htons(VLAN_CFI) ||
+                ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+                flow->vlan_tci &= ~htons(VLAN_VID_MASK);
+                flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
+                                   | htons(VLAN_CFI));
+            }
             break;
 
         case OFPACT_SET_VLAN_PCP:
             wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
-            flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
-            flow->vlan_tci |=
-                htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
-                      | VLAN_CFI);
+            if (flow->vlan_tci & htons(VLAN_CFI) ||
+                ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+                flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
+                flow->vlan_tci |= htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp
+                                         << VLAN_PCP_SHIFT) | VLAN_CFI);
+            }
             break;
 
         case OFPACT_STRIP_VLAN:

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 6ea7510..052c8cd 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2835,7 +2835,7 @@ reject_slave_controller(struct ofconn *ofconn)
  */
 static enum ofperr
 ofproto_check_ofpacts(struct ofproto *ofproto,
-                      const struct ofpact ofpacts[], size_t ofpacts_len,
+                      struct ofpact ofpacts[], size_t ofpacts_len,
                       struct flow *flow, uint8_t table_id,
                       const struct ofp_header *oh)
 {

diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 0909ce1..e3fc2bc 100644 (file)
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -138,10 +138,10 @@ AT_DATA([test-data], [dnl
 # actions=CONTROLLER:1234
 0000 0010 fffffffd 04d2 000000000000
 
-# actions=mod_vlan_vid:9
+# actions=set_vlan_vid:9
 0001 0008 0009 0000
 
-# actions=mod_vlan_pcp:6
+# actions=set_vlan_pcp:6
 0002 0008 06 000000
 
 # actions=mod_dl_src:00:11:22:33:44:55