On Thu, Nov 2, 2017 at 6:40 PM, Yang, Yi <yi.y.y...@intel.com> wrote:
> 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.y...@intel.com> 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.y...@intel.com> 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.y...@intel.com>
>> >> > ---
>> >> 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.
>
Following patch looks good to me. But I think we needs similar
eth_type check for nsh set action in validate_set() and in
__ovs_nla_copy_actions() for NSH_POP action.
Can you send patch with all changes?
Thanks.
> 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)",
