Hi Thomas, I am currently using the beta version of ovs(2.3.90). In my phy-phy scenario , I am configuring the two physical ports(eth0, eth1) attached to ovs bridge as trunk ports using the below commands.
ovs-vsctl --no-wait add-port br0 eth0 vlan_mode=trunk ovs-vsctl --no-wait add-port br0 eth1 vlan_mode=trunk I configured the bridge to work in legacy mode as shown below $ ovs-ofctl dump-flows br0 NXST_FLOW reply (xid=0x4): cookie=0x0, duration=15.458s, table=0, n_packets=0, n_bytes=0, idle_age=15, priority=0 actions=NORMAL I tried sending 802.1ad tagged(outer tag tpid=0x88a8, inter tag tpid=ox8100) packet from packetgen to phyport1 (eth0), and receiving back on phyport2(eth2). When I captured the received packet on eth1 , the received packet is not same as sent packet, means outer vlan TPID is not 0x88a8(instead 0x8100). Could you please let me know , whether the below mentioned patch helps here. Thanks in Advance, Uday -----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Thomas F Herbert Sent: Sunday, July 26, 2015 6:03 AM To: Pravin Shelar Cc: d...@openvswitch.org; netdev; therb...@redhat.com Subject: Re: [ovs-dev] [PATCH net-next 3/3] openvswitch: 802.1AD: Flow handling, actions, and vlan parsing On 6/30/15 12:16 AM, Pravin Shelar wrote: > On Tue, Jun 23, 2015 at 11:26 AM, Thomas F Herbert Pravin, I apologize because I realize now that I am finishing V12 of this patch that I never responded to your comments in this email. > <thomasfherb...@gmail.com> wrote: >> Add support for 802.1ad including the ability to push and pop double >> tagged vlans. Add support for 802.1ad to netlink parsing and flow >> conversion. Uses double nested encap attributes to represent double >> tagged vlan. Inner TPID encoded along with ctci in nested attributes. >> >> Signed-off-by: Thomas F Herbert <thomasfherb...@gmail.com> >> --- >> net/openvswitch/flow.c | 84 +++++++++++++++--- >> net/openvswitch/flow.h | 5 ++ >> net/openvswitch/flow_netlink.c | 195 >> ++++++++++++++++++++++++++++++++++------- >> 3 files changed, 242 insertions(+), 42 deletions(-) >> >> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index >> 2dacc7b..e268865 100644 >> --- a/net/openvswitch/flow.c >> +++ b/net/openvswitch/flow.c >> @@ -298,21 +298,80 @@ static bool icmp6hdr_ok(struct sk_buff *skb) >> static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) >> { >> struct qtag_prefix { >> - __be16 eth_type; /* ETH_P_8021Q */ >> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */ >> __be16 tci; >> }; >> - struct qtag_prefix *qp; >> + struct qtag_prefix *qp = (struct qtag_prefix *)skb->data; >> >> - if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16))) >> + struct qinqtag_prefix { >> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */ >> + __be16 tci; >> + __be16 inner_tpid; /* ETH_P_8021Q */ >> + __be16 ctci; >> + }; >> + >> + if (likely(skb_vlan_tag_present(skb))) { >> + key->eth.tci = htons(skb->vlan_tci); >> + >> + /* Case where upstream >> + * processing has already stripped the outer vlan tag. >> + */ >> + if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) { >> + if (unlikely(skb->len < sizeof(struct qtag_prefix) + >> + sizeof(__be16))) { >> + key->eth.tci = 0; >> + return 0; >> + } >> + >> + if (unlikely(!pskb_may_pull(skb, >> + sizeof(struct >> qtag_prefix) + >> + sizeof(__be16)))) { >> + return -ENOMEM; >> + } >> + >> + if (likely(qp->eth_type == htons(ETH_P_8021Q))) { >> + key->eth.cvlan.ctci = >> + qp->tci | htons(VLAN_TAG_PRESENT); >> + key->eth.cvlan.c_tpid = >> + skb->vlan_proto; > > We should directly copy qp->inner_tpid here. As you have done it for > non offloaded case below. Thanks! It is copied but it is set to the wrong tpid. The c_tpid field in the key should be set to the ethertype in the packet itself which is the inner tpid, not the offloaded skb-vlan_proto which is the outer tpid. Fixed in V12. > >> + __skb_pull(skb, sizeof(struct qtag_prefix)); >> + } >> + } >> return 0; >> + } >> >> - if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) + >> - sizeof(__be16)))) >> - return -ENOMEM; >> >> - qp = (struct qtag_prefix *) skb->data; >> - key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT); >> - __skb_pull(skb, sizeof(struct qtag_prefix)); >> + if (qp->eth_type == htons(ETH_P_8021AD)) { >> + struct qinqtag_prefix *qinqp = >> + (struct qinqtag_prefix >> + *)skb->data; >> + >> + if (unlikely(skb->len < sizeof(struct qinqtag_prefix) + >> + sizeof(__be16))) >> + return 0; >> + >> + if (unlikely(!pskb_may_pull(skb, sizeof(struct >> qinqtag_prefix) + >> + sizeof(__be16)))) { >> + return -ENOMEM; >> + } >> + key->eth.tci = qinqp->tci | htons(VLAN_TAG_PRESENT); >> + key->eth.cvlan.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT); >> + key->eth.cvlan.c_tpid = qinqp->inner_tpid; >> + >> + __skb_pull(skb, sizeof(struct qinqtag_prefix)); >> + >> + return 0; >> + } >> + if (qp->eth_type == htons(ETH_P_8021Q)) { >> + if (unlikely(skb->len < sizeof(struct qtag_prefix) + >> + sizeof(__be16))) >> + return -ENOMEM; >> + >> + if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) + >> + sizeof(__be16)))) >> + return 0; >> + key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT); >> + >> + __skb_pull(skb, sizeof(struct qtag_prefix)); >> + } >> >> return 0; >> } >> @@ -474,9 +533,10 @@ static int key_extract(struct sk_buff *skb, struct >> sw_flow_key *key) >> */ >> >> key->eth.tci = 0; >> - if (skb_vlan_tag_present(skb)) >> - key->eth.tci = htons(skb->vlan_tci); >> - else if (eth->h_proto == htons(ETH_P_8021Q)) >> + key->eth.cvlan.ctci = 0; > cvlan.c_tpid is not initialized for all cases. Fixed in V12 > >> + if ((skb_vlan_tag_present(skb)) || >> + (eth->h_proto == htons(ETH_P_8021Q)) || >> + (eth->h_proto == htons(ETH_P_8021AD))) > These are redundant check. so we can directly call this function. I don't agree that these 3 checks are redundant. parse_vlan parses both the offloaded and non-offloaded cases. In V12, I changed it to call eth_type_vlan() to do the checks for the non-offloaded ethertypes. Hmmm ... maybe I should add another function to if_vlan.h to check if a packet is a vlan regardless of whether it is offloaded or not? > >> if (unlikely(parse_vlan(skb, key))) >> return -ENOMEM; >> > > ... >> >> +static int cust_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs, >> + const struct nlattr **a, bool is_mask, >> + bool log) { >> + /* This should be nested inner or "customer" tci" */ >> + if (attrs & (1 << OVS_KEY_ATTR_VLAN)) { >> + __be16 ctci; >> + >> + ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]); >> + if (!(ctci & htons(VLAN_TAG_PRESENT))) { >> + if (is_mask) >> + OVS_NLERR(log, "VLAN CTCI mask does not have >> exact match for VLAN_TAG_PRESENT bit."); >> + else >> + OVS_NLERR(log, "VLAN CTCI does not >> + have VLAN_TAG_PRESENT bit set."); >> + >> + return -EINVAL; >> + } >> + SW_FLOW_KEY_PUT(match, eth.cvlan.c_tpid, ctci, is_mask); >> + SW_FLOW_KEY_PUT(match, eth.cvlan.ctci, ctci, is_mask); >> + } > Same value is set to tpid and tci. Thanks! Fixed in V12. > >> + return 0; >> +} >> + >> static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs, >> const struct nlattr **a, bool is_mask, >> bool log) @@ -1024,6 +1047,105 @@ >> static void mask_set_nlattr(struct nlattr *attr, u8 val) >> nlattr_set(attr, val, ovs_key_lens); >> } >> >> +static int parse_vlan_from_nlattrs(const struct nlattr *nla, >> + struct sw_flow_match *match, >> + u64 *key_attrs, bool *ie_valid, >> + const struct nlattr **a, bool is_mask, >> + bool log) { >> + int err; >> + __be16 tci; >> + const struct nlattr *encap; >> + >> + if (!is_mask) { >> + u64 v_attrs = 0; >> + >> + tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]); >> + >> + if (tci & htons(VLAN_TAG_PRESENT)) { >> + if >> (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == >> + htons(ETH_P_8021AD)))) { > Since we have added tpid to flow key, we have flexibility of > supporting generic double encapsulation. Therefore in netlink parsing > of key, double encap should not be limited to just 8021AD. for example > it should allow 8021Q in 8021Q header as valid key. I agree. Although Open Flow specification doesn't support non 802.1AD double tagged vlans, we probably should be as least restrictive as possible here in the kernel module. I recoded this in V12 to allow a more "generic" qinq. > >> + err = parse_flow_nlattrs(nla, a, &v_attrs, >> log); >> + if (err) >> + return err; >> + if (!v_attrs) >> + return -EINVAL; >> + _______________________________________________ dev mailing list d...@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
