Re: [PATCH net-next v15] openvswitch: enable NSH support

["Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Thu, Nov 02, 2017 at 05:06:47AM -0700, Pravin Shelar wrote:
> On Wed, Nov 1, 2017 at 7:50 PM, Yang, Yi <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote:
> >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> >> >
> >> > OVS master and 2.8 branch has merged NSH userspace
> >> > patch series, this patch is to enable NSH support
> >> > in kernel data path in order that OVS can support
> >> > NSH in compat mode by porting this.
> >> >
> >> > Signed-off-by: Yi Yang <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> > ---
> >> I have comment related to checksum, otherwise patch looks good to me.
> >
> > Pravin, thank you for your comments, the below part is incremental patch
> > for checksum, please help check it, I'll send out v16 with this after
> > you confirm.
> >
> This change looks good to me.
> I noticed couple of more issues.
> 1. Can you move the ovs_key_nsh to the union of ipv4 an ipv6?
> ipv4/ipv6/nsh key data is mutually exclusive so there is no need for
> separate space for nsh key in the ovs key.
> 2. We need to fix match_validate() with nsh check. Datapath can not
> allow any l3 or l4 match if the flow key contains nsh match and
> vice-versa. such flow key should be rejected.

Pravin, the below incremental patch should fix the issues you pionted
out, please help confirm/ack, then I'll send out v16 with all acks
from you all for merge. BTW, it has been verified in my sfc test
environment.

diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 8eeae749..c670dd2 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -149,8 +149,8 @@ struct sw_flow_key {
 				} nd;
 			};
 		} ipv6;
+		struct ovs_key_nsh nsh;         /* network service header */
 	};
-	struct ovs_key_nsh nsh;         /* network service header */
 	struct {
 		/* Connection tracking fields not packed above. */
 		struct {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 0d7d4ae..090103c 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -178,7 +178,8 @@ static bool match_validate(const struct sw_flow_match *match,
 			| (1 << OVS_KEY_ATTR_ICMPV6)
 			| (1 << OVS_KEY_ATTR_ARP)
 			| (1 << OVS_KEY_ATTR_ND)
-			| (1 << OVS_KEY_ATTR_MPLS));
+			| (1 << OVS_KEY_ATTR_MPLS)
+			| (1 << OVS_KEY_ATTR_NSH));
 
 	/* Always allowed mask fields. */
 	mask_allowed |= ((1 << OVS_KEY_ATTR_TUNNEL)
@@ -287,6 +288,14 @@ static bool match_validate(const struct sw_flow_match *match,
 		}
 	}
 
+	if (match->key->eth.type == htons(ETH_P_NSH)) {
+		key_expected |= 1 << OVS_KEY_ATTR_NSH;
+		if (match->mask &&
+		    match->mask->key.eth.type == htons(0xffff)) {
+			mask_allowed |= 1 << OVS_KEY_ATTR_NSH;
+		}
+	}
+
 	if ((key_attrs & key_expected) != key_expected) {
 		/* Key attributes check failed. */
 		OVS_NLERR(log, "Missing key (keys=%llx, expected=%llx)",