[ofw] WinVerbs v2

[Fab Tillier]

Hi Sean,

Some comments and questions inline...

>typedef enum _WV_COMPLETION_OPCODE
>{
>        WvWcSend,
>        WvWcRdmaWrite,
>        WvWcRecv,
>        WvWcRdmaRead,
>        WvWcBindWindow,
>        WvWcFetchAdd,
>        WvWcCompareExchange,
>        WvWcRecvRdmaWrite
>
>}       WV_COMPLETION_OPCODE;

Can this simply be WV_OPCODE?  This lets you drop the 'Wc' from the enum values.  So you have WvSend, etc.

>
>typedef enum _WV_COMPLETION_STATUS
>{
>        WvWcSuccess,

Ugh, the WvWc is a pain to type and read.  Any reason not to merge these with the regular status values?  If an app gets a WC with a Status of WV_SUCCESS, do you think that's any less helpful than WvWcSuccess?  Also, if you merge the two it makes it simple to test for failure because you can always use the FAILED() macro consistently throughout the app.

>        WvWcLocalLengthError,
>        WvWcLocalOpError,
>        WvWcLocalProtectionError,
>        WvWcWrFlushedError,
>        WvWcBindWindowError,
>        WvWcRemoteAccessError,
>        WvWcRemoteOpError,
>        WvWcRnrRetryError,
>        WvWcTimeoutRetryError,
>        WvWcRemoteInvalidRequestError,
>        WvWcBadResponseError,
>        WvWcLocalAccessError,
>        WvWcUnknownError
>
>}       WV_COMPLETION_STATUS;
>
>// Completion flags
>#define WV_WC_IMMEDIATE         0x00000001
>// reserved                     0x00000002

What for?

>#define WV_WC_GRH_VALID         0x00000004
>
>typedef struct _WV_COMPLETION
>{
>        void*                   QpContext;

You sometimes use 'void' and sometimes 'VOID'.

>        UINT64                  WrId;
>        WV_COMPLETION_OPCODE    Opcode;
>        UINT32                  Length;
>        UINT64                  VendorCode;
>        WV_COMPLETION_STATUS    Status;
>
>        // Receive completion data
>        DWORD                   Flags;
>        UINT32                  ImmediateData;          // Network byte order
>        UINT32                  SourceQpn;              // Network byte order
>        UINT16                  PkeyIndex;
>        UINT16                  SLid;                   // Network byte order
>        UINT8                   SL;
>        UINT8                   DLidPathBits;
>
>}       WV_COMPLETION;
>
>// Access flags
>#define WV_ACCESS_REMOTE_READ   0x00000001
>#define WV_ACCESS_REMOTE_WRITE  0x00000002

Any reason not to make this set the LOCAL_WRITE bit automatically?  Otherwise anywhere you have WV_ACCESS_REMOTE_WRITE you must also have '| WV_ACCESS_LOCAL_WRITE'.

>#define WV_ACCESS_REMOTE_ATOMIC 0x00000004
>#define WV_ACCESS_LOCAL_WRITE   0x00000008
>#define WV_ACCESS_MW_BIND       0x00000010
>
>// Send queue operation flags
>#define WV_SEND_IMMEDIATE       0X00000001
>#define WV_SEND_FENCE           0X00000002
>#define WV_SEND_SIGNALED        0X00000004
>#define WV_SEND_SOLICITED       0X00000008
>#define WV_SEND_INLINE          0X00000010
>#define WV_SEND_LOCAL           0X00000020

I assume this is for local MAD processing.  Would user-mode ever use this?  I thought local MAD processing was always initiated by the kernel...

>
>typedef struct _WV_SGE
>{
>        UINT64                  Address;

Why not a pointer?

>        UINT32                  Length;

Why not SIZE_T?

>        UINT32                  Lkey;
>
>}       WV_SGE;
>

<snip>

>typedef enum _WV_QP_TYPE
>{
>        WvQpTypeRc,
>        WvQpTypeUc,
>        WvQpTypeReserved,

What's this for?  Just to reserve the RD Transport Service Type?

>        WvQpTypeUd
>
>}       WV_QP_TYPE;
>

<snip>

>typedef enum _WV_CM_EVENT_TYPE
>{
>        WvCmEventAddrResolved,
>        WvCmEventAddrError,

Do you need these?  Wouldn't the completion of the IWVEndpoint::ResolveAddress method serve as the event notification as well as status?

>        WvCmEventRouteResolved,
>        WvCmEventRouteError,

Same for IWVEndpoint::ResolveRoute?

>        WvCmEventConnectRequest,
>        WvCmEventLookupRequest = WvCmEventConnectRequest,

Completion from IWVListen::Accept?

>        WvCmEventConnectResponse,
>        WvCmEventConnectError,
>        WvCmEventUnreachable,

Completion from IWVEndpoint::Connect?

>        WvCmEventRejected,
>        WvCmEventEstablished,

Completion from IWVEndpoint::Accept?

>        WvCmEventDisconnected,
>        WvCmEventDeviceRemoval,
>        WvCmEventMulticastJoin,
>        WvCmEventMulticastError
>
>}       WV_CM_EVENT_TYPE;
>
>#define WV_CM_UDP_QKEY          0x01234567
>
>typedef struct _WV_CONNECT_PARAM
>{
>        const VOID*             pPrivateData;

Is this buffer provided by the user when used as part of the WV_CONNECT_EVENT?  The usage of the event structures isn't clear.

>        SIZE_T                  PrivateDataLength;
>        SIZE_T                  ResponderResources;
>        SIZE_T                  InitiatorDepth;
>        UINT8                   RetryCount;             // Ignored when accepting
>        UINT8                   RnrRetryCount;
>
>}       WV_CONNECT_PARAM;

<snip...>

>        // IWVQueuePair methods
>        STDMETHOD(Notify)(
>                THIS_
>                __in OVERLAPPED* pOverlapped
>                ) PURE;

It might be handy to allow users to filter the notification type they are requesting.

<snip...>

>        // IWVDatagramQueuePair Methods
>        STDMETHOD(Send)(
>                THIS_
>                __in UINT64 WrId,
>                __in IWVAddressHandle* pAddressHandle,
>                __in WV_SGE* pSgl,

Only one SGE allowed?  If so, name it as pSge rather than pSgl.  Otherwise you're missing the nSge parameter.

>                __in DWORD Flags,
>                __in UINT32 DestinationQpn,
>                __in UINT32 DestinationQkey,
>                __in UINT16 PkeyIndex
>                ) PURE;