[ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

Hi Amber,

The patch fails to apply:
"error: corrupt patch at line 198"

Please rebase on master.

Trimming the patch a bit and comments inlined.
> -----Original Message-----
> From: Amber Hu <qhu at vmware.com>
> Sent: Tuesday, January 14, 2020 7:50 AM
> To: dev at openvswitch.org; Alin Serdean <aserdean at cloudbasesolutions.com>
> Cc: Qiong Wang <wqiong at vmware.com>; Wenyu Zhang
> <wenyuz at vmware.com>; Jinjun Gao <jinjung at vmware.com>
> Subject: [patch] datapath-windows: Append tunnel info to upcall for correct
> template
> 

> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
> windows/ovsext/Actions.c
> 
> index 0c4d64b..936e68b 100644
> 
> --- a/datapath-windows/ovsext/Actions.c
> 
> +++ b/datapath-windows/ovsext/Actions.c
> 
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>                           const PNL_ATTR attr)
> 
> {
> 
>      NTSTATUS status = NDIS_STATUS_SUCCESS;
> 
> -    PNL_ATTR userdataAttr;
> 
> -    PNL_ATTR queueAttr;
 [Alin Serdean] queueAttr can be safely removed since it's not actually used.
> 
> +    PNL_ATTR userdataAttr = NULL;
> 
> +    PNL_ATTR queueAttr = NULL;
> 
> +    PNL_ATTR egrTunAttr = NULL;
> 
> +    PNL_ATTR a = NULL;
[Alin Serdean] Not sure why did is needed. NlAttrFindNested does the safe version
of NL_ATTR_FOR_EACH_UNSAFE. Any reason to change it?
> 
>      POVS_PACKET_QUEUE_ELEM elem;
> 
>      POVS_PACKET_HDR_INFO layers = &ovsFwdCtx->layers;
> 
>      BOOLEAN isRecv = FALSE;
> 
> +    INT rem = 0;
> 
> +    OVS_FWD_INFO fwdInfo;
> 
> +    OvsIPv4TunnelKey tunKey;
> 
>      POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx-
> >switchContext,
> 
>                                                    ovsFwdCtx->srcVportNo);
> 
> @@ -1855,13 +1860,41 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>          }
> 
>      }
> 
> -    queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> 
> -    userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> 
> +    NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
[Alin Serdean] NlAttrFindNested does the safe version of NL_ATTR_FOR_EACH_UNSAFE.
Any reason to change it?
> 
> +        switch (NlAttrType(a)) {
> 
> +            case OVS_USERSPACE_ATTR_PID:
> 
> +                queueAttr = a;
> 
> +                break;
> 
> +            case OVS_USERSPACE_ATTR_USERDATA:
> 
> +                userdataAttr = a;
> 
> +                break;
> 
> +            case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> 
> +                /* Indicate the packet is from egress tunnel port */
> 
> +                egrTunAttr = a;
> 
> +                break;
> 
> +            default:
> 
> +                break;
> 
> +        }
> 
> +    }
> 
> +
> 
> +    /* Fill tunnel key to export to usersspace to calculate the template id */
> 
> +    if (egrTunAttr) {
> 
[Alin Serdean] It would be safer to zero out tunKey before this.
> +        RtlCopyMemory(&tunKey, &ovsFwdCtx->tunKey, sizeof tunKey);
> 
> +        if (!tunKey.src) {
> 
> +            status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, &fwdInfo);
> 
> +            if (status == NDIS_STATUS_SUCCESS && tunKey.dst ==
> fwdInfo.dstIpAddr) {
> 
> +                tunKey.src = fwdInfo.srcIpAddr;
> 
> +            }
> 
> +        }
> 
> +        tunKey.flow_hash = tunKey.flow_hash?tunKey.flow_hash:MAXINT16;
[Alin Serdean] Missing a whitespace before and after '?'
> 
> +    }
> 
>      elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
> 
>                                    NlAttrGetSize(userdataAttr),
> 
>                                    OVS_PACKET_CMD_ACTION,
> 
> -                                  vport, key, ovsFwdCtx->curNbl,
> 
> +                                  vport, key,
> 
> +                                  egrTunAttr?&(tunKey):NULL,
[Alin Serdean] Missing whitespace before and after '?'
> 
> +                                  ovsFwdCtx->curNbl,
> 
>                                    NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl),
> 
>                                    isRecv,
> 
>                                    layers);
> 
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-
> windows/ovsext/Flow.c
> 
> index fdb1010..ac0582c 100644
> 
> --- a/datapath-windows/ovsext/Flow.c
> 
> +++ b/datapath-windows/ovsext/Flow.c
> 
> @@ -1094,6 +1094,18 @@ MapFlowTunKeyToNlKey(PNL_BUFFER nlBuf,
> 
>          goto done;
> 
>      }
> 
> +    if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_SRC,
> 
> +                         tunKey->flow_hash)) {
> 
> +        rc = STATUS_UNSUCCESSFUL;
> 
> +        goto done;
> 
> +    }
> 
> +
> 
> +    if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_DST,
> 
> +                         tunKey->dst_port)) {
> 
> +        rc = STATUS_UNSUCCESSFUL;
> 
> +        goto done;
> 
> +    }
> 
> +
> 
> done:
> 
>      NlMsgEndNested(nlBuf, offset);
> 
> error_nested_start:
> 
> diff --git a/datapath-windows/ovsext/User.c b/datapath-
> windows/ovsext/User.c
> 
> index 16d6fd2..a1d05e1 100644
> 
> --- a/datapath-windows/ovsext/User.c
> 
> +++ b/datapath-windows/ovsext/User.c
> 
> @@ -833,7 +833,7 @@ OvsCreateAndAddPackets(PVOID userData,
> 
>      nb = NET_BUFFER_LIST_FIRST_NB(nbl);
> 
>      while (nb) {
> 
>          elem = OvsCreateQueueNlPacket(userData, userDataLen,
> 
> -                                    cmd, vport, key, nbl, nb,
> 
> +                                    cmd, vport, key, NULL, nbl, nb,
> 
>                                      isRecv, hdrInfo);
> 
>          if (elem) {
> 
>              InsertTailList(list, &elem->link);
> 
> @@ -1016,6 +1016,7 @@ OvsCreateQueueNlPacket(PVOID userData,
> 
>                         UINT32 cmd,
> 
>                         POVS_VPORT_ENTRY vport,
> 
>                         OvsFlowKey *key,
> 
> +                       OvsIPv4TunnelKey *tunnelKey,
> 
>                         PNET_BUFFER_LIST nbl,
> 
>                         PNET_BUFFER nb,
> 
>                         BOOLEAN isRecv,
> 
> @@ -1028,7 +1029,6 @@ OvsCreateQueueNlPacket(PVOID userData,
> 
>      NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
> 
>      PNDIS_NET_BUFFER_LIST_8021Q_INFO vlanInfo = NULL;
> 
>      PVOID vlanTag;
> 
> -    OvsIPv4TunnelKey *tunnelKey = (OvsIPv4TunnelKey *)&key->tunKey;
> 
>      UINT32 pid;
> 
>      UINT32 nlMsgSize;
> 
>      NL_BUFFER nlBuf;
> 
> @@ -1131,6 +1131,12 @@ OvsCreateQueueNlPacket(PVOID userData,
> 
>      }
> 
[Alin Serdean] I guess this can be removed now 😊.
>      /* XXX must send OVS_PACKET_ATTR_EGRESS_TUN_KEY if set by vswtchd */
> 
> +    if (tunnelKey) {
> 
> +        if (MapFlowTunKeyToNlKey(&nlBuf, tunnelKey,
> 
> +                                 OVS_PACKET_ATTR_EGRESS_TUN_KEY) != STATUS_SUCCESS)
> {
> 
> +            goto fail;
> 
> +        }
> 
> +    }
> 
>      if (userData){
> 
>          if (!NlMsgPutTailUnspec(&nlBuf, OVS_PACKET_ATTR_USERDATA,
> 
>                                  userData, (UINT16)userDataLen)) {
> 
> diff --git a/datapath-windows/ovsext/User.h b/datapath-
> windows/ovsext/User.h
> 
> index 3a42888..ccca0ba 100644
> 
> --- a/datapath-windows/ovsext/User.h
> 
> +++ b/datapath-windows/ovsext/User.h
> 
> @@ -75,6 +75,7 @@ POVS_PACKET_QUEUE_ELEM
> OvsCreateQueueNlPacket(PVOID userData,
> 
>                                                UINT32 cmd,
> 
>                                                POVS_VPORT_ENTRY vport,
> 
>                                                OvsFlowKey *key,
> 
> +                                              OvsIPv4TunnelKey *tunnelKey,
> 
>                                                PNET_BUFFER_LIST nbl,
> 
>                                                PNET_BUFFER nb,
> 
>                                                BOOLEAN isRecv,
> 
> --
> 
> 2.6.2
> 
> 
> 
> BR/Amber