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

[Tom Tucker]

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, but these macros aren't used anymore and should be removed. 
> 
> > +/*
> > + *  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?
> 
> > +	/* 
> > +	 * 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?
> 
> > +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?
> 
> > +/*
> > + * 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_*?
> 
> > +/*
> > + * WARNING:  All of these structs need to align any 64bit types on   
> > + * 64 bit boundaries!  64bit types include u64 and u64.
> 
> comment typo ...                              ^^^^^^^^^^^
> 
> > +struct c2wr_hdr {
> > +	/* wqe_count is part of the cqe.  It is put here so the
> > +	 * adapter can write to it while the wr is pending without
> > +	 * clobbering part of the wr.  This word need not be dma'd
> > +	 * from the host to adapter by libccil, but we copy it anyway
> > +	 * to make the memcpy to the adapter better aligned.
> > +	 */
> > +	u32 wqe_count;
> > +
> > +	/* Put these fields next so that later 32- and 64-bit
> > +	 * quantities are naturally aligned.
> > +	 */
> > +	u8 id;
> > +	u8 result;		/* adapter -> host */
> > +	u8 sge_count;		/* host -> adapter */
> > +	u8 flags;		/* host -> adapter */
> > +
> > +	u64 context;
> > +#ifdef CCMSGMAGIC
> > +	u32 magic;
> > +	u32 pad;
> > +#endif
> > +} __attribute__((packed));
> 
> Does anyone know if using packed when it's not needed results in less efficient 
> code?
> 
> - Sean