[PATCH v6 0/4] Implement vdpasim suspend operation

[PATCH v6 0/4] Implement vdpasim suspend operation

[Eugenio Pérez]

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

[PATCH v6 1/4] vdpa: Add suspend operation

[Eugenio Pérez]

This operation is optional: It it's not implemented, backend feature bit
will not be exposed.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/linux/vdpa.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 7b4a13d3bd91..d282f464d2f1 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -218,6 +218,9 @@ struct vdpa_map_file {
  * @reset:			Reset device
  *				@vdev: vdpa device
  *				Returns integer: success (0) or error (< 0)
+ * @suspend:			Suspend or resume the device (optional)
+ *				@vdev: vdpa device
+ *				Returns integer: success (0) or error (< 0)
  * @get_config_size:		Get the size of the configuration space includes
  *				fields that are conditional on feature bits.
  *				@vdev: vdpa device
@@ -319,6 +322,7 @@ struct vdpa_config_ops {
 	u8 (*get_status)(struct vdpa_device *vdev);
 	void (*set_status)(struct vdpa_device *vdev, u8 status);
 	int (*reset)(struct vdpa_device *vdev);
+	int (*suspend)(struct vdpa_device *vdev);
 	size_t (*get_config_size)(struct vdpa_device *vdev);
 	void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
 			   void *buf, unsigned int len);
-- 
2.31.1

[PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Eugenio Pérez]

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 <eperezma@redhat.com>
---
 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

[PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Eugenio Pérez]

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 <eperezma@redhat.com>
---
 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

[PATCH v6 4/4] vdpa_sim: Implement suspend vdpa op

[Eugenio Pérez]

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 <sgarzare@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 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

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Stefano Garzarella]

On Thu, Jun 23, 2022 at 06:07:35PM +0200, Eugenio Pérez wrote:
>This operation is optional: It it's not implemented, backend feature bit
>will not be exposed.
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> include/linux/vdpa.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>index 7b4a13d3bd91..d282f464d2f1 100644
>--- a/include/linux/vdpa.h
>+++ b/include/linux/vdpa.h
>@@ -218,6 +218,9 @@ struct vdpa_map_file {
>  * @reset:			Reset device
>  *				@vdev: vdpa device
>  *				Returns integer: success (0) or error (< 0)
>+ * @suspend:			Suspend or resume the device (optional)
                                            ^
IIUC we removed the resume operation (that should be done with reset),
so should we update this documentation?

Thanks,
Stefano

>+ *				@vdev: vdpa device
>+ *				Returns integer: success (0) or error (< 0)
>  * @get_config_size:		Get the size of the configuration space includes
>  *				fields that are conditional on feature bits.
>  *				@vdev: vdpa device
>@@ -319,6 +322,7 @@ struct vdpa_config_ops {
> 	u8 (*get_status)(struct vdpa_device *vdev);
> 	void (*set_status)(struct vdpa_device *vdev, u8 status);
> 	int (*reset)(struct vdpa_device *vdev);
>+	int (*suspend)(struct vdpa_device *vdev);
> 	size_t (*get_config_size)(struct vdpa_device *vdev);
> 	void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
> 			   void *buf, unsigned int len);
>-- 
>2.31.1
>

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Stefano Garzarella]

On Thu, Jun 23, 2022 at 06:07:35PM +0200, Eugenio Pérez wrote:
>This operation is optional: It it's not implemented, backend feature bit
>will not be exposed.
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> include/linux/vdpa.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>index 7b4a13d3bd91..d282f464d2f1 100644
>--- a/include/linux/vdpa.h
>+++ b/include/linux/vdpa.h
>@@ -218,6 +218,9 @@ struct vdpa_map_file {
>  * @reset:			Reset device
>  *				@vdev: vdpa device
>  *				Returns integer: success (0) or error (< 0)
>+ * @suspend:			Suspend or resume the device (optional)
                                            ^
IIUC we removed the resume operation (that should be done with reset),
so should we update this documentation?

Thanks,
Stefano

>+ *				@vdev: vdpa device
>+ *				Returns integer: success (0) or error (< 0)
>  * @get_config_size:		Get the size of the configuration space includes
>  *				fields that are conditional on feature bits.
>  *				@vdev: vdpa device
>@@ -319,6 +322,7 @@ struct vdpa_config_ops {
> 	u8 (*get_status)(struct vdpa_device *vdev);
> 	void (*set_status)(struct vdpa_device *vdev, u8 status);
> 	int (*reset)(struct vdpa_device *vdev);
>+	int (*suspend)(struct vdpa_device *vdev);
> 	size_t (*get_config_size)(struct vdpa_device *vdev);
> 	void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
> 			   void *buf, unsigned int len);
>-- 
>2.31.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Stefano Garzarella]

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 <eperezma@redhat.com>
>---
> 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
>

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Stefano Garzarella]

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 <eperezma@redhat.com>
>---
> 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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Stefano Garzarella]

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 <eperezma@redhat.com>
>---
> 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Stefano Garzarella]

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 <eperezma@redhat.com>
>---
> 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

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Jason Wang]

On Fri, Jun 24, 2022 at 12:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> This operation is optional: It it's not implemented, backend feature bit
> will not be exposed.

A question, do we allow suspending a device without DRIVER_OK?

Thanks

>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  include/linux/vdpa.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 7b4a13d3bd91..d282f464d2f1 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -218,6 +218,9 @@ struct vdpa_map_file {
>   * @reset:                     Reset device
>   *                             @vdev: vdpa device
>   *                             Returns integer: success (0) or error (< 0)
> + * @suspend:                   Suspend or resume the device (optional)
> + *                             @vdev: vdpa device
> + *                             Returns integer: success (0) or error (< 0)
>   * @get_config_size:           Get the size of the configuration space includes
>   *                             fields that are conditional on feature bits.
>   *                             @vdev: vdpa device
> @@ -319,6 +322,7 @@ struct vdpa_config_ops {
>         u8 (*get_status)(struct vdpa_device *vdev);
>         void (*set_status)(struct vdpa_device *vdev, u8 status);
>         int (*reset)(struct vdpa_device *vdev);
> +       int (*suspend)(struct vdpa_device *vdev);
>         size_t (*get_config_size)(struct vdpa_device *vdev);
>         void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
>                            void *buf, unsigned int len);
> --
> 2.31.1
>

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Jason Wang]

On Fri, Jun 24, 2022 at 12:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> This operation is optional: It it's not implemented, backend feature bit
> will not be exposed.

A question, do we allow suspending a device without DRIVER_OK?

Thanks

>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  include/linux/vdpa.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 7b4a13d3bd91..d282f464d2f1 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -218,6 +218,9 @@ struct vdpa_map_file {
>   * @reset:                     Reset device
>   *                             @vdev: vdpa device
>   *                             Returns integer: success (0) or error (< 0)
> + * @suspend:                   Suspend or resume the device (optional)
> + *                             @vdev: vdpa device
> + *                             Returns integer: success (0) or error (< 0)
>   * @get_config_size:           Get the size of the configuration space includes
>   *                             fields that are conditional on feature bits.
>   *                             @vdev: vdpa device
> @@ -319,6 +322,7 @@ struct vdpa_config_ops {
>         u8 (*get_status)(struct vdpa_device *vdev);
>         void (*set_status)(struct vdpa_device *vdev, u8 status);
>         int (*reset)(struct vdpa_device *vdev);
> +       int (*suspend)(struct vdpa_device *vdev);
>         size_t (*get_config_size)(struct vdpa_device *vdev);
>         void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
>                            void *buf, unsigned int len);
> --
> 2.31.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Jason Wang]

On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> ---
>  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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Jason Wang]

On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> ---
>  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
>

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Jason Wang]

On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> ---
>  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
>

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Jason Wang]

On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> ---
>  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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 4/4] vdpa_sim: Implement suspend vdpa op

[Jason Wang]

On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <sgarzare@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  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
>

Re: [PATCH v6 4/4] vdpa_sim: Implement suspend vdpa op

[Jason Wang]

On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <sgarzare@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Eugenio Perez Martin]

On Tue, Jun 28, 2022 at 3:40 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Jun 23, 2022 at 06:07:35PM +0200, Eugenio Pérez wrote:
> >This operation is optional: It it's not implemented, backend feature bit
> >will not be exposed.
> >
> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> > include/linux/vdpa.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >index 7b4a13d3bd91..d282f464d2f1 100644
> >--- a/include/linux/vdpa.h
> >+++ b/include/linux/vdpa.h
> >@@ -218,6 +218,9 @@ struct vdpa_map_file {
> >  * @reset:                    Reset device
> >  *                            @vdev: vdpa device
> >  *                            Returns integer: success (0) or error (< 0)
> >+ * @suspend:                  Suspend or resume the device (optional)
>                                             ^
> IIUC we removed the resume operation (that should be done with reset),
> so should we update this documentation?
>

Totally, I forgot to update the doc. I'll send a new version with that.

Thanks!


> Thanks,
> Stefano
>
> >+ *                            @vdev: vdpa device
> >+ *                            Returns integer: success (0) or error (< 0)
> >  * @get_config_size:          Get the size of the configuration space includes
> >  *                            fields that are conditional on feature bits.
> >  *                            @vdev: vdpa device
> >@@ -319,6 +322,7 @@ struct vdpa_config_ops {
> >       u8 (*get_status)(struct vdpa_device *vdev);
> >       void (*set_status)(struct vdpa_device *vdev, u8 status);
> >       int (*reset)(struct vdpa_device *vdev);
> >+      int (*suspend)(struct vdpa_device *vdev);
> >       size_t (*get_config_size)(struct vdpa_device *vdev);
> >       void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
> >                          void *buf, unsigned int len);
> >--
> >2.31.1
> >
>

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Eugenio Perez Martin]

On Wed, Jun 29, 2022 at 6:10 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jun 24, 2022 at 12:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > This operation is optional: It it's not implemented, backend feature bit
> > will not be exposed.
>
> A question, do we allow suspending a device without DRIVER_OK?
>

That should be invalid. In particular, vdpa_sim will resume in that
case, but I guess it would depend on the device.

Do you think it should be controlled in the vdpa frontend code?

Thanks!

> Thanks
>
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  include/linux/vdpa.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index 7b4a13d3bd91..d282f464d2f1 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -218,6 +218,9 @@ struct vdpa_map_file {
> >   * @reset:                     Reset device
> >   *                             @vdev: vdpa device
> >   *                             Returns integer: success (0) or error (< 0)
> > + * @suspend:                   Suspend or resume the device (optional)
> > + *                             @vdev: vdpa device
> > + *                             Returns integer: success (0) or error (< 0)
> >   * @get_config_size:           Get the size of the configuration space includes
> >   *                             fields that are conditional on feature bits.
> >   *                             @vdev: vdpa device
> > @@ -319,6 +322,7 @@ struct vdpa_config_ops {
> >         u8 (*get_status)(struct vdpa_device *vdev);
> >         void (*set_status)(struct vdpa_device *vdev, u8 status);
> >         int (*reset)(struct vdpa_device *vdev);
> > +       int (*suspend)(struct vdpa_device *vdev);
> >         size_t (*get_config_size)(struct vdpa_device *vdev);
> >         void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
> >                            void *buf, unsigned int len);
> > --
> > 2.31.1
> >
>

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Eugenio Perez Martin]

On Tue, Jun 28, 2022 at 3:43 PM Stefano Garzarella <sgarzare@redhat.com> 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 <eperezma@redhat.com>
> >---
> > 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
> >
>

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Eugenio Perez Martin]

On Wed, Jun 29, 2022 at 6:12 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> > ---
> >  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
> >
>

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Eugenio Perez Martin]

On Tue, Jun 28, 2022 at 3:45 PM Stefano Garzarella <sgarzare@redhat.com> 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 <eperezma@redhat.com>
> >---
> > 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
>

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Eugenio Perez Martin]

On Wed, Jun 29, 2022 at 6:16 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> > ---
> >  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
> >
>

Re: [PATCH v6 4/4] vdpa_sim: Implement suspend vdpa op

[Eugenio Perez Martin]

On Wed, Jun 29, 2022 at 6:18 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <sgarzare@redhat.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  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
> >
>

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Stefano Garzarella]

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 <sgarzare@redhat.com> 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

Re: [PATCH v6 2/4] vhost-vdpa: introduce SUSPEND backend feature bit

[Stefano Garzarella]

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 <sgarzare@redhat.com> 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Jason Wang]

On Fri, Jul 8, 2022 at 7:31 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Jun 29, 2022 at 6:10 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Jun 24, 2022 at 12:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > This operation is optional: It it's not implemented, backend feature bit
> > > will not be exposed.
> >
> > A question, do we allow suspending a device without DRIVER_OK?
> >
>
> That should be invalid. In particular, vdpa_sim will resume in that
> case, but I guess it would depend on the device.

Yes, and that will match our virtio spec patch (STOP bit).

>
> Do you think it should be controlled in the vdpa frontend code?

The vdpa bus should validate this at least.

Thanks

>
> Thanks!
>
> > Thanks
> >
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  include/linux/vdpa.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > > index 7b4a13d3bd91..d282f464d2f1 100644
> > > --- a/include/linux/vdpa.h
> > > +++ b/include/linux/vdpa.h
> > > @@ -218,6 +218,9 @@ struct vdpa_map_file {
> > >   * @reset:                     Reset device
> > >   *                             @vdev: vdpa device
> > >   *                             Returns integer: success (0) or error (< 0)
> > > + * @suspend:                   Suspend or resume the device (optional)
> > > + *                             @vdev: vdpa device
> > > + *                             Returns integer: success (0) or error (< 0)
> > >   * @get_config_size:           Get the size of the configuration space includes
> > >   *                             fields that are conditional on feature bits.
> > >   *                             @vdev: vdpa device
> > > @@ -319,6 +322,7 @@ struct vdpa_config_ops {
> > >         u8 (*get_status)(struct vdpa_device *vdev);
> > >         void (*set_status)(struct vdpa_device *vdev, u8 status);
> > >         int (*reset)(struct vdpa_device *vdev);
> > > +       int (*suspend)(struct vdpa_device *vdev);
> > >         size_t (*get_config_size)(struct vdpa_device *vdev);
> > >         void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
> > >                            void *buf, unsigned int len);
> > > --
> > > 2.31.1
> > >
> >
>

Re: [PATCH v6 1/4] vdpa: Add suspend operation

[Jason Wang]

On Fri, Jul 8, 2022 at 7:31 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Jun 29, 2022 at 6:10 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Jun 24, 2022 at 12:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > This operation is optional: It it's not implemented, backend feature bit
> > > will not be exposed.
> >
> > A question, do we allow suspending a device without DRIVER_OK?
> >
>
> That should be invalid. In particular, vdpa_sim will resume in that
> case, but I guess it would depend on the device.

Yes, and that will match our virtio spec patch (STOP bit).

>
> Do you think it should be controlled in the vdpa frontend code?

The vdpa bus should validate this at least.

Thanks

>
> Thanks!
>
> > Thanks
> >
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  include/linux/vdpa.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > > index 7b4a13d3bd91..d282f464d2f1 100644
> > > --- a/include/linux/vdpa.h
> > > +++ b/include/linux/vdpa.h
> > > @@ -218,6 +218,9 @@ struct vdpa_map_file {
> > >   * @reset:                     Reset device
> > >   *                             @vdev: vdpa device
> > >   *                             Returns integer: success (0) or error (< 0)
> > > + * @suspend:                   Suspend or resume the device (optional)
> > > + *                             @vdev: vdpa device
> > > + *                             Returns integer: success (0) or error (< 0)
> > >   * @get_config_size:           Get the size of the configuration space includes
> > >   *                             fields that are conditional on feature bits.
> > >   *                             @vdev: vdpa device
> > > @@ -319,6 +322,7 @@ struct vdpa_config_ops {
> > >         u8 (*get_status)(struct vdpa_device *vdev);
> > >         void (*set_status)(struct vdpa_device *vdev, u8 status);
> > >         int (*reset)(struct vdpa_device *vdev);
> > > +       int (*suspend)(struct vdpa_device *vdev);
> > >         size_t (*get_config_size)(struct vdpa_device *vdev);
> > >         void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
> > >                            void *buf, unsigned int len);
> > > --
> > > 2.31.1
> > >
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Jason Wang]

On Fri, Jul 8, 2022 at 7:53 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Jun 29, 2022 at 6:16 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> > > ---
> > >  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
> > >
> >
>

Re: [PATCH v6 3/4] vhost-vdpa: uAPI to suspend the device

[Jason Wang]

On Fri, Jul 8, 2022 at 7:53 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Jun 29, 2022 at 6:16 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Jun 24, 2022 at 12:08 AM Eugenio Pérez <eperezma@redhat.com> 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 <eperezma@redhat.com>
> > > ---
> > >  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
> > >
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization