In order to support running vdpa using vritio_vdpa driver, we need to
create a different kind of MR, one that has 1:1 mapping, since the
addresses referring to virtqueues are dma addresses.
We create the 1:1 MR in mlx5_vdpa_dev_add() only in case firmware
supports the general capability umem_uid_0. The reason for that is that
1:1 MRs must be created with uid == 0 while virtqueue objects can be
created with uid == 0 only when the firmware capability is on.
If the set_map() callback is called with new translations provided
through iotlb, the driver will destroy the 1:1 MR and create a regular
one.
Signed-off-by: Eli Cohen <[email protected]>
---
v0 --> v1:
1. Clear user_mr after successful creation of DMA MR
2. Check return code of mlx5_vdpa_create_mr() and emit warning if
failed.
drivers/vdpa/mlx5/core/mlx5_vdpa.h | 1 +
drivers/vdpa/mlx5/core/mr.c | 84 +++++++++++++++++++++++++-----
drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +++++-
3 files changed, 85 insertions(+), 15 deletions(-)
diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
index b6cc53ba980c..09a16a3d1b2a 100644
--- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
@@ -35,6 +35,7 @@ struct mlx5_vdpa_mr {
/* serialize mkey creation and destruction */
struct mutex mkey_mtx;
+ bool user_mr;
};
struct mlx5_vdpa_resources {
diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
index 800cfd1967ad..3c6c1d846f5e 100644
--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c
@@ -360,7 +360,7 @@ static int add_direct_chain(struct mlx5_vdpa_dev *mvdev, u64 start, u64 size, u8
* indirect memory key that provides access to the enitre address space given
* by iotlb.
*/
-static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
+static int create_user_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
{
struct mlx5_vdpa_mr *mr = &mvdev->mr;
struct mlx5_vdpa_direct_mr *dmr;
@@ -374,9 +374,6 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
int err = 0;
int nnuls;
- if (mr->initialized)
- return 0;
-
INIT_LIST_HEAD(&mr->head);
for (map = vhost_iotlb_itree_first(iotlb, start, last); map;
map = vhost_iotlb_itree_next(map, start, last)) {
@@ -414,7 +411,7 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
if (err)
goto err_chain;
- mr->initialized = true;
+ mr->user_mr = true;
return 0;
err_chain:
@@ -426,33 +423,92 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
return err;
}
-int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
+static int create_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
+{
+ int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
+ void *mkc;
+ u32 *in;
+ int err;
+
+ in = kzalloc(inlen, GFP_KERNEL);
+ if (!in)
+ return -ENOMEM;
+
+ mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
+
+ MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_PA);
+ MLX5_SET(mkc, mkc, length64, 1);
+ MLX5_SET(mkc, mkc, lw, 1);
+ MLX5_SET(mkc, mkc, lr, 1);
+ MLX5_SET(mkc, mkc, pd, mvdev->res.pdn);
+ MLX5_SET(mkc, mkc, qpn, 0xffffff);
+
+ err = mlx5_vdpa_create_mkey(mvdev, &mr->mkey, in, inlen);
+ if (!err)
+ mr->user_mr = false;
+
+ kfree(in);
+ return err;
+}
+
+static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
+{
+ mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
+}
+
+static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
{
struct mlx5_vdpa_mr *mr = &mvdev->mr;
int err;
- mutex_lock(&mr->mkey_mtx);
+ if (mr->initialized)
+ return 0;
+
+ if (iotlb)
+ err = create_user_mr(mvdev, iotlb);
+ else
+ err = create_dma_mr(mvdev, mr);
+
+ mr->initialized = true;
+ return err;
+}
+
+int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
+{
+ int err;
+
+ mutex_lock(&mvdev->mr.mkey_mtx);
err = _mlx5_vdpa_create_mr(mvdev, iotlb);
- mutex_unlock(&mr->mkey_mtx);
+ mutex_unlock(&mvdev->mr.mkey_mtx);
return err;
}
-void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
+static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
{
- struct mlx5_vdpa_mr *mr = &mvdev->mr;
struct mlx5_vdpa_direct_mr *dmr;
struct mlx5_vdpa_direct_mr *n;
- mutex_lock(&mr->mkey_mtx);
- if (!mr->initialized)
- goto out;
-
destroy_indirect_key(mvdev, mr);
list_for_each_entry_safe_reverse(dmr, n, &mr->head, list) {
list_del_init(&dmr->list);
unmap_direct_mr(mvdev, dmr);
kfree(dmr);
}
+}
+
+void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
+{
+ struct mlx5_vdpa_mr *mr = &mvdev->mr;
+
+ mutex_lock(&mr->mkey_mtx);
+ if (!mr->initialized)
+ goto out;
+
+ if (mr->user_mr)
+ destroy_user_mr(mvdev, mr);
+ else
+ destroy_dma_mr(mvdev, mr);
+
memset(mr, 0, sizeof(*mr));
mr->initialized = false;
out:
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index fdf3e74bffbd..02a05492204c 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1780,6 +1780,10 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
ndev->mvdev.status = 0;
ndev->mvdev.mlx_features = 0;
++mvdev->generation;
+ if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
+ if (mlx5_vdpa_create_mr(mvdev, NULL))
+ mlx5_vdpa_warn(mvdev, "create MR failed\n");
+ }
return;
}
@@ -1859,6 +1863,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
ndev = to_mlx5_vdpa_ndev(mvdev);
free_resources(ndev);
+ mlx5_vdpa_destroy_mr(mvdev);
mlx5_vdpa_free_resources(&ndev->mvdev);
mutex_destroy(&ndev->reslock);
}
@@ -2023,9 +2028,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
if (err)
goto err_mtu;
+ if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
+ err = mlx5_vdpa_create_mr(mvdev, NULL);
+ if (err)
+ goto err_res;
+ }
+
err = alloc_resources(ndev);
if (err)
- goto err_res;
+ goto err_mr;
mvdev->vdev.mdev = &mgtdev->mgtdev;
err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
@@ -2037,6 +2048,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
err_reg:
free_resources(ndev);
+err_mr:
+ mlx5_vdpa_destroy_mr(mvdev);
err_res:
mlx5_vdpa_free_resources(&ndev->mvdev);
err_mtu:
--
2.31.1
Hi Eli,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.13-rc3]
[cannot apply to linus/master v5.13-rc4 next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Eli-Cohen/vdpa-mlx5-Add-support-for-running-with-virtio_vdpa/20210601-010404
base: c4681547bcce777daf576925a966ffa824edd09d
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6f3930e43ca375bc3d3325f02350db3226b891cf
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Eli-Cohen/vdpa-mlx5-Add-support-for-running-with-virtio_vdpa/20210601-010404
git checkout 6f3930e43ca375bc3d3325f02350db3226b891cf
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from include/linux/byteorder/big_endian.h:5,
from arch/arc/include/uapi/asm/byteorder.h:14,
from include/asm-generic/bitops/le.h:7,
from arch/arc/include/asm/bitops.h:373,
from include/linux/bitops.h:32,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/vdpa/mlx5/net/mlx5_vnet.c:4:
drivers/vdpa/mlx5/net/mlx5_vnet.c: In function 'mlx5_vdpa_set_status':
>> include/linux/compiler_types.h:140:35: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
| ^~~~~~~~~~~~~~~~~~
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of macro '__be32_to_cpu'
40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
| ^
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:51:35: note: in expansion of macro 'offsetof'
51 | #define __mlx5_bit_off(typ, fld) (offsetof(struct mlx5_ifc_##typ##_bits, fld))
| ^~~~~~~~
include/linux/mlx5/device.h:53:34: note: in expansion of macro '__mlx5_bit_off'
53 | #define __mlx5_dw_off(typ, fld) (__mlx5_bit_off(typ, fld) / 32)
| ^~~~~~~~~~~~~~
include/linux/mlx5/device.h:96:1: note: in expansion of macro '__mlx5_dw_off'
96 | __mlx5_dw_off(typ, fld))) >> __mlx5_dw_bit_off(typ, fld)) & \
| ^~~~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:1788:7: note: in expansion of macro 'MLX5_CAP_GEN'
1788 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from include/linux/mlx5/driver.h:53,
from include/linux/mlx5/cq.h:36,
from drivers/vdpa/mlx5/net/mlx5_vnet.c:11:
>> include/linux/mlx5/device.h:50:57: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
50 | #define __mlx5_bit_sz(typ, fld) sizeof(__mlx5_nullp(typ)->fld)
| ^~
include/linux/mlx5/device.h:56:43: note: in expansion of macro '__mlx5_bit_sz'
56 | #define __mlx5_dw_bit_off(typ, fld) (32 - __mlx5_bit_sz(typ, fld) - (__mlx5_bit_off(typ, fld) & 0x1f))
| ^~~~~~~~~~~~~
include/linux/mlx5/device.h:96:30: note: in expansion of macro '__mlx5_dw_bit_off'
96 | __mlx5_dw_off(typ, fld))) >> __mlx5_dw_bit_off(typ, fld)) & \
| ^~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:1788:7: note: in expansion of macro 'MLX5_CAP_GEN'
1788 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from <command-line>:
>> include/linux/compiler_types.h:140:35: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
| ^~~~~~~~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:51:35: note: in expansion of macro 'offsetof'
51 | #define __mlx5_bit_off(typ, fld) (offsetof(struct mlx5_ifc_##typ##_bits, fld))
| ^~~~~~~~
include/linux/mlx5/device.h:56:70: note: in expansion of macro '__mlx5_bit_off'
56 | #define __mlx5_dw_bit_off(typ, fld) (32 - __mlx5_bit_sz(typ, fld) - (__mlx5_bit_off(typ, fld) & 0x1f))
| ^~~~~~~~~~~~~~
include/linux/mlx5/device.h:96:30: note: in expansion of macro '__mlx5_dw_bit_off'
96 | __mlx5_dw_off(typ, fld))) >> __mlx5_dw_bit_off(typ, fld)) & \
| ^~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:1788:7: note: in expansion of macro 'MLX5_CAP_GEN'
1788 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from include/linux/mlx5/driver.h:53,
from include/linux/mlx5/cq.h:36,
from drivers/vdpa/mlx5/net/mlx5_vnet.c:11:
>> include/linux/mlx5/device.h:50:57: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
50 | #define __mlx5_bit_sz(typ, fld) sizeof(__mlx5_nullp(typ)->fld)
| ^~
include/linux/mlx5/device.h:57:47: note: in expansion of macro '__mlx5_bit_sz'
57 | #define __mlx5_mask(typ, fld) ((u32)((1ull << __mlx5_bit_sz(typ, fld)) - 1))
| ^~~~~~~~~~~~~
include/linux/mlx5/device.h:97:1: note: in expansion of macro '__mlx5_mask'
97 | __mlx5_mask(typ, fld))
| ^~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:1788:7: note: in expansion of macro 'MLX5_CAP_GEN'
1788 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from include/linux/byteorder/big_endian.h:5,
from arch/arc/include/uapi/asm/byteorder.h:14,
from include/asm-generic/bitops/le.h:7,
from arch/arc/include/asm/bitops.h:373,
from include/linux/bitops.h:32,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/vdpa/mlx5/net/mlx5_vnet.c:4:
drivers/vdpa/mlx5/net/mlx5_vnet.c: In function 'mlx5_vdpa_dev_add':
>> include/linux/compiler_types.h:140:35: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
| ^~~~~~~~~~~~~~~~~~
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of macro '__be32_to_cpu'
40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
| ^
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:51:35: note: in expansion of macro 'offsetof'
51 | #define __mlx5_bit_off(typ, fld) (offsetof(struct mlx5_ifc_##typ##_bits, fld))
| ^~~~~~~~
include/linux/mlx5/device.h:53:34: note: in expansion of macro '__mlx5_bit_off'
53 | #define __mlx5_dw_off(typ, fld) (__mlx5_bit_off(typ, fld) / 32)
| ^~~~~~~~~~~~~~
include/linux/mlx5/device.h:96:1: note: in expansion of macro '__mlx5_dw_off'
96 | __mlx5_dw_off(typ, fld))) >> __mlx5_dw_bit_off(typ, fld)) & \
| ^~~~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:2036:6: note: in expansion of macro 'MLX5_CAP_GEN'
2036 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from include/linux/mlx5/driver.h:53,
from include/linux/mlx5/cq.h:36,
from drivers/vdpa/mlx5/net/mlx5_vnet.c:11:
>> include/linux/mlx5/device.h:50:57: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
50 | #define __mlx5_bit_sz(typ, fld) sizeof(__mlx5_nullp(typ)->fld)
| ^~
include/linux/mlx5/device.h:56:43: note: in expansion of macro '__mlx5_bit_sz'
56 | #define __mlx5_dw_bit_off(typ, fld) (32 - __mlx5_bit_sz(typ, fld) - (__mlx5_bit_off(typ, fld) & 0x1f))
| ^~~~~~~~~~~~~
include/linux/mlx5/device.h:96:30: note: in expansion of macro '__mlx5_dw_bit_off'
96 | __mlx5_dw_off(typ, fld))) >> __mlx5_dw_bit_off(typ, fld)) & \
| ^~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:2036:6: note: in expansion of macro 'MLX5_CAP_GEN'
2036 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from <command-line>:
>> include/linux/compiler_types.h:140:35: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
140 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
| ^~~~~~~~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:51:35: note: in expansion of macro 'offsetof'
51 | #define __mlx5_bit_off(typ, fld) (offsetof(struct mlx5_ifc_##typ##_bits, fld))
| ^~~~~~~~
include/linux/mlx5/device.h:56:70: note: in expansion of macro '__mlx5_bit_off'
56 | #define __mlx5_dw_bit_off(typ, fld) (32 - __mlx5_bit_sz(typ, fld) - (__mlx5_bit_off(typ, fld) & 0x1f))
| ^~~~~~~~~~~~~~
include/linux/mlx5/device.h:96:30: note: in expansion of macro '__mlx5_dw_bit_off'
96 | __mlx5_dw_off(typ, fld))) >> __mlx5_dw_bit_off(typ, fld)) & \
| ^~~~~~~~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:2036:6: note: in expansion of macro 'MLX5_CAP_GEN'
2036 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
In file included from include/linux/mlx5/driver.h:53,
from include/linux/mlx5/cq.h:36,
from drivers/vdpa/mlx5/net/mlx5_vnet.c:11:
>> include/linux/mlx5/device.h:50:57: error: 'struct mlx5_ifc_cmd_hca_cap_bits' has no member named 'umem_uid_0'
50 | #define __mlx5_bit_sz(typ, fld) sizeof(__mlx5_nullp(typ)->fld)
| ^~
include/linux/mlx5/device.h:57:47: note: in expansion of macro '__mlx5_bit_sz'
57 | #define __mlx5_mask(typ, fld) ((u32)((1ull << __mlx5_bit_sz(typ, fld)) - 1))
| ^~~~~~~~~~~~~
include/linux/mlx5/device.h:97:1: note: in expansion of macro '__mlx5_mask'
97 | __mlx5_mask(typ, fld))
| ^~~~~~~~~~~
include/linux/mlx5/device.h:1215:2: note: in expansion of macro 'MLX5_GET'
1215 | MLX5_GET(cmd_hca_cap, mdev->caps.hca_cur[MLX5_CAP_GENERAL], cap)
| ^~~~~~~~
drivers/vdpa/mlx5/net/mlx5_vnet.c:2036:6: note: in expansion of macro 'MLX5_CAP_GEN'
2036 | if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
| ^~~~~~~~~~~~
vim +140 include/linux/compiler_types.h
71391bdd2e9aab Xiaozhou Liu 2018-12-14 139
71391bdd2e9aab Xiaozhou Liu 2018-12-14 @140 #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
71391bdd2e9aab Xiaozhou Liu 2018-12-14 141
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
?? 2021/6/1 ????12:04, Eli Cohen д??:
> In order to support running vdpa using vritio_vdpa driver, we need to
> create a different kind of MR, one that has 1:1 mapping, since the
> addresses referring to virtqueues are dma addresses.
>
> We create the 1:1 MR in mlx5_vdpa_dev_add() only in case firmware
> supports the general capability umem_uid_0. The reason for that is that
> 1:1 MRs must be created with uid == 0 while virtqueue objects can be
> created with uid == 0 only when the firmware capability is on.
>
> If the set_map() callback is called with new translations provided
> through iotlb, the driver will destroy the 1:1 MR and create a regular
> one.
>
> Signed-off-by: Eli Cohen <[email protected]>
> ---
> v0 --> v1:
> 1. Clear user_mr after successful creation of DMA MR
> 2. Check return code of mlx5_vdpa_create_mr() and emit warning if
> failed.
>
> drivers/vdpa/mlx5/core/mlx5_vdpa.h | 1 +
> drivers/vdpa/mlx5/core/mr.c | 84 +++++++++++++++++++++++++-----
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +++++-
> 3 files changed, 85 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index b6cc53ba980c..09a16a3d1b2a 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -35,6 +35,7 @@ struct mlx5_vdpa_mr {
>
> /* serialize mkey creation and destruction */
> struct mutex mkey_mtx;
> + bool user_mr;
> };
>
> struct mlx5_vdpa_resources {
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index 800cfd1967ad..3c6c1d846f5e 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -360,7 +360,7 @@ static int add_direct_chain(struct mlx5_vdpa_dev *mvdev, u64 start, u64 size, u8
> * indirect memory key that provides access to the enitre address space given
> * by iotlb.
> */
> -static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +static int create_user_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> {
> struct mlx5_vdpa_mr *mr = &mvdev->mr;
> struct mlx5_vdpa_direct_mr *dmr;
> @@ -374,9 +374,6 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> int err = 0;
> int nnuls;
>
> - if (mr->initialized)
> - return 0;
> -
> INIT_LIST_HEAD(&mr->head);
> for (map = vhost_iotlb_itree_first(iotlb, start, last); map;
> map = vhost_iotlb_itree_next(map, start, last)) {
> @@ -414,7 +411,7 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> if (err)
> goto err_chain;
>
> - mr->initialized = true;
> + mr->user_mr = true;
> return 0;
>
> err_chain:
> @@ -426,33 +423,92 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> return err;
> }
>
> -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +static int create_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> +{
> + int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
> + void *mkc;
> + u32 *in;
> + int err;
> +
> + in = kzalloc(inlen, GFP_KERNEL);
> + if (!in)
> + return -ENOMEM;
> +
> + mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
> +
> + MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_PA);
> + MLX5_SET(mkc, mkc, length64, 1);
> + MLX5_SET(mkc, mkc, lw, 1);
> + MLX5_SET(mkc, mkc, lr, 1);
> + MLX5_SET(mkc, mkc, pd, mvdev->res.pdn);
> + MLX5_SET(mkc, mkc, qpn, 0xffffff);
> +
> + err = mlx5_vdpa_create_mkey(mvdev, &mr->mkey, in, inlen);
> + if (!err)
> + mr->user_mr = false;
Rethink about this. I wonder this is correct when we fail to create
memory key.
In this case, user_mr is true but user_mr is already destroyed. Can this
lead double free for user mr?
Thanks
> +
> + kfree(in);
> + return err;
> +}
> +
> +static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> +{
> + mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
> +}
> +
> +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> {
> struct mlx5_vdpa_mr *mr = &mvdev->mr;
> int err;
>
> - mutex_lock(&mr->mkey_mtx);
> + if (mr->initialized)
> + return 0;
> +
> + if (iotlb)
> + err = create_user_mr(mvdev, iotlb);
> + else
> + err = create_dma_mr(mvdev, mr);
> +
> + mr->initialized = true;
> + return err;
> +}
> +
> +int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +{
> + int err;
> +
> + mutex_lock(&mvdev->mr.mkey_mtx);
> err = _mlx5_vdpa_create_mr(mvdev, iotlb);
> - mutex_unlock(&mr->mkey_mtx);
> + mutex_unlock(&mvdev->mr.mkey_mtx);
> return err;
> }
>
> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> +static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> {
> - struct mlx5_vdpa_mr *mr = &mvdev->mr;
> struct mlx5_vdpa_direct_mr *dmr;
> struct mlx5_vdpa_direct_mr *n;
>
> - mutex_lock(&mr->mkey_mtx);
> - if (!mr->initialized)
> - goto out;
> -
> destroy_indirect_key(mvdev, mr);
> list_for_each_entry_safe_reverse(dmr, n, &mr->head, list) {
> list_del_init(&dmr->list);
> unmap_direct_mr(mvdev, dmr);
> kfree(dmr);
> }
> +}
> +
> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> +{
> + struct mlx5_vdpa_mr *mr = &mvdev->mr;
> +
> + mutex_lock(&mr->mkey_mtx);
> + if (!mr->initialized)
> + goto out;
> +
> + if (mr->user_mr)
> + destroy_user_mr(mvdev, mr);
> + else
> + destroy_dma_mr(mvdev, mr);
> +
> memset(mr, 0, sizeof(*mr));
> mr->initialized = false;
> out:
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fdf3e74bffbd..02a05492204c 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1780,6 +1780,10 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> ndev->mvdev.status = 0;
> ndev->mvdev.mlx_features = 0;
> ++mvdev->generation;
> + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> + if (mlx5_vdpa_create_mr(mvdev, NULL))
> + mlx5_vdpa_warn(mvdev, "create MR failed\n");
> + }
> return;
> }
>
> @@ -1859,6 +1863,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
> ndev = to_mlx5_vdpa_ndev(mvdev);
>
> free_resources(ndev);
> + mlx5_vdpa_destroy_mr(mvdev);
> mlx5_vdpa_free_resources(&ndev->mvdev);
> mutex_destroy(&ndev->reslock);
> }
> @@ -2023,9 +2028,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> if (err)
> goto err_mtu;
>
> + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> + err = mlx5_vdpa_create_mr(mvdev, NULL);
> + if (err)
> + goto err_res;
> + }
> +
> err = alloc_resources(ndev);
> if (err)
> - goto err_res;
> + goto err_mr;
>
> mvdev->vdev.mdev = &mgtdev->mgtdev;
> err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
> @@ -2037,6 +2048,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>
> err_reg:
> free_resources(ndev);
> +err_mr:
> + mlx5_vdpa_destroy_mr(mvdev);
> err_res:
> mlx5_vdpa_free_resources(&ndev->mvdev);
> err_mtu:
On Tue, Jun 01, 2021 at 10:09:45AM +0800, Jason Wang wrote:
>
> 在 2021/6/1 上午12:04, Eli Cohen 写道:
> > In order to support running vdpa using vritio_vdpa driver, we need to
> > create a different kind of MR, one that has 1:1 mapping, since the
> > addresses referring to virtqueues are dma addresses.
> >
> > We create the 1:1 MR in mlx5_vdpa_dev_add() only in case firmware
> > supports the general capability umem_uid_0. The reason for that is that
> > 1:1 MRs must be created with uid == 0 while virtqueue objects can be
> > created with uid == 0 only when the firmware capability is on.
> >
> > If the set_map() callback is called with new translations provided
> > through iotlb, the driver will destroy the 1:1 MR and create a regular
> > one.
> >
> > Signed-off-by: Eli Cohen <[email protected]>
> > ---
> > v0 --> v1:
> > 1. Clear user_mr after successful creation of DMA MR
> > 2. Check return code of mlx5_vdpa_create_mr() and emit warning if
> > failed.
> >
> > drivers/vdpa/mlx5/core/mlx5_vdpa.h | 1 +
> > drivers/vdpa/mlx5/core/mr.c | 84 +++++++++++++++++++++++++-----
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +++++-
> > 3 files changed, 85 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > index b6cc53ba980c..09a16a3d1b2a 100644
> > --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > @@ -35,6 +35,7 @@ struct mlx5_vdpa_mr {
> > /* serialize mkey creation and destruction */
> > struct mutex mkey_mtx;
> > + bool user_mr;
> > };
> > struct mlx5_vdpa_resources {
> > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > index 800cfd1967ad..3c6c1d846f5e 100644
> > --- a/drivers/vdpa/mlx5/core/mr.c
> > +++ b/drivers/vdpa/mlx5/core/mr.c
> > @@ -360,7 +360,7 @@ static int add_direct_chain(struct mlx5_vdpa_dev *mvdev, u64 start, u64 size, u8
> > * indirect memory key that provides access to the enitre address space given
> > * by iotlb.
> > */
> > -static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > +static int create_user_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > {
> > struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > struct mlx5_vdpa_direct_mr *dmr;
> > @@ -374,9 +374,6 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > int err = 0;
> > int nnuls;
> > - if (mr->initialized)
> > - return 0;
> > -
> > INIT_LIST_HEAD(&mr->head);
> > for (map = vhost_iotlb_itree_first(iotlb, start, last); map;
> > map = vhost_iotlb_itree_next(map, start, last)) {
> > @@ -414,7 +411,7 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > if (err)
> > goto err_chain;
> > - mr->initialized = true;
> > + mr->user_mr = true;
> > return 0;
> > err_chain:
> > @@ -426,33 +423,92 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > return err;
> > }
> > -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > +static int create_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> > +{
> > + int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
> > + void *mkc;
> > + u32 *in;
> > + int err;
> > +
> > + in = kzalloc(inlen, GFP_KERNEL);
> > + if (!in)
> > + return -ENOMEM;
> > +
> > + mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
> > +
> > + MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_PA);
> > + MLX5_SET(mkc, mkc, length64, 1);
> > + MLX5_SET(mkc, mkc, lw, 1);
> > + MLX5_SET(mkc, mkc, lr, 1);
> > + MLX5_SET(mkc, mkc, pd, mvdev->res.pdn);
> > + MLX5_SET(mkc, mkc, qpn, 0xffffff);
> > +
> > + err = mlx5_vdpa_create_mkey(mvdev, &mr->mkey, in, inlen);
> > + if (!err)
> > + mr->user_mr = false;
>
>
> Rethink about this. I wonder this is correct when we fail to create memory
> key.
>
> In this case, user_mr is true but user_mr is already destroyed. Can this
> lead double free for user mr?
mr->user_mr is a binary flag and its sole purpose is to tell the flavour
of the MR but is valid only when mr->initialized is true. MR won't be
freed if mr->initialized is false.
>
> Thanks
>
>
> > +
> > + kfree(in);
> > + return err;
> > +}
> > +
> > +static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> > +{
> > + mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
> > +}
> > +
> > +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > {
> > struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > int err;
> > - mutex_lock(&mr->mkey_mtx);
> > + if (mr->initialized)
> > + return 0;
> > +
> > + if (iotlb)
> > + err = create_user_mr(mvdev, iotlb);
> > + else
> > + err = create_dma_mr(mvdev, mr);
> > +
> > + mr->initialized = true;
> > + return err;
> > +}
> > +
> > +int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > +{
> > + int err;
> > +
> > + mutex_lock(&mvdev->mr.mkey_mtx);
> > err = _mlx5_vdpa_create_mr(mvdev, iotlb);
> > - mutex_unlock(&mr->mkey_mtx);
> > + mutex_unlock(&mvdev->mr.mkey_mtx);
> > return err;
> > }
> > -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> > +static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> > {
> > - struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > struct mlx5_vdpa_direct_mr *dmr;
> > struct mlx5_vdpa_direct_mr *n;
> > - mutex_lock(&mr->mkey_mtx);
> > - if (!mr->initialized)
> > - goto out;
> > -
> > destroy_indirect_key(mvdev, mr);
> > list_for_each_entry_safe_reverse(dmr, n, &mr->head, list) {
> > list_del_init(&dmr->list);
> > unmap_direct_mr(mvdev, dmr);
> > kfree(dmr);
> > }
> > +}
> > +
> > +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> > +{
> > + struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > +
> > + mutex_lock(&mr->mkey_mtx);
> > + if (!mr->initialized)
> > + goto out;
> > +
> > + if (mr->user_mr)
> > + destroy_user_mr(mvdev, mr);
> > + else
> > + destroy_dma_mr(mvdev, mr);
> > +
> > memset(mr, 0, sizeof(*mr));
> > mr->initialized = false;
> > out:
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index fdf3e74bffbd..02a05492204c 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1780,6 +1780,10 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> > ndev->mvdev.status = 0;
> > ndev->mvdev.mlx_features = 0;
> > ++mvdev->generation;
> > + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> > + if (mlx5_vdpa_create_mr(mvdev, NULL))
> > + mlx5_vdpa_warn(mvdev, "create MR failed\n");
> > + }
> > return;
> > }
> > @@ -1859,6 +1863,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
> > ndev = to_mlx5_vdpa_ndev(mvdev);
> > free_resources(ndev);
> > + mlx5_vdpa_destroy_mr(mvdev);
> > mlx5_vdpa_free_resources(&ndev->mvdev);
> > mutex_destroy(&ndev->reslock);
> > }
> > @@ -2023,9 +2028,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> > if (err)
> > goto err_mtu;
> > + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> > + err = mlx5_vdpa_create_mr(mvdev, NULL);
> > + if (err)
> > + goto err_res;
> > + }
> > +
> > err = alloc_resources(ndev);
> > if (err)
> > - goto err_res;
> > + goto err_mr;
> > mvdev->vdev.mdev = &mgtdev->mgtdev;
> > err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
> > @@ -2037,6 +2048,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> > err_reg:
> > free_resources(ndev);
> > +err_mr:
> > + mlx5_vdpa_destroy_mr(mvdev);
> > err_res:
> > mlx5_vdpa_free_resources(&ndev->mvdev);
> > err_mtu:
>
在 2021/6/1 上午11:40, Eli Cohen 写道:
> On Tue, Jun 01, 2021 at 10:09:45AM +0800, Jason Wang wrote:
>> 在 2021/6/1 上午12:04, Eli Cohen 写道:
>>> In order to support running vdpa using vritio_vdpa driver, we need to
>>> create a different kind of MR, one that has 1:1 mapping, since the
>>> addresses referring to virtqueues are dma addresses.
>>>
>>> We create the 1:1 MR in mlx5_vdpa_dev_add() only in case firmware
>>> supports the general capability umem_uid_0. The reason for that is that
>>> 1:1 MRs must be created with uid == 0 while virtqueue objects can be
>>> created with uid == 0 only when the firmware capability is on.
>>>
>>> If the set_map() callback is called with new translations provided
>>> through iotlb, the driver will destroy the 1:1 MR and create a regular
>>> one.
>>>
>>> Signed-off-by: Eli Cohen <[email protected]>
>>> ---
>>> v0 --> v1:
>>> 1. Clear user_mr after successful creation of DMA MR
>>> 2. Check return code of mlx5_vdpa_create_mr() and emit warning if
>>> failed.
>>>
>>> drivers/vdpa/mlx5/core/mlx5_vdpa.h | 1 +
>>> drivers/vdpa/mlx5/core/mr.c | 84 +++++++++++++++++++++++++-----
>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +++++-
>>> 3 files changed, 85 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> index b6cc53ba980c..09a16a3d1b2a 100644
>>> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> @@ -35,6 +35,7 @@ struct mlx5_vdpa_mr {
>>> /* serialize mkey creation and destruction */
>>> struct mutex mkey_mtx;
>>> + bool user_mr;
>>> };
>>> struct mlx5_vdpa_resources {
>>> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
>>> index 800cfd1967ad..3c6c1d846f5e 100644
>>> --- a/drivers/vdpa/mlx5/core/mr.c
>>> +++ b/drivers/vdpa/mlx5/core/mr.c
>>> @@ -360,7 +360,7 @@ static int add_direct_chain(struct mlx5_vdpa_dev *mvdev, u64 start, u64 size, u8
>>> * indirect memory key that provides access to the enitre address space given
>>> * by iotlb.
>>> */
>>> -static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +static int create_user_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> {
>>> struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> struct mlx5_vdpa_direct_mr *dmr;
>>> @@ -374,9 +374,6 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>>> int err = 0;
>>> int nnuls;
>>> - if (mr->initialized)
>>> - return 0;
>>> -
>>> INIT_LIST_HEAD(&mr->head);
>>> for (map = vhost_iotlb_itree_first(iotlb, start, last); map;
>>> map = vhost_iotlb_itree_next(map, start, last)) {
>>> @@ -414,7 +411,7 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>>> if (err)
>>> goto err_chain;
>>> - mr->initialized = true;
>>> + mr->user_mr = true;
>>> return 0;
>>> err_chain:
>>> @@ -426,33 +423,92 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>>> return err;
>>> }
>>> -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +static int create_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
>>> +{
>>> + int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
>>> + void *mkc;
>>> + u32 *in;
>>> + int err;
>>> +
>>> + in = kzalloc(inlen, GFP_KERNEL);
>>> + if (!in)
>>> + return -ENOMEM;
>>> +
>>> + mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
>>> +
>>> + MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_PA);
>>> + MLX5_SET(mkc, mkc, length64, 1);
>>> + MLX5_SET(mkc, mkc, lw, 1);
>>> + MLX5_SET(mkc, mkc, lr, 1);
>>> + MLX5_SET(mkc, mkc, pd, mvdev->res.pdn);
>>> + MLX5_SET(mkc, mkc, qpn, 0xffffff);
>>> +
>>> + err = mlx5_vdpa_create_mkey(mvdev, &mr->mkey, in, inlen);
>>> + if (!err)
>>> + mr->user_mr = false;
>>
>> Rethink about this. I wonder this is correct when we fail to create memory
>> key.
>>
>> In this case, user_mr is true but user_mr is already destroyed. Can this
>> lead double free for user mr?
> mr->user_mr is a binary flag and its sole purpose is to tell the flavour
> of the MR but is valid only when mr->initialized is true. MR won't be
> freed if mr->initialized is false.
So we have:
static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct
vhost_iotlb *iotlb)
{
struct mlx5_vdpa_mr *mr = &mvdev->mr;
int err;
if (mr->initialized)
return 0;
if (iotlb)
err = create_user_mr(mvdev, iotlb);
else
err = create_dma_mr(mvdev, mr);
mr->initialized = true;
return err;
}
It looks to me we need to check err before set mr->initialized.
Thanks
>
>> Thanks
>>
>>
>>> +
>>> + kfree(in);
>>> + return err;
>>> +}
>>> +
>>> +static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
>>> +{
>>> + mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
>>> +}
>>> +
>>> +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> {
>>> struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> int err;
>>> - mutex_lock(&mr->mkey_mtx);
>>> + if (mr->initialized)
>>> + return 0;
>>> +
>>> + if (iotlb)
>>> + err = create_user_mr(mvdev, iotlb);
>>> + else
>>> + err = create_dma_mr(mvdev, mr);
>>> +
>>> + mr->initialized = true;
>>> + return err;
>>> +}
>>> +
>>> +int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +{
>>> + int err;
>>> +
>>> + mutex_lock(&mvdev->mr.mkey_mtx);
>>> err = _mlx5_vdpa_create_mr(mvdev, iotlb);
>>> - mutex_unlock(&mr->mkey_mtx);
>>> + mutex_unlock(&mvdev->mr.mkey_mtx);
>>> return err;
>>> }
>>> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>>> +static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
>>> {
>>> - struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> struct mlx5_vdpa_direct_mr *dmr;
>>> struct mlx5_vdpa_direct_mr *n;
>>> - mutex_lock(&mr->mkey_mtx);
>>> - if (!mr->initialized)
>>> - goto out;
>>> -
>>> destroy_indirect_key(mvdev, mr);
>>> list_for_each_entry_safe_reverse(dmr, n, &mr->head, list) {
>>> list_del_init(&dmr->list);
>>> unmap_direct_mr(mvdev, dmr);
>>> kfree(dmr);
>>> }
>>> +}
>>> +
>>> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>>> +{
>>> + struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> +
>>> + mutex_lock(&mr->mkey_mtx);
>>> + if (!mr->initialized)
>>> + goto out;
>>> +
>>> + if (mr->user_mr)
>>> + destroy_user_mr(mvdev, mr);
>>> + else
>>> + destroy_dma_mr(mvdev, mr);
>>> +
>>> memset(mr, 0, sizeof(*mr));
>>> mr->initialized = false;
>>> out:
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index fdf3e74bffbd..02a05492204c 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1780,6 +1780,10 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>>> ndev->mvdev.status = 0;
>>> ndev->mvdev.mlx_features = 0;
>>> ++mvdev->generation;
>>> + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
>>> + if (mlx5_vdpa_create_mr(mvdev, NULL))
>>> + mlx5_vdpa_warn(mvdev, "create MR failed\n");
>>> + }
>>> return;
>>> }
>>> @@ -1859,6 +1863,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
>>> ndev = to_mlx5_vdpa_ndev(mvdev);
>>> free_resources(ndev);
>>> + mlx5_vdpa_destroy_mr(mvdev);
>>> mlx5_vdpa_free_resources(&ndev->mvdev);
>>> mutex_destroy(&ndev->reslock);
>>> }
>>> @@ -2023,9 +2028,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>> if (err)
>>> goto err_mtu;
>>> + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
>>> + err = mlx5_vdpa_create_mr(mvdev, NULL);
>>> + if (err)
>>> + goto err_res;
>>> + }
>>> +
>>> err = alloc_resources(ndev);
>>> if (err)
>>> - goto err_res;
>>> + goto err_mr;
>>> mvdev->vdev.mdev = &mgtdev->mgtdev;
>>> err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
>>> @@ -2037,6 +2048,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>> err_reg:
>>> free_resources(ndev);
>>> +err_mr:
>>> + mlx5_vdpa_destroy_mr(mvdev);
>>> err_res:
>>> mlx5_vdpa_free_resources(&ndev->mvdev);
>>> err_mtu:
On Tue, Jun 01, 2021 at 01:00:05PM +0800, Jason Wang wrote:
>
> 在 2021/6/1 上午11:40, Eli Cohen 写道:
> > On Tue, Jun 01, 2021 at 10:09:45AM +0800, Jason Wang wrote:
> > > 在 2021/6/1 上午12:04, Eli Cohen 写道:
> > > > In order to support running vdpa using vritio_vdpa driver, we need to
> > > > create a different kind of MR, one that has 1:1 mapping, since the
> > > > addresses referring to virtqueues are dma addresses.
> > > >
> > > > We create the 1:1 MR in mlx5_vdpa_dev_add() only in case firmware
> > > > supports the general capability umem_uid_0. The reason for that is that
> > > > 1:1 MRs must be created with uid == 0 while virtqueue objects can be
> > > > created with uid == 0 only when the firmware capability is on.
> > > >
> > > > If the set_map() callback is called with new translations provided
> > > > through iotlb, the driver will destroy the 1:1 MR and create a regular
> > > > one.
> > > >
> > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > ---
> > > > v0 --> v1:
> > > > 1. Clear user_mr after successful creation of DMA MR
> > > > 2. Check return code of mlx5_vdpa_create_mr() and emit warning if
> > > > failed.
> > > >
> > > > drivers/vdpa/mlx5/core/mlx5_vdpa.h | 1 +
> > > > drivers/vdpa/mlx5/core/mr.c | 84 +++++++++++++++++++++++++-----
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +++++-
> > > > 3 files changed, 85 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > > > index b6cc53ba980c..09a16a3d1b2a 100644
> > > > --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > > > +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > > > @@ -35,6 +35,7 @@ struct mlx5_vdpa_mr {
> > > > /* serialize mkey creation and destruction */
> > > > struct mutex mkey_mtx;
> > > > + bool user_mr;
> > > > };
> > > > struct mlx5_vdpa_resources {
> > > > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > > > index 800cfd1967ad..3c6c1d846f5e 100644
> > > > --- a/drivers/vdpa/mlx5/core/mr.c
> > > > +++ b/drivers/vdpa/mlx5/core/mr.c
> > > > @@ -360,7 +360,7 @@ static int add_direct_chain(struct mlx5_vdpa_dev *mvdev, u64 start, u64 size, u8
> > > > * indirect memory key that provides access to the enitre address space given
> > > > * by iotlb.
> > > > */
> > > > -static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > > > +static int create_user_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > > > {
> > > > struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > > > struct mlx5_vdpa_direct_mr *dmr;
> > > > @@ -374,9 +374,6 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > > > int err = 0;
> > > > int nnuls;
> > > > - if (mr->initialized)
> > > > - return 0;
> > > > -
> > > > INIT_LIST_HEAD(&mr->head);
> > > > for (map = vhost_iotlb_itree_first(iotlb, start, last); map;
> > > > map = vhost_iotlb_itree_next(map, start, last)) {
> > > > @@ -414,7 +411,7 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > > > if (err)
> > > > goto err_chain;
> > > > - mr->initialized = true;
> > > > + mr->user_mr = true;
> > > > return 0;
> > > > err_chain:
> > > > @@ -426,33 +423,92 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > > > return err;
> > > > }
> > > > -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > > > +static int create_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> > > > +{
> > > > + int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
> > > > + void *mkc;
> > > > + u32 *in;
> > > > + int err;
> > > > +
> > > > + in = kzalloc(inlen, GFP_KERNEL);
> > > > + if (!in)
> > > > + return -ENOMEM;
> > > > +
> > > > + mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
> > > > +
> > > > + MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_PA);
> > > > + MLX5_SET(mkc, mkc, length64, 1);
> > > > + MLX5_SET(mkc, mkc, lw, 1);
> > > > + MLX5_SET(mkc, mkc, lr, 1);
> > > > + MLX5_SET(mkc, mkc, pd, mvdev->res.pdn);
> > > > + MLX5_SET(mkc, mkc, qpn, 0xffffff);
> > > > +
> > > > + err = mlx5_vdpa_create_mkey(mvdev, &mr->mkey, in, inlen);
> > > > + if (!err)
> > > > + mr->user_mr = false;
> > >
> > > Rethink about this. I wonder this is correct when we fail to create memory
> > > key.
> > >
> > > In this case, user_mr is true but user_mr is already destroyed. Can this
> > > lead double free for user mr?
> > mr->user_mr is a binary flag and its sole purpose is to tell the flavour
> > of the MR but is valid only when mr->initialized is true. MR won't be
> > freed if mr->initialized is false.
>
>
> So we have:
>
> static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct
> vhost_iotlb *iotlb)
> {
> struct mlx5_vdpa_mr *mr = &mvdev->mr;
> int err;
>
> if (mr->initialized)
> return 0;
>
> if (iotlb)
> err = create_user_mr(mvdev, iotlb);
> else
> err = create_dma_mr(mvdev, mr);
>
> mr->initialized = true;
> return err;
> }
>
> It looks to me we need to check err before set mr->initialized.
Correct, will fix.
>
> Thanks
>
>
> >
> > > Thanks
> > >
> > >
> > > > +
> > > > + kfree(in);
> > > > + return err;
> > > > +}
> > > > +
> > > > +static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> > > > +{
> > > > + mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
> > > > +}
> > > > +
> > > > +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > > > {
> > > > struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > > > int err;
> > > > - mutex_lock(&mr->mkey_mtx);
> > > > + if (mr->initialized)
> > > > + return 0;
> > > > +
> > > > + if (iotlb)
> > > > + err = create_user_mr(mvdev, iotlb);
> > > > + else
> > > > + err = create_dma_mr(mvdev, mr);
> > > > +
> > > > + mr->initialized = true;
> > > > + return err;
> > > > +}
> > > > +
> > > > +int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> > > > +{
> > > > + int err;
> > > > +
> > > > + mutex_lock(&mvdev->mr.mkey_mtx);
> > > > err = _mlx5_vdpa_create_mr(mvdev, iotlb);
> > > > - mutex_unlock(&mr->mkey_mtx);
> > > > + mutex_unlock(&mvdev->mr.mkey_mtx);
> > > > return err;
> > > > }
> > > > -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> > > > +static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> > > > {
> > > > - struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > > > struct mlx5_vdpa_direct_mr *dmr;
> > > > struct mlx5_vdpa_direct_mr *n;
> > > > - mutex_lock(&mr->mkey_mtx);
> > > > - if (!mr->initialized)
> > > > - goto out;
> > > > -
> > > > destroy_indirect_key(mvdev, mr);
> > > > list_for_each_entry_safe_reverse(dmr, n, &mr->head, list) {
> > > > list_del_init(&dmr->list);
> > > > unmap_direct_mr(mvdev, dmr);
> > > > kfree(dmr);
> > > > }
> > > > +}
> > > > +
> > > > +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> > > > +{
> > > > + struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > > > +
> > > > + mutex_lock(&mr->mkey_mtx);
> > > > + if (!mr->initialized)
> > > > + goto out;
> > > > +
> > > > + if (mr->user_mr)
> > > > + destroy_user_mr(mvdev, mr);
> > > > + else
> > > > + destroy_dma_mr(mvdev, mr);
> > > > +
> > > > memset(mr, 0, sizeof(*mr));
> > > > mr->initialized = false;
> > > > out:
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index fdf3e74bffbd..02a05492204c 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1780,6 +1780,10 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> > > > ndev->mvdev.status = 0;
> > > > ndev->mvdev.mlx_features = 0;
> > > > ++mvdev->generation;
> > > > + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> > > > + if (mlx5_vdpa_create_mr(mvdev, NULL))
> > > > + mlx5_vdpa_warn(mvdev, "create MR failed\n");
> > > > + }
> > > > return;
> > > > }
> > > > @@ -1859,6 +1863,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
> > > > ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > free_resources(ndev);
> > > > + mlx5_vdpa_destroy_mr(mvdev);
> > > > mlx5_vdpa_free_resources(&ndev->mvdev);
> > > > mutex_destroy(&ndev->reslock);
> > > > }
> > > > @@ -2023,9 +2028,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> > > > if (err)
> > > > goto err_mtu;
> > > > + if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> > > > + err = mlx5_vdpa_create_mr(mvdev, NULL);
> > > > + if (err)
> > > > + goto err_res;
> > > > + }
> > > > +
> > > > err = alloc_resources(ndev);
> > > > if (err)
> > > > - goto err_res;
> > > > + goto err_mr;
> > > > mvdev->vdev.mdev = &mgtdev->mgtdev;
> > > > err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
> > > > @@ -2037,6 +2048,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> > > > err_reg:
> > > > free_resources(ndev);
> > > > +err_mr:
> > > > + mlx5_vdpa_destroy_mr(mvdev);
> > > > err_res:
> > > > mlx5_vdpa_free_resources(&ndev->mvdev);
> > > > err_mtu:
>