diff mbox

[rdma-core,RESEND] libqedr: Add iWARP support for user-space lib

Message ID 1504622214-9730-1-git-send-email-Ram.Amrani@cavium.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Amrani, Ram Sept. 5, 2017, 2:36 p.m. UTC 
From: Michal Kalderon <Michal.Kalderon@cavium.com>

Introduce IS_IWARP / IS_ROCE macros for libqedr as there are no global
macros for this in rdma-core.
There is a second doorbell required for RQ.
QP states in user lib are not updated, as they are updated implicitly by
the driver and not driven by modify_qp as they are in ib / roce.
For this reason, decisions based on states are encapsulated in IS_ROCE.

Signed-off-by: Michal Kalderon  <Michal.Kalderon@cavium.com>
---
 providers/qedr/qelr.h       |  2 ++
 providers/qedr/qelr_verbs.c | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 31 insertions(+), 5 deletions(-)

Comments

Jason Gunthorpe Sept. 5, 2017, 3:14 p.m. UTC | #1 
On Tue, Sep 05, 2017 at 05:36:54PM +0300, Ram Amrani wrote:
> +		if (iwarp) {
> +			writel(qp->rq.iwarp_db2_data.raw, qp->rq.iwarp_db2);
> +			mmio_flush_writes();
> +		}

Can you update this driver to use the macros in util/mmio.h and
discard this writel thing?

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Leon Romanovsky Sept. 6, 2017, 3:56 a.m. UTC | #2 
On Tue, Sep 05, 2017 at 09:14:12AM -0600, Jason Gunthorpe wrote:
> On Tue, Sep 05, 2017 at 05:36:54PM +0300, Ram Amrani wrote:
> > +		if (iwarp) {
> > +			writel(qp->rq.iwarp_db2_data.raw, qp->rq.iwarp_db2);
> > +			mmio_flush_writes();
> > +		}
>
> Can you update this driver to use the macros in util/mmio.h and
> discard this writel thing?

And fix failures reported by Travis CI.

Thanks

>
> Thanks,
> Jason
Amrani, Ram Sept. 6, 2017, 9:18 a.m. UTC | #3 
> On Tue, Sep 05, 2017 at 05:36:54PM +0300, Ram Amrani wrote:
> > +		if (iwarp) {
> > +			writel(qp->rq.iwarp_db2_data.raw, qp->rq.iwarp_db2);
> > +			mmio_flush_writes();
> > +		}
> 
> Can you update this driver to use the macros in util/mmio.h and
> discard this writel thing?

Sure. I can write a dedicated patch to convert all of the locations at once.

A couple of questions -

I see that for 16, as an example, there are three functions:
1) mmio_write16_le()
2) mmio_write16_be()
3) mmio_write16()
Per my understanding the first two do not manipulate the data and are only intended to
allow sparse to pass (and protect us). However the 3rd performs htole.
Is this correct?

#define MAKE_WRITE(_NAME_, _SZ_)                                               \
        static inline void _NAME_##_be(void *addr, __be##_SZ_ value)           \
        {                                                                      \
                atomic_store_explicit((_Atomic(uint##_SZ_##_t) *)addr,         \
                                      (__force uint##_SZ_##_t)value,           \
                                      memory_order_relaxed);                   \
        }                                                                      \
What is the effect of the "relaxed ordering" if we use a non-WC BUS?

Thanks,
Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Sept. 6, 2017, 3:19 p.m. UTC | #4 
On Wed, Sep 06, 2017 at 09:18:58AM +0000, Amrani, Ram wrote:
> > On Tue, Sep 05, 2017 at 05:36:54PM +0300, Ram Amrani wrote:
> > > +		if (iwarp) {
> > > +			writel(qp->rq.iwarp_db2_data.raw, qp->rq.iwarp_db2);
> > > +			mmio_flush_writes();
> > > +		}
> > 
> > Can you update this driver to use the macros in util/mmio.h and
> > discard this writel thing?
> 
> Sure. I can write a dedicated patch to convert all of the locations at once.
> 
> A couple of questions -
> 
> I see that for 16, as an example, there are three functions:
> 1) mmio_write16_le()
> 2) mmio_write16_be()
> 3) mmio_write16()
> Per my understanding the first two do not manipulate the data and are only intended to
> allow sparse to pass (and protect us).

Yes..

> However the 3rd performs htole. Is this correct?

Yes, if necessary it performs the swap to create the defined TLP on
any arches that require it.

It is basically equivalent to the kernel's writel_relaxed

> #define MAKE_WRITE(_NAME_, _SZ_)                                               \
>         static inline void _NAME_##_be(void *addr, __be##_SZ_ value)           \
>         {                                                                      \
>                 atomic_store_explicit((_Atomic(uint##_SZ_##_t) *)addr,         \
>                                       (__force uint##_SZ_##_t)value,           \
>                                       memory_order_relaxed);                   \
>         }                                                                      \
> What is the effect of the "relaxed ordering" if we use a non-WC BUS?

memory_order_relaxed tells the compiler to WRITE_ONCE but otherwise
not concern it self with ordering of stores.

eg

 *foo = 1
  mmio_write16(bar, 12);

Can be freely re-ordered during compilation and during execution.

Code needs to insert a util/barrier.h if it requires ordering.

But qedr is using WC right?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amrani, Ram Sept. 7, 2017, 6:45 a.m. UTC | #5 
Thanks.

> On Wed, Sep 06, 2017 at 09:18:58AM +0000, Amrani, Ram wrote:
> > > On Tue, Sep 05, 2017 at 05:36:54PM +0300, Ram Amrani wrote:
> > > > +		if (iwarp) {
> > > > +			writel(qp->rq.iwarp_db2_data.raw, qp->rq.iwarp_db2);
> > > > +			mmio_flush_writes();
> > > > +		}
> > >
> > > Can you update this driver to use the macros in util/mmio.h and
> > > discard this writel thing?
> >
> > Sure. I can write a dedicated patch to convert all of the locations at once.
> >
> > A couple of questions -
> >
> > I see that for 16, as an example, there are three functions:
> > 1) mmio_write16_le()
> > 2) mmio_write16_be()
> > 3) mmio_write16()
> > Per my understanding the first two do not manipulate the data and are only intended to
> > allow sparse to pass (and protect us).
> 
> Yes..
> 
> > However the 3rd performs htole. Is this correct?
> 
> Yes, if necessary it performs the swap to create the defined TLP on
> any arches that require it.
> 
> It is basically equivalent to the kernel's writel_relaxed
> 
> > #define MAKE_WRITE(_NAME_, _SZ_)                                               \
> >         static inline void _NAME_##_be(void *addr, __be##_SZ_ value)           \
> >         {                                                                      \
> >                 atomic_store_explicit((_Atomic(uint##_SZ_##_t) *)addr,         \
> >                                       (__force uint##_SZ_##_t)value,           \
> >                                       memory_order_relaxed);                   \
> >         }                                                                      \
> > What is the effect of the "relaxed ordering" if we use a non-WC BUS?
> 
> memory_order_relaxed tells the compiler to WRITE_ONCE but otherwise
> not concern it self with ordering of stores.
> 
> eg
> 
>  *foo = 1
>   mmio_write16(bar, 12);
> 
> Can be freely re-ordered during compilation and during execution.
> 
> Code needs to insert a util/barrier.h if it requires ordering.
> 
> But qedr is using WC right?

Yes.
Sometimes, however, I disable WC for various checks.

> 
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/providers/qedr/qelr.h b/providers/qedr/qelr.h
index 0dd6835..3c663e7 100644
--- a/providers/qedr/qelr.h
+++ b/providers/qedr/qelr.h
@@ -192,6 +192,8 @@  struct qelr_qp_hwq_info {
 	void					*db;      /* Doorbell address */
 	void					*edpm_db;
 	union db_prod32				db_data;  /* Doorbell data */
+	void					*iwarp_db2;      /* iWARP RQ Doorbell address */
+	union db_prod32				iwarp_db2_data;  /* iWARP RQ Doorbell data */
 
 	uint16_t				icid;
 };
diff --git a/providers/qedr/qelr_verbs.c b/providers/qedr/qelr_verbs.c
index 676b18c..a087499 100644
--- a/providers/qedr/qelr_verbs.c
+++ b/providers/qedr/qelr_verbs.c
@@ -59,6 +59,9 @@ 
 #define QELR_RQE_ELEMENT_SIZE	(sizeof(struct rdma_rq_sge))
 #define QELR_CQE_SIZE		(sizeof(union rdma_cqe))
 
+#define IS_IWARP(_dev)		(_dev->node_type == IBV_NODE_RNIC)
+#define IS_ROCE(_dev)		(_dev->node_type == IBV_NODE_CA)
+
 static void qelr_inc_sw_cons_u16(struct qelr_qp_hwq_info *info)
 {
 	info->cons = (info->cons + 1) % info->max_wr;
@@ -434,6 +437,9 @@  static inline int qelr_configure_qp_rq(struct qelr_devctx *cxt,
 	qp->rq.icid = resp->rq_icid;
 	qp->rq.db_data.data.icid = htole16(resp->rq_icid);
 	qp->rq.db = cxt->db_addr + resp->rq_db_offset;
+	qp->rq.iwarp_db2 = cxt->db_addr + resp->rq_db2_offset;
+	qp->rq.iwarp_db2_data.data.icid = qp->rq.icid;
+	qp->rq.iwarp_db2_data.data.value = DQ_TCM_IWARP_POST_RQ_CF_CMD;
 	qp->rq.prod = 0;
 
 	/* shadow RQ */
@@ -648,6 +654,12 @@  static int qelr_update_qp_state(struct qelr_qp *qp,
 	int status = 0;
 	enum qelr_qp_state new_state;
 
+	/* iWARP states are updated implicitely by driver and don't have a
+	 * real purpose in user-lib.
+	 */
+	if (IS_IWARP(qp->ibv_qp.context->device))
+		return 0;
+
 	new_state = get_qelr_qp_state(new_ib_state);
 
 	pthread_spin_lock(&qp->q_lock);
@@ -677,9 +689,11 @@  static int qelr_update_qp_state(struct qelr_qp *qp,
 			/* Update doorbell (in case post_recv was done before
 			 * move to RTR)
 			 */
-			mmio_wc_start();
-			writel(qp->rq.db_data.raw, qp->rq.db);
-			mmio_flush_writes();
+			if (IS_ROCE(qp->ibv_qp.context->device)) {
+				mmio_wc_start();
+				writel(qp->rq.db_data.raw, qp->rq.db);
+				mmio_flush_writes();
+			}
 			break;
 		case QELR_QPS_ERR:
 			break;
@@ -870,6 +884,10 @@  static inline void qelr_init_edpm_info(struct qelr_devctx *cxt,
 {
 	edpm->is_edpm = 0;
 
+	/* Currently dpm is not supported for iWARP */
+	if (IS_IWARP(cxt->ibv_ctx.device))
+		return;
+
 	if (qelr_chain_is_full(&qp->sq.chain) &&
 	    wr->send_flags & IBV_SEND_INLINE && !qp->edpm_disabled) {
 		memset(edpm, 0, sizeof(*edpm));
@@ -1405,7 +1423,8 @@  int qelr_post_send(struct ibv_qp *ib_qp, struct ibv_send_wr *wr,
 
 	pthread_spin_lock(&qp->q_lock);
 
-	if ((qp->state != QELR_QPS_RTS && qp->state != QELR_QPS_ERR &&
+	if (IS_ROCE(ib_qp->context->device) &&
+	    (qp->state != QELR_QPS_RTS && qp->state != QELR_QPS_ERR &&
 	     qp->state != QELR_QPS_SQD)) {
 		pthread_spin_unlock(&qp->q_lock);
 		*bad_wr = wr;
@@ -1445,10 +1464,11 @@  int qelr_post_recv(struct ibv_qp *ibqp, struct ibv_recv_wr *wr,
 	struct qelr_qp *qp =  get_qelr_qp(ibqp);
 	struct qelr_devctx *cxt = get_qelr_ctx(ibqp->context);
 	uint16_t db_val;
+	uint8_t iwarp = IS_IWARP(ibqp->context->device);
 
 	pthread_spin_lock(&qp->q_lock);
 
-	if (qp->state == QELR_QPS_RST) {
+	if (!iwarp && qp->state == QELR_QPS_RST) {
 		pthread_spin_unlock(&qp->q_lock);
 		*bad_wr = wr;
 		return -EINVAL;
@@ -1517,6 +1537,10 @@  int qelr_post_recv(struct ibv_qp *ibqp, struct ibv_recv_wr *wr,
 		writel(qp->rq.db_data.raw, qp->rq.db);
 		mmio_flush_writes();
 
+		if (iwarp) {
+			writel(qp->rq.iwarp_db2_data.raw, qp->rq.iwarp_db2);
+			mmio_flush_writes();
+		}
 		wr = wr->next;
 	}