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

[Sean Hefty]

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

> +/*
> + *  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