From ca287d2062b348b2081e0d44ffcca38e071185c8 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Thu, 24 Oct 2013 13:19:25 -0700 Subject: [PATCH] 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 Signed-off-by: Ben Pfaff --- lib/ofp-actions.c | 94 +++++++++++++++++++++++++++++++----- lib/ofp-actions.h | 20 ++++++-- lib/ofp-parse.c | 12 ++++- lib/ofp-util.def | 4 +- ofproto/ofproto-dpif-xlate.c | 19 +++++--- ofproto/ofproto.c | 2 +- tests/ofp-actions.at | 4 +- 7 files changed, 127 insertions(+), 28 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index cccd6b174..ca26038f1 100644 --- 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; } -/* 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 e097468ba..7be4e92ff 100644 --- 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 757b97168..73a56b807 100644 --- 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 752fd06a3..631a6b10b 100644 --- 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 48d078d8f..f19dbf1cd 100644 --- 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 6ea751028..052c8cd2e 100644 --- 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 0909ce14a..e3fc2bc10 100644 --- 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 -- 2.43.0