[PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

This series moves dealing with the safe all physical mr:

  ib_get_dma_mr(pd,IB_ACCESS_LOCAL_WRITE);

Into ib_alloc_pd, and in the process makes the global local_dma_lkey functionality
broadly enabled for all ULPs.

The remaining users of ib_get_dma_mr are all unsafe:
 drivers/infiniband/ulp/iser/iser_verbs.c:
	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
				   IB_ACCESS_REMOTE_WRITE |
				   IB_ACCESS_REMOTE_READ);

 drivers/infiniband/ulp/srp/ib_srp.c:
	srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
				    IB_ACCESS_LOCAL_WRITE |
				    IB_ACCESS_REMOTE_READ |
				    IB_ACCESS_REMOTE_WRITE);

 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c:
	int acflags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE;
		mr = ib_get_dma_mr(hdev->ibh_pd, acflags);

 net/rds/iw.c:
		rds_iwdev->mr = ib_get_dma_mr(rds_iwdev->pd,
					IB_ACCESS_REMOTE_READ |
					IB_ACCESS_REMOTE_WRITE |
					IB_ACCESS_LOCAL_WRITE);

 net/sunrpc/xprtrdma/svc_rdma_transport.c:
		if (rdma_protocol_iwarp(newxprt->sc_cm_id->device,
					newxprt->sc_cm_id->port_num) &&
		    !(newxprt->sc_dev_caps & SVCRDMA_DEVCAP_FAST_REG))
			dma_mr_acc |= IB_ACCESS_REMOTE_WRITE;
		newxprt->sc_phys_mr =
			ib_get_dma_mr(newxprt->sc_pd, dma_mr_acc);

 net/sunrpc/xprtrdma/verbs.c:
	case RPCRDMA_ALLPHYSICAL:
		ia->ri_ops = &rpcrdma_physical_memreg_ops;
		mem_priv = IB_ACCESS_LOCAL_WRITE |
				IB_ACCESS_REMOTE_WRITE |
				IB_ACCESS_REMOTE_READ;
		ia->ri_bind_mem = ib_get_dma_mr(ia->ri_pd, mem_priv);

Calling ib_get_dma_mr with IB_ACCESS_REMOTE_* flags is considered to be a
serious security problem and should not be done without the user directly
opting in to an off-by-default scheme. The call allows the peer on the QP
unrestricted access to local physical memory if they can guess the rkey value.

A future series will cause the kernel to be tainted by the above call sites to
promote migrating away from this.

To Migrate:
 * If ib_get_dma_mr was being used to get an lkey then use
   local_dma_lkey instead (I belive this series gets all of those cases).

   If the lkey is being used for RDMA_READ, and iWarp support is required then
   iWarp must be detected and FRMR must be used to create a limited temporary
   MR just for the RDMA_READ. (eg NFS, RDS)

 * If ib_get_dma_mr was being used to get an rkey then use FRMR to cerate
   limited temporary MR's (eg SRP, iSER, etc)

All patches are compile tested. I've done basic testing up to and including
the IPoIB patch, the rest required specialized setups I don't have access to,
but are fairly straightforward.

Jason Gunthorpe (12):
  IB/core: Guarantee that a local_dma_lkey is available
  IB/mad: Remove ib_get_dma_mr calls
  IB/ipoib: Remove ib_get_dma_mr calls
  IB/mlx4: Remove ib_get_dma_mr calls
  IB/mlx5: Remove ib_get_dma_mr calls
  IB/iser: Use pd->local_dma_lkey
  iser-target: Remove ib_get_dma_mr calls
  IB/srp: Use pd->local_dma_lkey
  IB/srp: Do not create an all physical insecure rkey by default
  ib_srpt: Remove ib_get_dma_mr calls
  net/9p: Remove ib_get_dma_mr calls
  rds/ib: Remove ib_get_dma_mr calls

 drivers/infiniband/core/mad.c                | 26 ++-------------
 drivers/infiniband/core/mad_priv.h           |  1 -
 drivers/infiniband/core/verbs.c              | 47 +++++++++++++++++++++++++---
 drivers/infiniband/hw/mlx4/mad.c             | 23 +++-----------
 drivers/infiniband/hw/mlx4/mlx4_ib.h         |  1 -
 drivers/infiniband/hw/mlx5/main.c            | 13 --------
 drivers/infiniband/hw/mlx5/mlx5_ib.h         |  1 -
 drivers/infiniband/hw/mlx5/mr.c              |  5 ++-
 drivers/infiniband/ulp/ipoib/ipoib.h         |  1 -
 drivers/infiniband/ulp/ipoib/ipoib_cm.c      |  2 +-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c   | 18 ++---------
 drivers/infiniband/ulp/iser/iscsi_iser.c     |  2 +-
 drivers/infiniband/ulp/iser/iser_initiator.c |  8 ++---
 drivers/infiniband/ulp/iser/iser_memory.c    |  2 +-
 drivers/infiniband/ulp/iser/iser_verbs.c     |  2 +-
 drivers/infiniband/ulp/isert/ib_isert.c      | 33 +++++++------------
 drivers/infiniband/ulp/isert/ib_isert.h      |  1 -
 drivers/infiniband/ulp/srp/ib_srp.c          | 33 ++++++++++++-------
 drivers/infiniband/ulp/srp/ib_srp.h          |  2 +-
 drivers/infiniband/ulp/srpt/ib_srpt.c        | 15 +++------
 drivers/infiniband/ulp/srpt/ib_srpt.h        |  1 -
 include/rdma/ib_mad.h                        |  1 -
 include/rdma/ib_verbs.h                      |  9 ++----
 net/9p/trans_rdma.c                          | 26 ++-------------
 net/rds/ib.c                                 |  8 -----
 net/rds/ib.h                                 |  2 --
 net/rds/ib_cm.c                              |  4 +--
 net/rds/ib_recv.c                            |  6 ++--
 net/rds/ib_send.c                            |  8 ++---
 29 files changed, 112 insertions(+), 189 deletions(-)

-- 
2.1.4

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

[PATCH v2 01/12] IB/core: Guarantee that a local_dma_lkey is available

[Jason Gunthorpe]

Every single ULP requires a local_dma_lkey to do anything with
a QP, so let us ensure one exists for every PD created.

If the driver can supply a global local_dma_lkey then use that, otherwise
ask the driver to create a local use all physical memory MR associated
with the new PD.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Acked-by: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 47 ++++++++++++++++++++++++++++++++++++-----
 include/rdma/ib_verbs.h         |  9 ++------
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index bac3fb406a74..bb561c8e51ad 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -213,24 +213,61 @@ EXPORT_SYMBOL(rdma_port_get_link_layer);
 
 /* Protection domains */
 
+/**
+ * ib_alloc_pd - Allocates an unused protection domain.
+ * @device: The device on which to allocate the protection domain.
+ *
+ * A protection domain object provides an association between QPs, shared
+ * receive queues, address handles, memory regions, and memory windows.
+ *
+ * Every PD has a local_dma_lkey which can be used as the lkey value for local
+ * memory operations.
+ */
 struct ib_pd *ib_alloc_pd(struct ib_device *device)
 {
 	struct ib_pd *pd;
+	struct ib_device_attr devattr;
+	int rc;
+
+	rc = ib_query_device(device, &devattr);
+	if (rc)
+		return ERR_PTR(rc);
 
 	pd = device->alloc_pd(device, NULL, NULL);
+	if (IS_ERR(pd))
+		return pd;
+
+	pd->device = device;
+	pd->uobject = NULL;
+	pd->local_mr = NULL;
+	atomic_set(&pd->usecnt, 0);
+
+	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY)
+		pd->local_dma_lkey = device->local_dma_lkey;
+	else {
+		struct ib_mr *mr;
+
+		mr = ib_get_dma_mr(pd, IB_ACCESS_LOCAL_WRITE);
+		if (IS_ERR(mr)) {
+			ib_dealloc_pd(pd);
+			return (struct ib_pd *)mr;
+		}
 
-	if (!IS_ERR(pd)) {
-		pd->device  = device;
-		pd->uobject = NULL;
-		atomic_set(&pd->usecnt, 0);
+		pd->local_mr = mr;
+		pd->local_dma_lkey = pd->local_mr->lkey;
 	}
-
 	return pd;
 }
 EXPORT_SYMBOL(ib_alloc_pd);
 
 int ib_dealloc_pd(struct ib_pd *pd)
 {
+	if (pd->local_mr) {
+		if (ib_dereg_mr(pd->local_mr))
+			return -EBUSY;
+		pd->local_mr = NULL;
+	}
+
 	if (atomic_read(&pd->usecnt))
 		return -EBUSY;
 
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index b0f898e3b2e7..eaec3081fb87 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1252,9 +1252,11 @@ struct ib_udata {
 };
 
 struct ib_pd {
+	u32			local_dma_lkey;
 	struct ib_device       *device;
 	struct ib_uobject      *uobject;
 	atomic_t          	usecnt; /* count all resources */
+	struct ib_mr	       *local_mr;
 };
 
 struct ib_xrcd {
@@ -2135,13 +2137,6 @@ int ib_find_gid(struct ib_device *device, union ib_gid *gid,
 int ib_find_pkey(struct ib_device *device,
 		 u8 port_num, u16 pkey, u16 *index);
 
-/**
- * ib_alloc_pd - Allocates an unused protection domain.
- * @device: The device on which to allocate the protection domain.
- *
- * A protection domain object provides an association between QPs, shared
- * receive queues, address handles, memory regions, and memory windows.
- */
 struct ib_pd *ib_alloc_pd(struct ib_device *device);
 
 /**
-- 
2.1.4

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

[PATCH v2 02/12] IB/mad: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/infiniband/core/mad.c      | 26 +++-----------------------
 drivers/infiniband/core/mad_priv.h |  1 -
 include/rdma/ib_mad.h              |  1 -
 3 files changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 786fc51bf04b..7c728a2d1d56 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -338,13 +338,6 @@ struct ib_mad_agent *ib_register_mad_agent(struct ib_device *device,
 		goto error1;
 	}
 
-	mad_agent_priv->agent.mr = ib_get_dma_mr(port_priv->qp_info[qpn].qp->pd,
-						 IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(mad_agent_priv->agent.mr)) {
-		ret = ERR_PTR(-ENOMEM);
-		goto error2;
-	}
-
 	if (mad_reg_req) {
 		reg_req = kmemdup(mad_reg_req, sizeof *reg_req, GFP_KERNEL);
 		if (!reg_req) {
@@ -429,8 +422,6 @@ error4:
 	spin_unlock_irqrestore(&port_priv->reg_lock, flags);
 	kfree(reg_req);
 error3:
-	ib_dereg_mr(mad_agent_priv->agent.mr);
-error2:
 	kfree(mad_agent_priv);
 error1:
 	return ret;
@@ -590,7 +581,6 @@ static void unregister_mad_agent(struct ib_mad_agent_private *mad_agent_priv)
 	wait_for_completion(&mad_agent_priv->comp);
 
 	kfree(mad_agent_priv->reg_req);
-	ib_dereg_mr(mad_agent_priv->agent.mr);
 	kfree(mad_agent_priv);
 }
 
@@ -1038,7 +1028,7 @@ struct ib_mad_send_buf * ib_create_send_mad(struct ib_mad_agent *mad_agent,
 
 	mad_send_wr->mad_agent_priv = mad_agent_priv;
 	mad_send_wr->sg_list[0].length = hdr_len;
-	mad_send_wr->sg_list[0].lkey = mad_agent->mr->lkey;
+	mad_send_wr->sg_list[0].lkey = mad_agent->qp->pd->local_dma_lkey;
 
 	/* OPA MADs don't have to be the full 2048 bytes */
 	if (opa && base_version == OPA_MGMT_BASE_VERSION &&
@@ -1047,7 +1037,7 @@ struct ib_mad_send_buf * ib_create_send_mad(struct ib_mad_agent *mad_agent,
 	else
 		mad_send_wr->sg_list[1].length = mad_size - hdr_len;
 
-	mad_send_wr->sg_list[1].lkey = mad_agent->mr->lkey;
+	mad_send_wr->sg_list[1].lkey = mad_agent->qp->pd->local_dma_lkey;
 
 	mad_send_wr->send_wr.wr_id = (unsigned long) mad_send_wr;
 	mad_send_wr->send_wr.sg_list = mad_send_wr->sg_list;
@@ -2885,7 +2875,7 @@ static int ib_mad_post_receive_mads(struct ib_mad_qp_info *qp_info,
 	struct ib_mad_queue *recv_queue = &qp_info->recv_queue;
 
 	/* Initialize common scatter list fields */
-	sg_list.lkey = (*qp_info->port_priv->mr).lkey;
+	sg_list.lkey = qp_info->port_priv->pd->local_dma_lkey;
 
 	/* Initialize common receive WR fields */
 	recv_wr.next = NULL;
@@ -3201,13 +3191,6 @@ static int ib_mad_port_open(struct ib_device *device,
 		goto error4;
 	}
 
-	port_priv->mr = ib_get_dma_mr(port_priv->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(port_priv->mr)) {
-		dev_err(&device->dev, "Couldn't get ib_mad DMA MR\n");
-		ret = PTR_ERR(port_priv->mr);
-		goto error5;
-	}
-
 	if (has_smi) {
 		ret = create_mad_qp(&port_priv->qp_info[0], IB_QPT_SMI);
 		if (ret)
@@ -3248,8 +3231,6 @@ error8:
 error7:
 	destroy_mad_qp(&port_priv->qp_info[0]);
 error6:
-	ib_dereg_mr(port_priv->mr);
-error5:
 	ib_dealloc_pd(port_priv->pd);
 error4:
 	ib_destroy_cq(port_priv->cq);
@@ -3284,7 +3265,6 @@ static int ib_mad_port_close(struct ib_device *device, int port_num)
 	destroy_workqueue(port_priv->wq);
 	destroy_mad_qp(&port_priv->qp_info[1]);
 	destroy_mad_qp(&port_priv->qp_info[0]);
-	ib_dereg_mr(port_priv->mr);
 	ib_dealloc_pd(port_priv->pd);
 	ib_destroy_cq(port_priv->cq);
 	cleanup_recv_queue(&port_priv->qp_info[1]);
diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index 5be89f98928f..4a4f7aad0978 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -199,7 +199,6 @@ struct ib_mad_port_private {
 	int port_num;
 	struct ib_cq *cq;
 	struct ib_pd *pd;
-	struct ib_mr *mr;
 
 	spinlock_t reg_lock;
 	struct ib_mad_mgmt_version_table version[MAX_MGMT_VERSION];
diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
index c8422d5a5a91..1f27023c919a 100644
--- a/include/rdma/ib_mad.h
+++ b/include/rdma/ib_mad.h
@@ -388,7 +388,6 @@ enum {
 struct ib_mad_agent {
 	struct ib_device	*device;
 	struct ib_qp		*qp;
-	struct ib_mr		*mr;
 	ib_mad_recv_handler	recv_handler;
 	ib_mad_send_handler	send_handler;
 	ib_mad_snoop_handler	snoop_handler;
-- 
2.1.4

[PATCH v2 03/12] IB/ipoib: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib.h       |  1 -
 drivers/infiniband/ulp/ipoib/ipoib_cm.c    |  2 +-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c | 18 +++---------------
 3 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 79859c4d43c9..ca2873698d75 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -342,7 +342,6 @@ struct ipoib_dev_priv {
 	u16		  pkey;
 	u16		  pkey_index;
 	struct ib_pd	 *pd;
-	struct ib_mr	 *mr;
 	struct ib_cq	 *recv_cq;
 	struct ib_cq	 *send_cq;
 	struct ib_qp	 *qp;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index ee39be6ccfb0..206227bb385f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -332,7 +332,7 @@ static void ipoib_cm_init_rx_wr(struct net_device *dev,
 	int i;
 
 	for (i = 0; i < priv->cm.num_frags; ++i)
-		sge[i].lkey = priv->mr->lkey;
+		sge[i].lkey = priv->pd->local_dma_lkey;
 
 	sge[0].length = IPOIB_CM_HEAD_SIZE;
 	for (i = 1; i < priv->cm.num_frags; ++i)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_verbs.c b/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
index 9e6ee82a8fd7..3423256d3500 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
@@ -152,12 +152,6 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 		return -ENODEV;
 	}
 
-	priv->mr = ib_get_dma_mr(priv->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(priv->mr)) {
-		printk(KERN_WARNING "%s: ib_get_dma_mr failed\n", ca->name);
-		goto out_free_pd;
-	}
-
 	/*
 	 * the various IPoIB tasks assume they will never race against
 	 * themselves, so always use a single thread workqueue
@@ -165,7 +159,7 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 	priv->wq = create_singlethread_workqueue("ipoib_wq");
 	if (!priv->wq) {
 		printk(KERN_WARNING "ipoib: failed to allocate device WQ\n");
-		goto out_free_mr;
+		goto out_free_pd;
 	}
 
 	size = ipoib_recvq_size + 1;
@@ -224,13 +218,13 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 	priv->dev->dev_addr[3] = (priv->qp->qp_num      ) & 0xff;
 
 	for (i = 0; i < MAX_SKB_FRAGS + 1; ++i)
-		priv->tx_sge[i].lkey = priv->mr->lkey;
+		priv->tx_sge[i].lkey = priv->pd->local_dma_lkey;
 
 	priv->tx_wr.opcode	= IB_WR_SEND;
 	priv->tx_wr.sg_list	= priv->tx_sge;
 	priv->tx_wr.send_flags	= IB_SEND_SIGNALED;
 
-	priv->rx_sge[0].lkey = priv->mr->lkey;
+	priv->rx_sge[0].lkey = priv->pd->local_dma_lkey;
 
 	priv->rx_sge[0].length = IPOIB_UD_BUF_SIZE(priv->max_ib_mtu);
 	priv->rx_wr.num_sge = 1;
@@ -253,9 +247,6 @@ out_free_wq:
 	destroy_workqueue(priv->wq);
 	priv->wq = NULL;
 
-out_free_mr:
-	ib_dereg_mr(priv->mr);
-
 out_free_pd:
 	ib_dealloc_pd(priv->pd);
 
@@ -288,9 +279,6 @@ void ipoib_transport_dev_cleanup(struct net_device *dev)
 		priv->wq = NULL;
 	}
 
-	if (ib_dereg_mr(priv->mr))
-		ipoib_warn(priv, "ib_dereg_mr failed\n");
-
 	if (ib_dealloc_pd(priv->pd))
 		ipoib_warn(priv, "ib_dealloc_pd failed\n");
 
-- 
2.1.4

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

[PATCH v2 04/12] IB/mlx4: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/infiniband/hw/mlx4/mad.c     | 23 ++++-------------------
 drivers/infiniband/hw/mlx4/mlx4_ib.h |  1 -
 2 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
index 68b3dfa922bf..1cd75ff02251 100644
--- a/drivers/infiniband/hw/mlx4/mad.c
+++ b/drivers/infiniband/hw/mlx4/mad.c
@@ -580,7 +580,7 @@ int mlx4_ib_send_to_slave(struct mlx4_ib_dev *dev, int slave, u8 port,
 
 	list.addr = tun_qp->tx_ring[tun_tx_ix].buf.map;
 	list.length = sizeof (struct mlx4_rcv_tunnel_mad);
-	list.lkey = tun_ctx->mr->lkey;
+	list.lkey = tun_ctx->pd->local_dma_lkey;
 
 	wr.wr.ud.ah = ah;
 	wr.wr.ud.port_num = port;
@@ -1133,7 +1133,7 @@ static int mlx4_ib_post_pv_qp_buf(struct mlx4_ib_demux_pv_ctx *ctx,
 
 	sg_list.addr = tun_qp->ring[index].map;
 	sg_list.length = size;
-	sg_list.lkey = ctx->mr->lkey;
+	sg_list.lkey = ctx->pd->local_dma_lkey;
 
 	recv_wr.next = NULL;
 	recv_wr.sg_list = &sg_list;
@@ -1244,7 +1244,7 @@ int mlx4_ib_send_to_wire(struct mlx4_ib_dev *dev, int slave, u8 port,
 
 	list.addr = sqp->tx_ring[wire_tx_ix].buf.map;
 	list.length = sizeof (struct mlx4_mad_snd_buf);
-	list.lkey = sqp_ctx->mr->lkey;
+	list.lkey = sqp_ctx->pd->local_dma_lkey;
 
 	wr.wr.ud.ah = ah;
 	wr.wr.ud.port_num = port;
@@ -1827,19 +1827,12 @@ static int create_pv_resources(struct ib_device *ibdev, int slave, int port,
 		goto err_cq;
 	}
 
-	ctx->mr = ib_get_dma_mr(ctx->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(ctx->mr)) {
-		ret = PTR_ERR(ctx->mr);
-		pr_err("Couldn't get tunnel DMA MR (%d)\n", ret);
-		goto err_pd;
-	}
-
 	if (ctx->has_smi) {
 		ret = create_pv_sqp(ctx, IB_QPT_SMI, create_tun);
 		if (ret) {
 			pr_err("Couldn't create %s QP0 (%d)\n",
 			       create_tun ? "tunnel for" : "",  ret);
-			goto err_mr;
+			goto err_pd;
 		}
 	}
 
@@ -1876,10 +1869,6 @@ err_qp0:
 		ib_destroy_qp(ctx->qp[0].qp);
 	ctx->qp[0].qp = NULL;
 
-err_mr:
-	ib_dereg_mr(ctx->mr);
-	ctx->mr = NULL;
-
 err_pd:
 	ib_dealloc_pd(ctx->pd);
 	ctx->pd = NULL;
@@ -1916,8 +1905,6 @@ static void destroy_pv_resources(struct mlx4_ib_dev *dev, int slave, int port,
 		ib_destroy_qp(ctx->qp[1].qp);
 		ctx->qp[1].qp = NULL;
 		mlx4_ib_free_pv_qp_bufs(ctx, IB_QPT_GSI, 1);
-		ib_dereg_mr(ctx->mr);
-		ctx->mr = NULL;
 		ib_dealloc_pd(ctx->pd);
 		ctx->pd = NULL;
 		ib_destroy_cq(ctx->cq);
@@ -2050,8 +2037,6 @@ static void mlx4_ib_free_sqp_ctx(struct mlx4_ib_demux_pv_ctx *sqp_ctx)
 		ib_destroy_qp(sqp_ctx->qp[1].qp);
 		sqp_ctx->qp[1].qp = NULL;
 		mlx4_ib_free_pv_qp_bufs(sqp_ctx, IB_QPT_GSI, 0);
-		ib_dereg_mr(sqp_ctx->mr);
-		sqp_ctx->mr = NULL;
 		ib_dealloc_pd(sqp_ctx->pd);
 		sqp_ctx->pd = NULL;
 		ib_destroy_cq(sqp_ctx->cq);
diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
index 334387f63358..9066fc2406cc 100644
--- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
+++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
@@ -415,7 +415,6 @@ struct mlx4_ib_demux_pv_ctx {
 	struct ib_device *ib_dev;
 	struct ib_cq *cq;
 	struct ib_pd *pd;
-	struct ib_mr *mr;
 	struct work_struct work;
 	struct workqueue_struct *wq;
 	struct mlx4_ib_demux_pv_qp qp[2];
-- 
2.1.4

[PATCH v2 05/12] IB/mlx5: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/infiniband/hw/mlx5/main.c    | 13 -------------
 drivers/infiniband/hw/mlx5/mlx5_ib.h |  1 -
 drivers/infiniband/hw/mlx5/mr.c      |  5 ++---
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 085c24b4b603..72ab0bdb415a 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -1121,7 +1121,6 @@ static void destroy_umrc_res(struct mlx5_ib_dev *dev)
 
 	mlx5_ib_destroy_qp(dev->umrc.qp);
 	ib_destroy_cq(dev->umrc.cq);
-	ib_dereg_mr(dev->umrc.mr);
 	ib_dealloc_pd(dev->umrc.pd);
 }
 
@@ -1136,7 +1135,6 @@ static int create_umr_res(struct mlx5_ib_dev *dev)
 	struct ib_pd *pd;
 	struct ib_cq *cq;
 	struct ib_qp *qp;
-	struct ib_mr *mr;
 	struct ib_cq_init_attr cq_attr = {};
 	int ret;
 
@@ -1154,13 +1152,6 @@ static int create_umr_res(struct mlx5_ib_dev *dev)
 		goto error_0;
 	}
 
-	mr = ib_get_dma_mr(pd,  IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(mr)) {
-		mlx5_ib_dbg(dev, "Couldn't create DMA MR for sync UMR QP\n");
-		ret = PTR_ERR(mr);
-		goto error_1;
-	}
-
 	cq_attr.cqe = 128;
 	cq = ib_create_cq(&dev->ib_dev, mlx5_umr_cq_handler, NULL, NULL,
 			  &cq_attr);
@@ -1218,7 +1209,6 @@ static int create_umr_res(struct mlx5_ib_dev *dev)
 
 	dev->umrc.qp = qp;
 	dev->umrc.cq = cq;
-	dev->umrc.mr = mr;
 	dev->umrc.pd = pd;
 
 	sema_init(&dev->umrc.sem, MAX_UMR_WR);
@@ -1240,9 +1230,6 @@ error_3:
 	ib_destroy_cq(cq);
 
 error_2:
-	ib_dereg_mr(mr);
-
-error_1:
 	ib_dealloc_pd(pd);
 
 error_0:
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 7cae09836481..446d80427773 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -349,7 +349,6 @@ struct umr_common {
 	struct ib_pd	*pd;
 	struct ib_cq	*cq;
 	struct ib_qp	*qp;
-	struct ib_mr	*mr;
 	/* control access to UMR QP
 	 */
 	struct semaphore	sem;
diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index bc9a0de897cb..4c92ca8a4181 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -690,12 +690,11 @@ static void prep_umr_reg_wqe(struct ib_pd *pd, struct ib_send_wr *wr,
 			     int access_flags)
 {
 	struct mlx5_ib_dev *dev = to_mdev(pd->device);
-	struct ib_mr *mr = dev->umrc.mr;
 	struct mlx5_umr_wr *umrwr = (struct mlx5_umr_wr *)&wr->wr.fast_reg;
 
 	sg->addr = dma;
 	sg->length = ALIGN(sizeof(u64) * n, 64);
-	sg->lkey = mr->lkey;
+	sg->lkey = dev->umrc.pd->local_dma_lkey;
 
 	wr->next = NULL;
 	wr->send_flags = 0;
@@ -926,7 +925,7 @@ int mlx5_ib_update_mtt(struct mlx5_ib_mr *mr, u64 start_page_index, int npages,
 		sg.addr = dma;
 		sg.length = ALIGN(npages * sizeof(u64),
 				MLX5_UMR_MTT_ALIGNMENT);
-		sg.lkey = dev->umrc.mr->lkey;
+		sg.lkey = dev->umrc.pd->local_dma_lkey;
 
 		wr.send_flags = MLX5_IB_SEND_UMR_FAIL_IF_FREE |
 				MLX5_IB_SEND_UMR_UPDATE_MTT;
-- 
2.1.4

[PATCH v2 06/12] IB/iser: Use pd->local_dma_lkey

[Jason Gunthorpe]

Replace all leys with  pd->local_dma_lkey. This driver does not support
iWarp, so this is safe.

The insecure use of ib_get_dma_mr is thus isolated to an rkey, and this
looks trivially fixed by forcing the use of registration in a future
patch.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c     | 2 +-
 drivers/infiniband/ulp/iser/iser_initiator.c | 8 ++++----
 drivers/infiniband/ulp/iser/iser_memory.c    | 2 +-
 drivers/infiniband/ulp/iser/iser_verbs.c     | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 6a594aac2290..f44c6b879329 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -204,7 +204,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
 	tx_desc->dma_addr = dma_addr;
 	tx_desc->tx_sg[0].addr   = tx_desc->dma_addr;
 	tx_desc->tx_sg[0].length = ISER_HEADERS_LEN;
-	tx_desc->tx_sg[0].lkey   = device->mr->lkey;
+	tx_desc->tx_sg[0].lkey   = device->pd->local_dma_lkey;
 
 	iser_task->iser_conn = iser_conn;
 out:
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 3e2118e8ed87..2d02f042c69a 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -173,8 +173,8 @@ static void iser_create_send_desc(struct iser_conn	*iser_conn,
 
 	tx_desc->num_sge = 1;
 
-	if (tx_desc->tx_sg[0].lkey != device->mr->lkey) {
-		tx_desc->tx_sg[0].lkey = device->mr->lkey;
+	if (tx_desc->tx_sg[0].lkey != device->pd->local_dma_lkey) {
+		tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey;
 		iser_dbg("sdesc %p lkey mismatch, fixing\n", tx_desc);
 	}
 }
@@ -291,7 +291,7 @@ int iser_alloc_rx_descriptors(struct iser_conn *iser_conn,
 		rx_sg = &rx_desc->rx_sg;
 		rx_sg->addr   = rx_desc->dma_addr;
 		rx_sg->length = ISER_RX_PAYLOAD_SIZE;
-		rx_sg->lkey   = device->mr->lkey;
+		rx_sg->lkey   = device->pd->local_dma_lkey;
 	}
 
 	iser_conn->rx_desc_head = 0;
@@ -543,7 +543,7 @@ int iser_send_control(struct iscsi_conn *conn,
 
 		tx_dsg->addr    = iser_conn->login_req_dma;
 		tx_dsg->length  = task->data_count;
-		tx_dsg->lkey    = device->mr->lkey;
+		tx_dsg->lkey    = device->pd->local_dma_lkey;
 		mdesc->num_sge = 2;
 	}
 
diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index f0cdc961eb11..3129a42150ff 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -393,7 +393,7 @@ iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
 {
 	struct scatterlist *sg = mem->sg;
 
-	reg->sge.lkey = device->mr->lkey;
+	reg->sge.lkey = device->pd->local_dma_lkey;
 	reg->rkey = device->mr->rkey;
 	reg->sge.addr = ib_sg_dma_address(device->ib_device, &sg[0]);
 	reg->sge.length = ib_sg_dma_len(device->ib_device, &sg[0]);
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 5c9f565ea0e8..52268356c79e 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -1017,7 +1017,7 @@ int iser_post_recvl(struct iser_conn *iser_conn)
 
 	sge.addr   = iser_conn->login_resp_dma;
 	sge.length = ISER_RX_LOGIN_SIZE;
-	sge.lkey   = ib_conn->device->mr->lkey;
+	sge.lkey   = ib_conn->device->pd->local_dma_lkey;
 
 	rx_wr.wr_id   = (uintptr_t)iser_conn->login_resp_buf;
 	rx_wr.sg_list = &sge;
-- 
2.1.4

[PATCH v2 07/12] iser-target: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 33 +++++++++++----------------------
 drivers/infiniband/ulp/isert/ib_isert.h |  1 -
 2 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 771700963127..00d75d977131 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -235,7 +235,7 @@ isert_alloc_rx_descriptors(struct isert_conn *isert_conn)
 		rx_sg = &rx_desc->rx_sg;
 		rx_sg->addr = rx_desc->dma_addr;
 		rx_sg->length = ISER_RX_PAYLOAD_SIZE;
-		rx_sg->lkey = device->mr->lkey;
+		rx_sg->lkey = device->pd->local_dma_lkey;
 	}
 
 	isert_conn->rx_desc_head = 0;
@@ -385,22 +385,12 @@ isert_create_device_ib_res(struct isert_device *device)
 		goto out_cq;
 	}
 
-	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(device->mr)) {
-		ret = PTR_ERR(device->mr);
-		isert_err("failed to create dma mr, device %p, ret=%d\n",
-			  device, ret);
-		goto out_mr;
-	}
-
 	/* Check signature cap */
 	device->pi_capable = dev_attr->device_cap_flags &
 			     IB_DEVICE_SIGNATURE_HANDOVER ? true : false;
 
 	return 0;
 
-out_mr:
-	ib_dealloc_pd(device->pd);
 out_cq:
 	isert_free_comps(device);
 	return ret;
@@ -411,7 +401,6 @@ isert_free_device_ib_res(struct isert_device *device)
 {
 	isert_info("device %p\n", device);
 
-	ib_dereg_mr(device->mr);
 	ib_dealloc_pd(device->pd);
 	isert_free_comps(device);
 }
@@ -1086,8 +1075,8 @@ isert_create_send_desc(struct isert_conn *isert_conn,
 	tx_desc->num_sge = 1;
 	tx_desc->isert_cmd = isert_cmd;
 
-	if (tx_desc->tx_sg[0].lkey != device->mr->lkey) {
-		tx_desc->tx_sg[0].lkey = device->mr->lkey;
+	if (tx_desc->tx_sg[0].lkey != device->pd->local_dma_lkey) {
+		tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey;
 		isert_dbg("tx_desc %p lkey mismatch, fixing\n", tx_desc);
 	}
 }
@@ -1110,7 +1099,7 @@ isert_init_tx_hdrs(struct isert_conn *isert_conn,
 	tx_desc->dma_addr = dma_addr;
 	tx_desc->tx_sg[0].addr	= tx_desc->dma_addr;
 	tx_desc->tx_sg[0].length = ISER_HEADERS_LEN;
-	tx_desc->tx_sg[0].lkey = device->mr->lkey;
+	tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey;
 
 	isert_dbg("Setup tx_sg[0].addr: 0x%llx length: %u lkey: 0x%x\n",
 		  tx_desc->tx_sg[0].addr, tx_desc->tx_sg[0].length,
@@ -1143,7 +1132,7 @@ isert_rdma_post_recvl(struct isert_conn *isert_conn)
 	memset(&sge, 0, sizeof(struct ib_sge));
 	sge.addr = isert_conn->login_req_dma;
 	sge.length = ISER_RX_LOGIN_SIZE;
-	sge.lkey = isert_conn->device->mr->lkey;
+	sge.lkey = isert_conn->device->pd->local_dma_lkey;
 
 	isert_dbg("Setup sge: addr: %llx length: %d 0x%08x\n",
 		sge.addr, sge.length, sge.lkey);
@@ -1193,7 +1182,7 @@ isert_put_login_tx(struct iscsi_conn *conn, struct iscsi_login *login,
 
 		tx_dsg->addr	= isert_conn->login_rsp_dma;
 		tx_dsg->length	= length;
-		tx_dsg->lkey	= isert_conn->device->mr->lkey;
+		tx_dsg->lkey	= isert_conn->device->pd->local_dma_lkey;
 		tx_desc->num_sge = 2;
 	}
 	if (!login->login_failed) {
@@ -2210,7 +2199,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 		isert_cmd->pdu_buf_len = pdu_len;
 		tx_dsg->addr	= isert_cmd->pdu_buf_dma;
 		tx_dsg->length	= pdu_len;
-		tx_dsg->lkey	= device->mr->lkey;
+		tx_dsg->lkey	= device->pd->local_dma_lkey;
 		isert_cmd->tx_desc.num_sge = 2;
 	}
 
@@ -2338,7 +2327,7 @@ isert_put_reject(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 	isert_cmd->pdu_buf_len = ISCSI_HDR_LEN;
 	tx_dsg->addr	= isert_cmd->pdu_buf_dma;
 	tx_dsg->length	= ISCSI_HDR_LEN;
-	tx_dsg->lkey	= device->mr->lkey;
+	tx_dsg->lkey	= device->pd->local_dma_lkey;
 	isert_cmd->tx_desc.num_sge = 2;
 
 	isert_init_send_wr(isert_conn, isert_cmd, send_wr);
@@ -2379,7 +2368,7 @@ isert_put_text_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 		isert_cmd->pdu_buf_len = txt_rsp_len;
 		tx_dsg->addr	= isert_cmd->pdu_buf_dma;
 		tx_dsg->length	= txt_rsp_len;
-		tx_dsg->lkey	= device->mr->lkey;
+		tx_dsg->lkey	= device->pd->local_dma_lkey;
 		isert_cmd->tx_desc.num_sge = 2;
 	}
 	isert_init_send_wr(isert_conn, isert_cmd, send_wr);
@@ -2420,7 +2409,7 @@ isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
 		ib_sge->addr = ib_sg_dma_address(ib_dev, tmp_sg) + page_off;
 		ib_sge->length = min_t(u32, data_left,
 				ib_sg_dma_len(ib_dev, tmp_sg) - page_off);
-		ib_sge->lkey = device->mr->lkey;
+		ib_sge->lkey = device->pd->local_dma_lkey;
 
 		isert_dbg("RDMA ib_sge: addr: 0x%llx  length: %u lkey: %x\n",
 			  ib_sge->addr, ib_sge->length, ib_sge->lkey);
@@ -2594,7 +2583,7 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
 	u32 page_off;
 
 	if (mem->dma_nents == 1) {
-		sge->lkey = device->mr->lkey;
+		sge->lkey = device->pd->local_dma_lkey;
 		sge->addr = ib_sg_dma_address(ib_dev, &mem->sg[0]);
 		sge->length = ib_sg_dma_len(ib_dev, &mem->sg[0]);
 		isert_dbg("sge: addr: 0x%llx  length: %u lkey: %x\n",
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 9ec23a786c02..6a04ba3c0f72 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -209,7 +209,6 @@ struct isert_device {
 	int			refcount;
 	struct ib_device	*ib_device;
 	struct ib_pd		*pd;
-	struct ib_mr		*mr;
 	struct isert_comp	*comps;
 	int                     comps_used;
 	struct list_head	dev_node;
-- 
2.1.4

[PATCH v2 08/12] IB/srp: Use pd->local_dma_lkey

[Jason Gunthorpe]

Replace all leys with  pd->local_dma_lkey. This driver does not support
iWarp, so this is safe.

The insecure use of ib_get_dma_mr is thus isolated to an rkey, and will
have to be fixed separately.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 31a20b462266..19a1356f8b2a 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3146,7 +3146,7 @@ static ssize_t srp_create_target(struct device *dev,
 	target->io_class	= SRP_REV16A_IB_IO_CLASS;
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
-	target->lkey		= host->srp_dev->mr->lkey;
+	target->lkey		= host->srp_dev->pd->local_dma_lkey;
 	target->rkey		= host->srp_dev->mr->rkey;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
-- 
2.1.4

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

[PATCH v2 09/12] IB/srp: Do not create an all physical insecure rkey by default

[Jason Gunthorpe]

The ULP only needs this if the insecure register_always performance
optimization is enabled, or if FRWR/FMR is not supported in the driver.

Do not create an all physical MR unless it is needed to support either of
those modes. Default register_always to true so the out of the box
configuration does not create an insecure all physical MR.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 31 +++++++++++++++++++++----------
 drivers/infiniband/ulp/srp/ib_srp.h |  2 +-
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 19a1356f8b2a..a8003079c232 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -69,7 +69,7 @@ static unsigned int cmd_sg_entries;
 static unsigned int indirect_sg_entries;
 static bool allow_ext_sg;
 static bool prefer_fr;
-static bool register_always;
+static bool register_always = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);
@@ -3147,7 +3147,8 @@ static ssize_t srp_create_target(struct device *dev,
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
 	target->lkey		= host->srp_dev->pd->local_dma_lkey;
-	target->rkey		= host->srp_dev->mr->rkey;
+	if (host->srp_dev->rkey_mr)
+		target->rkey		= host->srp_dev->rkey_mr->rkey;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
 	target->allow_ext_sg	= allow_ext_sg;
@@ -3378,6 +3379,7 @@ static void srp_add_one(struct ib_device *device)
 	struct srp_host *host;
 	int mr_page_shift, p;
 	u64 max_pages_per_mr;
+	unsigned int mr_flags = 0;
 
 	dev_attr = kmalloc(sizeof *dev_attr, GFP_KERNEL);
 	if (!dev_attr)
@@ -3396,8 +3398,11 @@ static void srp_add_one(struct ib_device *device)
 			    device->map_phys_fmr && device->unmap_fmr);
 	srp_dev->has_fr = (dev_attr->device_cap_flags &
 			   IB_DEVICE_MEM_MGT_EXTENSIONS);
-	if (!srp_dev->has_fmr && !srp_dev->has_fr)
+	if (!srp_dev->has_fmr && !srp_dev->has_fr) {
 		dev_warn(&device->dev, "neither FMR nor FR is supported\n");
+		/* Fall back to using an insecure all physical rkey */
+		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+	}
 
 	srp_dev->use_fast_reg = (srp_dev->has_fr &&
 				 (!srp_dev->has_fmr || prefer_fr));
@@ -3433,12 +3438,17 @@ static void srp_add_one(struct ib_device *device)
 	if (IS_ERR(srp_dev->pd))
 		goto free_dev;
 
-	srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
-				    IB_ACCESS_LOCAL_WRITE |
-				    IB_ACCESS_REMOTE_READ |
-				    IB_ACCESS_REMOTE_WRITE);
-	if (IS_ERR(srp_dev->mr))
-		goto err_pd;
+	if (!register_always)
+		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+
+	if (mr_flags) {
+		srp_dev->rkey_mr = ib_get_dma_mr(
+		    srp_dev->pd, IB_ACCESS_LOCAL_WRITE | mr_flags);
+		if (IS_ERR(srp_dev->rkey_mr))
+			goto err_pd;
+	} else
+		srp_dev->rkey_mr = NULL;
+
 
 	for (p = rdma_start_port(device); p <= rdma_end_port(device); ++p) {
 		host = srp_add_port(srp_dev, p);
@@ -3495,7 +3505,8 @@ static void srp_remove_one(struct ib_device *device)
 		kfree(host);
 	}
 
-	ib_dereg_mr(srp_dev->mr);
+	if (srp_dev->rkey_mr)
+		ib_dereg_mr(srp_dev->rkey_mr);
 	ib_dealloc_pd(srp_dev->pd);
 
 	kfree(srp_dev);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 17ee3f80ba55..8b241f17f8b8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -95,7 +95,7 @@ struct srp_device {
 	struct list_head	dev_list;
 	struct ib_device       *dev;
 	struct ib_pd	       *pd;
-	struct ib_mr	       *mr;
+	struct ib_mr	       *rkey_mr;
 	u64			mr_page_mask;
 	int			mr_page_size;
 	int			mr_max_size;
-- 
2.1.4

[PATCH v2 10/12] ib_srpt: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 15 ++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  1 -
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 60ff0a2390e5..20adb96ba0b2 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -783,7 +783,7 @@ static int srpt_post_recv(struct srpt_device *sdev,
 
 	list.addr = ioctx->ioctx.dma;
 	list.length = srp_max_req_size;
-	list.lkey = sdev->mr->lkey;
+	list.lkey = sdev->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	wr.sg_list = &list;
@@ -818,7 +818,7 @@ static int srpt_post_send(struct srpt_rdma_ch *ch,
 
 	list.addr = ioctx->ioctx.dma;
 	list.length = len;
-	list.lkey = sdev->mr->lkey;
+	list.lkey = sdev->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	wr.wr_id = encode_wr_id(SRPT_SEND, ioctx->ioctx.index);
@@ -1206,7 +1206,7 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
 
 		while (rsize > 0 && tsize > 0) {
 			sge->addr = dma_addr;
-			sge->lkey = ch->sport->sdev->mr->lkey;
+			sge->lkey = ch->sport->sdev->pd->local_dma_lkey;
 
 			if (rsize >= dma_len) {
 				sge->length =
@@ -3211,10 +3211,6 @@ static void srpt_add_one(struct ib_device *device)
 	if (IS_ERR(sdev->pd))
 		goto free_dev;
 
-	sdev->mr = ib_get_dma_mr(sdev->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(sdev->mr))
-		goto err_pd;
-
 	sdev->srq_size = min(srpt_srq_size, sdev->dev_attr.max_srq_wr);
 
 	srq_attr.event_handler = srpt_srq_event;
@@ -3226,7 +3222,7 @@ static void srpt_add_one(struct ib_device *device)
 
 	sdev->srq = ib_create_srq(sdev->pd, &srq_attr);
 	if (IS_ERR(sdev->srq))
-		goto err_mr;
+		goto err_pd;
 
 	pr_debug("%s: create SRQ #wr= %d max_allow=%d dev= %s\n",
 		 __func__, sdev->srq_size, sdev->dev_attr.max_srq_wr,
@@ -3311,8 +3307,6 @@ err_cm:
 	ib_destroy_cm_id(sdev->cm_id);
 err_srq:
 	ib_destroy_srq(sdev->srq);
-err_mr:
-	ib_dereg_mr(sdev->mr);
 err_pd:
 	ib_dealloc_pd(sdev->pd);
 free_dev:
@@ -3358,7 +3352,6 @@ static void srpt_remove_one(struct ib_device *device)
 	srpt_release_sdev(sdev);
 
 	ib_destroy_srq(sdev->srq);
-	ib_dereg_mr(sdev->mr);
 	ib_dealloc_pd(sdev->pd);
 
 	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 21f8df67522a..5faad8acd789 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -393,7 +393,6 @@ struct srpt_port {
 struct srpt_device {
 	struct ib_device	*device;
 	struct ib_pd		*pd;
-	struct ib_mr		*mr;
 	struct ib_srq		*srq;
 	struct ib_cm_id		*cm_id;
 	struct ib_device_attr	dev_attr;
-- 
2.1.4

[PATCH v2 11/12] net/9p: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Dominique Martinet <dominique.martinet@cea.fr>
---
 net/9p/trans_rdma.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 37a78d20c0f6..ba1210253f5e 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -94,8 +94,6 @@ struct p9_trans_rdma {
 	struct ib_pd *pd;
 	struct ib_qp *qp;
 	struct ib_cq *cq;
-	struct ib_mr *dma_mr;
-	u32 lkey;
 	long timeout;
 	int sq_depth;
 	struct semaphore sq_sem;
@@ -382,9 +380,6 @@ static void rdma_destroy_trans(struct p9_trans_rdma *rdma)
 	if (!rdma)
 		return;
 
-	if (rdma->dma_mr && !IS_ERR(rdma->dma_mr))
-		ib_dereg_mr(rdma->dma_mr);
-
 	if (rdma->qp && !IS_ERR(rdma->qp))
 		ib_destroy_qp(rdma->qp);
 
@@ -415,7 +410,7 @@ post_recv(struct p9_client *client, struct p9_rdma_context *c)
 
 	sge.addr = c->busa;
 	sge.length = client->msize;
-	sge.lkey = rdma->lkey;
+	sge.lkey = rdma->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	c->wc_op = IB_WC_RECV;
@@ -506,7 +501,7 @@ dont_need_post_recv:
 
 	sge.addr = c->busa;
 	sge.length = c->req->tc->size;
-	sge.lkey = rdma->lkey;
+	sge.lkey = rdma->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	c->wc_op = IB_WC_SEND;
@@ -647,7 +642,6 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args)
 	struct p9_trans_rdma *rdma;
 	struct rdma_conn_param conn_param;
 	struct ib_qp_init_attr qp_attr;
-	struct ib_device_attr devattr;
 	struct ib_cq_init_attr cq_attr = {};
 
 	/* Parse the transport specific mount options */
@@ -700,11 +694,6 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args)
 	if (err || (rdma->state != P9_RDMA_ROUTE_RESOLVED))
 		goto error;
 
-	/* Query the device attributes */
-	err = ib_query_device(rdma->cm_id->device, &devattr);
-	if (err)
-		goto error;
-
 	/* Create the Completion Queue */
 	cq_attr.cqe = opts.sq_depth + opts.rq_depth + 1;
 	rdma->cq = ib_create_cq(rdma->cm_id->device, cq_comp_handler,
@@ -719,17 +708,6 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args)
 	if (IS_ERR(rdma->pd))
 		goto error;
 
-	/* Cache the DMA lkey in the transport */
-	rdma->dma_mr = NULL;
-	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY)
-		rdma->lkey = rdma->cm_id->device->local_dma_lkey;
-	else {
-		rdma->dma_mr = ib_get_dma_mr(rdma->pd, IB_ACCESS_LOCAL_WRITE);
-		if (IS_ERR(rdma->dma_mr))
-			goto error;
-		rdma->lkey = rdma->dma_mr->lkey;
-	}
-
 	/* Create the Queue Pair */
 	memset(&qp_attr, 0, sizeof qp_attr);
 	qp_attr.event_handler = qp_event_handler;
-- 
2.1.4

[PATCH v2 12/12] rds/ib: Remove ib_get_dma_mr calls

[Jason Gunthorpe]

The pd now has a local_dma_lkey member which completely replaces
ib_get_dma_mr, use it instead.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 net/rds/ib.c      | 8 --------
 net/rds/ib.h      | 2 --
 net/rds/ib_cm.c   | 4 +---
 net/rds/ib_recv.c | 6 +++---
 net/rds/ib_send.c | 8 ++++----
 5 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/net/rds/ib.c b/net/rds/ib.c
index ba2dffeff608..56c570131667 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -99,8 +99,6 @@ static void rds_ib_dev_free(struct work_struct *work)
 
 	if (rds_ibdev->mr_pool)
 		rds_ib_destroy_mr_pool(rds_ibdev->mr_pool);
-	if (rds_ibdev->mr)
-		ib_dereg_mr(rds_ibdev->mr);
 	if (rds_ibdev->pd)
 		ib_dealloc_pd(rds_ibdev->pd);
 
@@ -164,12 +162,6 @@ static void rds_ib_add_one(struct ib_device *device)
 		goto put_dev;
 	}
 
-	rds_ibdev->mr = ib_get_dma_mr(rds_ibdev->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(rds_ibdev->mr)) {
-		rds_ibdev->mr = NULL;
-		goto put_dev;
-	}
-
 	rds_ibdev->mr_pool = rds_ib_create_mr_pool(rds_ibdev);
 	if (IS_ERR(rds_ibdev->mr_pool)) {
 		rds_ibdev->mr_pool = NULL;
diff --git a/net/rds/ib.h b/net/rds/ib.h
index 86d88ec5d556..36f7d808ffaa 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -100,7 +100,6 @@ struct rds_ib_connection {
 	/* alphabet soup, IBTA style */
 	struct rdma_cm_id	*i_cm_id;
 	struct ib_pd		*i_pd;
-	struct ib_mr		*i_mr;
 	struct ib_cq		*i_send_cq;
 	struct ib_cq		*i_recv_cq;
 
@@ -173,7 +172,6 @@ struct rds_ib_device {
 	struct list_head	conn_list;
 	struct ib_device	*dev;
 	struct ib_pd		*pd;
-	struct ib_mr		*mr;
 	struct rds_ib_mr_pool	*mr_pool;
 	unsigned int		fmr_max_remaps;
 	unsigned int		max_fmrs;
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 0da2a45b33bd..a75e8832bc23 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -269,7 +269,6 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
 
 	/* Protection domain and memory range */
 	ic->i_pd = rds_ibdev->pd;
-	ic->i_mr = rds_ibdev->mr;
 
 	cq_attr.cqe = ic->i_send_ring.w_nr + 1;
 	ic->i_send_cq = ib_create_cq(dev, rds_ib_send_cq_comp_handler,
@@ -375,7 +374,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
 
 	rds_ib_recv_init_ack(ic);
 
-	rdsdebug("conn %p pd %p mr %p cq %p %p\n", conn, ic->i_pd, ic->i_mr,
+	rdsdebug("conn %p pd %p cq %p %p\n", conn, ic->i_pd,
 		 ic->i_send_cq, ic->i_recv_cq);
 
 out:
@@ -678,7 +677,6 @@ void rds_ib_conn_shutdown(struct rds_connection *conn)
 
 		ic->i_cm_id = NULL;
 		ic->i_pd = NULL;
-		ic->i_mr = NULL;
 		ic->i_send_cq = NULL;
 		ic->i_recv_cq = NULL;
 		ic->i_send_hdrs = NULL;
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index cac5b4506ee3..0ceb4c60d2a3 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -62,12 +62,12 @@ void rds_ib_recv_init_ring(struct rds_ib_connection *ic)
 		sge = &recv->r_sge[0];
 		sge->addr = ic->i_recv_hdrs_dma + (i * sizeof(struct rds_header));
 		sge->length = sizeof(struct rds_header);
-		sge->lkey = ic->i_mr->lkey;
+		sge->lkey = ic->i_pd->local_dma_lkey;
 
 		sge = &recv->r_sge[1];
 		sge->addr = 0;
 		sge->length = RDS_FRAG_SIZE;
-		sge->lkey = ic->i_mr->lkey;
+		sge->lkey = ic->i_pd->local_dma_lkey;
 	}
 }
 
@@ -520,7 +520,7 @@ void rds_ib_recv_init_ack(struct rds_ib_connection *ic)
 
 	sge->addr = ic->i_ack_dma;
 	sge->length = sizeof(struct rds_header);
-	sge->lkey = ic->i_mr->lkey;
+	sge->lkey = ic->i_pd->local_dma_lkey;
 
 	wr->sg_list = sge;
 	wr->num_sge = 1;
diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index 5d0a704fa039..f6c829d43373 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -202,9 +202,9 @@ void rds_ib_send_init_ring(struct rds_ib_connection *ic)
 		sge = &send->s_sge[0];
 		sge->addr = ic->i_send_hdrs_dma + (i * sizeof(struct rds_header));
 		sge->length = sizeof(struct rds_header);
-		sge->lkey = ic->i_mr->lkey;
+		sge->lkey = ic->i_pd->local_dma_lkey;
 
-		send->s_sge[1].lkey = ic->i_mr->lkey;
+		send->s_sge[1].lkey = ic->i_pd->local_dma_lkey;
 	}
 }
 
@@ -813,7 +813,7 @@ int rds_ib_xmit_atomic(struct rds_connection *conn, struct rm_atomic_op *op)
 	/* Convert our struct scatterlist to struct ib_sge */
 	send->s_sge[0].addr = ib_sg_dma_address(ic->i_cm_id->device, op->op_sg);
 	send->s_sge[0].length = ib_sg_dma_len(ic->i_cm_id->device, op->op_sg);
-	send->s_sge[0].lkey = ic->i_mr->lkey;
+	send->s_sge[0].lkey = ic->i_pd->local_dma_lkey;
 
 	rdsdebug("rva %Lx rpa %Lx len %u\n", op->op_remote_addr,
 		 send->s_sge[0].addr, send->s_sge[0].length);
@@ -927,7 +927,7 @@ int rds_ib_xmit_rdma(struct rds_connection *conn, struct rm_rdma_op *op)
 			send->s_sge[j].addr =
 				 ib_sg_dma_address(ic->i_cm_id->device, scat);
 			send->s_sge[j].length = len;
-			send->s_sge[j].lkey = ic->i_mr->lkey;
+			send->s_sge[j].lkey = ic->i_pd->local_dma_lkey;
 
 			sent += len;
 			rdsdebug("ic %p sent %d remote_addr %llu\n", ic, sent, remote_addr);
-- 
2.1.4

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Christoph Hellwig]

Hi Jason,

this series look fine to me, although I don't feel comfortable enough
to give a Reviewed-by: tag for the RDMA subsystem yet.

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Steve Wise]

Series looks good.

Reviewed-by: Steve Wise <swise@opengridcomputing.com>

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 07/30/2015 04:22 PM, Jason Gunthorpe wrote:
> All patches are compile tested. I've done basic testing up to and including
> the IPoIB patch, the rest required specialized setups I don't have access to,
> but are fairly straightforward.

Hello Jason,

SRP login fails with this patch series applied on top of Linux kernel 
v4.2-rc4. At the target side the following message appears every time 
the SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed 
with status 10" (10=remote access error). That causes the initiator to 
receive a flush error (5).

Bart.

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

On Fri, Jul 31, 2015 at 03:20:40PM -0700, Bart Van Assche wrote:
> On 07/30/2015 04:22 PM, Jason Gunthorpe wrote:
> >All patches are compile tested. I've done basic testing up to and including
> >the IPoIB patch, the rest required specialized setups I don't have access to,
> >but are fairly straightforward.
> 
> Hello Jason,
> 
> SRP login fails with this patch series applied on top of Linux kernel
> v4.2-rc4. At the target side the following message appears every time the
> SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with
> status 10" (10=remote access error). That causes the initiator to receive a
> flush error (5).

Sounds like a bad rkey was sent to the target? I'm guessing #9 is the
cause? I'm guessing that target->rkey is being used someplace even though
register_always is set?

Doug, in this case, I'd just drop the impacted SRP patches.. Hopefully
just #9...

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 07/31/2015 03:32 PM, Jason Gunthorpe wrote:
> On Fri, Jul 31, 2015 at 03:20:40PM -0700, Bart Van Assche wrote:
>> On 07/30/2015 04:22 PM, Jason Gunthorpe wrote:
>>> All patches are compile tested. I've done basic testing up to and including
>>> the IPoIB patch, the rest required specialized setups I don't have access to,
>>> but are fairly straightforward.
>>
>> Hello Jason,
>>
>> SRP login fails with this patch series applied on top of Linux kernel
>> v4.2-rc4. At the target side the following message appears every time the
>> SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with
>> status 10" (10=remote access error). That causes the initiator to receive a
>> flush error (5).
>
> Sounds like a bad rkey was sent to the target? I'm guessing #9 is the
> cause? I'm guessing that target->rkey is being used someplace even though
> register_always is set?
>
> Doug, in this case, I'd just drop the impacted SRP patches.. Hopefully
> just #9...

Hello Jason,

With only patches 1/12 and 8/12 applied my test passes.

Bart.

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

Re: [PATCH v2 08/12] IB/srp: Use pd->local_dma_lkey

[Bart Van Assche]

On 07/30/2015 04:22 PM, Jason Gunthorpe wrote:
> Replace all leys with  pd->local_dma_lkey. This driver does not support
> iWarp, so this is safe.
>
> The insecure use of ib_get_dma_mr is thus isolated to an rkey, and will
> have to be fixed separately.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 31a20b462266..19a1356f8b2a 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -3146,7 +3146,7 @@ static ssize_t srp_create_target(struct device *dev,
>   	target->io_class	= SRP_REV16A_IB_IO_CLASS;
>   	target->scsi_host	= target_host;
>   	target->srp_host	= host;
> -	target->lkey		= host->srp_dev->mr->lkey;
> +	target->lkey		= host->srp_dev->pd->local_dma_lkey;
>   	target->rkey		= host->srp_dev->mr->rkey;
>   	target->cmd_sg_cnt	= cmd_sg_entries;
>   	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
>

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

On Fri, Jul 31, 2015 at 04:04:35PM -0700, Bart Van Assche wrote:
 
> With only patches 1/12 and 8/12 applied my test passes.

Thanks Bart!

If you like, I can try and work on #9 (no promises :(), but I think
I'll need some way to test it here.

Do you by chance have a straightforward recipe to setup SRP and SRPT
on two Linux's for this simple purpose?

Doug, just drop #9 'IB/srp: Do not create an all physical insecure
rkey by default' please, thanks.

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On 07/31/2015 04:14 PM, Jason Gunthorpe wrote:
> Do you by chance have a straightforward recipe to setup SRP and SRPT
> on two Linux's for this simple purpose?

Some time ago I wrote a script that loads the upstream SRP target driver 
and creates two LUNs for local RAM disks (attached to this e-mail). The 
only disadvantage of that script is that it is based on the obsolete 
lio-utils package.

In case you would prefer the SCST SRP target driver, the following 
instructions should be sufficient to build, install, load and configure 
it on an RPM based system (please replace the GUID shown below by a GUID 
of a port of the HCA in your the target system):

svn co svn://svn.code.sf.net/p/scst/svn/trunk scst-trunk ||
   git clone https://github.com/bvanassche/scst.git scst-trunk
cd scst-trunk
make rpm
su
rpm -U {,scstadmin/}rpmbuilddir/RPMS/x86_64/*.rpm
cat <<EOF >/etc/scst.conf
HANDLER vdisk_blockio {
     DEVICE brd {
	filename /dev/ram0
     }
}
TARGET_DRIVER ib_srpt {
     TARGET fe80:0000:0000:0000:24be:05ff:ffa9:cbb1 {
         enabled 1
         LUN 0 brd
     }
}
EOF
modprobe brd
/etc/init.d/scst restart

Bart.

[-- Attachment #2: restart-lio-srpt --]
[-- Type: text/plain, Size: 3426 bytes --]

#!/bin/bash

filesize=$((16*1024*1024))
hcas="$(cat /sys/devices/*/*/*/infiniband/*/ports/*/gids/0 | sed 's/^\(....\):\(....\):\(....\):\(....\):\(....\):\(....\):\(....\):\(....\)$/0x0000\2\3\4\5\6\7\8 0x\5\6\7\8 0x\1\2\3\4\5\6\7\8/')"
# Output of cat /sys/devices/*/*/*/infiniband/*/ports/*/gids/0 on the remote
# systems
initiators="\
0x00000000000000000002c9030003cca7 \
0x00000000000000000002c9030003cca8 \
0x00000000000000000002c9030005f34f \
0x00000000000000000002c9030005f350 \
0x00000000000000000002c90300a34271 \
0x00000000000000000002c90300a34272 \
0x00000000000000000002c90300fab7f1 \
0x00000000000000000002c90300fab7f2"

if [ ! -e /sys/module/configfs ]; then
  modprobe configfs
fi
if ! mount | grep -qw configfs; then
  mount -t configfs none /sys/kernel/config
fi

if cd /sys/kernel/config/target >&/dev/null; then
  for hca in ${hcas}; do
    if [ -e srpt/$hca/$hca/enable ]; then
      echo 0 >srpt/$hca/$hca/enable
    fi
  done
fi
cd /

rm -f /sys/kernel/config/target/srpt/*/*/acls/*/*/* >&/dev/null
rmdir /sys/kernel/config/target/srpt/*/*/acls/*/* >&/dev/null
rmdir /sys/kernel/config/target/srpt/*/*/acls/* >&/dev/null
rm -f /sys/kernel/config/target/srpt/*/*/lun/*/* >&/dev/null
rmdir /sys/kernel/config/target/srpt/*/*/lun/* >&/dev/null
rmdir /sys/kernel/config/target/srpt/*/* >&/dev/null
rmdir /sys/kernel/config/target/srpt/* >&/dev/null
rmdir /sys/kernel/config/target/srpt >&/dev/null

if [ -e /sys/module/ib_srpt ]; then
  rmmod ib_srpt
fi

rmdir /sys/kernel/config/target/core/*/* >&/dev/null
rmdir /sys/kernel/config/target/core/* >&/dev/null

#find /sys/kernel/config/target

for m in ib_srpt target_core_pscsi target_core_iblock target_core_file target_core_stgt target_core_user target_core_mod
do
  if [ -e /sys/module/$m ]; then
    rmmod $m
  fi
done

if [ "$1" = "stop" ]; then
  exit 0
fi

modprobe target_core_mod

if [ -e /sys/kernel/debug/dynamic_debug/control ]; then
  #echo 'module target_core_mod +p' > /sys/kernel/debug/dynamic_debug/control
  :
fi

insmod /lib/modules/$(uname -r)/kernel/drivers/infiniband/ulp/srpt/ib_srpt.ko srp_max_req_size=4200 || exit $?

if [ -e /sys/kernel/debug/dynamic_debug/control ]; then
  echo 'module ib_srpt +p' > /sys/kernel/debug/dynamic_debug/control
fi

modprobe target_core_file || exit $?
if [ ! -e /dev/ramdisk ]; then 
  dd if=/dev/zero of=/dev/ramdisk bs=${filesize} count=1
fi

if false && cd /sys/kernel/debug/tracing; then
  echo function >current_tracer
  { echo 'srpt_*'; echo 'transport_*'; } >set_ftrace_filter
  echo 1 >tracing_on
fi

vdev0="fileio_0/vdev0"
vdev1="fileio_1/vdev2"
vdev2="rd_dr_0/vdev1"
vdevs="$vdev0 $vdev1"
vdev_count=2
tcm_node --fileio $vdev0 /dev/ramdisk ${filesize}
tcm_node --fileio $vdev1 /dev/ramdisk 65536
if [ -e /sys/kernel/config/target/core/rd_dr_0 ]; then
  tcm_node --ramdisk $vdev2 2
  vdevs="$vdevs vdev2"
  vdev_count=3
fi
cd /sys/kernel/config/target || exit $?
mkdir srpt || exit $?
cd srpt || exit $?
for hca in ${hcas}; do
  mkdir $hca
  [ -e $hca ] || continue
  echo $hca
  mkdir $hca/$hca
  i=0
  for v in ${vdevs}; do
    mkdir $hca/$hca/lun/lun_$i
    ( cd $hca/$hca/lun/lun_$i && ln -s ../../../../../core/$v )
    i=$((i+1))
  done
  for ini in ${initiators}; do
  (
    cd $hca/$hca/acls
    mkdir ${ini}
    cd ${ini}
    for ((i = 0; i < $vdev_count; i++)) do
      ( mkdir lun_$i && cd lun_$i && ln -s ../../../lun/lun_$i )
    done
  )
  echo 1 >$hca/$hca/enable
  done
done

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On 07/31/2015 07:14 PM, Jason Gunthorpe wrote:
> On Fri, Jul 31, 2015 at 04:04:35PM -0700, Bart Van Assche wrote:
>  
>> With only patches 1/12 and 8/12 applied my test passes.
> 
> Thanks Bart!
> 
> If you like, I can try and work on #9 (no promises :(), but I think
> I'll need some way to test it here.
> 
> Do you by chance have a straightforward recipe to setup SRP and SRPT
> on two Linux's for this simple purpose?
> 
> Doug, just drop #9 'IB/srp: Do not create an all physical insecure
> rkey by default' please, thanks.

I've pulled in the series for 4.3.  I've included patch #9, but that's
on the understanding that if it can't be fixed before the initial pull
request, I'll drop it at that time.  I have the resources to test this,
so I can work on it, and that factors into my path forward here.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 01/12] IB/core: Guarantee that a local_dma_lkey is available

[Haggai Eran]

On 31/07/2015 02:22, Jason Gunthorpe wrote:
>  int ib_dealloc_pd(struct ib_pd *pd)
>  {
> +	if (pd->local_mr) {
> +		if (ib_dereg_mr(pd->local_mr))
> +			return -EBUSY;
> +		pd->local_mr = NULL;
> +	}
> +

It looks like ib_uverbs_alloc_pd calls ib_device.alloc_pd() directly,
and some drivers don't use kzalloc for allocating the pd, so the
ib_dereg_mr call above results in a general protection fault.

Haggai

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Christoph Hellwig]

On Fri, Jul 31, 2015 at 03:20:40PM -0700, Bart Van Assche wrote:
> On 07/30/2015 04:22 PM, Jason Gunthorpe wrote:
> >All patches are compile tested. I've done basic testing up to and including
> >the IPoIB patch, the rest required specialized setups I don't have access to,
> >but are fairly straightforward.
> 
> Hello Jason,
> 
> SRP login fails with this patch series applied on top of Linux kernel
> v4.2-rc4. At the target side the following message appears every time the
> SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with
> status 10" (10=remote access error). That causes the initiator to receive a
> flush error (5).

Could this be caused because srp_map_sg_entry falls back to the phys
mapping for unaligned large requests in line 1389 in Dougs tree:

	if ((!dev->use_fast_reg && dma_addr & ~dev->mr_page_mask) ||
	    dma_len > dev->mr_max_size) {
		ret = srp_finish_mapping(state, ch);
		if (ret)
			return ret;

		srp_map_desc(state, dma_addr, dma_len, target->rkey);
		srp_map_update_start(state, NULL, 0, 0);
		return 0;
	}

Bart, do you know what hardware this workaround is for?

Also the SRP driver still falls back to phys registrations if the
better MR methods fail, something that will stop working with
Jasons patch.  It's something that looks a little questionable
and which other ULDs don't do either, so it would be good time
to review this practice.

Additionally the SRP_DATA_DESC_INDIRECT case always uses the global
rkey, so whenever we hits this things are going to break.

Jason, I suspect it might be a good idea to rename the rkey field
to something like global_rkey, and make sure it's guarded by a proper
conditional shared by all users.

Re: [PATCH v2 09/12] IB/srp: Do not create an all physical insecure rkey by default

[Christoph Hellwig]

In addition to the comments on the cover letter I think your
changes to srp_add_one could use this incremental cleanup:

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a546256..5e2cb53 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3381,7 +3381,7 @@ static void srp_add_one(struct ib_device *device)
 	struct srp_host *host;
 	int mr_page_shift, p;
 	u64 max_pages_per_mr;
-	unsigned int mr_flags = 0;
+	bool need_phys_rkey = false;
 
 	dev_attr = kmalloc(sizeof *dev_attr, GFP_KERNEL);
 	if (!dev_attr)
@@ -3403,7 +3403,7 @@ static void srp_add_one(struct ib_device *device)
 	if (!srp_dev->has_fmr && !srp_dev->has_fr) {
 		dev_warn(&device->dev, "neither FMR nor FR is supported\n");
 		/* Fall back to using an insecure all physical rkey */
-		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+		need_phys_rkey = true;
 	}
 
 	srp_dev->use_fast_reg = (srp_dev->has_fr &&
@@ -3441,11 +3441,13 @@ static void srp_add_one(struct ib_device *device)
 		goto free_dev;
 
 	if (!register_always)
-		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+		need_phys_rkey = true;
 
-	if (mr_flags) {
+	if (need_phys_rkey) {
 		srp_dev->rkey_mr = ib_get_dma_mr(
-		    srp_dev->pd, IB_ACCESS_LOCAL_WRITE | mr_flags);
+		    srp_dev->pd, IB_ACCESS_LOCAL_WRITE |
+		    		 IB_ACCESS_REMOTE_READ |
+				 IB_ACCESS_REMOTE_WRITE);
 		if (IS_ERR(srp_dev->rkey_mr))
 			goto err_pd;
 	} else

Re: [PATCH v2 09/12] IB/srp: Do not create an all physical insecure rkey by default

[Bart Van Assche]

On 07/30/2015 04:22 PM, Jason Gunthorpe wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 19a1356f8b2a..a8003079c232 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -69,7 +69,7 @@ static unsigned int cmd_sg_entries;
>   static unsigned int indirect_sg_entries;
>   static bool allow_ext_sg;
>   static bool prefer_fr;
> -static bool register_always;
> +static bool register_always = true;
>   static int topspin_workarounds = 1;

Hello Jason,

Can you modify this patch such that the default value of prefer_fr is 
changed from false into true ? The only reason I had not made fast 
registration the default myself when I introduced FR support in the SRP 
initiator driver is because at that time the FR code in the SRP 
initiator was brand new. However, since the introduction of FR support 
in the SRP initiator that code has been tested extensively (at least 
inside SanDisk).

Thanks,

Bart.

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/03/2015 08:24 AM, Christoph Hellwig wrote:
> On Fri, Jul 31, 2015 at 03:20:40PM -0700, Bart Van Assche wrote:
>> SRP login fails with this patch series applied on top of Linux kernel
>> v4.2-rc4. At the target side the following message appears every time the
>> SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with
>> status 10" (10=remote access error). That causes the initiator to receive a
>> flush error (5).
>
> Could this be caused because srp_map_sg_entry falls back to the phys
> mapping for unaligned large requests in line 1389 in Dougs tree:
>
> 	if ((!dev->use_fast_reg && dma_addr & ~dev->mr_page_mask) ||
> 	    dma_len > dev->mr_max_size) {
> 		ret = srp_finish_mapping(state, ch);
> 		if (ret)
> 			return ret;
>
> 		srp_map_desc(state, dma_addr, dma_len, target->rkey);
> 		srp_map_update_start(state, NULL, 0, 0);
> 		return 0;
> 	}

This morning I added WARN_ON_ONCE(!target->rkey) statements in the code
paths that use target->rkey and that revealed that the above code
was indeed the culprit. Changing the default registration method from 
FMR into FR should be sufficient to avoid that the above code is 
triggered for HCA's that support FR (e.g. ConnectX-3 and ConnectX-4). 
For HCA's that support FMR but not FR, how about telling the block layer 
that data for the SRP initiator driver should be aligned at a 4KB 
boundary ? In srp_add_one() one can see that the finest granularity the 
SRP initiator driver supports for memory registration is 4KB 
(mr_page_shift = ...).

> Bart, do you know what hardware this workaround is for?

I hope the HW vendors can comment on this. Sorry but I'm not sure which 
HCA models and/or firmware versions do not support FMR mapping with a 
non-zero offset.

 > Also the SRP driver still falls back to phys registrations if the
 > better MR methods fail, something that will stop working with
 > Jasons patch.  It's something that looks a little questionable
 > and which other ULDs don't do either, so it would be good time
 > to review this practice.

Agreed.

> Additionally the SRP_DATA_DESC_INDIRECT case always uses the global
> rkey, so whenever we hits this things are going to break.

Indeed ... I think avoiding passing the global rkey over the wire in 
this case means registering the indirect table explicitly. Some SRP 
target implementations (e.g. the upstream ib_srpt driver) do not support 
partial indirect tables and fetch the indirect table from the SRP_CMD 
information unit. This means that the upstream ib_srpt driver never uses 
the rkey that is passed in the indirect data buffer descriptor. Although 
the upstream SRP target driver would keep working fine if an invalid 
rkey would be passed in the indirect data buffer header, I haven't found 
any clause in the SRP specification that allows to do this even if the 
entire indirect descriptor is passed in the SRP_CMD IU.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/12] IB/core: Guarantee that a local_dma_lkey is available

[Jason Gunthorpe]

On Sun, Aug 02, 2015 at 04:09:01PM +0300, Haggai Eran wrote:
> On 31/07/2015 02:22, Jason Gunthorpe wrote:
> >  int ib_dealloc_pd(struct ib_pd *pd)
> >  {
> > +	if (pd->local_mr) {
> > +		if (ib_dereg_mr(pd->local_mr))
> > +			return -EBUSY;
> > +		pd->local_mr = NULL;
> > +	}
> > +
> 
> It looks like ib_uverbs_alloc_pd calls ib_device.alloc_pd() directly,
> and some drivers don't use kzalloc for allocating the pd, so the
> ib_dereg_mr call above results in a general protection fault.

Indeed it does..

I'll clean up the dealloc_pd mess too.

Jason

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

On Mon, Aug 03, 2015 at 11:33:51AM -0700, Bart Van Assche wrote:
> >Bart, do you know what hardware this workaround is for?
> 
> I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA
> models and/or firmware versions do not support FMR mapping with a non-zero
> offset.

Perhaps David can remember why he added this:

commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
    IB/srp: rework mapping engine to use multiple FMR entries
    
+       /* If we start at an offset into the FMR page, don't merge into
+        * the current FMR. Finish it out, and use the kernel's MR for this
+        * sg entry. This is to avoid potential bugs on some SRP targets
+        * that were never quite defined, but went away when the initiator
+        * avoided using FMR on such page fragments.
+        */
+       if (dma_addr & ~dev->fmr_page_mask || dma_len > dev->fmr_max_size) {
+               ret = srp_map_finish_fmr(state, target);
+               if (ret)
+                       return ret;
+
+               srp_map_desc(state, dma_addr, dma_len, target->rkey);
+               srp_map_update_start(state, NULL, 0, 0);
+               return 0;
        }

The way it is phrased makes it seem like a target bug, not a HCA bug?

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[David Dillow]

On Tue, 2015-08-04 at 12:09 -0600, Jason Gunthorpe wrote:
> On Mon, Aug 03, 2015 at 11:33:51AM -0700, Bart Van Assche wrote:
> > >Bart, do you know what hardware this workaround is for?
> > 
> > I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA
> > models and/or firmware versions do not support FMR mapping with a non-zero
> > offset.
> 
> Perhaps David can remember why he added this:
> 
> commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c

It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4,
and the list's attempts at code archaeology failed us: 
http://article.gmane.org/gmane.linux.drivers.rdma/7149

> The way it is phrased makes it seem like a target bug, not a HCA bug?

Yes, the original commit is explicit about it being a target issue.
Since it mentioned the boogeyman words "data corruption", I kept the
work around in the FMR work. 

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

On Tue, Aug 04, 2015 at 11:41:16PM -0700, David Dillow wrote:
> On Tue, 2015-08-04 at 12:09 -0600, Jason Gunthorpe wrote:
> > On Mon, Aug 03, 2015 at 11:33:51AM -0700, Bart Van Assche wrote:
> > > >Bart, do you know what hardware this workaround is for?
> > > 
> > > I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA
> > > models and/or firmware versions do not support FMR mapping with a non-zero
> > > offset.
> > 
> > Perhaps David can remember why he added this:
> > 
> > commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
> 
> It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4,
> and the list's attempts at code archaeology failed us: 
> http://article.gmane.org/gmane.linux.drivers.rdma/7149

Okay.. So over time we went from a clear target specific bug described
9 years ago in 559ce through chinese whispers to a general unspecific
fear of non-zero offset FMR?

But nobody has described FMR failure in this way in the past 9 years
with any specificity?

My random guesses would be broken mthca firmware at the start of the
FMR feature (long since fixed) or a wonky target that is now 10 years
obsolete..

If it was an HCA bug, I strongly have to think it is fixed now. We use
FMR all over the place and SRP is the only area I've noticed this
restriction..

If it is a target bug, then FRWR should trigger it as wel, so we are
already about to revert that workaround.

I'm inclined to drop this entirely.. What do you think Bart?

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/05/2015 12:51 PM, Jason Gunthorpe wrote:
> On Tue, Aug 04, 2015 at 11:41:16PM -0700, David Dillow wrote:
>> On Tue, 2015-08-04 at 12:09 -0600, Jason Gunthorpe wrote:
>>> On Mon, Aug 03, 2015 at 11:33:51AM -0700, Bart Van Assche wrote:
>>>>> Bart, do you know what hardware this workaround is for?
>>>>
>>>> I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA
>>>> models and/or firmware versions do not support FMR mapping with a non-zero
>>>> offset.
>>>
>>> Perhaps David can remember why he added this:
>>>
>>> commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
>>
>> It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4,
>> and the list's attempts at code archaeology failed us:
>> http://article.gmane.org/gmane.linux.drivers.rdma/7149
>
> Okay.. So over time we went from a clear target specific bug described
> 9 years ago in 559ce through chinese whispers to a general unspecific
> fear of non-zero offset FMR?
>
> But nobody has described FMR failure in this way in the past 9 years
> with any specificity?
>
> My random guesses would be broken mthca firmware at the start of the
> FMR feature (long since fixed) or a wonky target that is now 10 years
> obsolete..
>
> If it was an HCA bug, I strongly have to think it is fixed now. We use
> FMR all over the place and SRP is the only area I've noticed this
> restriction..
>
> If it is a target bug, then FRWR should trigger it as wel, so we are
> already about to revert that workaround.
>
> I'm inclined to drop this entirely.. What do you think Bart?

Even today I see memory corruption at the initiator side with FMR and 
not with FR if I leave out the alignment check. Since this only occurs 
with FMR and not with FR that excludes the target side as a possible 
cause. I will have a closer look at the srp_map_finish_fmr() function. 
Always passing 0 as the second argument of srp_map_desc() in 
srp_map_finish_fmr() is probably wrong. Before 2011 that second argument 
was the page offset of the first sg-list entry.

Bart.

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/05/2015 02:45 PM, Bart Van Assche wrote:
> On 08/05/2015 12:51 PM, Jason Gunthorpe wrote:
>> On Tue, Aug 04, 2015 at 11:41:16PM -0700, David Dillow wrote:
>>> On Tue, 2015-08-04 at 12:09 -0600, Jason Gunthorpe wrote:
>>>> On Mon, Aug 03, 2015 at 11:33:51AM -0700, Bart Van Assche wrote:
>>>>>> Bart, do you know what hardware this workaround is for?
>>>>>
>>>>> I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA
>>>>> models and/or firmware versions do not support FMR mapping with a non-zero
>>>>> offset.
>>>>
>>>> Perhaps David can remember why he added this:
>>>>
>>>> commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
>>>
>>> It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4,
>>> and the list's attempts at code archaeology failed us:
>>> http://article.gmane.org/gmane.linux.drivers.rdma/7149
>>
>> Okay.. So over time we went from a clear target specific bug described
>> 9 years ago in 559ce through chinese whispers to a general unspecific
>> fear of non-zero offset FMR?
>>
>> But nobody has described FMR failure in this way in the past 9 years
>> with any specificity?
>>
>> My random guesses would be broken mthca firmware at the start of the
>> FMR feature (long since fixed) or a wonky target that is now 10 years
>> obsolete..
>>
>> If it was an HCA bug, I strongly have to think it is fixed now. We use
>> FMR all over the place and SRP is the only area I've noticed this
>> restriction..
>>
>> If it is a target bug, then FRWR should trigger it as wel, so we are
>> already about to revert that workaround.
>>
>> I'm inclined to drop this entirely.. What do you think Bart?
>
> Even today I see memory corruption at the initiator side with FMR and
> not with FR if I leave out the alignment check. Since this only occurs
> with FMR and not with FR that excludes the target side as a possible
> cause. I will have a closer look at the srp_map_finish_fmr() function.
> Always passing 0 as the second argument of srp_map_desc() in
> srp_map_finish_fmr() is probably wrong. Before 2011 that second argument
> was the page offset of the first sg-list entry.

(replying to my own e-mail)

The patch below makes FMR registration work again for buffers that are 
not aligned on a page boundary.

Regarding the discussion in 2011 about FMR 
(http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011 
nobody recalled the root cause of the issue with non-page aligned FMR my 
proposal is to drop the page alignment check and if any issues occur to 
introduce a blacklist for the SRP target devices that have trouble with 
this.

Bart.


[PATCH] IB/srp: Restore FMR offset

Since srp_map_finish_fmr() is always called with base_dma_addr aligned
on a page boundary this patch does not change any functionality. See
also patch "IB/srp: rework mapping engine to use multiple FMR entries"
(commit ID 8f26c9ff9cd0; January 2011).
---
  drivers/infiniband/ulp/srp/ib_srp.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 48201b3..cac444e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1272,6 +1272,8 @@ static void srp_map_desc(struct srp_map_state 
*state, dma_addr_t dma_addr,
  static int srp_map_finish_fmr(struct srp_map_state *state,
  			      struct srp_rdma_ch *ch)
  {
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
  	struct ib_pool_fmr *fmr;
  	u64 io_addr = 0;

@@ -1283,7 +1285,8 @@ static int srp_map_finish_fmr(struct srp_map_state 
*state,
  	*state->next_fmr++ = fmr;
  	state->nmdesc++;

-	srp_map_desc(state, 0, state->dma_len, fmr->fmr->rkey);
+	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,
+		     state->dma_len, fmr->fmr->rkey);

  	return 0;
  }
-- 
2.1.4



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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

On Wed, Aug 05, 2015 at 03:41:46PM -0700, Bart Van Assche wrote:

> Regarding the discussion in 2011 about FMR
> (http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011
> nobody recalled the root cause of the issue with non-page aligned FMR my
> proposal is to drop the page alignment check and if any issues occur to
> introduce a blacklist for the SRP target devices that have trouble with
> this.

Nice find, that sounds like a solid plan.

That looks like it leaves only the srp_map_data flow and the error case
flow in srp_map_sg as remaining problems for the rkey patch? Any
thoughts on adressing those?

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/05/2015 05:10 PM, Jason Gunthorpe wrote:
> On Wed, Aug 05, 2015 at 03:41:46PM -0700, Bart Van Assche wrote:
>> Regarding the discussion in 2011 about FMR
>> (http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011
>> nobody recalled the root cause of the issue with non-page aligned FMR my
>> proposal is to drop the page alignment check and if any issues occur to
>> introduce a blacklist for the SRP target devices that have trouble with
>> this.
>
> Nice find, that sounds like a solid plan.
>
> That looks like it leaves only the srp_map_data flow and the error case
> flow in srp_map_sg as remaining problems for the rkey patch? Any
> thoughts on adressing those?

Hello Jason,

A few experimental and untested patches are available for review at 
https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.

Bart.

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Jason Gunthorpe]

On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:

> A few experimental and untested patches are available for review at
> https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.

I'll leave this is in your hands then..

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/05/2015 09:36 PM, Jason Gunthorpe wrote:
> On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:
>> A few experimental and untested patches are available for review at
>> https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
>
> I'll leave this is in your hands then..

Is there perhaps a tree available somewhere with the latest version of 
your patch series ?

Thanks,

Bart.

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

[PATCH 0/9] IB/srp: Do not create an all physical insecure rkey by default

[Bart Van Assche]

On 08/05/2015 09:36 PM, Jason Gunthorpe wrote:
> On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:
>
>> A few experimental and untested patches are available for review at
>> https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
>
> I'll leave this is in your hands then..

In case anyone would like to see what I am working on, I will post the 
patches I am currently testing in reply to this e-mail. The names of 
these patches are as follows:

0001-IB-srp-Re-enable-FMR-for-non-page-aligned-buffers.patch
0002-IB-srp-Use-multiple-registrations-for-large-memory-r.patch
0003-IB-srp-Add-memory-descriptor-array-pointer-range-che.patch
0004-IB-srp-Remove-the-memory-registration-backtracking-c.patch
0005-IB-srp-Remove-use_mr-argument-from-srp_map_sg_entry.patch
0006-IB-srp-Introduce-srp_device.use_fmr.patch
0007-IB-srp-Register-the-indirect-data-buffer-descriptor.patch
0008-IB-srp-Create-an-insecure-all-physical-rkey-only-if-.patch

These patches are also available at 
https://github.com/bvanassche/linux/tree/srp-initiator-for-next.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] IB/srp: Re-enable FMR for non-page aligned buffers

[Bart Van Assche]

During a discussion in 2011 nobody recalled why FMR was not used for
non-page aligned buffers (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/7149). Re-enable FMR
for such buffers. For the reason why the srp_map_fmr() function needs
to be modified, see also patch "IB/srp: rework mapping engine to use
multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 2968b7b..887e8ca 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1272,6 +1272,8 @@ static void srp_map_desc(struct srp_map_state *state, dma_addr_t dma_addr,
 static int srp_map_finish_fmr(struct srp_map_state *state,
 			      struct srp_rdma_ch *ch)
 {
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
 	struct ib_pool_fmr *fmr;
 	u64 io_addr = 0;
 
@@ -1283,7 +1285,8 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 	*state->next_fmr++ = fmr;
 	state->nmdesc++;
 
-	srp_map_desc(state, 0, state->dma_len, fmr->fmr->rkey);
+	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,
+		     state->dma_len, fmr->fmr->rkey);
 
 	return 0;
 }
@@ -1390,14 +1393,7 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 		return 0;
 	}
 
-	/*
-	 * Since not all RDMA HW drivers support non-zero page offsets for
-	 * FMR, if we start at an offset into a page, don't merge into the
-	 * current FMR mapping. Finish it out, and use the kernel's MR for
-	 * this sg entry.
-	 */
-	if ((!dev->use_fast_reg && dma_addr & ~dev->mr_page_mask) ||
-	    dma_len > dev->mr_max_size) {
+	if (dma_len > dev->mr_max_size) {
 		ret = srp_finish_mapping(state, ch);
 		if (ret)
 			return ret;
-- 
2.1.4

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

[PATCH 2/8] IB/srp: Use multiple registrations for large memory regions

[Bart Van Assche]

Instead of using the global rkey for large memory regions, use
multiple registrations. See also the while (dma_len) loop further
down in srp_map_sg_entry().

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 887e8ca..b6b9a55 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1393,16 +1393,6 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 		return 0;
 	}
 
-	if (dma_len > dev->mr_max_size) {
-		ret = srp_finish_mapping(state, ch);
-		if (ret)
-			return ret;
-
-		srp_map_desc(state, dma_addr, dma_len, target->rkey);
-		srp_map_update_start(state, NULL, 0, 0);
-		return 0;
-	}
-
 	/*
 	 * If this is the first sg that will be mapped via FMR or via FR, save
 	 * our position. We need to know the first unmapped entry, its index,
-- 
2.1.4

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

[PATCH 3/8] IB/srp: Add memory descriptor array pointer range checking

[Bart Van Assche]

Although most paths through which a request is submitted check
block layer parameters like the max_segments limit, these are
not checked when an SG_IO or direct I/O request is submitted.
Hence add a range check for the memory descriptor array pointer.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 16 ++++++++++++----
 drivers/infiniband/ulp/srp/ib_srp.h | 10 ++++++++--
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b6b9a55..f9fa220 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1277,12 +1277,15 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 	struct ib_pool_fmr *fmr;
 	u64 io_addr = 0;
 
+	if (state->fmr.next >= state->fmr.end)
+		return -ENOMEM;
+
 	fmr = ib_fmr_pool_map_phys(ch->fmr_pool, state->pages,
 				   state->npages, io_addr);
 	if (IS_ERR(fmr))
 		return PTR_ERR(fmr);
 
-	*state->next_fmr++ = fmr;
+	*state->fmr.next++ = fmr;
 	state->nmdesc++;
 
 	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,
@@ -1301,6 +1304,9 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 	struct srp_fr_desc *desc;
 	u32 rkey;
 
+	if (state->fr.next >= state->fr.end)
+		return -ENOMEM;
+
 	desc = srp_fr_pool_get(ch->fr_pool);
 	if (!desc)
 		return -ENOMEM;
@@ -1324,7 +1330,7 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 				       IB_ACCESS_REMOTE_WRITE);
 	wr.wr.fast_reg.rkey = desc->mr->lkey;
 
-	*state->next_fr++ = desc;
+	*state->fr.next++ = desc;
 	state->nmdesc++;
 
 	srp_map_desc(state, state->base_dma_addr, state->dma_len,
@@ -1450,10 +1456,12 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	state->desc	= req->indirect_desc;
 	state->pages	= req->map_page;
 	if (dev->use_fast_reg) {
-		state->next_fr = req->fr_list;
+		state->fr.next = req->fr_list;
+		state->fr.end = req->fr_list + target->cmd_sg_cnt;
 		use_mr = !!ch->fr_pool;
 	} else {
-		state->next_fmr = req->fmr_list;
+		state->fmr.next = req->fmr_list;
+		state->fmr.end = req->fmr_list + target->cmd_sg_cnt;
 		use_mr = !!ch->fmr_pool;
 	}
 
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 17ee3f8..2ab73bc 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -282,8 +282,14 @@ struct srp_fr_pool {
  */
 struct srp_map_state {
 	union {
-		struct ib_pool_fmr **next_fmr;
-		struct srp_fr_desc **next_fr;
+		struct {
+			struct ib_pool_fmr **next;
+			struct ib_pool_fmr **end;
+		} fmr;
+		struct {
+			struct srp_fr_desc **next;
+			struct srp_fr_desc **end;
+		} fr;
 	};
 	struct srp_direct_buf  *desc;
 	u64		       *pages;
-- 
2.1.4

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

[PATCH 4/8] IB/srp: Remove the memory registration backtracking code

[Bart Van Assche]

Mapping a discontiguous sg-list requires multiple memory regions
and hence can exhaust the memory region pool. The SRP initiator
already handles this by temporarily reducing the queue depth. This
means that it is safe to remove the memory registration backtracking
code. This patch has been tested with direct I/O sizes up to 256 MB.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 62 ++++++++-----------------------------
 drivers/infiniband/ulp/srp/ib_srp.h |  6 ----
 2 files changed, 13 insertions(+), 55 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index f9fa220..660a4a4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1364,15 +1364,6 @@ static int srp_finish_mapping(struct srp_map_state *state,
 	return ret;
 }
 
-static void srp_map_update_start(struct srp_map_state *state,
-				 struct scatterlist *sg, int sg_index,
-				 dma_addr_t dma_addr)
-{
-	state->unmapped_sg = sg;
-	state->unmapped_index = sg_index;
-	state->unmapped_addr = dma_addr;
-}
-
 static int srp_map_sg_entry(struct srp_map_state *state,
 			    struct srp_rdma_ch *ch,
 			    struct scatterlist *sg, int sg_index,
@@ -1399,23 +1390,12 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 		return 0;
 	}
 
-	/*
-	 * If this is the first sg that will be mapped via FMR or via FR, save
-	 * our position. We need to know the first unmapped entry, its index,
-	 * and the first unmapped address within that entry to be able to
-	 * restart mapping after an error.
-	 */
-	if (!state->unmapped_sg)
-		srp_map_update_start(state, sg, sg_index, dma_addr);
-
 	while (dma_len) {
 		unsigned offset = dma_addr & ~dev->mr_page_mask;
 		if (state->npages == dev->max_pages_per_mr || offset != 0) {
 			ret = srp_finish_mapping(state, ch);
 			if (ret)
 				return ret;
-
-			srp_map_update_start(state, sg, sg_index, dma_addr);
 		}
 
 		len = min_t(unsigned int, dma_len, dev->mr_page_size - offset);
@@ -1434,11 +1414,8 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 	 * boundaries.
 	 */
 	ret = 0;
-	if (len != dev->mr_page_size) {
+	if (len != dev->mr_page_size)
 		ret = srp_finish_mapping(state, ch);
-		if (!ret)
-			srp_map_update_start(state, NULL, 0, 0);
-	}
 	return ret;
 }
 
@@ -1448,9 +1425,8 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
-	struct ib_device *ibdev = dev->dev;
 	struct scatterlist *sg;
-	int i;
+	int i, ret;
 	bool use_mr;
 
 	state->desc	= req->indirect_desc;
@@ -1466,34 +1442,22 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	}
 
 	for_each_sg(scat, sg, count, i) {
-		if (srp_map_sg_entry(state, ch, sg, i, use_mr)) {
-			/*
-			 * Memory registration failed, so backtrack to the
-			 * first unmapped entry and continue on without using
-			 * memory registration.
-			 */
-			dma_addr_t dma_addr;
-			unsigned int dma_len;
-
-backtrack:
-			sg = state->unmapped_sg;
-			i = state->unmapped_index;
-
-			dma_addr = ib_sg_dma_address(ibdev, sg);
-			dma_len = ib_sg_dma_len(ibdev, sg);
-			dma_len -= (state->unmapped_addr - dma_addr);
-			dma_addr = state->unmapped_addr;
-			use_mr = false;
-			srp_map_desc(state, dma_addr, dma_len, target->rkey);
-		}
+		ret = srp_map_sg_entry(state, ch, sg, i, use_mr);
+		if (ret)
+			goto out;
 	}
 
-	if (use_mr && srp_finish_mapping(state, ch))
-		goto backtrack;
+	if (use_mr) {
+		ret = srp_finish_mapping(state, ch);
+		if (ret)
+			goto out;
+	}
 
 	req->nmdesc = state->nmdesc;
+	ret = 0;
 
-	return 0;
+out:
+	return ret;
 }
 
 static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 2ab73bc..1e42418 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -276,9 +276,6 @@ struct srp_fr_pool {
  * @npages:	    Number of page addresses in the pages[] array.
  * @nmdesc:	    Number of FMR or FR memory descriptors used for mapping.
  * @ndesc:	    Number of SRP buffer descriptors that have been filled in.
- * @unmapped_sg:    First element of the sg-list that is mapped via FMR or FR.
- * @unmapped_index: Index of the first element mapped via FMR or FR.
- * @unmapped_addr:  DMA address of the first element mapped via FMR or FR.
  */
 struct srp_map_state {
 	union {
@@ -299,9 +296,6 @@ struct srp_map_state {
 	unsigned int		npages;
 	unsigned int		nmdesc;
 	unsigned int		ndesc;
-	struct scatterlist     *unmapped_sg;
-	int			unmapped_index;
-	dma_addr_t		unmapped_addr;
 };
 
 #endif /* IB_SRP_H */
-- 
2.1.4

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

[PATCH 5/8] IB/srp: Remove use_mr argument from srp_map_sg_entry()

[Bart Van Assche]

Move the srp_map_desc() call from inside srp_map_sg_entry() to
srp_map_sg() such that the use_mr argument can be removed from
srp_map_sg_entry().

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 660a4a4..b8fc35b 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1260,6 +1260,8 @@ static void srp_map_desc(struct srp_map_state *state, dma_addr_t dma_addr,
 {
 	struct srp_direct_buf *desc = state->desc;
 
+	WARN_ON_ONCE(!dma_len);
+
 	desc->va = cpu_to_be64(dma_addr);
 	desc->key = cpu_to_be32(rkey);
 	desc->len = cpu_to_be32(dma_len);
@@ -1366,29 +1368,17 @@ static int srp_finish_mapping(struct srp_map_state *state,
 
 static int srp_map_sg_entry(struct srp_map_state *state,
 			    struct srp_rdma_ch *ch,
-			    struct scatterlist *sg, int sg_index,
-			    bool use_mr)
+			    struct scatterlist *sg, int sg_index)
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
 	struct ib_device *ibdev = dev->dev;
 	dma_addr_t dma_addr = ib_sg_dma_address(ibdev, sg);
 	unsigned int dma_len = ib_sg_dma_len(ibdev, sg);
-	unsigned int len;
+	unsigned int len = 0;
 	int ret;
 
-	if (!dma_len)
-		return 0;
-
-	if (!use_mr) {
-		/*
-		 * Once we're in direct map mode for a request, we don't
-		 * go back to FMR or FR mode, so no need to update anything
-		 * other than the descriptor.
-		 */
-		srp_map_desc(state, dma_addr, dma_len, target->rkey);
-		return 0;
-	}
+	WARN_ON_ONCE(!dma_len);
 
 	while (dma_len) {
 		unsigned offset = dma_addr & ~dev->mr_page_mask;
@@ -1441,16 +1431,20 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 		use_mr = !!ch->fmr_pool;
 	}
 
-	for_each_sg(scat, sg, count, i) {
-		ret = srp_map_sg_entry(state, ch, sg, i, use_mr);
-		if (ret)
-			goto out;
-	}
-
 	if (use_mr) {
+		for_each_sg(scat, sg, count, i) {
+			ret = srp_map_sg_entry(state, ch, sg, i);
+			if (ret)
+				goto out;
+		}
 		ret = srp_finish_mapping(state, ch);
 		if (ret)
 			goto out;
+	} else {
+		for_each_sg(scat, sg, count, i) {
+			srp_map_desc(state, ib_sg_dma_address(dev->dev, sg),
+				     ib_sg_dma_len(dev->dev, sg), target->rkey);
+		}
 	}
 
 	req->nmdesc = state->nmdesc;
-- 
2.1.4

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

[PATCH 6/8] IB/srp: Introduce srp_device.use_fmr

[Bart Van Assche]

Introduce the variable srp_device.use_fmr. Leave out the dev->has_fr /
dev->has_fmr and ch->fr_pool / ch->fmr_pool checks since these are
redundant. This patch does not change any functionality but makes the
source code easier to read.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++-----------
 drivers/infiniband/ulp/srp/ib_srp.h |  1 +
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b8fc35b..1029fd2 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -546,7 +546,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	if (ret)
 		goto err_qp;
 
-	if (dev->use_fast_reg && dev->has_fr) {
+	if (dev->use_fast_reg) {
 		fr_pool = srp_alloc_fr_pool(target);
 		if (IS_ERR(fr_pool)) {
 			ret = PTR_ERR(fr_pool);
@@ -557,7 +557,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 		if (ch->fr_pool)
 			srp_destroy_fr_pool(ch->fr_pool);
 		ch->fr_pool = fr_pool;
-	} else if (!dev->use_fast_reg && dev->has_fmr) {
+	} else if (dev->use_fmr) {
 		fmr_pool = srp_alloc_fmr_pool(target);
 		if (IS_ERR(fmr_pool)) {
 			ret = PTR_ERR(fmr_pool);
@@ -623,7 +623,7 @@ static void srp_free_ch_ib(struct srp_target_port *target,
 	if (dev->use_fast_reg) {
 		if (ch->fr_pool)
 			srp_destroy_fr_pool(ch->fr_pool);
-	} else {
+	} else if (dev->use_fmr) {
 		if (ch->fmr_pool)
 			ib_destroy_fmr_pool(ch->fmr_pool);
 	}
@@ -1085,7 +1085,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
 		if (req->nmdesc)
 			srp_fr_pool_put(ch->fr_pool, req->fr_list,
 					req->nmdesc);
-	} else {
+	} else if (dev->use_fmr) {
 		struct ib_pool_fmr **pfmr;
 
 		for (i = req->nmdesc, pfmr = req->fmr_list; i > 0; i--, pfmr++)
@@ -1345,8 +1345,11 @@ static int srp_finish_mapping(struct srp_map_state *state,
 			      struct srp_rdma_ch *ch)
 {
 	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
 	int ret = 0;
 
+	WARN_ON_ONCE(!dev->use_fast_reg && !dev->use_fmr);
+
 	if (state->npages == 0)
 		return 0;
 
@@ -1354,8 +1357,7 @@ static int srp_finish_mapping(struct srp_map_state *state,
 		srp_map_desc(state, state->base_dma_addr, state->dma_len,
 			     target->rkey);
 	else
-		ret = target->srp_host->srp_dev->use_fast_reg ?
-			srp_map_finish_fr(state, ch) :
+		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
 			srp_map_finish_fmr(state, ch);
 
 	if (ret == 0) {
@@ -1417,21 +1419,18 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	struct srp_device *dev = target->srp_host->srp_dev;
 	struct scatterlist *sg;
 	int i, ret;
-	bool use_mr;
 
 	state->desc	= req->indirect_desc;
 	state->pages	= req->map_page;
 	if (dev->use_fast_reg) {
 		state->fr.next = req->fr_list;
 		state->fr.end = req->fr_list + target->cmd_sg_cnt;
-		use_mr = !!ch->fr_pool;
-	} else {
+	} else if (dev->use_fmr) {
 		state->fmr.next = req->fmr_list;
 		state->fmr.end = req->fmr_list + target->cmd_sg_cnt;
-		use_mr = !!ch->fmr_pool;
 	}
 
-	if (use_mr) {
+	if (dev->use_fast_reg || dev->use_fmr) {
 		for_each_sg(scat, sg, count, i) {
 			ret = srp_map_sg_entry(state, ch, sg, i);
 			if (ret)
@@ -3355,6 +3354,7 @@ static void srp_add_one(struct ib_device *device)
 
 	srp_dev->use_fast_reg = (srp_dev->has_fr &&
 				 (!srp_dev->has_fmr || prefer_fr));
+	srp_dev->use_fmr = !srp_dev->use_fast_reg && srp_dev->has_fmr;
 
 	/*
 	 * Use the smallest page size supported by the HCA, down to a
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 1e42418..60a33c1 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -102,6 +102,7 @@ struct srp_device {
 	int			max_pages_per_mr;
 	bool			has_fmr;
 	bool			has_fr;
+	bool			use_fmr;
 	bool			use_fast_reg;
 };
 
-- 
2.1.4

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

[PATCH 7/8] IB/srp: Register the indirect data buffer descriptor

[Bart Van Assche]

Instead of always using the global rkey for the indirect data
buffer descriptor, register that descriptor with the HCA if
the kernel module parameter register_always has been set to Y.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 57 +++++++++++++++++++++++++++++++++++--
 drivers/infiniband/ulp/srp/ib_srp.h |  4 +++
 2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 1029fd2..2a16ab4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1453,18 +1453,58 @@ out:
 	return ret;
 }
 
+/*
+ * Register the indirect data buffer descriptor with the HCA.
+ *
+ * Note: since the indirect data buffer descriptor has been allocated with
+ * kmalloc() it is guaranteed that this buffer is a physically contiguous
+ * memory buffer.
+ */
+static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
+		       void **next_mr, void **end_mr, u32 idb_len,
+		       __be32 *idb_rkey)
+{
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
+	struct srp_map_state state;
+	struct srp_direct_buf idb_desc;
+	u64 idb_pages[1];
+	int ret;
+
+	memset(&state, 0, sizeof(state));
+	memset(&idb_desc, 0, sizeof(idb_desc));
+	state.gen.next = next_mr;
+	state.gen.end = end_mr;
+	state.desc = &idb_desc;
+	state.pages = idb_pages;
+	state.pages[0] = (req->indirect_dma_addr &
+			  dev->mr_page_mask);
+	state.npages = 1;
+	state.base_dma_addr = req->indirect_dma_addr;
+	state.dma_len = idb_len;
+	ret = srp_finish_mapping(&state, ch);
+	if (ret < 0)
+		goto out;
+
+	*idb_rkey = idb_desc.key;
+
+out:
+	return ret;
+}
+
 static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 			struct srp_request *req)
 {
 	struct srp_target_port *target = ch->target;
 	struct scatterlist *scat;
 	struct srp_cmd *cmd = req->cmd->buf;
-	int len, nents, count;
+	int len, nents, count, ret;
 	struct srp_device *dev;
 	struct ib_device *ibdev;
 	struct srp_map_state state;
 	struct srp_indirect_buf *indirect_hdr;
-	u32 table_len;
+	u32 idb_len, table_len;
+	__be32 idb_rkey;
 	u8 fmt;
 
 	if (!scsi_sglist(scmnd) || scmnd->sc_data_direction == DMA_NONE)
@@ -1546,6 +1586,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 
 	count = min(state.ndesc, target->cmd_sg_cnt);
 	table_len = state.ndesc * sizeof (struct srp_direct_buf);
+	idb_len = sizeof(struct srp_indirect_buf) + table_len;
 
 	fmt = SRP_DATA_DESC_INDIRECT;
 	len = sizeof(struct srp_cmd) + sizeof (struct srp_indirect_buf);
@@ -1554,8 +1595,18 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	memcpy(indirect_hdr->desc_list, req->indirect_desc,
 	       count * sizeof (struct srp_direct_buf));
 
+	if (register_always && (dev->use_fast_reg || dev->use_fmr)) {
+		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
+				  idb_len, &idb_rkey);
+		if (ret < 0)
+			return ret;
+		req->nmdesc++;
+	} else {
+		idb_rkey = target->rkey;
+	}
+
 	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);
-	indirect_hdr->table_desc.key = cpu_to_be32(target->rkey);
+	indirect_hdr->table_desc.key = idb_rkey;
 	indirect_hdr->table_desc.len = cpu_to_be32(table_len);
 	indirect_hdr->len = cpu_to_be32(state.total_len);
 
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 60a33c1..255b0e5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -288,6 +288,10 @@ struct srp_map_state {
 			struct srp_fr_desc **next;
 			struct srp_fr_desc **end;
 		} fr;
+		struct {
+			void		   **next;
+			void		   **end;
+		} gen;
 	};
 	struct srp_direct_buf  *desc;
 	u64		       *pages;
-- 
2.1.4

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

[PATCH 8/8] IB/srp: Create an insecure all physical rkey only if needed

[Bart Van Assche]

The SRP initiator only needs this if the insecure register_always=N
performance optimization is enabled, or if FRWR/FMR is not supported
in the driver.

Do not create an all physical MR unless it is needed to support
either of those modes. Default register_always to true so the out of
the box configuration does not create an insecure all physical MR.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
[bvanassche: reworked and rebased this patch]
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 40 +++++++++++++++++++++----------------
 drivers/infiniband/ulp/srp/ib_srp.h |  4 ++--
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 2a16ab4..192f737 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -68,8 +68,8 @@ static unsigned int srp_sg_tablesize;
 static unsigned int cmd_sg_entries;
 static unsigned int indirect_sg_entries;
 static bool allow_ext_sg;
-static bool prefer_fr;
-static bool register_always;
+static bool prefer_fr = true;
+static bool register_always = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);
@@ -1353,9 +1353,9 @@ static int srp_finish_mapping(struct srp_map_state *state,
 	if (state->npages == 0)
 		return 0;
 
-	if (state->npages == 1 && !register_always)
+	if (state->npages == 1 && target->global_mr)
 		srp_map_desc(state, state->base_dma_addr, state->dma_len,
-			     target->rkey);
+			     target->global_mr->rkey);
 	else
 		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
 			srp_map_finish_fmr(state, ch);
@@ -1442,7 +1442,8 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	} else {
 		for_each_sg(scat, sg, count, i) {
 			srp_map_desc(state, ib_sg_dma_address(dev->dev, sg),
-				     ib_sg_dma_len(dev->dev, sg), target->rkey);
+				     ib_sg_dma_len(dev->dev, sg),
+				     target->global_mr->rkey);
 		}
 	}
 
@@ -1531,7 +1532,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	fmt = SRP_DATA_DESC_DIRECT;
 	len = sizeof (struct srp_cmd) +	sizeof (struct srp_direct_buf);
 
-	if (count == 1 && !register_always) {
+	if (count == 1 && target->global_mr) {
 		/*
 		 * The midlayer only generated a single gather/scatter
 		 * entry, or DMA mapping coalesced everything to a
@@ -1541,7 +1542,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 		struct srp_direct_buf *buf = (void *) cmd->add_data;
 
 		buf->va  = cpu_to_be64(ib_sg_dma_address(ibdev, scat));
-		buf->key = cpu_to_be32(target->rkey);
+		buf->key = cpu_to_be32(target->global_mr->rkey);
 		buf->len = cpu_to_be32(ib_sg_dma_len(ibdev, scat));
 
 		req->nmdesc = 0;
@@ -1595,14 +1596,14 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	memcpy(indirect_hdr->desc_list, req->indirect_desc,
 	       count * sizeof (struct srp_direct_buf));
 
-	if (register_always && (dev->use_fast_reg || dev->use_fmr)) {
+	if (!target->global_mr) {
 		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
 				  idb_len, &idb_rkey);
 		if (ret < 0)
 			return ret;
 		req->nmdesc++;
 	} else {
-		idb_rkey = target->rkey;
+		idb_rkey = target->global_mr->rkey;
 	}
 
 	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);
@@ -3150,7 +3151,7 @@ static ssize_t srp_create_target(struct device *dev,
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
 	target->lkey		= host->srp_dev->pd->local_dma_lkey;
-	target->rkey		= host->srp_dev->mr->rkey;
+	target->global_mr	= host->srp_dev->global_mr;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
 	target->allow_ext_sg	= allow_ext_sg;
@@ -3438,12 +3439,16 @@ static void srp_add_one(struct ib_device *device)
 	if (IS_ERR(srp_dev->pd))
 		goto free_dev;
 
-	srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
-				    IB_ACCESS_LOCAL_WRITE |
-				    IB_ACCESS_REMOTE_READ |
-				    IB_ACCESS_REMOTE_WRITE);
-	if (IS_ERR(srp_dev->mr))
-		goto err_pd;
+	if (!register_always || (!srp_dev->has_fmr && !srp_dev->has_fr)) {
+		srp_dev->global_mr = ib_get_dma_mr(srp_dev->pd,
+						   IB_ACCESS_LOCAL_WRITE |
+						   IB_ACCESS_REMOTE_READ |
+						   IB_ACCESS_REMOTE_WRITE);
+		if (IS_ERR(srp_dev->global_mr))
+			goto err_pd;
+	} else {
+		srp_dev->global_mr = NULL;
+	}
 
 	for (p = rdma_start_port(device); p <= rdma_end_port(device); ++p) {
 		host = srp_add_port(srp_dev, p);
@@ -3500,7 +3505,8 @@ static void srp_remove_one(struct ib_device *device)
 		kfree(host);
 	}
 
-	ib_dereg_mr(srp_dev->mr);
+	if (srp_dev->global_mr)
+		ib_dereg_mr(srp_dev->global_mr);
 	ib_dealloc_pd(srp_dev->pd);
 
 	kfree(srp_dev);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 255b0e5..3608f2e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -95,7 +95,7 @@ struct srp_device {
 	struct list_head	dev_list;
 	struct ib_device       *dev;
 	struct ib_pd	       *pd;
-	struct ib_mr	       *mr;
+	struct ib_mr	       *global_mr;
 	u64			mr_page_mask;
 	int			mr_page_size;
 	int			mr_max_size;
@@ -183,10 +183,10 @@ struct srp_target_port {
 	spinlock_t		lock;
 
 	/* read only in the hot path */
+	struct ib_mr		*global_mr;
 	struct srp_rdma_ch	*ch;
 	u32			ch_count;
 	u32			lkey;
-	u32			rkey;
 	enum srp_target_state	state;
 	unsigned int		max_iu_len;
 	unsigned int		cmd_sg_cnt;
-- 
2.1.4

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

Re: [PATCH 0/9] IB/srp: Do not create an all physical insecure rkey by default

[Jason Gunthorpe]

On Mon, Aug 10, 2015 at 05:05:53PM -0700, Bart Van Assche wrote:
> On 08/05/2015 09:36 PM, Jason Gunthorpe wrote:
> >On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:
> >
> >>A few experimental and untested patches are available for review at
> >>https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
> >
> >I'll leave this is in your hands then..
> 
> In case anyone would like to see what I am working on, I will post the
> patches I am currently testing in reply to this e-mail. The names of these
> patches are as follows:

Nice, looks like it covers all the cases..

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/01/2015 01:05 PM, Doug Ledford wrote:
> On 07/31/2015 07:14 PM, Jason Gunthorpe wrote:
>> On Fri, Jul 31, 2015 at 04:04:35PM -0700, Bart Van Assche wrote:
>>
>>> With only patches 1/12 and 8/12 applied my test passes.
>>
>> Thanks Bart!
>>
>> If you like, I can try and work on #9 (no promises :(), but I think
>> I'll need some way to test it here.
>>
>> Do you by chance have a straightforward recipe to setup SRP and SRPT
>> on two Linux's for this simple purpose?
>>
>> Doug, just drop #9 'IB/srp: Do not create an all physical insecure
>> rkey by default' please, thanks.
>
> I've pulled in the series for 4.3.  I've included patch #9, but that's
> on the understanding that if it can't be fixed before the initial pull
> request, I'll drop it at that time.  I have the resources to test this,
> so I can work on it, and that factors into my path forward here.

Hi Doug,

Do you feel comfortable enough to queue the nine patches that replace 
Jason's patch #9 for kernel v4.3 (see also 
http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or 
would you rather prefer to take out patch #9 and queue these nine 
patches for kernel v4.4 ? Please advise.

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 12/12] rds/ib: Remove ib_get_dma_mr calls

[santosh shilimkar]

On 7/30/2015 4:22 PM, Jason Gunthorpe wrote:
> The pd now has a local_dma_lkey member which completely replaces
> ib_get_dma_mr, use it instead.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>   net/rds/ib.c      | 8 --------
>   net/rds/ib.h      | 2 --
>   net/rds/ib_cm.c   | 4 +---
>   net/rds/ib_recv.c | 6 +++---
>   net/rds/ib_send.c | 8 ++++----
>   5 files changed, 8 insertions(+), 20 deletions(-)
>
I wanted to try this series earlier but couldn't do it because of
broken RDS RDMA. Now I have that fixed with bunch of patches soon
to be posted, tried the series. It works as expected.

The rds change looks also straight forward since ib_get_dma_mr()
is being used for local write.

So feel free to add below tag if you need one.

Tested-Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On 08/11/2015 04:50 PM, Bart Van Assche wrote:
> On 08/01/2015 01:05 PM, Doug Ledford wrote:
>> On 07/31/2015 07:14 PM, Jason Gunthorpe wrote:
>>> On Fri, Jul 31, 2015 at 04:04:35PM -0700, Bart Van Assche wrote:
>>>
>>>> With only patches 1/12 and 8/12 applied my test passes.
>>>
>>> Thanks Bart!
>>>
>>> If you like, I can try and work on #9 (no promises :(), but I think
>>> I'll need some way to test it here.
>>>
>>> Do you by chance have a straightforward recipe to setup SRP and SRPT
>>> on two Linux's for this simple purpose?
>>>
>>> Doug, just drop #9 'IB/srp: Do not create an all physical insecure
>>> rkey by default' please, thanks.
>>
>> I've pulled in the series for 4.3.  I've included patch #9, but that's
>> on the understanding that if it can't be fixed before the initial pull
>> request, I'll drop it at that time.  I have the resources to test this,
>> so I can work on it, and that factors into my path forward here.
> 
> Hi Doug,
> 
> Do you feel comfortable enough to queue the nine patches that replace
> Jason's patch #9 for kernel v4.3 (see also
> http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or
> would you rather prefer to take out patch #9 and queue these nine
> patches for kernel v4.4 ? Please advise.

I was planning on queueing up the 9 replacement patches.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Bart Van Assche]

On 08/14/2015 06:36 AM, Doug Ledford wrote:
> On 08/11/2015 04:50 PM, Bart Van Assche wrote:
>> On 08/01/2015 01:05 PM, Doug Ledford wrote:
>>> On 07/31/2015 07:14 PM, Jason Gunthorpe wrote:
>>>> On Fri, Jul 31, 2015 at 04:04:35PM -0700, Bart Van Assche wrote:
>>>>
>>>>> With only patches 1/12 and 8/12 applied my test passes.
>>>>
>>>> Thanks Bart!
>>>>
>>>> If you like, I can try and work on #9 (no promises :(), but I think
>>>> I'll need some way to test it here.
>>>>
>>>> Do you by chance have a straightforward recipe to setup SRP and SRPT
>>>> on two Linux's for this simple purpose?
>>>>
>>>> Doug, just drop #9 'IB/srp: Do not create an all physical insecure
>>>> rkey by default' please, thanks.
>>>
>>> I've pulled in the series for 4.3.  I've included patch #9, but that's
>>> on the understanding that if it can't be fixed before the initial pull
>>> request, I'll drop it at that time.  I have the resources to test this,
>>> so I can work on it, and that factors into my path forward here.
>>
>> Hi Doug,
>>
>> Do you feel comfortable enough to queue the nine patches that replace
>> Jason's patch #9 for kernel v4.3 (see also
>> http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or
>> would you rather prefer to take out patch #9 and queue these nine
>> patches for kernel v4.4 ? Please advise.
>
> I was planning on queueing up the 9 replacement patches.

Hello Doug,

I hope that someone who has access to a DDN system has the time to test 
this patch series against a DDN array. The memory key that is associated 
with the indirect data buffer descriptor is never used by the upstream 
SRP target driver (ib_srpt) but is used by DDN arrays to transfer the 
indirect data buffer descriptor. DDN arrays support indirect data 
buffers that are larger than what fits in the SRP_CMD information unit 
but the upstream SRP target driver not.

Bart.

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

Re: [PATCH v2 00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

On 08/14/2015 12:20 PM, Bart Van Assche wrote:
> On 08/14/2015 06:36 AM, Doug Ledford wrote:
>> On 08/11/2015 04:50 PM, Bart Van Assche wrote:
>>> On 08/01/2015 01:05 PM, Doug Ledford wrote:
>>>> On 07/31/2015 07:14 PM, Jason Gunthorpe wrote:
>>>>> On Fri, Jul 31, 2015 at 04:04:35PM -0700, Bart Van Assche wrote:
>>>>>
>>>>>> With only patches 1/12 and 8/12 applied my test passes.
>>>>>
>>>>> Thanks Bart!
>>>>>
>>>>> If you like, I can try and work on #9 (no promises :(), but I think
>>>>> I'll need some way to test it here.
>>>>>
>>>>> Do you by chance have a straightforward recipe to setup SRP and SRPT
>>>>> on two Linux's for this simple purpose?
>>>>>
>>>>> Doug, just drop #9 'IB/srp: Do not create an all physical insecure
>>>>> rkey by default' please, thanks.
>>>>
>>>> I've pulled in the series for 4.3.  I've included patch #9, but that's
>>>> on the understanding that if it can't be fixed before the initial pull
>>>> request, I'll drop it at that time.  I have the resources to test this,
>>>> so I can work on it, and that factors into my path forward here.
>>>
>>> Hi Doug,
>>>
>>> Do you feel comfortable enough to queue the nine patches that replace
>>> Jason's patch #9 for kernel v4.3 (see also
>>> http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or
>>> would you rather prefer to take out patch #9 and queue these nine
>>> patches for kernel v4.4 ? Please advise.
>>
>> I was planning on queueing up the 9 replacement patches.
> 
> Hello Doug,
> 
> I hope that someone who has access to a DDN system has the time to test
> this patch series against a DDN array. The memory key that is associated
> with the indirect data buffer descriptor is never used by the upstream
> SRP target driver (ib_srpt) but is used by DDN arrays to transfer the
> indirect data buffer descriptor. DDN arrays support indirect data
> buffers that are larger than what fits in the SRP_CMD information unit
> but the upstream SRP target driver not.

It will be ready to go very early in the merge window, so they would
have plenty of time to test.  Even pre-merge window if they wanted to.

Ben, can you find out if there would be an ability to test an upstream
kernel against a DDN array where you are?

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH 1/8] IB/srp: Re-enable FMR for non-page aligned buffers

[Sagi Grimberg]

On 8/11/2015 3:06 AM, Bart Van Assche wrote:
> During a discussion in 2011 nobody recalled why FMR was not used for
> non-page aligned buffers (see also
> http://thread.gmane.org/gmane.linux.drivers.rdma/7149). Re-enable FMR
> for such buffers. For the reason why the srp_map_fmr() function needs
> to be modified, see also patch "IB/srp: rework mapping engine to use
> multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).
>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>

Hi Bart,

Just wanted to confirm that this was tested with FMR and non-aligned IO.
(the change log doesn't say it).
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] IB/srp: Add memory descriptor array pointer range checking

[Sagi Grimberg]

On 8/11/2015 3:07 AM, Bart Van Assche wrote:
> Although most paths through which a request is submitted check
> block layer parameters like the max_segments limit, these are
> not checked when an SG_IO or direct I/O request is submitted.

Why not check that there instead?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] IB/srp: Introduce srp_device.use_fmr

[Sagi Grimberg]

On 8/11/2015 3:08 AM, Bart Van Assche wrote:
> Introduce the variable srp_device.use_fmr. Leave out the dev->has_fr /
> dev->has_fmr and ch->fr_pool / ch->fmr_pool checks since these are
> redundant. This patch does not change any functionality but makes the
> source code easier to read.
>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++-----------
>   drivers/infiniband/ulp/srp/ib_srp.h |  1 +
>   2 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index b8fc35b..1029fd2 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -546,7 +546,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>   	if (ret)
>   		goto err_qp;
>
> -	if (dev->use_fast_reg && dev->has_fr) {
> +	if (dev->use_fast_reg) {
>   		fr_pool = srp_alloc_fr_pool(target);
>   		if (IS_ERR(fr_pool)) {
>   			ret = PTR_ERR(fr_pool);
> @@ -557,7 +557,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>   		if (ch->fr_pool)
>   			srp_destroy_fr_pool(ch->fr_pool);
>   		ch->fr_pool = fr_pool;
> -	} else if (!dev->use_fast_reg && dev->has_fmr) {
> +	} else if (dev->use_fmr) {
>   		fmr_pool = srp_alloc_fmr_pool(target);
>   		if (IS_ERR(fmr_pool)) {
>   			ret = PTR_ERR(fmr_pool);
> @@ -623,7 +623,7 @@ static void srp_free_ch_ib(struct srp_target_port *target,
>   	if (dev->use_fast_reg) {
>   		if (ch->fr_pool)
>   			srp_destroy_fr_pool(ch->fr_pool);
> -	} else {
> +	} else if (dev->use_fmr) {
>   		if (ch->fmr_pool)
>   			ib_destroy_fmr_pool(ch->fmr_pool);
>   	}
> @@ -1085,7 +1085,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
>   		if (req->nmdesc)
>   			srp_fr_pool_put(ch->fr_pool, req->fr_list,
>   					req->nmdesc);
> -	} else {
> +	} else if (dev->use_fmr) {
>   		struct ib_pool_fmr **pfmr;
>
>   		for (i = req->nmdesc, pfmr = req->fmr_list; i > 0; i--, pfmr++)
> @@ -1345,8 +1345,11 @@ static int srp_finish_mapping(struct srp_map_state *state,
>   			      struct srp_rdma_ch *ch)
>   {
>   	struct srp_target_port *target = ch->target;
> +	struct srp_device *dev = target->srp_host->srp_dev;
>   	int ret = 0;
>
> +	WARN_ON_ONCE(!dev->use_fast_reg && !dev->use_fmr);
> +
>   	if (state->npages == 0)
>   		return 0;
>
> @@ -1354,8 +1357,7 @@ static int srp_finish_mapping(struct srp_map_state *state,
>   		srp_map_desc(state, state->base_dma_addr, state->dma_len,
>   			     target->rkey);
>   	else
> -		ret = target->srp_host->srp_dev->use_fast_reg ?
> -			srp_map_finish_fr(state, ch) :
> +		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
>   			srp_map_finish_fmr(state, ch);

Is this related to the patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] IB/srp: Register the indirect data buffer descriptor

[Sagi Grimberg]

On 8/11/2015 3:09 AM, Bart Van Assche wrote:
> Instead of always using the global rkey for the indirect data
> buffer descriptor, register that descriptor with the HCA if
> the kernel module parameter register_always has been set to Y.
>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 57 +++++++++++++++++++++++++++++++++++--
>   drivers/infiniband/ulp/srp/ib_srp.h |  4 +++
>   2 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 1029fd2..2a16ab4 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1453,18 +1453,58 @@ out:
>   	return ret;
>   }
>
> +/*
> + * Register the indirect data buffer descriptor with the HCA.
> + *
> + * Note: since the indirect data buffer descriptor has been allocated with
> + * kmalloc() it is guaranteed that this buffer is a physically contiguous
> + * memory buffer.
> + */

kdoc style?

> +static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
> +		       void **next_mr, void **end_mr, u32 idb_len,
> +		       __be32 *idb_rkey)
> +{
> +	struct srp_target_port *target = ch->target;
> +	struct srp_device *dev = target->srp_host->srp_dev;
> +	struct srp_map_state state;
> +	struct srp_direct_buf idb_desc;
> +	u64 idb_pages[1];
> +	int ret;
> +
> +	memset(&state, 0, sizeof(state));
> +	memset(&idb_desc, 0, sizeof(idb_desc));
> +	state.gen.next = next_mr;
> +	state.gen.end = end_mr;
> +	state.desc = &idb_desc;
> +	state.pages = idb_pages;
> +	state.pages[0] = (req->indirect_dma_addr &
> +			  dev->mr_page_mask);
> +	state.npages = 1;
> +	state.base_dma_addr = req->indirect_dma_addr;
> +	state.dma_len = idb_len;
> +	ret = srp_finish_mapping(&state, ch);
> +	if (ret < 0)
> +		goto out;
> +
> +	*idb_rkey = idb_desc.key;
> +
> +out:
> +	return ret;
> +}
> +
>   static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
>   			struct srp_request *req)
>   {
>   	struct srp_target_port *target = ch->target;
>   	struct scatterlist *scat;
>   	struct srp_cmd *cmd = req->cmd->buf;
> -	int len, nents, count;
> +	int len, nents, count, ret;
>   	struct srp_device *dev;
>   	struct ib_device *ibdev;
>   	struct srp_map_state state;
>   	struct srp_indirect_buf *indirect_hdr;
> -	u32 table_len;
> +	u32 idb_len, table_len;
> +	__be32 idb_rkey;
>   	u8 fmt;
>
>   	if (!scsi_sglist(scmnd) || scmnd->sc_data_direction == DMA_NONE)
> @@ -1546,6 +1586,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
>
>   	count = min(state.ndesc, target->cmd_sg_cnt);
>   	table_len = state.ndesc * sizeof (struct srp_direct_buf);
> +	idb_len = sizeof(struct srp_indirect_buf) + table_len;
>
>   	fmt = SRP_DATA_DESC_INDIRECT;
>   	len = sizeof(struct srp_cmd) + sizeof (struct srp_indirect_buf);
> @@ -1554,8 +1595,18 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
>   	memcpy(indirect_hdr->desc_list, req->indirect_desc,
>   	       count * sizeof (struct srp_direct_buf));
>
> +	if (register_always && (dev->use_fast_reg || dev->use_fmr)) {
> +		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
> +				  idb_len, &idb_rkey);
> +		if (ret < 0)
> +			return ret;
> +		req->nmdesc++;
> +	} else {
> +		idb_rkey = target->rkey;
> +	}
> +
>   	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);
> -	indirect_hdr->table_desc.key = cpu_to_be32(target->rkey);
> +	indirect_hdr->table_desc.key = idb_rkey;
>   	indirect_hdr->table_desc.len = cpu_to_be32(table_len);
>   	indirect_hdr->len = cpu_to_be32(state.total_len);
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
> index 60a33c1..255b0e5 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.h
> +++ b/drivers/infiniband/ulp/srp/ib_srp.h
> @@ -288,6 +288,10 @@ struct srp_map_state {
>   			struct srp_fr_desc **next;
>   			struct srp_fr_desc **end;
>   		} fr;
> +		struct {
> +			void		   **next;
> +			void		   **end;
> +		} gen;

What does 'gen' stands for?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] IB/srp: Re-enable FMR for non-page aligned buffers

[Bart Van Assche]

On 08/16/15 03:53, Sagi Grimberg wrote:
> On 8/11/2015 3:06 AM, Bart Van Assche wrote:
>> During a discussion in 2011 nobody recalled why FMR was not used for
>> non-page aligned buffers (see also
>> http://thread.gmane.org/gmane.linux.drivers.rdma/7149). Re-enable FMR
>> for such buffers. For the reason why the srp_map_fmr() function needs
>> to be modified, see also patch "IB/srp: rework mapping engine to use
>> multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>
> Hi Bart,
>
> Just wanted to confirm that this was tested with FMR and non-aligned IO.
> (the change log doesn't say it).

Hello Sagi,

This patch series has been tested with FMR and I/O buffers that were not 
aligned to a page boundary by setting the ib_srp kernel module parameter 
prefer_fr to 0.

Bart.


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

Re: [PATCH 3/8] IB/srp: Add memory descriptor array pointer range checking

[Bart Van Assche]

On 08/16/15 03:57, Sagi Grimberg wrote:
> On 8/11/2015 3:07 AM, Bart Van Assche wrote:
>> Although most paths through which a request is submitted check
>> block layer parameters like the max_segments limit, these are
>> not checked when an SG_IO or direct I/O request is submitted.
>
> Why not check that there instead?

The scope of this patch series is to avoid that the SRP initiator needs 
to create a global memory key. Changing the behavior of SG_IO and/or 
direct I/O falls outside the scope of this patch series.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] IB/srp: Introduce srp_device.use_fmr

[Bart Van Assche]

On 08/16/15 04:03, Sagi Grimberg wrote:
> On 8/11/2015 3:08 AM, Bart Van Assche wrote:
>> @@ -1354,8 +1357,7 @@ static int srp_finish_mapping(struct srp_map_state *state,
>>    		srp_map_desc(state, state->base_dma_addr, state->dma_len,
>>    			     target->rkey);
>>    	else
>> -		ret = target->srp_host->srp_dev->use_fast_reg ?
>> -			srp_map_finish_fr(state, ch) :
>> +		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
>>    			srp_map_finish_fmr(state, ch);
>
> Is this related to the patch?

This change could have been split out as a separate patch. If I have to 
repost this patch series I will convert this change into a separate patch.

Bart.


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

Re: [PATCH 7/8] IB/srp: Register the indirect data buffer descriptor

[Bart Van Assche]

On 08/16/15 04:09, Sagi Grimberg wrote:
> On 8/11/2015 3:09 AM, Bart Van Assche wrote:
>> +/*
>> + * Register the indirect data buffer descriptor with the HCA.
>> + *
>> + * Note: since the indirect data buffer descriptor has been allocated with
>> + * kmalloc() it is guaranteed that this buffer is a physically contiguous
>> + * memory buffer.
>> + */
>
> kdoc style?

If I have to repost this patch series I will convert this comment into 
kernel-doc style.

>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
>> index 60a33c1..255b0e5 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.h
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.h
>> @@ -288,6 +288,10 @@ struct srp_map_state {
>>    			struct srp_fr_desc **next;
>>    			struct srp_fr_desc **end;
>>    		} fr;
>> +		struct {
>> +			void		   **next;
>> +			void		   **end;
>> +		} gen;
>
> What does 'gen' stands for?

In this context "gen" stands for "generic". This patch introduces some 
code that needs to access the "next" and "end" pointers and that is used 
both for FMR and FR.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html