From 27bcf966b4057623f7b4d856c0348a1e0eb452e0 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 18 Oct 2010 11:18:10 -0700 Subject: [PATCH] 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 Acked-by: Jesse Gross --- datapath/actions.c | 3 +-- datapath/datapath.c | 8 +------- include/openvswitch/datapath-protocol.h | 8 +++----- lib/dpif-netdev.c | 26 ++++++++----------------- lib/odp-util.c | 24 ++++------------------- lib/ofp-util.c | 18 +++++++++++++++++ ofproto/ofproto-sflow.c | 10 ++++------ ofproto/ofproto.c | 9 ++++----- vswitchd/bridge.c | 2 +- 9 files changed, 44 insertions(+), 64 deletions(-) diff --git a/datapath/actions.c b/datapath/actions.c index 5365278ab..5904c8312 100644 --- 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 9eeb28cd1..b41110dbc 100644 --- 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 1aa8066ca..b0e9dfbb3 100644 --- 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 aa4fdf830..3df6598d2 100644 --- 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 130d55de7..511ec3a99 100644 --- 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 d8d3ced58..99e5943c5 100644 --- 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 de9bf8987..784e5528f 100644 --- 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 bb2bf5a4a..ab5d476b6 100644 --- 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 4a7f90bfa..75cce3c8d 100644 --- 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; } -- 2.43.0