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
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
