datapath-windows: Update vport add code.

In this patch, we make the following updates to the vport add code:

1. Clarify the roles of the different hash tables, so it is easier to
   read/write code in the future.
2. Update OvsNewVportCmdHandler() to support adding bridge-internal
   ports.
3. Fixes in OvsNewVportCmdHandler() to support adding external port.
   Earlier, we'd hit ASSERTs.
4. I could not figure out way to add a port of type
   OVS_PORT_TYPE_INTERNAL with name "internal" to the confdb using
   ovs-vsctl.exe. And, this is needed in order to add the Hyper-V
   internal port from userspace. To workaround this problem, we treat a
   port of type OVS_PORT_TYPE_NETDEV with name "internal" as a request
   to add the Hyper-V internal port. This is a workaround. The end
   result is that there's a discrepancy between the port type in the
   datpaath v/s confdb, but this has not created any trouble in testing
   so far. If this ends up becoming an issue, we can mark the Hyper-V
   internal port to be of type OVS_PORT_TYPE_NETDEV. No harm.
5. Because of changes indicated in #1, we also update the vport dump
   code to look at the correct hash table for ports added from
   userspace.
6. Add a OvsGetTunnelVport() for convenience.
7. Update ASSERTs() while cleaning up the switch.
8. Nuke OvsGetExternalVport() and OvsGetExternalMtu().

Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Tested-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Acked-by: Ankur Sharma <ankursharma@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

5 files changed, 100 insertions, 101 deletions

diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 55b19a089..1b504a276 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -1513,7 +1513,8 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
                           NDIS_RWL_AT_DISPATCH_LEVEL);
 
-    if (gOvsSwitchContext->numHvVports > 0) {
+    if (gOvsSwitchContext->numHvVports > 0 ||
+            gOvsSwitchContext->numNonHvVports > 0) {
         /* inBucket: the bucket, used for lookup */
         UINT32 inBucket = instance->dumpState.index[0];
         /* inIndex: index within the given bucket, used for lookup */
@@ -1525,7 +1526,7 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
 
         for (i = inBucket; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
             PLIST_ENTRY head, link;
-            head = &(gOvsSwitchContext->portIdHashArray[i]);
+            head = &(gOvsSwitchContext->portNoHashArray[i]);
             POVS_VPORT_ENTRY vport = NULL;
 
             outIndex = 0;
@@ -1537,18 +1538,15 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                  * inIndex + 1 vport from the bucket.
                 */
                 if (outIndex >= inIndex) {
-                    vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portIdLink);
-
-                    if (vport->portNo != OVS_DPPORT_NUMBER_INVALID) {
-                        OvsCreateMsgFromVport(vport, msgIn,
-                                              usrParamsCtx->outputBuffer,
-                                              usrParamsCtx->outputLength,
-                                              gOvsSwitchContext->dpNo);
-                        ++outIndex;
-                        break;
-                    } else {
-                        vport = NULL;
-                    }
+                    vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink);
+
+                    ASSERT(vport->portNo != OVS_DPPORT_NUMBER_INVALID);
+                    OvsCreateMsgFromVport(vport, msgIn,
+                                          usrParamsCtx->outputBuffer,
+                                          usrParamsCtx->outputLength,
+                                          gOvsSwitchContext->dpNo);
+                    ++outIndex;
+                    break;
                 }
 
                 ++outIndex;
@@ -1748,7 +1746,9 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     PCHAR portName;
     ULONG portNameLen;
     UINT32 portType;
-    UINT32 hash;
+    BOOLEAN isBridgeInternal = FALSE;
+    BOOLEAN vportAllocated = FALSE;
+    BOOLEAN addInternalPortAsNetdev = FALSE;
 
     static const NL_POLICY ovsVportPolicy[] = {
         [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE },
@@ -1798,38 +1798,49 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
         goto Cleanup;
     }
 
-    if (portType == OVS_VPORT_TYPE_INTERNAL) {
+    if (portName && portType == OVS_VPORT_TYPE_NETDEV &&
+        !strcmp(OVS_DPPORT_INTERNAL_NAME_A, portName)) {
+        addInternalPortAsNetdev = TRUE;
+    }
+
+    if (portName && portType == OVS_VPORT_TYPE_INTERNAL &&
+        strcmp(OVS_DPPORT_INTERNAL_NAME_A, portName)) {
+        isBridgeInternal = TRUE;
+    }
+
+    if (portType == OVS_VPORT_TYPE_INTERNAL && !isBridgeInternal) {
         vport = gOvsSwitchContext->internalVport;
     } else if (portType == OVS_VPORT_TYPE_NETDEV) {
-        if (!strcmp(portName, "external")) {
-            vport = gOvsSwitchContext->virtualExternalVport;
-        } else {
-            vport = OvsFindVportByHvName(gOvsSwitchContext, portName);
-        }
+        /* External ports can also be looked up like VIF ports. */
+        vport = OvsFindVportByHvName(gOvsSwitchContext, portName);
     } else {
-        /* XXX: change when other tunneling ports are added */
-        ASSERT(portType == OVS_VPORT_TYPE_VXLAN);
-
-        if (gOvsSwitchContext->vxlanVport) {
-            nlError = NL_ERROR_EXIST;
-            goto Cleanup;
-        }
+        ASSERT(OvsIsTunnelVportType(portType) ||
+               (portType == OVS_VPORT_TYPE_INTERNAL && isBridgeInternal));
+        ASSERT(OvsGetTunnelVport(gOvsSwitchContext, portType) == NULL ||
+               !OvsIsTunnelVportType(portType));
 
         vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
         if (vport == NULL) {
             nlError = NL_ERROR_NOMEM;
             goto Cleanup;
         }
+        vportAllocated = TRUE;
 
-        vport->ovsState = OVS_STATE_PORT_CREATED;
+        if (OvsIsTunnelVportType(portType)) {
+            nlError = OvsInitTunnelVport(vport, portType, VXLAN_UDP_PORT);
+        } else {
+            OvsInitBridgeInternalVport(vport);
+        }
 
-        /*
-         * XXX: when we allow configuring the vxlan udp port, we should read
-         * this from vport->options instead!
-        */
-        nlError = OvsInitVxlanTunnel(vport, VXLAN_UDP_PORT);
-        if (nlError != NL_ERROR_SUCCESS) {
-            goto Cleanup;
+        if (nlError == NL_ERROR_SUCCESS) {
+            vport->ovsState = OVS_STATE_CONNECTED;
+            vport->nicState = NdisSwitchNicStateConnected;
+
+            /*
+             * Allow the vport to be deleted, because there is no
+             * corresponding hyper-v switch part.
+             */
+            vport->hvDeleted = TRUE;
         }
     }
 
@@ -1837,21 +1848,21 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
         nlError = NL_ERROR_INVAL;
         goto Cleanup;
     }
-
     if (vport->portNo != OVS_DPPORT_NUMBER_INVALID) {
         nlError = NL_ERROR_EXIST;
         goto Cleanup;
     }
 
-    /* Fill the data in vport */
-    vport->ovsType = portType;
-
+    /* Initialize the vport with OVS specific properties. */
+    if (addInternalPortAsNetdev != TRUE) {
+        vport->ovsType = portType;
+    }
     if (vportAttrs[OVS_VPORT_ATTR_PORT_NO] != NULL) {
         /*
-        * XXX: when we implement the limit for ovs port number to be
-        * MAXUINT16, we'll need to check the port number received from the
-        * userspace.
-        */
+         * XXX: when we implement the limit for ovs port number to be
+         * MAXUINT16, we'll need to check the port number received from the
+         * userspace.
+         */
         vport->portNo = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_PORT_NO]);
     } else {
         vport->portNo = OvsComputeVportNo(gOvsSwitchContext);
@@ -1866,42 +1877,19 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     ASSERT(portNameLen <= OVS_MAX_PORT_NAME_LENGTH);
 
     RtlCopyMemory(vport->ovsName, portName, portNameLen);
-
     /* if we don't have options, then vport->portOptions will be NULL */
     vport->portOptions = vportAttrs[OVS_VPORT_ATTR_OPTIONS];
 
     /*
-    * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath,
-    * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set,
-    * it means we have an array of pids, instead of a single pid.
-    * ATM we assume we have one pid only.
-    */
+     * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath,
+     * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set,
+     * it means we have an array of pids, instead of a single pid.
+     * ATM we assume we have one pid only.
+     */
     vport->upcallPid = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_UPCALL_PID]);
 
-    if (vport->ovsType == OVS_VPORT_TYPE_VXLAN) {
-        gOvsSwitchContext->vxlanVport = vport;
-    } else if (vport->ovsType == OVS_VPORT_TYPE_INTERNAL) {
-        gOvsSwitchContext->internalVport = vport;
-        gOvsSwitchContext->internalPortId = vport->portId;
-    } else if (vport->ovsType == OVS_VPORT_TYPE_NETDEV &&
-               vport->isExternal) {
-        gOvsSwitchContext->virtualExternalVport = vport;
-        gOvsSwitchContext->virtualExternalPortId = vport->portId;
-    }
-
-    /*
-     * insert the port into the hash array of ports: by port number and ovs
-     * and ovs (datapath) port name.
-     * NOTE: OvsJhashWords has portNo as "1" word. This is ok, because the
-     * portNo is stored in 2 bytes only (max port number = MAXUINT16).
-    */
-    hash = OvsJhashWords(&vport->portNo, 1, OVS_HASH_BASIS);
-    InsertHeadList(&gOvsSwitchContext->portNoHashArray[hash & OVS_VPORT_MASK],
-                   &vport->portNoLink);
-
-    hash = OvsJhashBytes(vport->ovsName, portNameLen, OVS_HASH_BASIS);
-    InsertHeadList(&gOvsSwitchContext->ovsPortNameHashArray[hash & OVS_VPORT_MASK],
-                   &vport->ovsNameLink);
+    status = InitOvsVportCommon(gOvsSwitchContext, vport);
+    ASSERT(status == STATUS_SUCCESS);
 
     status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer,
                                    usrParamsCtx->outputLength,
@@ -1916,7 +1904,7 @@ Cleanup:
         POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
             usrParamsCtx->outputBuffer;
 
-        if (vport && vport->ovsType == OVS_VPORT_TYPE_VXLAN) {
+        if (vport && vportAllocated == TRUE) {
             OvsRemoveAndDeleteVport(gOvsSwitchContext, vport);
         }
 
diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c
index a540926e8..2b68037c2 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -433,6 +433,7 @@ OvsUninitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
 
     /* We need to do cleanup for tunnel port here. */
     ASSERT(switchContext->numHvVports == 0);
+    ASSERT(switchContext->numNonHvVports == 0);
 
     NdisFreeRWLock(switchContext->dispatchLock);
     switchContext->dispatchLock = NULL;
@@ -493,12 +494,6 @@ cleanup:
     return status;
 }
 
-PVOID
-OvsGetExternalVport()
-{
-    return gOvsSwitchContext->virtualExternalVport;
-}
-
 
 /*
  * --------------------------------------------------------------------------
diff --git a/datapath-windows/ovsext/Switch.h b/datapath-windows/ovsext/Switch.h
index 11d9df623..8df250089 100644
--- a/datapath-windows/ovsext/Switch.h
+++ b/datapath-windows/ovsext/Switch.h
@@ -133,20 +133,28 @@ typedef struct _OVS_SWITCH_CONTEXT
 
     POVS_VPORT_ENTRY        vxlanVport;
 
-    PLIST_ENTRY             ovsPortNameHashArray;   // based on ovsName
+    /*
+     * 'portIdHashArray' ONLY contains ports that exist on the Hyper-V switch,
+     * namely: VIF (vNIC) ports, external port and Hyper-V internal port.
+     * 'numHvVports' counts the ports in 'portIdHashArray'.
+     *
+     * 'portNoHashArray' ONLY contains ports that are added from OVS userspace,
+     * regardless of whether that port exists on the Hyper-V switch or not.
+     * Tunnel ports and bridge-internal ports are examples of ports that do not
+     * exist on the Hyper-V switch, and 'numNonHvVports' counts such ports in
+     * 'portNoHashArray'.
+     *
+     * 'ovsPortNameHashArray' contains the same entries as 'portNoHashArray' but
+     * hashed on a different key.
+     */
     PLIST_ENTRY             portIdHashArray;        // based on Hyper-V portId
     PLIST_ENTRY             portNoHashArray;        // based on ovs port number
+    PLIST_ENTRY             ovsPortNameHashArray;   // based on ovsName
     PLIST_ENTRY             pidHashArray;           // based on packet pids
     NDIS_SPIN_LOCK          pidHashLock;            // Lock for pidHash table
 
-    /*
-     * 'numPhysicalNics' is the number of physical external NICs.
-     * 'numHvVports' is the number of Hyper-V switch ports added to OVS
-     * via the NDIS callbacks.
-     * 'numNonHvVports' is the number of ports added from userspace that are
-     * not on the Hyper-V switch. Eg. tunnel ports.
-     */
-    UINT32                  numPhysicalNics;
+    UINT32                  numPhysicalNics;        // the number of physical
+                                                    // external NICs.
     UINT32                  numHvVports;
     UINT32                  numNonHvVports;
 
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 11ec07db3..1e8154ed9 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -728,8 +728,6 @@ OvsInitTunnelVport(POVS_VPORT_ENTRY vport,
 {
     NTSTATUS status = STATUS_SUCCESS;
 
-    UNREFERENCED_PARAMETER(dstPort);
-
     vport->isBridgeInternal = FALSE;
     vport->ovsType = ovsType;
     vport->ovsState = OVS_STATE_PORT_CREATED;
@@ -739,8 +737,7 @@ OvsInitTunnelVport(POVS_VPORT_ENTRY vport,
     case OVS_VPORT_TYPE_GRE64:
         break;
     case OVS_VPORT_TYPE_VXLAN:
-        /* Will be enabled in later. */
-        /* status = OvsInitVxlanTunnel(vport, dstPort); */
+        status = OvsInitVxlanTunnel(vport, dstPort);
         break;
     default:
         ASSERT(0);
@@ -889,6 +886,10 @@ InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext,
         switchContext->vxlanVport = vport;
         switchContext->numNonHvVports++;
         break;
+    case OVS_VPORT_TYPE_INTERNAL:
+        if (vport->isBridgeInternal) {
+            switchContext->numNonHvVports++;
+        }
     default:
         break;
     }
@@ -1055,6 +1056,7 @@ OvsInitConfiguredSwitchNics(POVS_SWITCH_CONTEXT switchContext)
             nicParam->NicIndex != 0) {
             POVS_VPORT_ENTRY virtExtVport =
                    (POVS_VPORT_ENTRY)switchContext->virtualExternalVport;
+
             vport = OvsAllocateVport();
             if (vport) {
                 OvsInitPhysNicVport(vport, virtExtVport,
@@ -1125,7 +1127,7 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext)
         LIST_FORALL_SAFE(head, link, next) {
             POVS_VPORT_ENTRY vport;
             vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink);
-            ASSERT(OvsIsTunnelVportType(vport->portType) ||
+            ASSERT(OvsIsTunnelVportType(vport->ovsType) ||
                    (vport->ovsType == OVS_VPORT_TYPE_INTERNAL &&
                     vport->isBridgeInternal));
             OvsRemoveAndDeleteVport(switchContext, vport);
diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/Vport.h
index bf9028e74..b4e4fca06 100644
--- a/datapath-windows/ovsext/Vport.h
+++ b/datapath-windows/ovsext/Vport.h
@@ -179,6 +179,18 @@ OvsIsTunnelVportType(OVS_VPORT_TYPE ovsType)
            ovsType == OVS_VPORT_TYPE_GRE64;
 }
 
+static __inline POVS_VPORT_ENTRY
+OvsGetTunnelVport(POVS_SWITCH_CONTEXT switchContext,
+                  OVS_VPORT_TYPE ovsType)
+{
+    switch(ovsType) {
+    case OVS_VPORT_TYPE_VXLAN:
+        return switchContext->vxlanVport;
+    default:
+        return NULL;
+    }
+}
+
 static __inline BOOLEAN
 OvsIsInternalVportType(OVS_VPORT_TYPE ovsType)
 {
@@ -194,12 +206,6 @@ OvsIsBridgeInternalVport(POVS_VPORT_ENTRY vport)
     return vport->isBridgeInternal == TRUE;
 }
 
-static __inline UINT32
-OvsGetExternalMtu()
-{
-    return ((POVS_VPORT_ENTRY) OvsGetExternalVport())->mtu;
-}
-
 VOID OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext,
                              POVS_VPORT_ENTRY vport);