[ofw] WinVerbs v2

[Sean Hefty]

>>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.

Sure - I don't see why not.

>>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.

I can't think of any reason why couldn't merge these status values with
WV_STATUS.

>
>>        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?

It was defined by ibal.  If I can avoid mapping between ibal and winverbs, I'd
like to do that, especially on speed path operations.

>
>>#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'.

Yes - this makes sense

>>#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...

I wasn't going to bother trying to support MAD processing with winverbs, so
SEND_LOCAL should be removed.

>>typedef struct _WV_SGE
>>{
>>        UINT64                  Address;
>
>Why not a pointer?

I think this could be a pointer, assuming that the register call doesn't return
a different hardware address (in which case RegisterMemory would need to return
that).

>
>>        UINT32                  Length;
>
>Why not SIZE_T?

This is an area where I flip-flop.  I don't think any RDMA architectures support
operations larger than 4 GB, but using SIZE_T here seems to make more sense if
the device attributes also use SIZE_T.

>>typedef enum _WV_QP_TYPE
>>{
>>        WvQpTypeRc,
>>        WvQpTypeUc,
>>        WvQpTypeReserved,
>
>What's this for?  Just to reserve the RD Transport Service Type?

correct

>
>>        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?

I don't think these are needed.  The overlapped result should give the necessary
context.

>
>>        WvCmEventRouteResolved,
>>        WvCmEventRouteError,
>
>Same for IWVEndpoint::ResolveRoute?

same here

>>        WvCmEventConnectRequest,
>>        WvCmEventLookupRequest = WvCmEventConnectRequest,
>
>Completion from IWVListen::Accept?

and here

>
>>        WvCmEventConnectResponse,
>>        WvCmEventConnectError,
>>        WvCmEventUnreachable,
>
>Completion from IWVEndpoint::Connect?

and here

>
>>        WvCmEventRejected,
>>        WvCmEventEstablished,
>
>Completion from IWVEndpoint::Accept?

and here

>>        WvCmEventDisconnected,
>>        WvCmEventDeviceRemoval,

these still need some work...

>>        WvCmEventMulticastJoin,
>>        WvCmEventMulticastError

but overlapped will probably work here too

>>
>>}       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.

I was assuming that this would be allocated by the library and freed by the
user.

>
>>        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.

For this call, only 1 SGE is allowed.  The intent is to provide a simpler
interface for what I'm guessing would be the common usage model.  The
SendMessage() method allows for multiple SGEs, as well as additional send
operation control (inline, immediate data, completion notification, etc.).

- Sean