[ofw] RE: [RFC] [PATCH 7/12] winverbs: implement IWVProtectionDomain

[Fab Tillier]

>
>>Userspace WinVerb implementation for protection domains, address
>>handles, and memory windows.  Most calls are implemented, except
>>for memory registration, which requires more thought.
>
>I've implemented memory registration as follows.  Note that proposed WV API is
>modified slightly to be more efficient.
>
>STDMETHOD(RegisterMemory)(
>        THIS_
>        __in_bcount(BufferLength) const VOID* pBuffer,
>        __in SIZE_T BufferLength,
>        __in DWORD AccessFlags,
>        __in OVERLAPPED* pOverlapped,
>        __out WV_MEMORY_KEYS* pKeys
>        ) PURE;
>
>typedef struct _WV_MEMORY_KEYS
>{
>        UINT32                  Lkey;
>        UINT32                  Rkey;
>
>}       WV_MEMORY_KEYS;
>
>// User must call PD:Release() on asynchronous failure or
>// after DeregisterMemory() completes.

Why do you need this reference?  Is it to prevent having to clean up MRs when a PD is destroyed with MRs still existing?  Requiring an application to call Release in case of error really stinks.

>STDMETHODIMP CWVProtectionDomain::
>RegisterMemory(const VOID* pBuffer, SIZE_T BufferLength, DWORD AccessFlags,
>                   OVERLAPPED* pOverlapped, WV_MEMORY_KEYS *pKeys)
>{
>        WV_IO_MEMORY_REGISTER   reg;
>        DWORD                           bytes;
>        HRESULT                 hr;
>
>        AddRef();
>        reg.Id = m_Id;
>        reg.Address = (UINT64) (ULONG_PTR) pBuffer;
>        reg.BufferLength = BufferLength;
>        reg.AccessFlags = AccessFlags;
>        reg.Reserved = 0;
>
>        if (DeviceIoControl(m_hFile, WV_IOCTL_MEMORY_REGISTER, &reg, sizeof reg,
>                                pKeys, sizeof WV_MEMORY_KEYS, &bytes,
>pOverlapped)) {
>                hr = WV_SUCCESS;
>        } else {
>                hr = HRESULT_FROM_WIN32(GetLastError());
>                Release();

Note that if the request is pending, DeviceIoControl will return FALSE, and GetLastError will return ERROR_IO_PENDING.  In that case you don't want to release the PD.

>        }
>
>        return hr;
>}
>
>// User must call PD::Release() after operation completes.
>STDMETHODIMP CWVProtectionDomain::
>DeregisterMemory(UINT32 Lkey, OVERLAPPED* pOverlapped)
>{
>        WV_IO_ID        id;
>        DWORD           bytes;
>        HRESULT         hr;
>
>        id.Id = m_Id;
>        id.Data = Lkey;
>        hr = DeviceIoControl(m_hFile, WV_IOCTL_MEMORY_DEREGISTER, &id, sizeof
>id,
>                                 NULL, 0, &bytes, pOverlapped) ?
>                                 WV_SUCCESS :
>HRESULT_FROM_WIN32(GetLastError());

I think a more traditional if/else here would be better.  You can just have the body of the if and else return the appropriate value rather than setting the local hr variable.

>        return hr;
>}