| Message ID | 4c58f1aa2e9664b90ecdc478aef12213816cf1b7.1672819469.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rely on firmware to get special mkeys | expand |
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
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
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
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
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
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
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.
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
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.
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; } > >
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; }