Implement the suspend callback allowing to suspend the virtqueues so
they stop processing descriptors. This is required to allow the shadow
virtqueue to kick in.
Signed-off-by: Eli Cohen <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
2 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index fb0b23e71383..ea4bc8a0cd25 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
if (err)
goto err_cmd;
+ mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
kfree(in);
mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
@@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
return;
}
+ mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
umems_destroy(ndev, mvq);
}
@@ -1121,6 +1123,20 @@ 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)
+{
+ switch (oldstate) {
+ case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
+ return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
+ 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:
+ case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
+ default:
+ return false;
+ }
+}
+
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);
@@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
void *in;
int err;
+ if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
+ return 0;
+
+ if (!is_valid_state_change(mvq->fw_state, state))
+ return -EINVAL;
+
in = kzalloc(inlen, GFP_KERNEL);
if (!in)
return -ENOMEM;
@@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
struct mlx5_vdpa_virtqueue *mvq;
+ int err;
if (!mvdev->actual_features)
return;
@@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
}
mvq = &ndev->vqs[idx];
- if (!ready)
+ if (!ready) {
suspend_vq(ndev, mvq);
+ } else {
+ err = modify_virtqueue(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;
+ }
+ }
+
mvq->ready = ready;
}
@@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
return err;
}
+static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
+{
+ struct mlx5_control_vq *cvq;
+
+ if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
+ return;
+
+ cvq = &mvdev->cvq;
+ cvq->ready = !suspend;
+}
+
+static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
+{
+ struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+ struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+ struct mlx5_vdpa_virtqueue *mvq;
+ int i;
+
+ if (!suspend) {
+ mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ down_write(&ndev->reslock);
+ for (i = 0; i < ndev->cur_num_vqs; i++) {
+ mvq = &ndev->vqs[i];
+ suspend_vq(ndev, mvq);
+ }
+ mlx5_vdpa_cvq_suspend(mvdev, suspend);
+ up_write(&ndev->reslock);
+ return 0;
+}
+
static const struct vdpa_config_ops mlx5_vdpa_ops = {
.set_vq_address = mlx5_vdpa_set_vq_address,
.set_vq_num = mlx5_vdpa_set_vq_num,
@@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
.get_generation = mlx5_vdpa_get_generation,
.set_map = mlx5_vdpa_set_map,
.free = mlx5_vdpa_free,
+ .suspend = mlx5_vdpa_suspend,
};
static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
@@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
mvq->index = i;
mvq->ndev = ndev;
mvq->fwqp.fw = true;
+ mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
}
for (; i < ndev->mvdev.max_vqs; i++) {
mvq = &ndev->vqs[i];
diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
index 4414ed5b6ed2..423562f39d3c 100644
--- a/include/linux/mlx5/mlx5_ifc_vdpa.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -150,6 +150,14 @@ enum {
MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
};
+/* This indicates that the object was not created or has alreadyi
+ * been desroyed. It is very safe to assume that this object will never
+ * have so many states
+ */
+enum {
+ MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
+};
+
enum {
MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
--
2.35.1
On Thu, Jun 16, 2022 at 3:27 PM Eli Cohen <[email protected]> wrote:
>
> Implement the suspend callback allowing to suspend the virtqueues so
> they stop processing descriptors. This is required to allow the shadow
> virtqueue to kick in.
>
Maybe a more general description is "To get a meaningful virtqueue
state in live migration, trusting the device will not modify it from
the moment it is suspended"?
> Signed-off-by: Eli Cohen <[email protected]>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> 2 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fb0b23e71383..ea4bc8a0cd25 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> if (err)
> goto err_cmd;
>
> + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> kfree(in);
> mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
>
> @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> return;
> }
> + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> umems_destroy(ndev, mvq);
> }
>
> @@ -1121,6 +1123,20 @@ 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)
> +{
> + switch (oldstate) {
> + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> + 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:
> + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> + default:
> + return false;
> + }
> +}
> +
> 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);
> @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> void *in;
> int err;
>
> + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> + return 0;
> +
> + if (!is_valid_state_change(mvq->fw_state, state))
> + return -EINVAL;
> +
> in = kzalloc(inlen, GFP_KERNEL);
> if (!in)
> return -ENOMEM;
> @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> struct mlx5_vdpa_virtqueue *mvq;
> + int err;
>
> if (!mvdev->actual_features)
> return;
> @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> }
>
> mvq = &ndev->vqs[idx];
> - if (!ready)
> + if (!ready) {
> suspend_vq(ndev, mvq);
> + } else {
> + err = modify_virtqueue(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;
> + }
> + }
> +
>
> mvq->ready = ready;
> }
> @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> return err;
> }
>
> +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> +{
> + struct mlx5_control_vq *cvq;
> +
> + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> + return;
> +
> + cvq = &mvdev->cvq;
> + cvq->ready = !suspend;
> +}
> +
> +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> + struct mlx5_vdpa_virtqueue *mvq;
> + int i;
> +
> + if (!suspend) {
> + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + down_write(&ndev->reslock);
> + for (i = 0; i < ndev->cur_num_vqs; i++) {
> + mvq = &ndev->vqs[i];
> + suspend_vq(ndev, mvq);
> + }
> + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> + up_write(&ndev->reslock);
> + return 0;
> +}
> +
> static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .set_vq_address = mlx5_vdpa_set_vq_address,
> .set_vq_num = mlx5_vdpa_set_vq_num,
> @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .get_generation = mlx5_vdpa_get_generation,
> .set_map = mlx5_vdpa_set_map,
> .free = mlx5_vdpa_free,
> + .suspend = mlx5_vdpa_suspend,
> };
>
> static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> mvq->index = i;
> mvq->ndev = ndev;
> mvq->fwqp.fw = true;
> + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> }
> for (; i < ndev->mvdev.max_vqs; i++) {
> mvq = &ndev->vqs[i];
> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> index 4414ed5b6ed2..423562f39d3c 100644
> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> @@ -150,6 +150,14 @@ enum {
> MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> };
>
> +/* This indicates that the object was not created or has alreadyi
> + * been desroyed. It is very safe to assume that this object will never
Small typos: "already been destroyed".
> + * have so many states
> + */
> +enum {
> + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> +};
> +
> enum {
> MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> --
> 2.35.1
>
On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
>
> Implement the suspend callback allowing to suspend the virtqueues so
> they stop processing descriptors. This is required to allow the shadow
> virtqueue to kick in.
>
> Signed-off-by: Eli Cohen <[email protected]>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> 2 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fb0b23e71383..ea4bc8a0cd25 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> if (err)
> goto err_cmd;
>
> + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> kfree(in);
> mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
>
> @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> return;
> }
> + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> umems_destroy(ndev, mvq);
> }
>
> @@ -1121,6 +1123,20 @@ 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)
> +{
> + switch (oldstate) {
> + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> + 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:
> + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> + default:
> + return false;
> + }
> +}
> +
> 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);
> @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> void *in;
> int err;
>
> + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> + return 0;
> +
> + if (!is_valid_state_change(mvq->fw_state, state))
> + return -EINVAL;
> +
> in = kzalloc(inlen, GFP_KERNEL);
> if (!in)
> return -ENOMEM;
> @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> struct mlx5_vdpa_virtqueue *mvq;
> + int err;
>
> if (!mvdev->actual_features)
> return;
> @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> }
>
> mvq = &ndev->vqs[idx];
> - if (!ready)
> + if (!ready) {
> suspend_vq(ndev, mvq);
> + } else {
> + err = modify_virtqueue(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;
> + }
> + }
> +
>
> mvq->ready = ready;
> }
> @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> return err;
> }
>
> +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> +{
> + struct mlx5_control_vq *cvq;
> +
> + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> + return;
> +
> + cvq = &mvdev->cvq;
> + cvq->ready = !suspend;
> +}
It looks to me we need to synchronize this with reslock. And this
probably deserve a dedicated fix.
> +
> +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> + struct mlx5_vdpa_virtqueue *mvq;
> + int i;
> +
> + if (!suspend) {
> + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + down_write(&ndev->reslock);
> + for (i = 0; i < ndev->cur_num_vqs; i++) {
> + mvq = &ndev->vqs[i];
> + suspend_vq(ndev, mvq);
> + }
> + mlx5_vdpa_cvq_suspend(mvdev, suspend);
Do we need to synchronize with the carrier work here? Otherwise we may
get config notification after suspending.
> + up_write(&ndev->reslock);
> + return 0;
> +}
> +
> static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .set_vq_address = mlx5_vdpa_set_vq_address,
> .set_vq_num = mlx5_vdpa_set_vq_num,
> @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> .get_generation = mlx5_vdpa_get_generation,
> .set_map = mlx5_vdpa_set_map,
> .free = mlx5_vdpa_free,
> + .suspend = mlx5_vdpa_suspend,
I don't see the vDPA bus patch to enable this method. Or anything I missed here?
Thanks
> };
>
> static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> mvq->index = i;
> mvq->ndev = ndev;
> mvq->fwqp.fw = true;
> + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> }
> for (; i < ndev->mvdev.max_vqs; i++) {
> mvq = &ndev->vqs[i];
> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> index 4414ed5b6ed2..423562f39d3c 100644
> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> @@ -150,6 +150,14 @@ enum {
> MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> };
>
> +/* This indicates that the object was not created or has alreadyi
> + * been desroyed. It is very safe to assume that this object will never
> + * have so many states
> + */
> +enum {
> + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> +};
> +
> enum {
> MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> --
> 2.35.1
>
On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
>
> On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> >
> > Implement the suspend callback allowing to suspend the virtqueues so
> > they stop processing descriptors. This is required to allow the shadow
> > virtqueue to kick in.
> >
> > Signed-off-by: Eli Cohen <[email protected]>
> > ---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > 2 files changed, 75 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index fb0b23e71383..ea4bc8a0cd25 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > if (err)
> > goto err_cmd;
> >
> > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > kfree(in);
> > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> >
> > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > return;
> > }
> > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > umems_destroy(ndev, mvq);
> > }
> >
> > @@ -1121,6 +1123,20 @@ 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)
> > +{
> > + switch (oldstate) {
> > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > + 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:
> > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > 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);
> > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > void *in;
> > int err;
> >
> > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > + return 0;
> > +
> > + if (!is_valid_state_change(mvq->fw_state, state))
> > + return -EINVAL;
> > +
> > in = kzalloc(inlen, GFP_KERNEL);
> > if (!in)
> > return -ENOMEM;
> > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > struct mlx5_vdpa_virtqueue *mvq;
> > + int err;
> >
> > if (!mvdev->actual_features)
> > return;
> > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > }
> >
> > mvq = &ndev->vqs[idx];
> > - if (!ready)
> > + if (!ready) {
> > suspend_vq(ndev, mvq);
> > + } else {
> > + err = modify_virtqueue(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;
> > + }
> > + }
> > +
> >
> > mvq->ready = ready;
> > }
> > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > return err;
> > }
> >
> > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > +{
> > + struct mlx5_control_vq *cvq;
> > +
> > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > + return;
> > +
> > + cvq = &mvdev->cvq;
> > + cvq->ready = !suspend;
> > +}
>
> It looks to me we need to synchronize this with reslock. And this
> probably deserve a dedicated fix.
>
> > +
> > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > +{
> > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > + struct mlx5_vdpa_virtqueue *mvq;
> > + int i;
> > +
> > + if (!suspend) {
> > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + down_write(&ndev->reslock);
> > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > + mvq = &ndev->vqs[i];
> > + suspend_vq(ndev, mvq);
> > + }
> > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
>
> Do we need to synchronize with the carrier work here? Otherwise we may
> get config notification after suspending.
>
> > + up_write(&ndev->reslock);
> > + return 0;
> > +}
> > +
> > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > .set_vq_address = mlx5_vdpa_set_vq_address,
> > .set_vq_num = mlx5_vdpa_set_vq_num,
> > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > .get_generation = mlx5_vdpa_get_generation,
> > .set_map = mlx5_vdpa_set_map,
> > .free = mlx5_vdpa_free,
> > + .suspend = mlx5_vdpa_suspend,
>
> I don't see the vDPA bus patch to enable this method. Or anything I missed here?
>
Should we add
Based-on: <[email protected]>
To this series?
> Thanks
>
> > };
> >
> > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > mvq->index = i;
> > mvq->ndev = ndev;
> > mvq->fwqp.fw = true;
> > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > }
> > for (; i < ndev->mvdev.max_vqs; i++) {
> > mvq = &ndev->vqs[i];
> > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > index 4414ed5b6ed2..423562f39d3c 100644
> > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > @@ -150,6 +150,14 @@ enum {
> > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > };
> >
> > +/* This indicates that the object was not created or has alreadyi
> > + * been desroyed. It is very safe to assume that this object will never
> > + * have so many states
> > + */
> > +enum {
> > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > +};
> > +
> > enum {
> > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > --
> > 2.35.1
> >
>
> +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> + struct mlx5_vdpa_virtqueue *mvq;
> + int i;
> +
> + if (!suspend) {
> + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
If the resume part it's a problem, maybe we can split the vdpa_sim
series so it only adds a callback to suspend() the device. If needed,
we can add a resume() later. suspend is the only operation we need to
perform LM.
Thoughts on this?
Thanks!
> + return -EOPNOTSUPP;
> + }
> +
> + down_write(&ndev->reslock);
> + for (i = 0; i < ndev->cur_num_vqs; i++) {
> + mvq = &ndev->vqs[i];
> + suspend_vq(ndev, mvq);
> + }
> + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> + up_write(&ndev->reslock);
> + return 0;
> +}
> +
On Mon, Jun 20, 2022 at 11:58:33AM +0200, Eugenio Perez Martin wrote:
> On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> >
> > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > >
> > > Implement the suspend callback allowing to suspend the virtqueues so
> > > they stop processing descriptors. This is required to allow the shadow
> > > virtqueue to kick in.
> > >
> > > Signed-off-by: Eli Cohen <[email protected]>
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > if (err)
> > > goto err_cmd;
> > >
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > kfree(in);
> > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > >
> > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > return;
> > > }
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > umems_destroy(ndev, mvq);
> > > }
> > >
> > > @@ -1121,6 +1123,20 @@ 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)
> > > +{
> > > + switch (oldstate) {
> > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > + 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:
> > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > + default:
> > > + return false;
> > > + }
> > > +}
> > > +
> > > 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);
> > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > void *in;
> > > int err;
> > >
> > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > + return 0;
> > > +
> > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > + return -EINVAL;
> > > +
> > > in = kzalloc(inlen, GFP_KERNEL);
> > > if (!in)
> > > return -ENOMEM;
> > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > struct mlx5_vdpa_virtqueue *mvq;
> > > + int err;
> > >
> > > if (!mvdev->actual_features)
> > > return;
> > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > }
> > >
> > > mvq = &ndev->vqs[idx];
> > > - if (!ready)
> > > + if (!ready) {
> > > suspend_vq(ndev, mvq);
> > > + } else {
> > > + err = modify_virtqueue(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;
> > > + }
> > > + }
> > > +
> > >
> > > mvq->ready = ready;
> > > }
> > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > return err;
> > > }
> > >
> > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > +{
> > > + struct mlx5_control_vq *cvq;
> > > +
> > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > + return;
> > > +
> > > + cvq = &mvdev->cvq;
> > > + cvq->ready = !suspend;
> > > +}
> >
> > It looks to me we need to synchronize this with reslock. And this
> > probably deserve a dedicated fix.
> >
> > > +
> > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > +{
> > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > + struct mlx5_vdpa_virtqueue *mvq;
> > > + int i;
> > > +
> > > + if (!suspend) {
> > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + down_write(&ndev->reslock);
> > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > + mvq = &ndev->vqs[i];
> > > + suspend_vq(ndev, mvq);
> > > + }
> > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> >
> > Do we need to synchronize with the carrier work here? Otherwise we may
> > get config notification after suspending.
> >
> > > + up_write(&ndev->reslock);
> > > + return 0;
> > > +}
> > > +
> > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > .get_generation = mlx5_vdpa_get_generation,
> > > .set_map = mlx5_vdpa_set_map,
> > > .free = mlx5_vdpa_free,
> > > + .suspend = mlx5_vdpa_suspend,
> >
> > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> >
>
> Should we add
> Based-on: <[email protected]>
>
> To this series?
If it's based on your patch then mentioning this in the log and
including the S.O.B. is customary. what would this tag add?
was there relevant discussion?
> > Thanks
> >
> > > };
> > >
> > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > mvq->index = i;
> > > mvq->ndev = ndev;
> > > mvq->fwqp.fw = true;
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > }
> > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > mvq = &ndev->vqs[i];
> > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > index 4414ed5b6ed2..423562f39d3c 100644
> > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > @@ -150,6 +150,14 @@ enum {
> > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > };
> > >
> > > +/* This indicates that the object was not created or has alreadyi
> > > + * been desroyed. It is very safe to assume that this object will never
> > > + * have so many states
> > > + */
> > > +enum {
> > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > +};
> > > +
> > > enum {
> > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > --
> > > 2.35.1
> > >
> >
On Mon, Jun 20, 2022 at 12:07 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Mon, Jun 20, 2022 at 11:58:33AM +0200, Eugenio Perez Martin wrote:
> > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > >
> > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > they stop processing descriptors. This is required to allow the shadow
> > > > virtqueue to kick in.
> > > >
> > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > if (err)
> > > > goto err_cmd;
> > > >
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > kfree(in);
> > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > >
> > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > return;
> > > > }
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > umems_destroy(ndev, mvq);
> > > > }
> > > >
> > > > @@ -1121,6 +1123,20 @@ 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)
> > > > +{
> > > > + switch (oldstate) {
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > + 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:
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > + default:
> > > > + return false;
> > > > + }
> > > > +}
> > > > +
> > > > 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);
> > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > void *in;
> > > > int err;
> > > >
> > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > + return 0;
> > > > +
> > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > + return -EINVAL;
> > > > +
> > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > if (!in)
> > > > return -ENOMEM;
> > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > + int err;
> > > >
> > > > if (!mvdev->actual_features)
> > > > return;
> > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > }
> > > >
> > > > mvq = &ndev->vqs[idx];
> > > > - if (!ready)
> > > > + if (!ready) {
> > > > suspend_vq(ndev, mvq);
> > > > + } else {
> > > > + err = modify_virtqueue(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;
> > > > + }
> > > > + }
> > > > +
> > > >
> > > > mvq->ready = ready;
> > > > }
> > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > return err;
> > > > }
> > > >
> > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > +{
> > > > + struct mlx5_control_vq *cvq;
> > > > +
> > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > + return;
> > > > +
> > > > + cvq = &mvdev->cvq;
> > > > + cvq->ready = !suspend;
> > > > +}
> > >
> > > It looks to me we need to synchronize this with reslock. And this
> > > probably deserve a dedicated fix.
> > >
> > > > +
> > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > +{
> > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > + int i;
> > > > +
> > > > + if (!suspend) {
> > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + down_write(&ndev->reslock);
> > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > + mvq = &ndev->vqs[i];
> > > > + suspend_vq(ndev, mvq);
> > > > + }
> > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > >
> > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > get config notification after suspending.
> > >
> > > > + up_write(&ndev->reslock);
> > > > + return 0;
> > > > +}
> > > > +
> > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .get_generation = mlx5_vdpa_get_generation,
> > > > .set_map = mlx5_vdpa_set_map,
> > > > .free = mlx5_vdpa_free,
> > > > + .suspend = mlx5_vdpa_suspend,
> > >
> > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > >
> >
> > Should we add
> > Based-on: <[email protected]>
> >
> > To this series?
>
> If it's based on your patch then mentioning this in the log and
> including the S.O.B. is customary. what would this tag add?
> was there relevant discussion?
>
Sorry I think I need to expand it. I was using the meaning of qemu's
submitting patches guide, which is:
It is also okay to base patches on top of other on-going work that is
not yet part of the git master branch.
So these patches are not modifications of my patches, they should be
applied on top of that series.
My series is the one that adds the "vdpa bus method" Jason is
referring to (.suspend). That series is able to suspend the vdpa net
simulator by itself. If we apply this series *on top* of my previous
series, it gives us the vdpa op to suspend the mlx device.
Thanks!
>
> > > Thanks
> > >
> > > > };
> > > >
> > > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > > mvq->index = i;
> > > > mvq->ndev = ndev;
> > > > mvq->fwqp.fw = true;
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > }
> > > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > > mvq = &ndev->vqs[i];
> > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > index 4414ed5b6ed2..423562f39d3c 100644
> > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > @@ -150,6 +150,14 @@ enum {
> > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > > };
> > > >
> > > > +/* This indicates that the object was not created or has alreadyi
> > > > + * been desroyed. It is very safe to assume that this object will never
> > > > + * have so many states
> > > > + */
> > > > +enum {
> > > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > > +};
> > > > +
> > > > enum {
> > > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > > --
> > > > 2.35.1
> > > >
> > >
>
> -----Original Message-----
> From: Jason Wang <[email protected]>
> Sent: Monday, June 20, 2022 11:56 AM
> To: Eli Cohen <[email protected]>
> Cc: eperezma <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-
> kernel <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
>
> On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> >
> > Implement the suspend callback allowing to suspend the virtqueues so
> > they stop processing descriptors. This is required to allow the shadow
> > virtqueue to kick in.
> >
> > Signed-off-by: Eli Cohen <[email protected]>
> > ---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > 2 files changed, 75 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index fb0b23e71383..ea4bc8a0cd25 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > if (err)
> > goto err_cmd;
> >
> > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > kfree(in);
> > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> >
> > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > return;
> > }
> > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > umems_destroy(ndev, mvq);
> > }
> >
> > @@ -1121,6 +1123,20 @@ 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)
> > +{
> > + switch (oldstate) {
> > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > + 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:
> > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > 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);
> > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > void *in;
> > int err;
> >
> > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > + return 0;
> > +
> > + if (!is_valid_state_change(mvq->fw_state, state))
> > + return -EINVAL;
> > +
> > in = kzalloc(inlen, GFP_KERNEL);
> > if (!in)
> > return -ENOMEM;
> > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > struct mlx5_vdpa_virtqueue *mvq;
> > + int err;
> >
> > if (!mvdev->actual_features)
> > return;
> > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > }
> >
> > mvq = &ndev->vqs[idx];
> > - if (!ready)
> > + if (!ready) {
> > suspend_vq(ndev, mvq);
> > + } else {
> > + err = modify_virtqueue(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;
> > + }
> > + }
> > +
> >
> > mvq->ready = ready;
> > }
> > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > return err;
> > }
> >
> > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > +{
> > + struct mlx5_control_vq *cvq;
> > +
> > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > + return;
> > +
> > + cvq = &mvdev->cvq;
> > + cvq->ready = !suspend;
> > +}
>
> It looks to me we need to synchronize this with reslock. And this
> probably deserve a dedicated fix.
>
It's already being held by mlx5_vdpa_suspend
> > +
> > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > +{
> > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > + struct mlx5_vdpa_virtqueue *mvq;
> > + int i;
> > +
> > + if (!suspend) {
> > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + down_write(&ndev->reslock);
> > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > + mvq = &ndev->vqs[i];
> > + suspend_vq(ndev, mvq);
> > + }
> > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
>
> Do we need to synchronize with the carrier work here? Otherwise we may
> get config notification after suspending.
>
Are you saying we should not allow carrier updates after the VQs have been suspended?
Link state should not be related to suspension of VQs.
> > + up_write(&ndev->reslock);
> > + return 0;
> > +}
> > +
> > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > .set_vq_address = mlx5_vdpa_set_vq_address,
> > .set_vq_num = mlx5_vdpa_set_vq_num,
> > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > .get_generation = mlx5_vdpa_get_generation,
> > .set_map = mlx5_vdpa_set_map,
> > .free = mlx5_vdpa_free,
> > + .suspend = mlx5_vdpa_suspend,
>
> I don't see the vDPA bus patch to enable this method. Or anything I missed here?
>
> Thanks
>
> > };
> >
> > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > mvq->index = i;
> > mvq->ndev = ndev;
> > mvq->fwqp.fw = true;
> > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > }
> > for (; i < ndev->mvdev.max_vqs; i++) {
> > mvq = &ndev->vqs[i];
> > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > index 4414ed5b6ed2..423562f39d3c 100644
> > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > @@ -150,6 +150,14 @@ enum {
> > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > };
> >
> > +/* This indicates that the object was not created or has alreadyi
> > + * been desroyed. It is very safe to assume that this object will never
> > + * have so many states
> > + */
> > +enum {
> > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > +};
> > +
> > enum {
> > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > --
> > 2.35.1
> >
On Mon, Jun 20, 2022 at 9:09 PM Eli Cohen <[email protected]> wrote:
>
> > -----Original Message-----
> > From: Jason Wang <[email protected]>
> > Sent: Monday, June 20, 2022 11:56 AM
> > To: Eli Cohen <[email protected]>
> > Cc: eperezma <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-
> > kernel <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> >
> > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > >
> > > Implement the suspend callback allowing to suspend the virtqueues so
> > > they stop processing descriptors. This is required to allow the shadow
> > > virtqueue to kick in.
> > >
> > > Signed-off-by: Eli Cohen <[email protected]>
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > if (err)
> > > goto err_cmd;
> > >
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > kfree(in);
> > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > >
> > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > return;
> > > }
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > umems_destroy(ndev, mvq);
> > > }
> > >
> > > @@ -1121,6 +1123,20 @@ 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)
> > > +{
> > > + switch (oldstate) {
> > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > + 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:
> > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > + default:
> > > + return false;
> > > + }
> > > +}
> > > +
> > > 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);
> > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > void *in;
> > > int err;
> > >
> > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > + return 0;
> > > +
> > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > + return -EINVAL;
> > > +
> > > in = kzalloc(inlen, GFP_KERNEL);
> > > if (!in)
> > > return -ENOMEM;
> > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > struct mlx5_vdpa_virtqueue *mvq;
> > > + int err;
> > >
> > > if (!mvdev->actual_features)
> > > return;
> > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > }
> > >
> > > mvq = &ndev->vqs[idx];
> > > - if (!ready)
> > > + if (!ready) {
> > > suspend_vq(ndev, mvq);
> > > + } else {
> > > + err = modify_virtqueue(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;
> > > + }
> > > + }
> > > +
> > >
> > > mvq->ready = ready;
> > > }
> > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > return err;
> > > }
> > >
> > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > +{
> > > + struct mlx5_control_vq *cvq;
> > > +
> > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > + return;
> > > +
> > > + cvq = &mvdev->cvq;
> > > + cvq->ready = !suspend;
> > > +}
> >
> > It looks to me we need to synchronize this with reslock. And this
> > probably deserve a dedicated fix.
> >
>
> It's already being held by mlx5_vdpa_suspend
Right, but I meant this seems kind of duplicated with set_cvq_ready(),
can we unify them? (We don't hold reslock there).
>
> > > +
> > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > +{
> > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > + struct mlx5_vdpa_virtqueue *mvq;
> > > + int i;
> > > +
> > > + if (!suspend) {
> > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + down_write(&ndev->reslock);
> > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > + mvq = &ndev->vqs[i];
> > > + suspend_vq(ndev, mvq);
> > > + }
> > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> >
> > Do we need to synchronize with the carrier work here? Otherwise we may
> > get config notification after suspending.
> >
>
> Are you saying we should not allow carrier updates after the VQs have been suspended?
> Link state should not be related to suspension of VQs.
Yes, it's not related to the VQ but we suspend the device here. So we
probably need to flush the carrier work.
Thanks
>
> > > + up_write(&ndev->reslock);
> > > + return 0;
> > > +}
> > > +
> > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > .get_generation = mlx5_vdpa_get_generation,
> > > .set_map = mlx5_vdpa_set_map,
> > > .free = mlx5_vdpa_free,
> > > + .suspend = mlx5_vdpa_suspend,
> >
> > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> >
> > Thanks
> >
> > > };
> > >
> > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > mvq->index = i;
> > > mvq->ndev = ndev;
> > > mvq->fwqp.fw = true;
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > }
> > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > mvq = &ndev->vqs[i];
> > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > index 4414ed5b6ed2..423562f39d3c 100644
> > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > @@ -150,6 +150,14 @@ enum {
> > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > };
> > >
> > > +/* This indicates that the object was not created or has alreadyi
> > > + * been desroyed. It is very safe to assume that this object will never
> > > + * have so many states
> > > + */
> > > +enum {
> > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > +};
> > > +
> > > enum {
> > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > --
> > > 2.35.1
> > >
>
On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
<[email protected]> wrote:
>
> On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> >
> > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > >
> > > Implement the suspend callback allowing to suspend the virtqueues so
> > > they stop processing descriptors. This is required to allow the shadow
> > > virtqueue to kick in.
> > >
> > > Signed-off-by: Eli Cohen <[email protected]>
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > if (err)
> > > goto err_cmd;
> > >
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > kfree(in);
> > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > >
> > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > return;
> > > }
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > umems_destroy(ndev, mvq);
> > > }
> > >
> > > @@ -1121,6 +1123,20 @@ 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)
> > > +{
> > > + switch (oldstate) {
> > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > + 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:
> > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > + default:
> > > + return false;
> > > + }
> > > +}
> > > +
> > > 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);
> > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > void *in;
> > > int err;
> > >
> > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > + return 0;
> > > +
> > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > + return -EINVAL;
> > > +
> > > in = kzalloc(inlen, GFP_KERNEL);
> > > if (!in)
> > > return -ENOMEM;
> > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > struct mlx5_vdpa_virtqueue *mvq;
> > > + int err;
> > >
> > > if (!mvdev->actual_features)
> > > return;
> > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > }
> > >
> > > mvq = &ndev->vqs[idx];
> > > - if (!ready)
> > > + if (!ready) {
> > > suspend_vq(ndev, mvq);
> > > + } else {
> > > + err = modify_virtqueue(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;
> > > + }
> > > + }
> > > +
> > >
> > > mvq->ready = ready;
> > > }
> > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > return err;
> > > }
> > >
> > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > +{
> > > + struct mlx5_control_vq *cvq;
> > > +
> > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > + return;
> > > +
> > > + cvq = &mvdev->cvq;
> > > + cvq->ready = !suspend;
> > > +}
> >
> > It looks to me we need to synchronize this with reslock. And this
> > probably deserve a dedicated fix.
> >
> > > +
> > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > +{
> > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > + struct mlx5_vdpa_virtqueue *mvq;
> > > + int i;
> > > +
> > > + if (!suspend) {
> > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + down_write(&ndev->reslock);
> > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > + mvq = &ndev->vqs[i];
> > > + suspend_vq(ndev, mvq);
> > > + }
> > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> >
> > Do we need to synchronize with the carrier work here? Otherwise we may
> > get config notification after suspending.
> >
> > > + up_write(&ndev->reslock);
> > > + return 0;
> > > +}
> > > +
> > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > .get_generation = mlx5_vdpa_get_generation,
> > > .set_map = mlx5_vdpa_set_map,
> > > .free = mlx5_vdpa_free,
> > > + .suspend = mlx5_vdpa_suspend,
> >
> > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> >
>
> Should we add
> Based-on: <[email protected]>
>
> To this series?
Probably, but that series seems to support resume while this series doesn't.
Any reason for this?
(I don't see any blocker for this especially considering parents can
choose to do reset + set_vring_state etc.)
Thanks
>
> > Thanks
> >
> > > };
> > >
> > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > mvq->index = i;
> > > mvq->ndev = ndev;
> > > mvq->fwqp.fw = true;
> > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > }
> > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > mvq = &ndev->vqs[i];
> > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > index 4414ed5b6ed2..423562f39d3c 100644
> > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > @@ -150,6 +150,14 @@ enum {
> > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > };
> > >
> > > +/* This indicates that the object was not created or has alreadyi
> > > + * been desroyed. It is very safe to assume that this object will never
> > > + * have so many states
> > > + */
> > > +enum {
> > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > +};
> > > +
> > > enum {
> > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > --
> > > 2.35.1
> > >
> >
>
On Tue, Jun 21, 2022 at 5:05 AM Jason Wang <[email protected]> wrote:
>
> On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> <[email protected]> wrote:
> >
> > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > >
> > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > they stop processing descriptors. This is required to allow the shadow
> > > > virtqueue to kick in.
> > > >
> > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > if (err)
> > > > goto err_cmd;
> > > >
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > kfree(in);
> > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > >
> > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > return;
> > > > }
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > umems_destroy(ndev, mvq);
> > > > }
> > > >
> > > > @@ -1121,6 +1123,20 @@ 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)
> > > > +{
> > > > + switch (oldstate) {
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > + 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:
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > + default:
> > > > + return false;
> > > > + }
> > > > +}
> > > > +
> > > > 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);
> > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > void *in;
> > > > int err;
> > > >
> > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > + return 0;
> > > > +
> > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > + return -EINVAL;
> > > > +
> > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > if (!in)
> > > > return -ENOMEM;
> > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > + int err;
> > > >
> > > > if (!mvdev->actual_features)
> > > > return;
> > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > }
> > > >
> > > > mvq = &ndev->vqs[idx];
> > > > - if (!ready)
> > > > + if (!ready) {
> > > > suspend_vq(ndev, mvq);
> > > > + } else {
> > > > + err = modify_virtqueue(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;
> > > > + }
> > > > + }
> > > > +
> > > >
> > > > mvq->ready = ready;
> > > > }
> > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > return err;
> > > > }
> > > >
> > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > +{
> > > > + struct mlx5_control_vq *cvq;
> > > > +
> > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > + return;
> > > > +
> > > > + cvq = &mvdev->cvq;
> > > > + cvq->ready = !suspend;
> > > > +}
> > >
> > > It looks to me we need to synchronize this with reslock. And this
> > > probably deserve a dedicated fix.
> > >
> > > > +
> > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > +{
> > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > + int i;
> > > > +
> > > > + if (!suspend) {
> > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + down_write(&ndev->reslock);
> > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > + mvq = &ndev->vqs[i];
> > > > + suspend_vq(ndev, mvq);
> > > > + }
> > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > >
> > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > get config notification after suspending.
> > >
> > > > + up_write(&ndev->reslock);
> > > > + return 0;
> > > > +}
> > > > +
> > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .get_generation = mlx5_vdpa_get_generation,
> > > > .set_map = mlx5_vdpa_set_map,
> > > > .free = mlx5_vdpa_free,
> > > > + .suspend = mlx5_vdpa_suspend,
> > >
> > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > >
> >
> > Should we add
> > Based-on: <[email protected]>
> >
> > To this series?
>
> Probably, but that series seems to support resume while this series doesn't.
>
> Any reason for this?
>
> (I don't see any blocker for this especially considering parents can
> choose to do reset + set_vring_state etc.)
>
I suggest starting simple and modify the vdpa_sim series so it only
provides suspend() operation, with no parameters. We can always add
the resume() later if needed at all.
To provide the reset + set_vring_state etc seems simpler if done from userland.
Thanks!
> Thanks
>
> >
> > > Thanks
> > >
> > > > };
> > > >
> > > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > > mvq->index = i;
> > > > mvq->ndev = ndev;
> > > > mvq->fwqp.fw = true;
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > }
> > > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > > mvq = &ndev->vqs[i];
> > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > index 4414ed5b6ed2..423562f39d3c 100644
> > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > @@ -150,6 +150,14 @@ enum {
> > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > > };
> > > >
> > > > +/* This indicates that the object was not created or has alreadyi
> > > > + * been desroyed. It is very safe to assume that this object will never
> > > > + * have so many states
> > > > + */
> > > > +enum {
> > > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > > +};
> > > > +
> > > > enum {
> > > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > > --
> > > > 2.35.1
> > > >
> > >
> >
>
On Tue, Jun 21, 2022 at 3:49 PM Eugenio Perez Martin
<[email protected]> wrote:
>
> On Tue, Jun 21, 2022 at 5:05 AM Jason Wang <[email protected]> wrote:
> >
> > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > <[email protected]> wrote:
> > >
> > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > >
> > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > virtqueue to kick in.
> > > > >
> > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > ---
> > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > if (err)
> > > > > goto err_cmd;
> > > > >
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > kfree(in);
> > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > >
> > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > return;
> > > > > }
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > umems_destroy(ndev, mvq);
> > > > > }
> > > > >
> > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > +{
> > > > > + switch (oldstate) {
> > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > + 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:
> > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > + default:
> > > > > + return false;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > 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);
> > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > void *in;
> > > > > int err;
> > > > >
> > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > + return 0;
> > > > > +
> > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > + return -EINVAL;
> > > > > +
> > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > if (!in)
> > > > > return -ENOMEM;
> > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > + int err;
> > > > >
> > > > > if (!mvdev->actual_features)
> > > > > return;
> > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > }
> > > > >
> > > > > mvq = &ndev->vqs[idx];
> > > > > - if (!ready)
> > > > > + if (!ready) {
> > > > > suspend_vq(ndev, mvq);
> > > > > + } else {
> > > > > + err = modify_virtqueue(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;
> > > > > + }
> > > > > + }
> > > > > +
> > > > >
> > > > > mvq->ready = ready;
> > > > > }
> > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > return err;
> > > > > }
> > > > >
> > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > +{
> > > > > + struct mlx5_control_vq *cvq;
> > > > > +
> > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > + return;
> > > > > +
> > > > > + cvq = &mvdev->cvq;
> > > > > + cvq->ready = !suspend;
> > > > > +}
> > > >
> > > > It looks to me we need to synchronize this with reslock. And this
> > > > probably deserve a dedicated fix.
> > > >
> > > > > +
> > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > +{
> > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > + int i;
> > > > > +
> > > > > + if (!suspend) {
> > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > + return -EOPNOTSUPP;
> > > > > + }
> > > > > +
> > > > > + down_write(&ndev->reslock);
> > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > + mvq = &ndev->vqs[i];
> > > > > + suspend_vq(ndev, mvq);
> > > > > + }
> > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > >
> > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > get config notification after suspending.
> > > >
> > > > > + up_write(&ndev->reslock);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > .set_map = mlx5_vdpa_set_map,
> > > > > .free = mlx5_vdpa_free,
> > > > > + .suspend = mlx5_vdpa_suspend,
> > > >
> > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > >
> > >
> > > Should we add
> > > Based-on: <[email protected]>
> > >
> > > To this series?
> >
> > Probably, but that series seems to support resume while this series doesn't.
> >
> > Any reason for this?
> >
> > (I don't see any blocker for this especially considering parents can
> > choose to do reset + set_vring_state etc.)
> >
>
> I suggest starting simple and modify the vdpa_sim series so it only
> provides suspend() operation, with no parameters. We can always add
> the resume() later if needed at all.
This complicates the feature a little bit.
>
> To provide the reset + set_vring_state etc seems simpler if done from userland.
One issue for the current API is that it only works for networking
devices since we don't have a way to set device state.
By having stop/resume, we know the device state is kept.
Thanks
>
> Thanks!
>
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > > };
> > > > >
> > > > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > > > mvq->index = i;
> > > > > mvq->ndev = ndev;
> > > > > mvq->fwqp.fw = true;
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > }
> > > > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > > > mvq = &ndev->vqs[i];
> > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > index 4414ed5b6ed2..423562f39d3c 100644
> > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > @@ -150,6 +150,14 @@ enum {
> > > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > > > };
> > > > >
> > > > > +/* This indicates that the object was not created or has alreadyi
> > > > > + * been desroyed. It is very safe to assume that this object will never
> > > > > + * have so many states
> > > > > + */
> > > > > +enum {
> > > > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > > > +};
> > > > > +
> > > > > enum {
> > > > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > > > --
> > > > > 2.35.1
> > > > >
> > > >
> > >
> >
>
> -----Original Message-----
> From: Jason Wang <[email protected]>
> Sent: Tuesday, June 21, 2022 5:59 AM
> To: Eli Cohen <[email protected]>
> Cc: eperezma <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-
> kernel <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
>
> On Mon, Jun 20, 2022 at 9:09 PM Eli Cohen <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Jason Wang <[email protected]>
> > > Sent: Monday, June 20, 2022 11:56 AM
> > > To: Eli Cohen <[email protected]>
> > > Cc: eperezma <[email protected]>; mst <[email protected]>; virtualization <[email protected]>;
> linux-
> > > kernel <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> > >
> > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > >
> > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > they stop processing descriptors. This is required to allow the shadow
> > > > virtqueue to kick in.
> > > >
> > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > if (err)
> > > > goto err_cmd;
> > > >
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > kfree(in);
> > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > >
> > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > return;
> > > > }
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > umems_destroy(ndev, mvq);
> > > > }
> > > >
> > > > @@ -1121,6 +1123,20 @@ 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)
> > > > +{
> > > > + switch (oldstate) {
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > + 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:
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > + default:
> > > > + return false;
> > > > + }
> > > > +}
> > > > +
> > > > 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);
> > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > void *in;
> > > > int err;
> > > >
> > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > + return 0;
> > > > +
> > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > + return -EINVAL;
> > > > +
> > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > if (!in)
> > > > return -ENOMEM;
> > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > + int err;
> > > >
> > > > if (!mvdev->actual_features)
> > > > return;
> > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > }
> > > >
> > > > mvq = &ndev->vqs[idx];
> > > > - if (!ready)
> > > > + if (!ready) {
> > > > suspend_vq(ndev, mvq);
> > > > + } else {
> > > > + err = modify_virtqueue(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;
> > > > + }
> > > > + }
> > > > +
> > > >
> > > > mvq->ready = ready;
> > > > }
> > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > return err;
> > > > }
> > > >
> > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > +{
> > > > + struct mlx5_control_vq *cvq;
> > > > +
> > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > + return;
> > > > +
> > > > + cvq = &mvdev->cvq;
> > > > + cvq->ready = !suspend;
> > > > +}
> > >
> > > It looks to me we need to synchronize this with reslock. And this
> > > probably deserve a dedicated fix.
> > >
> >
> > It's already being held by mlx5_vdpa_suspend
>
> Right, but I meant this seems kind of duplicated with set_cvq_ready(),
> can we unify them? (We don't hold reslock there).
Do you mean call set_vq_ready(mvdev, !suspend) and abandon mlx5_vdpa_cvq_suspend()?
>
> >
> > > > +
> > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > +{
> > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > + int i;
> > > > +
> > > > + if (!suspend) {
> > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + down_write(&ndev->reslock);
> > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > + mvq = &ndev->vqs[i];
> > > > + suspend_vq(ndev, mvq);
> > > > + }
> > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > >
> > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > get config notification after suspending.
> > >
> >
> > Are you saying we should not allow carrier updates after the VQs have been suspended?
> > Link state should not be related to suspension of VQs.
>
> Yes, it's not related to the VQ but we suspend the device here. So we
> probably need to flush the carrier work.
>
Right.
> Thanks
>
> >
> > > > + up_write(&ndev->reslock);
> > > > + return 0;
> > > > +}
> > > > +
> > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .get_generation = mlx5_vdpa_get_generation,
> > > > .set_map = mlx5_vdpa_set_map,
> > > > .free = mlx5_vdpa_free,
> > > > + .suspend = mlx5_vdpa_suspend,
> > >
> > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > >
> > > Thanks
> > >
> > > > };
> > > >
> > > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > > mvq->index = i;
> > > > mvq->ndev = ndev;
> > > > mvq->fwqp.fw = true;
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > }
> > > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > > mvq = &ndev->vqs[i];
> > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > index 4414ed5b6ed2..423562f39d3c 100644
> > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > @@ -150,6 +150,14 @@ enum {
> > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > > };
> > > >
> > > > +/* This indicates that the object was not created or has alreadyi
> > > > + * been desroyed. It is very safe to assume that this object will never
> > > > + * have so many states
> > > > + */
> > > > +enum {
> > > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > > +};
> > > > +
> > > > enum {
> > > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > > --
> > > > 2.35.1
> > > >
> >
> From: Jason Wang <[email protected]>
> Sent: Tuesday, June 21, 2022 6:05 AM
> To: Eugenio Perez Martin <[email protected]>
> Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
>
> On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> <[email protected]> wrote:
> >
> > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > >
> > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > they stop processing descriptors. This is required to allow the shadow
> > > > virtqueue to kick in.
> > > >
> > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > if (err)
> > > > goto err_cmd;
> > > >
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > kfree(in);
> > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > >
> > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > return;
> > > > }
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > umems_destroy(ndev, mvq);
> > > > }
> > > >
> > > > @@ -1121,6 +1123,20 @@ 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)
> > > > +{
> > > > + switch (oldstate) {
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > + 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:
> > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > + default:
> > > > + return false;
> > > > + }
> > > > +}
> > > > +
> > > > 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);
> > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > void *in;
> > > > int err;
> > > >
> > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > + return 0;
> > > > +
> > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > + return -EINVAL;
> > > > +
> > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > if (!in)
> > > > return -ENOMEM;
> > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > + int err;
> > > >
> > > > if (!mvdev->actual_features)
> > > > return;
> > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > }
> > > >
> > > > mvq = &ndev->vqs[idx];
> > > > - if (!ready)
> > > > + if (!ready) {
> > > > suspend_vq(ndev, mvq);
> > > > + } else {
> > > > + err = modify_virtqueue(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;
> > > > + }
> > > > + }
> > > > +
> > > >
> > > > mvq->ready = ready;
> > > > }
> > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > return err;
> > > > }
> > > >
> > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > +{
> > > > + struct mlx5_control_vq *cvq;
> > > > +
> > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > + return;
> > > > +
> > > > + cvq = &mvdev->cvq;
> > > > + cvq->ready = !suspend;
> > > > +}
> > >
> > > It looks to me we need to synchronize this with reslock. And this
> > > probably deserve a dedicated fix.
> > >
> > > > +
> > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > +{
> > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > + int i;
> > > > +
> > > > + if (!suspend) {
> > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + down_write(&ndev->reslock);
> > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > + mvq = &ndev->vqs[i];
> > > > + suspend_vq(ndev, mvq);
> > > > + }
> > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > >
> > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > get config notification after suspending.
> > >
> > > > + up_write(&ndev->reslock);
> > > > + return 0;
> > > > +}
> > > > +
> > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > .get_generation = mlx5_vdpa_get_generation,
> > > > .set_map = mlx5_vdpa_set_map,
> > > > .free = mlx5_vdpa_free,
> > > > + .suspend = mlx5_vdpa_suspend,
> > >
> > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > >
> >
> > Should we add
> > Based-on: <[email protected]>
> >
> > To this series?
>
> Probably, but that series seems to support resume while this series doesn't.
>
> Any reason for this?
I think Eugenio agreed that resume is not really required since we're going stop using this
instance and migrate. In any case, we don't support resume for the hardware object
though it could be simulated should it be absolutely necessary.
>
> (I don't see any blocker for this especially considering parents can
> choose to do reset + set_vring_state etc.)
>
> Thanks
>
> >
> > > Thanks
> > >
> > > > };
> > > >
> > > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > > mvq->index = i;
> > > > mvq->ndev = ndev;
> > > > mvq->fwqp.fw = true;
> > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > }
> > > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > > mvq = &ndev->vqs[i];
> > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > index 4414ed5b6ed2..423562f39d3c 100644
> > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > @@ -150,6 +150,14 @@ enum {
> > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > > };
> > > >
> > > > +/* This indicates that the object was not created or has alreadyi
> > > > + * been desroyed. It is very safe to assume that this object will never
> > > > + * have so many states
> > > > + */
> > > > +enum {
> > > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > > +};
> > > > +
> > > > enum {
> > > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > > --
> > > > 2.35.1
> > > >
> > >
> >
On Mon, Jul 11, 2022 at 8:14 AM Eli Cohen <[email protected]> wrote:
>
> > From: Jason Wang <[email protected]>
> > Sent: Tuesday, June 21, 2022 6:05 AM
> > To: Eugenio Perez Martin <[email protected]>
> > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> >
> > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > <[email protected]> wrote:
> > >
> > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > >
> > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > virtqueue to kick in.
> > > > >
> > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > ---
> > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > if (err)
> > > > > goto err_cmd;
> > > > >
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > kfree(in);
> > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > >
> > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > return;
> > > > > }
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > umems_destroy(ndev, mvq);
> > > > > }
> > > > >
> > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > +{
> > > > > + switch (oldstate) {
> > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > + 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:
> > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > + default:
> > > > > + return false;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > 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);
> > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > void *in;
> > > > > int err;
> > > > >
> > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > + return 0;
> > > > > +
> > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > + return -EINVAL;
> > > > > +
> > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > if (!in)
> > > > > return -ENOMEM;
> > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > + int err;
> > > > >
> > > > > if (!mvdev->actual_features)
> > > > > return;
> > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > }
> > > > >
> > > > > mvq = &ndev->vqs[idx];
> > > > > - if (!ready)
> > > > > + if (!ready) {
> > > > > suspend_vq(ndev, mvq);
> > > > > + } else {
> > > > > + err = modify_virtqueue(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;
> > > > > + }
> > > > > + }
> > > > > +
> > > > >
> > > > > mvq->ready = ready;
> > > > > }
> > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > return err;
> > > > > }
> > > > >
> > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > +{
> > > > > + struct mlx5_control_vq *cvq;
> > > > > +
> > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > + return;
> > > > > +
> > > > > + cvq = &mvdev->cvq;
> > > > > + cvq->ready = !suspend;
> > > > > +}
> > > >
> > > > It looks to me we need to synchronize this with reslock. And this
> > > > probably deserve a dedicated fix.
> > > >
> > > > > +
> > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > +{
> > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > + int i;
> > > > > +
> > > > > + if (!suspend) {
> > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > + return -EOPNOTSUPP;
> > > > > + }
> > > > > +
> > > > > + down_write(&ndev->reslock);
> > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > + mvq = &ndev->vqs[i];
> > > > > + suspend_vq(ndev, mvq);
> > > > > + }
> > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > >
> > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > get config notification after suspending.
> > > >
> > > > > + up_write(&ndev->reslock);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > .set_map = mlx5_vdpa_set_map,
> > > > > .free = mlx5_vdpa_free,
> > > > > + .suspend = mlx5_vdpa_suspend,
> > > >
> > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > >
> > >
> > > Should we add
> > > Based-on: <[email protected]>
> > >
> > > To this series?
> >
> > Probably, but that series seems to support resume while this series doesn't.
> >
> > Any reason for this?
>
> I think Eugenio agreed that resume is not really required since we're going stop using this
> instance and migrate. In any case, we don't support resume for the hardware object
> though it could be simulated should it be absolutely necessary.
>
That's right, to resume the device it's not mandatory to achieve Live
Migration use case and we can always add another backend feature bit
to support the resume of a device from my point of view.
Thanks!
On Mon, Jul 11, 2022 at 2:14 PM Eli Cohen <[email protected]> wrote:
>
> > From: Jason Wang <[email protected]>
> > Sent: Tuesday, June 21, 2022 6:05 AM
> > To: Eugenio Perez Martin <[email protected]>
> > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> >
> > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > <[email protected]> wrote:
> > >
> > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > >
> > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > virtqueue to kick in.
> > > > >
> > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > ---
> > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > if (err)
> > > > > goto err_cmd;
> > > > >
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > kfree(in);
> > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > >
> > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > return;
> > > > > }
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > umems_destroy(ndev, mvq);
> > > > > }
> > > > >
> > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > +{
> > > > > + switch (oldstate) {
> > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > + 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:
> > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > + default:
> > > > > + return false;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > 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);
> > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > void *in;
> > > > > int err;
> > > > >
> > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > + return 0;
> > > > > +
> > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > + return -EINVAL;
> > > > > +
> > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > if (!in)
> > > > > return -ENOMEM;
> > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > + int err;
> > > > >
> > > > > if (!mvdev->actual_features)
> > > > > return;
> > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > }
> > > > >
> > > > > mvq = &ndev->vqs[idx];
> > > > > - if (!ready)
> > > > > + if (!ready) {
> > > > > suspend_vq(ndev, mvq);
> > > > > + } else {
> > > > > + err = modify_virtqueue(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;
> > > > > + }
> > > > > + }
> > > > > +
> > > > >
> > > > > mvq->ready = ready;
> > > > > }
> > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > return err;
> > > > > }
> > > > >
> > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > +{
> > > > > + struct mlx5_control_vq *cvq;
> > > > > +
> > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > + return;
> > > > > +
> > > > > + cvq = &mvdev->cvq;
> > > > > + cvq->ready = !suspend;
> > > > > +}
> > > >
> > > > It looks to me we need to synchronize this with reslock. And this
> > > > probably deserve a dedicated fix.
> > > >
> > > > > +
> > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > +{
> > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > + int i;
> > > > > +
> > > > > + if (!suspend) {
> > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > + return -EOPNOTSUPP;
> > > > > + }
> > > > > +
> > > > > + down_write(&ndev->reslock);
> > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > + mvq = &ndev->vqs[i];
> > > > > + suspend_vq(ndev, mvq);
> > > > > + }
> > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > >
> > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > get config notification after suspending.
> > > >
> > > > > + up_write(&ndev->reslock);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > .set_map = mlx5_vdpa_set_map,
> > > > > .free = mlx5_vdpa_free,
> > > > > + .suspend = mlx5_vdpa_suspend,
> > > >
> > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > >
> > >
> > > Should we add
> > > Based-on: <[email protected]>
> > >
> > > To this series?
> >
> > Probably, but that series seems to support resume while this series doesn't.
> >
> > Any reason for this?
>
> I think Eugenio agreed that resume is not really required since we're going stop using this
> instance and migrate. In any case, we don't support resume for the hardware object
> though it could be simulated should it be absolutely necessary.
This is fine if everything is fine during the live migration. But when
migration fails due to some reason, management (libvirt) may choose to
restart the device in the source.
This means we should either
1) support resume in the parent
2) emulate it in the qemu (with a lot of restoring of the states)
And it is not only used for live migration, it could be used for vmstop/start.
Thanks
>
> >
> > (I don't see any blocker for this especially considering parents can
> > choose to do reset + set_vring_state etc.)
> >
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > > };
> > > > >
> > > > > static int query_mtu(struct mlx5_core_dev *mdev, u16 *mtu)
> > > > > @@ -2827,6 +2892,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > > > > mvq->index = i;
> > > > > mvq->ndev = ndev;
> > > > > mvq->fwqp.fw = true;
> > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > }
> > > > > for (; i < ndev->mvdev.max_vqs; i++) {
> > > > > mvq = &ndev->vqs[i];
> > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > index 4414ed5b6ed2..423562f39d3c 100644
> > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > @@ -150,6 +150,14 @@ enum {
> > > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR = 0x3,
> > > > > };
> > > > >
> > > > > +/* This indicates that the object was not created or has alreadyi
> > > > > + * been desroyed. It is very safe to assume that this object will never
> > > > > + * have so many states
> > > > > + */
> > > > > +enum {
> > > > > + MLX5_VIRTIO_NET_Q_OBJECT_NONE = 0xffffffff
> > > > > +};
> > > > > +
> > > > > enum {
> > > > > MLX5_RQTC_LIST_Q_TYPE_RQ = 0x0,
> > > > > MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q = 0x1,
> > > > > --
> > > > > 2.35.1
> > > > >
> > > >
> > >
>
On Tue, Jul 12, 2022 at 10:14 AM Jason Wang <[email protected]> wrote:
>
> On Mon, Jul 11, 2022 at 2:14 PM Eli Cohen <[email protected]> wrote:
> >
> > > From: Jason Wang <[email protected]>
> > > Sent: Tuesday, June 21, 2022 6:05 AM
> > > To: Eugenio Perez Martin <[email protected]>
> > > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> > > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> > >
> > > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > > <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > > >
> > > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > > virtqueue to kick in.
> > > > > >
> > > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > > ---
> > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > if (err)
> > > > > > goto err_cmd;
> > > > > >
> > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > > kfree(in);
> > > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > > >
> > > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > > return;
> > > > > > }
> > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > > umems_destroy(ndev, mvq);
> > > > > > }
> > > > > >
> > > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > > +{
> > > > > > + switch (oldstate) {
> > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > > + 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:
> > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > > + default:
> > > > > > + return false;
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > 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);
> > > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > void *in;
> > > > > > int err;
> > > > > >
> > > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > > + return 0;
> > > > > > +
> > > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > > if (!in)
> > > > > > return -ENOMEM;
> > > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > > + int err;
> > > > > >
> > > > > > if (!mvdev->actual_features)
> > > > > > return;
> > > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > }
> > > > > >
> > > > > > mvq = &ndev->vqs[idx];
> > > > > > - if (!ready)
> > > > > > + if (!ready) {
> > > > > > suspend_vq(ndev, mvq);
> > > > > > + } else {
> > > > > > + err = modify_virtqueue(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;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > >
> > > > > > mvq->ready = ready;
> > > > > > }
> > > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > > return err;
> > > > > > }
> > > > > >
> > > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > > +{
> > > > > > + struct mlx5_control_vq *cvq;
> > > > > > +
> > > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > + return;
> > > > > > +
> > > > > > + cvq = &mvdev->cvq;
> > > > > > + cvq->ready = !suspend;
> > > > > > +}
> > > > >
> > > > > It looks to me we need to synchronize this with reslock. And this
> > > > > probably deserve a dedicated fix.
> > > > >
> > > > > > +
> > > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > > +{
> > > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > > + int i;
> > > > > > +
> > > > > > + if (!suspend) {
> > > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > > + return -EOPNOTSUPP;
> > > > > > + }
> > > > > > +
> > > > > > + down_write(&ndev->reslock);
> > > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > > + mvq = &ndev->vqs[i];
> > > > > > + suspend_vq(ndev, mvq);
> > > > > > + }
> > > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > > >
> > > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > > get config notification after suspending.
> > > > >
> > > > > > + up_write(&ndev->reslock);
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > > .set_map = mlx5_vdpa_set_map,
> > > > > > .free = mlx5_vdpa_free,
> > > > > > + .suspend = mlx5_vdpa_suspend,
> > > > >
> > > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > > >
> > > >
> > > > Should we add
> > > > Based-on: <[email protected]>
> > > >
> > > > To this series?
> > >
> > > Probably, but that series seems to support resume while this series doesn't.
> > >
> > > Any reason for this?
> >
> > I think Eugenio agreed that resume is not really required since we're going stop using this
> > instance and migrate. In any case, we don't support resume for the hardware object
> > though it could be simulated should it be absolutely necessary.
>
> This is fine if everything is fine during the live migration. But when
> migration fails due to some reason, management (libvirt) may choose to
> restart the device in the source.
>
> This means we should either
>
> 1) support resume in the parent
> 2) emulate it in the qemu (with a lot of restoring of the states)
>
I think it should be handled in qemu (at least the POC reset the
device), but I didn't exercise a lot of the failure paths there
because, well, it was a POC :).
> And it is not only used for live migration, it could be used for vmstop/start.
>
I think it would be easier if we dedicate a feature flag for resuming
the device in the future. Qemu could take advantage of it at some
error paths of live migration, but less than it seems because it
overrides things like ring addresses. And, obviously, in the
vmstop/vmstart.
Actually, net devices should be ok to restore with a full reset. The
problem should be filesystems etc that are not part of vdpa at the
moment.
Thanks!
On Tue, Jul 12, 2022 at 5:16 PM Eugenio Perez Martin
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 10:14 AM Jason Wang <[email protected]> wrote:
> >
> > On Mon, Jul 11, 2022 at 2:14 PM Eli Cohen <[email protected]> wrote:
> > >
> > > > From: Jason Wang <[email protected]>
> > > > Sent: Tuesday, June 21, 2022 6:05 AM
> > > > To: Eugenio Perez Martin <[email protected]>
> > > > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> > > > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> > > >
> > > > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > > > >
> > > > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > > > >
> > > > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > > > virtqueue to kick in.
> > > > > > >
> > > > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > > > ---
> > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > > if (err)
> > > > > > > goto err_cmd;
> > > > > > >
> > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > > > kfree(in);
> > > > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > > > >
> > > > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > > > return;
> > > > > > > }
> > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > > > umems_destroy(ndev, mvq);
> > > > > > > }
> > > > > > >
> > > > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > > > +{
> > > > > > > + switch (oldstate) {
> > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > > > + 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:
> > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > > > + default:
> > > > > > > + return false;
> > > > > > > + }
> > > > > > > +}
> > > > > > > +
> > > > > > > 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);
> > > > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > > void *in;
> > > > > > > int err;
> > > > > > >
> > > > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > > > if (!in)
> > > > > > > return -ENOMEM;
> > > > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > > > + int err;
> > > > > > >
> > > > > > > if (!mvdev->actual_features)
> > > > > > > return;
> > > > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > > }
> > > > > > >
> > > > > > > mvq = &ndev->vqs[idx];
> > > > > > > - if (!ready)
> > > > > > > + if (!ready) {
> > > > > > > suspend_vq(ndev, mvq);
> > > > > > > + } else {
> > > > > > > + err = modify_virtqueue(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;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > >
> > > > > > > mvq->ready = ready;
> > > > > > > }
> > > > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > > > return err;
> > > > > > > }
> > > > > > >
> > > > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > > > +{
> > > > > > > + struct mlx5_control_vq *cvq;
> > > > > > > +
> > > > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > > + return;
> > > > > > > +
> > > > > > > + cvq = &mvdev->cvq;
> > > > > > > + cvq->ready = !suspend;
> > > > > > > +}
> > > > > >
> > > > > > It looks to me we need to synchronize this with reslock. And this
> > > > > > probably deserve a dedicated fix.
> > > > > >
> > > > > > > +
> > > > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > > > +{
> > > > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > > > + int i;
> > > > > > > +
> > > > > > > + if (!suspend) {
> > > > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > > > + return -EOPNOTSUPP;
> > > > > > > + }
> > > > > > > +
> > > > > > > + down_write(&ndev->reslock);
> > > > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > > > + mvq = &ndev->vqs[i];
> > > > > > > + suspend_vq(ndev, mvq);
> > > > > > > + }
> > > > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > > > >
> > > > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > > > get config notification after suspending.
> > > > > >
> > > > > > > + up_write(&ndev->reslock);
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > > > .set_map = mlx5_vdpa_set_map,
> > > > > > > .free = mlx5_vdpa_free,
> > > > > > > + .suspend = mlx5_vdpa_suspend,
> > > > > >
> > > > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > > > >
> > > > >
> > > > > Should we add
> > > > > Based-on: <[email protected]>
> > > > >
> > > > > To this series?
> > > >
> > > > Probably, but that series seems to support resume while this series doesn't.
> > > >
> > > > Any reason for this?
> > >
> > > I think Eugenio agreed that resume is not really required since we're going stop using this
> > > instance and migrate. In any case, we don't support resume for the hardware object
> > > though it could be simulated should it be absolutely necessary.
> >
> > This is fine if everything is fine during the live migration. But when
> > migration fails due to some reason, management (libvirt) may choose to
> > restart the device in the source.
> >
> > This means we should either
> >
> > 1) support resume in the parent
> > 2) emulate it in the qemu (with a lot of restoring of the states)
> >
>
> I think it should be handled in qemu (at least the POC reset the
> device), but I didn't exercise a lot of the failure paths there
> because, well, it was a POC :).
It looks like a must in the production environment. The failure is not
necessarily related to shadow virtqueue itself.
Thanks
>
> > And it is not only used for live migration, it could be used for vmstop/start.
> >
>
> I think it would be easier if we dedicate a feature flag for resuming
> the device in the future. Qemu could take advantage of it at some
> error paths of live migration, but less than it seems because it
> overrides things like ring addresses. And, obviously, in the
> vmstop/vmstart.
>
> Actually, net devices should be ok to restore with a full reset. The
> problem should be filesystems etc that are not part of vdpa at the
> moment.
>
> Thanks!
>
> From: Jason Wang <[email protected]>
> Sent: Wednesday, July 13, 2022 6:29 AM
> To: Eugenio Perez Martin <[email protected]>
> Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
>
> On Tue, Jul 12, 2022 at 5:16 PM Eugenio Perez Martin
> <[email protected]> wrote:
> >
> > On Tue, Jul 12, 2022 at 10:14 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Mon, Jul 11, 2022 at 2:14 PM Eli Cohen <[email protected]> wrote:
> > > >
> > > > > From: Jason Wang <[email protected]>
> > > > > Sent: Tuesday, June 21, 2022 6:05 AM
> > > > > To: Eugenio Perez Martin <[email protected]>
> > > > > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-
> kernel
> > > > > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > > > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> > > > >
> > > > > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > > > > >
> > > > > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > > > > virtqueue to kick in.
> > > > > > > >
> > > > > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > > > if (err)
> > > > > > > > goto err_cmd;
> > > > > > > >
> > > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > > > > kfree(in);
> > > > > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > > > > >
> > > > > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > > > > return;
> > > > > > > > }
> > > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > > > > umems_destroy(ndev, mvq);
> > > > > > > > }
> > > > > > > >
> > > > > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > > > > +{
> > > > > > > > + switch (oldstate) {
> > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > > > > + 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:
> > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > > > > + default:
> > > > > > > > + return false;
> > > > > > > > + }
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > 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);
> > > > > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > > > void *in;
> > > > > > > > int err;
> > > > > > > >
> > > > > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > > > > + return -EINVAL;
> > > > > > > > +
> > > > > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > > > > if (!in)
> > > > > > > > return -ENOMEM;
> > > > > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > > > > + int err;
> > > > > > > >
> > > > > > > > if (!mvdev->actual_features)
> > > > > > > > return;
> > > > > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > > > }
> > > > > > > >
> > > > > > > > mvq = &ndev->vqs[idx];
> > > > > > > > - if (!ready)
> > > > > > > > + if (!ready) {
> > > > > > > > suspend_vq(ndev, mvq);
> > > > > > > > + } else {
> > > > > > > > + err = modify_virtqueue(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;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > >
> > > > > > > > mvq->ready = ready;
> > > > > > > > }
> > > > > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > > > > return err;
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > > > > +{
> > > > > > > > + struct mlx5_control_vq *cvq;
> > > > > > > > +
> > > > > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > > > + return;
> > > > > > > > +
> > > > > > > > + cvq = &mvdev->cvq;
> > > > > > > > + cvq->ready = !suspend;
> > > > > > > > +}
> > > > > > >
> > > > > > > It looks to me we need to synchronize this with reslock. And this
> > > > > > > probably deserve a dedicated fix.
> > > > > > >
> > > > > > > > +
> > > > > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > > > > +{
> > > > > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > > > > + int i;
> > > > > > > > +
> > > > > > > > + if (!suspend) {
> > > > > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > > > > + return -EOPNOTSUPP;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + down_write(&ndev->reslock);
> > > > > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > > > > + mvq = &ndev->vqs[i];
> > > > > > > > + suspend_vq(ndev, mvq);
> > > > > > > > + }
> > > > > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > > > > >
> > > > > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > > > > get config notification after suspending.
> > > > > > >
> > > > > > > > + up_write(&ndev->reslock);
> > > > > > > > + return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > > > > .set_map = mlx5_vdpa_set_map,
> > > > > > > > .free = mlx5_vdpa_free,
> > > > > > > > + .suspend = mlx5_vdpa_suspend,
> > > > > > >
> > > > > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > > > > >
> > > > > >
> > > > > > Should we add
> > > > > > Based-on: <[email protected]>
> > > > > >
> > > > > > To this series?
> > > > >
> > > > > Probably, but that series seems to support resume while this series doesn't.
> > > > >
> > > > > Any reason for this?
> > > >
> > > > I think Eugenio agreed that resume is not really required since we're going stop using this
> > > > instance and migrate. In any case, we don't support resume for the hardware object
> > > > though it could be simulated should it be absolutely necessary.
> > >
> > > This is fine if everything is fine during the live migration. But when
> > > migration fails due to some reason, management (libvirt) may choose to
> > > restart the device in the source.
> > >
> > > This means we should either
> > >
> > > 1) support resume in the parent
> > > 2) emulate it in the qemu (with a lot of restoring of the states)
> > >
> >
> > I think it should be handled in qemu (at least the POC reset the
> > device), but I didn't exercise a lot of the failure paths there
> > because, well, it was a POC :).
>
> It looks like a must in the production environment. The failure is not
> necessarily related to shadow virtqueue itself.
>
I don't see a specific interface to resume the device after suspend.
Reset however could do the job and we already have it.
> Thanks
>
> >
> > > And it is not only used for live migration, it could be used for vmstop/start.
> > >
> >
> > I think it would be easier if we dedicate a feature flag for resuming
> > the device in the future. Qemu could take advantage of it at some
> > error paths of live migration, but less than it seems because it
> > overrides things like ring addresses. And, obviously, in the
> > vmstop/vmstart.
> >
> > Actually, net devices should be ok to restore with a full reset. The
> > problem should be filesystems etc that are not part of vdpa at the
> > moment.
> >
> > Thanks!
> >
On Wed, Jul 13, 2022 at 1:18 PM Eli Cohen <[email protected]> wrote:
>
> > From: Jason Wang <[email protected]>
> > Sent: Wednesday, July 13, 2022 6:29 AM
> > To: Eugenio Perez Martin <[email protected]>
> > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-kernel
> > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> >
> > On Tue, Jul 12, 2022 at 5:16 PM Eugenio Perez Martin
> > <[email protected]> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 10:14 AM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Mon, Jul 11, 2022 at 2:14 PM Eli Cohen <[email protected]> wrote:
> > > > >
> > > > > > From: Jason Wang <[email protected]>
> > > > > > Sent: Tuesday, June 21, 2022 6:05 AM
> > > > > > To: Eugenio Perez Martin <[email protected]>
> > > > > > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization <[email protected]>; linux-
> > kernel
> > > > > > <[email protected]>; Si-Wei Liu <[email protected]>; Parav Pandit <[email protected]>
> > > > > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue callback
> > > > > >
> > > > > > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > Implement the suspend callback allowing to suspend the virtqueues so
> > > > > > > > > they stop processing descriptors. This is required to allow the shadow
> > > > > > > > > virtqueue to kick in.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > > > > > > ---
> > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 +++++++++++++++++++++++++++++-
> > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++
> > > > > > > > > 2 files changed, 75 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > index fb0b23e71383..ea4bc8a0cd25 100644
> > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > > > > if (err)
> > > > > > > > > goto err_cmd;
> > > > > > > > >
> > > > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT;
> > > > > > > > > kfree(in);
> > > > > > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> > > > > > > > >
> > > > > > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq
> > > > > > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy virtqueue 0x%x\n", mvq->virtq_id);
> > > > > > > > > return;
> > > > > > > > > }
> > > > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE;
> > > > > > > > > umems_destroy(ndev, mvq);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > @@ -1121,6 +1123,20 @@ 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)
> > > > > > > > > +{
> > > > > > > > > + switch (oldstate) {
> > > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT:
> > > > > > > > > + return newstate == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY;
> > > > > > > > > + 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:
> > > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR:
> > > > > > > > > + default:
> > > > > > > > > + return false;
> > > > > > > > > + }
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > 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);
> > > > > > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > > > > > > > > void *in;
> > > > > > > > > int err;
> > > > > > > > >
> > > > > > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > > > > > > > > + return 0;
> > > > > > > > > +
> > > > > > > > > + if (!is_valid_state_change(mvq->fw_state, state))
> > > > > > > > > + return -EINVAL;
> > > > > > > > > +
> > > > > > > > > in = kzalloc(inlen, GFP_KERNEL);
> > > > > > > > > if (!in)
> > > > > > > > > return -ENOMEM;
> > > > > > > > > @@ -1991,6 +2013,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > > > struct mlx5_vdpa_virtqueue *mvq;
> > > > > > > > > + int err;
> > > > > > > > >
> > > > > > > > > if (!mvdev->actual_features)
> > > > > > > > > return;
> > > > > > > > > @@ -2004,8 +2027,16 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > mvq = &ndev->vqs[idx];
> > > > > > > > > - if (!ready)
> > > > > > > > > + if (!ready) {
> > > > > > > > > suspend_vq(ndev, mvq);
> > > > > > > > > + } else {
> > > > > > > > > + err = modify_virtqueue(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;
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > >
> > > > > > > > > mvq->ready = ready;
> > > > > > > > > }
> > > > > > > > > @@ -2732,6 +2763,39 @@ static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> > > > > > > > > return err;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev *mvdev, bool suspend)
> > > > > > > > > +{
> > > > > > > > > + struct mlx5_control_vq *cvq;
> > > > > > > > > +
> > > > > > > > > + if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > > > > > > > + return;
> > > > > > > > > +
> > > > > > > > > + cvq = &mvdev->cvq;
> > > > > > > > > + cvq->ready = !suspend;
> > > > > > > > > +}
> > > > > > > >
> > > > > > > > It looks to me we need to synchronize this with reslock. And this
> > > > > > > > probably deserve a dedicated fix.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool suspend)
> > > > > > > > > +{
> > > > > > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > > > > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > > > + struct mlx5_vdpa_virtqueue *mvq;
> > > > > > > > > + int i;
> > > > > > > > > +
> > > > > > > > > + if (!suspend) {
> > > > > > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues is not supported\n");
> > > > > > > > > + return -EOPNOTSUPP;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + down_write(&ndev->reslock);
> > > > > > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) {
> > > > > > > > > + mvq = &ndev->vqs[i];
> > > > > > > > > + suspend_vq(ndev, mvq);
> > > > > > > > > + }
> > > > > > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend);
> > > > > > > >
> > > > > > > > Do we need to synchronize with the carrier work here? Otherwise we may
> > > > > > > > get config notification after suspending.
> > > > > > > >
> > > > > > > > > + up_write(&ndev->reslock);
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > > > > .set_vq_address = mlx5_vdpa_set_vq_address,
> > > > > > > > > .set_vq_num = mlx5_vdpa_set_vq_num,
> > > > > > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
> > > > > > > > > .get_generation = mlx5_vdpa_get_generation,
> > > > > > > > > .set_map = mlx5_vdpa_set_map,
> > > > > > > > > .free = mlx5_vdpa_free,
> > > > > > > > > + .suspend = mlx5_vdpa_suspend,
> > > > > > > >
> > > > > > > > I don't see the vDPA bus patch to enable this method. Or anything I missed here?
> > > > > > > >
> > > > > > >
> > > > > > > Should we add
> > > > > > > Based-on: <[email protected]>
> > > > > > >
> > > > > > > To this series?
> > > > > >
> > > > > > Probably, but that series seems to support resume while this series doesn't.
> > > > > >
> > > > > > Any reason for this?
> > > > >
> > > > > I think Eugenio agreed that resume is not really required since we're going stop using this
> > > > > instance and migrate. In any case, we don't support resume for the hardware object
> > > > > though it could be simulated should it be absolutely necessary.
> > > >
> > > > This is fine if everything is fine during the live migration. But when
> > > > migration fails due to some reason, management (libvirt) may choose to
> > > > restart the device in the source.
> > > >
> > > > This means we should either
> > > >
> > > > 1) support resume in the parent
> > > > 2) emulate it in the qemu (with a lot of restoring of the states)
> > > >
> > >
> > > I think it should be handled in qemu (at least the POC reset the
> > > device), but I didn't exercise a lot of the failure paths there
> > > because, well, it was a POC :).
> >
> > It looks like a must in the production environment. The failure is not
> > necessarily related to shadow virtqueue itself.
> >
>
> I don't see a specific interface to resume the device after suspend.
> Reset however could do the job and we already have it.
Yes, this is fine as long as we can emulate it via set_vq_state + reset.
Thanks
>
> > Thanks
> >
> > >
> > > > And it is not only used for live migration, it could be used for vmstop/start.
> > > >
> > >
> > > I think it would be easier if we dedicate a feature flag for resuming
> > > the device in the future. Qemu could take advantage of it at some
> > > error paths of live migration, but less than it seems because it
> > > overrides things like ring addresses. And, obviously, in the
> > > vmstop/vmstart.
> > >
> > > Actually, net devices should be ok to restore with a full reset. The
> > > problem should be filesystems etc that are not part of vdpa at the
> > > moment.
> > >
> > > Thanks!
> > >
>