[ovs-dev,v3,1/6] userspace: Add support for NSH MD1 match fields

From: Ben Pfaff [mailto:blp@ovn.org]

Ben, thank you so much for your comments and incremental, I have fixed your comments and merged your incremental patch, new version v4 has been posted.

https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336864.html


-----Original Message-----
From: Ben Pfaff [mailto:blp@ovn.org] 
Sent: Saturday, August 5, 2017 2:19 AM
To: Yang, Yi Y <yi.y.yang@intel.com>
Cc: dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com>
Subject: Re: [PATCH v3 1/6] userspace: Add support for NSH MD1 match fields

On Thu, Aug 03, 2017 at 09:14:55AM +0800, Yi Yang wrote:
> From: Jan Scheurich <jan.scheurich@ericsson.com>
> 
> This patch adds support for NSH packet header fields to the OVS 
> control plane and the userspace datapath. Initially we support the 
> fields of the NSH base header as defined in 
> https://www.ietf.org/id/draft-ietf-sfc-nsh-13.txt
> and the fixed context headers specified for metadata format MD1.
> The variable length MD2 format is parsed but the TLV context headers 
> are not yet available for matching.
> 
> The NSH fields are modelled as OXM fields with the dedicated OXM class 
> 0x8004 proposed for NSH in ONF. The following fields are defined:
> 
> OXM code            ofctl name    Size      Comment
> =====================================================================
> OXM_NSH_FLAGS       nsh_flags       8       Bits 2-9 of 1st NSH word
> (0x8004,1)
> OXM_NSH_MDTYPE      nsh_mdtype      8       Bits 16-23
> (0x8004,2)
> OXM_NSH_NEXTPROTO   nsh_np          8       Bits 24-31
> (0x8004,3)
> OXM_NSH_SPI         nsh_spi         24      Bits 0-23 of 2nd NSH word
> (0x8004,4)
> OXM_NSH_SI          nsh_si          8       Bits 24-31
> (0x8004,5)
> OXM_NSH_C1          nsh_c1          32      Maskable, nsh_mdtype==1
> (0x8004,6)
> OXM_NSH_C2          nsh_c2          32      Maskable, nsh_mdtype==1
> (0x8004,7)
> OXM_NSH_C3          nsh_c3          32      Maskable, nsh_mdtype==1
> (0x8004,8)
> OXM_NSH_C4          nsh_c4          32      Maskable, nsh_mdtype==1
> (0x8004,9)
> 
> Co-authored-by: Johnson Li <johnson.li@intel.com>
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>

Thanks for working on this!

I do not think that we should use the ONF-reserved class 0x8004.
Although some drafts assign 0x8004 for NSH use, I do not think that any of them have been officially approved.  So, I suggest that we use a randomly chosen OUI to form an experimenter class.

The documentation in meta-flow.xml is incomplete.  Please expand it to include the kinds of details that we see in other sections of the document.

Please add OVS_KEY_ATTR_NSH to the existing #ifndef __KERNEL__ block.

In meta-flow.xml, please indicate the 24-bit size of nsh_spi as part of the type.  Also, it's not necessary to redundantly include the size in a comment.

I find the NSH version field confusing.  The diagram shows the version as the most significant 2 bits of the first 16-bit field of the header, and NSH_VER_MASK is a 2-bit mask, but NSH_VER_SHIFT is defined as 12.
Why isn't it 14?

I don't see anything in parse_nsh() that verifies that the packet's length is long enough for the NSH header length.  Probably, it needs to check before trying to access the length field, and then again when to verify that it's long enough for the encoded length.

In format_nsh_masked(), it is redundant to check the masks before each call, because the format_*_masked() functions check them too.

In mf_is_value_valid(), I think we should verify that the high 8 bits of MFF_NSH_SPI are zero, since it is a 24-bit field.

I'm appending a suggested incremental that addresses many of the issues I noted above.

Thanks,

Ben.

--8<--------------------------cut here-------------------------->8--

     whose most significant byte is zero and whose remaining bytes are an
     Organizationally Unique Identifier (OUI) assigned by the IEEE [IEEE OUI],
-    as shown below.  OpenFlow says that support for experimenter fields is
-    optional.  Open vSwitch 2.4 and later does support them, primarily so that
-    it can support the <code>ONFOXM_ET_</code>* code points defined by official
-    Open Networking Foundation extensions to OpenFlow 1.3 in e.g. [TCP Flags
-    Match Field Extension].
+    as shown below.
   </p>
 
   <diagram>
@@ -717,6 +713,26 @@ tcp,tp_src=0x07c0/0xfff0
   </diagram>
 
   <p>
+    OpenFlow says that support for experimenter fields is optional.  Open
+    vSwitch 2.4 and later does support them, so that it can support the
+    following experimenter classes:
+  </p>
+
+  <dl>
+    <dt>0x4f4e4600 (<code>ONFOXM_ET</code>)</dt>
+    <dd>
+      Used by official Open Networking Foundation extensions to OpenFlow 1.3 in
+      e.g. [TCP Flags Match Field Extension].
+    </dd>
+
+    <dt>0x005ad650 (<code>NXOXM_NSH</code>)</dt>
+    <dd>
+      Used by Open vSwitch for NSH extensions, in the absence of an official
+      ONF-assigned class.  (This OUI is randomly generated.)
+    </dd>
+  </dl>
+
+  <p>
     Taken as a unit, <code>class</code> (or <code>vendor</code>),
     <code>field</code>, and <code>experimenter</code> (when present) uniquely
     identify a particular field.


[ovs-dev,v3,1/6] userspace: Add support for NSH MD1 match fields

Commit Message

Patch