On Sun, Sep 25 2016 at 10:26:24AM +0300, Leon Romanovsky wrote:
> > On Sat, Sep 24, 2016 at 04:21:26PM -0700, Adit Ranadive wrote:
> > We share some common structures with the user-level driver. This patch adds
> > those structures and shared functions to traverse the QP/CQ rings.
<...>
> > +
> > +#include <linux/types.h>
> > +
> > +#define PVRDMA_UVERBS_ABI_VERSION 3
> > +#define PVRDMA_BOARD_ID 1
> > +#define PVRDMA_REV_ID 1
> >
> > Please don't add defines which you are not using in the library and the
> > two above are not in use.
> >
I'll move these to the pvrdma.h file.
<...>
> > diff --git a/include/uapi/rdma/pvrdma-uapi.h
> > b/include/uapi/rdma/pvrdma-uapi.h
> > new file mode 100644
> > index 0000000..430d8a5
<...>
> > +
> > +#ifndef __PVRDMA_UAPI_H__
> > +#define __PVRDMA_UAPI_H__
> > +
> > +#include <linux/types.h>
> > +
> > +#define PVRDMA_VERSION 17
> >
> > What do you plan to do with this VERSION?
> > How is it related to ABI?
> >
Not related. This is only for the driver to know which APIs to support.
For example, an older driver would still be able to work with a newer
device. I can move this to pvrdma.h as well.
To be honest, I thought I can move this file into the uapi folder since
the structures here are shared with the user-level library. Based on
your comments in this thread and the other ones, I think it makes sense
to move this file back to the pvrdma driver folder and rename it
(pvrdma_wqe.h?) to avoid confusion. There might still be some duplicate
code (especially the UAR offsets and WQE structs) here and in our
user-level library.
Let me know if that makes sense.
> > +
> > +#define PVRDMA_UAR_HANDLE_MASK 0x00FFFFFF /* Bottom 24 bits. */
> > +#define PVRDMA_UAR_QP_OFFSET 0 /* Offset of QP
> > doorbell. */
> > +#define PVRDMA_UAR_QP_SEND BIT(30) /* Send bit. */
> > +#define PVRDMA_UAR_QP_RECV BIT(31) /* Recv bit. */
> > +#define PVRDMA_UAR_CQ_OFFSET 4 /* Offset of CQ
> > doorbell. */
> > +#define PVRDMA_UAR_CQ_ARM_SOL BIT(29) /* Arm solicited bit. */
> > +#define PVRDMA_UAR_CQ_ARM BIT(30) /* Arm bit. */
> > +#define PVRDMA_UAR_CQ_POLL BIT(31) /* Poll bit. */
> > +#define PVRDMA_INVALID_IDX -1 /* Invalid index. */
> >
> > +
> > +/* PVRDMA atomic compare and swap */
> > +struct pvrdma_exp_cmp_swap {
> >
> > _EXP_ looks very similar to MLNX_OFED naming convention.
> >
Yes, the operation was based on that. Any concerns?
I can rename this and the one below.
> > + __u64 swap_val;
> > + __u64 compare_val;
> > + __u64 swap_mask;
> > + __u64 compare_mask;
> > +};
> > +
> > +/* PVRDMA atomic fetch and add */
> > +struct pvrdma_exp_fetch_add {
> >
> > The same as above.
> >
> > + __u64 add_val;
> > + __u64 field_boundary;
> > +};
> > +
> > +/* PVRDMA address vector. */
> > +struct pvrdma_av {
> > + __u32 port_pd;
> > + __u32 sl_tclass_flowlabel;
> > + __u8 dgid[16];
> > + __u8 src_path_bits;
> > + __u8 gid_index;
> > + __u8 stat_rate;
> > + __u8 hop_limit;
> > + __u8 dmac[6];
> > + __u8 reserved[6];
> > +};
> > +
> > +/* PVRDMA scatter/gather entry */
> > +struct pvrdma_sge {
> > + __u64 addr;
> > + __u32 length;
> > + __u32 lkey;
> > +};
> > +
> > +/* PVRDMA receive queue work request */
> > +struct pvrdma_rq_wqe_hdr {
> > + __u64 wr_id; /* wr id */
> > + __u32 num_sge; /* size of s/g array */
> > + __u32 total_len; /* reserved */
> > +};
> > +/* Use pvrdma_sge (ib_sge) for receive queue s/g array elements. */
> > +
> > +/* PVRDMA send queue work request */
> > +struct pvrdma_sq_wqe_hdr {
> > + __u64 wr_id; /* wr id */
> > + __u32 num_sge; /* size of s/g array */
> > + __u32 total_len; /* reserved */
> > + __u32 opcode; /* operation type */
> > + __u32 send_flags; /* wr flags */
> > + union {
> > + __u32 imm_data;
> > + __u32 invalidate_rkey;
> > + } ex;
> > + __u32 reserved;
> > + union {
> > + struct {
> > + __u64 remote_addr;
> > + __u32 rkey;
> > + __u8 reserved[4];
> > + } rdma;
> > + struct {
> > + __u64 remote_addr;
> > + __u64 compare_add;
> > + __u64 swap;
> > + __u32 rkey;
> > + __u32 reserved;
> > + } atomic;
> > + struct {
> > + __u64 remote_addr;
> > + __u32 log_arg_sz;
> > + __u32 rkey;
> > + union {
> > + struct pvrdma_exp_cmp_swap cmp_swap;
> > + struct pvrdma_exp_fetch_add fetch_add;
> > + } wr_data;
> > + } masked_atomics;
> > + struct {
> > + __u64 iova_start;
> > + __u64 pl_pdir_dma;
> > + __u32 page_shift;
> > + __u32 page_list_len;
> > + __u32 length;
> > + __u32 access_flags;
> > + __u32 rkey;
> > + } fast_reg;
> > + struct {
> > + __u32 remote_qpn;
> > + __u32 remote_qkey;
> > + struct pvrdma_av av;
> > + } ud;
> > + } wr;
> > +};
> >
> > No, I have half-baked patch series which refactors this structure in kernel.
> > There is no need to put this structure in UAPI.
> >
This is specific to our device.. We do need to enqueue the WQE in this format
for the device to recognize it. This is the same format that the user-level
library will put the WQE in. As I said above, we can move this to the main
pvrdma driver directory if you prefer.
> > +/* Use pvrdma_sge (ib_sge) for send queue s/g array elements. */
> > +
> > +/* Completion queue element. */
> > +struct pvrdma_cqe {
> > + __u64 wr_id;
> > + __u64 qp;
> > + __u32 opcode;
> > + __u32 status;
> > + __u32 byte_len;
> > + __u32 imm_data;
> > + __u32 src_qp;
> > + __u32 wc_flags;
> > + __u32 vendor_err;
> > + __u16 pkey_index;
> > + __u16 slid;
> > + __u8 sl;
> > + __u8 dlid_path_bits;
> > + __u8 port_num;
> > + __u8 smac[6];
> > + __u8 reserved2[7]; /* Pad to next power of 2 (64). */
> > +};
> > +
> > +struct pvrdma_ring {
> > + atomic_t prod_tail; /* Producer tail. */
> > + atomic_t cons_head; /* Consumer head. */
> > +};
> > +
> > +struct pvrdma_ring_state {
> > + struct pvrdma_ring tx; /* Tx ring. */
> > + struct pvrdma_ring rx; /* Rx ring. */
> > +};
> > +
> > +static inline int pvrdma_idx_valid(__u32 idx, __u32 max_elems)
> > +{
> > + /* Generates fewer instructions than a less-than. */
> > + return (idx & ~((max_elems << 1) - 1)) == 0;
> > +}
> > +
> > +static inline __s32 pvrdma_idx(atomic_t *var, __u32 max_elems)
> > +{
> > + const unsigned int idx = atomic_read(var);
> > +
> > + if (pvrdma_idx_valid(idx, max_elems))
> > + return idx & (max_elems - 1);
> > + return PVRDMA_INVALID_IDX;
> > +}
> > +
> > +static inline void pvrdma_idx_ring_inc(atomic_t *var, __u32 max_elems)
> > +{
> > + __u32 idx = atomic_read(var) + 1; /* Increment. */
> >
> > It is definitely different atomic_read than you expect. From my grep
> > searches on my machine, linux kernel doesn't export in standard headers
> > the atomic_* functions and C has their implementation of that functions.
> >
This would probably change for the user-level library, so no need have this file
in UAPI.
> > +
> > + idx &= (max_elems << 1) - 1; /* Modulo size, flip gen. */
> > + atomic_set(var, idx);
> > +}
> > +
> > +static inline __s32 pvrdma_idx_ring_has_space(const struct pvrdma_ring *r,
> > + __u32 max_elems, __u32 *out_tail)
> > +{
> > + const __u32 tail = atomic_read(&r->prod_tail);
> > + const __u32 head = atomic_read(&r->cons_head);
> > +
> > + if (pvrdma_idx_valid(tail, max_elems) &&
> > + pvrdma_idx_valid(head, max_elems)) {
> > + *out_tail = tail & (max_elems - 1);
> > + return tail != (head ^ max_elems);
> > + }
> > + return PVRDMA_INVALID_IDX;
> > +}
> > +
> > +static inline __s32 pvrdma_idx_ring_has_data(const struct pvrdma_ring *r,
> > + __u32 max_elems, __u32 *out_head)
> > +{
> > + const __u32 tail = atomic_read(&r->prod_tail);
> > + const __u32 head = atomic_read(&r->cons_head);
> > +
> > + if (pvrdma_idx_valid(tail, max_elems) &&
> > + pvrdma_idx_valid(head, max_elems)) {
> > + *out_head = head & (max_elems - 1);
> > + return tail != head;
> > + }
> > + return PVRDMA_INVALID_IDX;
> > +}
> > +
> > +static inline bool pvrdma_idx_ring_is_valid_idx(const struct pvrdma_ring
> > *r,
> > + __u32 max_elems, __u32 *idx)
> > +{
> > + const __u32 tail = atomic_read(&r->prod_tail);
> > + const __u32 head = atomic_read(&r->cons_head);
> > +
> > + if (pvrdma_idx_valid(tail, max_elems) &&
> > + pvrdma_idx_valid(head, max_elems) &&
> > + pvrdma_idx_valid(*idx, max_elems)) {
> > + if (tail > head && (*idx < tail && *idx >= head))
> > + return true;
> > + else if (head > tail && (*idx >= head || *idx < tail))
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > +#endif /* __PVRDMA_UAPI_H__ */
> >
> > I suggest completely remove this file from UAPI headers folder.
> >
I can move this back to the pvrdma driver folder.
