netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH rdma-next 0/4] Rely on firmware to get special mkeys
@ 2023-01-04  8:11 Leon Romanovsky
  2023-01-04  8:11 ` [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying " Leon Romanovsky
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04  8:11 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, Eric Dumazet, Jakub Kicinski, linux-kernel,
	linux-rdma, Michael Guralnik, netdev, Or Har-Toov, Paolo Abeni,
	Saeed Mahameed

From: Leon Romanovsky <leonro@nvidia.com>

This series from Or extends mlx5 driver to rely on firmware to get
special mkey values.

Thanks

Or Har-Toov (4):
  net/mlx5: Expose bits for querying special mkeys
  net/mlx5: Change define name for 0x100 lkey value
  net/mlx5: Use query_special_contexts for mkeys
  RDMA/mlx5: Use query_special_contexts for mkeys

 drivers/infiniband/hw/mlx5/cmd.c              | 41 ++++++++++---------
 drivers/infiniband/hw/mlx5/cmd.h              |  3 +-
 drivers/infiniband/hw/mlx5/main.c             | 10 ++---
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |  9 +++-
 drivers/infiniband/hw/mlx5/odp.c              | 27 ++++--------
 drivers/infiniband/hw/mlx5/srq.c              |  2 +-
 drivers/infiniband/hw/mlx5/wr.c               |  2 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  3 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    | 27 ++++++++++++
 include/linux/mlx5/driver.h                   |  1 +
 include/linux/mlx5/mlx5_ifc.h                 | 10 ++++-
 include/linux/mlx5/qp.h                       |  2 +-
 12 files changed, 85 insertions(+), 52 deletions(-)

-- 
2.38.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying special mkeys
  2023-01-04  8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
@ 2023-01-04  8:11 ` Leon Romanovsky
  2023-01-04  8:11 ` [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value Leon Romanovsky
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04  8:11 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, Eric Dumazet, Jakub Kicinski, linux-rdma,
	Michael Guralnik, netdev, Paolo Abeni, Saeed Mahameed

From: Or Har-Toov <ohartoov@nvidia.com>

Add needed HW bits to query the values of all special mkeys.

Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/mlx5/mlx5_ifc.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index f3d1c62c98dd..a2ed927c8f9f 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -1479,7 +1479,9 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 	u8         relaxed_ordering_write[0x1];
 	u8         relaxed_ordering_read[0x1];
 	u8         log_max_mkey[0x6];
-	u8         reserved_at_f0[0x8];
+	u8         reserved_at_f0[0x6];
+	u8	   terminate_scatter_list_mkey[0x1];
+	u8	   repeated_mkey[0x1];
 	u8         dump_fill_mkey[0x1];
 	u8         reserved_at_f9[0x2];
 	u8         fast_teardown[0x1];
@@ -5197,7 +5199,11 @@ struct mlx5_ifc_query_special_contexts_out_bits {
 
 	u8         null_mkey[0x20];
 
-	u8         reserved_at_a0[0x60];
+	u8	   terminate_scatter_list_mkey[0x20];
+
+	u8	   repeated_mkey[0x20];
+
+	u8         reserved_at_a0[0x20];
 };
 
 struct mlx5_ifc_query_special_contexts_in_bits {
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value
  2023-01-04  8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
  2023-01-04  8:11 ` [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying " Leon Romanovsky
@ 2023-01-04  8:11 ` Leon Romanovsky
  2023-01-04  8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
  2023-01-04  8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
  3 siblings, 0 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04  8:11 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, Eric Dumazet, Jakub Kicinski, linux-rdma,
	Michael Guralnik, netdev, Paolo Abeni, Saeed Mahameed

From: Or Har-Toov <ohartoov@nvidia.com>

Change define of 0x100 lkey value from MLX5_INVALID_LKEY to be
MLX5_TERMINATE_SCATTER_LIST_LKEY as 0x100 is the value of
terminate_scatter_list_mkey.

Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/odp.c                  | 10 +++++-----
 drivers/infiniband/hw/mlx5/srq.c                  |  2 +-
 drivers/infiniband/hw/mlx5/wr.c                   |  2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |  3 ++-
 include/linux/mlx5/qp.h                           |  2 +-
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index e6e021af6aa9..b4ebeadce67c 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -986,7 +986,7 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
 {
 	int ret = 0, npages = 0;
 	u64 io_virt;
-	u32 key;
+	__be32 key;
 	u32 byte_count;
 	size_t bcnt;
 	int inline_segment;
@@ -1000,7 +1000,7 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
 		struct mlx5_wqe_data_seg *dseg = wqe;
 
 		io_virt = be64_to_cpu(dseg->addr);
-		key = be32_to_cpu(dseg->lkey);
+		key = dseg->lkey;
 		byte_count = be32_to_cpu(dseg->byte_count);
 		inline_segment = !!(byte_count &  MLX5_INLINE_SEG);
 		bcnt	       = byte_count & ~MLX5_INLINE_SEG;
@@ -1014,8 +1014,8 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
 		}
 
 		/* receive WQE end of sg list. */
-		if (receive_queue && bcnt == 0 && key == MLX5_INVALID_LKEY &&
-		    io_virt == 0)
+		if (receive_queue && bcnt == 0 &&
+		    key == MLX5_TERMINATE_SCATTER_LIST_LKEY && io_virt == 0)
 			break;
 
 		if (!inline_segment && total_wqe_bytes) {
@@ -1034,7 +1034,7 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
 			continue;
 		}
 
-		ret = pagefault_single_data_segment(dev, NULL, key,
+		ret = pagefault_single_data_segment(dev, NULL, be32_to_cpu(key),
 						    io_virt, bcnt,
 						    &pfault->bytes_committed,
 						    bytes_mapped);
diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index 757756c50cc6..bcceb14a07f9 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -447,7 +447,7 @@ int mlx5_ib_post_srq_recv(struct ib_srq *ibsrq, const struct ib_recv_wr *wr,
 
 		if (i < srq->msrq.max_avail_gather) {
 			scat[i].byte_count = 0;
-			scat[i].lkey       = cpu_to_be32(MLX5_INVALID_LKEY);
+			scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
 			scat[i].addr       = 0;
 		}
 	}
diff --git a/drivers/infiniband/hw/mlx5/wr.c b/drivers/infiniband/hw/mlx5/wr.c
index 855f3f4fefad..bc44551493e2 100644
--- a/drivers/infiniband/hw/mlx5/wr.c
+++ b/drivers/infiniband/hw/mlx5/wr.c
@@ -1252,7 +1252,7 @@ int mlx5_ib_post_recv(struct ib_qp *ibqp, const struct ib_recv_wr *wr,
 
 		if (i < qp->rq.max_gs) {
 			scat[i].byte_count = 0;
-			scat[i].lkey       = cpu_to_be32(MLX5_INVALID_LKEY);
+			scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
 			scat[i].addr       = 0;
 		}
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 8d36e2de53a9..c76f15505a76 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -825,7 +825,8 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
 			/* check if num_frags is not a pow of two */
 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
 				wqe->data[f].byte_count = 0;
-				wqe->data[f].lkey = cpu_to_be32(MLX5_INVALID_LKEY);
+				wqe->data[f].lkey =
+					MLX5_TERMINATE_SCATTER_LIST_LKEY;
 				wqe->data[f].addr = 0;
 			}
 		}
diff --git a/include/linux/mlx5/qp.h b/include/linux/mlx5/qp.h
index 4657d5c54abe..df55fbb65717 100644
--- a/include/linux/mlx5/qp.h
+++ b/include/linux/mlx5/qp.h
@@ -36,7 +36,7 @@
 #include <linux/mlx5/device.h>
 #include <linux/mlx5/driver.h>
 
-#define MLX5_INVALID_LKEY	0x100
+#define MLX5_TERMINATE_SCATTER_LIST_LKEY cpu_to_be32(0x100)
 /* UMR (3 WQE_BB's) + SIG (3 WQE_BB's) + PSV (mem) + PSV (wire) */
 #define MLX5_SIG_WQE_SIZE	(MLX5_SEND_WQE_BB * 8)
 #define MLX5_DIF_SIZE		8
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
  2023-01-04  8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
  2023-01-04  8:11 ` [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying " Leon Romanovsky
  2023-01-04  8:11 ` [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value Leon Romanovsky
@ 2023-01-04  8:11 ` Leon Romanovsky
  2023-01-06  0:58   ` Saeed Mahameed
  2023-01-04  8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
  3 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04  8:11 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, Eric Dumazet, Jakub Kicinski, linux-rdma,
	Michael Guralnik, netdev, Paolo Abeni, Saeed Mahameed

From: Or Har-Toov <ohartoov@nvidia.com>

Using query_sepcial_contexts in order to get the correct value of
terminate_scatter_list_mkey, as FW will change it in some configurations.
This is done one time when the device is loading and the value is being
saved in the device context.

Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
 include/linux/mlx5/driver.h                   |  1 +
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index c76f15505a76..33d7a7095988 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
 				wqe->data[f].byte_count = 0;
 				wqe->data[f].lkey =
-					MLX5_TERMINATE_SCATTER_LIST_LKEY;
+					mdev->terminate_scatter_list_mkey;
 				wqe->data[f].addr = 0;
 			}
 		}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 7f5db13e3550..d39d758744a0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
 	return 0;
 }
 
+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
+	int err;
+
+	MLX5_SET(query_special_contexts_in, in, opcode,
+		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
+	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
+	if (err)
+		return err;
+
+	if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+		dev->terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
+	return 0;
+}
+
 static int mlx5_load(struct mlx5_core_dev *dev)
 {
 	int err;
@@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
 	mlx5_events_start(dev);
 	mlx5_pagealloc_start(dev);
 
+	err = mlx5_get_terminate_scatter_list_mkey(dev);
+	if (err) {
+		mlx5_core_err(dev, "Failed to query special contexts\n");
+		goto err_irq_table;
+	}
 	err = mlx5_irq_table_create(dev);
 	if (err) {
 		mlx5_core_err(dev, "Failed to alloc IRQs\n");
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index d476255c9a3f..5f2c4038d638 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -801,6 +801,7 @@ struct mlx5_core_dev {
 	struct mlx5_rsc_dump    *rsc_dump;
 	u32                      vsc_addr;
 	struct mlx5_hv_vhca	*hv_vhca;
+	__be32			terminate_scatter_list_mkey;
 };
 
 struct mlx5_db {
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04  8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
                   ` (2 preceding siblings ...)
  2023-01-04  8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
@ 2023-01-04  8:11 ` Leon Romanovsky
  2023-01-04 13:03   ` Jason Gunthorpe
  3 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04  8:11 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

From: Or Har-Toov <ohartoov@nvidia.com>

Using query_sepcial_contexts to get the correct value of mkeys such as
null_mkey, terminate_scatter_list_mkey and dump_fill_mkey, as FW will
change them in some configurations.

Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/cmd.c     | 41 ++++++++++++++--------------
 drivers/infiniband/hw/mlx5/cmd.h     |  3 +-
 drivers/infiniband/hw/mlx5/main.c    | 10 +++----
 drivers/infiniband/hw/mlx5/mlx5_ib.h |  9 +++++-
 drivers/infiniband/hw/mlx5/odp.c     | 21 ++++----------
 drivers/infiniband/hw/mlx5/srq.c     |  2 +-
 drivers/infiniband/hw/mlx5/wr.c      |  2 +-
 7 files changed, 43 insertions(+), 45 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/cmd.c b/drivers/infiniband/hw/mlx5/cmd.c
index ff3742b0460a..f2e465aadd8e 100644
--- a/drivers/infiniband/hw/mlx5/cmd.c
+++ b/drivers/infiniband/hw/mlx5/cmd.c
@@ -5,7 +5,7 @@
 
 #include "cmd.h"
 
-int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey)
+int mlx5_cmd_query_special_mkeys(struct mlx5_ib_dev *dev)
 {
 	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
 	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
@@ -13,26 +13,27 @@ int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey)
 
 	MLX5_SET(query_special_contexts_in, in, opcode,
 		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*mkey = MLX5_GET(query_special_contexts_out, out,
-				 dump_fill_mkey);
-	return err;
-}
-
-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;
+	return 0;
 }
 
 int mlx5_cmd_query_cong_params(struct mlx5_core_dev *dev, int cong_point,
diff --git a/drivers/infiniband/hw/mlx5/cmd.h b/drivers/infiniband/hw/mlx5/cmd.h
index ee46638db5de..79ccd7dfa67a 100644
--- a/drivers/infiniband/hw/mlx5/cmd.h
+++ b/drivers/infiniband/hw/mlx5/cmd.h
@@ -37,8 +37,7 @@
 #include <linux/kernel.h>
 #include <linux/mlx5/driver.h>
 
-int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey);
-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey);
+int mlx5_cmd_query_special_mkeys(struct mlx5_ib_dev *dev);
 int mlx5_cmd_query_cong_params(struct mlx5_core_dev *dev, int cong_point,
 			       void *out);
 int mlx5_cmd_dealloc_pd(struct mlx5_core_dev *dev, u32 pdn, u16 uid);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index c669ef6e47e7..12e8bf99a40e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -1756,13 +1756,9 @@ static int set_ucontext_resp(struct ib_ucontext *uctx,
 	struct mlx5_ib_dev *dev = to_mdev(ibdev);
 	struct mlx5_ib_ucontext *context = to_mucontext(uctx);
 	struct mlx5_bfreg_info *bfregi = &context->bfregi;
-	int err;
 
 	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey)) {
-		err = mlx5_cmd_dump_fill_mkey(dev->mdev,
-					      &resp->dump_fill_mkey);
-		if (err)
-			return err;
+		resp->dump_fill_mkey = dev->mkeys.dump_fill_mkey;
 		resp->comp_mask |=
 			MLX5_IB_ALLOC_UCONTEXT_RESP_MASK_DUMP_FILL_MKEY;
 	}
@@ -3634,6 +3630,10 @@ static int mlx5_ib_stage_init_init(struct mlx5_ib_dev *dev)
 		dev->port[i].roce.last_port_state = IB_PORT_DOWN;
 	}
 
+	err = mlx5_cmd_query_special_mkeys(dev);
+	if (err)
+		return err;
+
 	err = mlx5_ib_init_multiport_master(dev);
 	if (err)
 		return err;
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 093aa69af0ef..42a0d1c8d85c 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1051,6 +1051,13 @@ struct mlx5_port_caps {
 	u8 ext_port_cap;
 };
 
+
+struct mlx5_special_mkeys {
+	u32 dump_fill_mkey;
+	__be32 null_mkey;
+	__be32 terminate_scatter_list_mkey;
+};
+
 struct mlx5_ib_dev {
 	struct ib_device		ib_dev;
 	struct mlx5_core_dev		*mdev;
@@ -1081,7 +1088,6 @@ struct mlx5_ib_dev {
 
 	struct xarray		odp_mkeys;
 
-	u32			null_mkey;
 	struct mlx5_ib_flow_db	*flow_db;
 	/* protect resources needed as part of reset flow */
 	spinlock_t		reset_flow_resource_lock;
@@ -1110,6 +1116,7 @@ struct mlx5_ib_dev {
 	struct mlx5_port_caps port_caps[MLX5_MAX_PORTS];
 	u16 pkey_table_len;
 	u8 lag_ports;
+	struct mlx5_special_mkeys mkeys;
 };
 
 static inline struct mlx5_ib_cq *to_mibcq(struct mlx5_core_cq *mcq)
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index b4ebeadce67c..4998eaeadcbb 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -104,7 +104,7 @@ static void populate_klm(struct mlx5_klm *pklm, size_t idx, size_t nentries,
 	if (flags & MLX5_IB_UPD_XLT_ZAP) {
 		for (; pklm != end; pklm++, idx++) {
 			pklm->bcount = cpu_to_be32(MLX5_IMR_MTT_SIZE);
-			pklm->key = cpu_to_be32(mr_to_mdev(imr)->null_mkey);
+			pklm->key = mr_to_mdev(imr)->mkeys.null_mkey;
 			pklm->va = 0;
 		}
 		return;
@@ -137,7 +137,7 @@ static void populate_klm(struct mlx5_klm *pklm, size_t idx, size_t nentries,
 			pklm->key = cpu_to_be32(mtt->ibmr.lkey);
 			pklm->va = cpu_to_be64(idx * MLX5_IMR_MTT_SIZE);
 		} else {
-			pklm->key = cpu_to_be32(mr_to_mdev(imr)->null_mkey);
+			pklm->key = mr_to_mdev(imr)->mkeys.null_mkey;
 			pklm->va = 0;
 		}
 	}
@@ -1015,7 +1015,8 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
 
 		/* receive WQE end of sg list. */
 		if (receive_queue && bcnt == 0 &&
-		    key == MLX5_TERMINATE_SCATTER_LIST_LKEY && io_virt == 0)
+		    key == dev->mkeys.terminate_scatter_list_mkey &&
+		    io_virt == 0)
 			break;
 
 		if (!inline_segment && total_wqe_bytes) {
@@ -1615,25 +1616,15 @@ static const struct ib_device_ops mlx5_ib_dev_odp_ops = {
 
 int mlx5_ib_odp_init_one(struct mlx5_ib_dev *dev)
 {
-	int ret = 0;
-
 	internal_fill_odp_caps(dev);
 
 	if (!(dev->odp_caps.general_caps & IB_ODP_SUPPORT))
-		return ret;
+		return 0;
 
 	ib_set_device_ops(&dev->ib_dev, &mlx5_ib_dev_odp_ops);
 
-	if (dev->odp_caps.general_caps & IB_ODP_SUPPORT_IMPLICIT) {
-		ret = mlx5_cmd_null_mkey(dev->mdev, &dev->null_mkey);
-		if (ret) {
-			mlx5_ib_err(dev, "Error getting null_mkey %d\n", ret);
-			return ret;
-		}
-	}
-
 	mutex_init(&dev->odp_eq_mutex);
-	return ret;
+	return 0;
 }
 
 void mlx5_ib_odp_cleanup_one(struct mlx5_ib_dev *dev)
diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index bcceb14a07f9..32c6643d0f7a 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -447,7 +447,7 @@ int mlx5_ib_post_srq_recv(struct ib_srq *ibsrq, const struct ib_recv_wr *wr,
 
 		if (i < srq->msrq.max_avail_gather) {
 			scat[i].byte_count = 0;
-			scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
+			scat[i].lkey = dev->mkeys.terminate_scatter_list_mkey;
 			scat[i].addr       = 0;
 		}
 	}
diff --git a/drivers/infiniband/hw/mlx5/wr.c b/drivers/infiniband/hw/mlx5/wr.c
index bc44551493e2..df1d1b0a3ef7 100644
--- a/drivers/infiniband/hw/mlx5/wr.c
+++ b/drivers/infiniband/hw/mlx5/wr.c
@@ -1252,7 +1252,7 @@ int mlx5_ib_post_recv(struct ib_qp *ibqp, const struct ib_recv_wr *wr,
 
 		if (i < qp->rq.max_gs) {
 			scat[i].byte_count = 0;
-			scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
+			scat[i].lkey = dev->mkeys.terminate_scatter_list_mkey;
 			scat[i].addr       = 0;
 		}
 
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04  8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
@ 2023-01-04 13:03   ` Jason Gunthorpe
  2023-01-04 13:09     ` Leon Romanovsky
  0 siblings, 1 reply; 18+ messages in thread
From: Jason Gunthorpe @ 2023-01-04 13:03 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> -{
> -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> -	int err;
> +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> +	if (err)
> +		return err;
>  
> -	MLX5_SET(query_special_contexts_in, in, opcode,
> -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> -	if (!err)
> -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> -				      null_mkey);
> -	return err;
> +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> +						     out, dump_fill_mkey);
> +
> +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> +		dev->mkeys.null_mkey = cpu_to_be32(
> +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> +
> +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> +		dev->mkeys.terminate_scatter_list_mkey =
> +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> +					     terminate_scatter_list_mkey));
> +		return 0;
> +	}
> +	dev->mkeys.terminate_scatter_list_mkey =
> +		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it
here?

Jason

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04 13:03   ` Jason Gunthorpe
@ 2023-01-04 13:09     ` Leon Romanovsky
  2023-01-04 13:13       ` Jason Gunthorpe
  0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 13:09 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > -{
> > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > -	int err;
> > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > +	if (err)
> > +		return err;
> >  
> > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > -	if (!err)
> > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > -				      null_mkey);
> > -	return err;
> > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > +						     out, dump_fill_mkey);
> > +
> > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > +		dev->mkeys.null_mkey = cpu_to_be32(
> > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > +
> > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > +		dev->mkeys.terminate_scatter_list_mkey =
> > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > +					     terminate_scatter_list_mkey));
> > +		return 0;
> > +	}
> > +	dev->mkeys.terminate_scatter_list_mkey =
> > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> 
> This is already stored in the core dev, why are you recalculating it
> here?

It is not recalculating but setting default value. In core dev, we will
have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
is true.

Thanks

> 
> Jason

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04 13:09     ` Leon Romanovsky
@ 2023-01-04 13:13       ` Jason Gunthorpe
  2023-01-04 13:55         ` Leon Romanovsky
  0 siblings, 1 reply; 18+ messages in thread
From: Jason Gunthorpe @ 2023-01-04 13:13 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > -{
> > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > -	int err;
> > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > +	if (err)
> > > +		return err;
> > >  
> > > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > -	if (!err)
> > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > -				      null_mkey);
> > > -	return err;
> > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > +						     out, dump_fill_mkey);
> > > +
> > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > +		dev->mkeys.null_mkey = cpu_to_be32(
> > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > +
> > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > +		dev->mkeys.terminate_scatter_list_mkey =
> > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > +					     terminate_scatter_list_mkey));
> > > +		return 0;
> > > +	}
> > > +	dev->mkeys.terminate_scatter_list_mkey =
> > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > 
> > This is already stored in the core dev, why are you recalculating it
> > here?
> 
> It is not recalculating but setting default value. In core dev, we will
> have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> is true.

No, it has the identical code:

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Jason

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04 13:13       ` Jason Gunthorpe
@ 2023-01-04 13:55         ` Leon Romanovsky
  2023-01-04 13:56           ` Jason Gunthorpe
  0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 13:55 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > -{
> > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > -	int err;
> > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > +	if (err)
> > > > +		return err;
> > > >  
> > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > -	if (!err)
> > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > -				      null_mkey);
> > > > -	return err;
> > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > +						     out, dump_fill_mkey);
> > > > +
> > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > +		dev->mkeys.null_mkey = cpu_to_be32(
> > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > +
> > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > +		dev->mkeys.terminate_scatter_list_mkey =
> > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > +					     terminate_scatter_list_mkey));
> > > > +		return 0;
> > > > +	}
> > > > +	dev->mkeys.terminate_scatter_list_mkey =
> > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > 
> > > This is already stored in the core dev, why are you recalculating it
> > > here?
> > 
> > It is not recalculating but setting default value. In core dev, we will
> > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > is true.
> 
> No, it has the identical code:
> 
> +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> +{
> +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> +               dev->terminate_scatter_list_mkey =
> +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> +                                            terminate_scatter_list_mkey));
> +               return 0;
> +       }
> +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Thanks

> 
> Jason

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04 13:55         ` Leon Romanovsky
@ 2023-01-04 13:56           ` Jason Gunthorpe
  2023-01-04 14:03             ` Leon Romanovsky
  0 siblings, 1 reply; 18+ messages in thread
From: Jason Gunthorpe @ 2023-01-04 13:56 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > -{
> > > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > -	int err;
> > > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > +	if (err)
> > > > > +		return err;
> > > > >  
> > > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > -	if (!err)
> > > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > -				      null_mkey);
> > > > > -	return err;
> > > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > +						     out, dump_fill_mkey);
> > > > > +
> > > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > +		dev->mkeys.null_mkey = cpu_to_be32(
> > > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > +
> > > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > +		dev->mkeys.terminate_scatter_list_mkey =
> > > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > +					     terminate_scatter_list_mkey));
> > > > > +		return 0;
> > > > > +	}
> > > > > +	dev->mkeys.terminate_scatter_list_mkey =
> > > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > 
> > > > This is already stored in the core dev, why are you recalculating it
> > > > here?
> > > 
> > > It is not recalculating but setting default value. In core dev, we will
> > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > is true.
> > 
> > No, it has the identical code:
> > 
> > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > +{
> > +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > +               dev->terminate_scatter_list_mkey =
> > +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > +                                            terminate_scatter_list_mkey));
> > +               return 0;
> > +       }
> > +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> 
> Ahh, you are talking about that.
> terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> which is needed to get other mkeys. So instead of doing special logic
> for the terminate_scatter_list_mkey, we decided to use same pattern as
> for other mkeys, which don't belong to core.

Regardless, don't duplicate the code and maybe don't even duplicate
the storage of the terminate_scatter_list_mkey

Jason

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04 13:56           ` Jason Gunthorpe
@ 2023-01-04 14:03             ` Leon Romanovsky
  2023-01-06  1:04               ` Saeed Mahameed
  0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 14:03 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > > -{
> > > > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > > -	int err;
> > > > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > > +	if (err)
> > > > > > +		return err;
> > > > > >  
> > > > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > > -	if (!err)
> > > > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > > -				      null_mkey);
> > > > > > -	return err;
> > > > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > > +						     out, dump_fill_mkey);
> > > > > > +
> > > > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > > +		dev->mkeys.null_mkey = cpu_to_be32(
> > > > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > > +
> > > > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > > +		dev->mkeys.terminate_scatter_list_mkey =
> > > > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > +					     terminate_scatter_list_mkey));
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +	dev->mkeys.terminate_scatter_list_mkey =
> > > > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > 
> > > > > This is already stored in the core dev, why are you recalculating it
> > > > > here?
> > > > 
> > > > It is not recalculating but setting default value. In core dev, we will
> > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > > is true.
> > > 
> > > No, it has the identical code:
> > > 
> > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > > +{
> > > +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > > +               dev->terminate_scatter_list_mkey =
> > > +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > +                                            terminate_scatter_list_mkey));
> > > +               return 0;
> > > +       }
> > > +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > 
> > Ahh, you are talking about that.
> > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> > which is needed to get other mkeys. So instead of doing special logic
> > for the terminate_scatter_list_mkey, we decided to use same pattern as
> > for other mkeys, which don't belong to core.
> 
> Regardless, don't duplicate the code and maybe don't even duplicate
> the storage of the terminate_scatter_list_mkey

ok, will update and resend.

Thanks

> 
> Jason

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
  2023-01-04  8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
@ 2023-01-06  0:58   ` Saeed Mahameed
  2023-01-08 10:32     ` Leon Romanovsky
  0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-06  0:58 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Jason Gunthorpe, Or Har-Toov, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On 04 Jan 10:11, Leon Romanovsky wrote:
>From: Or Har-Toov <ohartoov@nvidia.com>
>
>Using query_sepcial_contexts in order to get the correct value of
>terminate_scatter_list_mkey, as FW will change it in some configurations.
>This is done one time when the device is loading and the value is being
>saved in the device context.
>
>Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
>Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> .../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
> .../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
> include/linux/mlx5/driver.h                   |  1 +
> 3 files changed, 29 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>index c76f15505a76..33d7a7095988 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
> 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
> 				wqe->data[f].byte_count = 0;
> 				wqe->data[f].lkey =
>-					MLX5_TERMINATE_SCATTER_LIST_LKEY;
>+					mdev->terminate_scatter_list_mkey;
> 				wqe->data[f].addr = 0;
> 			}
> 		}
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>index 7f5db13e3550..d39d758744a0 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
> 	return 0;
> }
>
>+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>+{
>+	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>+	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>+	int err;
>+
>+	MLX5_SET(query_special_contexts_in, in, opcode,
>+		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>+	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>+	if (err)
>+		return err;
>+
>+	if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {

Why did you execute the command unconditionally if the output is only read
conditionally? 

early exit before executing the command, older FW might fail and driver will
be unusable .. 

>+		dev->terminate_scatter_list_mkey =
>+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>+					     terminate_scatter_list_mkey));
>+		return 0;
>+	}
>+	dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Another concern, what happens with old driver that is using the hardcoded
value with newer fw ? will it fail ? will it be accepted ? 

The commit message doesn't explain what's going on here very well,
Let's discuss internally before you submit V2.

>+	return 0;
>+}
>+
> static int mlx5_load(struct mlx5_core_dev *dev)
> {
> 	int err;
>@@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
> 	mlx5_events_start(dev);
> 	mlx5_pagealloc_start(dev);
>
>+	err = mlx5_get_terminate_scatter_list_mkey(dev);
>+	if (err) {
>+		mlx5_core_err(dev, "Failed to query special contexts\n");
print the err;
>+		goto err_irq_table;
>+	}

you are querying FW too soon, no issue here but better if you move it after
mlx5_eq_table_create() to use the command EQ rather than the primitive cmd
interface. 

> 	err = mlx5_irq_table_create(dev);
> 	if (err) {
> 		mlx5_core_err(dev, "Failed to alloc IRQs\n");
>diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
>index d476255c9a3f..5f2c4038d638 100644
>--- a/include/linux/mlx5/driver.h
>+++ b/include/linux/mlx5/driver.h
>@@ -801,6 +801,7 @@ struct mlx5_core_dev {
> 	struct mlx5_rsc_dump    *rsc_dump;
> 	u32                      vsc_addr;
> 	struct mlx5_hv_vhca	*hv_vhca;
>+	__be32			terminate_scatter_list_mkey;
> };
>
> struct mlx5_db {
>-- 
>2.38.1
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-04 14:03             ` Leon Romanovsky
@ 2023-01-06  1:04               ` Saeed Mahameed
  2023-01-08 10:21                 ` Leon Romanovsky
  0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-06  1:04 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
	Paolo Abeni, Saeed Mahameed

On 04 Jan 16:03, Leon Romanovsky wrote:
>On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
>> On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
>> > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
>> > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
>> > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
>> > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
>> > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
>> > > > > > -{
>> > > > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>> > > > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>> > > > > > -	int err;
>> > > > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
>> > > > > > +	if (err)
>> > > > > > +		return err;
>> > > > > >
>> > > > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
>> > > > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>> > > > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>> > > > > > -	if (!err)
>> > > > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
>> > > > > > -				      null_mkey);
>> > > > > > -	return err;
>> > > > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
>> > > > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
>> > > > > > +						     out, dump_fill_mkey);
>> > > > > > +
>> > > > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
>> > > > > > +		dev->mkeys.null_mkey = cpu_to_be32(
>> > > > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
>> > > > > > +
>> > > > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
>> > > > > > +		dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > > > > +					     terminate_scatter_list_mkey));
>> > > > > > +		return 0;
>> > > > > > +	}
>> > > > > > +	dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > > > >
>> > > > > This is already stored in the core dev, why are you recalculating it
>> > > > > here?
>> > > >
>> > > > It is not recalculating but setting default value. In core dev, we will
>> > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
>> > > > is true.
>> > >
>> > > No, it has the identical code:
>> > >
>> > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>> > > +{
>> > > +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>> > > +               dev->terminate_scatter_list_mkey =
>> > > +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > +                                            terminate_scatter_list_mkey));
>> > > +               return 0;
>> > > +       }
>> > > +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> >
>> > Ahh, you are talking about that.
>> > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
>> > which is needed to get other mkeys. So instead of doing special logic
>> > for the terminate_scatter_list_mkey, we decided to use same pattern as
>> > for other mkeys, which don't belong to core.
>>
>> Regardless, don't duplicate the code and maybe don't even duplicate
>> the storage of the terminate_scatter_list_mkey
>
>ok, will update and resend.
>

Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
it in dev->xyz.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-06  1:04               ` Saeed Mahameed
@ 2023-01-08 10:21                 ` Leon Romanovsky
  2023-01-09 22:24                   ` Saeed Mahameed
  0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-08 10:21 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
	Paolo Abeni, Saeed Mahameed

On Thu, Jan 05, 2023 at 05:04:03PM -0800, Saeed Mahameed wrote:
> On 04 Jan 16:03, Leon Romanovsky wrote:
> > On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > > > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > > > > -{
> > > > > > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > > > > -	int err;
> > > > > > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > > > > +	if (err)
> > > > > > > > +		return err;
> > > > > > > >
> > > > > > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > > > > -	if (!err)
> > > > > > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > > > > -				      null_mkey);
> > > > > > > > -	return err;
> > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > > > > +						     out, dump_fill_mkey);
> > > > > > > > +
> > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > > > > +		dev->mkeys.null_mkey = cpu_to_be32(
> > > > > > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > > > > +
> > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > > > > +		dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > > > +					     terminate_scatter_list_mkey));
> > > > > > > > +		return 0;
> > > > > > > > +	}
> > > > > > > > +	dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > > >
> > > > > > > This is already stored in the core dev, why are you recalculating it
> > > > > > > here?
> > > > > >
> > > > > > It is not recalculating but setting default value. In core dev, we will
> > > > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > > > > is true.
> > > > >
> > > > > No, it has the identical code:
> > > > >
> > > > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > > > > +{
> > > > > +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > > > > +               dev->terminate_scatter_list_mkey =
> > > > > +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > +                                            terminate_scatter_list_mkey));
> > > > > +               return 0;
> > > > > +       }
> > > > > +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > >
> > > > Ahh, you are talking about that.
> > > > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> > > > which is needed to get other mkeys. So instead of doing special logic
> > > > for the terminate_scatter_list_mkey, we decided to use same pattern as
> > > > for other mkeys, which don't belong to core.
> > > 
> > > Regardless, don't duplicate the code and maybe don't even duplicate
> > > the storage of the terminate_scatter_list_mkey
> > 
> > ok, will update and resend.
> > 
> 
> Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
> it in dev->xyz.

This helper was used in this version of patch and it is:
 MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey))
Which was dropped in favor of Jason's request to rely on mlx5_core_dev
as this value already known:
https://lore.kernel.org/all/cover.1672917578.git.leonro@nvidia.com

I don't like the idea to add obfuscation function just for the sake of
obfuscation. If you don't want access to mlx5_core_dev, please argue with
Jason to accept first variant.

Thanks

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
  2023-01-06  0:58   ` Saeed Mahameed
@ 2023-01-08 10:32     ` Leon Romanovsky
  2023-01-09 22:31       ` Saeed Mahameed
  0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-08 10:32 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: Jason Gunthorpe, Or Har-Toov, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On Thu, Jan 05, 2023 at 04:58:22PM -0800, Saeed Mahameed wrote:
> On 04 Jan 10:11, Leon Romanovsky wrote:
> > From: Or Har-Toov <ohartoov@nvidia.com>
> > 
> > Using query_sepcial_contexts in order to get the correct value of
> > terminate_scatter_list_mkey, as FW will change it in some configurations.
> > This is done one time when the device is loading and the value is being
> > saved in the device context.
> > 
> > Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
> > .../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
> > include/linux/mlx5/driver.h                   |  1 +
> > 3 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index c76f15505a76..33d7a7095988 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
> > 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
> > 				wqe->data[f].byte_count = 0;
> > 				wqe->data[f].lkey =
> > -					MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > +					mdev->terminate_scatter_list_mkey;
> > 				wqe->data[f].addr = 0;
> > 			}
> > 		}
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 7f5db13e3550..d39d758744a0 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
> > 	return 0;
> > }
> > 
> > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > +{
> > +	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > +	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > +	int err;
> > +
> > +	MLX5_SET(query_special_contexts_in, in, opcode,
> > +		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > +	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > +	if (err)
> > +		return err;
> > +
> > +	if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> 
> Why did you execute the command unconditionally if the output is only read
> conditionally?
> 
> early exit before executing the command, older FW might fail and driver will
> be unusable ..

According to the documentation, this functionality was forever, but you
are presenting valid concern.

> 
> > +		dev->terminate_scatter_list_mkey =
> > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > +					     terminate_scatter_list_mkey));
> > +		return 0;
> > +	}
> > +	dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> 
> Another concern, what happens with old driver that is using the hardcoded
> value with newer fw ? will it fail ? will it be accepted ?

It will be accepted and everything will work without glitches.
FW keeps backward compatibility and use MLX5_TERMINATE_SCATTER_LIST_LKEY
internally as a default.

> 
> The commit message doesn't explain what's going on here very well,
> Let's discuss internally before you submit V2.

NP.

> 
> > +	return 0;
> > +}
> > +
> > static int mlx5_load(struct mlx5_core_dev *dev)
> > {
> > 	int err;
> > @@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
> > 	mlx5_events_start(dev);
> > 	mlx5_pagealloc_start(dev);
> > 
> > +	err = mlx5_get_terminate_scatter_list_mkey(dev);
> > +	if (err) {
> > +		mlx5_core_err(dev, "Failed to query special contexts\n");
> print the err;
> > +		goto err_irq_table;
> > +	}
> 
> you are querying FW too soon, no issue here but better if you move it after
> mlx5_eq_table_create() to use the command EQ rather than the primitive cmd
> interface.

I'll take a look.

Thanks

> 
> > 	err = mlx5_irq_table_create(dev);
> > 	if (err) {
> > 		mlx5_core_err(dev, "Failed to alloc IRQs\n");
> > diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> > index d476255c9a3f..5f2c4038d638 100644
> > --- a/include/linux/mlx5/driver.h
> > +++ b/include/linux/mlx5/driver.h
> > @@ -801,6 +801,7 @@ struct mlx5_core_dev {
> > 	struct mlx5_rsc_dump    *rsc_dump;
> > 	u32                      vsc_addr;
> > 	struct mlx5_hv_vhca	*hv_vhca;
> > +	__be32			terminate_scatter_list_mkey;
> > };
> > 
> > struct mlx5_db {
> > -- 
> > 2.38.1
> > 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-08 10:21                 ` Leon Romanovsky
@ 2023-01-09 22:24                   ` Saeed Mahameed
  2023-01-10  8:45                     ` Leon Romanovsky
  0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-09 22:24 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
	Paolo Abeni, Saeed Mahameed

On 08 Jan 12:21, Leon Romanovsky wrote:
>On Thu, Jan 05, 2023 at 05:04:03PM -0800, Saeed Mahameed wrote:
>> On 04 Jan 16:03, Leon Romanovsky wrote:
>> > On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
>> > > On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
>> > > > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
>> > > > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
>> > > > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
>> > > > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
>> > > > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
>> > > > > > > > -{
>> > > > > > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>> > > > > > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>> > > > > > > > -	int err;
>> > > > > > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
>> > > > > > > > +	if (err)
>> > > > > > > > +		return err;
>> > > > > > > >
>> > > > > > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
>> > > > > > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>> > > > > > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>> > > > > > > > -	if (!err)
>> > > > > > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
>> > > > > > > > -				      null_mkey);
>> > > > > > > > -	return err;
>> > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
>> > > > > > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
>> > > > > > > > +						     out, dump_fill_mkey);
>> > > > > > > > +
>> > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
>> > > > > > > > +		dev->mkeys.null_mkey = cpu_to_be32(
>> > > > > > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
>> > > > > > > > +
>> > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
>> > > > > > > > +		dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > > > > > > +					     terminate_scatter_list_mkey));
>> > > > > > > > +		return 0;
>> > > > > > > > +	}
>> > > > > > > > +	dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > > > > > >
>> > > > > > > This is already stored in the core dev, why are you recalculating it
>> > > > > > > here?
>> > > > > >
>> > > > > > It is not recalculating but setting default value. In core dev, we will
>> > > > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
>> > > > > > is true.
>> > > > >
>> > > > > No, it has the identical code:
>> > > > >
>> > > > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>> > > > > +{
>> > > > > +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>> > > > > +               dev->terminate_scatter_list_mkey =
>> > > > > +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > > > +                                            terminate_scatter_list_mkey));
>> > > > > +               return 0;
>> > > > > +       }
>> > > > > +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > > >
>> > > > Ahh, you are talking about that.
>> > > > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
>> > > > which is needed to get other mkeys. So instead of doing special logic
>> > > > for the terminate_scatter_list_mkey, we decided to use same pattern as
>> > > > for other mkeys, which don't belong to core.
>> > >
>> > > Regardless, don't duplicate the code and maybe don't even duplicate
>> > > the storage of the terminate_scatter_list_mkey
>> >
>> > ok, will update and resend.
>> >
>>
>> Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
>> it in dev->xyz.
>
>This helper was used in this version of patch and it is:
> MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey))
>Which was dropped in favor of Jason's request to rely on mlx5_core_dev
>as this value already known:
>https://lore.kernel.org/all/cover.1672917578.git.leonro@nvidia.com
>
>I don't like the idea to add obfuscation function just for the sake of
>obfuscation. If you don't want access to mlx5_core_dev, please argue with
>Jason to accept first variant.

I think you are confused, 1st version duplicated the logic that already
existed in mlx5 core, didn't just access with MLX5_GET.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
  2023-01-08 10:32     ` Leon Romanovsky
@ 2023-01-09 22:31       ` Saeed Mahameed
  0 siblings, 0 replies; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-09 22:31 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Jason Gunthorpe, Or Har-Toov, Eric Dumazet, Jakub Kicinski,
	linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
	Saeed Mahameed

On 08 Jan 12:32, Leon Romanovsky wrote:
>On Thu, Jan 05, 2023 at 04:58:22PM -0800, Saeed Mahameed wrote:
>> On 04 Jan 10:11, Leon Romanovsky wrote:
>> > From: Or Har-Toov <ohartoov@nvidia.com>
>> >
>> > Using query_sepcial_contexts in order to get the correct value of
>> > terminate_scatter_list_mkey, as FW will change it in some configurations.
>> > This is done one time when the device is loading and the value is being
>> > saved in the device context.
>> >
>> > Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
>> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>> > ---
>> > .../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
>> > .../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
>> > include/linux/mlx5/driver.h                   |  1 +
>> > 3 files changed, 29 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > index c76f15505a76..33d7a7095988 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > @@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
>> > 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
>> > 				wqe->data[f].byte_count = 0;
>> > 				wqe->data[f].lkey =
>> > -					MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > +					mdev->terminate_scatter_list_mkey;
>> > 				wqe->data[f].addr = 0;
>> > 			}
>> > 		}
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > index 7f5db13e3550..d39d758744a0 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > @@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
>> > 	return 0;
>> > }
>> >
>> > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>> > +{
>> > +	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>> > +	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>> > +	int err;
>> > +
>> > +	MLX5_SET(query_special_contexts_in, in, opcode,
>> > +		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>> > +	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>>
>> Why did you execute the command unconditionally if the output is only read
>> conditionally?
>>
>> early exit before executing the command, older FW might fail and driver will
>> be unusable ..
>
>According to the documentation, this functionality was forever, but you
>are presenting valid concern.
>
>>
>> > +		dev->terminate_scatter_list_mkey =
>> > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > +					     terminate_scatter_list_mkey));
>> > +		return 0;
>> > +	}
>> > +	dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>>
>> Another concern, what happens with old driver that is using the hardcoded
>> value with newer fw ? will it fail ? will it be accepted ?
>
>It will be accepted and everything will work without glitches.
>FW keeps backward compatibility and use MLX5_TERMINATE_SCATTER_LIST_LKEY
>internally as a default.
>

Then please don't change mlx5e to use  mdev->terminate_scatter_list_mkey in
this patch, just keep it as is and have a separate patch to change mlx5e.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
  2023-01-09 22:24                   ` Saeed Mahameed
@ 2023-01-10  8:45                     ` Leon Romanovsky
  0 siblings, 0 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-10  8:45 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
	Paolo Abeni, Saeed Mahameed

On Mon, Jan 09, 2023 at 02:24:28PM -0800, Saeed Mahameed wrote:
> On 08 Jan 12:21, Leon Romanovsky wrote:
> > On Thu, Jan 05, 2023 at 05:04:03PM -0800, Saeed Mahameed wrote:
> > > On 04 Jan 16:03, Leon Romanovsky wrote:
> > > > On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
> > > > > On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> > > > > > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > > > > > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > > > > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > > > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > > > > > > -{
> > > > > > > > > > -	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > > > > > > -	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > > > > > > -	int err;
> > > > > > > > > > +	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > > > > > > +	if (err)
> > > > > > > > > > +		return err;
> > > > > > > > > >
> > > > > > > > > > -	MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > > > > > > -		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > > > > > > -	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > > > > > > -	if (!err)
> > > > > > > > > > -		*null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > > > > > > -				      null_mkey);
> > > > > > > > > > -	return err;
> > > > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > > > > > > +		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > > > > > > +						     out, dump_fill_mkey);
> > > > > > > > > > +
> > > > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > > > > > > +		dev->mkeys.null_mkey = cpu_to_be32(
> > > > > > > > > > +			MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > > > > > > +
> > > > > > > > > > +	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > > > > > > +		dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > > > +			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > > > > > +					     terminate_scatter_list_mkey));
> > > > > > > > > > +		return 0;
> > > > > > > > > > +	}
> > > > > > > > > > +	dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > > > +		MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > > > > >
> > > > > > > > > This is already stored in the core dev, why are you recalculating it
> > > > > > > > > here?
> > > > > > > >
> > > > > > > > It is not recalculating but setting default value. In core dev, we will
> > > > > > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > > > > > > is true.
> > > > > > >
> > > > > > > No, it has the identical code:
> > > > > > >
> > > > > > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > > > > > > +{
> > > > > > > +       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > > > > > > +               dev->terminate_scatter_list_mkey =
> > > > > > > +                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > > +                                            terminate_scatter_list_mkey));
> > > > > > > +               return 0;
> > > > > > > +       }
> > > > > > > +       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > >
> > > > > > Ahh, you are talking about that.
> > > > > > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> > > > > > which is needed to get other mkeys. So instead of doing special logic
> > > > > > for the terminate_scatter_list_mkey, we decided to use same pattern as
> > > > > > for other mkeys, which don't belong to core.
> > > > >
> > > > > Regardless, don't duplicate the code and maybe don't even duplicate
> > > > > the storage of the terminate_scatter_list_mkey
> > > >
> > > > ok, will update and resend.
> > > >
> > > 
> > > Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
> > > it in dev->xyz.
> > 
> > This helper was used in this version of patch and it is:
> > MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey))
> > Which was dropped in favor of Jason's request to rely on mlx5_core_dev
> > as this value already known:
> > https://lore.kernel.org/all/cover.1672917578.git.leonro@nvidia.com
> > 
> > I don't like the idea to add obfuscation function just for the sake of
> > obfuscation. If you don't want access to mlx5_core_dev, please argue with
> > Jason to accept first variant.
> 
> I think you are confused, 1st version duplicated the logic that already
> existed in mlx5 core, didn't just access with MLX5_GET.

No, the end result is the same: call to the memory to get terminate_scatter_list_mkey
value. MLX5_GET(..) will return exactly the same value as dev->terminate_scatter_list_mkey
or some other function call:

int mlx5_get_terminate_scatter_list_mkey() {
 return dev->terminate_scatter_list_mkey;
}

> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-01-10  8:47 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-04  8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
2023-01-04  8:11 ` [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying " Leon Romanovsky
2023-01-04  8:11 ` [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value Leon Romanovsky
2023-01-04  8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
2023-01-06  0:58   ` Saeed Mahameed
2023-01-08 10:32     ` Leon Romanovsky
2023-01-09 22:31       ` Saeed Mahameed
2023-01-04  8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
2023-01-04 13:03   ` Jason Gunthorpe
2023-01-04 13:09     ` Leon Romanovsky
2023-01-04 13:13       ` Jason Gunthorpe
2023-01-04 13:55         ` Leon Romanovsky
2023-01-04 13:56           ` Jason Gunthorpe
2023-01-04 14:03             ` Leon Romanovsky
2023-01-06  1:04               ` Saeed Mahameed
2023-01-08 10:21                 ` Leon Romanovsky
2023-01-09 22:24                   ` Saeed Mahameed
2023-01-10  8:45                     ` Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).