[openib-general] [PATCH 3/7] AMSO1100 Work Request Definitions REPOST

[Steve Wise]

On Fri, 2006-03-24 at 09:48 -0800, Sean Hefty wrote:
> Tom Tucker wrote:
> > +#define PTR_TO_CTX(p) (u64)(u32)(p)
> > +
> > +#define C2_PTR_TO_64(p) (u64)(u32)(p)
> > +#define C2_64_TO_PTR(c) (void*)(u32)(c)
> 
> Should these use (unsigned long) instead of (u32)?
> 

Yes.  This won't work with 64b ptrs.  These used to be unsigned longs.
I think it got changed by mistake...

> > +/*
> > + *  CCIL Work Request Identifiers
> > + */
> > +enum c2wr_ids {
> > +	CCWR_RNIC_OPEN = 1,
> > +	CCWR_RNIC_QUERY,
> > +	CCWR_RNIC_SETCONFIG,
> > +	CCWR_RNIC_GETCONFIG,
> > +	CCWR_RNIC_CLOSE,
> > +	CCWR_CQ_CREATE,
> > +	CCWR_CQ_QUERY,
> > +	CCWR_CQ_MODIFY,
> > +	CCWR_CQ_DESTROY,
> > +	CCWR_QP_CONNECT,
> > +	CCWR_PD_ALLOC,
> > +	CCWR_PD_DEALLOC,
> > +	CCWR_SRQ_CREATE,
> > +	CCWR_SRQ_QUERY,
> > +	CCWR_SRQ_MODIFY,
> > +	CCWR_SRQ_DESTROY,
> > +	CCWR_QP_CREATE,
> > +	CCWR_QP_QUERY,
> > +	CCWR_QP_MODIFY,
> > +	CCWR_QP_DESTROY,
> > +	CCWR_NSMR_STAG_ALLOC,
> > +	CCWR_NSMR_REGISTER,
> > +	CCWR_NSMR_PBL,
> > +	CCWR_STAG_DEALLOC,
> > +	CCWR_NSMR_REREGISTER,
> > +	CCWR_SMR_REGISTER,
> > +	CCWR_MR_QUERY,
> > +	CCWR_MW_ALLOC,
> > +	CCWR_MW_QUERY,
> > +	CCWR_EP_CREATE,
> > +	CCWR_EP_GETOPT,
> > +	CCWR_EP_SETOPT,
> > +	CCWR_EP_DESTROY,
> > +	CCWR_EP_BIND,
> > +	CCWR_EP_CONNECT,
> > +	CCWR_EP_LISTEN,
> > +	CCWR_EP_SHUTDOWN,
> > +	CCWR_EP_LISTEN_CREATE,
> > +	CCWR_EP_LISTEN_DESTROY,
> > +	CCWR_EP_QUERY,
> > +	CCWR_CR_ACCEPT,
> > +	CCWR_CR_REJECT,
> > +	CCWR_CONSOLE,
> > +	CCWR_TERM,
> > +	CCWR_FLASH_INIT,
> > +	CCWR_FLASH,
> > +	CCWR_BUF_ALLOC,
> > +	CCWR_BUF_FREE,
> > +	CCWR_FLASH_WRITE,
> > +	CCWR_INIT,		/* WARNING: Don't move this ever again! */
> > +
> > +
> > +
> > +	/* Add new IDs here */
> > +
> > +
> > +
> > +	/* 
> > +	 * WARNING: CCWR_LAST must always be the last verbs id defined! 
> > +	 *          All the preceding IDs are fixed, and must not change.
> > +	 *          You can add new IDs, but must not remove or reorder
> > +	 *          any IDs. If you do, YOU will ruin any hope of
> > +	 *          compatability between versions.
> > +	 */
> > +	CCWR_LAST,
> > +
> > +	/*
> > +	 * Start over at 1 so that arrays indexed by user wr id's
> > +	 * begin at 1.  This is OK since the verbs and user wr id's
> > +	 * are always used on disjoint sets of queues.
> > +	 */
> 
> Should we just have two separate enums here?
> 

Yes, we could if it makes things more understandable.

> > +	/* 
> > +	 * The order of the CCWR_SEND_XX verbs must 
> > +	 * match the order of the RDMA_OPs 
> > +	 */
> > +	CCWR_SEND = 1,
> > +	CCWR_SEND_INV,
> > +	CCWR_SEND_SE,
> > +	CCWR_SEND_SE_INV,
> > +	CCWR_RDMA_WRITE,
> > +	CCWR_RDMA_READ,
> > +	CCWR_RDMA_READ_INV,
> > +	CCWR_MW_BIND,
> > +	CCWR_NSMR_FASTREG,
> > +	CCWR_STAG_INVALIDATE,
> > +	CCWR_RECV,
> > +	CCWR_NOP,
> > +	CCWR_UNIMPL,		
> > +/* WARNING: This must always be the last user wr id defined! */
> > +};
> > +#define RDMA_SEND_OPCODE_FROM_WR_ID(x)   (x+2)
> > +
> > +/*
> > + * SQ/RQ Work Request Types
> > + */
> > +enum c2_wr_type {
> > +	C2_WR_TYPE_SEND = CCWR_SEND,
> > +	C2_WR_TYPE_SEND_SE = CCWR_SEND_SE,
> > +	C2_WR_TYPE_SEND_INV = CCWR_SEND_INV,
> > +	C2_WR_TYPE_SEND_SE_INV = CCWR_SEND_SE_INV,
> > +	C2_WR_TYPE_RDMA_WRITE = CCWR_RDMA_WRITE,
> > +	C2_WR_TYPE_RDMA_READ = CCWR_RDMA_READ,
> > +	C2_WR_TYPE_RDMA_READ_INV_STAG = CCWR_RDMA_READ_INV,
> > +	C2_WR_TYPE_BIND_MW = CCWR_MW_BIND,
> > +	C2_WR_TYPE_FASTREG_NSMR = CCWR_NSMR_FASTREG,
> > +	C2_WR_TYPE_INV_STAG = CCWR_STAG_INVALIDATE,
> > +	C2_WR_TYPE_RECV = CCWR_RECV,
> > +	C2_WR_TYPE_NOP = CCWR_NOP,
> > +};
> 
> I haven't read far enough into the code yet, but why is a second enum required?
> 

This 2nd enum isn't really needed. It was there to fit into the old
Ammasso verbs API.  IE The C2_WR_TYPE fields were exposed as the CCIL
API.  The CCWR_'s are internal.  They can be removed.


> > +struct c2_netaddr {
> > +	u32 ip_addr;
> > +	u32 netmask;
> > +	u32 mtu;
> > +};
> > +
> > +struct c2_route {
> > +	u32 ip_addr;		/* 0 indicates the default route */
> > +	u32 netmask;		/* netmask associated with dst */
> > +	u32 flags;
> > +	union {
> > +		u32 ipaddr;	/* address of the nexthop interface */
> > +		u8 enaddr[6];
> > +	} nexthop;
> > +};
> 
> Does the card support IPv6?
> 

No.

> > +/*
> > + * CCIL API ACF flags defined in terms of the low level mem flags.
> > + * This minimizes translation needed in the user API
> > + */
> > +enum c2_acf {
> > +	C2_ACF_LOCAL_READ = MEM_LOCAL_READ,
> > +	C2_ACF_LOCAL_WRITE = MEM_LOCAL_WRITE,
> > +	C2_ACF_REMOTE_READ = MEM_REMOTE_READ,
> > +	C2_ACF_REMOTE_WRITE = MEM_REMOTE_WRITE,
> > +	C2_ACF_WINDOW_BIND = MEM_WINDOW_BIND
> > +};
> 
> Similar question, is a second enum required, or could we just use MEM_*?
> 

The 2nd enum isn't required...