2023-12-01 10:49:46

by Dragos Tatulea

Subject: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

Add support for resumable vqs in the driver. This is a firmware feature
that can be used for the following benefits:
- Full device .suspend/.resume.
- .set_map doesn't need to destroy and create new vqs anymore just to
update the map. When resumable vqs are supported it is enough to
suspend the vqs, set the new maps, and then resume the vqs.

The first patch exposes the relevant bits in mlx5_ifc.h. That means it
needs to be applied to the mlx5-vhost tree [0] first. Once applied
there, the change has to be pulled from mlx5-vhost into the vhost tree
and only then the remaining patches can be applied. Same flow as the vq
descriptor mappings patchset [1].

To be able to use resumable vqs properly, support for selectively modifying
vq parameters was needed. This is what the middle part of the series
consists of.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
[1] https://lore.kernel.org/virtualization/[email protected]/

Dragos Tatulea (7):
vdpa/mlx5: Expose resumable vq capability
vdpa/mlx5: Split function into locked and unlocked variants
vdpa/mlx5: Allow modifying multiple vq fields in one modify command
vdpa/mlx5: Introduce per vq and device resume
vdpa/mlx5: Mark vq addrs for modification in hw vq
vdpa/mlx5: Mark vq state for modification in hw vq
vdpa/mlx5: Use vq suspend/resume during .set_map

drivers/vdpa/mlx5/core/mr.c | 31 +++---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
include/linux/mlx5/mlx5_ifc.h | 3 +-
include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
4 files changed, 174 insertions(+), 36 deletions(-)

--
2.42.0

2023-12-01 10:50:12

by Dragos Tatulea

Subject: [PATCH vhost 3/7] vdpa/mlx5: Allow modifying multiple vq fields in one modify command

Add a bitmask variable that tracks hw vq field changes that
are supposed to be modified on next hw vq change command.

This will be useful to set multiple vq fields when resuming the vq.

The state needs to remain as a parameter as it doesn't make sense to
make it part of the vq struct: fw_state is updated only after a
successful command.

Signed-off-by: Dragos Tatulea <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 48 +++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 12ac3397f39b..d06285e46fe2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -120,6 +120,9 @@ struct mlx5_vdpa_virtqueue {
u16 avail_idx;
u16 used_idx;
int fw_state;
+
+ u64 modified_fields;
+
struct msi_map map;

/* keep last in the struct */
@@ -1181,7 +1184,19 @@ static bool is_valid_state_change(int oldstate, int newstate)
}
}

-static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, int state)
+static bool modifiable_virtqueue_fields(struct mlx5_vdpa_virtqueue *mvq)
+{
+ /* Only state is always modifiable */
+ if (mvq->modified_fields & ~MLX5_VIRTQ_MODIFY_MASK_STATE)
+ return mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT ||
+ mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
+
+ return true;
+}
+
+static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
+ struct mlx5_vdpa_virtqueue *mvq,
+ int state)
{
int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
@@ -1193,6 +1208,9 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
return 0;

+ if (!modifiable_virtqueue_fields(mvq))
+ return -EINVAL;
+
if (!is_valid_state_change(mvq->fw_state, state))
return -EINVAL;

@@ -1208,17 +1226,28 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);

obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
- MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select,
- MLX5_VIRTQ_MODIFY_MASK_STATE);
- MLX5_SET(virtio_net_q_object, obj_context, state, state);
+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
+ MLX5_SET(virtio_net_q_object, obj_context, state, state);
+
+ MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
kfree(in);
if (!err)
mvq->fw_state = state;

+ mvq->modified_fields = 0;
+
return err;
}

+static int modify_virtqueue_state(struct mlx5_vdpa_net *ndev,
+ struct mlx5_vdpa_virtqueue *mvq,
+ unsigned int state)
+{
+ mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_STATE;
+ return modify_virtqueue(ndev, mvq, state);
+}
+
static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
{
u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
@@ -1347,7 +1376,7 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
goto err_vq;

if (mvq->ready) {
- err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
+ err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
if (err) {
mlx5_vdpa_warn(&ndev->mvdev, "failed to modify to ready vq idx %d(%d)\n",
idx, err);
@@ -1382,7 +1411,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
return;

- if (modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
+ if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
mlx5_vdpa_warn(&ndev->mvdev, "modify to suspend failed\n");

if (query_virtqueue(ndev, mvq, &attr)) {
@@ -1407,6 +1436,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
return;

suspend_vq(ndev, mvq);
+ mvq->modified_fields = 0;
destroy_virtqueue(ndev, mvq);
dealloc_vector(ndev, mvq);
counter_set_dealloc(ndev, mvq);
@@ -2207,7 +2237,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
if (!ready) {
suspend_vq(ndev, mvq);
} else {
- err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
+ err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
if (err) {
mlx5_vdpa_warn(mvdev, "modify VQ %d to ready failed (%d)\n", idx, err);
ready = false;
@@ -2804,8 +2834,10 @@ static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
{
int i;

- for (i = 0; i < ndev->mvdev.max_vqs; i++)
+ for (i = 0; i < ndev->mvdev.max_vqs; i++) {
ndev->vqs[i].ready = false;
+ ndev->vqs[i].modified_fields = 0;
+ }

ndev->mvdev.cvq.ready = false;
}
--
2.42.0

2023-12-01 10:50:28

by Dragos Tatulea

Subject: [PATCH vhost 5/7] vdpa/mlx5: Mark vq addrs for modification in hw vq

Addresses get set by .set_vq_address. hw vq addresses will be updated on
next modify_virtqueue.

Signed-off-by: Dragos Tatulea <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
2 files changed, 10 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 68e534cb57e2..2277daf4814f 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
bool state_change = false;
void *obj_context;
void *cmd_hdr;
+ void *vq_ctx;
void *in;
int err;

@@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);

obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
+ vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);

if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
@@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
state_change = true;
}

+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
+ MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
+ MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
+ MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
+ }
+
MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
if (err)
@@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
mvq->desc_addr = desc_area;
mvq->device_addr = device_area;
mvq->driver_addr = driver_area;
+ mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
return 0;
}

diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
index b86d51a855f6..9594ac405740 100644
--- a/include/linux/mlx5/mlx5_ifc_vdpa.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -145,6 +145,7 @@ enum {
MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
+ MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
};

--
2.42.0

2023-12-01 10:50:29

by Dragos Tatulea

Subject: [PATCH mlx5-vhost 1/7] vdpa/mlx5: Expose resumable vq capability

Necessary for checking if resumable vqs are supported by the hardware.
Actual support will be added in a downstream patch.

Signed-off-by: Dragos Tatulea <[email protected]>
---
include/linux/mlx5/mlx5_ifc.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 3388007c645f..21dcfa034b7b 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -1235,7 +1235,8 @@ struct mlx5_ifc_virtio_emulation_cap_bits {

u8 reserved_at_c0[0x13];
u8 desc_group_mkey_supported[0x1];
- u8 reserved_at_d4[0xc];
+ u8 freeze_to_rdy_supported[0x1];
+ u8 reserved_at_d5[0xb];

u8 reserved_at_e0[0x20];

--
2.42.0

2023-12-01 10:50:29

by Dragos Tatulea

Subject: [PATCH vhost 4/7] vdpa/mlx5: Introduce per vq and device resume

Implement vdpa vq and device resume if capability detected. Add support
for suspend -> ready state change.

Signed-off-by: Dragos Tatulea <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 67 +++++++++++++++++++++++++++----
1 file changed, 60 insertions(+), 7 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index d06285e46fe2..68e534cb57e2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1170,7 +1170,12 @@ static int query_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueu
return err;
}

-static bool is_valid_state_change(int oldstate, int newstate)
+static bool is_resumable(struct mlx5_vdpa_net *ndev)
+{
+ return ndev->mvdev.vdev.config->resume;
+}
+
+static bool is_valid_state_change(int oldstate, int newstate, bool resumable)
{
switch (oldstate) {
case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
@@ -1178,6 +1183,7 @@ static bool is_valid_state_change(int oldstate, int newstate)
case MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY:
return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
case MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND:
+ return resumable ? newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY : false;
case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
default:
return false;
@@ -1200,6 +1206,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
{
int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
+ bool state_change = false;
void *obj_context;
void *cmd_hdr;
void *in;
@@ -1211,9 +1218,6 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
if (!modifiable_virtqueue_fields(mvq))
return -EINVAL;

- if (!is_valid_state_change(mvq->fw_state, state))
- return -EINVAL;
-
in = kzalloc(inlen, GFP_KERNEL);
if (!in)
return -ENOMEM;
@@ -1226,17 +1230,29 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);

obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
- if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
+
+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
+ if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
+ err = -EINVAL;
+ goto done;
+ }
+
MLX5_SET(virtio_net_q_object, obj_context, state, state);
+ state_change = true;
+ }

MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
- kfree(in);
- if (!err)
+ if (err)
+ goto done;
+
+ if (state_change)
mvq->fw_state = state;

mvq->modified_fields = 0;

+done:
+ kfree(in);
return err;
}

@@ -1430,6 +1446,24 @@ static void suspend_vqs(struct mlx5_vdpa_net *ndev)
suspend_vq(ndev, &ndev->vqs[i]);
}

+static void resume_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
+{
+ if (!mvq->initialized || !is_resumable(ndev))
+ return;
+
+ if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND)
+ return;
+
+ if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY))
+ mlx5_vdpa_warn(&ndev->mvdev, "modify to resume failed\n");
+}
+
+static void resume_vqs(struct mlx5_vdpa_net *ndev)
+{
+ for (int i = 0; i < ndev->mvdev.max_vqs; i++)
+ resume_vq(ndev, &ndev->vqs[i]);
+}
+
static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
{
if (!mvq->initialized)
@@ -3256,6 +3290,21 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
return 0;
}

+static int mlx5_vdpa_resume(struct vdpa_device *vdev)
+{
+ struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+ struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+
+ mlx5_vdpa_info(mvdev, "resuming device\n");
+
+ down_write(&ndev->reslock);
+ mvdev->suspended = false;
+ resume_vqs(ndev);
+ register_link_notifier(ndev);
+ up_write(&ndev->reslock);
+ return 0;
+}
+
static int mlx5_set_group_asid(struct vdpa_device *vdev, u32 group,
unsigned int asid)
{
@@ -3312,6 +3361,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
.get_vq_dma_dev = mlx5_get_vq_dma_dev,
.free = mlx5_vdpa_free,
.suspend = mlx5_vdpa_suspend,
+ .resume = mlx5_vdpa_resume, /* Op disabled if not supported. */
};

static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
@@ -3683,6 +3733,9 @@ static int mlx5v_probe(struct auxiliary_device *adev,
if (!MLX5_CAP_DEV_VDPA_EMULATION(mdev, desc_group_mkey_supported))
mgtdev->vdpa_ops.get_vq_desc_group = NULL;

+ if (!MLX5_CAP_DEV_VDPA_EMULATION(mdev, freeze_to_rdy_supported))
+ mgtdev->vdpa_ops.resume = NULL;
+
err = vdpa_mgmtdev_register(&mgtdev->mgtdev);
if (err)
goto reg_err;
--
2.42.0

2023-12-01 10:50:38

by Dragos Tatulea

Subject: [PATCH vhost 6/7] vdpa/mlx5: Mark vq state for modification in hw vq

.set_vq_state will set the indices and mark the fields to be modified in
the hw vq.

Signed-off-by: Dragos Tatulea <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 8 ++++++++
include/linux/mlx5/mlx5_ifc_vdpa.h | 2 ++
2 files changed, 10 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 2277daf4814f..6325aef045e2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1249,6 +1249,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
}

+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX)
+ MLX5_SET(virtio_net_q_object, obj_context, hw_available_index, mvq->avail_idx);
+
+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX)
+ MLX5_SET(virtio_net_q_object, obj_context, hw_used_index, mvq->used_idx);
+
MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
if (err)
@@ -2328,6 +2334,8 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,

mvq->used_idx = state->split.avail_index;
mvq->avail_idx = state->split.avail_index;
+ mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX |
+ MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX;
return 0;
}

diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
index 9594ac405740..32e712106e68 100644
--- a/include/linux/mlx5/mlx5_ifc_vdpa.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -146,6 +146,8 @@ enum {
MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
+ MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX = (u64)1 << 7,
+ MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX = (u64)1 << 8,
MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
};

--
2.42.0

2023-12-01 10:51:20

by Dragos Tatulea

Subject: [PATCH vhost 2/7] vdpa/mlx5: Split function into locked and unlocked variants

mlx5_vdpa_destroy_mr contains more logic than _mlx5_vdpa_destroy_mr.
There is no reason for this to be the case. All the logic can go into
the unlocked variant.

Using the unlocked version is needed in a follow-up patch. And it also
makes it more consistent with mlx5_vdpa_create_mr.

Signed-off-by: Dragos Tatulea <[email protected]>
---
drivers/vdpa/mlx5/core/mr.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
index 2197c46e563a..8c80d9e77935 100644
--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c
@@ -498,32 +498,32 @@ static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr

static void _mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
{
+ if (!mr)
+ return;
+
if (mr->user_mr)
destroy_user_mr(mvdev, mr);
else
destroy_dma_mr(mvdev, mr);

+ for (int i = 0; i < MLX5_VDPA_NUM_AS; i++) {
+ if (mvdev->mr[i] == mr)
+ mvdev->mr[i] = NULL;
+ }
+
vhost_iotlb_free(mr->iotlb);
+
+ kfree(mr);
}

void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev,
struct mlx5_vdpa_mr *mr)
{
- if (!mr)
- return;
-
mutex_lock(&mvdev->mr_mtx);

_mlx5_vdpa_destroy_mr(mvdev, mr);

- for (int i = 0; i < MLX5_VDPA_NUM_AS; i++) {
- if (mvdev->mr[i] == mr)
- mvdev->mr[i] = NULL;
- }
-
mutex_unlock(&mvdev->mr_mtx);
-
- kfree(mr);
}

void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
@@ -535,10 +535,7 @@ void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
mutex_lock(&mvdev->mr_mtx);

mvdev->mr[asid] = new_mr;
- if (old_mr) {
- _mlx5_vdpa_destroy_mr(mvdev, old_mr);
- kfree(old_mr);
- }
+ _mlx5_vdpa_destroy_mr(mvdev, old_mr);

mutex_unlock(&mvdev->mr_mtx);

@@ -546,8 +543,12 @@ void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,

void mlx5_vdpa_destroy_mr_resources(struct mlx5_vdpa_dev *mvdev)
{
+ mutex_lock(&mvdev->mr_mtx);
+
for (int i = 0; i < MLX5_VDPA_NUM_AS; i++)
- mlx5_vdpa_destroy_mr(mvdev, mvdev->mr[i]);
+ _mlx5_vdpa_destroy_mr(mvdev, mvdev->mr[i]);
+
+ mutex_unlock(&mvdev->mr_mtx);

prune_iotlb(mvdev->cvq.iotlb);
}
--
2.42.0

2023-12-01 10:51:21

by Dragos Tatulea

Subject: [PATCH vhost 7/7] vdpa/mlx5: Use vq suspend/resume during .set_map

Instead of tearing down and setting up vq resources, use vq
suspend/resume during .set_map to speed things up a bit.

The vq mr is updated with the new mapping while the vqs are suspended.

If the device doesn't support resumable vqs, do the old teardown and
setup dance.

Signed-off-by: Dragos Tatulea <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 46 ++++++++++++++++++++++++------
include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 6325aef045e2..eb0ac3d9c9d2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1206,6 +1206,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
{
int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
+ struct mlx5_vdpa_dev *mvdev = &ndev->mvdev;
bool state_change = false;
void *obj_context;
void *cmd_hdr;
@@ -1255,6 +1256,24 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX)
MLX5_SET(virtio_net_q_object, obj_context, hw_used_index, mvq->used_idx);

+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_MKEY) {
+ struct mlx5_vdpa_mr *mr = mvdev->mr[mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP]];
+
+ if (mr)
+ MLX5_SET(virtio_q, vq_ctx, virtio_q_mkey, mr->mkey);
+ else
+ mvq->modified_fields &= ~MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_MKEY;
+ }
+
+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY) {
+ struct mlx5_vdpa_mr *mr = mvdev->mr[mvdev->group2asid[MLX5_VDPA_DATAVQ_DESC_GROUP]];
+
+ if (mr && MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, desc_group_mkey_supported))
+ MLX5_SET(virtio_q, vq_ctx, desc_group_mkey, mr->mkey);
+ else
+ mvq->modified_fields &= ~MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY;
+ }
+
MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
if (err)
@@ -2784,24 +2803,35 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
unsigned int asid)
{
struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+ bool teardown = !is_resumable(ndev);
int err;

suspend_vqs(ndev);
- err = save_channels_info(ndev);
- if (err)
- return err;
+ if (teardown) {
+ err = save_channels_info(ndev);
+ if (err)
+ return err;

- teardown_driver(ndev);
+ teardown_driver(ndev);
+ }

mlx5_vdpa_update_mr(mvdev, new_mr, asid);

+ for (int i = 0; i < ndev->cur_num_vqs; i++)
+ ndev->vqs[i].modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_MKEY |
+ MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY;
+
if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || mvdev->suspended)
return 0;

- restore_channels_info(ndev);
- err = setup_driver(mvdev);
- if (err)
- return err;
+ if (teardown) {
+ restore_channels_info(ndev);
+ err = setup_driver(mvdev);
+ if (err)
+ return err;
+ }
+
+ resume_vqs(ndev);

return 0;
}
diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
index 32e712106e68..40371c916cf9 100644
--- a/include/linux/mlx5/mlx5_ifc_vdpa.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -148,6 +148,7 @@ enum {
MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX = (u64)1 << 7,
MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX = (u64)1 << 8,
+ MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_MKEY = (u64)1 << 11,
MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
};

--
2.42.0

2023-12-01 11:47:38

by Eugenio Perez Martin

Subject: Re: [PATCH vhost 2/7] vdpa/mlx5: Split function into locked and unlocked variants

On Fri, Dec 1, 2023 at 11:49 AM Dragos Tatulea <[email protected]> wrote:
>
> mlx5_vdpa_destroy_mr contains more logic than _mlx5_vdpa_destroy_mr.
> There is no reason for this to be the case. All the logic can go into
> the unlocked variant.
>
> Using the unlocked version is needed in a follow-up patch. And it also
> makes it more consistent with mlx5_vdpa_create_mr.
>
> Signed-off-by: Dragos Tatulea <[email protected]>

Acked-by: Eugenio Pérez <[email protected]>

> ---
> drivers/vdpa/mlx5/core/mr.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index 2197c46e563a..8c80d9e77935 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -498,32 +498,32 @@ static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr
>
> static void _mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> {
> + if (!mr)
> + return;
> +
> if (mr->user_mr)
> destroy_user_mr(mvdev, mr);
> else
> destroy_dma_mr(mvdev, mr);
>
> + for (int i = 0; i < MLX5_VDPA_NUM_AS; i++) {
> + if (mvdev->mr[i] == mr)
> + mvdev->mr[i] = NULL;
> + }
> +
> vhost_iotlb_free(mr->iotlb);
> +
> + kfree(mr);
> }
>
> void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev,
> struct mlx5_vdpa_mr *mr)
> {
> - if (!mr)
> - return;
> -
> mutex_lock(&mvdev->mr_mtx);
>
> _mlx5_vdpa_destroy_mr(mvdev, mr);
>
> - for (int i = 0; i < MLX5_VDPA_NUM_AS; i++) {
> - if (mvdev->mr[i] == mr)
> - mvdev->mr[i] = NULL;
> - }
> -
> mutex_unlock(&mvdev->mr_mtx);
> -
> - kfree(mr);
> }
>
> void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
> @@ -535,10 +535,7 @@ void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
> mutex_lock(&mvdev->mr_mtx);
>
> mvdev->mr[asid] = new_mr;
> - if (old_mr) {
> - _mlx5_vdpa_destroy_mr(mvdev, old_mr);
> - kfree(old_mr);
> - }
> + _mlx5_vdpa_destroy_mr(mvdev, old_mr);
>
> mutex_unlock(&mvdev->mr_mtx);
>
> @@ -546,8 +543,12 @@ void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
>
> void mlx5_vdpa_destroy_mr_resources(struct mlx5_vdpa_dev *mvdev)
> {
> + mutex_lock(&mvdev->mr_mtx);
> +
> for (int i = 0; i < MLX5_VDPA_NUM_AS; i++)
> - mlx5_vdpa_destroy_mr(mvdev, mvdev->mr[i]);
> + _mlx5_vdpa_destroy_mr(mvdev, mvdev->mr[i]);
> +
> + mutex_unlock(&mvdev->mr_mtx);
>
> prune_iotlb(mvdev->cvq.iotlb);
> }
> --
> 2.42.0
>

2023-12-01 14:48:12

by Eugenio Perez Martin

Subject: Re: [PATCH vhost 3/7] vdpa/mlx5: Allow modifying multiple vq fields in one modify command

On Fri, Dec 1, 2023 at 11:49 AM Dragos Tatulea <[email protected]> wrote:
>
> Add a bitmask variable that tracks hw vq field changes that
> are supposed to be modified on next hw vq change command.
>
> This will be useful to set multiple vq fields when resuming the vq.
>
> The state needs to remain as a parameter as it doesn't make sense to
> make it part of the vq struct: fw_state is updated only after a
> successful command.
>

I don't get this paragraph, "modified_fields" is a member of
"mlx5_vdpa_virtqueue". Am I missing something?


> Signed-off-by: Dragos Tatulea <[email protected]>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 48 +++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 12ac3397f39b..d06285e46fe2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -120,6 +120,9 @@ struct mlx5_vdpa_virtqueue {
> u16 avail_idx;
> u16 used_idx;
> int fw_state;
> +
> + u64 modified_fields;
> +
> struct msi_map map;
>
> /* keep last in the struct */
> @@ -1181,7 +1184,19 @@ static bool is_valid_state_change(int oldstate, int newstate)
> }
> }
>
> -static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, int state)
> +static bool modifiable_virtqueue_fields(struct mlx5_vdpa_virtqueue *mvq)
> +{
> + /* Only state is always modifiable */
> + if (mvq->modified_fields & ~MLX5_VIRTQ_MODIFY_MASK_STATE)
> + return mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT ||
> + mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
> +
> + return true;
> +}
> +
> +static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> + struct mlx5_vdpa_virtqueue *mvq,
> + int state)
> {
> int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
> u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
> @@ -1193,6 +1208,9 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> return 0;
>
> + if (!modifiable_virtqueue_fields(mvq))
> + return -EINVAL;
> +

I'm ok with this change, but since modified_fields is (or will be) a
bitmap tracking changes to state, addresses, mkey, etc, doesn't have
more sense to check it like:

if (modified_fields & 1<<change_1_flag)
// perform change 1
if (modified_fields & 1<<change_2_flag)
// perform change 2
if (modified_fields & 1<<change_3_flag)
// perform change 13
---

Instead of:
if (!modified_fields)
return

if (modified_fields & 1<<change_1_flag)
// perform change 1
if (modified_fields & 1<<change_2_flag)
// perform change 2

// perform change 3, no checking, as it is the only possible value of
modified_fields
---

Or am I missing something?

The rest looks good to me.

> if (!is_valid_state_change(mvq->fw_state, state))
> return -EINVAL;
>
> @@ -1208,17 +1226,28 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
>
> obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
> - MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select,
> - MLX5_VIRTQ_MODIFY_MASK_STATE);
> - MLX5_SET(virtio_net_q_object, obj_context, state, state);
> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
> + MLX5_SET(virtio_net_q_object, obj_context, state, state);
> +
> + MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
> err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
> kfree(in);
> if (!err)
> mvq->fw_state = state;
>
> + mvq->modified_fields = 0;
> +
> return err;
> }
>
> +static int modify_virtqueue_state(struct mlx5_vdpa_net *ndev,
> + struct mlx5_vdpa_virtqueue *mvq,
> + unsigned int state)
> +{
> + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_STATE;
> + return modify_virtqueue(ndev, mvq, state);
> +}
> +
> static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> {
> u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
> @@ -1347,7 +1376,7 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> goto err_vq;
>
> if (mvq->ready) {
> - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> if (err) {
> mlx5_vdpa_warn(&ndev->mvdev, "failed to modify to ready vq idx %d(%d)\n",
> idx, err);
> @@ -1382,7 +1411,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
> return;
>
> - if (modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> + if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> mlx5_vdpa_warn(&ndev->mvdev, "modify to suspend failed\n");
>
> if (query_virtqueue(ndev, mvq, &attr)) {
> @@ -1407,6 +1436,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> return;
>
> suspend_vq(ndev, mvq);
> + mvq->modified_fields = 0;
> destroy_virtqueue(ndev, mvq);
> dealloc_vector(ndev, mvq);
> counter_set_dealloc(ndev, mvq);
> @@ -2207,7 +2237,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> if (!ready) {
> suspend_vq(ndev, mvq);
> } else {
> - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> if (err) {
> mlx5_vdpa_warn(mvdev, "modify VQ %d to ready failed (%d)\n", idx, err);
> ready = false;
> @@ -2804,8 +2834,10 @@ static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> {
> int i;
>
> - for (i = 0; i < ndev->mvdev.max_vqs; i++)
> + for (i = 0; i < ndev->mvdev.max_vqs; i++) {
> ndev->vqs[i].ready = false;
> + ndev->vqs[i].modified_fields = 0;
> + }
>
> ndev->mvdev.cvq.ready = false;
> }
> --
> 2.42.0
>

2023-12-01 14:52:33

by Eugenio Perez Martin

Subject: Re: [PATCH vhost 4/7] vdpa/mlx5: Introduce per vq and device resume

On Fri, Dec 1, 2023 at 11:50 AM Dragos Tatulea <[email protected]> wrote:
>
> Implement vdpa vq and device resume if capability detected. Add support
> for suspend -> ready state change.
>
> Signed-off-by: Dragos Tatulea <[email protected]>

Acked-by: Eugenio Pérez <[email protected]>

> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 67 +++++++++++++++++++++++++++----
> 1 file changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d06285e46fe2..68e534cb57e2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1170,7 +1170,12 @@ static int query_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueu
> return err;
> }
>
> -static bool is_valid_state_change(int oldstate, int newstate)
> +static bool is_resumable(struct mlx5_vdpa_net *ndev)
> +{
> + return ndev->mvdev.vdev.config->resume;
> +}
> +
> +static bool is_valid_state_change(int oldstate, int newstate, bool resumable)
> {
> switch (oldstate) {
> case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> @@ -1178,6 +1183,7 @@ static bool is_valid_state_change(int oldstate, int newstate)
> case MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY:
> return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
> case MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND:
> + return resumable ? newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY : false;
> case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> default:
> return false;
> @@ -1200,6 +1206,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> {
> int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
> u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
> + bool state_change = false;
> void *obj_context;
> void *cmd_hdr;
> void *in;
> @@ -1211,9 +1218,6 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> if (!modifiable_virtqueue_fields(mvq))
> return -EINVAL;
>
> - if (!is_valid_state_change(mvq->fw_state, state))
> - return -EINVAL;
> -
> in = kzalloc(inlen, GFP_KERNEL);
> if (!in)
> return -ENOMEM;
> @@ -1226,17 +1230,29 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
>
> obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
> - if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
> +
> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> + if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> + err = -EINVAL;
> + goto done;
> + }
> +
> MLX5_SET(virtio_net_q_object, obj_context, state, state);
> + state_change = true;
> + }
>
> MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
> err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
> - kfree(in);
> - if (!err)
> + if (err)
> + goto done;
> +
> + if (state_change)
> mvq->fw_state = state;
>
> mvq->modified_fields = 0;
>
> +done:
> + kfree(in);
> return err;
> }
>
> @@ -1430,6 +1446,24 @@ static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> suspend_vq(ndev, &ndev->vqs[i]);
> }
>
> +static void resume_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> +{
> + if (!mvq->initialized || !is_resumable(ndev))
> + return;
> +
> + if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND)
> + return;
> +
> + if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY))
> + mlx5_vdpa_warn(&ndev->mvdev, "modify to resume failed\n");
> +}
> +
> +static void resume_vqs(struct mlx5_vdpa_net *ndev)
> +{
> + for (int i = 0; i < ndev->mvdev.max_vqs; i++)
> + resume_vq(ndev, &ndev->vqs[i]);
> +}
> +
> static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> {
> if (!mvq->initialized)
> @@ -3256,6 +3290,21 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
> return 0;
> }
>
> +static int mlx5_vdpa_resume(struct vdpa_device *vdev)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +
> + mlx5_vdpa_info(mvdev, "resuming device\n");
> +
> + down_write(&ndev->reslock);
> + mvdev->suspended = false;
> + resume_vqs(ndev);
> + register_link_notifier(ndev);
> + up_write(&ndev->reslock);
> + return 0;
> +}
> +
> static int mlx5_set_group_asid(struct vdpa_device *vdev, u32 group,
> unsigned int asid)
> {
> @@ -3312,6 +3361,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .get_vq_dma_dev = mlx5_get_vq_dma_dev,
> .free = mlx5_vdpa_free,
> .suspend = mlx5_vdpa_suspend,
> + .resume = mlx5_vdpa_resume, /* Op disabled if not supported. */
> };
>
> static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> @@ -3683,6 +3733,9 @@ static int mlx5v_probe(struct auxiliary_device *adev,
> if (!MLX5_CAP_DEV_VDPA_EMULATION(mdev, desc_group_mkey_supported))
> mgtdev->vdpa_ops.get_vq_desc_group = NULL;
>
> + if (!MLX5_CAP_DEV_VDPA_EMULATION(mdev, freeze_to_rdy_supported))
> + mgtdev->vdpa_ops.resume = NULL;
> +
> err = vdpa_mgmtdev_register(&mgtdev->mgtdev);
> if (err)
> goto reg_err;
> --
> 2.42.0
>

2023-12-01 15:19:41

by Eugenio Perez Martin

Subject: Re: [PATCH vhost 6/7] vdpa/mlx5: Mark vq state for modification in hw vq

On Fri, Dec 1, 2023 at 11:50 AM Dragos Tatulea <[email protected]> wrote:
>
> .set_vq_state will set the indices and mark the fields to be modified in
> the hw vq.
>
> Signed-off-by: Dragos Tatulea <[email protected]>

Acked-by: Eugenio Pérez <[email protected]>

> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 8 ++++++++
> include/linux/mlx5/mlx5_ifc_vdpa.h | 2 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 2277daf4814f..6325aef045e2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1249,6 +1249,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> }
>
> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX)
> + MLX5_SET(virtio_net_q_object, obj_context, hw_available_index, mvq->avail_idx);
> +
> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX)
> + MLX5_SET(virtio_net_q_object, obj_context, hw_used_index, mvq->used_idx);
> +
> MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
> err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
> if (err)
> @@ -2328,6 +2334,8 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>
> mvq->used_idx = state->split.avail_index;
> mvq->avail_idx = state->split.avail_index;
> + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX |
> + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX;
> return 0;
> }
>
> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> index 9594ac405740..32e712106e68 100644
> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> @@ -146,6 +146,8 @@ enum {
> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_AVAIL_IDX = (u64)1 << 7,
> + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_USED_IDX = (u64)1 << 8,
> MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> };
>
> --
> 2.42.0
>

2023-12-01 15:27:12

by Dragos Tatulea

Subject: Re: [PATCH vhost 3/7] vdpa/mlx5: Allow modifying multiple vq fields in one modify command

On 12/01, Eugenio Perez Martin wrote:
> On Fri, Dec 1, 2023 at 11:49 AM Dragos Tatulea <[email protected]> wrote:
> >
> > Add a bitmask variable that tracks hw vq field changes that
> > are supposed to be modified on next hw vq change command.
> >
> > This will be useful to set multiple vq fields when resuming the vq.
> >
> > The state needs to remain as a parameter as it doesn't make sense to
> > make it part of the vq struct: fw_state is updated only after a
> > successful command.
> >
>
> I don't get this paragraph, "modified_fields" is a member of
> "mlx5_vdpa_virtqueue". Am I missing something?
>
I think this paragraph adds more confusion than clarification. I meant
to say that the state argument from modified_virtqueue needs to remain
there, as opposed to integrating it into the mlx5_vdpa_virtqueue struct.

>
> > Signed-off-by: Dragos Tatulea <[email protected]>
> > ---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 48 +++++++++++++++++++++++++------
> > 1 file changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 12ac3397f39b..d06285e46fe2 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -120,6 +120,9 @@ struct mlx5_vdpa_virtqueue {
> > u16 avail_idx;
> > u16 used_idx;
> > int fw_state;
> > +
> > + u64 modified_fields;
> > +
> > struct msi_map map;
> >
> > /* keep last in the struct */
> > @@ -1181,7 +1184,19 @@ static bool is_valid_state_change(int oldstate, int newstate)
> > }
> > }
> >
> > -static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, int state)
> > +static bool modifiable_virtqueue_fields(struct mlx5_vdpa_virtqueue *mvq)
> > +{
> > + /* Only state is always modifiable */
> > + if (mvq->modified_fields & ~MLX5_VIRTQ_MODIFY_MASK_STATE)
> > + return mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT ||
> > + mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
> > +
> > + return true;
> > +}
> > +
> > +static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > + struct mlx5_vdpa_virtqueue *mvq,
> > + int state)
> > {
> > int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
> > u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
> > @@ -1193,6 +1208,9 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > return 0;
> >
> > + if (!modifiable_virtqueue_fields(mvq))
> > + return -EINVAL;
> > +
>
> I'm ok with this change, but since modified_fields is (or will be) a
> bitmap tracking changes to state, addresses, mkey, etc, doesn't have
> more sense to check it like:
>
> if (modified_fields & 1<<change_1_flag)
> // perform change 1
> if (modified_fields & 1<<change_2_flag)
> // perform change 2
> if (modified_fields & 1<<change_3_flag)
> // perform change 13
> ---
>
> Instead of:
> if (!modified_fields)
> return
>
> if (modified_fields & 1<<change_1_flag)
> // perform change 1
> if (modified_fields & 1<<change_2_flag)
> // perform change 2
>
> // perform change 3, no checking, as it is the only possible value of
> modified_fields
> ---
>
> Or am I missing something?
>
modifiable_virtqueue_fields() is meant to check that the modification is
done only in the INIT or SUSPEND state of the queue. Or did I
misunderstand your question?

> The rest looks good to me.
>
Thanks for reviewing my patches Eugenio!

> > if (!is_valid_state_change(mvq->fw_state, state))
> > return -EINVAL;
> >
> > @@ -1208,17 +1226,28 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
> >
> > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
> > - MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select,
> > - MLX5_VIRTQ_MODIFY_MASK_STATE);
> > - MLX5_SET(virtio_net_q_object, obj_context, state, state);
> > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
> > + MLX5_SET(virtio_net_q_object, obj_context, state, state);
> > +
> > + MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
> > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
> > kfree(in);
> > if (!err)
> > mvq->fw_state = state;
> >
> > + mvq->modified_fields = 0;
> > +
> > return err;
> > }
> >
> > +static int modify_virtqueue_state(struct mlx5_vdpa_net *ndev,
> > + struct mlx5_vdpa_virtqueue *mvq,
> > + unsigned int state)
> > +{
> > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_STATE;
> > + return modify_virtqueue(ndev, mvq, state);
> > +}
> > +
> > static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> > {
> > u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
> > @@ -1347,7 +1376,7 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> > goto err_vq;
> >
> > if (mvq->ready) {
> > - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > if (err) {
> > mlx5_vdpa_warn(&ndev->mvdev, "failed to modify to ready vq idx %d(%d)\n",
> > idx, err);
> > @@ -1382,7 +1411,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> > if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
> > return;
> >
> > - if (modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> > + if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> > mlx5_vdpa_warn(&ndev->mvdev, "modify to suspend failed\n");
> >
> > if (query_virtqueue(ndev, mvq, &attr)) {
> > @@ -1407,6 +1436,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> > return;
> >
> > suspend_vq(ndev, mvq);
> > + mvq->modified_fields = 0;
> > destroy_virtqueue(ndev, mvq);
> > dealloc_vector(ndev, mvq);
> > counter_set_dealloc(ndev, mvq);
> > @@ -2207,7 +2237,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > if (!ready) {
> > suspend_vq(ndev, mvq);
> > } else {
> > - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > if (err) {
> > mlx5_vdpa_warn(mvdev, "modify VQ %d to ready failed (%d)\n", idx, err);
> > ready = false;
> > @@ -2804,8 +2834,10 @@ static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> > {
> > int i;
> >
> > - for (i = 0; i < ndev->mvdev.max_vqs; i++)
> > + for (i = 0; i < ndev->mvdev.max_vqs; i++) {
> > ndev->vqs[i].ready = false;
> > + ndev->vqs[i].modified_fields = 0;
> > + }
> >
> > ndev->mvdev.cvq.ready = false;
> > }
> > --
> > 2.42.0
> >
>

2023-12-02 20:27:01

by Michael S. Tsirkin

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> Add support for resumable vqs in the driver. This is a firmware feature
> that can be used for the following benefits:
> - Full device .suspend/.resume.
> - .set_map doesn't need to destroy and create new vqs anymore just to
> update the map. When resumable vqs are supported it is enough to
> suspend the vqs, set the new maps, and then resume the vqs.
>
> The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> needs to be applied to the mlx5-vhost tree [0] first.

I didn't get this. Why does this need to go through that tree?
Is there a dependency on some other commit from that tree?

> Once applied
> there, the change has to be pulled from mlx5-vhost into the vhost tree
> and only then the remaining patches can be applied. Same flow as the vq
> descriptor mappings patchset [1].
>
> To be able to use resumable vqs properly, support for selectively modifying
> vq parameters was needed. This is what the middle part of the series
> consists of.
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
> [1] https://lore.kernel.org/virtualization/[email protected]/
>
> Dragos Tatulea (7):
> vdpa/mlx5: Expose resumable vq capability
> vdpa/mlx5: Split function into locked and unlocked variants
> vdpa/mlx5: Allow modifying multiple vq fields in one modify command
> vdpa/mlx5: Introduce per vq and device resume
> vdpa/mlx5: Mark vq addrs for modification in hw vq
> vdpa/mlx5: Mark vq state for modification in hw vq
> vdpa/mlx5: Use vq suspend/resume during .set_map
>
> drivers/vdpa/mlx5/core/mr.c | 31 +++---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
> include/linux/mlx5/mlx5_ifc.h | 3 +-
> include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
> 4 files changed, 174 insertions(+), 36 deletions(-)
>
> --
> 2.42.0

2023-12-03 15:21:30

by Dragos Tatulea

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > Add support for resumable vqs in the driver. This is a firmware feature
> > that can be used for the following benefits:
> > - Full device .suspend/.resume.
> > - .set_map doesn't need to destroy and create new vqs anymore just to
> > update the map. When resumable vqs are supported it is enough to
> > suspend the vqs, set the new maps, and then resume the vqs.
> >
> > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > needs to be applied to the mlx5-vhost tree [0] first.
>
> I didn't get this. Why does this need to go through that tree?
> Is there a dependency on some other commit from that tree?
>
To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
the "vq descriptor mappings" patchset [1].

Thanks,
Dragos

> > Once applied
> > there, the change has to be pulled from mlx5-vhost into the vhost tree
> > and only then the remaining patches can be applied. Same flow as the vq
> > descriptor mappings patchset [1].
> >
> > To be able to use resumable vqs properly, support for selectively modifying
> > vq parameters was needed. This is what the middle part of the series
> > consists of.
> >
> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
> > [1] https://lore.kernel.org/virtualization/[email protected]/
> >
> > Dragos Tatulea (7):
> > vdpa/mlx5: Expose resumable vq capability
> > vdpa/mlx5: Split function into locked and unlocked variants
> > vdpa/mlx5: Allow modifying multiple vq fields in one modify command
> > vdpa/mlx5: Introduce per vq and device resume
> > vdpa/mlx5: Mark vq addrs for modification in hw vq
> > vdpa/mlx5: Mark vq state for modification in hw vq
> > vdpa/mlx5: Use vq suspend/resume during .set_map
> >
> > drivers/vdpa/mlx5/core/mr.c | 31 +++---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
> > include/linux/mlx5/mlx5_ifc.h | 3 +-
> > include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
> > 4 files changed, 174 insertions(+), 36 deletions(-)
> >
> > --
> > 2.42.0
>

2023-12-03 16:27:28

by Michael S. Tsirkin

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Sun, Dec 03, 2023 at 03:21:01PM +0000, Dragos Tatulea wrote:
> On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> > On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > > Add support for resumable vqs in the driver. This is a firmware feature
> > > that can be used for the following benefits:
> > > - Full device .suspend/.resume.
> > > - .set_map doesn't need to destroy and create new vqs anymore just to
> > > update the map. When resumable vqs are supported it is enough to
> > > suspend the vqs, set the new maps, and then resume the vqs.
> > >
> > > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > > needs to be applied to the mlx5-vhost tree [0] first.
> >
> > I didn't get this. Why does this need to go through that tree?
> > Is there a dependency on some other commit from that tree?
> >
> To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
> the "vq descriptor mappings" patchset [1].
>
> Thanks,
> Dragos

Are there other changes in that area that will cause non-trivial merge
conflicts?

> > > Once applied
> > > there, the change has to be pulled from mlx5-vhost into the vhost tree
> > > and only then the remaining patches can be applied. Same flow as the vq
> > > descriptor mappings patchset [1].
> > >
> > > To be able to use resumable vqs properly, support for selectively modifying
> > > vq parameters was needed. This is what the middle part of the series
> > > consists of.
> > >
> > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
> > > [1] https://lore.kernel.org/virtualization/[email protected]/
> > >
> > > Dragos Tatulea (7):
> > > vdpa/mlx5: Expose resumable vq capability
> > > vdpa/mlx5: Split function into locked and unlocked variants
> > > vdpa/mlx5: Allow modifying multiple vq fields in one modify command
> > > vdpa/mlx5: Introduce per vq and device resume
> > > vdpa/mlx5: Mark vq addrs for modification in hw vq
> > > vdpa/mlx5: Mark vq state for modification in hw vq
> > > vdpa/mlx5: Use vq suspend/resume during .set_map
> > >
> > > drivers/vdpa/mlx5/core/mr.c | 31 +++---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
> > > include/linux/mlx5/mlx5_ifc.h | 3 +-
> > > include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
> > > 4 files changed, 174 insertions(+), 36 deletions(-)
> > >
> > > --
> > > 2.42.0
> >
>

2023-12-04 08:53:42

by Dragos Tatulea

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Sun, 2023-12-03 at 11:23 -0500, Michael S. Tsirkin wrote:
> On Sun, Dec 03, 2023 at 03:21:01PM +0000, Dragos Tatulea wrote:
> > On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> > > On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > > > Add support for resumable vqs in the driver. This is a firmware feature
> > > > that can be used for the following benefits:
> > > > - Full device .suspend/.resume.
> > > > - .set_map doesn't need to destroy and create new vqs anymore just to
> > > > update the map. When resumable vqs are supported it is enough to
> > > > suspend the vqs, set the new maps, and then resume the vqs.
> > > >
> > > > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > > > needs to be applied to the mlx5-vhost tree [0] first.
> > >
> > > I didn't get this. Why does this need to go through that tree?
> > > Is there a dependency on some other commit from that tree?
> > >
> > To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
> > the "vq descriptor mappings" patchset [1].
> >
> > Thanks,
> > Dragos
>
> Are there other changes in that area that will cause non-trivial merge
> conflicts?
>
There are pending changes in mlx5_ifc.h for net-next. I haven't seen any changes
around the touched structure but I would prefer not to take any risk.

Thanks,
Dragos

> > > > Once applied
> > > > there, the change has to be pulled from mlx5-vhost into the vhost tree
> > > > and only then the remaining patches can be applied. Same flow as the vq
> > > > descriptor mappings patchset [1].
> > > >
> > > > To be able to use resumable vqs properly, support for selectively modifying
> > > > vq parameters was needed. This is what the middle part of the series
> > > > consists of.
> > > >
> > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
> > > > [1] https://lore.kernel.org/virtualization/[email protected]/
> > > >
> > > > Dragos Tatulea (7):
> > > > vdpa/mlx5: Expose resumable vq capability
> > > > vdpa/mlx5: Split function into locked and unlocked variants
> > > > vdpa/mlx5: Allow modifying multiple vq fields in one modify command
> > > > vdpa/mlx5: Introduce per vq and device resume
> > > > vdpa/mlx5: Mark vq addrs for modification in hw vq
> > > > vdpa/mlx5: Mark vq state for modification in hw vq
> > > > vdpa/mlx5: Use vq suspend/resume during .set_map
> > > >
> > > > drivers/vdpa/mlx5/core/mr.c | 31 +++---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
> > > > include/linux/mlx5/mlx5_ifc.h | 3 +-
> > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
> > > > 4 files changed, 174 insertions(+), 36 deletions(-)
> > > >
> > > > --
> > > > 2.42.0
> > >
> >
>

2023-12-04 08:55:24

by Michael S. Tsirkin

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Mon, Dec 04, 2023 at 08:53:26AM +0000, Dragos Tatulea wrote:
> On Sun, 2023-12-03 at 11:23 -0500, Michael S. Tsirkin wrote:
> > On Sun, Dec 03, 2023 at 03:21:01PM +0000, Dragos Tatulea wrote:
> > > On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> > > > On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > > > > Add support for resumable vqs in the driver. This is a firmware feature
> > > > > that can be used for the following benefits:
> > > > > - Full device .suspend/.resume.
> > > > > - .set_map doesn't need to destroy and create new vqs anymore just to
> > > > > update the map. When resumable vqs are supported it is enough to
> > > > > suspend the vqs, set the new maps, and then resume the vqs.
> > > > >
> > > > > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > > > > needs to be applied to the mlx5-vhost tree [0] first.
> > > >
> > > > I didn't get this. Why does this need to go through that tree?
> > > > Is there a dependency on some other commit from that tree?
> > > >
> > > To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
> > > the "vq descriptor mappings" patchset [1].
> > >
> > > Thanks,
> > > Dragos
> >
> > Are there other changes in that area that will cause non-trivial merge
> > conflicts?
> >
> There are pending changes in mlx5_ifc.h for net-next. I haven't seen any changes
> around the touched structure but I would prefer not to take any risk.
>
> Thanks,
> Dragos

This is exactly what linux-next is for.


> > > > > Once applied
> > > > > there, the change has to be pulled from mlx5-vhost into the vhost tree
> > > > > and only then the remaining patches can be applied. Same flow as the vq
> > > > > descriptor mappings patchset [1].
> > > > >
> > > > > To be able to use resumable vqs properly, support for selectively modifying
> > > > > vq parameters was needed. This is what the middle part of the series
> > > > > consists of.
> > > > >
> > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
> > > > > [1] https://lore.kernel.org/virtualization/[email protected]/
> > > > >
> > > > > Dragos Tatulea (7):
> > > > > vdpa/mlx5: Expose resumable vq capability
> > > > > vdpa/mlx5: Split function into locked and unlocked variants
> > > > > vdpa/mlx5: Allow modifying multiple vq fields in one modify command
> > > > > vdpa/mlx5: Introduce per vq and device resume
> > > > > vdpa/mlx5: Mark vq addrs for modification in hw vq
> > > > > vdpa/mlx5: Mark vq state for modification in hw vq
> > > > > vdpa/mlx5: Use vq suspend/resume during .set_map
> > > > >
> > > > > drivers/vdpa/mlx5/core/mr.c | 31 +++---
> > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
> > > > > include/linux/mlx5/mlx5_ifc.h | 3 +-
> > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
> > > > > 4 files changed, 174 insertions(+), 36 deletions(-)
> > > > >
> > > > > --
> > > > > 2.42.0
> > > >
> > >
> >
>

2023-12-04 09:16:17

by Dragos Tatulea

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Mon, 2023-12-04 at 03:55 -0500, Michael S. Tsirkin wrote:
> On Mon, Dec 04, 2023 at 08:53:26AM +0000, Dragos Tatulea wrote:
> > On Sun, 2023-12-03 at 11:23 -0500, Michael S. Tsirkin wrote:
> > > On Sun, Dec 03, 2023 at 03:21:01PM +0000, Dragos Tatulea wrote:
> > > > On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> > > > > On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > > > > > Add support for resumable vqs in the driver. This is a firmware feature
> > > > > > that can be used for the following benefits:
> > > > > > - Full device .suspend/.resume.
> > > > > > - .set_map doesn't need to destroy and create new vqs anymore just to
> > > > > > update the map. When resumable vqs are supported it is enough to
> > > > > > suspend the vqs, set the new maps, and then resume the vqs.
> > > > > >
> > > > > > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > > > > > needs to be applied to the mlx5-vhost tree [0] first.
> > > > >
> > > > > I didn't get this. Why does this need to go through that tree?
> > > > > Is there a dependency on some other commit from that tree?
> > > > >
> > > > To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
> > > > the "vq descriptor mappings" patchset [1].
> > > >
> > > > Thanks,
> > > > Dragos
> > >
> > > Are there other changes in that area that will cause non-trivial merge
> > > conflicts?
> > >
> > There are pending changes in mlx5_ifc.h for net-next. I haven't seen any changes
> > around the touched structure but I would prefer not to take any risk.
> >
> > Thanks,
> > Dragos
>
> This is exactly what linux-next is for.
>
Not sure what the suggestion is here. Is it:

1) To post patch 1/7 to net-next? Then we'd have to wait for a few weeks to make
sure that it gets into the next tree.

or

2) To apply it into the vhost tree directly? Then we run the risk of having
merge issues.

The "pull from branch" approach for cross subsystem changes was suggested by
Linus this merge issue.

[0]
https://lore.kernel.org/all/CA+55aFxxoO=i7neGBRGW_afHsSZ7K-x6fMO8v-8po3Ls_Ew0Rg@mail.gmail.com/

Thanks,
Dragos
>
> > > > > > Once applied
> > > > > > there, the change has to be pulled from mlx5-vhost into the vhost tree
> > > > > > and only then the remaining patches can be applied. Same flow as the vq
> > > > > > descriptor mappings patchset [1].
> > > > > >
> > > > > > To be able to use resumable vqs properly, support for selectively modifying
> > > > > > vq parameters was needed. This is what the middle part of the series
> > > > > > consists of.
> > > > > >
> > > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git/log/?h=mlx5-vhost
> > > > > > [1] https://lore.kernel.org/virtualization/[email protected]/
> > > > > >
> > > > > > Dragos Tatulea (7):
> > > > > > vdpa/mlx5: Expose resumable vq capability
> > > > > > vdpa/mlx5: Split function into locked and unlocked variants
> > > > > > vdpa/mlx5: Allow modifying multiple vq fields in one modify command
> > > > > > vdpa/mlx5: Introduce per vq and device resume
> > > > > > vdpa/mlx5: Mark vq addrs for modification in hw vq
> > > > > > vdpa/mlx5: Mark vq state for modification in hw vq
> > > > > > vdpa/mlx5: Use vq suspend/resume during .set_map
> > > > > >
> > > > > > drivers/vdpa/mlx5/core/mr.c | 31 +++---
> > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 172 +++++++++++++++++++++++++----
> > > > > > include/linux/mlx5/mlx5_ifc.h | 3 +-
> > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 4 +
> > > > > > 4 files changed, 174 insertions(+), 36 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.42.0
> > > > >
> > > >
> > >
> >
>

2023-12-04 11:05:05

by Michael S. Tsirkin

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Mon, Dec 04, 2023 at 09:16:07AM +0000, Dragos Tatulea wrote:
> On Mon, 2023-12-04 at 03:55 -0500, Michael S. Tsirkin wrote:
> > On Mon, Dec 04, 2023 at 08:53:26AM +0000, Dragos Tatulea wrote:
> > > On Sun, 2023-12-03 at 11:23 -0500, Michael S. Tsirkin wrote:
> > > > On Sun, Dec 03, 2023 at 03:21:01PM +0000, Dragos Tatulea wrote:
> > > > > On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> > > > > > On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > > > > > > Add support for resumable vqs in the driver. This is a firmware feature
> > > > > > > that can be used for the following benefits:
> > > > > > > - Full device .suspend/.resume.
> > > > > > > - .set_map doesn't need to destroy and create new vqs anymore just to
> > > > > > > update the map. When resumable vqs are supported it is enough to
> > > > > > > suspend the vqs, set the new maps, and then resume the vqs.
> > > > > > >
> > > > > > > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > > > > > > needs to be applied to the mlx5-vhost tree [0] first.
> > > > > >
> > > > > > I didn't get this. Why does this need to go through that tree?
> > > > > > Is there a dependency on some other commit from that tree?
> > > > > >
> > > > > To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
> > > > > the "vq descriptor mappings" patchset [1].
> > > > >
> > > > > Thanks,
> > > > > Dragos
> > > >
> > > > Are there other changes in that area that will cause non-trivial merge
> > > > conflicts?
> > > >
> > > There are pending changes in mlx5_ifc.h for net-next. I haven't seen any changes
> > > around the touched structure but I would prefer not to take any risk.
> > >
> > > Thanks,
> > > Dragos
> >
> > This is exactly what linux-next is for.
> >
> Not sure what the suggestion is here. Is it:
>
> 1) To post patch 1/7 to net-next? Then we'd have to wait for a few weeks to make
> sure that it gets into the next tree.
>
> or
>
> 2) To apply it into the vhost tree directly? Then we run the risk of having
> merge issues.
>
> The "pull from branch" approach for cross subsystem changes was suggested by
> Linus this merge issue.
>
> [0]
> https://lore.kernel.org/all/CA+55aFxxoO=i7neGBRGW_afHsSZ7K-x6fMO8v-8po3Ls_Ew0Rg@mail.gmail.com/
>
> Thanks,
> Dragos

I will park this in my tree for now so it can get testing in linux next.
When it's available in some other tree as well, let me know and
I'll figure it out.

--
MST

2023-12-04 11:18:26

by Dragos Tatulea

Subject: Re: [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Mon, 2023-12-04 at 06:04 -0500, Michael S. Tsirkin wrote:
> On Mon, Dec 04, 2023 at 09:16:07AM +0000, Dragos Tatulea wrote:
> > On Mon, 2023-12-04 at 03:55 -0500, Michael S. Tsirkin wrote:
> > > On Mon, Dec 04, 2023 at 08:53:26AM +0000, Dragos Tatulea wrote:
> > > > On Sun, 2023-12-03 at 11:23 -0500, Michael S. Tsirkin wrote:
> > > > > On Sun, Dec 03, 2023 at 03:21:01PM +0000, Dragos Tatulea wrote:
> > > > > > On Sat, 2023-12-02 at 15:26 -0500, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Dec 01, 2023 at 12:48:50PM +0200, Dragos Tatulea wrote:
> > > > > > > > Add support for resumable vqs in the driver. This is a firmware feature
> > > > > > > > that can be used for the following benefits:
> > > > > > > > - Full device .suspend/.resume.
> > > > > > > > - .set_map doesn't need to destroy and create new vqs anymore just to
> > > > > > > > update the map. When resumable vqs are supported it is enough to
> > > > > > > > suspend the vqs, set the new maps, and then resume the vqs.
> > > > > > > >
> > > > > > > > The first patch exposes the relevant bits in mlx5_ifc.h. That means it
> > > > > > > > needs to be applied to the mlx5-vhost tree [0] first.
> > > > > > >
> > > > > > > I didn't get this. Why does this need to go through that tree?
> > > > > > > Is there a dependency on some other commit from that tree?
> > > > > > >
> > > > > > To avoid merge issues in Linus's tree in mlx5_ifc.h. The idea is the same as for
> > > > > > the "vq descriptor mappings" patchset [1].
> > > > > >
> > > > > > Thanks,
> > > > > > Dragos
> > > > >
> > > > > Are there other changes in that area that will cause non-trivial merge
> > > > > conflicts?
> > > > >
> > > > There are pending changes in mlx5_ifc.h for net-next. I haven't seen any changes
> > > > around the touched structure but I would prefer not to take any risk.
> > > >
> > > > Thanks,
> > > > Dragos
> > >
> > > This is exactly what linux-next is for.
> > >
> > Not sure what the suggestion is here. Is it:
> >
> > 1) To post patch 1/7 to net-next? Then we'd have to wait for a few weeks to make
> > sure that it gets into the next tree.
> >
> > or
> >
> > 2) To apply it into the vhost tree directly? Then we run the risk of having
> > merge issues.
> >
> > The "pull from branch" approach for cross subsystem changes was suggested by
> > Linus this merge issue.
> >
> > [0]
> > https://lore.kernel.org/all/CA+55aFxxoO=i7neGBRGW_afHsSZ7K-x6fMO8v-8po3Ls_Ew0Rg@mail.gmail.com/
> >
> > Thanks,
> > Dragos
>
> I will park this in my tree for now so it can get testing in linux next.
> When it's available in some other tree as well, let me know and
> I'll figure it out.
>
Thanks! But don't park it just yet. I would like to send a v2 in the next few
days that contains 2 more patches on top. I've sent the v1 early to get reviews
for the bulk of the work. Forgot to mention that in the cover letter. My bad,
sorry.

Thanks,
Dragos

2023-12-04 13:11:51

by Leon Romanovsky

Subject: Re: (subset) [PATCH vhost 0/7] vdpa/mlx5: Add support for resumable vqs

On Fri, 01 Dec 2023 12:48:50 +0200, Dragos Tatulea wrote:
> Add support for resumable vqs in the driver. This is a firmware feature
> that can be used for the following benefits:
> - Full device .suspend/.resume.
> - .set_map doesn't need to destroy and create new vqs anymore just to
> update the map. When resumable vqs are supported it is enough to
> suspend the vqs, set the new maps, and then resume the vqs.
>
> [...]

Applied, thanks!

[1/7] vdpa/mlx5: Expose resumable vq capability
https://git.kernel.org/rdma/rdma/c/b24910e1be0e76

Best regards,
--
Leon Romanovsky <[email protected]>