On Thu, Aug 03, 2017 at 09:14:55AM +0800, Yi Yang wrote:
> From: Jan Scheurich <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Yi Yang <[email protected]>
> Signed-off-by: Jan Scheurich <[email protected]>
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--
diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 7b6151f3384b..184b75e36bef 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -67,9 +67,9 @@ PREREQS = {"none": "MFP_NONE",
# If a name matches more than one prefix, the longest one is used.
OXM_CLASSES = {"NXM_OF_": (0, 0x0000),
"NXM_NX_": (0, 0x0001),
+ "NXOXM_NSH_": (0x005ad650, 0xffff),
"OXM_OF_": (0, 0x8000),
"OXM_OF_PKT_REG": (0, 0x8001),
- "OXM_NSH_": (0, 0x8004),
"ONFOXM_ET_": (0x4f4e4600, 0xffff),
# This is the experimenter OXM class for Nicira, which is the
diff --git a/datapath/linux/compat/include/linux/openvswitch.h
b/datapath/linux/compat/include/linux/openvswitch.h
index 2381ed2c8c3c..a98ecc0ebedc 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -360,9 +360,6 @@ enum ovs_key_attr {
OVS_KEY_ATTR_CT_LABELS, /* 16-octet connection tracking labels */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */
-#ifndef __KERNEL__
- OVS_KEY_ATTR_NSH, /* struct ovs_key_nsh */
-#endif
#ifdef __KERNEL__
/* Only used within kernel data path. */
@@ -372,6 +369,7 @@ enum ovs_key_attr {
#ifndef __KERNEL__
/* Only used within userspace data path. */
OVS_KEY_ATTR_PACKET_TYPE, /* be32 packet type */
+ OVS_KEY_ATTR_NSH, /* struct ovs_key_nsh */
#endif
__OVS_KEY_ATTR_MAX
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 6192898f76e3..27c97b38288c 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -1745,7 +1745,7 @@ enum OVS_PACKED_ENUM mf_field_id {
/* "nsh_flags".
*
- * flags field in NSH base header (8 bits).
+ * flags field in NSH base header.
*
* Type: u8.
* Maskable: bitwise.
@@ -1753,13 +1753,13 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_FLAGS(1) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_FLAGS(1) since OF1.3 and v2.8.
*/
MFF_NSH_FLAGS,
/* "nsh_mdtype".
*
- * mdtype field in NSH base header (8 bits).
+ * mdtype field in NSH base header.
*
* Type: u8.
* Maskable: no.
@@ -1767,13 +1767,13 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read-only.
* NXM: none.
- * OXM: OXM_NSH_MDTYPE(2) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_MDTYPE(2) since OF1.3 and v2.8.
*/
MFF_NSH_MDTYPE,
/* "nsh_np".
*
- * np (next protocol) field in NSH base header (8 bits)
+ * np (next protocol) field in NSH base header
*
* Type: u8.
* Maskable: no.
@@ -1781,28 +1781,27 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read-only.
* NXM: none.
- * OXM: OXM_NSH_NP(3) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_NP(3) since OF1.3 and v2.8.
*/
MFF_NSH_NP,
/* "nsh_spi" (aka "nsp").
*
- * spi (service path identifier) field in NSH base
- * header (24 bits).
+ * spi (service path identifier) field in NSH base header.
*
- * Type: be32.
+ * Type: be32 (low 24 bits).
* Maskable: no.
* Formatting: hexadecimal.
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_SPI(4) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_SPI(4) since OF1.3 and v2.8.
*/
MFF_NSH_SPI,
/* "nsh_si" (aka "nsi").
*
- * si (service index) field in NSH base header (8 bits).
+ * si (service index) field in NSH base header.
*
* Type: u8.
* Maskable: no.
@@ -1810,13 +1809,13 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_SI(5) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_SI(5) since OF1.3 and v2.8.
*/
MFF_NSH_SI,
/* "nsh_c1" (aka "nshc1").
*
- * c1 (Network Platform Context) field in NSH context header (32 bits)
+ * c1 (Network Platform Context) field in NSH context header.
*
* Type: be32.
* Maskable: bitwise.
@@ -1824,13 +1823,13 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_C1(6) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_C1(6) since OF1.3 and v2.8.
*/
MFF_NSH_C1,
/* "nsh_c2" (aka "nshc2").
*
- * c2 (Network Shared Context) field in NSH context header (32 bits)
+ * c2 (Network Shared Context) field in NSH context header.
*
* Type: be32.
* Maskable: bitwise.
@@ -1838,13 +1837,13 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_C2(7) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_C2(7) since OF1.3 and v2.8.
*/
MFF_NSH_C2,
/* "nsh_c3" (aka "nshc3").
*
- * c3 (Service Platform Context) field in NSH context header (32 bits)
+ * c3 (Service Platform Context) field in NSH context header.
*
* Type: be32.
* Maskable: bitwise.
@@ -1852,13 +1851,13 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_C3(8) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_C3(8) since OF1.3 and v2.8.
*/
MFF_NSH_C3,
/* "nsh_c4" (aka "nshc4").
*
- * c4 (Service Shared Context) field in NSH context header (32 bits)
+ * c4 (Service Shared Context) field in NSH context header.
*
* Type: be32.
* Maskable: bitwise.
@@ -1866,7 +1865,7 @@ enum OVS_PACKED_ENUM mf_field_id {
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
- * OXM: OXM_NSH_C4(9) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_C4(9) since OF1.3 and v2.8.
*/
MFF_NSH_C4,
diff --git a/lib/match.c b/lib/match.c
index f287df1be5d2..6968dfe17ecd 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -1253,34 +1253,15 @@ format_ct_label_masked(struct ds *s, const ovs_u128
*key, const ovs_u128 *mask)
static void
format_nsh_masked(struct ds *s, const struct flow *f, const struct flow *m)
{
- if (m->nsh.flags) {
- format_uint8_masked(s, "nsh_flags", f->nsh.flags, m->nsh.flags);
- }
- if (m->nsh.mdtype) {
- format_uint8_masked(s, "nsh_mdtype", f->nsh.mdtype, m->nsh.mdtype);
- }
- if (m->nsh.np) {
- format_uint8_masked(s, "nsh_np", f->nsh.np, m->nsh.np);
- }
- if (m->nsh.spi) {
- format_be32_masked(s, "nsh_spi", f->nsh.spi, m->nsh.spi);
- }
- if (m->nsh.si) {
- format_uint8_masked(s, "nsh_si", f->nsh.si, m->nsh.si);
- }
-
- if (m->nsh.c1) {
- format_be32_masked_hex(s, "nsh_c1", f->nsh.c1, m->nsh.c1);
- }
- if (m->nsh.c2) {
- format_be32_masked_hex(s, "nsh_c2", f->nsh.c2, m->nsh.c2);
- }
- if (m->nsh.c3) {
- format_be32_masked_hex(s, "nsh_c3", f->nsh.c3, m->nsh.c3);
- }
- if (m->nsh.c4) {
- format_be32_masked_hex(s, "nsh_c4", f->nsh.c4, m->nsh.c4);
- }
+ format_uint8_masked(s, "nsh_flags", f->nsh.flags, m->nsh.flags);
+ format_uint8_masked(s, "nsh_mdtype", f->nsh.mdtype, m->nsh.mdtype);
+ format_uint8_masked(s, "nsh_np", f->nsh.np, m->nsh.np);
+ format_be32_masked(s, "nsh_spi", f->nsh.spi, m->nsh.spi);
+ format_uint8_masked(s, "nsh_si", f->nsh.si, m->nsh.si);
+ format_be32_masked_hex(s, "nsh_c1", f->nsh.c1, m->nsh.c1);
+ format_be32_masked_hex(s, "nsh_c2", f->nsh.c2, m->nsh.c2);
+ format_be32_masked_hex(s, "nsh_c3", f->nsh.c3, m->nsh.c3);
+ format_be32_masked_hex(s, "nsh_c4", f->nsh.c4, m->nsh.c4);
}
/* Appends a string representation of 'match' to 's'. If 'priority' is
diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
index b634c517242b..83fe421f0c40 100644
--- a/lib/meta-flow.xml
+++ b/lib/meta-flow.xml
@@ -689,11 +689,7 @@ tcp,tp_src=0x07c0/0xfff0
using the first 32 bits of the body as an <code>experimenter</code> field
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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
