Re: [ovs-dev] [PATCH v3 5/6] Generic encap and decap support for NSH

2017-08-04 Thread Yang, Yi Y 
Ben, thank you so much for your comments, I have fixed your comments, moved 
some changes in this patch to patch 1, fixed compiling warnings and aligned 
cast errors, new version v4 has been posted out.

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


-Original Message-
From: Ben Pfaff [mailto:b...@ovn.org] 
Sent: Saturday, August 5, 2017 4:39 AM
To: Yang, Yi Y 
Cc: d...@openvswitch.org; Jan Scheurich 
Subject: Re: [PATCH v3 5/6] Generic encap and decap support for NSH

On Thu, Aug 03, 2017 at 09:14:59AM +0800, Yi Yang wrote:
> From: Jan Scheurich 
> 
> This commit adds translation and netdev datapath support for generic 
> encap and decap actions for the NSH MD1 header. The generic encap and 
> decap actions are mapped to specific encap_nsh and decap_nsh actions 
> in the datapath.
> 
> The translation follows that general scheme that decap() of an NSH 
> packet triggers recirculation after decapsulation, while encap(nsh) 
> just modifies struct flow and sets the ctx->pending_encap flag to 
> generate the encap_nsh action at the next commit to be able to include 
> subsequent set_field actions for NSH headers.
> 
> Support for the flexible MD2 format using TLV properties is foreseen 
> in encap(nsh), but not yet fully implemented.
> 
> The CLI syntax for encap of NSH is
> encap(nsh(md_type=1))
> encap(nsh(md_type=2[,tlv(,,),...]))
> 
> Signed-off-by: Jan Scheurich 
> Signed-off-by: Yi Yang 

I don't see the new parts added to openvswitch.h in upstream Linux (even in 
net-next), so I think that all of them should be enclosed in #ifndef __KERNEL__ 
to make that clear.

struct ovs_action_encap_nsh is absurdly large due to the metadata array.
It does not make sense for it to be that big given only MD1 support.
Ideally, struct ovs_action_encap_nsh would be converted into nested Netlink 
attributes; then, the metadata could be variable length.  That is the right way 
to add kernel support.  Before kernel support, though, it would make more sense 
to just use a __be32[4] metadata array.

This patch seems to make a lot of changes that should have been made in 
whatever patch originally added the code.  For example, all the changes to 
format_nsh_key() and format_be32_masked() appear to be in this category.

There are some new compiler warnings or errors:

../ofproto/ofproto-dpif-ipfix.c:2793:17: error: enumeration values 
'OVS_ACTION_ATTR_ENCAP_NSH' and 'OVS_ACTION_ATTR_DECAP_NSH' not explicitly 
handled in switch [-Werror,-Wswitch-enum]
../ofproto/ofproto-dpif-xlate.c:5824:43: error: cast from 'const char *' to 
'struct ofpact_ed_prop *' increases required alignment from 1 to 2 
[-Werror,-Wcast-align]
../lib/odp-util.c:1847:21: error: cast from 'uint8_t *' (aka 'unsigned char 
*') to 'struct nsh_md1_ctx *' increases required alignment from 1 to 4 
[-Werror,-Wcast-align]
../lib/odp-util.c:6818:35: error: cast from 'uint8_t *' (aka 'unsigned char 
*') to 'struct nsh_md1_ctx *' increases required alignment from 1 to 4 
[-Werror,-Wcast-align]

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 5/6] Generic encap and decap support for NSH

2017-08-04 Thread Ben Pfaff 
On Thu, Aug 03, 2017 at 09:14:59AM +0800, Yi Yang wrote:
> From: Jan Scheurich 
> 
> This commit adds translation and netdev datapath support for generic
> encap and decap actions for the NSH MD1 header. The generic encap and
> decap actions are mapped to specific encap_nsh and decap_nsh actions
> in the datapath.
> 
> The translation follows that general scheme that decap() of an NSH
> packet triggers recirculation after decapsulation, while encap(nsh)
> just modifies struct flow and sets the ctx->pending_encap flag to
> generate the encap_nsh action at the next commit to be able to include
> subsequent set_field actions for NSH headers.
> 
> Support for the flexible MD2 format using TLV properties is foreseen
> in encap(nsh), but not yet fully implemented.
> 
> The CLI syntax for encap of NSH is
> encap(nsh(md_type=1))
> encap(nsh(md_type=2[,tlv(,,),...]))
> 
> Signed-off-by: Jan Scheurich 
> Signed-off-by: Yi Yang 

I don't see the new parts added to openvswitch.h in upstream Linux (even
in net-next), so I think that all of them should be enclosed in #ifndef
__KERNEL__ to make that clear.

struct ovs_action_encap_nsh is absurdly large due to the metadata array.
It does not make sense for it to be that big given only MD1 support.
Ideally, struct ovs_action_encap_nsh would be converted into nested
Netlink attributes; then, the metadata could be variable length.  That
is the right way to add kernel support.  Before kernel support, though,
it would make more sense to just use a __be32[4] metadata array.

This patch seems to make a lot of changes that should have been made in
whatever patch originally added the code.  For example, all the changes
to format_nsh_key() and format_be32_masked() appear to be in this
category.

There are some new compiler warnings or errors:

../ofproto/ofproto-dpif-ipfix.c:2793:17: error: enumeration values 
'OVS_ACTION_ATTR_ENCAP_NSH' and 'OVS_ACTION_ATTR_DECAP_NSH' not explicitly 
handled in switch [-Werror,-Wswitch-enum]
../ofproto/ofproto-dpif-xlate.c:5824:43: error: cast from 'const char *' to 
'struct ofpact_ed_prop *' increases required alignment from 1 to 2 
[-Werror,-Wcast-align]
../lib/odp-util.c:1847:21: error: cast from 'uint8_t *' (aka 'unsigned char 
*') to 'struct nsh_md1_ctx *' increases required alignment from 1 to 4 
[-Werror,-Wcast-align]
../lib/odp-util.c:6818:35: error: cast from 'uint8_t *' (aka 'unsigned char 
*') to 'struct nsh_md1_ctx *' increases required alignment from 1 to 4 
[-Werror,-Wcast-align]

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 5/6] Generic encap and decap support for NSH

2017-08-02 Thread Yi Yang 
From: Jan Scheurich 

This commit adds translation and netdev datapath support for generic
encap and decap actions for the NSH MD1 header. The generic encap and
decap actions are mapped to specific encap_nsh and decap_nsh actions
in the datapath.

The translation follows that general scheme that decap() of an NSH
packet triggers recirculation after decapsulation, while encap(nsh)
just modifies struct flow and sets the ctx->pending_encap flag to
generate the encap_nsh action at the next commit to be able to include
subsequent set_field actions for NSH headers.

Support for the flexible MD2 format using TLV properties is foreseen
in encap(nsh), but not yet fully implemented.

The CLI syntax for encap of NSH is
encap(nsh(md_type=1))
encap(nsh(md_type=2[,tlv(,,),...]))

Signed-off-by: Jan Scheurich 
Signed-off-by: Yi Yang 
---
 datapath/linux/compat/include/linux/openvswitch.h |  23 ++
 include/openvswitch/nsh.h |  12 +-
 include/openvswitch/ofp-ed-props.h|  44 +++-
 lib/dpif-netdev.c |   2 +
 lib/dpif.c|   2 +
 lib/match.c   |  30 +--
 lib/odp-execute.c |  36 ++-
 lib/odp-util.c| 278 --
 lib/odp-util.h|   3 +-
 lib/ofp-actions.c |   5 +
 lib/ofp-ed-props.c| 179 ++
 lib/packets.c |  82 +++
 lib/packets.h |   4 +
 ofproto/ofproto-dpif-sflow.c  |   2 +
 ofproto/ofproto-dpif-xlate.c  | 180 +-
 tests/nsh.at  |  12 +-
 16 files changed, 827 insertions(+), 67 deletions(-)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
b/datapath/linux/compat/include/linux/openvswitch.h
index 2381ed2..277c729 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -798,6 +798,25 @@ struct ovs_action_push_eth {
struct ovs_key_ethernet addresses;
 };
 
+#define OVS_ENCAP_NSH_MAX_MD_LEN 256
+/*
+ * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
+ * @flags: NSH header flags.
+ * @mdtype: NSH metadata type.
+ * @mdlen: Length of NSH metadata in bytes.
+ * @np: NSH next_protocol: Inner packet type.
+ * @path_hdr: NSH service path id and service index.
+ * @metadata: NSH metadata for MD type 1 or 2
+ */
+struct ovs_action_encap_nsh {
+uint8_t flags;
+uint8_t mdtype;
+uint8_t mdlen;
+uint8_t np;
+__be32 path_hdr;
+uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
+};
+
 /**
  * enum ovs_nat_attr - Attributes for %OVS_CT_ATTR_NAT.
  *
@@ -873,6 +892,8 @@ enum ovs_nat_attr {
  * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the
  * packet.
  * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the packet.
+ * @OVS_ACTION_ATTR_ENCAP_NSH: encap NSH action to push NSH header.
+ * @OVS_ACTION_ATTR_DECAP_NSH: decap NSH action to remove NSH header.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -908,6 +929,8 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_TRUNC,/* u32 struct ovs_action_trunc. */
OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
OVS_ACTION_ATTR_POP_ETH,  /* No argument. */
+   OVS_ACTION_ATTR_ENCAP_NSH,/* struct ovs_action_encap_nsh. */
+   OVS_ACTION_ATTR_DECAP_NSH,/* No argument. */
 
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
diff --git a/include/openvswitch/nsh.h b/include/openvswitch/nsh.h
index 1593da2..ad89935 100644
--- a/include/openvswitch/nsh.h
+++ b/include/openvswitch/nsh.h
@@ -95,6 +95,8 @@ struct nsh_hdr {
 #define NSH_P_IPV40x01
 #define NSH_P_IPV60x02
 #define NSH_P_ETHERNET0x03
+#define NSH_P_NSH 0x04
+#define NSH_P_MPLS0x05
 
 /* MD Type Registry. */
 #define NSH_M_TYPE1 0x01
@@ -102,8 +104,14 @@ struct nsh_hdr {
 #define NSH_M_EXP1  0xFE
 #define NSH_M_EXP2  0xFF
 
-/* sizeof(struct nsh_hdr) + sizeof(struct nsh_md1_ctx). */
-#define NSH_M_TYPE1_LEN  24
+/* NSH Metadata Length. */
+#define NSH_M_TYPE1_MDLEN 16
+
+/* NSH Base Header Length */
+#define NSH_BASE_HDR_LEN  8
+
+/* NSH MD Type 1 header Length. */
+#define NSH_M_TYPE1_LEN   24
 
 static inline uint16_t
 nsh_hdr_len(const struct nsh_hdr *nsh)
diff --git a/include/openvswitch/ofp-ed-props.h 
b/include/openvswitch/ofp-ed-props.h
index cf2fa62..306c6fe 100644
--- a/include/openvswitch/ofp-ed-props.h
+++ b/include/openvswitch/ofp-ed-props.h
@@ -27,9 +27,10 @@ extern "C" {
 
 enum ofp_ed_prop_class {
 OFPPPC_BASIC = 0,/