Implement suspend operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively suspend the device.
This is a must before getting virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).
After a successful return of ioctl the device must not process more virtqueue
descriptors. The device can answer to read or writes of config fields as if it
were not suspended. In particular, writing to "queue_enable" with a value of 1
will not make the device start processing virtqueue buffers.
In the future, we will provide features similar to
VHOST_USER_GET_INFLIGHT_FD so the device can save pending operations.
Comments are welcome.
v7:
* Remove the resume operation, making the ioctl simpler. We can always add
another ioctl for VM_STOP/VM_RESUME operation later.
v6:
* s/stop/suspend/ to differentiate more from reset.
* Clarify scope of the suspend operation.
v5:
* s/not stop/resume/ in doc.
v4:
* Replace VHOST_STOP to VHOST_VDPA_STOP in vhost ioctl switch case too.
v3:
* s/VHOST_STOP/VHOST_VDPA_STOP/
* Add documentation and requirements of the ioctl above its definition.
v2:
* Replace raw _F_STOP with BIT_ULL(_F_STOP).
* Fix obtaining of stop ioctl arg (it was not obtained but written).
* Add stop to vdpa_sim_blk.
Eugenio Pérez (4):
vdpa: Add suspend operation
vhost-vdpa: introduce SUSPEND backend feature bit
vhost-vdpa: uAPI to suspend the device
vdpa_sim: Implement suspend vdpa op
drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++
drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 +++
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
drivers/vhost/vdpa.c | 35 +++++++++++++++++++++++++++-
include/linux/vdpa.h | 4 ++++
include/uapi/linux/vhost.h | 14 +++++++++++
include/uapi/linux/vhost_types.h | 2 ++
8 files changed, 82 insertions(+), 1 deletion(-)
--
2.31.1
Userland knows if it can suspend the device or not by checking this feature
bit.
It's only offered if the vdpa driver backend implements the suspend()
operation callback, and to offer it or userland to ack it if the backend
does not offer that callback is an error.
Signed-off-by: Eugenio Pérez <[email protected]>
---
drivers/vhost/vdpa.c | 16 +++++++++++++++-
include/uapi/linux/vhost_types.h | 2 ++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 23dcbfdfa13b..3d636e192061 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
return 0;
}
+static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
+{
+ struct vdpa_device *vdpa = v->vdpa;
+ const struct vdpa_config_ops *ops = vdpa->config;
+
+ return ops->suspend;
+}
+
static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
{
struct vdpa_device *vdpa = v->vdpa;
@@ -577,7 +585,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
if (cmd == VHOST_SET_BACKEND_FEATURES) {
if (copy_from_user(&features, featurep, sizeof(features)))
return -EFAULT;
- if (features & ~VHOST_VDPA_BACKEND_FEATURES)
+ if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
+ BIT_ULL(VHOST_BACKEND_F_SUSPEND)))
+ return -EOPNOTSUPP;
+ if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
+ !vhost_vdpa_can_suspend(v))
return -EOPNOTSUPP;
vhost_set_backend_features(&v->vdev, features);
return 0;
@@ -628,6 +640,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
break;
case VHOST_GET_BACKEND_FEATURES:
features = VHOST_VDPA_BACKEND_FEATURES;
+ if (vhost_vdpa_can_suspend(v))
+ features |= BIT_ULL(VHOST_BACKEND_F_SUSPEND);
if (copy_to_user(featurep, &features, sizeof(features)))
r = -EFAULT;
break;
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index 634cee485abb..1bdd6e363f4c 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
* message
*/
#define VHOST_BACKEND_F_IOTLB_ASID 0x3
+/* Device can be suspended */
+#define VHOST_BACKEND_F_SUSPEND 0x4
#endif
--
2.31.1
Implement suspend operation for vdpa_sim devices, so vhost-vdpa will
offer that backend feature and userspace can effectively suspend the
device.
This is a must before get virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).
Reviewed-by: Stefano Garzarella <[email protected]>
Signed-off-by: Eugenio Pérez <[email protected]>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 +++
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
4 files changed, 28 insertions(+)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 0f2865899647..213883487f9b 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
for (i = 0; i < vdpasim->dev_attr.nas; i++)
vhost_iotlb_reset(&vdpasim->iommu[i]);
+ vdpasim->running = true;
spin_unlock(&vdpasim->iommu_lock);
vdpasim->features = 0;
@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
return 0;
}
+static int vdpasim_suspend(struct vdpa_device *vdpa)
+{
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ int i;
+
+ spin_lock(&vdpasim->lock);
+ vdpasim->running = false;
+ if (vdpasim->running) {
+ /* Check for missed buffers */
+ for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
+ vdpasim_kick_vq(vdpa, i);
+
+ }
+ spin_unlock(&vdpasim->lock);
+
+ return 0;
+}
+
static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset = vdpasim_reset,
+ .suspend = vdpasim_suspend,
.get_config_size = vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
@@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset = vdpasim_reset,
+ .suspend = vdpasim_suspend,
.get_config_size = vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 622782e92239..061986f30911 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -66,6 +66,7 @@ struct vdpasim {
u32 generation;
u64 features;
u32 groups;
+ bool running;
/* spinlock to synchronize iommu table */
spinlock_t iommu_lock;
};
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index 42d401d43911..bcdb1982c378 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -204,6 +204,9 @@ static void vdpasim_blk_work(struct work_struct *work)
if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
goto out;
+ if (!vdpasim->running)
+ goto out;
+
for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 5125976a4df8..886449e88502 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
spin_lock(&vdpasim->lock);
+ if (!vdpasim->running)
+ goto out;
+
if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
goto out;
--
2.31.1
The ioctl adds support for suspending the device from userspace.
This is a must before getting virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).
After a successful return of the ioctl call the device must not process
more virtqueue descriptors. The device can answer to read or writes of
config fields as if it were not suspended. In particular, writing to
"queue_enable" with a value of 1 will not make the device start
processing buffers of the virtqueue.
Signed-off-by: Eugenio Pérez <[email protected]>
---
drivers/vhost/vdpa.c | 19 +++++++++++++++++++
include/uapi/linux/vhost.h | 14 ++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 3d636e192061..7fa671ac4bdf 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -478,6 +478,22 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
return 0;
}
+/* After a successful return of ioctl the device must not process more
+ * virtqueue descriptors. The device can answer to read or writes of config
+ * fields as if it were not suspended. In particular, writing to "queue_enable"
+ * with a value of 1 will not make the device start processing buffers.
+ */
+static long vhost_vdpa_suspend(struct vhost_vdpa *v)
+{
+ struct vdpa_device *vdpa = v->vdpa;
+ const struct vdpa_config_ops *ops = vdpa->config;
+
+ if (!ops->suspend)
+ return -EOPNOTSUPP;
+
+ return ops->suspend(vdpa);
+}
+
static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
void __user *argp)
{
@@ -654,6 +670,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
case VHOST_VDPA_GET_VQS_COUNT:
r = vhost_vdpa_get_vqs_count(v, argp);
break;
+ case VHOST_VDPA_SUSPEND:
+ r = vhost_vdpa_suspend(v);
+ break;
default:
r = vhost_dev_ioctl(&v->vdev, cmd, argp);
if (r == -ENOIOCTLCMD)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index cab645d4a645..6d9f45163155 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -171,4 +171,18 @@
#define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
struct vhost_vring_state)
+/* Suspend or resume a device so it does not process virtqueue requests anymore
+ *
+ * After the return of ioctl with suspend != 0, the device must finish any
+ * pending operations like in flight requests. It must also preserve all the
+ * necessary state (the virtqueue vring base plus the possible device specific
+ * states) that is required for restoring in the future. The device must not
+ * change its configuration after that point.
+ *
+ * After the return of ioctl with suspend == 0, the device can continue
+ * processing buffers as long as typical conditions are met (vq is enabled,
+ * DRIVER_OK status bit is enabled, etc).
+ */
+#define VHOST_VDPA_SUSPEND _IOW(VHOST_VIRTIO, 0x7D, int)
+
#endif
--
2.31.1
On Thu, Jun 23, 2022 at 06:07:36PM +0200, Eugenio P?rez wrote:
>Userland knows if it can suspend the device or not by checking this feature
>bit.
>
>It's only offered if the vdpa driver backend implements the suspend()
>operation callback, and to offer it or userland to ack it if the backend
>does not offer that callback is an error.
Should we document in the previous patch that the callback must be
implemented only if the drive/device support it?
The rest LGTM although I have a doubt whether it is better to move this
patch after patch 3, or merge it with patch 3, for bisectability since
we enable the feature here but if the userspace calls ioctl() with
VHOST_VDPA_SUSPEND we reply back that it is not supported.
Thanks,
Stefano
>
>Signed-off-by: Eugenio P?rez <[email protected]>
>---
> drivers/vhost/vdpa.c | 16 +++++++++++++++-
> include/uapi/linux/vhost_types.h | 2 ++
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>index 23dcbfdfa13b..3d636e192061 100644
>--- a/drivers/vhost/vdpa.c
>+++ b/drivers/vhost/vdpa.c
>@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> return 0;
> }
>
>+static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
>+{
>+ struct vdpa_device *vdpa = v->vdpa;
>+ const struct vdpa_config_ops *ops = vdpa->config;
>+
>+ return ops->suspend;
>+}
>+
> static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> {
> struct vdpa_device *vdpa = v->vdpa;
>@@ -577,7 +585,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> if (cmd == VHOST_SET_BACKEND_FEATURES) {
> if (copy_from_user(&features, featurep, sizeof(features)))
> return -EFAULT;
>- if (features & ~VHOST_VDPA_BACKEND_FEATURES)
>+ if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
>+ BIT_ULL(VHOST_BACKEND_F_SUSPEND)))
>+ return -EOPNOTSUPP;
>+ if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
>+ !vhost_vdpa_can_suspend(v))
> return -EOPNOTSUPP;
> vhost_set_backend_features(&v->vdev, features);
> return 0;
>@@ -628,6 +640,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> break;
> case VHOST_GET_BACKEND_FEATURES:
> features = VHOST_VDPA_BACKEND_FEATURES;
>+ if (vhost_vdpa_can_suspend(v))
>+ features |= BIT_ULL(VHOST_BACKEND_F_SUSPEND);
> if (copy_to_user(featurep, &features, sizeof(features)))
> r = -EFAULT;
> break;
>diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
>index 634cee485abb..1bdd6e363f4c 100644
>--- a/include/uapi/linux/vhost_types.h
>+++ b/include/uapi/linux/vhost_types.h
>@@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> * message
> */
> #define VHOST_BACKEND_F_IOTLB_ASID 0x3
>+/* Device can be suspended */
>+#define VHOST_BACKEND_F_SUSPEND 0x4
>
> #endif
>--
>2.31.1
>
On Thu, Jun 23, 2022 at 06:07:37PM +0200, Eugenio P?rez wrote:
>The ioctl adds support for suspending the device from userspace.
>
>This is a must before getting virtqueue indexes (base) for live migration,
>since the device could modify them after userland gets them. There are
>individual ways to perform that action for some devices
>(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
>way to perform it for any vhost device (and, in particular, vhost-vdpa).
>
>After a successful return of the ioctl call the device must not process
>more virtqueue descriptors. The device can answer to read or writes of
>config fields as if it were not suspended. In particular, writing to
>"queue_enable" with a value of 1 will not make the device start
>processing buffers of the virtqueue.
>
>Signed-off-by: Eugenio P?rez <[email protected]>
>---
> drivers/vhost/vdpa.c | 19 +++++++++++++++++++
> include/uapi/linux/vhost.h | 14 ++++++++++++++
> 2 files changed, 33 insertions(+)
>
>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>index 3d636e192061..7fa671ac4bdf 100644
>--- a/drivers/vhost/vdpa.c
>+++ b/drivers/vhost/vdpa.c
>@@ -478,6 +478,22 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> return 0;
> }
>
>+/* After a successful return of ioctl the device must not process more
>+ * virtqueue descriptors. The device can answer to read or writes of config
>+ * fields as if it were not suspended. In particular, writing to "queue_enable"
>+ * with a value of 1 will not make the device start processing buffers.
>+ */
>+static long vhost_vdpa_suspend(struct vhost_vdpa *v)
>+{
>+ struct vdpa_device *vdpa = v->vdpa;
>+ const struct vdpa_config_ops *ops = vdpa->config;
>+
>+ if (!ops->suspend)
>+ return -EOPNOTSUPP;
>+
>+ return ops->suspend(vdpa);
>+}
>+
> static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> void __user *argp)
> {
>@@ -654,6 +670,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> case VHOST_VDPA_GET_VQS_COUNT:
> r = vhost_vdpa_get_vqs_count(v, argp);
> break;
>+ case VHOST_VDPA_SUSPEND:
>+ r = vhost_vdpa_suspend(v);
>+ break;
> default:
> r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> if (r == -ENOIOCTLCMD)
>diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>index cab645d4a645..6d9f45163155 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -171,4 +171,18 @@
> #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> struct vhost_vring_state)
>
>+/* Suspend or resume a device so it does not process virtqueue requests anymore
>+ *
>+ * After the return of ioctl with suspend != 0, the device must finish any
>+ * pending operations like in flight requests. It must also preserve all the
>+ * necessary state (the virtqueue vring base plus the possible device specific
>+ * states) that is required for restoring in the future. The device must not
>+ * change its configuration after that point.
>+ *
>+ * After the return of ioctl with suspend == 0, the device can continue
>+ * processing buffers as long as typical conditions are met (vq is enabled,
>+ * DRIVER_OK status bit is enabled, etc).
>+ */
>+#define VHOST_VDPA_SUSPEND _IOW(VHOST_VIRTIO, 0x7D, int)
^
IIUC we are not using the argument anymore, so this should be changed in
_IO(VHOST_VIRTIO, 0x7D).
And we should update a bit the documentation.
Thanks,
Stefano
On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
>
> Userland knows if it can suspend the device or not by checking this feature
> bit.
>
> It's only offered if the vdpa driver backend implements the suspend()
> operation callback, and to offer it or userland to ack it if the backend
> does not offer that callback is an error.
>
> Signed-off-by: Eugenio Pérez <[email protected]>
> ---
> drivers/vhost/vdpa.c | 16 +++++++++++++++-
> include/uapi/linux/vhost_types.h | 2 ++
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 23dcbfdfa13b..3d636e192061 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> return 0;
> }
>
> +static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
> +{
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> +
> + return ops->suspend;
> +}
> +
> static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> {
> struct vdpa_device *vdpa = v->vdpa;
> @@ -577,7 +585,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> if (cmd == VHOST_SET_BACKEND_FEATURES) {
> if (copy_from_user(&features, featurep, sizeof(features)))
> return -EFAULT;
> - if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> + if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> + BIT_ULL(VHOST_BACKEND_F_SUSPEND)))
> + return -EOPNOTSUPP;
> + if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> + !vhost_vdpa_can_suspend(v))
Do we need to advertise this to the management?
Thanks
> return -EOPNOTSUPP;
> vhost_set_backend_features(&v->vdev, features);
> return 0;
> @@ -628,6 +640,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> break;
> case VHOST_GET_BACKEND_FEATURES:
> features = VHOST_VDPA_BACKEND_FEATURES;
> + if (vhost_vdpa_can_suspend(v))
> + features |= BIT_ULL(VHOST_BACKEND_F_SUSPEND);
> if (copy_to_user(featurep, &features, sizeof(features)))
> r = -EFAULT;
> break;
> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> index 634cee485abb..1bdd6e363f4c 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> * message
> */
> #define VHOST_BACKEND_F_IOTLB_ASID 0x3
> +/* Device can be suspended */
> +#define VHOST_BACKEND_F_SUSPEND 0x4
>
> #endif
> --
> 2.31.1
>
On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
>
> Implement suspend operation for vdpa_sim devices, so vhost-vdpa will
> offer that backend feature and userspace can effectively suspend the
> device.
>
> This is a must before get virtqueue indexes (base) for live migration,
> since the device could modify them after userland gets them. There are
> individual ways to perform that action for some devices
> (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> way to perform it for any vhost device (and, in particular, vhost-vdpa).
>
> Reviewed-by: Stefano Garzarella <[email protected]>
> Signed-off-by: Eugenio Pérez <[email protected]>
> ---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 +++
> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
> 4 files changed, 28 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 0f2865899647..213883487f9b 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> for (i = 0; i < vdpasim->dev_attr.nas; i++)
> vhost_iotlb_reset(&vdpasim->iommu[i]);
>
> + vdpasim->running = true;
> spin_unlock(&vdpasim->iommu_lock);
>
> vdpasim->features = 0;
> @@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
> return 0;
> }
>
> +static int vdpasim_suspend(struct vdpa_device *vdpa)
> +{
> + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> + int i;
> +
> + spin_lock(&vdpasim->lock);
> + vdpasim->running = false;
> + if (vdpasim->running) {
> + /* Check for missed buffers */
> + for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
> + vdpasim_kick_vq(vdpa, i);
This seems only valid if we allow resuming?
Thanks
> +
> + }
> + spin_unlock(&vdpasim->lock);
> +
> + return 0;
> +}
> +
> static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> @@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .get_status = vdpasim_get_status,
> .set_status = vdpasim_set_status,
> .reset = vdpasim_reset,
> + .suspend = vdpasim_suspend,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> .set_config = vdpasim_set_config,
> @@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .get_status = vdpasim_get_status,
> .set_status = vdpasim_set_status,
> .reset = vdpasim_reset,
> + .suspend = vdpasim_suspend,
> .get_config_size = vdpasim_get_config_size,
> .get_config = vdpasim_get_config,
> .set_config = vdpasim_set_config,
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index 622782e92239..061986f30911 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -66,6 +66,7 @@ struct vdpasim {
> u32 generation;
> u64 features;
> u32 groups;
> + bool running;
> /* spinlock to synchronize iommu table */
> spinlock_t iommu_lock;
> };
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index 42d401d43911..bcdb1982c378 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -204,6 +204,9 @@ static void vdpasim_blk_work(struct work_struct *work)
> if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> goto out;
>
> + if (!vdpasim->running)
> + goto out;
> +
> for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> index 5125976a4df8..886449e88502 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> @@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
>
> spin_lock(&vdpasim->lock);
>
> + if (!vdpasim->running)
> + goto out;
> +
> if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> goto out;
>
> --
> 2.31.1
>
On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
>
> The ioctl adds support for suspending the device from userspace.
>
> This is a must before getting virtqueue indexes (base) for live migration,
> since the device could modify them after userland gets them. There are
> individual ways to perform that action for some devices
> (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> way to perform it for any vhost device (and, in particular, vhost-vdpa).
>
> After a successful return of the ioctl call the device must not process
> more virtqueue descriptors. The device can answer to read or writes of
> config fields as if it were not suspended. In particular, writing to
> "queue_enable" with a value of 1 will not make the device start
> processing buffers of the virtqueue.
>
> Signed-off-by: Eugenio Pérez <[email protected]>
> ---
> drivers/vhost/vdpa.c | 19 +++++++++++++++++++
> include/uapi/linux/vhost.h | 14 ++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 3d636e192061..7fa671ac4bdf 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -478,6 +478,22 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> return 0;
> }
>
> +/* After a successful return of ioctl the device must not process more
> + * virtqueue descriptors. The device can answer to read or writes of config
> + * fields as if it were not suspended. In particular, writing to "queue_enable"
> + * with a value of 1 will not make the device start processing buffers.
> + */
> +static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> +{
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> +
> + if (!ops->suspend)
> + return -EOPNOTSUPP;
> +
> + return ops->suspend(vdpa);
> +}
> +
> static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> void __user *argp)
> {
> @@ -654,6 +670,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> case VHOST_VDPA_GET_VQS_COUNT:
> r = vhost_vdpa_get_vqs_count(v, argp);
> break;
> + case VHOST_VDPA_SUSPEND:
> + r = vhost_vdpa_suspend(v);
> + break;
> default:
> r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> if (r == -ENOIOCTLCMD)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index cab645d4a645..6d9f45163155 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -171,4 +171,18 @@
> #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> struct vhost_vring_state)
>
> +/* Suspend or resume a device so it does not process virtqueue requests anymore
> + *
> + * After the return of ioctl with suspend != 0, the device must finish any
> + * pending operations like in flight requests.
I'm not sure we should mandate the flush here. This probably blocks us
from adding inflight descriptor reporting in the future.
Thanks
It must also preserve all the
> + * necessary state (the virtqueue vring base plus the possible device specific
> + * states) that is required for restoring in the future. The device must not
> + * change its configuration after that point.
> + *
> + * After the return of ioctl with suspend == 0, the device can continue
> + * processing buffers as long as typical conditions are met (vq is enabled,
> + * DRIVER_OK status bit is enabled, etc).
> + */
> +#define VHOST_VDPA_SUSPEND _IOW(VHOST_VIRTIO, 0x7D, int)
> +
> #endif
> --
> 2.31.1
>
On Tue, Jun 28, 2022 at 3:43 PM Stefano Garzarella <[email protected]> wrote:
>
> On Thu, Jun 23, 2022 at 06:07:36PM +0200, Eugenio Pérez wrote:
> >Userland knows if it can suspend the device or not by checking this feature
> >bit.
> >
> >It's only offered if the vdpa driver backend implements the suspend()
> >operation callback, and to offer it or userland to ack it if the backend
> >does not offer that callback is an error.
>
> Should we document in the previous patch that the callback must be
> implemented only if the drive/device support it?
>
It's marked as optional in the doc, following other optional callbacks
like set_group_asid for example. But I'm ok with documenting this
behavior further.
> The rest LGTM although I have a doubt whether it is better to move this
> patch after patch 3, or merge it with patch 3, for bisectability since
> we enable the feature here but if the userspace calls ioctl() with
> VHOST_VDPA_SUSPEND we reply back that it is not supported.
>
I'm fine with moving it, but we will have that behavior with all the
devices anyway. Regarding userspace, we just replace ENOIOCTL with
EOPNOTSUPP. Or I'm missing something?
Thanks!
> Thanks,
> Stefano
>
> >
> >Signed-off-by: Eugenio Pérez <[email protected]>
> >---
> > drivers/vhost/vdpa.c | 16 +++++++++++++++-
> > include/uapi/linux/vhost_types.h | 2 ++
> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> >index 23dcbfdfa13b..3d636e192061 100644
> >--- a/drivers/vhost/vdpa.c
> >+++ b/drivers/vhost/vdpa.c
> >@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> > return 0;
> > }
> >
> >+static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
> >+{
> >+ struct vdpa_device *vdpa = v->vdpa;
> >+ const struct vdpa_config_ops *ops = vdpa->config;
> >+
> >+ return ops->suspend;
> >+}
> >+
> > static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> > {
> > struct vdpa_device *vdpa = v->vdpa;
> >@@ -577,7 +585,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > if (copy_from_user(&features, featurep, sizeof(features)))
> > return -EFAULT;
> >- if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> >+ if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> >+ BIT_ULL(VHOST_BACKEND_F_SUSPEND)))
> >+ return -EOPNOTSUPP;
> >+ if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> >+ !vhost_vdpa_can_suspend(v))
> > return -EOPNOTSUPP;
> > vhost_set_backend_features(&v->vdev, features);
> > return 0;
> >@@ -628,6 +640,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > break;
> > case VHOST_GET_BACKEND_FEATURES:
> > features = VHOST_VDPA_BACKEND_FEATURES;
> >+ if (vhost_vdpa_can_suspend(v))
> >+ features |= BIT_ULL(VHOST_BACKEND_F_SUSPEND);
> > if (copy_to_user(featurep, &features, sizeof(features)))
> > r = -EFAULT;
> > break;
> >diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> >index 634cee485abb..1bdd6e363f4c 100644
> >--- a/include/uapi/linux/vhost_types.h
> >+++ b/include/uapi/linux/vhost_types.h
> >@@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> > * message
> > */
> > #define VHOST_BACKEND_F_IOTLB_ASID 0x3
> >+/* Device can be suspended */
> >+#define VHOST_BACKEND_F_SUSPEND 0x4
> >
> > #endif
> >--
> >2.31.1
> >
>
On Wed, Jun 29, 2022 at 6:12 AM Jason Wang <[email protected]> wrote:
>
> On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
> >
> > Userland knows if it can suspend the device or not by checking this feature
> > bit.
> >
> > It's only offered if the vdpa driver backend implements the suspend()
> > operation callback, and to offer it or userland to ack it if the backend
> > does not offer that callback is an error.
> >
> > Signed-off-by: Eugenio Pérez <[email protected]>
> > ---
> > drivers/vhost/vdpa.c | 16 +++++++++++++++-
> > include/uapi/linux/vhost_types.h | 2 ++
> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 23dcbfdfa13b..3d636e192061 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> > return 0;
> > }
> >
> > +static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
> > +{
> > + struct vdpa_device *vdpa = v->vdpa;
> > + const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > + return ops->suspend;
> > +}
> > +
> > static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
> > {
> > struct vdpa_device *vdpa = v->vdpa;
> > @@ -577,7 +585,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > if (copy_from_user(&features, featurep, sizeof(features)))
> > return -EFAULT;
> > - if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> > + if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> > + BIT_ULL(VHOST_BACKEND_F_SUSPEND)))
> > + return -EOPNOTSUPP;
> > + if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> > + !vhost_vdpa_can_suspend(v))
>
> Do we need to advertise this to the management?
>
Not sure if I follow, the feature bit is not exposed if the backend
cannot suspend.
Thanks!
> Thanks
>
> > return -EOPNOTSUPP;
> > vhost_set_backend_features(&v->vdev, features);
> > return 0;
> > @@ -628,6 +640,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > break;
> > case VHOST_GET_BACKEND_FEATURES:
> > features = VHOST_VDPA_BACKEND_FEATURES;
> > + if (vhost_vdpa_can_suspend(v))
> > + features |= BIT_ULL(VHOST_BACKEND_F_SUSPEND);
> > if (copy_to_user(featurep, &features, sizeof(features)))
> > r = -EFAULT;
> > break;
> > diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> > index 634cee485abb..1bdd6e363f4c 100644
> > --- a/include/uapi/linux/vhost_types.h
> > +++ b/include/uapi/linux/vhost_types.h
> > @@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
> > * message
> > */
> > #define VHOST_BACKEND_F_IOTLB_ASID 0x3
> > +/* Device can be suspended */
> > +#define VHOST_BACKEND_F_SUSPEND 0x4
> >
> > #endif
> > --
> > 2.31.1
> >
>
On Tue, Jun 28, 2022 at 3:45 PM Stefano Garzarella <[email protected]> wrote:
>
> On Thu, Jun 23, 2022 at 06:07:37PM +0200, Eugenio Pérez wrote:
> >The ioctl adds support for suspending the device from userspace.
> >
> >This is a must before getting virtqueue indexes (base) for live migration,
> >since the device could modify them after userland gets them. There are
> >individual ways to perform that action for some devices
> >(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> >way to perform it for any vhost device (and, in particular, vhost-vdpa).
> >
> >After a successful return of the ioctl call the device must not process
> >more virtqueue descriptors. The device can answer to read or writes of
> >config fields as if it were not suspended. In particular, writing to
> >"queue_enable" with a value of 1 will not make the device start
> >processing buffers of the virtqueue.
> >
> >Signed-off-by: Eugenio Pérez <[email protected]>
> >---
> > drivers/vhost/vdpa.c | 19 +++++++++++++++++++
> > include/uapi/linux/vhost.h | 14 ++++++++++++++
> > 2 files changed, 33 insertions(+)
> >
> >diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> >index 3d636e192061..7fa671ac4bdf 100644
> >--- a/drivers/vhost/vdpa.c
> >+++ b/drivers/vhost/vdpa.c
> >@@ -478,6 +478,22 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> > return 0;
> > }
> >
> >+/* After a successful return of ioctl the device must not process more
> >+ * virtqueue descriptors. The device can answer to read or writes of config
> >+ * fields as if it were not suspended. In particular, writing to "queue_enable"
> >+ * with a value of 1 will not make the device start processing buffers.
> >+ */
> >+static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> >+{
> >+ struct vdpa_device *vdpa = v->vdpa;
> >+ const struct vdpa_config_ops *ops = vdpa->config;
> >+
> >+ if (!ops->suspend)
> >+ return -EOPNOTSUPP;
> >+
> >+ return ops->suspend(vdpa);
> >+}
> >+
> > static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> > void __user *argp)
> > {
> >@@ -654,6 +670,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > case VHOST_VDPA_GET_VQS_COUNT:
> > r = vhost_vdpa_get_vqs_count(v, argp);
> > break;
> >+ case VHOST_VDPA_SUSPEND:
> >+ r = vhost_vdpa_suspend(v);
> >+ break;
> > default:
> > r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> > if (r == -ENOIOCTLCMD)
> >diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> >index cab645d4a645..6d9f45163155 100644
> >--- a/include/uapi/linux/vhost.h
> >+++ b/include/uapi/linux/vhost.h
> >@@ -171,4 +171,18 @@
> > #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> > struct vhost_vring_state)
> >
> >+/* Suspend or resume a device so it does not process virtqueue requests anymore
> >+ *
> >+ * After the return of ioctl with suspend != 0, the device must finish any
> >+ * pending operations like in flight requests. It must also preserve all the
> >+ * necessary state (the virtqueue vring base plus the possible device specific
> >+ * states) that is required for restoring in the future. The device must not
> >+ * change its configuration after that point.
> >+ *
> >+ * After the return of ioctl with suspend == 0, the device can continue
> >+ * processing buffers as long as typical conditions are met (vq is enabled,
> >+ * DRIVER_OK status bit is enabled, etc).
> >+ */
> >+#define VHOST_VDPA_SUSPEND _IOW(VHOST_VIRTIO, 0x7D, int)
> ^
> IIUC we are not using the argument anymore, so this should be changed in
> _IO(VHOST_VIRTIO, 0x7D).
>
> And we should update a bit the documentation.
>
Totally right, replacing it for the next version.
Thanks!
> Thanks,
> Stefano
>
On Wed, Jun 29, 2022 at 6:18 AM Jason Wang <[email protected]> wrote:
>
> On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
> >
> > Implement suspend operation for vdpa_sim devices, so vhost-vdpa will
> > offer that backend feature and userspace can effectively suspend the
> > device.
> >
> > This is a must before get virtqueue indexes (base) for live migration,
> > since the device could modify them after userland gets them. There are
> > individual ways to perform that action for some devices
> > (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> > way to perform it for any vhost device (and, in particular, vhost-vdpa).
> >
> > Reviewed-by: Stefano Garzarella <[email protected]>
> > Signed-off-by: Eugenio Pérez <[email protected]>
> > ---
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
> > drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
> > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 +++
> > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
> > 4 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index 0f2865899647..213883487f9b 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> > for (i = 0; i < vdpasim->dev_attr.nas; i++)
> > vhost_iotlb_reset(&vdpasim->iommu[i]);
> >
> > + vdpasim->running = true;
> > spin_unlock(&vdpasim->iommu_lock);
> >
> > vdpasim->features = 0;
> > @@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
> > return 0;
> > }
> >
> > +static int vdpasim_suspend(struct vdpa_device *vdpa)
> > +{
> > + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > + int i;
> > +
> > + spin_lock(&vdpasim->lock);
> > + vdpasim->running = false;
> > + if (vdpasim->running) {
> > + /* Check for missed buffers */
> > + for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
> > + vdpasim_kick_vq(vdpa, i);
>
> This seems only valid if we allow resuming?
>
Right, deleting for v7.
Thanks!
> Thanks
>
> > +
> > + }
> > + spin_unlock(&vdpasim->lock);
> > +
> > + return 0;
> > +}
> > +
> > static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
> > {
> > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > @@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> > .get_status = vdpasim_get_status,
> > .set_status = vdpasim_set_status,
> > .reset = vdpasim_reset,
> > + .suspend = vdpasim_suspend,
> > .get_config_size = vdpasim_get_config_size,
> > .get_config = vdpasim_get_config,
> > .set_config = vdpasim_set_config,
> > @@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> > .get_status = vdpasim_get_status,
> > .set_status = vdpasim_set_status,
> > .reset = vdpasim_reset,
> > + .suspend = vdpasim_suspend,
> > .get_config_size = vdpasim_get_config_size,
> > .get_config = vdpasim_get_config,
> > .set_config = vdpasim_set_config,
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> > index 622782e92239..061986f30911 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> > @@ -66,6 +66,7 @@ struct vdpasim {
> > u32 generation;
> > u64 features;
> > u32 groups;
> > + bool running;
> > /* spinlock to synchronize iommu table */
> > spinlock_t iommu_lock;
> > };
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > index 42d401d43911..bcdb1982c378 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > @@ -204,6 +204,9 @@ static void vdpasim_blk_work(struct work_struct *work)
> > if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > goto out;
> >
> > + if (!vdpasim->running)
> > + goto out;
> > +
> > for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index 5125976a4df8..886449e88502 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
> >
> > spin_lock(&vdpasim->lock);
> >
> > + if (!vdpasim->running)
> > + goto out;
> > +
> > if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > goto out;
> >
> > --
> > 2.31.1
> >
>
On Wed, Jun 29, 2022 at 6:16 AM Jason Wang <[email protected]> wrote:
>
> On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
> >
> > The ioctl adds support for suspending the device from userspace.
> >
> > This is a must before getting virtqueue indexes (base) for live migration,
> > since the device could modify them after userland gets them. There are
> > individual ways to perform that action for some devices
> > (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> > way to perform it for any vhost device (and, in particular, vhost-vdpa).
> >
> > After a successful return of the ioctl call the device must not process
> > more virtqueue descriptors. The device can answer to read or writes of
> > config fields as if it were not suspended. In particular, writing to
> > "queue_enable" with a value of 1 will not make the device start
> > processing buffers of the virtqueue.
> >
> > Signed-off-by: Eugenio Pérez <[email protected]>
> > ---
> > drivers/vhost/vdpa.c | 19 +++++++++++++++++++
> > include/uapi/linux/vhost.h | 14 ++++++++++++++
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 3d636e192061..7fa671ac4bdf 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -478,6 +478,22 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> > return 0;
> > }
> >
> > +/* After a successful return of ioctl the device must not process more
> > + * virtqueue descriptors. The device can answer to read or writes of config
> > + * fields as if it were not suspended. In particular, writing to "queue_enable"
> > + * with a value of 1 will not make the device start processing buffers.
> > + */
> > +static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> > +{
> > + struct vdpa_device *vdpa = v->vdpa;
> > + const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > + if (!ops->suspend)
> > + return -EOPNOTSUPP;
> > +
> > + return ops->suspend(vdpa);
> > +}
> > +
> > static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> > void __user *argp)
> > {
> > @@ -654,6 +670,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > case VHOST_VDPA_GET_VQS_COUNT:
> > r = vhost_vdpa_get_vqs_count(v, argp);
> > break;
> > + case VHOST_VDPA_SUSPEND:
> > + r = vhost_vdpa_suspend(v);
> > + break;
> > default:
> > r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> > if (r == -ENOIOCTLCMD)
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index cab645d4a645..6d9f45163155 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -171,4 +171,18 @@
> > #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> > struct vhost_vring_state)
> >
> > +/* Suspend or resume a device so it does not process virtqueue requests anymore
> > + *
> > + * After the return of ioctl with suspend != 0, the device must finish any
> > + * pending operations like in flight requests.
>
> I'm not sure we should mandate the flush here. This probably blocks us
> from adding inflight descriptor reporting in the future.
>
That's right. Maybe we should add a flags argument to allow not to
flush in flight descriptors in the future? Or maybe the right solution
is to discard that requirement and to mandate in_order to be
migratable at the moment?
Thanks!
> Thanks
>
> It must also preserve all the
> > + * necessary state (the virtqueue vring base plus the possible device specific
> > + * states) that is required for restoring in the future. The device must not
> > + * change its configuration after that point.
> > + *
> > + * After the return of ioctl with suspend == 0, the device can continue
> > + * processing buffers as long as typical conditions are met (vq is enabled,
> > + * DRIVER_OK status bit is enabled, etc).
> > + */
> > +#define VHOST_VDPA_SUSPEND _IOW(VHOST_VIRTIO, 0x7D, int)
> > +
> > #endif
> > --
> > 2.31.1
> >
>
On Fri, Jul 08, 2022 at 01:38:45PM +0200, Eugenio Perez Martin wrote:
>On Tue, Jun 28, 2022 at 3:43 PM Stefano Garzarella <[email protected]> wrote:
>>
>> On Thu, Jun 23, 2022 at 06:07:36PM +0200, Eugenio P?rez wrote:
>> >Userland knows if it can suspend the device or not by checking this feature
>> >bit.
>> >
>> >It's only offered if the vdpa driver backend implements the suspend()
>> >operation callback, and to offer it or userland to ack it if the backend
>> >does not offer that callback is an error.
>>
>> Should we document in the previous patch that the callback must be
>> implemented only if the drive/device support it?
>>
>
>It's marked as optional in the doc, following other optional callbacks
>like set_group_asid for example. But I'm ok with documenting this
>behavior further.
>
>> The rest LGTM although I have a doubt whether it is better to move this
>> patch after patch 3, or merge it with patch 3, for bisectability since
>> we enable the feature here but if the userspace calls ioctl() with
>> VHOST_VDPA_SUSPEND we reply back that it is not supported.
>>
>
>I'm fine with moving it, but we will have that behavior with all the
>devices anyway. Regarding userspace, we just replace ENOIOCTL with
>EOPNOTSUPP. Or I'm missing something?
Yep, you're right, this is fine! ;-)
Stefano
On Fri, Jul 8, 2022 at 7:53 PM Eugenio Perez Martin <[email protected]> wrote:
>
> On Wed, Jun 29, 2022 at 6:16 AM Jason Wang <[email protected]> wrote:
> >
> > On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <[email protected]> wrote:
> > >
> > > The ioctl adds support for suspending the device from userspace.
> > >
> > > This is a must before getting virtqueue indexes (base) for live migration,
> > > since the device could modify them after userland gets them. There are
> > > individual ways to perform that action for some devices
> > > (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
> > > way to perform it for any vhost device (and, in particular, vhost-vdpa).
> > >
> > > After a successful return of the ioctl call the device must not process
> > > more virtqueue descriptors. The device can answer to read or writes of
> > > config fields as if it were not suspended. In particular, writing to
> > > "queue_enable" with a value of 1 will not make the device start
> > > processing buffers of the virtqueue.
> > >
> > > Signed-off-by: Eugenio Pérez <[email protected]>
> > > ---
> > > drivers/vhost/vdpa.c | 19 +++++++++++++++++++
> > > include/uapi/linux/vhost.h | 14 ++++++++++++++
> > > 2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index 3d636e192061..7fa671ac4bdf 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -478,6 +478,22 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> > > return 0;
> > > }
> > >
> > > +/* After a successful return of ioctl the device must not process more
> > > + * virtqueue descriptors. The device can answer to read or writes of config
> > > + * fields as if it were not suspended. In particular, writing to "queue_enable"
> > > + * with a value of 1 will not make the device start processing buffers.
> > > + */
> > > +static long vhost_vdpa_suspend(struct vhost_vdpa *v)
> > > +{
> > > + struct vdpa_device *vdpa = v->vdpa;
> > > + const struct vdpa_config_ops *ops = vdpa->config;
> > > +
> > > + if (!ops->suspend)
> > > + return -EOPNOTSUPP;
> > > +
> > > + return ops->suspend(vdpa);
> > > +}
> > > +
> > > static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> > > void __user *argp)
> > > {
> > > @@ -654,6 +670,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > > case VHOST_VDPA_GET_VQS_COUNT:
> > > r = vhost_vdpa_get_vqs_count(v, argp);
> > > break;
> > > + case VHOST_VDPA_SUSPEND:
> > > + r = vhost_vdpa_suspend(v);
> > > + break;
> > > default:
> > > r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> > > if (r == -ENOIOCTLCMD)
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index cab645d4a645..6d9f45163155 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -171,4 +171,18 @@
> > > #define VHOST_VDPA_SET_GROUP_ASID _IOW(VHOST_VIRTIO, 0x7C, \
> > > struct vhost_vring_state)
> > >
> > > +/* Suspend or resume a device so it does not process virtqueue requests anymore
> > > + *
> > > + * After the return of ioctl with suspend != 0, the device must finish any
> > > + * pending operations like in flight requests.
> >
> > I'm not sure we should mandate the flush here. This probably blocks us
> > from adding inflight descriptor reporting in the future.
> >
>
> That's right. Maybe we should add a flags argument to allow not to
> flush in flight descriptors in the future? Or maybe the right solution
> is to discard that requirement and to mandate in_order to be
> migratable at the moment?
I think it's better not to limit the device behaviour like flush or
in_order here. This may simplify the work for adding inflight
descriptor support.
For the device that doesn't care about the inflight descriptor, this
patch is sufficient for doing live migration.
For the device that requires an inflight descriptor, this patch is
insufficient, it requires future extension to get those descriptors.
In this case, device has the flexibility to flush or not so:
1) if we don't get any inflight descriptors, the device may do the flush before
2) if we get inflight descriptors, we need to restore them
Thanks
>
> Thanks!
>
> > Thanks
> >
> > It must also preserve all the
> > > + * necessary state (the virtqueue vring base plus the possible device specific
> > > + * states) that is required for restoring in the future. The device must not
> > > + * change its configuration after that point.
> > > + *
> > > + * After the return of ioctl with suspend == 0, the device can continue
> > > + * processing buffers as long as typical conditions are met (vq is enabled,
> > > + * DRIVER_OK status bit is enabled, etc).
> > > + */
> > > +#define VHOST_VDPA_SUSPEND _IOW(VHOST_VIRTIO, 0x7D, int)
> > > +
> > > #endif
> > > --
> > > 2.31.1
> > >
> >
>