[PATCH for-next v3 0/6] RDMA/rxe: Replace mr page map with an xarray

[PATCH for-next v3 0/6] RDMA/rxe: Replace mr page map with an xarray

[Bob Pearson]

This patch series replaces the page map carried in each memory region
with a struct xarray. It is based on a sketch developed by Jason
Gunthorpe. The first five patches are preparation that tries to
cleanly isolate all the mr specific code into rxe_mr.c. The sixth
patch is the actual change.

v3:
  Fixed an error reported by kernel test robot
v2:
  Rebased to 6.2.0-rc1+
  Minor cleanups
  Fixed error reported by Jason in 4/6 missing if after else.

Bob Pearson (6):
  RDMA/rxe: Cleanup mr_check_range
  RDMA/rxe: Move rxe_map_mr_sg to rxe_mr.c
  RDMA-rxe: Isolate mr code from atomic_reply()
  RDMA-rxe: Isolate mr code from atomic_write_reply()
  RDMA/rxe: Cleanup page variables in rxe_mr.c
  RDMA/rxe: Replace rxe_map and rxe_phys_buf by xarray

 drivers/infiniband/sw/rxe/rxe_loc.h   |   6 +-
 drivers/infiniband/sw/rxe/rxe_mr.c    | 574 +++++++++++++++-----------
 drivers/infiniband/sw/rxe/rxe_resp.c  | 107 ++---
 drivers/infiniband/sw/rxe/rxe_verbs.c |  36 --
 drivers/infiniband/sw/rxe/rxe_verbs.h |  32 +-
 5 files changed, 374 insertions(+), 381 deletions(-)


base-commit: bd99ede8ef2dc03e29a181b755ba4f78da2644e6
-- 
2.37.2

[PATCH for-next v3 1/6] RDMA/rxe: Cleanup mr_check_range

[Bob Pearson]

Remove blank lines and replace EFAULT by EINVAL when an invalid
mr type is used.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_mr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 072eac4b65d2..632ee1e516a1 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -26,8 +26,6 @@ u8 rxe_get_next_key(u32 last_key)
 
 int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length)
 {
-
-
 	switch (mr->ibmr.type) {
 	case IB_MR_TYPE_DMA:
 		return 0;
@@ -41,7 +39,7 @@ int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length)
 
 	default:
 		rxe_dbg_mr(mr, "type (%d) not supported\n", mr->ibmr.type);
-		return -EFAULT;
+		return -EINVAL;
 	}
 }
 
-- 
2.37.2

[PATCH for-next v3 2/6] RDMA/rxe: Move rxe_map_mr_sg to rxe_mr.c

[Bob Pearson]

Move rxe_map_mr_sg() to rxe_mr.c where it makes a little more sense.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_loc.h   |  2 ++
 drivers/infiniband/sw/rxe/rxe_mr.c    | 36 +++++++++++++++++++++++++++
 drivers/infiniband/sw/rxe/rxe_verbs.c | 36 ---------------------------
 3 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index 948ce4902b10..29b6c2143045 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -69,6 +69,8 @@ int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, int length,
 		enum rxe_mr_copy_dir dir);
 int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
 	      void *addr, int length, enum rxe_mr_copy_dir dir);
+int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
+		  int sg_nents, unsigned int *sg_offset);
 void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
 struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
 			 enum rxe_mr_lookup_type type);
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 632ee1e516a1..229c7259644c 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -223,6 +223,42 @@ int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
 	return err;
 }
 
+static int rxe_set_page(struct ib_mr *ibmr, u64 addr)
+{
+	struct rxe_mr *mr = to_rmr(ibmr);
+	struct rxe_map *map;
+	struct rxe_phys_buf *buf;
+
+	if (unlikely(mr->nbuf == mr->num_buf))
+		return -ENOMEM;
+
+	map = mr->map[mr->nbuf / RXE_BUF_PER_MAP];
+	buf = &map->buf[mr->nbuf % RXE_BUF_PER_MAP];
+
+	buf->addr = addr;
+	buf->size = ibmr->page_size;
+	mr->nbuf++;
+
+	return 0;
+}
+
+int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
+		  int sg_nents, unsigned int *sg_offset)
+{
+	struct rxe_mr *mr = to_rmr(ibmr);
+	int n;
+
+	mr->nbuf = 0;
+
+	n = ib_sg_to_pages(ibmr, sg, sg_nents, sg_offset, rxe_set_page);
+
+	mr->page_shift = ilog2(ibmr->page_size);
+	mr->page_mask = ibmr->page_size - 1;
+	mr->offset = ibmr->iova & mr->page_mask;
+
+	return n;
+}
+
 static void lookup_iova(struct rxe_mr *mr, u64 iova, int *m_out, int *n_out,
 			size_t *offset_out)
 {
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 025b35bf014e..7a902e0a0607 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -948,42 +948,6 @@ static struct ib_mr *rxe_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type,
 	return ERR_PTR(err);
 }
 
-static int rxe_set_page(struct ib_mr *ibmr, u64 addr)
-{
-	struct rxe_mr *mr = to_rmr(ibmr);
-	struct rxe_map *map;
-	struct rxe_phys_buf *buf;
-
-	if (unlikely(mr->nbuf == mr->num_buf))
-		return -ENOMEM;
-
-	map = mr->map[mr->nbuf / RXE_BUF_PER_MAP];
-	buf = &map->buf[mr->nbuf % RXE_BUF_PER_MAP];
-
-	buf->addr = addr;
-	buf->size = ibmr->page_size;
-	mr->nbuf++;
-
-	return 0;
-}
-
-static int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
-			 int sg_nents, unsigned int *sg_offset)
-{
-	struct rxe_mr *mr = to_rmr(ibmr);
-	int n;
-
-	mr->nbuf = 0;
-
-	n = ib_sg_to_pages(ibmr, sg, sg_nents, sg_offset, rxe_set_page);
-
-	mr->page_shift = ilog2(ibmr->page_size);
-	mr->page_mask = ibmr->page_size - 1;
-	mr->offset = ibmr->iova & mr->page_mask;
-
-	return n;
-}
-
 static ssize_t parent_show(struct device *device,
 			   struct device_attribute *attr, char *buf)
 {
-- 
2.37.2

[PATCH for-next v3 3/6] RDMA-rxe: Isolate mr code from atomic_reply()

[Bob Pearson]

Isolate mr specific code from atomic_reply() in rxe_resp.c into
a subroutine rxe_mr_do_atomic_op() in rxe_mr.c.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_loc.h  |  2 ++
 drivers/infiniband/sw/rxe/rxe_mr.c   | 30 +++++++++++++++++
 drivers/infiniband/sw/rxe/rxe_resp.c | 49 ++++++++--------------------
 3 files changed, 45 insertions(+), 36 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index 29b6c2143045..bcb1bbcf50df 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -72,6 +72,8 @@ int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
 int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
 		  int sg_nents, unsigned int *sg_offset);
 void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
+int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
+			u64 compare, u64 swap_add, u64 *orig_val);
 struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
 			 enum rxe_mr_lookup_type type);
 int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 229c7259644c..791731be6067 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -538,6 +538,36 @@ int copy_data(
 	return err;
 }
 
+static DEFINE_SPINLOCK(atomic_ops_lock);
+
+int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
+			u64 compare, u64 swap_add, u64 *orig_val)
+{
+	u64 *vaddr = iova_to_vaddr(mr, iova, sizeof(u64));
+	u64 value;
+
+	/* needs to match rxe_resp.c */
+	if (mr->state != RXE_MR_STATE_VALID || !vaddr)
+		return -EFAULT;
+	if ((uintptr_t)vaddr & 0x7)
+		return -EINVAL;
+
+	spin_lock_bh(&atomic_ops_lock);
+	value = *orig_val = *vaddr;
+
+	if (opcode == IB_OPCODE_RC_COMPARE_SWAP) {
+		if (value == compare)
+			value = swap_add;
+	} else {
+		value += swap_add;
+	}
+
+	*vaddr = value;
+	spin_unlock_bh(&atomic_ops_lock);
+
+	return 0;
+}
+
 int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
 {
 	struct rxe_sge		*sge	= &dma->sge[dma->cur_sge];
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index c74972244f08..02301e3f343c 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -725,17 +725,12 @@ static enum resp_states process_flush(struct rxe_qp *qp,
 	return RESPST_ACKNOWLEDGE;
 }
 
-/* Guarantee atomicity of atomic operations at the machine level. */
-static DEFINE_SPINLOCK(atomic_ops_lock);
-
 static enum resp_states atomic_reply(struct rxe_qp *qp,
-					 struct rxe_pkt_info *pkt)
+				     struct rxe_pkt_info *pkt)
 {
-	u64 *vaddr;
-	enum resp_states ret;
 	struct rxe_mr *mr = qp->resp.mr;
 	struct resp_res *res = qp->resp.res;
-	u64 value;
+	int err;
 
 	if (!res) {
 		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_MASK);
@@ -743,32 +738,16 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
 	}
 
 	if (!res->replay) {
-		if (mr->state != RXE_MR_STATE_VALID) {
-			ret = RESPST_ERR_RKEY_VIOLATION;
-			goto out;
-		}
-
-		vaddr = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset,
-					sizeof(u64));
-
-		/* check vaddr is 8 bytes aligned. */
-		if (!vaddr || (uintptr_t)vaddr & 7) {
-			ret = RESPST_ERR_MISALIGNED_ATOMIC;
-			goto out;
-		}
-
-		spin_lock_bh(&atomic_ops_lock);
-		res->atomic.orig_val = value = *vaddr;
-
-		if (pkt->opcode == IB_OPCODE_RC_COMPARE_SWAP) {
-			if (value == atmeth_comp(pkt))
-				value = atmeth_swap_add(pkt);
-		} else {
-			value += atmeth_swap_add(pkt);
-		}
-
-		*vaddr = value;
-		spin_unlock_bh(&atomic_ops_lock);
+		u64 iova = qp->resp.va + qp->resp.offset;
+
+		err = rxe_mr_do_atomic_op(mr, iova, pkt->opcode,
+					  atmeth_comp(pkt),
+					  atmeth_swap_add(pkt),
+					  &res->atomic.orig_val);
+		if (err == -EINVAL)
+			return RESPST_ERR_MISALIGNED_ATOMIC;
+		else if (err)
+			return RESPST_ERR_RKEY_VIOLATION;
 
 		qp->resp.msn++;
 
@@ -780,9 +759,7 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
 		qp->resp.status = IB_WC_SUCCESS;
 	}
 
-	ret = RESPST_ACKNOWLEDGE;
-out:
-	return ret;
+	return RESPST_ACKNOWLEDGE;
 }
 
 #ifdef CONFIG_64BIT
-- 
2.37.2

[PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Bob Pearson]

Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
Check length for atomic write operation.
Make iova_to_vaddr() static.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
v3:
  Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
  write if CONFIG_64BIT is not defined as orignally intended by the
  developers of the atomic write implementation.
link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/

 drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
 drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
 drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
 3 files changed, 73 insertions(+), 36 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index bcb1bbcf50df..fd70c71a9e4e 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
 void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
 int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 			u64 compare, u64 swap_add, u64 *orig_val);
+int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
 struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
 			 enum rxe_mr_lookup_type type);
 int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 791731be6067..1e74f5e8e10b 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 	return 0;
 }
 
+/**
+ * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
+ * @mr: memory region
+ * @iova: iova in mr
+ * @addr: source of data to write
+ *
+ * Returns:
+ *	 0 on success
+ *	-1 for misaligned address
+ *	-2 for access errors
+ *	-3 for cpu without native 64 bit support
+ */
+int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
+{
+#if defined CONFIG_64BIT
+	u64 *vaddr;
+	u64 value;
+	unsigned int length = 8;
+
+	/* See IBA oA19-28 */
+	if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
+		rxe_dbg_mr(mr, "mr not valid");
+		return -2;
+	}
+
+	/* See IBA A19.4.2 */
+	if (unlikely((uintptr_t)vaddr & 0x7 || iova & 0x7)) {
+		rxe_dbg_mr(mr, "misaligned address");
+		return -1;
+	}
+
+	vaddr = iova_to_vaddr(mr, iova, length);
+	if (unlikely(!vaddr)) {
+		rxe_dbg_mr(mr, "iova out of range");
+		return -2;
+	}
+
+	/* this makes no sense. What of payload is not 8? */
+	memcpy(&value, addr, length);
+
+	/* Do atomic write after all prior operations have completed */
+	smp_store_release(vaddr, value);
+
+	return 0;
+#else
+	rxe_dbg_mr(mr, "64 bit integers not atomic");
+	return -3;
+#endif
+}
+
 int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
 {
 	struct rxe_sge		*sge	= &dma->sge[dma->cur_sge];
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 02301e3f343c..1083cda22c65 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -762,26 +762,33 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
 	return RESPST_ACKNOWLEDGE;
 }
 
-#ifdef CONFIG_64BIT
-static enum resp_states do_atomic_write(struct rxe_qp *qp,
-					struct rxe_pkt_info *pkt)
+static enum resp_states atomic_write_reply(struct rxe_qp *qp,
+					   struct rxe_pkt_info *pkt)
 {
+	u64 iova = qp->resp.va + qp->resp.offset;
+	struct resp_res *res = qp->resp.res;
 	struct rxe_mr *mr = qp->resp.mr;
+	void *addr = payload_addr(pkt);
 	int payload = payload_size(pkt);
-	u64 src, *dst;
-
-	if (mr->state != RXE_MR_STATE_VALID)
-		return RESPST_ERR_RKEY_VIOLATION;
+	int err;
 
-	memcpy(&src, payload_addr(pkt), payload);
+	if (!res) {
+		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
+		qp->resp.res = res;
+	}
 
-	dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
-	/* check vaddr is 8 bytes aligned. */
-	if (!dst || (uintptr_t)dst & 7)
-		return RESPST_ERR_MISALIGNED_ATOMIC;
+	if (res->replay)
+		return RESPST_ACKNOWLEDGE;
 
-	/* Do atomic write after all prior operations have completed */
-	smp_store_release(dst, src);
+	err = rxe_mr_do_atomic_write(mr, iova, addr);
+	if (unlikely(err)) {
+		if (err == -3)
+			return RESPST_ERR_UNSUPPORTED_OPCODE;
+		else if (err == -2)
+			return RESPST_ERR_RKEY_VIOLATION;
+		else
+			return RESPST_ERR_MISALIGNED_ATOMIC;
+	}
 
 	/* decrease resp.resid to zero */
 	qp->resp.resid -= sizeof(payload);
@@ -794,29 +801,8 @@ static enum resp_states do_atomic_write(struct rxe_qp *qp,
 
 	qp->resp.opcode = pkt->opcode;
 	qp->resp.status = IB_WC_SUCCESS;
-	return RESPST_ACKNOWLEDGE;
-}
-#else
-static enum resp_states do_atomic_write(struct rxe_qp *qp,
-					struct rxe_pkt_info *pkt)
-{
-	return RESPST_ERR_UNSUPPORTED_OPCODE;
-}
-#endif /* CONFIG_64BIT */
-
-static enum resp_states atomic_write_reply(struct rxe_qp *qp,
-					   struct rxe_pkt_info *pkt)
-{
-	struct resp_res *res = qp->resp.res;
 
-	if (!res) {
-		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
-		qp->resp.res = res;
-	}
-
-	if (res->replay)
-		return RESPST_ACKNOWLEDGE;
-	return do_atomic_write(qp, pkt);
+	return RESPST_ACKNOWLEDGE;
 }
 
 static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
-- 
2.37.2

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[lizhijian]

On 14/01/2023 07:27, Bob Pearson wrote:
> Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
> a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
> Check length for atomic write operation.
> Make iova_to_vaddr() static.
> 
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
> v3:
>    Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
>    write if CONFIG_64BIT is not defined as orignally intended by the
>    developers of the atomic write implementation.
> link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/
> 
>   drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
>   drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
>   drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
>   3 files changed, 73 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index bcb1bbcf50df..fd70c71a9e4e 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
>   void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
>   int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>   			u64 compare, u64 swap_add, u64 *orig_val);
> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
>   struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
>   			 enum rxe_mr_lookup_type type);
>   int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index 791731be6067..1e74f5e8e10b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>   	return 0;
>   }
>   
> +/**
> + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
> + * @mr: memory region
> + * @iova: iova in mr
> + * @addr: source of data to write
> + *
> + * Returns:
> + *	 0 on success
> + *	-1 for misaligned address
> + *	-2 for access errors
> + *	-3 for cpu without native 64 bit support

Well, i'm not favor of such error code. especialy when it's not a staic subroutine.
Any comments from other maintainers ?


> + */
> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
> +{
> +#if defined CONFIG_64BIT
> +	u64 *vaddr;
> +	u64 value;
> +	unsigned int length = 8;
> +
> +	/* See IBA oA19-28 */
> +	if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
> +		rxe_dbg_mr(mr, "mr not valid");
> +		return -2;
> +	}
> +
> +	/* See IBA A19.4.2 */
> +	if (unlikely((uintptr_t)vaddr & 0x7 || iova & 0x7)) {

vaddr used before being initialized


Thanks
Zhijian


> +		rxe_dbg_mr(mr, "misaligned address");
> +		return -1;
> +	}
> +
> +	vaddr = iova_to_vaddr(mr, iova, length);
> +	if (unlikely(!vaddr)) {
> +		rxe_dbg_mr(mr, "iova out of range");
> +		return -2;
> +	}
> +
> +	/* this makes no sense. What of payload is not 8? */
> +	memcpy(&value, addr, length);
> +
> +	/* Do atomic write after all prior operations have completed */
> +	smp_store_release(vaddr, value);
> +
> +	return 0;
> +#else
> +	rxe_dbg_mr(mr, "64 bit integers not atomic");
> +	return -3;
> +#endif
> +}
> +
>   int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
>   {
>   	struct rxe_sge		*sge	= &dma->sge[dma->cur_sge];
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index 02301e3f343c..1083cda22c65 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -762,26 +762,33 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
>   	return RESPST_ACKNOWLEDGE;
>   }
>   
> -#ifdef CONFIG_64BIT
> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
> -					struct rxe_pkt_info *pkt)
> +static enum resp_states atomic_write_reply(struct rxe_qp *qp,
> +					   struct rxe_pkt_info *pkt)
>   {
> +	u64 iova = qp->resp.va + qp->resp.offset;
> +	struct resp_res *res = qp->resp.res;
>   	struct rxe_mr *mr = qp->resp.mr;
> +	void *addr = payload_addr(pkt);
>   	int payload = payload_size(pkt);
> -	u64 src, *dst;
> -
> -	if (mr->state != RXE_MR_STATE_VALID)
> -		return RESPST_ERR_RKEY_VIOLATION;
> +	int err;
>   
> -	memcpy(&src, payload_addr(pkt), payload);
> +	if (!res) {
> +		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
> +		qp->resp.res = res;
> +	}
>   
> -	dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
> -	/* check vaddr is 8 bytes aligned. */
> -	if (!dst || (uintptr_t)dst & 7)
> -		return RESPST_ERR_MISALIGNED_ATOMIC;
> +	if (res->replay)
> +		return RESPST_ACKNOWLEDGE;
>   
> -	/* Do atomic write after all prior operations have completed */
> -	smp_store_release(dst, src);
> +	err = rxe_mr_do_atomic_write(mr, iova, addr);
> +	if (unlikely(err)) {
> +		if (err == -3)
> +			return RESPST_ERR_UNSUPPORTED_OPCODE;
> +		else if (err == -2)
> +			return RESPST_ERR_RKEY_VIOLATION;
> +		else
> +			return RESPST_ERR_MISALIGNED_ATOMIC;
> +	}
>   
>   	/* decrease resp.resid to zero */
>   	qp->resp.resid -= sizeof(payload);
> @@ -794,29 +801,8 @@ static enum resp_states do_atomic_write(struct rxe_qp *qp,
>   
>   	qp->resp.opcode = pkt->opcode;
>   	qp->resp.status = IB_WC_SUCCESS;
> -	return RESPST_ACKNOWLEDGE;
> -}
> -#else
> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
> -					struct rxe_pkt_info *pkt)
> -{
> -	return RESPST_ERR_UNSUPPORTED_OPCODE;
> -}
> -#endif /* CONFIG_64BIT */
> -
> -static enum resp_states atomic_write_reply(struct rxe_qp *qp,
> -					   struct rxe_pkt_info *pkt)
> -{
> -	struct resp_res *res = qp->resp.res;
>   
> -	if (!res) {
> -		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
> -		qp->resp.res = res;
> -	}
> -
> -	if (res->replay)
> -		return RESPST_ACKNOWLEDGE;
> -	return do_atomic_write(qp, pkt);
> +	return RESPST_ACKNOWLEDGE;
>   }
>   
>   static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Jason Gunthorpe]

On Mon, Jan 16, 2023 at 02:11:29AM +0000, lizhijian@fujitsu.com wrote:
> > +/**
> > + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
> > + * @mr: memory region
> > + * @iova: iova in mr
> > + * @addr: source of data to write
> > + *
> > + * Returns:
> > + *	 0 on success
> > + *	-1 for misaligned address
> > + *	-2 for access errors
> > + *	-3 for cpu without native 64 bit support
> 
> Well, i'm not favor of such error code. especialy when it's not a staic subroutine.
> Any comments from other maintainers ?

It should at least have proper constants, but can it be structured to
avoid this in the first place?

Jason

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Zhu Yanjun]

On Sat, Jan 14, 2023 at 7:28 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
> a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
> Check length for atomic write operation.
> Make iova_to_vaddr() static.
>
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
> v3:
>   Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
>   write if CONFIG_64BIT is not defined as orignally intended by the
>   developers of the atomic write implementation.
> link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/
>
>  drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
>  drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
>  drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
>  3 files changed, 73 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index bcb1bbcf50df..fd70c71a9e4e 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
>  void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
>  int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>                         u64 compare, u64 swap_add, u64 *orig_val);
> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
>  struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
>                          enum rxe_mr_lookup_type type);
>  int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index 791731be6067..1e74f5e8e10b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>         return 0;
>  }
>
> +/**
> + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
> + * @mr: memory region
> + * @iova: iova in mr
> + * @addr: source of data to write
> + *
> + * Returns:
> + *      0 on success
> + *     -1 for misaligned address
> + *     -2 for access errors
> + *     -3 for cpu without native 64 bit support
> + */
> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
> +{
> +#if defined CONFIG_64BIT

IS_ENABLED is better?

Zhu Yanjun

> +       u64 *vaddr;
> +       u64 value;
> +       unsigned int length = 8;
> +
> +       /* See IBA oA19-28 */
> +       if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
> +               rxe_dbg_mr(mr, "mr not valid");
> +               return -2;
> +       }
> +
> +       /* See IBA A19.4.2 */
> +       if (unlikely((uintptr_t)vaddr & 0x7 || iova & 0x7)) {
> +               rxe_dbg_mr(mr, "misaligned address");
> +               return -1;
> +       }
> +
> +       vaddr = iova_to_vaddr(mr, iova, length);
> +       if (unlikely(!vaddr)) {
> +               rxe_dbg_mr(mr, "iova out of range");
> +               return -2;
> +       }
> +
> +       /* this makes no sense. What of payload is not 8? */
> +       memcpy(&value, addr, length);
> +
> +       /* Do atomic write after all prior operations have completed */
> +       smp_store_release(vaddr, value);
> +
> +       return 0;
> +#else
> +       rxe_dbg_mr(mr, "64 bit integers not atomic");
> +       return -3;
> +#endif
> +}
> +
>  int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
>  {
>         struct rxe_sge          *sge    = &dma->sge[dma->cur_sge];
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index 02301e3f343c..1083cda22c65 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -762,26 +762,33 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
>         return RESPST_ACKNOWLEDGE;
>  }
>
> -#ifdef CONFIG_64BIT
> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
> -                                       struct rxe_pkt_info *pkt)
> +static enum resp_states atomic_write_reply(struct rxe_qp *qp,
> +                                          struct rxe_pkt_info *pkt)
>  {
> +       u64 iova = qp->resp.va + qp->resp.offset;
> +       struct resp_res *res = qp->resp.res;
>         struct rxe_mr *mr = qp->resp.mr;
> +       void *addr = payload_addr(pkt);
>         int payload = payload_size(pkt);
> -       u64 src, *dst;
> -
> -       if (mr->state != RXE_MR_STATE_VALID)
> -               return RESPST_ERR_RKEY_VIOLATION;
> +       int err;
>
> -       memcpy(&src, payload_addr(pkt), payload);
> +       if (!res) {
> +               res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
> +               qp->resp.res = res;
> +       }
>
> -       dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
> -       /* check vaddr is 8 bytes aligned. */
> -       if (!dst || (uintptr_t)dst & 7)
> -               return RESPST_ERR_MISALIGNED_ATOMIC;
> +       if (res->replay)
> +               return RESPST_ACKNOWLEDGE;
>
> -       /* Do atomic write after all prior operations have completed */
> -       smp_store_release(dst, src);
> +       err = rxe_mr_do_atomic_write(mr, iova, addr);
> +       if (unlikely(err)) {
> +               if (err == -3)
> +                       return RESPST_ERR_UNSUPPORTED_OPCODE;
> +               else if (err == -2)
> +                       return RESPST_ERR_RKEY_VIOLATION;
> +               else
> +                       return RESPST_ERR_MISALIGNED_ATOMIC;
> +       }
>
>         /* decrease resp.resid to zero */
>         qp->resp.resid -= sizeof(payload);
> @@ -794,29 +801,8 @@ static enum resp_states do_atomic_write(struct rxe_qp *qp,
>
>         qp->resp.opcode = pkt->opcode;
>         qp->resp.status = IB_WC_SUCCESS;
> -       return RESPST_ACKNOWLEDGE;
> -}
> -#else
> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
> -                                       struct rxe_pkt_info *pkt)
> -{
> -       return RESPST_ERR_UNSUPPORTED_OPCODE;
> -}
> -#endif /* CONFIG_64BIT */
> -
> -static enum resp_states atomic_write_reply(struct rxe_qp *qp,
> -                                          struct rxe_pkt_info *pkt)
> -{
> -       struct resp_res *res = qp->resp.res;
>
> -       if (!res) {
> -               res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
> -               qp->resp.res = res;
> -       }
> -
> -       if (res->replay)
> -               return RESPST_ACKNOWLEDGE;
> -       return do_atomic_write(qp, pkt);
> +       return RESPST_ACKNOWLEDGE;
>  }
>
>  static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
> --
> 2.37.2
>

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Jason Gunthorpe]

On Tue, Jan 17, 2023 at 09:36:02AM +0800, Zhu Yanjun wrote:
> On Sat, Jan 14, 2023 at 7:28 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >
> > Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
> > a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
> > Check length for atomic write operation.
> > Make iova_to_vaddr() static.
> >
> > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> > ---
> > v3:
> >   Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
> >   write if CONFIG_64BIT is not defined as orignally intended by the
> >   developers of the atomic write implementation.
> > link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/
> >
> >  drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
> >  drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
> >  drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
> >  3 files changed, 73 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> > index bcb1bbcf50df..fd70c71a9e4e 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> > @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
> >  void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
> >  int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> >                         u64 compare, u64 swap_add, u64 *orig_val);
> > +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
> >  struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
> >                          enum rxe_mr_lookup_type type);
> >  int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
> > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> > index 791731be6067..1e74f5e8e10b 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> > @@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> >         return 0;
> >  }
> >
> > +/**
> > + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
> > + * @mr: memory region
> > + * @iova: iova in mr
> > + * @addr: source of data to write
> > + *
> > + * Returns:
> > + *      0 on success
> > + *     -1 for misaligned address
> > + *     -2 for access errors
> > + *     -3 for cpu without native 64 bit support
> > + */
> > +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
> > +{
> > +#if defined CONFIG_64BIT
> 
> IS_ENABLED is better?

is_enabled won't work here because the code doesn't compile.

Jason

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Bob Pearson]

On 1/17/23 07:47, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2023 at 09:36:02AM +0800, Zhu Yanjun wrote:
>> On Sat, Jan 14, 2023 at 7:28 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>>
>>> Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
>>> a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
>>> Check length for atomic write operation.
>>> Make iova_to_vaddr() static.
>>>
>>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>>> ---
>>> v3:
>>>   Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
>>>   write if CONFIG_64BIT is not defined as orignally intended by the
>>>   developers of the atomic write implementation.
>>> link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/
>>>
>>>  drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
>>>  drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
>>>  drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
>>>  3 files changed, 73 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
>>> index bcb1bbcf50df..fd70c71a9e4e 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
>>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
>>> @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
>>>  void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
>>>  int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>>>                         u64 compare, u64 swap_add, u64 *orig_val);
>>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
>>>  struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
>>>                          enum rxe_mr_lookup_type type);
>>>  int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> index 791731be6067..1e74f5e8e10b 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> @@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>>>         return 0;
>>>  }
>>>
>>> +/**
>>> + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
>>> + * @mr: memory region
>>> + * @iova: iova in mr
>>> + * @addr: source of data to write
>>> + *
>>> + * Returns:
>>> + *      0 on success
>>> + *     -1 for misaligned address
>>> + *     -2 for access errors
>>> + *     -3 for cpu without native 64 bit support
>>> + */
>>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
>>> +{
>>> +#if defined CONFIG_64BIT
>>
>> IS_ENABLED is better?
> 
> is_enabled won't work here because the code doesn't compile.
> 
> Jason

exactly.

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Zhu Yanjun]

On Wed, Jan 18, 2023 at 12:45 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 1/17/23 07:47, Jason Gunthorpe wrote:
> > On Tue, Jan 17, 2023 at 09:36:02AM +0800, Zhu Yanjun wrote:
> >> On Sat, Jan 14, 2023 at 7:28 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>>
> >>> Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
> >>> a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
> >>> Check length for atomic write operation.
> >>> Make iova_to_vaddr() static.
> >>>
> >>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> >>> ---
> >>> v3:
> >>>   Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
> >>>   write if CONFIG_64BIT is not defined as orignally intended by the
> >>>   developers of the atomic write implementation.
> >>> link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/
> >>>
> >>>  drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
> >>>  drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
> >>>  drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
> >>>  3 files changed, 73 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> >>> index bcb1bbcf50df..fd70c71a9e4e 100644
> >>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> >>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> >>> @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
> >>>  void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
> >>>  int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> >>>                         u64 compare, u64 swap_add, u64 *orig_val);
> >>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
> >>>  struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
> >>>                          enum rxe_mr_lookup_type type);
> >>>  int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> >>> index 791731be6067..1e74f5e8e10b 100644
> >>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> >>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> >>> @@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> >>>         return 0;
> >>>  }
> >>>
> >>> +/**
> >>> + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
> >>> + * @mr: memory region
> >>> + * @iova: iova in mr
> >>> + * @addr: source of data to write
> >>> + *
> >>> + * Returns:
> >>> + *      0 on success
> >>> + *     -1 for misaligned address
> >>> + *     -2 for access errors
> >>> + *     -3 for cpu without native 64 bit support
> >>> + */
> >>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
> >>> +{
> >>> +#if defined CONFIG_64BIT
> >>
> >> IS_ENABLED is better?
> >
> > is_enabled won't work here because the code doesn't compile.
> >

drivers/infiniband/sw/rxe/rxe_net.c:

 45
 46 #if IS_ENABLED(CONFIG_IPV6)
 47 static struct dst_entry *rxe_find_route6(struct rxe_qp *qp,
 48                                          struct net_device *ndev,
 49                                          struct in6_addr *saddr,
 50                                          struct in6_addr *daddr)
 51 {
 52         struct dst_entry *ndst;
 53         struct flowi6 fl6 = { { 0 } };

Zhu Yanjun

> > Jason
>
> exactly.

Re: [PATCH for-next v3 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Jason Gunthorpe]

On Wed, Jan 18, 2023 at 08:18:06AM +0800, Zhu Yanjun wrote:
> On Wed, Jan 18, 2023 at 12:45 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >
> > On 1/17/23 07:47, Jason Gunthorpe wrote:
> > > On Tue, Jan 17, 2023 at 09:36:02AM +0800, Zhu Yanjun wrote:
> > >> On Sat, Jan 14, 2023 at 7:28 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> > >>>
> > >>> Isolate mr specific code from atomic_write_reply() in rxe_resp.c into
> > >>> a subroutine rxe_mr_do_atomic_write() in rxe_mr.c.
> > >>> Check length for atomic write operation.
> > >>> Make iova_to_vaddr() static.
> > >>>
> > >>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> > >>> ---
> > >>> v3:
> > >>>   Fixed bug reported by kernel test robot. Ifdef'ed out atomic 8 byte
> > >>>   write if CONFIG_64BIT is not defined as orignally intended by the
> > >>>   developers of the atomic write implementation.
> > >>> link: https://lore.kernel.org/linux-rdma/202301131143.CmoyVcul-lkp@intel.com/
> > >>>
> > >>>  drivers/infiniband/sw/rxe/rxe_loc.h  |  1 +
> > >>>  drivers/infiniband/sw/rxe/rxe_mr.c   | 50 ++++++++++++++++++++++++
> > >>>  drivers/infiniband/sw/rxe/rxe_resp.c | 58 +++++++++++-----------------
> > >>>  3 files changed, 73 insertions(+), 36 deletions(-)
> > >>>
> > >>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> > >>> index bcb1bbcf50df..fd70c71a9e4e 100644
> > >>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> > >>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> > >>> @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
> > >>>  void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
> > >>>  int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> > >>>                         u64 compare, u64 swap_add, u64 *orig_val);
> > >>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
> > >>>  struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
> > >>>                          enum rxe_mr_lookup_type type);
> > >>>  int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
> > >>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> > >>> index 791731be6067..1e74f5e8e10b 100644
> > >>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> > >>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> > >>> @@ -568,6 +568,56 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> > >>>         return 0;
> > >>>  }
> > >>>
> > >>> +/**
> > >>> + * rxe_mr_do_atomic_write() - write 64bit value to iova from addr
> > >>> + * @mr: memory region
> > >>> + * @iova: iova in mr
> > >>> + * @addr: source of data to write
> > >>> + *
> > >>> + * Returns:
> > >>> + *      0 on success
> > >>> + *     -1 for misaligned address
> > >>> + *     -2 for access errors
> > >>> + *     -3 for cpu without native 64 bit support
> > >>> + */
> > >>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
> > >>> +{
> > >>> +#if defined CONFIG_64BIT
> > >>
> > >> IS_ENABLED is better?
> > >
> > > is_enabled won't work here because the code doesn't compile.
> > >
> 
> drivers/infiniband/sw/rxe/rxe_net.c:
>
>  45
>  46 #if IS_ENABLED(CONFIG_IPV6)

You only need this pattern if the config symbol is tristate

CONFIG_64BIT is a bool

Jason

[PATCH for-next v3 5/6] RDMA/rxe: Cleanup page variables in rxe_mr.c

[Bob Pearson]

Cleanup usage of mr->page_shift and mr->page_mask and introduce
an extractor for mr->ibmr.page_size. Normal usage in the kernel
has page_mask masking out offset in page rather than masking out
the page number. The rxe driver had reversed that which was confusing.
Implicitly there can be a per mr page_size which was not uniformly
supported.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_mr.c    | 31 ++++++++++++---------------
 drivers/infiniband/sw/rxe/rxe_verbs.h | 11 +++++++---
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 1e74f5e8e10b..fd5537ee7f04 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -60,6 +60,9 @@ static void rxe_mr_init(int access, struct rxe_mr *mr)
 	mr->lkey = mr->ibmr.lkey = lkey;
 	mr->rkey = mr->ibmr.rkey = rkey;
 
+	mr->ibmr.page_size = PAGE_SIZE;
+	mr->page_mask = PAGE_MASK;
+	mr->page_shift = PAGE_SHIFT;
 	mr->state = RXE_MR_STATE_INVALID;
 }
 
@@ -149,9 +152,6 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
 		goto err_release_umem;
 	}
 
-	mr->page_shift = PAGE_SHIFT;
-	mr->page_mask = PAGE_SIZE - 1;
-
 	num_buf			= 0;
 	map = mr->map;
 	if (length > 0) {
@@ -180,7 +180,7 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
 				goto err_release_umem;
 			}
 			buf->addr = (uintptr_t)vaddr;
-			buf->size = PAGE_SIZE;
+			buf->size = mr_page_size(mr);
 			num_buf++;
 			buf++;
 
@@ -189,10 +189,9 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
 
 	mr->umem = umem;
 	mr->access = access;
-	mr->offset = ib_umem_offset(umem);
+	mr->page_offset = ib_umem_offset(umem);
 	mr->state = RXE_MR_STATE_VALID;
 	mr->ibmr.type = IB_MR_TYPE_USER;
-	mr->ibmr.page_size = PAGE_SIZE;
 
 	return 0;
 
@@ -246,29 +245,27 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
 		  int sg_nents, unsigned int *sg_offset)
 {
 	struct rxe_mr *mr = to_rmr(ibmr);
-	int n;
-
-	mr->nbuf = 0;
+	unsigned int page_size = mr_page_size(mr);
 
-	n = ib_sg_to_pages(ibmr, sg, sg_nents, sg_offset, rxe_set_page);
+	mr->page_shift = ilog2(page_size);
+	mr->page_mask = ~((u64)page_size - 1);
+	mr->page_offset = ibmr->iova & (page_size - 1);
 
-	mr->page_shift = ilog2(ibmr->page_size);
-	mr->page_mask = ibmr->page_size - 1;
-	mr->offset = ibmr->iova & mr->page_mask;
+	mr->nbuf = 0;
 
-	return n;
+	return ib_sg_to_pages(ibmr, sg, sg_nents, sg_offset, rxe_set_page);
 }
 
 static void lookup_iova(struct rxe_mr *mr, u64 iova, int *m_out, int *n_out,
 			size_t *offset_out)
 {
-	size_t offset = iova - mr->ibmr.iova + mr->offset;
+	size_t offset = iova - mr->ibmr.iova + mr->page_offset;
 	int			map_index;
 	int			buf_index;
 	u64			length;
 
 	if (likely(mr->page_shift)) {
-		*offset_out = offset & mr->page_mask;
+		*offset_out = offset & (mr_page_size(mr) - 1);
 		offset >>= mr->page_shift;
 		*n_out = offset & mr->map_mask;
 		*m_out = offset >> mr->map_shift;
@@ -342,7 +339,7 @@ int rxe_flush_pmem_iova(struct rxe_mr *mr, u64 iova, int length)
 	if (mr->ibmr.type == IB_MR_TYPE_DMA)
 		return -EFAULT;
 
-	offset = (iova - mr->ibmr.iova + mr->offset) & mr->page_mask;
+	offset = (iova - mr->ibmr.iova + mr->page_offset) & mr->page_mask;
 	while (length > 0) {
 		u8 *va;
 		int bytes;
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index 19ddfa890480..bfc94caaeec5 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -310,11 +310,11 @@ struct rxe_mr {
 	u32			lkey;
 	u32			rkey;
 	enum rxe_mr_state	state;
-	u32			offset;
 	int			access;
 
-	int			page_shift;
-	int			page_mask;
+	unsigned int		page_offset;
+	unsigned int		page_shift;
+	u64			page_mask;
 	int			map_shift;
 	int			map_mask;
 
@@ -329,6 +329,11 @@ struct rxe_mr {
 	struct rxe_map		**map;
 };
 
+static inline unsigned int mr_page_size(struct rxe_mr *mr)
+{
+	return mr ? mr->ibmr.page_size : PAGE_SIZE;
+}
+
 enum rxe_mw_state {
 	RXE_MW_STATE_INVALID	= RXE_MR_STATE_INVALID,
 	RXE_MW_STATE_FREE	= RXE_MR_STATE_FREE,
-- 
2.37.2

[PATCH for-next v3 6/6] RDMA/rxe: Replace rxe_map and rxe_phys_buf by xarray

[Bob Pearson]

Replace struct rxe-phys_buf and struct rxe_map by struct xarray
in rxe_verbs.h. This allows using rcu locking on reads for
the memory maps stored in each mr.

This is based off of a sketch of a patch from Jason Gunthorpe in the
link below. Some changes were needed to make this work. It applies
cleanly to the current for-next and passes the pyverbs, perftest
and the same blktests test cases which run today.

Link: https://lore.kernel.org/linux-rdma/Y3gvZr6%2FNCii9Avy@nvidia.com/
Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_loc.h   |   1 -
 drivers/infiniband/sw/rxe/rxe_mr.c    | 533 ++++++++++++--------------
 drivers/infiniband/sw/rxe/rxe_resp.c  |   2 +-
 drivers/infiniband/sw/rxe/rxe_verbs.h |  21 +-
 4 files changed, 251 insertions(+), 306 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index fd70c71a9e4e..0cf78f9bb27c 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -71,7 +71,6 @@ int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
 	      void *addr, int length, enum rxe_mr_copy_dir dir);
 int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
 		  int sg_nents, unsigned int *sg_offset);
-void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
 int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 			u64 compare, u64 swap_add, u64 *orig_val);
 int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index fd5537ee7f04..e4634279080a 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -60,145 +60,113 @@ static void rxe_mr_init(int access, struct rxe_mr *mr)
 	mr->lkey = mr->ibmr.lkey = lkey;
 	mr->rkey = mr->ibmr.rkey = rkey;
 
+	mr->access = access;
 	mr->ibmr.page_size = PAGE_SIZE;
 	mr->page_mask = PAGE_MASK;
 	mr->page_shift = PAGE_SHIFT;
 	mr->state = RXE_MR_STATE_INVALID;
 }
 
-static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf)
-{
-	int i;
-	int num_map;
-	struct rxe_map **map = mr->map;
-
-	num_map = (num_buf + RXE_BUF_PER_MAP - 1) / RXE_BUF_PER_MAP;
-
-	mr->map = kmalloc_array(num_map, sizeof(*map), GFP_KERNEL);
-	if (!mr->map)
-		goto err1;
-
-	for (i = 0; i < num_map; i++) {
-		mr->map[i] = kmalloc(sizeof(**map), GFP_KERNEL);
-		if (!mr->map[i])
-			goto err2;
-	}
-
-	BUILD_BUG_ON(!is_power_of_2(RXE_BUF_PER_MAP));
-
-	mr->map_shift = ilog2(RXE_BUF_PER_MAP);
-	mr->map_mask = RXE_BUF_PER_MAP - 1;
-
-	mr->num_buf = num_buf;
-	mr->num_map = num_map;
-	mr->max_buf = num_map * RXE_BUF_PER_MAP;
-
-	return 0;
-
-err2:
-	for (i--; i >= 0; i--)
-		kfree(mr->map[i]);
-
-	kfree(mr->map);
-	mr->map = NULL;
-err1:
-	return -ENOMEM;
-}
-
 void rxe_mr_init_dma(int access, struct rxe_mr *mr)
 {
 	rxe_mr_init(access, mr);
 
-	mr->access = access;
 	mr->state = RXE_MR_STATE_VALID;
 	mr->ibmr.type = IB_MR_TYPE_DMA;
 }
 
-static bool is_pmem_page(struct page *pg)
+static unsigned long rxe_mr_iova_to_index(struct rxe_mr *mr, u64 iova)
 {
-	unsigned long paddr = page_to_phys(pg);
+	return (iova >> mr->page_shift) - (mr->ibmr.iova >> mr->page_shift);
+}
 
-	return REGION_INTERSECTS ==
-	       region_intersects(paddr, PAGE_SIZE, IORESOURCE_MEM,
-				 IORES_DESC_PERSISTENT_MEMORY);
+static unsigned long rxe_mr_iova_to_page_offset(struct rxe_mr *mr, u64 iova)
+{
+	return iova & (mr_page_size(mr) - 1);
+}
+
+static int rxe_mr_fill_pages_from_sgt(struct rxe_mr *mr, struct sg_table *sgt)
+{
+	XA_STATE(xas, &mr->pages, 0);
+	struct sg_page_iter sg_iter;
+
+	xa_init(&mr->pages);
+
+	__sg_page_iter_start(&sg_iter, sgt->sgl, sgt->orig_nents, 0);
+	if (!__sg_page_iter_next(&sg_iter))
+		return 0;
+
+	do {
+		xas_lock(&xas);
+		while (true) {
+			xas_store(&xas, sg_page_iter_page(&sg_iter));
+			if (xas_error(&xas))
+				break;
+			xas_next(&xas);
+			if (!__sg_page_iter_next(&sg_iter))
+				break;
+		}
+		xas_unlock(&xas);
+	} while (xas_nomem(&xas, GFP_KERNEL));
+
+	return xas_error(&xas);
 }
 
 int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
 		     int access, struct rxe_mr *mr)
 {
-	struct rxe_map		**map;
-	struct rxe_phys_buf	*buf = NULL;
-	struct ib_umem		*umem;
-	struct sg_page_iter	sg_iter;
-	int			num_buf;
-	void			*vaddr;
+	struct ib_umem *umem;
 	int err;
 
+	rxe_mr_init(access, mr);
+
 	umem = ib_umem_get(&rxe->ib_dev, start, length, access);
 	if (IS_ERR(umem)) {
 		rxe_dbg_mr(mr, "Unable to pin memory region err = %d\n",
 			(int)PTR_ERR(umem));
-		err = PTR_ERR(umem);
-		goto err_out;
+		return PTR_ERR(umem);
 	}
 
-	num_buf = ib_umem_num_pages(umem);
-
-	rxe_mr_init(access, mr);
-
-	err = rxe_mr_alloc(mr, num_buf);
+	err = rxe_mr_fill_pages_from_sgt(mr, &umem->sgt_append.sgt);
 	if (err) {
-		rxe_dbg_mr(mr, "Unable to allocate memory for map\n");
-		goto err_release_umem;
+		ib_umem_release(umem);
+		return err;
 	}
 
-	num_buf			= 0;
-	map = mr->map;
-	if (length > 0) {
-		bool persistent_access = access & IB_ACCESS_FLUSH_PERSISTENT;
-
-		buf = map[0]->buf;
-		for_each_sgtable_page (&umem->sgt_append.sgt, &sg_iter, 0) {
-			struct page *pg = sg_page_iter_page(&sg_iter);
+	mr->umem = umem;
+	mr->ibmr.type = IB_MR_TYPE_USER;
+	mr->state = RXE_MR_STATE_VALID;
 
-			if (persistent_access && !is_pmem_page(pg)) {
-				rxe_dbg_mr(mr, "Unable to register persistent access to non-pmem device\n");
-				err = -EINVAL;
-				goto err_release_umem;
-			}
+	return 0;
+}
 
-			if (num_buf >= RXE_BUF_PER_MAP) {
-				map++;
-				buf = map[0]->buf;
-				num_buf = 0;
-			}
+static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf)
+{
+	XA_STATE(xas, &mr->pages, 0);
+	int i = 0;
+	int err;
 
-			vaddr = page_address(pg);
-			if (!vaddr) {
-				rxe_dbg_mr(mr, "Unable to get virtual address\n");
-				err = -ENOMEM;
-				goto err_release_umem;
-			}
-			buf->addr = (uintptr_t)vaddr;
-			buf->size = mr_page_size(mr);
-			num_buf++;
-			buf++;
+	xa_init(&mr->pages);
 
+	do {
+		xas_lock(&xas);
+		while (i != num_buf) {
+			xas_store(&xas, XA_ZERO_ENTRY);
+			if (xas_error(&xas))
+				break;
+			xas_next(&xas);
+			i++;
 		}
-	}
+		xas_unlock(&xas);
+	} while (xas_nomem(&xas, GFP_KERNEL));
 
-	mr->umem = umem;
-	mr->access = access;
-	mr->page_offset = ib_umem_offset(umem);
-	mr->state = RXE_MR_STATE_VALID;
-	mr->ibmr.type = IB_MR_TYPE_USER;
+	err = xas_error(&xas);
+	if (err)
+		return err;
 
-	return 0;
+	mr->num_buf = num_buf;
 
-err_release_umem:
-	ib_umem_release(umem);
-err_out:
-	return err;
+	return 0;
 }
 
 int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
@@ -212,7 +180,6 @@ int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
 	if (err)
 		goto err1;
 
-	mr->max_buf = max_pages;
 	mr->state = RXE_MR_STATE_FREE;
 	mr->ibmr.type = IB_MR_TYPE_MEM_REG;
 
@@ -222,116 +189,100 @@ int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
 	return err;
 }
 
-static int rxe_set_page(struct ib_mr *ibmr, u64 addr)
+static int rxe_set_page(struct ib_mr *ibmr, u64 iova)
 {
 	struct rxe_mr *mr = to_rmr(ibmr);
-	struct rxe_map *map;
-	struct rxe_phys_buf *buf;
+	struct page *page = virt_to_page(iova & mr->page_mask);
+	XA_STATE(xas, &mr->pages, mr->nbuf);
+	int err;
 
 	if (unlikely(mr->nbuf == mr->num_buf))
 		return -ENOMEM;
 
-	map = mr->map[mr->nbuf / RXE_BUF_PER_MAP];
-	buf = &map->buf[mr->nbuf % RXE_BUF_PER_MAP];
+	do {
+		xas_lock(&xas);
+		xas_store(&xas, page);
+		xas_unlock(&xas);
+	} while (xas_nomem(&xas, GFP_KERNEL));
 
-	buf->addr = addr;
-	buf->size = ibmr->page_size;
-	mr->nbuf++;
+	err = xas_error(&xas);
+	if (err)
+		return err;
 
+	mr->nbuf++;
 	return 0;
 }
 
-int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
+int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sgl,
 		  int sg_nents, unsigned int *sg_offset)
 {
 	struct rxe_mr *mr = to_rmr(ibmr);
 	unsigned int page_size = mr_page_size(mr);
 
+	mr->nbuf = 0;
 	mr->page_shift = ilog2(page_size);
 	mr->page_mask = ~((u64)page_size - 1);
-	mr->page_offset = ibmr->iova & (page_size - 1);
+	mr->page_offset = mr->ibmr.iova & (page_size - 1);
 
-	mr->nbuf = 0;
-
-	return ib_sg_to_pages(ibmr, sg, sg_nents, sg_offset, rxe_set_page);
+	return ib_sg_to_pages(ibmr, sgl, sg_nents, sg_offset, rxe_set_page);
 }
 
-static void lookup_iova(struct rxe_mr *mr, u64 iova, int *m_out, int *n_out,
-			size_t *offset_out)
-{
-	size_t offset = iova - mr->ibmr.iova + mr->page_offset;
-	int			map_index;
-	int			buf_index;
-	u64			length;
-
-	if (likely(mr->page_shift)) {
-		*offset_out = offset & (mr_page_size(mr) - 1);
-		offset >>= mr->page_shift;
-		*n_out = offset & mr->map_mask;
-		*m_out = offset >> mr->map_shift;
-	} else {
-		map_index = 0;
-		buf_index = 0;
-
-		length = mr->map[map_index]->buf[buf_index].size;
-
-		while (offset >= length) {
-			offset -= length;
-			buf_index++;
-
-			if (buf_index == RXE_BUF_PER_MAP) {
-				map_index++;
-				buf_index = 0;
-			}
-			length = mr->map[map_index]->buf[buf_index].size;
-		}
-
-		*m_out = map_index;
-		*n_out = buf_index;
-		*offset_out = offset;
-	}
-}
-
-void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length)
+/*
+ * TODO: Attempting to modify the mr page map between the time
+ * a packet is received and the map is referenced as here
+ * in xa_load(&mr->pages) will cause problems. It is OK to
+ * deregister the mr since the mr reference counts will preserve
+ * it until memory accesses are complete. Currently reregister mr
+ * operations are not supported by the rxe driver but could be
+ * in the future. Invalidate followed by fast_reg mr will change
+ * the map and then the rkey so delayed packets arriving in the
+ * middle could use the wrong map entries. This isn't new but was
+ * already the case in the earlier implementation. This won't be
+ * a problem for well behaved programs which wait until all the
+ * outstanding packets for the first FMR before remapping to the
+ * second.
+ */
+static int rxe_mr_copy_xarray(struct rxe_mr *mr, void *addr,
+			      unsigned long index,
+			      unsigned int page_offset, unsigned int length,
+			      enum rxe_mr_copy_dir dir)
 {
-	size_t offset;
-	int m, n;
-	void *addr;
-
-	if (mr->state != RXE_MR_STATE_VALID) {
-		rxe_dbg_mr(mr, "Not in valid state\n");
-		addr = NULL;
-		goto out;
-	}
-
-	if (!mr->map) {
-		addr = (void *)(uintptr_t)iova;
-		goto out;
-	}
+	struct page *page;
+	unsigned int bytes;
+	void *va;
 
-	if (mr_check_range(mr, iova, length)) {
-		rxe_dbg_mr(mr, "Range violation\n");
-		addr = NULL;
-		goto out;
-	}
-
-	lookup_iova(mr, iova, &m, &n, &offset);
-
-	if (offset + length > mr->map[m]->buf[n].size) {
-		rxe_dbg_mr(mr, "Crosses page boundary\n");
-		addr = NULL;
-		goto out;
+	while (length) {
+		page = xa_load(&mr->pages, index);
+		if (WARN_ON(!page))
+			return -EINVAL;
+
+		bytes = min_t(unsigned int, length,
+				mr_page_size(mr) - page_offset);
+		va = kmap_local_page(page);
+		if (dir == RXE_FROM_MR_OBJ)
+			memcpy(addr, va + page_offset, bytes);
+		else
+			memcpy(va + page_offset, addr, bytes);
+		kunmap_local(va);
+
+		page_offset = 0;
+		addr += bytes;
+		length -= bytes;
+		index++;
 	}
 
-	addr = (void *)(uintptr_t)mr->map[m]->buf[n].addr + offset;
-
-out:
-	return addr;
+	return 0;
 }
 
+// TODO convert iova to va through xarray and then flush
 int rxe_flush_pmem_iova(struct rxe_mr *mr, u64 iova, int length)
 {
-	size_t offset;
+	unsigned int page_offset;
+	unsigned long index;
+	struct page *page;
+	int bytes;
+	int err;
+	u8 *va;
 
 	if (length == 0)
 		return 0;
@@ -339,104 +290,84 @@ int rxe_flush_pmem_iova(struct rxe_mr *mr, u64 iova, int length)
 	if (mr->ibmr.type == IB_MR_TYPE_DMA)
 		return -EFAULT;
 
-	offset = (iova - mr->ibmr.iova + mr->page_offset) & mr->page_mask;
-	while (length > 0) {
-		u8 *va;
-		int bytes;
+	err = mr_check_range(mr, iova, length);
+	if (err)
+		return err;
 
-		bytes = mr->ibmr.page_size - offset;
+	while (length > 0) {
+		page_offset = rxe_mr_iova_to_page_offset(mr, iova);
+		bytes = mr->ibmr.page_size - page_offset;
 		if (bytes > length)
 			bytes = length;
 
-		va = iova_to_vaddr(mr, iova, length);
+		index = rxe_mr_iova_to_index(mr, iova);
+		page = xa_load(&mr->pages, index);
+		if (!page)
+			return -2;
+
+		va = kmap_local_page(page);
 		if (!va)
 			return -EFAULT;
 
-		arch_wb_cache_pmem(va, bytes);
+		arch_wb_cache_pmem(va + page_offset, bytes);
 
 		length -= bytes;
 		iova += bytes;
-		offset = 0;
+		page_offset = 0;
 	}
 
 	return 0;
 }
 
-/* copy data from a range (vaddr, vaddr+length-1) to or from
- * a mr object starting at iova.
- */
+static void rxe_mr_copy_dma(struct rxe_mr *mr, u64 iova, void *addr,
+			    unsigned int page_offset, unsigned int length,
+			    enum rxe_mr_copy_dir dir)
+{
+	unsigned int bytes;
+	struct page *page;
+	u8 *va;
+
+	while (length) {
+		page = virt_to_page(iova & mr->page_mask);
+		va = kmap_local_page(page);
+		bytes = min_t(unsigned int, length, PAGE_SIZE - page_offset);
+
+		if (dir == RXE_TO_MR_OBJ)
+			memcpy(va + page_offset, addr, bytes);
+		else
+			memcpy(addr, va + page_offset, bytes);
+
+		kunmap_local(va);
+		page_offset = 0;
+		iova += bytes;
+		addr += bytes;
+		length -= bytes;
+	}
+}
+
 int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, int length,
 		enum rxe_mr_copy_dir dir)
 {
-	int			err;
-	int			bytes;
-	u8			*va;
-	struct rxe_map		**map;
-	struct rxe_phys_buf	*buf;
-	int			m;
-	int			i;
-	size_t			offset;
+	unsigned int page_offset;
+	unsigned long index;
+	int err;
 
 	if (length == 0)
 		return 0;
 
 	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
-		u8 *src, *dest;
-
-		src = (dir == RXE_TO_MR_OBJ) ? addr : ((void *)(uintptr_t)iova);
-
-		dest = (dir == RXE_TO_MR_OBJ) ? ((void *)(uintptr_t)iova) : addr;
-
-		memcpy(dest, src, length);
-
+		page_offset = iova & (PAGE_SIZE - 1);
+		rxe_mr_copy_dma(mr, iova, addr, page_offset, length, dir);
 		return 0;
 	}
 
-	WARN_ON_ONCE(!mr->map);
-
 	err = mr_check_range(mr, iova, length);
-	if (err) {
-		err = -EFAULT;
-		goto err1;
-	}
-
-	lookup_iova(mr, iova, &m, &i, &offset);
-
-	map = mr->map + m;
-	buf	= map[0]->buf + i;
-
-	while (length > 0) {
-		u8 *src, *dest;
-
-		va	= (u8 *)(uintptr_t)buf->addr + offset;
-		src = (dir == RXE_TO_MR_OBJ) ? addr : va;
-		dest = (dir == RXE_TO_MR_OBJ) ? va : addr;
-
-		bytes	= buf->size - offset;
-
-		if (bytes > length)
-			bytes = length;
-
-		memcpy(dest, src, bytes);
-
-		length	-= bytes;
-		addr	+= bytes;
-
-		offset	= 0;
-		buf++;
-		i++;
-
-		if (i == RXE_BUF_PER_MAP) {
-			i = 0;
-			map++;
-			buf = map[0]->buf;
-		}
-	}
-
-	return 0;
+	if (err)
+		return err;
 
-err1:
-	return err;
+	page_offset = rxe_mr_iova_to_page_offset(mr, iova);
+	index = rxe_mr_iova_to_index(mr, iova);
+	return rxe_mr_copy_xarray(mr, addr, index, page_offset, length, dir);
 }
 
 /* copy data in or out of a wqe, i.e. sg list
@@ -508,7 +439,6 @@ int copy_data(
 
 		if (bytes > 0) {
 			iova = sge->addr + offset;
-
 			err = rxe_mr_copy(mr, iova, addr, bytes, dir);
 			if (err)
 				goto err2;
@@ -537,20 +467,47 @@ int copy_data(
 
 static DEFINE_SPINLOCK(atomic_ops_lock);
 
+/*
+ * Returns:
+ *	 0	on success
+ *	-1	address is misaligned
+ *	-2	access violations
+ */
 int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 			u64 compare, u64 swap_add, u64 *orig_val)
 {
-	u64 *vaddr = iova_to_vaddr(mr, iova, sizeof(u64));
+	unsigned int page_offset;
+	struct page *page;
 	u64 value;
+	u64 *va;
 
-	/* needs to match rxe_resp.c */
-	if (mr->state != RXE_MR_STATE_VALID || !vaddr)
-		return -EFAULT;
-	if ((uintptr_t)vaddr & 0x7)
-		return -EINVAL;
+	if (unlikely(mr->state != RXE_MR_STATE_VALID))
+		return -2;
+
+	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
+		page_offset = iova & (PAGE_SIZE - 1);
+		page = virt_to_page(iova & PAGE_MASK);
+	} else {
+		unsigned long index;
+		int err;
+
+		err = mr_check_range(mr, iova, 8);
+		if (err)
+			return err;
+		page_offset = rxe_mr_iova_to_page_offset(mr, iova);
+		index = rxe_mr_iova_to_index(mr, iova);
+		page = xa_load(&mr->pages, index);
+		if (!page)
+			return -2;
+	}
+
+	if (unlikely(page_offset & 0x7))
+		return -1;
+
+	va = kmap_local_page(page);
 
 	spin_lock_bh(&atomic_ops_lock);
-	value = *orig_val = *vaddr;
+	*orig_val = value = va[page_offset >> 3];
 
 	if (opcode == IB_OPCODE_RC_COMPARE_SWAP) {
 		if (value == compare)
@@ -559,9 +516,11 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 		value += swap_add;
 	}
 
-	*vaddr = value;
+	va[page_offset >> 3] = value;
 	spin_unlock_bh(&atomic_ops_lock);
 
+	kunmap_local(va);
+
 	return 0;
 }
 
@@ -580,9 +539,11 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
 {
 #if defined CONFIG_64BIT
-	u64 *vaddr;
-	u64 value;
+	unsigned int page_offset;
+	struct page *page;
 	unsigned int length = 8;
+	u64 value;
+	u64 *va;
 
 	/* See IBA oA19-28 */
 	if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
@@ -590,23 +551,38 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr)
 		return -2;
 	}
 
+	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
+		page_offset = iova & (PAGE_SIZE - 1);
+		page = virt_to_page(iova & PAGE_MASK);
+	} else {
+		unsigned long index;
+		int err;
+
+		/* See IBA oA19-28 */
+		err = mr_check_range(mr, iova, length);
+		if (unlikely(err)) {
+			rxe_dbg_mr(mr, "iova out of range");
+			return -2;
+		}
+		page_offset = rxe_mr_iova_to_page_offset(mr, iova);
+		index = rxe_mr_iova_to_index(mr, iova);
+		page = xa_load(&mr->pages, index);
+		if (WARN_ON(!page))
+			return -2;
+	}
+
 	/* See IBA A19.4.2 */
-	if (unlikely((uintptr_t)vaddr & 0x7 || iova & 0x7)) {
+	if (iova & 0x7) {
 		rxe_dbg_mr(mr, "misaligned address");
 		return -1;
 	}
 
-	vaddr = iova_to_vaddr(mr, iova, length);
-	if (unlikely(!vaddr)) {
-		rxe_dbg_mr(mr, "iova out of range");
-		return -2;
-	}
-
-	/* this makes no sense. What of payload is not 8? */
+	va = kmap_local_page(page);
 	memcpy(&value, addr, length);
 
 	/* Do atomic write after all prior operations have completed */
-	smp_store_release(vaddr, value);
+	smp_store_release(&va[page_offset >> 3], value);
+	kunmap_local(va);
 
 	return 0;
 #else
@@ -648,12 +624,6 @@ int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
 	return 0;
 }
 
-/* (1) find the mr corresponding to lkey/rkey
- *     depending on lookup_type
- * (2) verify that the (qp) pd matches the mr pd
- * (3) verify that the mr can support the requested access
- * (4) verify that mr state is valid
- */
 struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
 			 enum rxe_mr_lookup_type type)
 {
@@ -774,15 +744,8 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
 void rxe_mr_cleanup(struct rxe_pool_elem *elem)
 {
 	struct rxe_mr *mr = container_of(elem, typeof(*mr), elem);
-	int i;
 
 	rxe_put(mr_pd(mr));
 	ib_umem_release(mr->umem);
-
-	if (mr->map) {
-		for (i = 0; i < mr->num_map; i++)
-			kfree(mr->map[i]);
-
-		kfree(mr->map);
-	}
+	xa_destroy(&mr->pages);
 }
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 1083cda22c65..b0ff36f6dc4a 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -744,7 +744,7 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
 					  atmeth_comp(pkt),
 					  atmeth_swap_add(pkt),
 					  &res->atomic.orig_val);
-		if (err == -EINVAL)
+		if (err == -1)
 			return RESPST_ERR_MISALIGNED_ATOMIC;
 		else if (err)
 			return RESPST_ERR_RKEY_VIOLATION;
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index bfc94caaeec5..64551cf354a6 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -283,17 +283,6 @@ enum rxe_mr_lookup_type {
 	RXE_LOOKUP_REMOTE,
 };
 
-#define RXE_BUF_PER_MAP		(PAGE_SIZE / sizeof(struct rxe_phys_buf))
-
-struct rxe_phys_buf {
-	u64      addr;
-	u64      size;
-};
-
-struct rxe_map {
-	struct rxe_phys_buf	buf[RXE_BUF_PER_MAP];
-};
-
 static inline int rkey_is_mw(u32 rkey)
 {
 	u32 index = rkey >> 8;
@@ -311,22 +300,16 @@ struct rxe_mr {
 	u32			rkey;
 	enum rxe_mr_state	state;
 	int			access;
+	atomic_t		num_mw;
 
 	unsigned int		page_offset;
 	unsigned int		page_shift;
 	u64			page_mask;
-	int			map_shift;
-	int			map_mask;
 
 	u32			num_buf;
 	u32			nbuf;
 
-	u32			max_buf;
-	u32			num_map;
-
-	atomic_t		num_mw;
-
-	struct rxe_map		**map;
+	struct xarray		pages;
 };
 
 static inline unsigned int mr_page_size(struct rxe_mr *mr)
-- 
2.37.2

Re: [PATCH for-next v3 6/6] RDMA/rxe: Replace rxe_map and rxe_phys_buf by xarray

[lizhijian]

On 14/01/2023 07:27, Bob Pearson wrote:
> Replace struct rxe-phys_buf and struct rxe_map by struct xarray
> in rxe_verbs.h. This allows using rcu locking on reads for
> the memory maps stored in each mr.
> 
> This is based off of a sketch of a patch from Jason Gunthorpe in the
> link below. Some changes were needed to make this work. It applies
> cleanly to the current for-next and passes the pyverbs, perftest
> and the same blktests test cases which run today.
> 
> Link:https://lore.kernel.org/linux-rdma/Y3gvZr6%2FNCii9Avy@nvidia.com/
> Co-developed-by: Jason Gunthorpe<jgg@nvidia.com>
> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_loc.h   |   1 -
>   drivers/infiniband/sw/rxe/rxe_mr.c    | 533 ++++++++++++--------------
>   drivers/infiniband/sw/rxe/rxe_resp.c  |   2 +-
>   drivers/infiniband/sw/rxe/rxe_verbs.h |  21 +-
>   4 files changed, 251 insertions(+), 306 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index fd70c71a9e4e..0cf78f9bb27c 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -71,7 +71,6 @@ int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
>   	      void *addr, int length, enum rxe_mr_copy_dir dir);
>   int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
>   		  int sg_nents, unsigned int *sg_offset);
> -void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
>   int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>   			u64 compare, u64 swap_add, u64 *orig_val);
>   int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index fd5537ee7f04..e4634279080a 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
[snip...]

> -static bool is_pmem_page(struct page *pg)
> +static unsigned long rxe_mr_iova_to_index(struct rxe_mr *mr, u64 iova)
>   {
> -	unsigned long paddr = page_to_phys(pg);
> +	return (iova >> mr->page_shift) - (mr->ibmr.iova >> mr->page_shift);
> +}
>   
> -	return REGION_INTERSECTS ==
> -	       region_intersects(paddr, PAGE_SIZE, IORESOURCE_MEM,
> -				 IORES_DESC_PERSISTENT_MEMORY);

[snip...]

> +	rxe_mr_init(access, mr);
> +
>   	umem = ib_umem_get(&rxe->ib_dev, start, length, access);
>   	if (IS_ERR(umem)) {
>   		rxe_dbg_mr(mr, "Unable to pin memory region err = %d\n",
>   			(int)PTR_ERR(umem));
> -		err = PTR_ERR(umem);
> -		goto err_out;
> +		return PTR_ERR(umem);
>   	}
>   
> -	num_buf = ib_umem_num_pages(umem);
> -
> -	rxe_mr_init(access, mr);
> -
> -	err = rxe_mr_alloc(mr, num_buf);
> +	err = rxe_mr_fill_pages_from_sgt(mr, &umem->sgt_append.sgt);
>   	if (err) {
> -		rxe_dbg_mr(mr, "Unable to allocate memory for map\n");
> -		goto err_release_umem;
> +		ib_umem_release(umem);
> +		return err;
>   	}
>   
> -	num_buf			= 0;
> -	map = mr->map;
> -	if (length > 0) {
> -		bool persistent_access = access & IB_ACCESS_FLUSH_PERSISTENT;
> -
> -		buf = map[0]->buf;
> -		for_each_sgtable_page (&umem->sgt_append.sgt, &sg_iter, 0) {
> -			struct page *pg = sg_page_iter_page(&sg_iter);
> +	mr->umem = umem;
> +	mr->ibmr.type = IB_MR_TYPE_USER;
> +	mr->state = RXE_MR_STATE_VALID;
>   
> -			if (persistent_access && !is_pmem_page(pg)) {
> -				rxe_dbg_mr(mr, "Unable to register persistent access to non-pmem device\n");
> -				err = -EINVAL;
> -				goto err_release_umem;
> -			}

I read you removed is_pmem_page and its checking, but there is no description about this.
IMO, it's required by IBA spec.

Thanks
Zhijian

Re: [PATCH for-next v3 6/6] RDMA/rxe: Replace rxe_map and rxe_phys_buf by xarray

[Bob Pearson]

On 1/15/23 20:21, lizhijian@fujitsu.com wrote:
> 
> 
> On 14/01/2023 07:27, Bob Pearson wrote:
>> Replace struct rxe-phys_buf and struct rxe_map by struct xarray
>> in rxe_verbs.h. This allows using rcu locking on reads for
>> the memory maps stored in each mr.
>>
>> This is based off of a sketch of a patch from Jason Gunthorpe in the
>> link below. Some changes were needed to make this work. It applies
>> cleanly to the current for-next and passes the pyverbs, perftest
>> and the same blktests test cases which run today.
>>
>> Link:https://lore.kernel.org/linux-rdma/Y3gvZr6%2FNCii9Avy@nvidia.com/
>> Co-developed-by: Jason Gunthorpe<jgg@nvidia.com>
>> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_loc.h   |   1 -
>>   drivers/infiniband/sw/rxe/rxe_mr.c    | 533 ++++++++++++--------------
>>   drivers/infiniband/sw/rxe/rxe_resp.c  |   2 +-
>>   drivers/infiniband/sw/rxe/rxe_verbs.h |  21 +-
>>   4 files changed, 251 insertions(+), 306 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
>> index fd70c71a9e4e..0cf78f9bb27c 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
>> @@ -71,7 +71,6 @@ int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
>>   	      void *addr, int length, enum rxe_mr_copy_dir dir);
>>   int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
>>   		  int sg_nents, unsigned int *sg_offset);
>> -void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length);
>>   int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>>   			u64 compare, u64 swap_add, u64 *orig_val);
>>   int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr);
>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
>> index fd5537ee7f04..e4634279080a 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> [snip...]
> 
>> -static bool is_pmem_page(struct page *pg)
>> +static unsigned long rxe_mr_iova_to_index(struct rxe_mr *mr, u64 iova)
>>   {
>> -	unsigned long paddr = page_to_phys(pg);
>> +	return (iova >> mr->page_shift) - (mr->ibmr.iova >> mr->page_shift);
>> +}
>>   
>> -	return REGION_INTERSECTS ==
>> -	       region_intersects(paddr, PAGE_SIZE, IORESOURCE_MEM,
>> -				 IORES_DESC_PERSISTENT_MEMORY);
> 
> [snip...]
> 
>> +	rxe_mr_init(access, mr);
>> +
>>   	umem = ib_umem_get(&rxe->ib_dev, start, length, access);
>>   	if (IS_ERR(umem)) {
>>   		rxe_dbg_mr(mr, "Unable to pin memory region err = %d\n",
>>   			(int)PTR_ERR(umem));
>> -		err = PTR_ERR(umem);
>> -		goto err_out;
>> +		return PTR_ERR(umem);
>>   	}
>>   
>> -	num_buf = ib_umem_num_pages(umem);
>> -
>> -	rxe_mr_init(access, mr);
>> -
>> -	err = rxe_mr_alloc(mr, num_buf);
>> +	err = rxe_mr_fill_pages_from_sgt(mr, &umem->sgt_append.sgt);
>>   	if (err) {
>> -		rxe_dbg_mr(mr, "Unable to allocate memory for map\n");
>> -		goto err_release_umem;
>> +		ib_umem_release(umem);
>> +		return err;
>>   	}
>>   
>> -	num_buf			= 0;
>> -	map = mr->map;
>> -	if (length > 0) {
>> -		bool persistent_access = access & IB_ACCESS_FLUSH_PERSISTENT;
>> -
>> -		buf = map[0]->buf;
>> -		for_each_sgtable_page (&umem->sgt_append.sgt, &sg_iter, 0) {
>> -			struct page *pg = sg_page_iter_page(&sg_iter);
>> +	mr->umem = umem;
>> +	mr->ibmr.type = IB_MR_TYPE_USER;
>> +	mr->state = RXE_MR_STATE_VALID;
>>   
>> -			if (persistent_access && !is_pmem_page(pg)) {
>> -				rxe_dbg_mr(mr, "Unable to register persistent access to non-pmem device\n");
>> -				err = -EINVAL;
>> -				goto err_release_umem;
>> -			}
> 
> I read you removed is_pmem_page and its checking, but there is no description about this.
> IMO, it's required by IBA spec.
> 
> Thanks
> Zhijian

Zhijian,

It was dropped by accident. I just posted an update with the calls put back. Please take a
look and if possible can you test to see if it doesn't regress your pmem changes. I have no
way to test pmem. Also look at a comment I added to the pmem flush call. I am suspicious of
the smp_store_release() call based on the original comment.

Regards,

Bob