diff mbox

[ovs-dev] datapath-windows: Support for OVS_KEY_ATTR_MPLS attribute

Message ID 1444253688-12089-1-git-send-email-svinturis@cloudbasesolutions.com
State Changes Requested
Headers show

Commit Message

Sorin Vinturis Nov. 11, 2015, 8:11 p.m. UTC 
This patch adds OVS_KEY_ATTR_MPLS to the OVS flow mechanism.

Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
---
 datapath-windows/ovsext/Actions.c      | 176 +++++++++++++++++++++++++++++++++
 datapath-windows/ovsext/DpInternal.h   |   7 ++
 datapath-windows/ovsext/Ethernet.h     |   2 +
 datapath-windows/ovsext/Flow.c         |  89 ++++++++++++++++-
 datapath-windows/ovsext/NetProto.h     |  33 +++++++
 datapath-windows/ovsext/PacketParser.c |  12 +--
 datapath-windows/ovsext/PacketParser.h |   7 ++
 7 files changed, 318 insertions(+), 8 deletions(-)

Comments

Sairam Venugopal Nov. 18, 2015, 10:34 p.m. UTC | #1 
Hey Sorin,

Were you able to test out the changes? Can you clarify how we can
reproduce it? It will be nice to have documentation around testing this
since it¹s a new feature. I remember you having difficulties with testing
this earlier, hopefully that is now resolved.

I took a look at your changes and have mentioned my comments inline.

Thanks,
Sairam

On 11/11/15, 12:11 PM, "Sorin Vinturis" <svinturis@cloudbasesolutions.com>
wrote:

>This patch adds OVS_KEY_ATTR_MPLS to the OVS flow mechanism.
>
>Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
>---
> datapath-windows/ovsext/Actions.c      | 176
>+++++++++++++++++++++++++++++++++
> datapath-windows/ovsext/DpInternal.h   |   7 ++
> datapath-windows/ovsext/Ethernet.h     |   2 +
> datapath-windows/ovsext/Flow.c         |  89 ++++++++++++++++-
> datapath-windows/ovsext/NetProto.h     |  33 +++++++
> datapath-windows/ovsext/PacketParser.c |  12 +--
> datapath-windows/ovsext/PacketParser.h |   7 ++
> 7 files changed, 318 insertions(+), 8 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index ce592b3..9ee1763 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1074,6 +1074,142 @@ OvsPopVlanInPktBuf(OvsForwardingContext
>*ovsFwdCtx)
>     return NDIS_STATUS_SUCCESS;
> }

I think we should move the MPLS-Push and POP functions to a separate file
mpls.h/mpls.c
to keep Actions.c limited to what it does.

> 
>+static __inline NDIS_STATUS
>+OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
>+                  const struct ovs_action_push_mpls *mpls)
>+{
>+    NDIS_STATUS status;
>+    PNET_BUFFER curNb = NULL;
>+    PMDL curMdl = NULL;
>+    PUINT8 bufferStart = NULL;
>+    OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+    EthHdr *ethHdr = NULL;
>+    MPLSHdr *mplsHdr = NULL;
>+    UINT32 mdlLen = 0, curMdlOffset = 0;
>+    UINT32 packetLen = 0;
>+    PNET_BUFFER_LIST newNbl;
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    ASSERT(mdlLen > 0);
>+    /* Bail out if the L2 header is not in a contiguous buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof *ethHdr) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    ASSERT((INT)mdlLen >= 0);
>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               MPLS_HLEN, 0, TRUE /*copy NBL info*/);
>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                L"Complete after partial copy.");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"OVS-Dropped due to resources");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!curMdl) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    ASSERT(mdlLen >= MPLS_HLEN);
>+
>+    ethHdr = (EthHdr *)(bufferStart + curMdlOffset);
>+    ethHdr->Type = mpls->mpls_ethertype;
>+
>+    mplsHdr = (MPLSHdr *)(bufferStart + curMdlOffset + MPLS_HLEN);
>+    mplsHdr->mpls_lse = mpls->mpls_lse;
>+
>+    layers->l3Offset += MPLS_HLEN;
>+    layers->isIPv4 = 0;
>+    layers->isIPv6 = 0;
>+

Should we introduce an isMpls label in OVS_PACKET_HDR_INFO to identify
this?

>+    NdisRetreatNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+

This function seems like a copy of OvsPopVlanInPktBuf(OvsForwardingContext
*ovsFwdCtx)
except for the shiftLength/MPLS_HLEN. You can have 1 function that takes
in the length instead.

>+static __inline NDIS_STATUS
>+OvsActionMplsPop(OvsForwardingContext *ovsFwdCtx)
>+{
>+    PNET_BUFFER curNb;
>+    PMDL curMdl;
>+    PUINT8 bufferStart;
>+    ULONG dataLength = sizeof (DL_EUI48) + sizeof (DL_EUI48);
>+    UINT32 packetLen, mdlLen;
>+    PNET_BUFFER_LIST newNbl;
>+    NDIS_STATUS status;
>+
>+    PUINT8 tempBuffer[sizeof (DL_EUI48) + sizeof (DL_EUI48)];
>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               0, 0, TRUE /* copy NBL info */);
>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    /* Complete the original NBL and create a copy to modify. */
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx, L"OVS-Dropped due to copy");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"Dropped due to resouces");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    /* Bail out if L2 + MPLS header is not contiguous in the first
>buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof(EthHdr) + MPLS_HLEN) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    RtlCopyMemory(tempBuffer, bufferStart, dataLength);
>+    RtlCopyMemory(bufferStart + MPLS_HLEN, tempBuffer, dataLength);
>+    NdisAdvanceNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
> /*
>  * 
>--------------------------------------------------------------------------
>  * OvsTunnelAttrToIPv4TunnelKey --
>@@ -1513,6 +1649,46 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT
>switchContext,
>             break;
>         }
> 
>+        case OVS_ACTION_ATTR_PUSH_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPush(&ovsFwdCtx,
>+                                       (struct ovs_action_push_mpls
>*)NlAttrGet
>+                                       ((const PNL_ATTR)a));
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set push MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>+        case OVS_ACTION_ATTR_POP_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPop(&ovsFwdCtx);
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set pop MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>         case OVS_ACTION_ATTR_USERSPACE:
>         {
>             PNL_ATTR userdataAttr;
>diff --git a/datapath-windows/ovsext/DpInternal.h
>b/datapath-windows/ovsext/DpInternal.h
>index 8de48a2..c195494 100644
>--- a/datapath-windows/ovsext/DpInternal.h
>+++ b/datapath-windows/ovsext/DpInternal.h
>@@ -20,6 +20,7 @@
> #include <netioapi.h>
> #define IFNAMSIZ IF_NAMESIZE
> #include "../ovsext/Netlink/Netlink.h"
>+#include "NetProto.h"
> 
> #define OVS_DP_NUMBER   ((uint32_t) 0)
> 
>@@ -149,6 +150,11 @@ typedef union OvsIPv4TunnelKey {
>     uint64_t attr[NUM_PKT_ATTR_REQUIRED];
> } OvsIPv4TunnelKey;
> 
>+typedef struct MplsKey {
>+    ovs_be32 top_lse;            /* MPLS topmost label stack entry. */
>+    uint8    pad[4];
>+} MplsKey; /* Size of 8 bytes. */
>+
> typedef __declspec(align(8)) struct OvsFlowKey {
>     OvsIPv4TunnelKey tunKey;     /* 24 bytes */
>     L2Key l2;                    /* 24 bytes */
>@@ -157,6 +163,7 @@ typedef __declspec(align(8)) struct OvsFlowKey {
>         ArpKey arpKey;           /* size 24 */
>         Ipv6Key ipv6Key;         /* size 48 */
>         Icmp6Key icmp6Key;       /* size 72 */
>+        MplsKey mplsKey;         /* size 8 */
>     };
> } OvsFlowKey;
> 
>diff --git a/datapath-windows/ovsext/Ethernet.h
>b/datapath-windows/ovsext/Ethernet.h
>index 22aa27c..1d69d47 100644
>--- a/datapath-windows/ovsext/Ethernet.h
>+++ b/datapath-windows/ovsext/Ethernet.h
>@@ -66,6 +66,8 @@ typedef enum {
>     ETH_TYPE_CDP         = 0x2000,
>     ETH_TYPE_802_1PQ     = 0x8100, // not really a DIX type, but used as
>such
>     ETH_TYPE_LLC         = 0xFFFF, // 0xFFFF is IANA reserved, used to
>mark LLC
>+    ETH_TYPE_MPLS        = 0x8847,
>+    ETH_TYPE_MPLS_MCAST  = 0x8848,
> } Eth_DixType;
> 
> typedef enum {
>diff --git a/datapath-windows/ovsext/Flow.c
>b/datapath-windows/ovsext/Flow.c
>index b629c93..c989c14 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -80,6 +80,8 @@ static NTSTATUS _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf,
>                                        Icmp6Key *ipv6FlowPutIcmpKey);
> static NTSTATUS _MapFlowArpKeyToNlKey(PNL_BUFFER nlBuf,
>                                       ArpKey *arpFlowPutKey);
>+static NTSTATUS _MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf,
>+                                       MplsKey *mplsFlowPutKey);
> 
> static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
>                                OvsFlowDumpOutput *dumpOutput,
>@@ -108,7 +110,7 @@ const NL_POLICY nlFlowPolicy[] = {
> 
> /* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
>  * Some of the attributes like OVS_KEY_ATTR_RECIRC_ID
>- * & OVS_KEY_ATTR_MPLS are not supported yet. */
>+ * are not supported yet. */
> 
> const NL_POLICY nlFlowKeyPolicy[] = {
>     [OVS_KEY_ATTR_ENCAP] = {.type = NL_A_VAR_LEN, .optional = TRUE},
>@@ -872,6 +874,13 @@ MapFlowKeyToNlKey(PNL_BUFFER nlBuf,
>         break;
>         }
> 
>+        case ETH_TYPE_MPLS:
>+        case ETH_TYPE_MPLS_MCAST: {
>+        MplsKey *mplsFlowPutKey = &(flowKey->mplsKey);
>+        rc = _MapFlowMplsKeyToNlKey(nlBuf, mplsFlowPutKey);
>+        break;
>+        }
>+
>         default:
>         break;
>     }
>@@ -1194,6 +1203,31 @@ done:
> 
> /*
>  
>*-------------------------------------------------------------------------
>---
>+ *  _MapFlowMplsKeyToNlKey --
>+ *    Maps _MapFlowMplsKeyToNlKey to OVS_KEY_ATTR_MPLS attribute.
>+ 
>*-------------------------------------------------------------------------
>---
>+ */
>+static NTSTATUS
>+_MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf, MplsKey *mplsFlowPutKey)
>+{
>+    NTSTATUS rc = STATUS_SUCCESS;
>+    struct ovs_key_mpls *mplsKey;
>+
>+    mplsKey = (struct ovs_key_mpls *)
>+        NlMsgPutTailUnspecUninit(nlBuf, OVS_KEY_ATTR_MPLS,
>sizeof(*mplsKey));
>+    if (!mplsKey) {
>+        rc = STATUS_UNSUCCESSFUL;
>+        goto done;
>+    }
>+
>+    mplsKey->mpls_lse = mplsFlowPutKey->top_lse;
>+
>+done:
>+    return rc;
>+}
>+
>+/*
>+ 
>*-------------------------------------------------------------------------
>---
>  *  _MapNlToFlowPut --
>  *    Maps input netlink message to OvsFlowPut.
>  
>*-------------------------------------------------------------------------
>---
>@@ -1469,8 +1503,28 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
>             arpFlowPutKey->pad[1] = 0;
>             arpFlowPutKey->pad[2] = 0;
>             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
>-            break;
>         }
>+        break;
>+    }
>+    case ETH_TYPE_MPLS:
>+    case ETH_TYPE_MPLS_MCAST: {
>+
>+        if (keyAttrs[OVS_KEY_ATTR_MPLS]) {
>+            MplsKey *mplsFlowPutKey = &destKey->mplsKey;
>+            const struct ovs_key_mpls *mplsKey;
>+            UINT32 size = NlAttrGetSize(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+            UINT32 n = size / sizeof(struct ovs_key_mpls);
>+
>+            mplsKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+
>+            mplsFlowPutKey->top_lse = mplsKey->mpls_lse;
>+            mplsFlowPutKey->pad[0] = 0;
>+            mplsFlowPutKey->pad[1] = 0;
>+            mplsFlowPutKey->pad[2] = 0;
>+            mplsFlowPutKey->pad[3] = 0;
>+            destKey->l2.keyLen += (UINT16)n * sizeof(MplsKey);
>+        }
>+        break;
>     }
>     }
> }
>@@ -1864,6 +1918,37 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,
>                 memcpy(arpKey->arpTha, arp->arp_tha, ETH_ADDR_LENGTH);
>             }
>         }
>+    } else if (flow->l2.dlType == htons(ETH_TYPE_MPLS) ||
>+               flow->l2.dlType == htons(ETH_TYPE_MPLS_MCAST)) {
>+        MPLSHdr mplsStorage;
>+        const MPLSHdr *mpls;
>+        MplsKey *mplsKey = &flow->mplsKey;
>+        ((UINT64 *)mplsKey)[0] = 0;
>+
>+        /* In the presence of an MPLS label stack the end of the L2
>+         * header and the beginning of the L3 header differ.
>+         *
>+         * A network packet may contain multiple MPLS labels, but we
>+         * are only interested in the topmost label stack entry.
>+         *
>+         * Advance network header to the beginning of the L3 header.
>+         * layers->l3Offset corresponds to the end of the L2 header.
>+         */
>+        for (UINT32 i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
>+            mpls = OvsGetMpls(packet, layers->l3Offset, &mplsStorage);
>+            if (mpls) {
>+
>+                /* Keep only the topmost MPLS label stack entry. */
>+                if (i == 0) {
>+                    mplsKey->top_lse = mpls->mpls_lse;
>+                }
>+
>+                layers->l3Offset += sizeof(MPLSHdr);
>+
>+                if (mpls->mpls_lse & htonl(MPLS_LS_S_MASK))
>+                    break;
>+            }
>+        }
>     }
> 
>     return NDIS_STATUS_SUCCESS;
>diff --git a/datapath-windows/ovsext/NetProto.h
>b/datapath-windows/ovsext/NetProto.h
>index a364869..12e8bd9 100644
>--- a/datapath-windows/ovsext/NetProto.h
>+++ b/datapath-windows/ovsext/NetProto.h
>@@ -366,4 +366,37 @@ typedef struct IPOpt {
> #define SOCKET_IPPROTO_UDP   17
> #define SOCKET_IPPROTO_GRE   47
> 
>+/* Reference: RFC 5462, RFC 3032
>+ *
>+ *  0                   1                   2                   3
>+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ * |                Label                  | TC  |S|       TTL     |
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ *
>+ *	Label:  Label Value, 20 bits
>+ *	TC:     Traffic Class field, 3 bits
>+ *	S:      Bottom of Stack, 1 bit
>+ *	TTL:    Time to Live, 8 bits
>+ */
>+

Can¹t you simply define the structure with the respective bits that are
used instead of the mask and shift?
You can name it lse instead of redundant mpls_lse

>+typedef struct MPLSHdr {
>+    ovs_be32 mpls_lse;
>+} MPLSHdr;
>+
>+/*
>+ * MPLS definitions
>+ */
>+#define MPLS_HLEN               4
>+#define FLOW_MAX_MPLS_LABELS    3
>+#define MPLS_LS_LABEL_MASK      0xFFFFF000
>+#define MPLS_LS_LABEL_SHIFT     12
>+#define MPLS_LS_TC_MASK         0x00000E00
>+#define MPLS_LS_TC_SHIFT        9
>+#define MPLS_LS_S_MASK          0x00000100
>+#define MPLS_LS_S_SHIFT         8
>+#define MPLS_LS_TTL_MASK        0x000000FF
>+#define MPLS_LS_TTL_SHIFT       0
>+
>+
> #endif /* __NET_PROTO_H_ */
>diff --git a/datapath-windows/ovsext/PacketParser.c
>b/datapath-windows/ovsext/PacketParser.c
>index e01be17..246c603 100644
>--- a/datapath-windows/ovsext/PacketParser.c
>+++ b/datapath-windows/ovsext/PacketParser.c
>@@ -84,8 +84,8 @@ OvsGetPacketBytes(const NET_BUFFER_LIST *nbl,
> 
> NDIS_STATUS
> OvsParseIPv6(const NET_BUFFER_LIST *packet,
>-          OvsFlowKey *key,
>-          POVS_PACKET_HDR_INFO layers)
>+             OvsFlowKey *key,
>+             POVS_PACKET_HDR_INFO layers)
> {
>     UINT16 ofs = layers->l3Offset;
>     IPv6Hdr ipv6HdrStorage;
>@@ -178,8 +178,8 @@ OvsParseIPv6(const NET_BUFFER_LIST *packet,
> 
> VOID
> OvsParseTcp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     TCPHdr tcpStorage;
>     const TCPHdr *tcp = OvsGetTcp(packet, layers->l4Offset, &tcpStorage);
>@@ -193,8 +193,8 @@ OvsParseTcp(const NET_BUFFER_LIST *packet,
> 
> VOID
> OvsParseUdp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     UDPHdr udpStorage;
>     const UDPHdr *udp = OvsGetUdp(packet, layers->l4Offset, &udpStorage);
>diff --git a/datapath-windows/ovsext/PacketParser.h
>b/datapath-windows/ovsext/PacketParser.h
>index 55d110f..96136b7 100644
>--- a/datapath-windows/ovsext/PacketParser.h
>+++ b/datapath-windows/ovsext/PacketParser.h
>@@ -141,4 +141,11 @@ OvsGetIcmp(const NET_BUFFER_LIST *packet,
>     return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
> }
> 
>+static const MPLSHdr *
>+OvsGetMpls(const NET_BUFFER_LIST *packet,
>+           UINT32 ofs,
>+           MPLSHdr *storage)
>+{
>+    return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
>+}
> #endif /* __PACKET_PARSER_H_ */
>-- 
>1.9.0.msysgit.0
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=4jij1GpaBzb8M2GdcLn47dOarUO1na
>f-2Is_kWuRby4&s=XYH34q49hUVRrnPrL3HqLDdPj3AJPM0N_ab6JxhriJk&e=
Sorin Vinturis Nov. 19, 2015, 12:31 p.m. UTC | #2 
Hi Sai,

I manage to test the MPLS patch using a Devstack machine on which I have installed OVS. I had also added flows to push up to 3 MPLS labels on the network packets and then ping a Hyper-V machine, which had installed the custom extension containing the MPLS support.

Regarding your review comments, please see my answers inline.

Thanks for your review,
-Sorin

-----Original Message-----
From: Sairam Venugopal [mailto:vsairam@vmware.com] 
Sent: Thursday, 19 November, 2015 00:35
To: Sorin Vinturis; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Support for OVS_KEY_ATTR_MPLS attribute

Hey Sorin,

Were you able to test out the changes? Can you clarify how we can reproduce it? It will be nice to have documentation around testing this since it¹s a new feature. I remember you having difficulties with testing this earlier, hopefully that is now resolved.

I took a look at your changes and have mentioned my comments inline.

Thanks,
Sairam

On 11/11/15, 12:11 PM, "Sorin Vinturis" <svinturis@cloudbasesolutions.com>
wrote:

>This patch adds OVS_KEY_ATTR_MPLS to the OVS flow mechanism.
>
>Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
>---
> datapath-windows/ovsext/Actions.c      | 176
>+++++++++++++++++++++++++++++++++
> datapath-windows/ovsext/DpInternal.h   |   7 ++
> datapath-windows/ovsext/Ethernet.h     |   2 +
> datapath-windows/ovsext/Flow.c         |  89 ++++++++++++++++-
> datapath-windows/ovsext/NetProto.h     |  33 +++++++
> datapath-windows/ovsext/PacketParser.c |  12 +--
> datapath-windows/ovsext/PacketParser.h |   7 ++
> 7 files changed, 318 insertions(+), 8 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index ce592b3..9ee1763 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1074,6 +1074,142 @@ OvsPopVlanInPktBuf(OvsForwardingContext
>*ovsFwdCtx)
>     return NDIS_STATUS_SUCCESS;
> }

I think we should move the MPLS-Push and POP functions to a separate file mpls.h/mpls.c to keep Actions.c limited to what it does.
SV: Currently all actions related functions are kept in Actions.c. I don't think we need to separate MPLS functionality yet, as there are
not many features related to MPLS besides push and pop.

> 
>+static __inline NDIS_STATUS
>+OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
>+                  const struct ovs_action_push_mpls *mpls) {
>+    NDIS_STATUS status;
>+    PNET_BUFFER curNb = NULL;
>+    PMDL curMdl = NULL;
>+    PUINT8 bufferStart = NULL;
>+    OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+    EthHdr *ethHdr = NULL;
>+    MPLSHdr *mplsHdr = NULL;
>+    UINT32 mdlLen = 0, curMdlOffset = 0;
>+    UINT32 packetLen = 0;
>+    PNET_BUFFER_LIST newNbl;
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    ASSERT(mdlLen > 0);
>+    /* Bail out if the L2 header is not in a contiguous buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof *ethHdr) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    ASSERT((INT)mdlLen >= 0);
>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               MPLS_HLEN, 0, TRUE /*copy NBL info*/);
>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                L"Complete after partial copy.");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"OVS-Dropped due to resources");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!curMdl) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    ASSERT(mdlLen >= MPLS_HLEN);
>+
>+    ethHdr = (EthHdr *)(bufferStart + curMdlOffset);
>+    ethHdr->Type = mpls->mpls_ethertype;
>+
>+    mplsHdr = (MPLSHdr *)(bufferStart + curMdlOffset + MPLS_HLEN);
>+    mplsHdr->mpls_lse = mpls->mpls_lse;
>+
>+    layers->l3Offset += MPLS_HLEN;
>+    layers->isIPv4 = 0;
>+    layers->isIPv6 = 0;
>+

Should we introduce an isMpls label in OVS_PACKET_HDR_INFO to identify this?
SV: There is no current need to have such a flag.

>+    NdisRetreatNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+

This function seems like a copy of OvsPopVlanInPktBuf(OvsForwardingContext
*ovsFwdCtx)
except for the shiftLength/MPLS_HLEN. You can have 1 function that takes in the length instead.
SV: Sure. Will do that.

>+static __inline NDIS_STATUS
>+OvsActionMplsPop(OvsForwardingContext *ovsFwdCtx) {
>+    PNET_BUFFER curNb;
>+    PMDL curMdl;
>+    PUINT8 bufferStart;
>+    ULONG dataLength = sizeof (DL_EUI48) + sizeof (DL_EUI48);
>+    UINT32 packetLen, mdlLen;
>+    PNET_BUFFER_LIST newNbl;
>+    NDIS_STATUS status;
>+
>+    PUINT8 tempBuffer[sizeof (DL_EUI48) + sizeof (DL_EUI48)];
>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               0, 0, TRUE /* copy NBL info */);
>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    /* Complete the original NBL and create a copy to modify. */
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx, L"OVS-Dropped due to 
>+ copy");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"Dropped due to resouces");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    /* Bail out if L2 + MPLS header is not contiguous in the first
>buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof(EthHdr) + MPLS_HLEN) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    RtlCopyMemory(tempBuffer, bufferStart, dataLength);
>+    RtlCopyMemory(bufferStart + MPLS_HLEN, tempBuffer, dataLength);
>+    NdisAdvanceNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
> /*
>  *
>-----------------------------------------------------------------------
>---
>  * OvsTunnelAttrToIPv4TunnelKey --
>@@ -1513,6 +1649,46 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT
>switchContext,
>             break;
>         }
> 
>+        case OVS_ACTION_ATTR_PUSH_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || 
>+ ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPush(&ovsFwdCtx,
>+                                       (struct ovs_action_push_mpls
>*)NlAttrGet
>+                                       ((const PNL_ATTR)a));
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set push MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>+        case OVS_ACTION_ATTR_POP_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || 
>+ ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPop(&ovsFwdCtx);
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set pop MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>         case OVS_ACTION_ATTR_USERSPACE:
>         {
>             PNL_ATTR userdataAttr;
>diff --git a/datapath-windows/ovsext/DpInternal.h
>b/datapath-windows/ovsext/DpInternal.h
>index 8de48a2..c195494 100644
>--- a/datapath-windows/ovsext/DpInternal.h
>+++ b/datapath-windows/ovsext/DpInternal.h
>@@ -20,6 +20,7 @@
> #include <netioapi.h>
> #define IFNAMSIZ IF_NAMESIZE
> #include "../ovsext/Netlink/Netlink.h"
>+#include "NetProto.h"
> 
> #define OVS_DP_NUMBER   ((uint32_t) 0)
> 
>@@ -149,6 +150,11 @@ typedef union OvsIPv4TunnelKey {
>     uint64_t attr[NUM_PKT_ATTR_REQUIRED];  } OvsIPv4TunnelKey;
> 
>+typedef struct MplsKey {
>+    ovs_be32 top_lse;            /* MPLS topmost label stack entry. */
>+    uint8    pad[4];
>+} MplsKey; /* Size of 8 bytes. */
>+
> typedef __declspec(align(8)) struct OvsFlowKey {
>     OvsIPv4TunnelKey tunKey;     /* 24 bytes */
>     L2Key l2;                    /* 24 bytes */
>@@ -157,6 +163,7 @@ typedef __declspec(align(8)) struct OvsFlowKey {
>         ArpKey arpKey;           /* size 24 */
>         Ipv6Key ipv6Key;         /* size 48 */
>         Icmp6Key icmp6Key;       /* size 72 */
>+        MplsKey mplsKey;         /* size 8 */
>     };
> } OvsFlowKey;
> 
>diff --git a/datapath-windows/ovsext/Ethernet.h
>b/datapath-windows/ovsext/Ethernet.h
>index 22aa27c..1d69d47 100644
>--- a/datapath-windows/ovsext/Ethernet.h
>+++ b/datapath-windows/ovsext/Ethernet.h
>@@ -66,6 +66,8 @@ typedef enum {
>     ETH_TYPE_CDP         = 0x2000,
>     ETH_TYPE_802_1PQ     = 0x8100, // not really a DIX type, but used as
>such
>     ETH_TYPE_LLC         = 0xFFFF, // 0xFFFF is IANA reserved, used to
>mark LLC
>+    ETH_TYPE_MPLS        = 0x8847,
>+    ETH_TYPE_MPLS_MCAST  = 0x8848,
> } Eth_DixType;
> 
> typedef enum {
>diff --git a/datapath-windows/ovsext/Flow.c 
>b/datapath-windows/ovsext/Flow.c index b629c93..c989c14 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -80,6 +80,8 @@ static NTSTATUS _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf,
>                                        Icmp6Key *ipv6FlowPutIcmpKey);  
>static NTSTATUS _MapFlowArpKeyToNlKey(PNL_BUFFER nlBuf,
>                                       ArpKey *arpFlowPutKey);
>+static NTSTATUS _MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf,
>+                                       MplsKey *mplsFlowPutKey);
> 
> static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
>                                OvsFlowDumpOutput *dumpOutput, @@ 
>-108,7 +110,7 @@ const NL_POLICY nlFlowPolicy[] = {
> 
> /* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
>  * Some of the attributes like OVS_KEY_ATTR_RECIRC_ID
>- * & OVS_KEY_ATTR_MPLS are not supported yet. */
>+ * are not supported yet. */
> 
> const NL_POLICY nlFlowKeyPolicy[] = {
>     [OVS_KEY_ATTR_ENCAP] = {.type = NL_A_VAR_LEN, .optional = TRUE}, 
>@@ -872,6 +874,13 @@ MapFlowKeyToNlKey(PNL_BUFFER nlBuf,
>         break;
>         }
> 
>+        case ETH_TYPE_MPLS:
>+        case ETH_TYPE_MPLS_MCAST: {
>+        MplsKey *mplsFlowPutKey = &(flowKey->mplsKey);
>+        rc = _MapFlowMplsKeyToNlKey(nlBuf, mplsFlowPutKey);
>+        break;
>+        }
>+
>         default:
>         break;
>     }
>@@ -1194,6 +1203,31 @@ done:
> 
> /*
>  
>*----------------------------------------------------------------------
>---
>---
>+ *  _MapFlowMplsKeyToNlKey --
>+ *    Maps _MapFlowMplsKeyToNlKey to OVS_KEY_ATTR_MPLS attribute.
>+ 
>*----------------------------------------------------------------------
>---
>---
>+ */
>+static NTSTATUS
>+_MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf, MplsKey *mplsFlowPutKey) {
>+    NTSTATUS rc = STATUS_SUCCESS;
>+    struct ovs_key_mpls *mplsKey;
>+
>+    mplsKey = (struct ovs_key_mpls *)
>+        NlMsgPutTailUnspecUninit(nlBuf, OVS_KEY_ATTR_MPLS,
>sizeof(*mplsKey));
>+    if (!mplsKey) {
>+        rc = STATUS_UNSUCCESSFUL;
>+        goto done;
>+    }
>+
>+    mplsKey->mpls_lse = mplsFlowPutKey->top_lse;
>+
>+done:
>+    return rc;
>+}
>+
>+/*
>+ 
>*----------------------------------------------------------------------
>---
>---
>  *  _MapNlToFlowPut --
>  *    Maps input netlink message to OvsFlowPut.
>  
>*----------------------------------------------------------------------
>---
>---
>@@ -1469,8 +1503,28 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
>             arpFlowPutKey->pad[1] = 0;
>             arpFlowPutKey->pad[2] = 0;
>             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
>-            break;
>         }
>+        break;
>+    }
>+    case ETH_TYPE_MPLS:
>+    case ETH_TYPE_MPLS_MCAST: {
>+
>+        if (keyAttrs[OVS_KEY_ATTR_MPLS]) {
>+            MplsKey *mplsFlowPutKey = &destKey->mplsKey;
>+            const struct ovs_key_mpls *mplsKey;
>+            UINT32 size = NlAttrGetSize(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+            UINT32 n = size / sizeof(struct ovs_key_mpls);
>+
>+            mplsKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+
>+            mplsFlowPutKey->top_lse = mplsKey->mpls_lse;
>+            mplsFlowPutKey->pad[0] = 0;
>+            mplsFlowPutKey->pad[1] = 0;
>+            mplsFlowPutKey->pad[2] = 0;
>+            mplsFlowPutKey->pad[3] = 0;
>+            destKey->l2.keyLen += (UINT16)n * sizeof(MplsKey);
>+        }
>+        break;
>     }
>     }
> }
>@@ -1864,6 +1918,37 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,
>                 memcpy(arpKey->arpTha, arp->arp_tha, ETH_ADDR_LENGTH);
>             }
>         }
>+    } else if (flow->l2.dlType == htons(ETH_TYPE_MPLS) ||
>+               flow->l2.dlType == htons(ETH_TYPE_MPLS_MCAST)) {
>+        MPLSHdr mplsStorage;
>+        const MPLSHdr *mpls;
>+        MplsKey *mplsKey = &flow->mplsKey;
>+        ((UINT64 *)mplsKey)[0] = 0;
>+
>+        /* In the presence of an MPLS label stack the end of the L2
>+         * header and the beginning of the L3 header differ.
>+         *
>+         * A network packet may contain multiple MPLS labels, but we
>+         * are only interested in the topmost label stack entry.
>+         *
>+         * Advance network header to the beginning of the L3 header.
>+         * layers->l3Offset corresponds to the end of the L2 header.
>+         */
>+        for (UINT32 i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
>+            mpls = OvsGetMpls(packet, layers->l3Offset, &mplsStorage);
>+            if (mpls) {
>+
>+                /* Keep only the topmost MPLS label stack entry. */
>+                if (i == 0) {
>+                    mplsKey->top_lse = mpls->mpls_lse;
>+                }
>+
>+                layers->l3Offset += sizeof(MPLSHdr);
>+
>+                if (mpls->mpls_lse & htonl(MPLS_LS_S_MASK))
>+                    break;
>+            }
>+        }
>     }
> 
>     return NDIS_STATUS_SUCCESS;
>diff --git a/datapath-windows/ovsext/NetProto.h
>b/datapath-windows/ovsext/NetProto.h
>index a364869..12e8bd9 100644
>--- a/datapath-windows/ovsext/NetProto.h
>+++ b/datapath-windows/ovsext/NetProto.h
>@@ -366,4 +366,37 @@ typedef struct IPOpt {
> #define SOCKET_IPPROTO_UDP   17
> #define SOCKET_IPPROTO_GRE   47
> 
>+/* Reference: RFC 5462, RFC 3032
>+ *
>+ *  0                   1                   2                   3
>+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ * |                Label                  | TC  |S|       TTL     |
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ *
>+ *	Label:  Label Value, 20 bits
>+ *	TC:     Traffic Class field, 3 bits
>+ *	S:      Bottom of Stack, 1 bit
>+ *	TTL:    Time to Live, 8 bits
>+ */
>+

Can¹t you simply define the structure with the respective bits that are used instead of the mask and shift?
You can name it lse instead of redundant mpls_lse.
SV: I can do that, sure.

>+typedef struct MPLSHdr {
>+    ovs_be32 mpls_lse;
>+} MPLSHdr;
>+
>+/*
>+ * MPLS definitions
>+ */
>+#define MPLS_HLEN               4
>+#define FLOW_MAX_MPLS_LABELS    3
>+#define MPLS_LS_LABEL_MASK      0xFFFFF000
>+#define MPLS_LS_LABEL_SHIFT     12
>+#define MPLS_LS_TC_MASK         0x00000E00
>+#define MPLS_LS_TC_SHIFT        9
>+#define MPLS_LS_S_MASK          0x00000100
>+#define MPLS_LS_S_SHIFT         8
>+#define MPLS_LS_TTL_MASK        0x000000FF
>+#define MPLS_LS_TTL_SHIFT       0
>+
>+
> #endif /* __NET_PROTO_H_ */
>diff --git a/datapath-windows/ovsext/PacketParser.c
>b/datapath-windows/ovsext/PacketParser.c
>index e01be17..246c603 100644
>--- a/datapath-windows/ovsext/PacketParser.c
>+++ b/datapath-windows/ovsext/PacketParser.c
>@@ -84,8 +84,8 @@ OvsGetPacketBytes(const NET_BUFFER_LIST *nbl,
> 
> NDIS_STATUS
> OvsParseIPv6(const NET_BUFFER_LIST *packet,
>-          OvsFlowKey *key,
>-          POVS_PACKET_HDR_INFO layers)
>+             OvsFlowKey *key,
>+             POVS_PACKET_HDR_INFO layers)
> {
>     UINT16 ofs = layers->l3Offset;
>     IPv6Hdr ipv6HdrStorage;
>@@ -178,8 +178,8 @@ OvsParseIPv6(const NET_BUFFER_LIST *packet,
> 
> VOID
> OvsParseTcp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     TCPHdr tcpStorage;
>     const TCPHdr *tcp = OvsGetTcp(packet, layers->l4Offset, 
>&tcpStorage); @@ -193,8 +193,8 @@ OvsParseTcp(const NET_BUFFER_LIST 
>*packet,
> 
> VOID
> OvsParseUdp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     UDPHdr udpStorage;
>     const UDPHdr *udp = OvsGetUdp(packet, layers->l4Offset, 
>&udpStorage); diff --git a/datapath-windows/ovsext/PacketParser.h
>b/datapath-windows/ovsext/PacketParser.h
>index 55d110f..96136b7 100644
>--- a/datapath-windows/ovsext/PacketParser.h
>+++ b/datapath-windows/ovsext/PacketParser.h
>@@ -141,4 +141,11 @@ OvsGetIcmp(const NET_BUFFER_LIST *packet,
>     return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);  
>}
> 
>+static const MPLSHdr *
>+OvsGetMpls(const NET_BUFFER_LIST *packet,
>+           UINT32 ofs,
>+           MPLSHdr *storage)
>+{
>+    return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage); }
> #endif /* __PACKET_PARSER_H_ */
>--
>1.9.0.msysgit.0
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mai
>lma 
>n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r
>=Dc 
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=4jij1GpaBzb8M2GdcLn47dOarUO
>1na f-2Is_kWuRby4&s=XYH34q49hUVRrnPrL3HqLDdPj3AJPM0N_ab6JxhriJk&e=
Nithin Raju Dec. 8, 2015, 1:02 a.m. UTC | #3 
Hi Sorin,
I found some bugs in OvsActionMplsPush(). Can you pls. clarify if my
comments are valid? If they are genuine bugs, did your testing not catch
them? Apologies, if I missed anything.

I¹ll review the rest of the patch after the claritification.

Thanks,
-- Nithin

-----Original Message-----
From: dev <dev-bounces@openvswitch.org> on behalf of Sorin Vinturis
<svinturis@cloudbasesolutions.com>
Date: Wednesday, November 11, 2015 at 12:11 PM
To: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: [ovs-dev] [PATCH] datapath-windows: Support for
OVS_KEY_ATTR_MPLS	attribute

>This patch adds OVS_KEY_ATTR_MPLS to the OVS flow mechanism.
>
>Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
>---
> datapath-windows/ovsext/Actions.c      | 176
>+++++++++++++++++++++++++++++++++
> datapath-windows/ovsext/DpInternal.h   |   7 ++
> datapath-windows/ovsext/Ethernet.h     |   2 +
> datapath-windows/ovsext/Flow.c         |  89 ++++++++++++++++-
> datapath-windows/ovsext/NetProto.h     |  33 +++++++
> datapath-windows/ovsext/PacketParser.c |  12 +--
> datapath-windows/ovsext/PacketParser.h |   7 ++
> 7 files changed, 318 insertions(+), 8 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index ce592b3..9ee1763 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1074,6 +1074,142 @@ OvsPopVlanInPktBuf(OvsForwardingContext
>*ovsFwdCtx)
>     return NDIS_STATUS_SUCCESS;
> }
> 
>+static __inline NDIS_STATUS
>+OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
>+                  const struct ovs_action_push_mpls *mpls)
>+{
>+    NDIS_STATUS status;
>+    PNET_BUFFER curNb = NULL;
>+    PMDL curMdl = NULL;
>+    PUINT8 bufferStart = NULL;
>+    OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+    EthHdr *ethHdr = NULL;
>+    MPLSHdr *mplsHdr = NULL;
>+    UINT32 mdlLen = 0, curMdlOffset = 0;
>+    UINT32 packetLen = 0;
>+    PNET_BUFFER_LIST newNbl;
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    ASSERT(mdlLen > 0);
>+    /* Bail out if the L2 header is not in a contiguous buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof *ethHdr) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    ASSERT((INT)mdlLen >= 0);

You don¹t need any of these checks if the NBL is going to be copied
anyway. These checks are needed if you are going to modify the packet
inline. Look at OvsUpdateIPv4Header() for correct usage.

>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               MPLS_HLEN, 0, TRUE /*copy NBL info*/);

What is the intention of this partial copy? Are you trying to allocate
more headroom or creating a copy so as to update the L2 header?

You probably want to do:
newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl,
                           L2 Header size + MPLS_HLEN, MPLS_HLEN, TRUE
/*copy NBL info*/);


>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                L"Complete after partial copy.");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"OVS-Dropped due to resources");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!curMdl) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    ASSERT(mdlLen >= MPLS_HLEN);
>+
>+    ethHdr = (EthHdr *)(bufferStart + curMdlOffset);
>+    ethHdr->Type = mpls->mpls_ethertype;
>+
>+    mplsHdr = (MPLSHdr *)(bufferStart + curMdlOffset + MPLS_HLEN);

mplsHdr should be at: (void *)ethHdr + MPLS_HLEN.

>+    mplsHdr->mpls_lse = mpls->mpls_lse;

As I understood, the packet was like:
ETH + IP

The new NBL just after the partial copy looks like:
HEADROOM (4B) + ETH + IP

You want this to look like:
ETH + MPLS + IP

You need to left shift the Eth header, including the source and
destination MAC addresses. Only the ethtype seems to be updated. Nothing
more.

>+
>+    layers->l3Offset += MPLS_HLEN;
>+    layers->isIPv4 = 0;
>+    layers->isIPv6 = 0;

Should we be resetting layers->isIpv4 and isIPv6 to 0? The inner packet is
still intact, right?

>+
>+    NdisRetreatNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
>+static __inline NDIS_STATUS
>+OvsActionMplsPop(OvsForwardingContext *ovsFwdCtx)
>+{
>+    PNET_BUFFER curNb;
>+    PMDL curMdl;
>+    PUINT8 bufferStart;
>+    ULONG dataLength = sizeof (DL_EUI48) + sizeof (DL_EUI48);
>+    UINT32 packetLen, mdlLen;
>+    PNET_BUFFER_LIST newNbl;
>+    NDIS_STATUS status;
>+
>+    PUINT8 tempBuffer[sizeof (DL_EUI48) + sizeof (DL_EUI48)];
>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               0, 0, TRUE /* copy NBL info */);
>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    /* Complete the original NBL and create a copy to modify. */
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx, L"OVS-Dropped due to copy");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"Dropped due to resouces");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    /* Bail out if L2 + MPLS header is not contiguous in the first
>buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof(EthHdr) + MPLS_HLEN) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    RtlCopyMemory(tempBuffer, bufferStart, dataLength);
>+    RtlCopyMemory(bufferStart + MPLS_HLEN, tempBuffer, dataLength);
>+    NdisAdvanceNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
> /*
>  * 
>--------------------------------------------------------------------------
>  * OvsTunnelAttrToIPv4TunnelKey --
>@@ -1513,6 +1649,46 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT
>switchContext,
>             break;
>         }
> 
>+        case OVS_ACTION_ATTR_PUSH_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPush(&ovsFwdCtx,
>+                                       (struct ovs_action_push_mpls
>*)NlAttrGet
>+                                       ((const PNL_ATTR)a));
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set push MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>+        case OVS_ACTION_ATTR_POP_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPop(&ovsFwdCtx);
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set pop MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>         case OVS_ACTION_ATTR_USERSPACE:
>         {
>             PNL_ATTR userdataAttr;
>diff --git a/datapath-windows/ovsext/DpInternal.h
>b/datapath-windows/ovsext/DpInternal.h
>index 8de48a2..c195494 100644
>--- a/datapath-windows/ovsext/DpInternal.h
>+++ b/datapath-windows/ovsext/DpInternal.h
>@@ -20,6 +20,7 @@
> #include <netioapi.h>
> #define IFNAMSIZ IF_NAMESIZE
> #include "../ovsext/Netlink/Netlink.h"
>+#include "NetProto.h"
> 
> #define OVS_DP_NUMBER   ((uint32_t) 0)
> 
>@@ -149,6 +150,11 @@ typedef union OvsIPv4TunnelKey {
>     uint64_t attr[NUM_PKT_ATTR_REQUIRED];
> } OvsIPv4TunnelKey;
> 
>+typedef struct MplsKey {
>+    ovs_be32 top_lse;            /* MPLS topmost label stack entry. */
>+    uint8    pad[4];
>+} MplsKey; /* Size of 8 bytes. */
>+
> typedef __declspec(align(8)) struct OvsFlowKey {
>     OvsIPv4TunnelKey tunKey;     /* 24 bytes */
>     L2Key l2;                    /* 24 bytes */
>@@ -157,6 +163,7 @@ typedef __declspec(align(8)) struct OvsFlowKey {
>         ArpKey arpKey;           /* size 24 */
>         Ipv6Key ipv6Key;         /* size 48 */
>         Icmp6Key icmp6Key;       /* size 72 */
>+        MplsKey mplsKey;         /* size 8 */
>     };
> } OvsFlowKey;
> 
>diff --git a/datapath-windows/ovsext/Ethernet.h
>b/datapath-windows/ovsext/Ethernet.h
>index 22aa27c..1d69d47 100644
>--- a/datapath-windows/ovsext/Ethernet.h
>+++ b/datapath-windows/ovsext/Ethernet.h
>@@ -66,6 +66,8 @@ typedef enum {
>     ETH_TYPE_CDP         = 0x2000,
>     ETH_TYPE_802_1PQ     = 0x8100, // not really a DIX type, but used as
>such
>     ETH_TYPE_LLC         = 0xFFFF, // 0xFFFF is IANA reserved, used to
>mark LLC
>+    ETH_TYPE_MPLS        = 0x8847,
>+    ETH_TYPE_MPLS_MCAST  = 0x8848,
> } Eth_DixType;
> 
> typedef enum {
>diff --git a/datapath-windows/ovsext/Flow.c
>b/datapath-windows/ovsext/Flow.c
>index b629c93..c989c14 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -80,6 +80,8 @@ static NTSTATUS _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf,
>                                        Icmp6Key *ipv6FlowPutIcmpKey);
> static NTSTATUS _MapFlowArpKeyToNlKey(PNL_BUFFER nlBuf,
>                                       ArpKey *arpFlowPutKey);
>+static NTSTATUS _MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf,
>+                                       MplsKey *mplsFlowPutKey);
> 
> static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
>                                OvsFlowDumpOutput *dumpOutput,
>@@ -108,7 +110,7 @@ const NL_POLICY nlFlowPolicy[] = {
> 
> /* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
>  * Some of the attributes like OVS_KEY_ATTR_RECIRC_ID
>- * & OVS_KEY_ATTR_MPLS are not supported yet. */
>+ * are not supported yet. */
> 
> const NL_POLICY nlFlowKeyPolicy[] = {
>     [OVS_KEY_ATTR_ENCAP] = {.type = NL_A_VAR_LEN, .optional = TRUE},
>@@ -872,6 +874,13 @@ MapFlowKeyToNlKey(PNL_BUFFER nlBuf,
>         break;
>         }
> 
>+        case ETH_TYPE_MPLS:
>+        case ETH_TYPE_MPLS_MCAST: {
>+        MplsKey *mplsFlowPutKey = &(flowKey->mplsKey);
>+        rc = _MapFlowMplsKeyToNlKey(nlBuf, mplsFlowPutKey);
>+        break;
>+        }
>+
>         default:
>         break;
>     }
>@@ -1194,6 +1203,31 @@ done:
> 
> /*
>  
>*-------------------------------------------------------------------------
>---
>+ *  _MapFlowMplsKeyToNlKey --
>+ *    Maps _MapFlowMplsKeyToNlKey to OVS_KEY_ATTR_MPLS attribute.
>+ 
>*-------------------------------------------------------------------------
>---
>+ */
>+static NTSTATUS
>+_MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf, MplsKey *mplsFlowPutKey)
>+{
>+    NTSTATUS rc = STATUS_SUCCESS;
>+    struct ovs_key_mpls *mplsKey;
>+
>+    mplsKey = (struct ovs_key_mpls *)
>+        NlMsgPutTailUnspecUninit(nlBuf, OVS_KEY_ATTR_MPLS,
>sizeof(*mplsKey));
>+    if (!mplsKey) {
>+        rc = STATUS_UNSUCCESSFUL;
>+        goto done;
>+    }
>+
>+    mplsKey->mpls_lse = mplsFlowPutKey->top_lse;
>+
>+done:
>+    return rc;
>+}
>+
>+/*
>+ 
>*-------------------------------------------------------------------------
>---
>  *  _MapNlToFlowPut --
>  *    Maps input netlink message to OvsFlowPut.
>  
>*-------------------------------------------------------------------------
>---
>@@ -1469,8 +1503,28 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
>             arpFlowPutKey->pad[1] = 0;
>             arpFlowPutKey->pad[2] = 0;
>             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
>-            break;
>         }
>+        break;
>+    }
>+    case ETH_TYPE_MPLS:
>+    case ETH_TYPE_MPLS_MCAST: {
>+
>+        if (keyAttrs[OVS_KEY_ATTR_MPLS]) {
>+            MplsKey *mplsFlowPutKey = &destKey->mplsKey;
>+            const struct ovs_key_mpls *mplsKey;
>+            UINT32 size = NlAttrGetSize(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+            UINT32 n = size / sizeof(struct ovs_key_mpls);
>+
>+            mplsKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+
>+            mplsFlowPutKey->top_lse = mplsKey->mpls_lse;
>+            mplsFlowPutKey->pad[0] = 0;
>+            mplsFlowPutKey->pad[1] = 0;
>+            mplsFlowPutKey->pad[2] = 0;
>+            mplsFlowPutKey->pad[3] = 0;
>+            destKey->l2.keyLen += (UINT16)n * sizeof(MplsKey);
>+        }
>+        break;
>     }
>     }
> }
>@@ -1864,6 +1918,37 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,
>                 memcpy(arpKey->arpTha, arp->arp_tha, ETH_ADDR_LENGTH);
>             }
>         }
>+    } else if (flow->l2.dlType == htons(ETH_TYPE_MPLS) ||
>+               flow->l2.dlType == htons(ETH_TYPE_MPLS_MCAST)) {
>+        MPLSHdr mplsStorage;
>+        const MPLSHdr *mpls;
>+        MplsKey *mplsKey = &flow->mplsKey;
>+        ((UINT64 *)mplsKey)[0] = 0;
>+
>+        /* In the presence of an MPLS label stack the end of the L2
>+         * header and the beginning of the L3 header differ.
>+         *
>+         * A network packet may contain multiple MPLS labels, but we
>+         * are only interested in the topmost label stack entry.
>+         *
>+         * Advance network header to the beginning of the L3 header.
>+         * layers->l3Offset corresponds to the end of the L2 header.
>+         */
>+        for (UINT32 i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
>+            mpls = OvsGetMpls(packet, layers->l3Offset, &mplsStorage);
>+            if (mpls) {
>+
>+                /* Keep only the topmost MPLS label stack entry. */
>+                if (i == 0) {
>+                    mplsKey->top_lse = mpls->mpls_lse;
>+                }
>+
>+                layers->l3Offset += sizeof(MPLSHdr);
>+
>+                if (mpls->mpls_lse & htonl(MPLS_LS_S_MASK))
>+                    break;
>+            }
>+        }
>     }
> 
>     return NDIS_STATUS_SUCCESS;
>diff --git a/datapath-windows/ovsext/NetProto.h
>b/datapath-windows/ovsext/NetProto.h
>index a364869..12e8bd9 100644
>--- a/datapath-windows/ovsext/NetProto.h
>+++ b/datapath-windows/ovsext/NetProto.h
>@@ -366,4 +366,37 @@ typedef struct IPOpt {
> #define SOCKET_IPPROTO_UDP   17
> #define SOCKET_IPPROTO_GRE   47
> 
>+/* Reference: RFC 5462, RFC 3032
>+ *
>+ *  0                   1                   2                   3
>+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ * |                Label                  | TC  |S|       TTL     |
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ *
>+ *	Label:  Label Value, 20 bits
>+ *	TC:     Traffic Class field, 3 bits
>+ *	S:      Bottom of Stack, 1 bit
>+ *	TTL:    Time to Live, 8 bits
>+ */
>+
>+typedef struct MPLSHdr {
>+    ovs_be32 mpls_lse;
>+} MPLSHdr;
>+
>+/*
>+ * MPLS definitions
>+ */
>+#define MPLS_HLEN               4
>+#define FLOW_MAX_MPLS_LABELS    3
>+#define MPLS_LS_LABEL_MASK      0xFFFFF000
>+#define MPLS_LS_LABEL_SHIFT     12
>+#define MPLS_LS_TC_MASK         0x00000E00
>+#define MPLS_LS_TC_SHIFT        9
>+#define MPLS_LS_S_MASK          0x00000100
>+#define MPLS_LS_S_SHIFT         8
>+#define MPLS_LS_TTL_MASK        0x000000FF
>+#define MPLS_LS_TTL_SHIFT       0
>+
>+
> #endif /* __NET_PROTO_H_ */
>diff --git a/datapath-windows/ovsext/PacketParser.c
>b/datapath-windows/ovsext/PacketParser.c
>index e01be17..246c603 100644
>--- a/datapath-windows/ovsext/PacketParser.c
>+++ b/datapath-windows/ovsext/PacketParser.c
>@@ -84,8 +84,8 @@ OvsGetPacketBytes(const NET_BUFFER_LIST *nbl,
> 
> NDIS_STATUS
> OvsParseIPv6(const NET_BUFFER_LIST *packet,
>-          OvsFlowKey *key,
>-          POVS_PACKET_HDR_INFO layers)
>+             OvsFlowKey *key,
>+             POVS_PACKET_HDR_INFO layers)
> {
>     UINT16 ofs = layers->l3Offset;
>     IPv6Hdr ipv6HdrStorage;
>@@ -178,8 +178,8 @@ OvsParseIPv6(const NET_BUFFER_LIST *packet,
> 
> VOID
> OvsParseTcp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     TCPHdr tcpStorage;
>     const TCPHdr *tcp = OvsGetTcp(packet, layers->l4Offset, &tcpStorage);
>@@ -193,8 +193,8 @@ OvsParseTcp(const NET_BUFFER_LIST *packet,
> 
> VOID
> OvsParseUdp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     UDPHdr udpStorage;
>     const UDPHdr *udp = OvsGetUdp(packet, layers->l4Offset, &udpStorage);
>diff --git a/datapath-windows/ovsext/PacketParser.h
>b/datapath-windows/ovsext/PacketParser.h
>index 55d110f..96136b7 100644
>--- a/datapath-windows/ovsext/PacketParser.h
>+++ b/datapath-windows/ovsext/PacketParser.h
>@@ -141,4 +141,11 @@ OvsGetIcmp(const NET_BUFFER_LIST *packet,
>     return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
> }
> 
>+static const MPLSHdr *
>+OvsGetMpls(const NET_BUFFER_LIST *packet,
>+           UINT32 ofs,
>+           MPLSHdr *storage)
>+{
>+    return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
>+}
> #endif /* __PACKET_PARSER_H_ */
>-- 
>1.9.0.msysgit.0
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pN
>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=nTWi1YZmmSTMCtjYpqMkthOFY5xBca
>i4dNH_JYc9kKs&s=h1Rf2WEWL_eMR1TjEUbBPzkw7OEWRVhCukH_1uWl9NY&e=
Sorin Vinturis Dec. 8, 2015, 9:21 a.m. UTC | #4 
Hi Nithin,

Thanks for your review. The mpls push and pop actions were tested by manually changing the action type when one was received. I don't know how to trigger the MPLS push/pop actions for properly testing this part. 

The part of the patch regarding MPLS key extraction or NL buffer parsing/setup were tested using a Hyper-V machine and a Devstack one. On the Devstack machine I have installed OVS and added mpls push flows to a tun bridge. In this way the network packets leaving the Devstack had the MPLS labels.

Your comments are valid and will be addressed in a subsequent patch.

Thanks,
-Sorin

-----Original Message-----
From: Nithin Raju [mailto:nithin@vmware.com] 
Sent: Tuesday, 8 December, 2015 03:03
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Support for OVS_KEY_ATTR_MPLS attribute

Hi Sorin,
I found some bugs in OvsActionMplsPush(). Can you pls. clarify if my comments are valid? If they are genuine bugs, did your testing not catch them? Apologies, if I missed anything.

I¹ll review the rest of the patch after the claritification.

Thanks,
-- Nithin

-----Original Message-----
From: dev <dev-bounces@openvswitch.org> on behalf of Sorin Vinturis <svinturis@cloudbasesolutions.com>
Date: Wednesday, November 11, 2015 at 12:11 PM
To: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: [ovs-dev] [PATCH] datapath-windows: Support for
OVS_KEY_ATTR_MPLS	attribute

>This patch adds OVS_KEY_ATTR_MPLS to the OVS flow mechanism.
>
>Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
>---
> datapath-windows/ovsext/Actions.c      | 176
>+++++++++++++++++++++++++++++++++
> datapath-windows/ovsext/DpInternal.h   |   7 ++
> datapath-windows/ovsext/Ethernet.h     |   2 +
> datapath-windows/ovsext/Flow.c         |  89 ++++++++++++++++-
> datapath-windows/ovsext/NetProto.h     |  33 +++++++
> datapath-windows/ovsext/PacketParser.c |  12 +--
> datapath-windows/ovsext/PacketParser.h |   7 ++
> 7 files changed, 318 insertions(+), 8 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index ce592b3..9ee1763 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1074,6 +1074,142 @@ OvsPopVlanInPktBuf(OvsForwardingContext
>*ovsFwdCtx)
>     return NDIS_STATUS_SUCCESS;
> }
> 
>+static __inline NDIS_STATUS
>+OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
>+                  const struct ovs_action_push_mpls *mpls) {
>+    NDIS_STATUS status;
>+    PNET_BUFFER curNb = NULL;
>+    PMDL curMdl = NULL;
>+    PUINT8 bufferStart = NULL;
>+    OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+    EthHdr *ethHdr = NULL;
>+    MPLSHdr *mplsHdr = NULL;
>+    UINT32 mdlLen = 0, curMdlOffset = 0;
>+    UINT32 packetLen = 0;
>+    PNET_BUFFER_LIST newNbl;
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    ASSERT(mdlLen > 0);
>+    /* Bail out if the L2 header is not in a contiguous buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof *ethHdr) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    ASSERT((INT)mdlLen >= 0);

You don¹t need any of these checks if the NBL is going to be copied anyway. These checks are needed if you are going to modify the packet inline. Look at OvsUpdateIPv4Header() for correct usage.

>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               MPLS_HLEN, 0, TRUE /*copy NBL info*/);

What is the intention of this partial copy? Are you trying to allocate more headroom or creating a copy so as to update the L2 header?

You probably want to do:
newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl,
                           L2 Header size + MPLS_HLEN, MPLS_HLEN, TRUE /*copy NBL info*/);


>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                L"Complete after partial copy.");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"OVS-Dropped due to resources");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!curMdl) {
>+        ovsActionStats.noResource++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    mdlLen -= curMdlOffset;
>+    ASSERT(mdlLen >= MPLS_HLEN);
>+
>+    ethHdr = (EthHdr *)(bufferStart + curMdlOffset);
>+    ethHdr->Type = mpls->mpls_ethertype;
>+
>+    mplsHdr = (MPLSHdr *)(bufferStart + curMdlOffset + MPLS_HLEN);

mplsHdr should be at: (void *)ethHdr + MPLS_HLEN.

>+    mplsHdr->mpls_lse = mpls->mpls_lse;

As I understood, the packet was like:
ETH + IP

The new NBL just after the partial copy looks like:
HEADROOM (4B) + ETH + IP

You want this to look like:
ETH + MPLS + IP

You need to left shift the Eth header, including the source and destination MAC addresses. Only the ethtype seems to be updated. Nothing more.

>+
>+    layers->l3Offset += MPLS_HLEN;
>+    layers->isIPv4 = 0;
>+    layers->isIPv6 = 0;

Should we be resetting layers->isIpv4 and isIPv6 to 0? The inner packet is still intact, right?

>+
>+    NdisRetreatNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
>+static __inline NDIS_STATUS
>+OvsActionMplsPop(OvsForwardingContext *ovsFwdCtx) {
>+    PNET_BUFFER curNb;
>+    PMDL curMdl;
>+    PUINT8 bufferStart;
>+    ULONG dataLength = sizeof (DL_EUI48) + sizeof (DL_EUI48);
>+    UINT32 packetLen, mdlLen;
>+    PNET_BUFFER_LIST newNbl;
>+    NDIS_STATUS status;
>+
>+    PUINT8 tempBuffer[sizeof (DL_EUI48) + sizeof (DL_EUI48)];
>+
>+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+                               0, 0, TRUE /* copy NBL info */);
>+    if (!newNbl) {
>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    /* Complete the original NBL and create a copy to modify. */
>+    OvsCompleteNBLForwardingCtx(ovsFwdCtx, L"OVS-Dropped due to 
>+ copy");
>+
>+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
>+                 
>NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>+                                  NULL, &ovsFwdCtx->layers, FALSE);
>+    if (status != NDIS_STATUS_SUCCESS) {
>+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                    L"Dropped due to resouces");
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
>+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
>+    ASSERT(curNb->Next == NULL);
>+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+    if (!bufferStart) {
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    /* Bail out if L2 + MPLS header is not contiguous in the first
>buffer. */
>+    if (MIN(packetLen, mdlLen) < sizeof(EthHdr) + MPLS_HLEN) {
>+        ASSERT(FALSE);
>+        return NDIS_STATUS_FAILURE;
>+    }
>+    bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>+    RtlCopyMemory(tempBuffer, bufferStart, dataLength);
>+    RtlCopyMemory(bufferStart + MPLS_HLEN, tempBuffer, dataLength);
>+    NdisAdvanceNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
> /*
>  *
>-----------------------------------------------------------------------
>---
>  * OvsTunnelAttrToIPv4TunnelKey --
>@@ -1513,6 +1649,46 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT
>switchContext,
>             break;
>         }
> 
>+        case OVS_ACTION_ATTR_PUSH_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || 
>+ ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPush(&ovsFwdCtx,
>+                                       (struct ovs_action_push_mpls
>*)NlAttrGet
>+                                       ((const PNL_ATTR)a));
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set push MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>+        case OVS_ACTION_ATTR_POP_MPLS:
>+        {
>+            if (ovsFwdCtx.destPortsSizeOut > 0 || 
>+ ovsFwdCtx.tunnelTxNic
>!= NULL
>+                || ovsFwdCtx.tunnelRxNic != NULL) {
>+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"OVS-adding destination failed";
>+                    goto dropit;
>+                }
>+            }
>+
>+            status = OvsActionMplsPop(&ovsFwdCtx);
>+            if (status != NDIS_STATUS_SUCCESS) {
>+                dropReason = L"OVS-set pop MPLS failed";
>+                goto dropit;
>+            }
>+            break;
>+        }
>+
>         case OVS_ACTION_ATTR_USERSPACE:
>         {
>             PNL_ATTR userdataAttr;
>diff --git a/datapath-windows/ovsext/DpInternal.h
>b/datapath-windows/ovsext/DpInternal.h
>index 8de48a2..c195494 100644
>--- a/datapath-windows/ovsext/DpInternal.h
>+++ b/datapath-windows/ovsext/DpInternal.h
>@@ -20,6 +20,7 @@
> #include <netioapi.h>
> #define IFNAMSIZ IF_NAMESIZE
> #include "../ovsext/Netlink/Netlink.h"
>+#include "NetProto.h"
> 
> #define OVS_DP_NUMBER   ((uint32_t) 0)
> 
>@@ -149,6 +150,11 @@ typedef union OvsIPv4TunnelKey {
>     uint64_t attr[NUM_PKT_ATTR_REQUIRED];  } OvsIPv4TunnelKey;
> 
>+typedef struct MplsKey {
>+    ovs_be32 top_lse;            /* MPLS topmost label stack entry. */
>+    uint8    pad[4];
>+} MplsKey; /* Size of 8 bytes. */
>+
> typedef __declspec(align(8)) struct OvsFlowKey {
>     OvsIPv4TunnelKey tunKey;     /* 24 bytes */
>     L2Key l2;                    /* 24 bytes */
>@@ -157,6 +163,7 @@ typedef __declspec(align(8)) struct OvsFlowKey {
>         ArpKey arpKey;           /* size 24 */
>         Ipv6Key ipv6Key;         /* size 48 */
>         Icmp6Key icmp6Key;       /* size 72 */
>+        MplsKey mplsKey;         /* size 8 */
>     };
> } OvsFlowKey;
> 
>diff --git a/datapath-windows/ovsext/Ethernet.h
>b/datapath-windows/ovsext/Ethernet.h
>index 22aa27c..1d69d47 100644
>--- a/datapath-windows/ovsext/Ethernet.h
>+++ b/datapath-windows/ovsext/Ethernet.h
>@@ -66,6 +66,8 @@ typedef enum {
>     ETH_TYPE_CDP         = 0x2000,
>     ETH_TYPE_802_1PQ     = 0x8100, // not really a DIX type, but used as
>such
>     ETH_TYPE_LLC         = 0xFFFF, // 0xFFFF is IANA reserved, used to
>mark LLC
>+    ETH_TYPE_MPLS        = 0x8847,
>+    ETH_TYPE_MPLS_MCAST  = 0x8848,
> } Eth_DixType;
> 
> typedef enum {
>diff --git a/datapath-windows/ovsext/Flow.c 
>b/datapath-windows/ovsext/Flow.c index b629c93..c989c14 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -80,6 +80,8 @@ static NTSTATUS _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf,
>                                        Icmp6Key *ipv6FlowPutIcmpKey);  
>static NTSTATUS _MapFlowArpKeyToNlKey(PNL_BUFFER nlBuf,
>                                       ArpKey *arpFlowPutKey);
>+static NTSTATUS _MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf,
>+                                       MplsKey *mplsFlowPutKey);
> 
> static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
>                                OvsFlowDumpOutput *dumpOutput, @@ 
>-108,7 +110,7 @@ const NL_POLICY nlFlowPolicy[] = {
> 
> /* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
>  * Some of the attributes like OVS_KEY_ATTR_RECIRC_ID
>- * & OVS_KEY_ATTR_MPLS are not supported yet. */
>+ * are not supported yet. */
> 
> const NL_POLICY nlFlowKeyPolicy[] = {
>     [OVS_KEY_ATTR_ENCAP] = {.type = NL_A_VAR_LEN, .optional = TRUE}, 
>@@ -872,6 +874,13 @@ MapFlowKeyToNlKey(PNL_BUFFER nlBuf,
>         break;
>         }
> 
>+        case ETH_TYPE_MPLS:
>+        case ETH_TYPE_MPLS_MCAST: {
>+        MplsKey *mplsFlowPutKey = &(flowKey->mplsKey);
>+        rc = _MapFlowMplsKeyToNlKey(nlBuf, mplsFlowPutKey);
>+        break;
>+        }
>+
>         default:
>         break;
>     }
>@@ -1194,6 +1203,31 @@ done:
> 
> /*
>  
>*----------------------------------------------------------------------
>---
>---
>+ *  _MapFlowMplsKeyToNlKey --
>+ *    Maps _MapFlowMplsKeyToNlKey to OVS_KEY_ATTR_MPLS attribute.
>+ 
>*----------------------------------------------------------------------
>---
>---
>+ */
>+static NTSTATUS
>+_MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf, MplsKey *mplsFlowPutKey) {
>+    NTSTATUS rc = STATUS_SUCCESS;
>+    struct ovs_key_mpls *mplsKey;
>+
>+    mplsKey = (struct ovs_key_mpls *)
>+        NlMsgPutTailUnspecUninit(nlBuf, OVS_KEY_ATTR_MPLS,
>sizeof(*mplsKey));
>+    if (!mplsKey) {
>+        rc = STATUS_UNSUCCESSFUL;
>+        goto done;
>+    }
>+
>+    mplsKey->mpls_lse = mplsFlowPutKey->top_lse;
>+
>+done:
>+    return rc;
>+}
>+
>+/*
>+ 
>*----------------------------------------------------------------------
>---
>---
>  *  _MapNlToFlowPut --
>  *    Maps input netlink message to OvsFlowPut.
>  
>*----------------------------------------------------------------------
>---
>---
>@@ -1469,8 +1503,28 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
>             arpFlowPutKey->pad[1] = 0;
>             arpFlowPutKey->pad[2] = 0;
>             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
>-            break;
>         }
>+        break;
>+    }
>+    case ETH_TYPE_MPLS:
>+    case ETH_TYPE_MPLS_MCAST: {
>+
>+        if (keyAttrs[OVS_KEY_ATTR_MPLS]) {
>+            MplsKey *mplsFlowPutKey = &destKey->mplsKey;
>+            const struct ovs_key_mpls *mplsKey;
>+            UINT32 size = NlAttrGetSize(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+            UINT32 n = size / sizeof(struct ovs_key_mpls);
>+
>+            mplsKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_MPLS]);
>+
>+            mplsFlowPutKey->top_lse = mplsKey->mpls_lse;
>+            mplsFlowPutKey->pad[0] = 0;
>+            mplsFlowPutKey->pad[1] = 0;
>+            mplsFlowPutKey->pad[2] = 0;
>+            mplsFlowPutKey->pad[3] = 0;
>+            destKey->l2.keyLen += (UINT16)n * sizeof(MplsKey);
>+        }
>+        break;
>     }
>     }
> }
>@@ -1864,6 +1918,37 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,
>                 memcpy(arpKey->arpTha, arp->arp_tha, ETH_ADDR_LENGTH);
>             }
>         }
>+    } else if (flow->l2.dlType == htons(ETH_TYPE_MPLS) ||
>+               flow->l2.dlType == htons(ETH_TYPE_MPLS_MCAST)) {
>+        MPLSHdr mplsStorage;
>+        const MPLSHdr *mpls;
>+        MplsKey *mplsKey = &flow->mplsKey;
>+        ((UINT64 *)mplsKey)[0] = 0;
>+
>+        /* In the presence of an MPLS label stack the end of the L2
>+         * header and the beginning of the L3 header differ.
>+         *
>+         * A network packet may contain multiple MPLS labels, but we
>+         * are only interested in the topmost label stack entry.
>+         *
>+         * Advance network header to the beginning of the L3 header.
>+         * layers->l3Offset corresponds to the end of the L2 header.
>+         */
>+        for (UINT32 i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
>+            mpls = OvsGetMpls(packet, layers->l3Offset, &mplsStorage);
>+            if (mpls) {
>+
>+                /* Keep only the topmost MPLS label stack entry. */
>+                if (i == 0) {
>+                    mplsKey->top_lse = mpls->mpls_lse;
>+                }
>+
>+                layers->l3Offset += sizeof(MPLSHdr);
>+
>+                if (mpls->mpls_lse & htonl(MPLS_LS_S_MASK))
>+                    break;
>+            }
>+        }
>     }
> 
>     return NDIS_STATUS_SUCCESS;
>diff --git a/datapath-windows/ovsext/NetProto.h
>b/datapath-windows/ovsext/NetProto.h
>index a364869..12e8bd9 100644
>--- a/datapath-windows/ovsext/NetProto.h
>+++ b/datapath-windows/ovsext/NetProto.h
>@@ -366,4 +366,37 @@ typedef struct IPOpt {
> #define SOCKET_IPPROTO_UDP   17
> #define SOCKET_IPPROTO_GRE   47
> 
>+/* Reference: RFC 5462, RFC 3032
>+ *
>+ *  0                   1                   2                   3
>+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ * |                Label                  | TC  |S|       TTL     |
>+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>+ *
>+ *	Label:  Label Value, 20 bits
>+ *	TC:     Traffic Class field, 3 bits
>+ *	S:      Bottom of Stack, 1 bit
>+ *	TTL:    Time to Live, 8 bits
>+ */
>+
>+typedef struct MPLSHdr {
>+    ovs_be32 mpls_lse;
>+} MPLSHdr;
>+
>+/*
>+ * MPLS definitions
>+ */
>+#define MPLS_HLEN               4
>+#define FLOW_MAX_MPLS_LABELS    3
>+#define MPLS_LS_LABEL_MASK      0xFFFFF000
>+#define MPLS_LS_LABEL_SHIFT     12
>+#define MPLS_LS_TC_MASK         0x00000E00
>+#define MPLS_LS_TC_SHIFT        9
>+#define MPLS_LS_S_MASK          0x00000100
>+#define MPLS_LS_S_SHIFT         8
>+#define MPLS_LS_TTL_MASK        0x000000FF
>+#define MPLS_LS_TTL_SHIFT       0
>+
>+
> #endif /* __NET_PROTO_H_ */
>diff --git a/datapath-windows/ovsext/PacketParser.c
>b/datapath-windows/ovsext/PacketParser.c
>index e01be17..246c603 100644
>--- a/datapath-windows/ovsext/PacketParser.c
>+++ b/datapath-windows/ovsext/PacketParser.c
>@@ -84,8 +84,8 @@ OvsGetPacketBytes(const NET_BUFFER_LIST *nbl,
> 
> NDIS_STATUS
> OvsParseIPv6(const NET_BUFFER_LIST *packet,
>-          OvsFlowKey *key,
>-          POVS_PACKET_HDR_INFO layers)
>+             OvsFlowKey *key,
>+             POVS_PACKET_HDR_INFO layers)
> {
>     UINT16 ofs = layers->l3Offset;
>     IPv6Hdr ipv6HdrStorage;
>@@ -178,8 +178,8 @@ OvsParseIPv6(const NET_BUFFER_LIST *packet,
> 
> VOID
> OvsParseTcp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     TCPHdr tcpStorage;
>     const TCPHdr *tcp = OvsGetTcp(packet, layers->l4Offset, 
>&tcpStorage); @@ -193,8 +193,8 @@ OvsParseTcp(const NET_BUFFER_LIST 
>*packet,
> 
> VOID
> OvsParseUdp(const NET_BUFFER_LIST *packet,
>-         L4Key *flow,
>-         POVS_PACKET_HDR_INFO layers)
>+            L4Key *flow,
>+            POVS_PACKET_HDR_INFO layers)
> {
>     UDPHdr udpStorage;
>     const UDPHdr *udp = OvsGetUdp(packet, layers->l4Offset, 
>&udpStorage); diff --git a/datapath-windows/ovsext/PacketParser.h
>b/datapath-windows/ovsext/PacketParser.h
>index 55d110f..96136b7 100644
>--- a/datapath-windows/ovsext/PacketParser.h
>+++ b/datapath-windows/ovsext/PacketParser.h
>@@ -141,4 +141,11 @@ OvsGetIcmp(const NET_BUFFER_LIST *packet,
>     return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);  
>}
> 
>+static const MPLSHdr *
>+OvsGetMpls(const NET_BUFFER_LIST *packet,
>+           UINT32 ofs,
>+           MPLSHdr *storage)
>+{
>+    return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage); }
> #endif /* __PACKET_PARSER_H_ */
>--
>1.9.0.msysgit.0
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mai
>lma 
>n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r
>=pN 
>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=nTWi1YZmmSTMCtjYpqMkthOFY5x
>Bca i4dNH_JYc9kKs&s=h1Rf2WEWL_eMR1TjEUbBPzkw7OEWRVhCukH_1uWl9NY&e=
diff mbox

Patch

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index ce592b3..9ee1763 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1074,6 +1074,142 @@  OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
     return NDIS_STATUS_SUCCESS;
 }
 
+static __inline NDIS_STATUS
+OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
+                  const struct ovs_action_push_mpls *mpls)
+{
+    NDIS_STATUS status;
+    PNET_BUFFER curNb = NULL;
+    PMDL curMdl = NULL;
+    PUINT8 bufferStart = NULL;
+    OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
+    EthHdr *ethHdr = NULL;
+    MPLSHdr *mplsHdr = NULL;
+    UINT32 mdlLen = 0, curMdlOffset = 0;
+    UINT32 packetLen = 0;
+    PNET_BUFFER_LIST newNbl;
+
+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
+    ASSERT(curNb->Next == NULL);
+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
+    if (!bufferStart) {
+        ovsActionStats.noResource++;
+        return NDIS_STATUS_RESOURCES;
+    }
+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+    mdlLen -= curMdlOffset;
+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+    ASSERT(mdlLen > 0);
+    /* Bail out if the L2 header is not in a contiguous buffer. */
+    if (MIN(packetLen, mdlLen) < sizeof *ethHdr) {
+        ASSERT(FALSE);
+        return NDIS_STATUS_FAILURE;
+    }
+    ASSERT((INT)mdlLen >= 0);
+
+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl,
+                               MPLS_HLEN, 0, TRUE /*copy NBL info*/);
+    if (!newNbl) {
+        ovsActionStats.noCopiedNbl++;
+        return NDIS_STATUS_RESOURCES;
+    }
+    OvsCompleteNBLForwardingCtx(ovsFwdCtx,
+                                L"Complete after partial copy.");
+
+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
+                                  NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
+                                  NULL, &ovsFwdCtx->layers, FALSE);
+    if (status != NDIS_STATUS_SUCCESS) {
+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
+                                    L"OVS-Dropped due to resources");
+        return NDIS_STATUS_RESOURCES;
+    }
+
+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
+    ASSERT(curNb->Next == NULL);
+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
+    if (!curMdl) {
+        ovsActionStats.noResource++;
+        return NDIS_STATUS_RESOURCES;
+    }
+    curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+    mdlLen -= curMdlOffset;
+    ASSERT(mdlLen >= MPLS_HLEN);
+
+    ethHdr = (EthHdr *)(bufferStart + curMdlOffset);
+    ethHdr->Type = mpls->mpls_ethertype;
+
+    mplsHdr = (MPLSHdr *)(bufferStart + curMdlOffset + MPLS_HLEN);
+    mplsHdr->mpls_lse = mpls->mpls_lse;
+
+    layers->l3Offset += MPLS_HLEN;
+    layers->isIPv4 = 0;
+    layers->isIPv6 = 0;
+
+    NdisRetreatNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
+
+    return NDIS_STATUS_SUCCESS;
+}
+
+static __inline NDIS_STATUS
+OvsActionMplsPop(OvsForwardingContext *ovsFwdCtx)
+{
+    PNET_BUFFER curNb;
+    PMDL curMdl;
+    PUINT8 bufferStart;
+    ULONG dataLength = sizeof (DL_EUI48) + sizeof (DL_EUI48);
+    UINT32 packetLen, mdlLen;
+    PNET_BUFFER_LIST newNbl;
+    NDIS_STATUS status;
+
+    PUINT8 tempBuffer[sizeof (DL_EUI48) + sizeof (DL_EUI48)];
+
+    newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl,
+                               0, 0, TRUE /* copy NBL info */);
+    if (!newNbl) {
+        ovsActionStats.noCopiedNbl++;
+        return NDIS_STATUS_RESOURCES;
+    }
+
+    /* Complete the original NBL and create a copy to modify. */
+    OvsCompleteNBLForwardingCtx(ovsFwdCtx, L"OVS-Dropped due to copy");
+
+    status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
+                                  newNbl, ovsFwdCtx->srcVportNo, 0,
+                                  NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
+                                  NULL, &ovsFwdCtx->layers, FALSE);
+    if (status != NDIS_STATUS_SUCCESS) {
+        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
+                                    L"Dropped due to resouces");
+        return NDIS_STATUS_RESOURCES;
+    }
+
+    curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
+    packetLen = NET_BUFFER_DATA_LENGTH(curNb);
+    ASSERT(curNb->Next == NULL);
+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
+    NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
+    if (!bufferStart) {
+        return NDIS_STATUS_RESOURCES;
+    }
+    mdlLen -= NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+    /* Bail out if L2 + MPLS header is not contiguous in the first buffer. */
+    if (MIN(packetLen, mdlLen) < sizeof(EthHdr) + MPLS_HLEN) {
+        ASSERT(FALSE);
+        return NDIS_STATUS_FAILURE;
+    }
+    bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+    RtlCopyMemory(tempBuffer, bufferStart, dataLength);
+    RtlCopyMemory(bufferStart + MPLS_HLEN, tempBuffer, dataLength);
+    NdisAdvanceNetBufferDataStart(curNb, MPLS_HLEN, FALSE, NULL);
+
+    return NDIS_STATUS_SUCCESS;
+}
+
 /*
  * --------------------------------------------------------------------------
  * OvsTunnelAttrToIPv4TunnelKey --
@@ -1513,6 +1649,46 @@  OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
             break;
         }
 
+        case OVS_ACTION_ATTR_PUSH_MPLS:
+        {
+            if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic != NULL
+                || ovsFwdCtx.tunnelRxNic != NULL) {
+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
+                if (status != NDIS_STATUS_SUCCESS) {
+                    dropReason = L"OVS-adding destination failed";
+                    goto dropit;
+                }
+            }
+
+            status = OvsActionMplsPush(&ovsFwdCtx,
+                                       (struct ovs_action_push_mpls *)NlAttrGet
+                                       ((const PNL_ATTR)a));
+            if (status != NDIS_STATUS_SUCCESS) {
+                dropReason = L"OVS-set push MPLS failed";
+                goto dropit;
+            }
+            break;
+        }
+
+        case OVS_ACTION_ATTR_POP_MPLS:
+        {
+            if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic != NULL
+                || ovsFwdCtx.tunnelRxNic != NULL) {
+                status = OvsOutputBeforeSetAction(&ovsFwdCtx);
+                if (status != NDIS_STATUS_SUCCESS) {
+                    dropReason = L"OVS-adding destination failed";
+                    goto dropit;
+                }
+            }
+
+            status = OvsActionMplsPop(&ovsFwdCtx);
+            if (status != NDIS_STATUS_SUCCESS) {
+                dropReason = L"OVS-set pop MPLS failed";
+                goto dropit;
+            }
+            break;
+        }
+
         case OVS_ACTION_ATTR_USERSPACE:
         {
             PNL_ATTR userdataAttr;
diff --git a/datapath-windows/ovsext/DpInternal.h b/datapath-windows/ovsext/DpInternal.h
index 8de48a2..c195494 100644
--- a/datapath-windows/ovsext/DpInternal.h
+++ b/datapath-windows/ovsext/DpInternal.h
@@ -20,6 +20,7 @@ 
 #include <netioapi.h>
 #define IFNAMSIZ IF_NAMESIZE
 #include "../ovsext/Netlink/Netlink.h"
+#include "NetProto.h"
 
 #define OVS_DP_NUMBER   ((uint32_t) 0)
 
@@ -149,6 +150,11 @@  typedef union OvsIPv4TunnelKey {
     uint64_t attr[NUM_PKT_ATTR_REQUIRED];
 } OvsIPv4TunnelKey;
 
+typedef struct MplsKey {
+    ovs_be32 top_lse;            /* MPLS topmost label stack entry. */
+    uint8    pad[4];
+} MplsKey; /* Size of 8 bytes. */
+
 typedef __declspec(align(8)) struct OvsFlowKey {
     OvsIPv4TunnelKey tunKey;     /* 24 bytes */
     L2Key l2;                    /* 24 bytes */
@@ -157,6 +163,7 @@  typedef __declspec(align(8)) struct OvsFlowKey {
         ArpKey arpKey;           /* size 24 */
         Ipv6Key ipv6Key;         /* size 48 */
         Icmp6Key icmp6Key;       /* size 72 */
+        MplsKey mplsKey;         /* size 8 */
     };
 } OvsFlowKey;
 
diff --git a/datapath-windows/ovsext/Ethernet.h b/datapath-windows/ovsext/Ethernet.h
index 22aa27c..1d69d47 100644
--- a/datapath-windows/ovsext/Ethernet.h
+++ b/datapath-windows/ovsext/Ethernet.h
@@ -66,6 +66,8 @@  typedef enum {
     ETH_TYPE_CDP         = 0x2000,
     ETH_TYPE_802_1PQ     = 0x8100, // not really a DIX type, but used as such
     ETH_TYPE_LLC         = 0xFFFF, // 0xFFFF is IANA reserved, used to mark LLC
+    ETH_TYPE_MPLS        = 0x8847,
+    ETH_TYPE_MPLS_MCAST  = 0x8848,
 } Eth_DixType;
 
 typedef enum {
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index b629c93..c989c14 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -80,6 +80,8 @@  static NTSTATUS _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf,
                                        Icmp6Key *ipv6FlowPutIcmpKey);
 static NTSTATUS _MapFlowArpKeyToNlKey(PNL_BUFFER nlBuf,
                                       ArpKey *arpFlowPutKey);
+static NTSTATUS _MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf,
+                                       MplsKey *mplsFlowPutKey);
 
 static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
                                OvsFlowDumpOutput *dumpOutput,
@@ -108,7 +110,7 @@  const NL_POLICY nlFlowPolicy[] = {
 
 /* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
  * Some of the attributes like OVS_KEY_ATTR_RECIRC_ID
- * & OVS_KEY_ATTR_MPLS are not supported yet. */
+ * are not supported yet. */
 
 const NL_POLICY nlFlowKeyPolicy[] = {
     [OVS_KEY_ATTR_ENCAP] = {.type = NL_A_VAR_LEN, .optional = TRUE},
@@ -872,6 +874,13 @@  MapFlowKeyToNlKey(PNL_BUFFER nlBuf,
         break;
         }
 
+        case ETH_TYPE_MPLS:
+        case ETH_TYPE_MPLS_MCAST: {
+        MplsKey *mplsFlowPutKey = &(flowKey->mplsKey);
+        rc = _MapFlowMplsKeyToNlKey(nlBuf, mplsFlowPutKey);
+        break;
+        }
+
         default:
         break;
     }
@@ -1194,6 +1203,31 @@  done:
 
 /*
  *----------------------------------------------------------------------------
+ *  _MapFlowMplsKeyToNlKey --
+ *    Maps _MapFlowMplsKeyToNlKey to OVS_KEY_ATTR_MPLS attribute.
+ *----------------------------------------------------------------------------
+ */
+static NTSTATUS
+_MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf, MplsKey *mplsFlowPutKey)
+{
+    NTSTATUS rc = STATUS_SUCCESS;
+    struct ovs_key_mpls *mplsKey;
+
+    mplsKey = (struct ovs_key_mpls *)
+        NlMsgPutTailUnspecUninit(nlBuf, OVS_KEY_ATTR_MPLS, sizeof(*mplsKey));
+    if (!mplsKey) {
+        rc = STATUS_UNSUCCESSFUL;
+        goto done;
+    }
+
+    mplsKey->mpls_lse = mplsFlowPutKey->top_lse;
+
+done:
+    return rc;
+}
+
+/*
+ *----------------------------------------------------------------------------
  *  _MapNlToFlowPut --
  *    Maps input netlink message to OvsFlowPut.
  *----------------------------------------------------------------------------
@@ -1469,8 +1503,28 @@  _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
             arpFlowPutKey->pad[1] = 0;
             arpFlowPutKey->pad[2] = 0;
             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
-            break;
         }
+        break;
+    }
+    case ETH_TYPE_MPLS:
+    case ETH_TYPE_MPLS_MCAST: {
+
+        if (keyAttrs[OVS_KEY_ATTR_MPLS]) {
+            MplsKey *mplsFlowPutKey = &destKey->mplsKey;
+            const struct ovs_key_mpls *mplsKey;
+            UINT32 size = NlAttrGetSize(keyAttrs[OVS_KEY_ATTR_MPLS]);
+            UINT32 n = size / sizeof(struct ovs_key_mpls);
+
+            mplsKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_MPLS]);
+
+            mplsFlowPutKey->top_lse = mplsKey->mpls_lse;
+            mplsFlowPutKey->pad[0] = 0;
+            mplsFlowPutKey->pad[1] = 0;
+            mplsFlowPutKey->pad[2] = 0;
+            mplsFlowPutKey->pad[3] = 0;
+            destKey->l2.keyLen += (UINT16)n * sizeof(MplsKey);
+        }
+        break;
     }
     }
 }
@@ -1864,6 +1918,37 @@  OvsExtractFlow(const NET_BUFFER_LIST *packet,
                 memcpy(arpKey->arpTha, arp->arp_tha, ETH_ADDR_LENGTH);
             }
         }
+    } else if (flow->l2.dlType == htons(ETH_TYPE_MPLS) ||
+               flow->l2.dlType == htons(ETH_TYPE_MPLS_MCAST)) {
+        MPLSHdr mplsStorage;
+        const MPLSHdr *mpls;
+        MplsKey *mplsKey = &flow->mplsKey;
+        ((UINT64 *)mplsKey)[0] = 0;
+
+        /* In the presence of an MPLS label stack the end of the L2
+         * header and the beginning of the L3 header differ.
+         *
+         * A network packet may contain multiple MPLS labels, but we
+         * are only interested in the topmost label stack entry.
+         *
+         * Advance network header to the beginning of the L3 header.
+         * layers->l3Offset corresponds to the end of the L2 header.
+         */
+        for (UINT32 i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
+            mpls = OvsGetMpls(packet, layers->l3Offset, &mplsStorage);
+            if (mpls) {
+
+                /* Keep only the topmost MPLS label stack entry. */
+                if (i == 0) {
+                    mplsKey->top_lse = mpls->mpls_lse;
+                }
+
+                layers->l3Offset += sizeof(MPLSHdr);
+
+                if (mpls->mpls_lse & htonl(MPLS_LS_S_MASK))
+                    break;
+            }
+        }
     }
 
     return NDIS_STATUS_SUCCESS;
diff --git a/datapath-windows/ovsext/NetProto.h b/datapath-windows/ovsext/NetProto.h
index a364869..12e8bd9 100644
--- a/datapath-windows/ovsext/NetProto.h
+++ b/datapath-windows/ovsext/NetProto.h
@@ -366,4 +366,37 @@  typedef struct IPOpt {
 #define SOCKET_IPPROTO_UDP   17
 #define SOCKET_IPPROTO_GRE   47
 
+/* Reference: RFC 5462, RFC 3032
+ *
+ *  0                   1                   2                   3
+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                Label                  | TC  |S|       TTL     |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ *	Label:  Label Value, 20 bits
+ *	TC:     Traffic Class field, 3 bits
+ *	S:      Bottom of Stack, 1 bit
+ *	TTL:    Time to Live, 8 bits
+ */
+
+typedef struct MPLSHdr {
+    ovs_be32 mpls_lse;
+} MPLSHdr;
+
+/*
+ * MPLS definitions
+ */
+#define MPLS_HLEN               4
+#define FLOW_MAX_MPLS_LABELS    3
+#define MPLS_LS_LABEL_MASK      0xFFFFF000
+#define MPLS_LS_LABEL_SHIFT     12
+#define MPLS_LS_TC_MASK         0x00000E00
+#define MPLS_LS_TC_SHIFT        9
+#define MPLS_LS_S_MASK          0x00000100
+#define MPLS_LS_S_SHIFT         8
+#define MPLS_LS_TTL_MASK        0x000000FF
+#define MPLS_LS_TTL_SHIFT       0
+
+
 #endif /* __NET_PROTO_H_ */
diff --git a/datapath-windows/ovsext/PacketParser.c b/datapath-windows/ovsext/PacketParser.c
index e01be17..246c603 100644
--- a/datapath-windows/ovsext/PacketParser.c
+++ b/datapath-windows/ovsext/PacketParser.c
@@ -84,8 +84,8 @@  OvsGetPacketBytes(const NET_BUFFER_LIST *nbl,
 
 NDIS_STATUS
 OvsParseIPv6(const NET_BUFFER_LIST *packet,
-          OvsFlowKey *key,
-          POVS_PACKET_HDR_INFO layers)
+             OvsFlowKey *key,
+             POVS_PACKET_HDR_INFO layers)
 {
     UINT16 ofs = layers->l3Offset;
     IPv6Hdr ipv6HdrStorage;
@@ -178,8 +178,8 @@  OvsParseIPv6(const NET_BUFFER_LIST *packet,
 
 VOID
 OvsParseTcp(const NET_BUFFER_LIST *packet,
-         L4Key *flow,
-         POVS_PACKET_HDR_INFO layers)
+            L4Key *flow,
+            POVS_PACKET_HDR_INFO layers)
 {
     TCPHdr tcpStorage;
     const TCPHdr *tcp = OvsGetTcp(packet, layers->l4Offset, &tcpStorage);
@@ -193,8 +193,8 @@  OvsParseTcp(const NET_BUFFER_LIST *packet,
 
 VOID
 OvsParseUdp(const NET_BUFFER_LIST *packet,
-         L4Key *flow,
-         POVS_PACKET_HDR_INFO layers)
+            L4Key *flow,
+            POVS_PACKET_HDR_INFO layers)
 {
     UDPHdr udpStorage;
     const UDPHdr *udp = OvsGetUdp(packet, layers->l4Offset, &udpStorage);
diff --git a/datapath-windows/ovsext/PacketParser.h b/datapath-windows/ovsext/PacketParser.h
index 55d110f..96136b7 100644
--- a/datapath-windows/ovsext/PacketParser.h
+++ b/datapath-windows/ovsext/PacketParser.h
@@ -141,4 +141,11 @@  OvsGetIcmp(const NET_BUFFER_LIST *packet,
     return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
 }
 
+static const MPLSHdr *
+OvsGetMpls(const NET_BUFFER_LIST *packet,
+           UINT32 ofs,
+           MPLSHdr *storage)
+{
+    return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
+}
 #endif /* __PACKET_PARSER_H_ */